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

Charvi Mendiratta [off-list ref] writes:

On Thu, 11 Mar 2021 at 06:01, Junio C Hamano [off-list ref] wrote:

quoted

Charvi Mendiratta [off-list ref] writes:

quoted

               * 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.

On Thu, Mar 11, 2021 at 12:37 AM Junio C Hamano [off-list ref] wrote:

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>.

I suggested a different way to rewrite this comment[1] in my review of
patch [2/6] before I was aware that you two were discussing rewrites
here in [3/6].

[1]: https://lore.kernel.org/git/CAPig+cRxZFV7DDtnYOxRqEYU4PwXK8gb2HSehDyGorqdgsmoNQ@mail.gmail.com/

On Thu, 11 Mar 2021 at 11:07, Junio C Hamano [off-list ref] wrote:

[...]

quoted

- 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`..."

Oops, I must have written a complete comment. I will take care of it.

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.

Okay, I will replace it.

Thanks for helping out and guiding.

Thanks and Regards,
Charvi