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

[Charvi Mendiratta <charvi077@gmail.com>]

On Tue, 2 Mar 2021 at 04:02, Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> 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.
>

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 ?

> > 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() ?