On Tue, 2 Mar 2021 at 04:02, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> 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.
>
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() ?
