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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Mar 3, 2021 at 2:41 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
> On Tue, 2 Mar 2021 at 04:02, Junio C Hamano <gitster@pobox.com> wrote:
> > 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 ?

Hmm, I see. I didn't follow whatever discussion led to the decision to
use this sort of prefix matching, but I have to wonder if it is a good
idea. Was the idea that it behave similarly to sequencer commands in
`git rebase --interactive` which are often abbreviated to a single
letter? I personally would feel much more comfortable requiring a
full-word match for `amend` and `reword` at initial implementation.
That leaves the door open to later loosening it to do prefix-matching
if enough people request such a feature, whereas starting with
prefix-matching closes that door since we can never later tighten it
to require full words.

Anyhow, if the decision is to keep this behavior, then it almost
certainly deserves an in-code comment explaining the sort of
prefix-matching it's doing since it's otherwise too easy for readers
to be fooled as Junio and I were by not noticing that you had reversed
the arguments to starts_with().