Re: [PATCH v3 2/6] commit: add amend suboption to --fixup to create amend! commit

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 1, 2021, 22:32 UTC
Message-ID: <xmqqlfb6mrnx.fsf@gitster.c.googlers.com>
In-Reply-To: <CAPig+cQAG3p6rhGHzBNGPUxMXQbbLZgiw-uUpizNiEWY9wm-3A@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 17 quoted lines

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