Re: [PATCH v3 0/3] sequencer: comment out properly in todo list

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Nov 25, 2024, 14:36 UTC
Message-ID: <62966a75-45fb-48ca-9ed2-6ff683de09c4@gmail.com>
In-Reply-To: <3bd7cd08-61b6-4b57-a293-1c94eb3529d7@app.fastmail.com>

Hi Kristoffer

On 25/11/2024 10:52, Kristoffer Haugsbakk wrote:

> On Mon, Nov 25, 2024, at 11:07, phillip.wood123@gmail.com wrote:
> 
> Hi Phillip, thanks for the review!

You're welcome, thanks for fixing this

Show 19 quoted lines

>>>       +    that hash3’s commit message should be used for the resulting commit.
>>>       +    So the user is presented with an editor where the two first commit
>>>       +    messages are commented out and the third is not.
>>
>> I'd perhaps say
>>
>>      If there are conflicts when applying commit hash3 then the user is
>>      presented ...
>>
>> as we only show all the messages to the user when there are conflicts.
> 
> I use `fixup -c` for the third/last commit here.  So I am prompted to
> edit the commit message. I tested this on this series:
> 
>      git checkout --detach kh/sequencer-comment-char
>      git rebase -i fd3785337beb285ed7fd67ce6fc3d3bed2097b40
> 
> Which gave me [this] editor without these changes (with
> `core.commentChar` set to `%`).

Oh, I see the same thing, I was sure we only showed the final message unless there were conflicts. I wonder if I've misremembered or the behavior has changed in any case that's outside the scope of this series.

Thanks

Phillip

Show 88 quoted lines

>>
>>> However this does
>>>       +    not work if `core.commentChar`/`core.commentString` is in use since
>>>       +    the comment char is hardcoded (#) in this `sequencer.c` function.
>>>       +    As a result the first commit message will not be commented out.
>>>       +
>>>       +    † 1: See 9e3cebd97cb (rebase -i: add fixup [-C | -c] command,
>>>       +        2021-01-29)
>>>       +
>>>       +    Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>            Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>       +    Reported-by: Taylor Blau <me@ttaylorr.com>
>>>            Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>
>> Thanks for updating the trailers, they look good to me
>>
>> Best Wishes
>>
>> Phillip
> 
> † this:
> 
>      % This is a combination of 3 commits.
>      % This is the 1st commit message:
> 
>      sequencer: comment checked-out branch properly
> 
>      `git rebase --update-ref` does not insert commands for dependent/sub-
>      branches which are checked out.[1]  Instead it leaves a comment about
>      that fact.  The comment char is hardcoded (#).  In turn the comment
>      line gets interpreted as an invalid command when `core.commentChar`/
>      `core.commentString` is in use.
> 
>      † 1: See 900b50c242 (rebase: add --update-refs option, 2022-07-19)
> 
>      Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> 
>      % The commit message #2 will be skipped:
> 
>      % sequencer: comment `--reference` subject line properly
>      %
>      % `git revert --reference <commit>` leaves behind a comment in the
>      % first line:[1]
>      %
>      %     # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***
>      %
>      % Meaning that the commit will just consist of the next line if the user
>      % exits the editor directly:
>      %
>      %     This reverts commit <--format=reference commit>
>      %
>      % But the comment char here is hardcoded (#).  Which means that the
>      % comment line will inadvertently be included in the commit message if
>      % `core.commentChar`/`core.commentString` is in use.
>      %
>      % † 1: See 43966ab3156 (revert: optionally refer to commit in the
>      %     "reference" format, 2022-05-26)
>      %
>      % Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> 
>      % This is the commit message #3:
> 
>      sequencer: comment commit messages properly
> 
>      The rebase todo editor has commands like `fixup -c` which affects
>      the commit messages of the rebased commits.[1]  For example:
> 
>          pick hash1 <msg>
>          fixup hash2 <msg>
>          fixup -c hash3 <msg>
> 
>      This says that hash2` and hash3 should be squashed into hash1 and
>      that hash3’s commit message should be used for the resulting commit.
>      So the user is presented with an editor where the two first commit
>      messages are commented out and the third is not.  However this does
>      not work if `core.commentChar`/`core.commentString` is in use since
>      the comment char is hardcoded (#) in this `sequencer.c` function.
>      As a result the first commit message will not be commented out.
> 
>      † 1: See 9e3cebd97cb (rebase -i: add fixup [-C | -c] command,
>          2021-01-29)
> 
>      Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>      Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>      Reported-by: Taylor Blau <me@ttaylorr.com>
>      Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> 
>      % Please enter the commit message for your changes. Lines starting