Re: [PATCH v3 2/2] status: improve rebase todo list parsing

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

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When there is rebase in progress "git status" displays the last couple
> of completed and the next couple of pending commands from the todo
> list. When it does this it tries to abbreviate the object ids of
> the commits to be picked. Unfortunately it does not abbreviate the
> object ids when the line starts with "fixup -C" or "merge -C". It
> also mistakenly replaces the refname in "reset main" and "update-ref
> refs/heads/main" with the object id that the ref points to.
>
> Fix this by using the function added in the last commit to parse the
> command name and only try to abbreviate the argument for commands that
> take an object id. If a command accepts a label then try to resolve the
> object name as a label first and only if that fails try to resolve it
> as an object_id. When trying to abbreviate an object id, only replace
> the object name if it starts with the abbreviated object id so that
> tag or branch names that contain only hex digits are left unchanged.

;-)  

Strictly speaking, the original that said "if begins with
exed, x, label, or l, then don't bother" style can be extended
without using the function added in the last commit to do this, but
it certainly is much more pleasant to read the resulting code
presented here that uses parsed command enum and switches on it.

> Comments are now processed after stripping any leading
> whitespace from the line. This matches what the sequencer does in
> parse_insn_line(). The existing test cases are updated to test a
> wider variety of commands. Only the pending commands in the tests
> are changed to avoid removing existing coverage.
>
> Helped-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> diff --git a/wt-status.c b/wt-status.c
> index 479ccc3304b..4b15bda76f4 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1363,6 +1363,71 @@ static int split_commit_in_progress(struct wt_status *s)
>  	free(rebase_orig_head);
>  
>  	return split_in_progress;
> +}
> +
> +/*
> + * If the whitespace-delimited token starting at or just after *pp
> + * is a hex object id that is longer than its default abbreviation,
> + * abbreviate it in-place, shrinking `line` accordingly. On return
> + * *pp points one past the (possibly abbreviated) token. Leaves both
> + * `line` and *pp-advanced-past-the-token unchanged in all other cases
> + * (non-hex token, label name, unresolvable, or a refname that happens
> + * to consist only of hex digits).
> + */
> +static void abbrev_oid_in_line(struct repository *r, struct strbuf *scratch,
> +			       struct strbuf *line, bool maybe_label, char **pp)
> +{
> +	char *p = *pp;
> +	char *end_of_object_name, saved;
> +	const char *abbrev;
> +	struct object_id oid;
> +	bool have_oid;
> +
> +	p += strspn(p, " \t");
> +	end_of_object_name = p + strcspn(p, " \t");
> +	/*
> +	 * For "merge" and "reset" the object name may be a label or
> +	 * ref rather than a hex object id. Only abbreviate the object
> +	 * name if it is a hex object id.
> +	 */
> +	for (const char *q = p; q < end_of_object_name; q++) {
> +		if (!isxdigit(*q))
> +			goto out;
> +	}

OK.  If the string has non hexdigit, it cannot be a raw object name
so there is no point in rewriting.  OK.

> +	if (maybe_label) {
> +		strbuf_reset(scratch);
> +		strbuf_addf(scratch, "refs/rewritten/%.*s",
> +			    (int)(end_of_object_name - p), p);
> +		if (refs_ref_exists(get_main_ref_store(r), scratch->buf))
> +			goto out; /* object name was a label */
> +	}

If it could be a label, then we check if such a label exists, and if
so, we won't modify it.  OK.

> +	saved = *end_of_object_name;
> +	*end_of_object_name = '\0';
> +	have_oid = !repo_get_oid(r, p, &oid);
> +	*end_of_object_name = saved;
> +	if (!have_oid)
> +		goto out; /* invalid object name */

We obviously cannot abbreviate if we cannot even recognize it as an
object name.  OK.

> +	abbrev = repo_find_unique_abbrev(r, &oid, DEFAULT_ABBREV);
> +	if (!starts_with(p, abbrev))
> +		goto out; /* object name was a refname containing only xdigits */

OK, nice to see sufficient paranoia ;-)

> +	p += strlen(abbrev);
> +	strbuf_remove(line, p - line->buf, end_of_object_name - p);
> +	end_of_object_name = p;

By abbreviating, line->buf only shrinks so we won't risk getting
confused by a realloc() happening under the hood.  Upon entry to
this helper, *pp must be pointing into line->buf, or everything will
go awry but for a file-scope static helper function like this, it
probably is too obvious to anybody that it does not have to be
spelled out.  OK.

> +out:
> +	*pp = end_of_object_name;
> +}


> +/* Skip "[ \t]*(-[cC])?", returns true if "-c/-C" was skipped. */
> +static bool skip_dash_c(char **pp)
> +{
> +	bool ret;
> +	char *p = *pp;
> +
> +	p += strspn(p, " \t");
> +	ret = skip_prefix(p, "-C", &p) || skip_prefix(p, "-c", &p);
> +	*pp = p;
> +
> +	return ret;
>  }

