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

[kristofferhaugsbakk@fastmail.com]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Fix three places where the comment char/string is hardcoded (#) in the
todo list.

§ Changes in v3

I expanded on patch/commit message 3/3. I also checked the state (such as
the proposed commit message for the revert) in the tests. Both from
reviewer feedback.

See the rest of the changes as notes on the patches.

§ CC

• Stolee for the first patch
• Reviewers on the previous rounds

Kristoffer Haugsbakk (3):
  sequencer: comment checked-out branch properly
  sequencer: comment `--reference` subject line properly
  sequencer: comment commit messages properly

 sequencer.c                     | 26 ++++++++++++++++----------
 t/t3400-rebase.sh               | 19 +++++++++++++++++++
 t/t3437-rebase-fixup-options.sh | 15 +++++++++++++++
 t/t3501-revert-cherry-pick.sh   | 14 ++++++++++++++
 4 files changed, 64 insertions(+), 10 deletions(-)

Interdiff against v2:
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index f61a717b190..711bd230695 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -469,7 +469,10 @@ test_expect_success 'git rebase --update-ref with core.commentChar and branch on
 	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
 '
 
 test_done
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 26d3cabb608..43476236131 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -231,12 +231,14 @@ test_expect_success 'identification of reverted commit (--reference)' '
 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 &&
-	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
 '
 
Range-diff against v2:
1:  fc3b4438845 ! 1:  a46767263f6 sequencer: comment checked-out branch properly
    @@ Commit message
         `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`
    -    is in use.
    +    line gets interpreted as an invalid command when `core.commentChar`/
    +    `core.commentString` is in use.
     
    -    † 1: 900b50c242 (rebase: add --update-refs option, 2022-07-19)
    +    † 1: See 900b50c242 (rebase: add --update-refs option, 2022-07-19)
     
         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
     
      ## Notes (series) ##
    +    v3:
    +    • Review feedback: check more in the test by inspecting the
    +      sequence editor
    +
    +      Link: https://lore.kernel.org/git/5ed77fab-678d-4a06-bbd0-ea25462a7562@gmail.com/
    +    • Message: consistency with the other two messages:
    +      • Mention both commentChar and commentString
    +      • Commit footnote style: See <commit>
         v2:
         • Message: “hardcoded” (more common according to `git grep`)
     
    @@ 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
     +'
     +
      test_done
2:  710c5b1a3f6 ! 2:  7a452142666 sequencer: comment `--reference` subject line properly
    @@ Metadata
      ## Commit message ##
         sequencer: comment `--reference` subject line properly
     
    -    Comment the subject line used in `git cherry-pick --reference`
    -    properly.
    +    `git revert --reference <commit>` leaves behind a comment in the
    +    first line:[1]
     
    -    Follow the existing pattern and use the case described in the original
    -    commit message[1] as the `core.commentChar` test case:
    +        # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***
     
    -        If the user exits the editor without touching this line by mistake,
    -        what we prepare to become the first line of the body, i.e. "This
    -        reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to
    -        be the title of the resulting commit.
    +    Meaning that the commit will just consist of the next line if the user
    +    exits the editor directly:
     
    -    † 1: 43966ab3156 (revert: optionally refer to commit in the "reference"
    -        format, 2022-05-26)
    +        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>
     
     
      ## Notes (series) ##
    +    v3:
    +    • Review feedback: check more in the test by inspecting the
    +      proposed commit message.
    +
    +      Link: https://lore.kernel.org/git/4c623fcf-01dd-4056-80c1-b3c860ab7f87@gmail.com/
    +    • Message:
    +      • Rewrite message now that we are testing something different
    +      • consistency with the other two messages (see previous)
         v2:
         • `strbuf_commented_addf` adds a newline, unlike the previous function.
            We need to remove a newline from the final `strbuf_addstr` with `This
    @@ 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 &&
    -+	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
     +'
     +
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
    +    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>
     
     
      ## Notes (series) ##
    -    v2
    +    v3:
    +    • Message: Explain to the best of my knowledge what is going on here in
    +      the message body
    +
    +      Link: https://lore.kernel.org/git/711b59d7-e649-4031-8924-a16fb632b4d4@gmail.com/
    +    • Fixed wrong/subpar use of trailers
    +
    +      Link: https://lore.kernel.org/git/711b59d7-e649-4031-8924-a16fb632b4d4@gmail.com/
    +    v2:
         • Phillip contributed the test and the `strbuf_setlen` changes
     
     

base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94
-- 
2.47.0