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

[Junio C Hamano <gitster@pobox.com>]

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

> +/*
> + * 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
	}

> +		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).

> +		/* 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.