Re: [PATCH v5 3/4] status: give more information during rebase -i

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2015, 16:18 UTC
Message-ID: <xmqq4mlnc0lr.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1435739433-18853-3-git-send-email-Matthieu.Moy@imag.fr>

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

Show 30 quoted lines

> +/*
> + * Turn
> + * pick d6a2f0303e897ec257dd0e0a39a5ccb709bc2047 some message
> + * into
> + * pick d6a2f03 some message
> + */
> +static void abbrev_sha1_in_line(struct strbuf *line)
> +{
> +	struct strbuf **split;
> +	int i;
> +
> +	if (starts_with(line->buf, "exec ") ||
> +	    starts_with(line->buf, "x "))
> +		return;
> +
> +	split = strbuf_split_max(line, ' ', 3);
> +	if (split[0] && split[1]) {
> +		unsigned char sha1[20];
> +		const char *abbrev;
> +
> +		/*
> +		 * strbuf_split_max left a space. Trim it and re-add
> +		 * it after abbreviation.
> +		 */
> +		strbuf_trim(split[1]);
> +		if (!get_sha1(split[1]->buf, sha1)) {
> +			abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
> +			strbuf_reset(split[1]);
> +			strbuf_addf(split[1], "%s ", abbrev);
> +		}

... else?

That is, "we thought there would be a full SHA-1, but it turns out that there wasn't, so we keep split[1] as-is" would need to add the space back, no? Perhaps be more strict and do this instead (without leading strbuf_trim):

	if (!get_sha1_hex(split[1]->buf, sha1) &&
	    !strcmp(split[1]->buf + 40, " ") {
		replace split[1] with "%s " abbrev
	}

Show 18 quoted lines

> +		strbuf_reset(line);
> +		for (i = 0; split[i]; i++)
> +			strbuf_addstr(line, split[i]->buf);
> +	}
> +	for (i = 0; split[i]; i++)
> +		strbuf_release(split[i]);
> +
> +}
> +
> +static void read_rebase_todolist(const char *fname, struct string_list *lines)
> +{
> +	struct strbuf line = STRBUF_INIT;
> +	FILE *f = fopen(git_path(fname), "r");
> +
> +	if (!f)
> +		die_errno("Could not open file %s for reading", git_path(fname));
> +	while (!strbuf_getline(&line, f, '\n')) {
> +		stripspace(&line, 1);

stripspace() is meant to be used for multi-line input (e.g. it collapses a multi-line paragraph break into one blank line) and it does not look a good fit in a loop that goes line-by-line. As you call (and you have to, because stripspace() fixes the incomplete line by adding LF at the end) rtrim() immediately afterward, this call is done only for removing comments (in other words, trailing whitespaces are removed without the call to stripspace() anyway).

Show 7 quoted lines

> +		/* Remove trailing \n */
> +		strbuf_rtrim(&line);
> +		abbrev_sha1_in_line(&line);
> +		string_list_append(lines, line.buf);
> +	}
> +	string_list_remove_empty_items(lines, 1);
> +}

Perhaps

        while (!strbuf_getline(&line, f, '\n')) {
                if (line.len && line.len[0] == comment_line_char)
                        continue;
                strbuf_rtrim(&line);
                if (!line.len)
                        continue;
                abbrev_sha1_in_line(&line);
                string_list_append(lines, line.buf);
        }

without the "we may have added cruft, so discard them at the end"?

Other than these two minor nits, I didn't spot anything questionable in the diff between this and the previous round.

Thanks.