Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
>> + if (len && fixup_message[len] == ':') {
>> + fixup_message[len++] = '\0';
>> + fixup_commit = fixup_message + len;
>> + if (starts_with("amend", fixup_message))
>> + fixup_prefix = "amend";
>> + else
>> + die(_("unknown option: --fixup=%s:%s"), fixup_message, fixup_commit);
>
> I haven't read ahead in the series yet, but I presume you're making
> this code extra generic because you plan to support additional `fixup`
> options (such as `reword`), but I wonder if the cognitive overhead is
> warranted or you could get by with something simpler, such as:
>
> if (skip_prefix(msg, "amend:", &arg) ||
> skip_prefix(msg, "reword:", &arg)) {
> ...
> }
You still need to compute "len" because you'd want to tell between
--fixup="HEAD^{/^area: string}" and --fixup=bogus:HEAD (the latter
would want to say "no such variant 'bogus' for --fixup", but the
colon in the former is not the end of the name of variant.
So, skip_prefix() would not buy us much, I guess.
But the use of starts_with() in the original patch is bogus, I
think. fixup_message[] by the time the comparison is made is
NULL terminated at where the colon was originally, so we should be
doing !strcmp() to reject "--fixup=amendo:HEAD~2" with "no, 'amendo'
is not a valid variant name for --fixup option".
> Also, am I misreading when I think that the use of starts_with() could
> be replaced with a simple strcmp() since you've already inserted a
> '\0' immediately after the final alphabetic character?
Correct.
