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

On Tue, 2 Mar 2021 at 04:02, Junio C Hamano [off-list ref] wrote:

Eric Sunshine [off-list ref] writes:

quoted

quoted

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

Yes, I also agree.

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

I am not sure about this because we used the starts_with() so that it can
support the _any_ prefix of `amend` or `reword` i.e to make all below
like combinations possible :
--fixup=a:HEAD~2
--fixup=am:HEAD~2

So, I am not sure if we need to replace it with !strcmp and work for
the specified prefix only ?

quoted

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.

Same reason for using the starts_with() applies for this part also
after inserting the '\0'.  So, I think we can keep starts_with() ?