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

[phillip.wood123@gmail.com]

Hi Kristoffer

Thanks for re-rolling, I've left some comments on the range-diff

On 24/11/2024 20:56, kristofferhaugsbakk@fastmail.com wrote:
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
> 
> Range-diff against v2:
> 1:  fc3b4438845 ! 1:  a46767263f6 sequencer: comment checked-out branch properly
 > [...]
>      @@ t/t3400-rebase.sh: test_expect_success 'rebase when inside worktree subdirectory
>       +	git checkout base &&
>       +	test_commit msg3 &&
>       +	git checkout topic2 &&
>      -+	git -c core.commentChar=% rebase --update-refs base
>      ++	GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \
>      ++		 rebase -i --update-refs base &&
>      ++	grep "% Ref refs/heads/wt-topic checked out at" actual &&
>      ++	grep "% Ref refs/heads/topic2 checked out at" actual

It would be nicer to use test_grep here as it prints a helpful message 
when the pattern is not found which aids debugging test failures

> 2:  710c5b1a3f6 ! 2:  7a452142666 sequencer: comment `--reference` subject line properly
 > [...]
>      @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'identification of reverted c
>       +test_expect_success 'git revert --reference with core.commentChar' '
>       +	test_when_finished "git reset --hard to-ident" &&
>       +	git checkout --detach to-ident &&
>      -+	git -c core.commentChar=% revert \
>      ++	GIT_EDITOR="cat | head -4 >actual" git -c core.commentChar=% revert \
>       +		--edit --reference HEAD &&

"cat" is not doing anything here, GIT_EDITOR="head -n4 > actual" is all 
you need (I've added "-n" there as I'm not sure how portable a bare "-4" 
is).

>      -+	git log -1 --format=%B HEAD >actual &&
>      -+	printf "This reverts commit $(git show -s \
>      -+ 		--pretty=reference HEAD^).\n\n" \
>      -+		>expect &&
>      ++	cat <<-EOF >expect &&
>      ++	% *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***
>      ++
>      ++	This reverts commit $(git show -s --pretty=reference HEAD^).
>      ++
>      ++	EOF
>       +	test_cmp expect actual

This looks good - we're now checking that the user sees the comment when 
they edit the message.

>       +'
>       +
> 3:  86b4b485e0b ! 3:  4c342bc0422 sequencer: comment commit messages properly
>      @@ Metadata
>        ## Commit message ##
>           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

Stray "`"

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

> 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