OK.

> @@ -1371,29 +1436,68 @@ static int split_commit_in_progress(struct wt_status *s)
>   * into
>   * "pick d6a2f03 some message"
>   *
> - * The function assumes that the line does not contain useless spaces
> - * before or after the command.
> + * Returns false on comment lines, true otherwise
>   */
> -static void abbrev_oid_in_line(struct repository *r, struct strbuf *line)
> +static bool format_todo_line(struct repository *r, struct strbuf *line)
>  {
> -	struct string_list split = STRING_LIST_INIT_DUP;
> -	struct object_id oid;
> -
> -	if (starts_with(line->buf, "exec ") ||
> -	    starts_with(line->buf, "x ") ||
> -	    starts_with(line->buf, "label ") ||
> -	    starts_with(line->buf, "l "))
> -		return;
> -
> -	if ((2 <= string_list_split(&split, line->buf, " ", 2)) &&
> -	    !repo_get_oid(r, split.items[1].string, &oid)) {
> -		strbuf_reset(line);
> -		strbuf_addf(line, "%s ", split.items[0].string);
> -		strbuf_add_unique_abbrev(line, &oid, DEFAULT_ABBREV);
> -		for (size_t i = 2; i < split.nr; i++)
> -			strbuf_addf(line, " %s", split.items[i].string);
> -	}
> -	string_list_clear(&split, 0);

We essentially said, "do not molest exec and label, but everything
else, as long as there are two (or more) tokens and the second token
looks like an object name, replace it with its abbreviation",
regardless of what the actual command was.  Now we do the right
thing by ...

> +	enum todo_command cmd;
> +	struct strbuf scratch = STRBUF_INIT;
> +	char *p = line->buf;
> +
> +	if (!sequencer_parse_todo_command((const char**)&p, &cmd))
> +		return true; /* keep invalid lines */

... parsing out what the line is about, and ...

> +	switch (cmd) {

... switching on it, to make it clear that we cover all the cases
known to us (and the code will be maintained like so, by not having
a "default" arm).

> +	case TODO_COMMENT:
> +		return false;
> +
> +	case TODO_MERGE: {
> +		/*
> +		 * The argument to -C cannot be a label, but the parents
> +		 * can be labels.
> +		 */
> +		bool maybe_label = !skip_dash_c(&p);
> +
> +		while (true) {
> +			p += strspn(p, " \t");
> +			if (!p[0] || (p[0] == '#' && (!p[1] || isspace(p[1]))))
> +				break;
> +			abbrev_oid_in_line(r, &scratch, line, maybe_label, &p);
> +			maybe_label = true;
> +		}
> +		break;
> +	}
> +
> +	case TODO_FIXUP:
> +		skip_dash_c(&p);
> +		/* fallthrough */

Fixup always refers to raw object ID and never a label, so it
would be OK to just skip -c/-C here ...

> +	case TODO_DROP:
> +	case TODO_EDIT:
> +	case TODO_PICK:
> +	case TODO_REVERT:
> +	case TODO_REWORD:
> +	case TODO_SQUASH:

... and pass "false" for "maybe_label".  OK.

> +		abbrev_oid_in_line(r, &scratch, line, false, &p);
> +		break;
> +
> +	case TODO_RESET:
> +		abbrev_oid_in_line(r, &scratch, line, true, &p);
> +		break;
> +	/*
> +	 * Avoid "default" and instead list all the other commands so
> +	 * that -Wswitch (which is included in -Wall) warns if a new
> +	 * command is added without handling it in this function.
> +	 */
> +	case TODO_BREAK:
> +	case TODO_EXEC:
> +	case TODO_LABEL:
> +	case TODO_NOOP:
> +	case TODO_UPDATE_REF:
> +		break;
> +	}
> +
> +	strbuf_release(&scratch);
> +	return true;
>  }
>  
>  static int read_rebase_todolist(struct repository *r, const char *fname, struct string_list *lines)
> @@ -1411,13 +1515,9 @@ static int read_rebase_todolist(struct repository *r, const char *fname, struct
>  			  repo_git_path_replace(r, &buf, "%s", fname));
>  	}
>  	while (!strbuf_getline_lf(&buf, f)) {
> -		if (starts_with(buf.buf, comment_line_str))
> -			continue;
>  		strbuf_trim(&buf);
> -		if (!buf.len)
> -			continue;
> -		abbrev_oid_in_line(r, &buf);
> -		string_list_append(lines, buf.buf);
> +		if (format_todo_line(r, &buf))
> +			string_list_append(lines, buf.buf);
>  	}
>  	fclose(f);

This loop got much nicer than the original.

Looks good.

Thanks.