[v3,2/3] sequencer: comment `--reference` subject line properly

Commit Message

Kristoffer Haugsbakk Nov. 24, 2024, 8:56 p.m. UTC

From: Kristoffer Haugsbakk <[e-mail]>

`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 <[e-mail]>
---

Notes (series):
    v3:
    • Review feedback: check more in the test by inspecting the
      proposed commit message.
    
      Link: https://lore.kernel.org/git/[e-mail]/
    • 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
       reverts commits` and add a newline to each of the other
       branches (`else if` and `else`).

 sequencer.c                   |  9 +++++----
 t/t3501-revert-cherry-pick.sh | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

Patch

diff --git a/sequencer.c b/sequencer.c
index 1b6fd86f70b..d26299cdea2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2341,8 +2341,8 @@  static int do_pick_commit(struct repository *r,
 		next = parent;
 		next_label = msg.parent_label;
 		if (opts->commit_use_reference) {
-			strbuf_addstr(&ctx->message,
-				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+			strbuf_commented_addf(&ctx->message, comment_line_str,
+				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
 		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
 			   /*
 			    * We don't touch pre-existing repeated reverts, because
@@ -2352,12 +2352,13 @@  static int do_pick_commit(struct repository *r,
 			   !starts_with(orig_subject, "Revert \"")) {
 			strbuf_addstr(&ctx->message, "Reapply \"");
 			strbuf_addstr(&ctx->message, orig_subject);
+			strbuf_addstr(&ctx->message, "\n");
 		} else {
 			strbuf_addstr(&ctx->message, "Revert \"");
 			strbuf_addstr(&ctx->message, msg.subject);
-			strbuf_addstr(&ctx->message, "\"");
+			strbuf_addstr(&ctx->message, "\"\n");
 		}
-		strbuf_addstr(&ctx->message, "\n\nThis reverts commit ");
+		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
 		refer_to_commit(opts, &ctx->message, commit);
 
 		if (commit->parents && commit->parents->next) {
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 411027fb58c..43476236131 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -228,6 +228,20 @@  test_expect_success 'identification of reverted commit (--reference)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git revert --reference with core.commentChar' '
+	test_when_finished "git reset --hard to-ident" &&
+	git checkout --detach to-ident &&
+	GIT_EDITOR="cat | head -4 >actual" git -c core.commentChar=% revert \
+		--edit --reference HEAD &&
+	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
+'
+
 test_expect_success 'identification of reverted commit (revert.reference)' '
 	git checkout --detach to-ident &&
 	git -c revert.reference=true revert --no-edit HEAD &&