Re: [PATCH v4 3/6] commit: add a reword suboption to --fixup

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 11, 2021, 05:37 UTC
Message-ID: <xmqqa6rafdyg.fsf@gitster.g>
In-Reply-To: <CAPSFM5dqRbuoCvEFcHQPSQt+EK1_pBVQ--f42jR+GNqCFd_SOg@mail.gmail.com>

Charvi Mendiratta <charvi077@gmail.com> writes:

Show 39 quoted lines

> On Thu, 11 Mar 2021 at 06:01, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Charvi Mendiratta <charvi077@gmail.com> 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.