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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Junio

On 31/05/2026 01:46, Junio C Hamano wrote:
> 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"?

In theory yes, but I had assumed it was so unlikely to happen that we 
could ignore it. If we want to be more careful then we could add a "bool 
maybe_label" argument for commands that accept a label or a revision and 
check if "refs/rewritten/$object_name" exists before trying repo_get_oid().

>> +	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?

The parents can be a label or any revision so we want to abbreviate the 
parent if it is a hex object id. The same is true for "reset" below.

Thanks

Phillip

> 
>> +	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;
>>   }
>