diff mbox series

[v2,3/3] sequencer: comment commit messages properly

Message ID 86b4b485e0b3ef023a5d559f437eae22f2fd458f.1731406513.git.code@khaugsbakk.name (mailing list archive)
State New
Headers show
Series sequencer: comment out properly in todo list | expand

Commit Message

Kristoffer Haugsbakk Nov. 12, 2024, 10:20 a.m. UTC 
From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2
    • Phillip contributed the test and the `strbuf_setlen` changes

Notes (meta-trailers):
    Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
        Link: https://lore.kernel.org/git/cfa466b8-a87d-4b5d-b330-6c660897de48@gmail.com/#t

 sequencer.c                     | 12 ++++++++----
 t/t3437-rebase-fixup-options.sh | 15 +++++++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Nov. 13, 2024, 1:03 a.m. UTC | #1 
kristofferhaugsbakk@fastmail.com writes:

> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>

Describe what happens when a custom comment string is used without
the fixed code in this space.

> Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---

> +test_expect_success 'conflicting fixup -C after fixup with custom comment string' '
> +	test_config core.commentString COMMENT &&
> +	test_when_finished "test_might_fail git rebase --abort" &&
> +	git checkout --detach A3 &&
> +	test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
> +	echo resolved >A &&
> +	git add A &&
> +	FAKE_COMMIT_AMEND=edited git rebase --continue &&
> +	test_commit_message HEAD <<-\EOF
> +	A3
> +
> +	edited
> +	EOF
> +'

Doing so would allow readers to imagine more easily how this test
would catch breakages when the code is not fixed (or broken again).

Thanks.
Phillip Wood Nov. 13, 2024, 2:49 p.m. UTC | #2 
On 13/11/2024 01:03, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
> 
>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>
> 
> Describe what happens when a custom comment string is used without
> the fixed code in this space.

It would also be helpful to explain how to trigger the bug [1]

If I remember correctly it was Taylor who first noticed this [2]. If so 
we should credit him with a "Reported-by:" trailer.

>> Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>

It seems odd to me to have a "Co-authored-by:" trailer without a 
corresponding "Signed-off-by:" If someone has contributed enough to 
deserve "Co-authored-by:" then they should be signing off the code they 
have contributed. In this case I'd be happy with "Helped-by:" instead 
but feel free to add my "Signed-off-by:" if you want to keep 
"Co-authored-by:".

Best Wishes

Phillip

[1] 
https://lore.kernel.org/git/cfa466b8-a87d-4b5d-b330-6c660897de48@gmail.com/
[2]https://lore.kernel.org/git/ZxlEJ+44M8z03VOj@nand.local/

>> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>> ---
> 
>> +test_expect_success 'conflicting fixup -C after fixup with custom comment string' '
>> +	test_config core.commentString COMMENT &&
>> +	test_when_finished "test_might_fail git rebase --abort" &&
>> +	git checkout --detach A3 &&
>> +	test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
>> +	echo resolved >A &&
>> +	git add A &&
>> +	FAKE_COMMIT_AMEND=edited git rebase --continue &&
>> +	test_commit_message HEAD <<-\EOF
>> +	A3
>> +
>> +	edited
>> +	EOF
>> +'
> 
> Doing so would allow readers to imagine more easily how this test
> would catch breakages when the code is not fixed (or broken again).
> 
> Thanks.
diff mbox series

Patch

diff --git a/sequencer.c b/sequencer.c
index d26299cdea2..42a6f257cbb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1941,10 +1941,10 @@  static int seen_squash(struct replay_ctx *ctx)
 
 static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
 {
-	strbuf_setlen(buf1, 2);
+	strbuf_setlen(buf1, strlen(comment_line_str) + 1);
 	strbuf_addf(buf1, _(nth_commit_msg_fmt), n);
 	strbuf_addch(buf1, '\n');
-	strbuf_setlen(buf2, 2);
+	strbuf_setlen(buf2, strlen(comment_line_str) + 1);
 	strbuf_addf(buf2, _(skip_nth_commit_msg_fmt), n);
 	strbuf_addch(buf2, '\n');
 }
@@ -1963,8 +1963,12 @@  static void update_squash_message_for_fixup(struct strbuf *msg)
 	size_t orig_msg_len;
 	int i = 1;
 
-	strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str));
-	strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str));
+	strbuf_add_commented_lines(&buf1, _(first_commit_msg_str),
+				   strlen(_(first_commit_msg_str)),
+				   comment_line_str);
+	strbuf_add_commented_lines(&buf2, _(skip_first_commit_msg_str),
+				   strlen(_(skip_first_commit_msg_str)),
+				   comment_line_str);
 	s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
 	while (s) {
 		const char *next;
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 7929e2e2e3a..a4b90e881e3 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -127,6 +127,21 @@  test_expect_success 'fixup -C with conflicts gives correct message' '
 	test_cmp expected-author actual-author
 '
 
+test_expect_success 'conflicting fixup -C after fixup with custom comment string' '
+	test_config core.commentString COMMENT &&
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout --detach A3 &&
+	test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
+	echo resolved >A &&
+	git add A &&
+	FAKE_COMMIT_AMEND=edited git rebase --continue &&
+	test_commit_message HEAD <<-\EOF
+	A3
+
+	edited
+	EOF
+'
+
 test_expect_success 'skipping fixup -C after fixup gives correct message' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&