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

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

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

> +static void abbrev_oid_in_line(struct repository *r,
> +			       struct strbuf *line, char **pp)
> +{
> ...
> +	have_oid = !repo_get_oid(r, p, &oid);
> +	*end_of_object_name = saved;
> +	if (!have_oid)
> +		goto out; /* object name was a label */

Can there be a label "deadbeef123" that is unrelated to an object whose
object name happens to abbreviate to "deadbeef123"?

> +	case TODO_MERGE:
> +		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, line, &p);
> +		}
> +		break;

What does this loop do?  A "merge" command may look like "merge
[[-C|-c] <commit>] <label>", and we give each whitespace-separated
token to abbrev_oid_in_line()?  Would "<label>" that is ambiguous
cause an issue?  You may want to limit the scope of what the loop
does a bit, e.g., massage only the token after -C/-c, or something?

> +	case TODO_FIXUP:
> +		skip_dash_c(&p);
> +		/* fallthrough */
> +	case TODO_DROP:
> +	case TODO_EDIT:
> +	case TODO_PICK:
> +	case TODO_RESET:

Doesn't RESET also take a <label>?  And if it happens to be the same
as an abbreviated object name, e.g., "deadbeef123", of an unrelated
object, would wt-status say "reset deadbeef1", causing a mismatch?
If this is indeed an issue, would moving this to the "no-op" section
below, next to TODO_LABEL, solve it?

> +	case TODO_REVERT:
> +	case TODO_REWORD:
> +	case TODO_SQUASH:
> +		abbrev_oid_in_line(r, line, &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;
>  	}
> -	string_list_clear(&split, 0);
> +
> +	return true;
>  }