Re: [PATCH] sequencer: comment checked-out branch properly

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Kristoffer

On 23/10/2024 20:53, Kristoffer Haugsbakk wrote:
> On Wed, Oct 23, 2024, at 20:44, Taylor Blau wrote:
> 
>> Makes sense, but the following command turns up a couple more results
>> even after applying:
>>
>>      $ git grep -p 'strbuf_addf([^,]*, "#'
>>      sequencer.c=static void update_squash_message_for_fixup(struct strbuf *msg)
>>      sequencer.c:    strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str));
>>      sequencer.c:    strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str));
>>
>> I imagine that we would want similar treatment there as well, no?
> 
> Here is where I got confused.  I tried to make this test appended to
> `t/t3437-rebase-fixup-options.sh` yesterday in order to exercise this
> code:
> 
> ```
> test_expect_success 'sequence of fixup, fixup -C & squash --signoff works (but with commentChar)' '
> 	git checkout --detach B3 &&
> 	FAKE_LINES="1 fixup 2 fixup_-C 3 fixup_-C 4 squash 5 fixup_-C 6" \
> 		FAKE_COMMIT_AMEND=squashed \
> 		FAKE_MESSAGE_COPY=actual-squash-message \
> 		git -c core.commentChar=% -c commit.status=false rebase -ik --signoff A &&
> 	git diff-tree --exit-code --patch HEAD B3 -- &&
> 	echo actual: &&
> 	cat actual-squash-message
> '
> ```> And the output looked correct, i.e. all-`%`.[1]
> † 1:
> 
> ```
> % This is a combination of 6 commits.
> % This is the 1st commit message:
> 
> B

This line should have been be commented out when adding
"amend! B" commit below

> Signed-off-by: Rebase Committer <rebase.committer@example.com>
> 
> % The commit message #2 will be skipped:
> 
> % fixup! B
> 
> % This is the commit message #3:
> 
> % amend! B
> 
> B
> 
> edited 1

This message should have been commented out when adding
"amend! amend! ..." below

> Signed-off-by: Rebase Committer <rebase.committer@example.com>
> 
> % This is the commit message #4:
> 
> % amend! amend! B
> 
> B
> 
> edited 1
> 
> edited 2
> 
> Signed-off-by: Rebase Committer <rebase.committer@example.com>

The diff below shows a fix and a new test that fails without the
sequencer changes. The fix is based on master, so it might need
updating to go on top of Junio's series. The test could probably
be improved to use the existing setup.

Best Wishes

Phillip


diff --git a/sequencer.c b/sequencer.c
index 353d804999b..6e45b8ef04f 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,8 @@ 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_addf(&buf1, "%s %s\n", comment_line_str, _(first_commit_msg_str));
+	strbuf_addf(&buf2, "%s %s\n", comment_line_str, _(skip_first_commit_msg_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..59c031005e6 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -220,4 +220,34 @@ test_expect_success 'fixup -[Cc]<commit> works' '
  	test_cmp expect actual
  '
  
+test_expect_success 'fixup -C comments out previous messages' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	test_config core.commentString COMMENT &&
+	git checkout B &&
+	git commit --fixup=HEAD --allow-empty -m "EMPTY FIXUP" &&
+	test_commit new-file &&
+	echo changed >new-file.t &&
+
+	write_script editor.sh <<-\EOF &&
+	sed -n -e "1,2 p
+		   3 {
+			c\\
+			edited
+			q
+		   }" "$1" >"$1.tmp"
+	cat "$1.tmp" >"$1"
+	EOF
+
+	(
+		test_set_editor "$(pwd)/editor.sh" &&
+		git commit --fixup=amend:B new-file.t
+	) &&
+
+	test_must_fail git rebase --autosquash A &&
+	echo resolved >new-file.t &&
+	git add new-file.t &&
+	test_must_fail git rebase --continue &&
+	test_commit_message HEAD -m edited
+'
+
  test_done


> % This is the commit message #5:
> 
> % squash! amend! amend! B
> 
> edited squash
> 
> % This is the commit message #6:
> 
> % amend! amend! amend! B
> 
> B
> 
> edited 1
> 
> edited 2
> 
> edited 3
> squashed
> ok 13 - sequence of fixup, fixup -C & squash --signoff works (but with commentChar)
> ```
>