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