Charvi Mendiratta <charvi077@xxxxxxxxx> writes:
> On Thu, 11 Mar 2021 at 06:01, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>
>> Charvi Mendiratta <charvi077@xxxxxxxxx> writes:
>>
>> > * reference for example: --fixup="HEAD^{/^area: string}" or
>> > * a suboption of `--fixup`.
>> > *
>> > - * As `amend` suboption contains only alpha character.
>> > - * So check if first non alpha character in fixup_message
>> > - * is ':'.
>> > + * As `amend`/`reword` suboptions contains only alpha
>> > + * characters. So check if first non alpha character
>> > + * in fixup_message is ':'.
>>
>> Sorry, but I cannot quite follow the logic.
>>
>> We limit --fixup's suboptions to only alpha characters. If
>> the first character after a len of alpha is colon, then the
>> part before the colon may be a known suboption name like
>> `amend` or `reword`, or a misspelt suboption name.
>>
>> Otherwise, we are dealing with --fixup=<commit> that happens
>> to have a colon in <commit> object name.
>>
>> perhaps?
>
> Yes, Agree. Here I just intend to mention the special case
> "--fixup=HEAD^{/^area: string}" because of which we chose the method
> to check if first non alpha char is ':' instead of directly checking
> the suboption like (skip_prefix(msg, "amend:", &arg). So maybe we can
> reword it like
>
> - To check if fixup_message that contains ':' is a commit
> - reference for example: --fixup="HEAD^{/^area: string}" or
> - a suboption of `--fixup`.
> + fixup_message could be a commit reference for example:
> + --fixup="HEAD^{/^area:string}" or a suboption of `--fixup`.
> +
> + As `amend` ...
My suggestion primarily started a reaction to that "As `amend`..."
which was not gramatically complete sentence, and I ended up
rewriting everything after "As `amend`..."
But re-reading what is in the paragraph before, I tend to think that
it places too much stress on 'colon' and should be removed.
The comment is about what is being parsed, so
We limit --fixup's suboptions to only alpha characters. If
the first character after a run of alpha is colon, then the
part before the colon may be a known suboption name like
`amend` or `reword`, or a misspelt suboption name. In
either case, we treat it as --fixup=<suboption>:<arg>
Otherwise, we are dealing with --fixup=<commit>.
would be good. The code, when it decides it is not in the
--fixup=<suboption>:<arg> form but it is --fixup=<commit>, does not
even care about a colon, so there is no need to mention colon in the
"Otherwise" part.
