[PATCH] sequencer: comment checked-out branch properly

[PATCH] sequencer: comment checked-out branch properly

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

`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 hard-coded (#).  In turn the comment
line gets interpreted as an invalid command when `core.commentChar`
is in use.

† 1: 900b50c242 (rebase: add --update-refs option, 2022-07-19)

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 sequencer.c       |  5 +++--
 t/t3400-rebase.sh | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 353d804999b..1b6fd86f70b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
 		/* If the branch is checked out, then leave a comment instead. */
 		if ((path = branch_checked_out(decoration->name))) {
 			item->command = TODO_COMMENT;
-			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
-				    decoration->name, path);
+			strbuf_commented_addf(ctx->buf, comment_line_str,
+					      "Ref %s checked out at '%s'\n",
+					      decoration->name, path);
 		} else {
 			struct string_list_item *sti;
 			item->command = TODO_UPDATE_REF;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 09f230eefb2..f61a717b190 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -456,4 +456,20 @@ test_expect_success 'rebase when inside worktree subdirectory' '
 	)
 '
 
+test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
+	test_when_finished git branch -D base topic2 &&
+	test_when_finished git checkout main &&
+	test_when_finished git branch -D wt-topic &&
+	test_when_finished git worktree remove wt-topic &&
+	git checkout main &&
+	git checkout -b base &&
+	git checkout -b topic2 &&
+	test_commit msg2 &&
+	git worktree add wt-topic &&
+	git checkout base &&
+	test_commit msg3 &&
+	git checkout topic2 &&
+	git -c core.commentChar=% rebase --update-refs base
+'
+
 test_done

base-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0
-- 
2.46.1.641.g54e7913fcb6

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

[Taylor Blau]

On Wed, Oct 23, 2024 at 07:27:58PM +0200, kristofferhaugsbakk@fastmail.com wrote:
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> `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 hard-coded (#).  In turn the comment
> line gets interpreted as an invalid command when `core.commentChar`
> is in use.

Nice find. My first thought when reading was that this was a regression
from 8b311478ad (config: allow multi-byte core.commentChar, 2024-03-12).
But thinking about it for a moment that is definitely not true, as this
has probably never worked since core.commentChar was introduced, and has
nothing to do with what range of value(s) it does or doesn't support.

> † 1: 900b50c242 (rebase: add --update-refs option, 2022-07-19)
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>  sequencer.c       |  5 +++--
>  t/t3400-rebase.sh | 16 ++++++++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 353d804999b..1b6fd86f70b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
>  		/* If the branch is checked out, then leave a comment instead. */
>  		if ((path = branch_checked_out(decoration->name))) {
>  			item->command = TODO_COMMENT;
> -			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
> -				    decoration->name, path);
> +			strbuf_commented_addf(ctx->buf, comment_line_str,
> +					      "Ref %s checked out at '%s'\n",
> +					      decoration->name, path);

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?

> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 09f230eefb2..f61a717b190 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -456,4 +456,20 @@ test_expect_success 'rebase when inside worktree subdirectory' '
>  	)
>  '
>
> +test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
> +	test_when_finished git branch -D base topic2 &&
> +	test_when_finished git checkout main &&
> +	test_when_finished git branch -D wt-topic &&
> +	test_when_finished git worktree remove wt-topic &&
> +	git checkout main &&
> +	git checkout -b base &&
> +	git checkout -b topic2 &&
> +	test_commit msg2 &&
> +	git worktree add wt-topic &&
> +	git checkout base &&
> +	test_commit msg3 &&
> +	git checkout topic2 &&
> +	git -c core.commentChar=% rebase --update-refs base
> +'
> +

Seems quite reasonable.

Thanks,
Taylor

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

[Kristoffer Haugsbakk]

On Wed, Oct 23, 2024, at 20:44, Taylor Blau wrote:
> Nice find. My first thought when reading was that this was a
> regression from 8b311478ad (config: allow multi-byte core.commentChar,
> 2024-03-12).  But thinking about it for a moment that is definitely
> not true, as this has probably never worked since core.commentChar was
> introduced, and has nothing to do with what range of value(s) it does
> or doesn't support.

Yeah, `%` turns out to be sufficient to reproduce (even though I use a
multi-byte one myself, and when I found this).

>> […]
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
>>  		/* If the branch is checked out, then leave a comment instead. */
>>  		if ((path = branch_checked_out(decoration->name))) {
>>  			item->command = TODO_COMMENT;
>> -			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
>> -				    decoration->name, path);
>> +			strbuf_commented_addf(ctx->buf, comment_line_str,
>> +					      "Ref %s checked out at '%s'\n",
>> +					      decoration->name, path);
>
> 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]

I didn’t understand that.  Maybe I exercised the wrong code.  But that’s
the point where I dropped that lead yesterday.  Figured that there was
some downstream string magic that I was unaware of.

(I could just change those two and see if any tests blow up)

However there is this one in `sequencer.c`:

```
		if (opts->commit_use_reference) {
			strbuf_addstr(&ctx->message,
				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
```

And that line is supposed to be a comment line according to the commit
(43966ab3156 (revert: optionally refer to commit in the "reference"
format, 2022-05-26)).  But it does just output hardcoded `#` if you
change comment char.  I’ll add that to the series.

>> […]
>> +test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
>> +	test_when_finished git branch -D base topic2 &&
>> +	test_when_finished git checkout main &&
>> +	test_when_finished git branch -D wt-topic &&
>> +	test_when_finished git worktree remove wt-topic &&
>> +	git checkout main &&
>> +	git checkout -b base &&
>> +	git checkout -b topic2 &&
>> +	test_commit msg2 &&
>> +	git worktree add wt-topic &&
>> +	git checkout base &&
>> +	test_commit msg3 &&
>> +	git checkout topic2 &&
>> +	git -c core.commentChar=% rebase --update-refs base
>> +'
>> +
>
> Seems quite reasonable.

In hindsight and with some `cat todo` it seems that the setup isn’t
minimal.  I stumbled upon this by accident while not using worktrees.
And the todo editor seems to comment out two lines, not just one.

Maybe detached `HEAD` would be more lean.

† 1:

```
% This is a combination of 6 commits.
% This is the 1st commit message:

B

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

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>

% 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)
```

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

[Taylor Blau]

On Wed, Oct 23, 2024 at 07:27:58PM +0200, kristofferhaugsbakk@fastmail.com wrote:
>  sequencer.c       |  5 +++--
>  t/t3400-rebase.sh | 16 ++++++++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)

I should have thought to mention this earlier, but this does not easily
integrate into 'seen' because of 'jc/strbuf-commented-something', which
turns, for e.g.:

    strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);

into:

    strbuf_add_comment_lines(&cbuf, buf.buf, buf.len);

Note that the function is both renamed, and already knows about
comment_line_str, so it is not necessary to pass it in as an argument
explicitly.

Perhaps you may want to rebuild your topic on that one?

Thanks,
Taylor

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

[Kristoffer Haugsbakk]

On Wed, Oct 23, 2024, at 22:43, Taylor Blau wrote:
> Perhaps you may want to rebuild your topic on that one?

Sure thing, and thanks!

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

[Phillip Wood]

On 23/10/2024 19:44, Taylor Blau wrote:
> On Wed, Oct 23, 2024 at 07:27:58PM +0200, kristofferhaugsbakk@fastmail.com wrote:
>> @@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
>>   		/* If the branch is checked out, then leave a comment instead. */
>>   		if ((path = branch_checked_out(decoration->name))) {
>>   			item->command = TODO_COMMENT;
>> -			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
>> -				    decoration->name, path);
>> +			strbuf_commented_addf(ctx->buf, comment_line_str,
>> +					      "Ref %s checked out at '%s'\n",
>> +					      decoration->name, path);
> 
> 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));

Good find - it's surprising that those have survived so long without 
anyone complaining. There is also

	"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***",

in do_pick_commit()

Best Wishes

Phillip

>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 09f230eefb2..f61a717b190 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -456,4 +456,20 @@ test_expect_success 'rebase when inside worktree subdirectory' '
>>   	)
>>   '
>>
>> +test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
>> +	test_when_finished git branch -D base topic2 &&
>> +	test_when_finished git checkout main &&
>> +	test_when_finished git branch -D wt-topic &&
>> +	test_when_finished git worktree remove wt-topic &&
>> +	git checkout main &&
>> +	git checkout -b base &&
>> +	git checkout -b topic2 &&
>> +	test_commit msg2 &&
>> +	git worktree add wt-topic &&
>> +	git checkout base &&
>> +	test_commit msg3 &&
>> +	git checkout topic2 &&
>> +	git -c core.commentChar=% rebase --update-refs base
>> +'
>> +
> 
> Seems quite reasonable.
> 
> Thanks,
> Taylor
> 

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

[Kristoffer Haugsbakk]

On Thu, Oct 31, 2024, at 10:58, Phillip Wood wrote:
> On 23/10/2024 19:44, Taylor Blau wrote:
>> On Wed, Oct 23, 2024 at 07:27:58PM +0200, kristofferhaugsbakk@fastmail.com wrote:
>>> @@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
>>>   		/* If the branch is checked out, then leave a comment instead. */
>>>   		if ((path = branch_checked_out(decoration->name))) {
>>>   			item->command = TODO_COMMENT;
>>> -			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
>>> -				    decoration->name, path);
>>> +			strbuf_commented_addf(ctx->buf, comment_line_str,
>>> +					      "Ref %s checked out at '%s'\n",
>>> +					      decoration->name, path);
>>
>> 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));
>
> Good find - it's surprising that those have survived so long without
> anyone complaining. There is also
>
> 	"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***",
>
> in do_pick_commit()

I’m including an update to “say why” in the next version.

Those others look correct to me: https://lore.kernel.org/git/c05e603f-1fd4-4ad2-ba03-21269f464ed2@gmail.com/T/#mf299f1ac7bdb772b396068200d32b5fac669fb55

Cheers :)

Kristoffer

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

[Phillip Wood]

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)
> ```
> 

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

[Phillip Wood]

Hi Kristoffer

On 31/10/2024 10:07, Kristoffer Haugsbakk wrote:
> On Thu, Oct 31, 2024, at 10:58, Phillip Wood wrote:
>> On 23/10/2024 19:44, Taylor Blau wrote:
>>> On Wed, Oct 23, 2024 at 07:27:58PM +0200, kristofferhaugsbakk@fastmail.com wrote:
>>>> @@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
>>>>    		/* If the branch is checked out, then leave a comment instead. */
>>>>    		if ((path = branch_checked_out(decoration->name))) {
>>>>    			item->command = TODO_COMMENT;
>>>> -			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
>>>> -				    decoration->name, path);
>>>> +			strbuf_commented_addf(ctx->buf, comment_line_str,
>>>> +					      "Ref %s checked out at '%s'\n",
>>>> +					      decoration->name, path);
>>>
>>> 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));
>>
>> Good find - it's surprising that those have survived so long without
>> anyone complaining. There is also
>>
>> 	"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***",
>>
>> in do_pick_commit()
> 
> I’m including an update to “say why” in the next version.
> 
> Those others look correct to me: https://lore.kernel.org/git/c05e603f-1fd4-4ad2-ba03-21269f464ed2@gmail.com/T/#mf299f1ac7bdb772b396068200d32b5fac669fb55

Oh, sorry I'd missed that message.

Best Wishes

Phillip

> Cheers :)
> 
> Kristoffer

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

[Kristoffer Haugsbakk]

On Thu, Oct 31, 2024, at 17:30, Phillip Wood wrote:
> 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.

Thank you!  That those lines apparently worked had kind of been
bothering me.  It’s nice to get some clarity on the issue.

But shouldn’t there be a signoff somewhere if I am to incorporate that
diff into the series?  Or is the signoff implied?

-- 
Kristoffer Haugsbakk

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

[phillip.wood123]

Hi Kristoffer

On 31/10/2024 17:25, Kristoffer Haugsbakk wrote:
> On Thu, Oct 31, 2024, at 17:30, Phillip Wood wrote:
>> 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.
> 
> Thank you!  That those lines apparently worked had kind of been
> bothering me.  It’s nice to get some clarity on the issue.
> 
> But shouldn’t there be a signoff somewhere if I am to incorporate that
> diff into the series?  Or is the signoff implied?

Here's a nicer test case, the key is that we need a conflicting
"fixup -C" after a "fixup" otherwise we just end up using the
fixup message file which only ever contains a single commit message.
Here's my signoff for this and the sequencer changes in my previous mail

Signed-off-by: Phillip.Wood <phillip.wood@dunelm.org.uk>

Best Wishes

Phillip

---- >8 ----
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 &&

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

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

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

This series does not depend on any other topics.
Topic `jc/strbuf-commented-something` was mentioned on the v1
discussion.  But it was kicked out of `seen` last week.  Also it doesn’t
compile when merged into `v2.47.0` or later:

```
strbuf.c: In function ‘strbuf_add_comment_lines’:
strbuf.c:384:24: error: ‘comment_line_str’ undeclared (first use in this function)
  384 |         add_lines(out, comment_line_str, buf, size, 1);
      |                        ^~~~~~~~~~~~~~~~
strbuf.c:384:24: note: each undeclared identifier is reported only once for each function it appears in
make: *** [Makefile:2795: strbuf.o] Error 1
```

§ Changes in v2

The first version just had patch 1 but this one fixes two other places.
The two other places where unearthered during the v1 discussion.

Rebased on `master` (b31fb630c0 (Merge https://github.com/j6t/git-gui,
2024-11-11)).

§ CI

Some failures that didn’t look relevant.

• linux-leaks
  • t0301-credential-cache
  • t9211-scalar-clone
• linux-reftable-leaks
  • ditto above

§ CC

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

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               | 16 ++++++++++++++++
 t/t3437-rebase-fixup-options.sh | 15 +++++++++++++++
 t/t3501-revert-cherry-pick.sh   | 12 ++++++++++++
 4 files changed, 59 insertions(+), 10 deletions(-)

Range-diff against v1:
1:  5267b9a9c8c ! 1:  fc3b4438845 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 hard-coded (#).  In turn the comment
    +    that fact.  The comment char is hardcoded (#).  In turn the comment
         line gets interpreted as an invalid command when `core.commentChar`
         is in use.
     
    @@ Commit message
     
         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
     
    +
    + ## Notes (series) ##
    +    v2:
    +    • Message: “hardcoded” (more common according to `git grep`)
    +
      ## sequencer.c ##
     @@ sequencer.c: static int add_decorations_to_list(const struct commit *commit,
      		/* If the branch is checked out, then leave a comment instead. */
-:  ----------- > 2:  710c5b1a3f6 sequencer: comment `--reference` subject line properly
-:  ----------- > 3:  86b4b485e0b sequencer: comment commit messages properly

base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94
-- 
2.47.0

[PATCH v2 1/3] sequencer: comment checked-out branch properly

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

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

† 1: 900b50c242 (rebase: add --update-refs option, 2022-07-19)

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2:
    • Message: “hardcoded” (more common according to `git grep`)

 sequencer.c       |  5 +++--
 t/t3400-rebase.sh | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 353d804999b..1b6fd86f70b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
 		/* If the branch is checked out, then leave a comment instead. */
 		if ((path = branch_checked_out(decoration->name))) {
 			item->command = TODO_COMMENT;
-			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
-				    decoration->name, path);
+			strbuf_commented_addf(ctx->buf, comment_line_str,
+					      "Ref %s checked out at '%s'\n",
+					      decoration->name, path);
 		} else {
 			struct string_list_item *sti;
 			item->command = TODO_UPDATE_REF;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 09f230eefb2..f61a717b190 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -456,4 +456,20 @@ test_expect_success 'rebase when inside worktree subdirectory' '
 	)
 '
 
+test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
+	test_when_finished git branch -D base topic2 &&
+	test_when_finished git checkout main &&
+	test_when_finished git branch -D wt-topic &&
+	test_when_finished git worktree remove wt-topic &&
+	git checkout main &&
+	git checkout -b base &&
+	git checkout -b topic2 &&
+	test_commit msg2 &&
+	git worktree add wt-topic &&
+	git checkout base &&
+	test_commit msg3 &&
+	git checkout topic2 &&
+	git -c core.commentChar=% rebase --update-refs base
+'
+
 test_done
-- 
2.47.0

[PATCH v2 2/3] sequencer: comment `--reference` subject line properly

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Comment the subject line used in `git cherry-pick --reference`
properly.

Follow the existing pattern and use the case described in the original
commit message[1] as the `core.commentChar` test case:

    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.

† 1: 43966ab3156 (revert: optionally refer to commit in the "reference"
    format, 2022-05-26)

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    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 | 12 ++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

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..26d3cabb608 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -228,6 +228,18 @@ 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 -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 &&
+	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 &&
-- 
2.47.0

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

[kristofferhaugsbakk]

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(-)

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 &&
-- 
2.47.0

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

[Junio C Hamano]

kristofferhaugsbakk@fastmail.com writes:

> The first version just had patch 1 but this one fixes two other places.
> The two other places where unearthered during the v1 discussion.

OK.  I guess they could be handled in a single patch, but the three
patches address different things to be commented properly, so having
them as three separate patches is good.

> Rebased on `master` (b31fb630c0 (Merge https://github.com/j6t/git-gui,
> 2024-11-11)).

Was there any reason, other than "newer must be better" (which is
not always true)?  I thought there isn't any in-flight topics that
touched the sequencer machinery.

> Some failures that didn’t look relevant.

Judging from https://github.com/git/git/actions/runs/11774751134
(which you rebased the topic on) passing all these CI tests,
if your topic saw CI breakages, there is nothing else we can put the
blame on.

But let's see what happens.  I won't see CI failures on individual
topic, but we will see what the topic, together with everybody else,
in the context of 'seen', produces when I push today's integration
result out.

Thanks.

Re: [PATCH v2 3/3] sequencer: comment commit messages properly

[Junio C Hamano]

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.

Re: [PATCH v2 1/3] sequencer: comment checked-out branch properly

[Junio C Hamano]

kristofferhaugsbakk@fastmail.com writes:

> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 09f230eefb2..f61a717b190 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -456,4 +456,20 @@ test_expect_success 'rebase when inside worktree subdirectory' '
>  	)
>  '
>  
> +test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
> +	test_when_finished git branch -D base topic2 &&
> +	test_when_finished git checkout main &&
> +	test_when_finished git branch -D wt-topic &&
> +	test_when_finished git worktree remove wt-topic &&
> +	git checkout main &&
> +	git checkout -b base &&
> +	git checkout -b topic2 &&
> +	test_commit msg2 &&
> +	git worktree add wt-topic &&
> +	git checkout base &&
> +	test_commit msg3 &&
> +	git checkout topic2 &&
> +	git -c core.commentChar=% rebase --update-refs base
> +'

Can we improve this test a bit to give it more visibility into the
breakage?

I am sure that the internal machinery gets confused because it wants
to skip commented out lines assuming '%' is used for comments, and
fails to skip lines that are commented with '#', but it is a bit too
opaque how this would break without the fix.  Perhaps add a line or
two of a comment to the test to describe what the expected mode of
failure is?

Re: [PATCH v2 2/3] sequencer: comment `--reference` subject line properly

[Junio C Hamano]

kristofferhaugsbakk@fastmail.com writes:

> @@ -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 ***");

With the switch to "commented_addf", we'd terminate this line with
LF here, which means ...

> @@ -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");

... we'd want to terminate the line in these two other if/else if/else
arms for symmetry, so that ...

>  		}
> -		strbuf_addstr(&ctx->message, "\n\nThis reverts commit ");
> +		strbuf_addstr(&ctx->message, "\nThis reverts commit ");

... we can lose the termination of the previous line from here.

Makes sense.

> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 411027fb58c..26d3cabb608 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -228,6 +228,18 @@ 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 -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 &&
> +	test_cmp expect actual
> +'

I guess this fails by leaving the "# *** SAY WHY" in the resulting
message, because the stripspace wants to see '%' to start commented
out lines to be stripped?  If we inspect with this test what the
temporary file we give to the editor looks like to make sure that
'%' is used for commenting, that would be a more direct test, but
without going that far, at least can we have a comment describing
how this is expected to fail without the fix?

Thanks.

Re: [PATCH v2 1/3] sequencer: comment checked-out branch properly

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> kristofferhaugsbakk@fastmail.com writes:
>
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 09f230eefb2..f61a717b190 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -456,4 +456,20 @@ test_expect_success 'rebase when inside worktree subdirectory' '
>>  	)
>>  '
>>  
>> +test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
>> +	test_when_finished git branch -D base topic2 &&
>> +	test_when_finished git checkout main &&
>> +	test_when_finished git branch -D wt-topic &&
>> +	test_when_finished git worktree remove wt-topic &&
>> +	git checkout main &&
>> +	git checkout -b base &&
>> +	git checkout -b topic2 &&
>> +	test_commit msg2 &&
>> +	git worktree add wt-topic &&
>> +	git checkout base &&
>> +	test_commit msg3 &&
>> +	git checkout topic2 &&
>> +	git -c core.commentChar=% rebase --update-refs base
>> +'
>
> Can we improve this test a bit to give it more visibility into the
> breakage?
>
> I am sure that the internal machinery gets confused because it wants
> to skip commented out lines assuming '%' is used for comments, and
> fails to skip lines that are commented with '#', but it is a bit too
> opaque how this would break without the fix.  Perhaps add a line or
> two of a comment to the test to describe what the expected mode of
> failure is?

Something like "a line commented-out with '#' in the list of
instructions used internally by "rebase" is not recognised as a
comment and you'd get an error that says '#' is not a valid
instruction", perhaps.

Re: [PATCH v2 1/3] sequencer: comment checked-out branch properly

[phillip.wood123]

On 13/11/2024 01:07, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
> 
>> +test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
>> +	test_when_finished git branch -D base topic2 &&
>> +	test_when_finished git checkout main &&
>> +	test_when_finished git branch -D wt-topic &&
>> +	test_when_finished git worktree remove wt-topic &&
>> +	git checkout main &&
>> +	git checkout -b base &&
>> +	git checkout -b topic2 &&
>> +	test_commit msg2 &&
>> +	git worktree add wt-topic &&
>> +	git checkout base &&
>> +	test_commit msg3 &&
>> +	git checkout topic2 &&
>> +	git -c core.commentChar=% rebase --update-refs base
>> +'
> 
> Can we improve this test a bit to give it more visibility into the
> breakage?
> 
> I am sure that the internal machinery gets confused because it wants
> to skip commented out lines assuming '%' is used for comments, and
> fails to skip lines that are commented with '#', but it is a bit too
> opaque how this would break without the fix.  Perhaps add a line or
> two of a comment to the test to describe what the expected mode of
> failure is?

Or check the todo list shown to the user with

	GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \
		rebase -i --update-refs base &&
	test_grep "% Ref refs/heads/wt-topic checked out at " actual

so that we are sure that line exists - I had a quick look and I can't 
see any existing coverage checking that the todo list contains this comment.

Best Wishes

Phillip

Re: [PATCH v2 2/3] sequencer: comment `--reference` subject line properly

[phillip.wood123]

On 13/11/2024 01:07, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
> 
>> +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 \
>> +		--edit --reference HEAD &&
>> +	git log -1 --format=%B HEAD >actual &&
>> +	printf "This reverts commit $(git show -s \
>> + 		--pretty=reference HEAD^).\n\n" \
>> +		>expect &&
>> +	test_cmp expect actual
>> +'
> 
> I guess this fails by leaving the "# *** SAY WHY" in the resulting
> message, because the stripspace wants to see '%' to start commented
> out lines to be stripped?  If we inspect with this test what the
> temporary file we give to the editor looks like to make sure that
> '%' is used for commenting, that would be a more direct test, but
> without going that far, at least can we have a comment describing
> how this is expected to fail without the fix?

For me something like

	GIT_EDITOR="cat >actual" git -c core.commentChar=% revert \
		--edit --reference HEAD &&
	test_grep "^% \*\*\* SAY WHY WE ARE REVERTING THE COMMIT \*\*\*" \
		  actual

Would be a more convincing test as it actually checks that the user sees
the line that we expect strbuf_stripspace() to remove from the final
message. If we want to check the commit message as well that's fine but
I'm not sure its necessary. (if we do we should use test_commit_message
like patch 3)

Best Wishes

Phillip

Re: [PATCH v2 3/3] sequencer: comment commit messages properly

[phillip.wood123]

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.

Re: [PATCH v2 1/3] sequencer: comment checked-out branch properly

[Junio C Hamano]

phillip.wood123@gmail.com writes:

>>> ...
>>> +	git checkout topic2 &&
>>> +	git -c core.commentChar=% rebase --update-refs base
>>> +'
>> Can we improve this test a bit to give it more visibility into the
>> breakage?
>> I am sure that the internal machinery gets confused because it wants
>> to skip commented out lines assuming '%' is used for comments, and
>> fails to skip lines that are commented with '#', but it is a bit too
>> opaque how this would break without the fix.  Perhaps add a line or
>> two of a comment to the test to describe what the expected mode of
>> failure is?
>
> Or check the todo list shown to the user with
>
> 	GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \
> 		rebase -i --update-refs base &&
> 	test_grep "% Ref refs/heads/wt-topic checked out at " actual
>
> so that we are sure that line exists - I had a quick look and I can't
> see any existing coverage checking that the todo list contains this
> comment.

Yeah, with "rebase -i", it is a part of end-user experience to see
these comments, and a check to make sure they are shown to guide the
user is certainly a good idea.

A test of "rebase" without "-i" is a different matter.  Maybe in the
future, while we still reuse the machinery used for the interactive
mode to implement the non-interactive rebase, we may tweak it so
that we do not generate commented lines, only to be skipped by the
parser, in the todo list file that is only used internally.

Thanks.

Re: [PATCH v2 2/3] sequencer: comment `--reference` subject line properly

[Junio C Hamano]

phillip.wood123@gmail.com writes:

>> I guess this fails by leaving the "# *** SAY WHY" in the resulting
>> message, because the stripspace wants to see '%' to start commented
>> out lines to be stripped?  If we inspect with this test what the
>> temporary file we give to the editor looks like to make sure that
>> '%' is used for commenting, that would be a more direct test, but
>> without going that far, at least can we have a comment describing
>> how this is expected to fail without the fix?
>
> For me something like
>
> 	GIT_EDITOR="cat >actual" git -c core.commentChar=% revert \
> 		--edit --reference HEAD &&
> 	test_grep "^% \*\*\* SAY WHY WE ARE REVERTING THE COMMIT \*\*\*" \
> 		  actual
>
> Would be a more convincing test as it actually checks that the user sees
> the line that we expect strbuf_stripspace() to remove from the final
> message.

Yes, it is a more direct test.  If we did that, that would of course
be preferrable.

Thanks.

Re: [PATCH v2 3/3] sequencer: comment commit messages properly

[Kristoffer Haugsbakk]

On Wed, Nov 13, 2024, at 15:49, phillip.wood123@gmail.com wrote:
> 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:".

I’ll fix that in the next version.  Thanks.

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

[Kristoffer Haugsbakk]

On Wed, Nov 13, 2024, at 01:26, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> The first version just had patch 1 but this one fixes two other places.
>> The two other places where unearthered during the v1 discussion.
>
> OK.  I guess they could be handled in a single patch, but the three
> patches address different things to be commented properly, so having
> them as three separate patches is good.
>
>> Rebased on `master` (b31fb630c0 (Merge https://github.com/j6t/git-gui,
>> 2024-11-11)).
>
> Was there any reason, other than "newer must be better" (which is
> not always true)?  I thought there isn't any in-flight topics that
> touched the sequencer machinery.

I’m sorry. I worked on the v2 by rebasing on `jc/strbuf-commented-something`
which was in `seen` at the time. Then when I had to `rebase --onto` back
someplace I picked `master` since I didn’t need the interdiff for the cover
letter (two new patches, no changes to the first one).

Re: [PATCH v2 1/3] sequencer: comment checked-out branch properly

[Kristoffer Haugsbakk]

On Wed, Nov 13, 2024, at 15:47, phillip.wood123@gmail.com wrote:
> Or check the todo list shown to the user with
>
> 	GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \
> 		rebase -i --update-refs base &&
> 	test_grep "% Ref refs/heads/wt-topic checked out at " actual
>
> so that we are sure that line exists - I had a quick look and I can't 
> see any existing coverage checking that the todo list contains this comment.

Thanks!

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

[kristofferhaugsbakk]

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

[PATCH v3 1/3] sequencer: comment checked-out branch properly

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

`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`/
`core.commentString` is in use.

† 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`)

 sequencer.c       |  5 +++--
 t/t3400-rebase.sh | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 353d804999b..1b6fd86f70b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
 		/* If the branch is checked out, then leave a comment instead. */
 		if ((path = branch_checked_out(decoration->name))) {
 			item->command = TODO_COMMENT;
-			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
-				    decoration->name, path);
+			strbuf_commented_addf(ctx->buf, comment_line_str,
+					      "Ref %s checked out at '%s'\n",
+					      decoration->name, path);
 		} else {
 			struct string_list_item *sti;
 			item->command = TODO_UPDATE_REF;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 09f230eefb2..711bd230695 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -456,4 +456,23 @@ test_expect_success 'rebase when inside worktree subdirectory' '
 	)
 '
 
+test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
+	test_when_finished git branch -D base topic2 &&
+	test_when_finished git checkout main &&
+	test_when_finished git branch -D wt-topic &&
+	test_when_finished git worktree remove wt-topic &&
+	git checkout main &&
+	git checkout -b base &&
+	git checkout -b topic2 &&
+	test_commit msg2 &&
+	git worktree add wt-topic &&
+	git checkout base &&
+	test_commit msg3 &&
+	git checkout topic2 &&
+	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.47.0

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

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

`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 <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
       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(-)

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 &&
-- 
2.47.0

[PATCH v3 3/3] sequencer: comment commit messages properly

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

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):
    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

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(-)

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 &&
-- 
2.47.0

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

[phillip.wood123]

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

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

[Kristoffer Haugsbakk]

On Mon, Nov 25, 2024, at 11:07, phillip.wood123@gmail.com wrote:
> Hi Kristoffer
>
> Thanks for re-rolling, I've left some comments on the range-diff

Hi Phillip, thanks for the review!

I should be able to fix these and reroll today.

> [...]
> 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.

I use `fixup -c` for the third/last commit here.  So I am prompted to
edit the commit message. I tested this on this series:

    git checkout --detach kh/sequencer-comment-char
    git rebase -i fd3785337beb285ed7fd67ce6fc3d3bed2097b40

Which gave me [this] editor without these changes (with
`core.commentChar` set to `%`).

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

† this:

    % This is a combination of 3 commits.
    % This is the 1st commit message:

    sequencer: comment checked-out branch properly

    `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`/
    `core.commentString` is in use.

    † 1: See 900b50c242 (rebase: add --update-refs option, 2022-07-19)

    Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>

    % The commit message #2 will be skipped:

    % sequencer: comment `--reference` subject line properly
    %
    % `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 <code@khaugsbakk.name>

    % This is the commit message #3:

    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>

    % Please enter the commit message for your changes. Lines starting

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

[phillip.wood123]

Hi Kristoffer

On 25/11/2024 10:52, Kristoffer Haugsbakk wrote:
> On Mon, Nov 25, 2024, at 11:07, phillip.wood123@gmail.com wrote:
> 
> Hi Phillip, thanks for the review!

You're welcome, thanks for fixing this

>>>       +    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.
> 
> I use `fixup -c` for the third/last commit here.  So I am prompted to
> edit the commit message. I tested this on this series:
> 
>      git checkout --detach kh/sequencer-comment-char
>      git rebase -i fd3785337beb285ed7fd67ce6fc3d3bed2097b40
> 
> Which gave me [this] editor without these changes (with
> `core.commentChar` set to `%`).

Oh, I see the same thing, I was sure we only showed the final message 
unless there were conflicts. I wonder if I've misremembered or the 
behavior has changed in any case that's outside the scope of this series.

Thanks

Phillip

>>
>>> 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
> 
> † this:
> 
>      % This is a combination of 3 commits.
>      % This is the 1st commit message:
> 
>      sequencer: comment checked-out branch properly
> 
>      `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`/
>      `core.commentString` is in use.
> 
>      † 1: See 900b50c242 (rebase: add --update-refs option, 2022-07-19)
> 
>      Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> 
>      % The commit message #2 will be skipped:
> 
>      % sequencer: comment `--reference` subject line properly
>      %
>      % `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 <code@khaugsbakk.name>
> 
>      % This is the commit message #3:
> 
>      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>
> 
>      % Please enter the commit message for your changes. Lines starting

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

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

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

§ Changes in v4

• Use `test_grep`
• Fix commit message (`)
• Don’t need to cat(1)
  • Also use `-n4` in case `-4` is not widely supported

§ 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 v3:
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 711bd230695..7c47af6dcd9 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -471,8 +471,8 @@ test_expect_success 'git rebase --update-ref with core.commentChar and branch on
 	git checkout topic2 &&
 	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_grep "% Ref refs/heads/wt-topic checked out at" actual &&
+	test_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 43476236131..b84fdfe8a32 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -231,7 +231,7 @@ 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_EDITOR="cat | head -4 >actual" git -c core.commentChar=% revert \
+	GIT_EDITOR="head -n4 >actual" git -c core.commentChar=% revert \
 		--edit --reference HEAD &&
 	cat <<-EOF >expect &&
 	% *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***
Range-diff against v3:
1:  a46767263f6 ! 1:  a8813b5f14c sequencer: comment checked-out branch properly
    @@ Commit message
     
     
      ## Notes (series) ##
    +    v4
    +    • Use `test_grep`
    +
    +      Link: https://lore.kernel.org/git/5267b9a9c8cc5cc66979117dc4c1e4d7329e2a03.1729704370.git.code@khaugsbakk.name/T/#me80519debcd013aa8c8a5e5003c58cff7281fac9
         v3:
         • Review feedback: check more in the test by inspecting the
           sequence editor
    @@ t/t3400-rebase.sh: test_expect_success 'rebase when inside worktree subdirectory
     +	git checkout topic2 &&
     +	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_grep "% Ref refs/heads/wt-topic checked out at" actual &&
    ++	test_grep "% Ref refs/heads/topic2 checked out at" actual
     +'
     +
      test_done
2:  7a452142666 ! 2:  4d10ad4ab55 sequencer: comment `--reference` subject line properly
    @@ Commit message
     
     
      ## Notes (series) ##
    +    v4:
    +    • Don’t need to cat(1)
    +      • Also use `-n4` in case `-4` is not widely supported
    +
    +      Link: https://lore.kernel.org/git/7739a6e2-8758-4d0f-b1d6-f0879a89590f@gmail.com/
         v3:
         • Review feedback: check more in the test by inspecting the
           proposed commit message.
    @@ 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_EDITOR="cat | head -4 >actual" git -c core.commentChar=% revert \
    ++	GIT_EDITOR="head -n4 >actual" git -c core.commentChar=% revert \
     +		--edit --reference HEAD &&
     +	cat <<-EOF >expect &&
     +	% *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***
3:  4c342bc0422 ! 3:  42b9fbd12d6 sequencer: comment commit messages properly
    @@ Commit message
             fixup hash2 <msg>
             fixup -c hash3 <msg>
     
    -    This says that hash2` and hash3 should be squashed into hash1 and
    +    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
    @@ Commit message
     
     
      ## Notes (series) ##
    +    v4:
    +    • Fix commit message (`)
    +
    +      Link: https://lore.kernel.org/git/5267b9a9c8cc5cc66979117dc4c1e4d7329e2a03.1729704370.git.code@khaugsbakk.name/T/#me80519debcd013aa8c8a5e5003c58cff7281fac9
         v3:
         • Message: Explain to the best of my knowledge what is going on here in
           the message body

base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94
-- 
2.47.0

[PATCH v4 1/3] sequencer: comment checked-out branch properly

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

`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`/
`core.commentString` is in use.

† 1: See 900b50c242 (rebase: add --update-refs option, 2022-07-19)

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v4
    • Use `test_grep`
    
      Link: https://lore.kernel.org/git/5267b9a9c8cc5cc66979117dc4c1e4d7329e2a03.1729704370.git.code@khaugsbakk.name/T/#me80519debcd013aa8c8a5e5003c58cff7281fac9
    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`)

 sequencer.c       |  5 +++--
 t/t3400-rebase.sh | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 353d804999b..1b6fd86f70b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
 		/* If the branch is checked out, then leave a comment instead. */
 		if ((path = branch_checked_out(decoration->name))) {
 			item->command = TODO_COMMENT;
-			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
-				    decoration->name, path);
+			strbuf_commented_addf(ctx->buf, comment_line_str,
+					      "Ref %s checked out at '%s'\n",
+					      decoration->name, path);
 		} else {
 			struct string_list_item *sti;
 			item->command = TODO_UPDATE_REF;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 09f230eefb2..7c47af6dcd9 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -456,4 +456,23 @@ test_expect_success 'rebase when inside worktree subdirectory' '
 	)
 '
 
+test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
+	test_when_finished git branch -D base topic2 &&
+	test_when_finished git checkout main &&
+	test_when_finished git branch -D wt-topic &&
+	test_when_finished git worktree remove wt-topic &&
+	git checkout main &&
+	git checkout -b base &&
+	git checkout -b topic2 &&
+	test_commit msg2 &&
+	git worktree add wt-topic &&
+	git checkout base &&
+	test_commit msg3 &&
+	git checkout topic2 &&
+	GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \
+		 rebase -i --update-refs base &&
+	test_grep "% Ref refs/heads/wt-topic checked out at" actual &&
+	test_grep "% Ref refs/heads/topic2 checked out at" actual
+'
+
 test_done
-- 
2.47.0

[PATCH v4 2/3] sequencer: comment `--reference` subject line properly

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

`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 <code@khaugsbakk.name>
---

Notes (series):
    v4:
    • Don’t need to cat(1)
      • Also use `-n4` in case `-4` is not widely supported
    
      Link: https://lore.kernel.org/git/7739a6e2-8758-4d0f-b1d6-f0879a89590f@gmail.com/
    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
       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(-)

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..b84fdfe8a32 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="head -n4 >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 &&
-- 
2.47.0

[PATCH v4 3/3] sequencer: comment commit messages properly

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

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):
    v4:
    • Fix commit message (`)
    
      Link: https://lore.kernel.org/git/5267b9a9c8cc5cc66979117dc4c1e4d7329e2a03.1729704370.git.code@khaugsbakk.name/T/#me80519debcd013aa8c8a5e5003c58cff7281fac9
    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

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(-)

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 &&
-- 
2.47.0

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

[Junio C Hamano]

kristofferhaugsbakk@fastmail.com writes:

> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> Fix three places where the comment char/string is hardcoded (#) in the
> todo list.
>
> § Changes in v4
>
> • Use `test_grep`
> • Fix commit message (`)
> • Don’t need to cat(1)
>   • Also use `-n4` in case `-4` is not widely supported

All changes make sense.  Will queue.

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

[Phillip Wood]

On 26/11/2024 01:11, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
> 
>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>
>> Fix three places where the comment char/string is hardcoded (#) in the
>> todo list.
>>
>> § Changes in v4
>>
>> • Use `test_grep`
>> • Fix commit message (`)
>> • Don’t need to cat(1)
>>    • Also use `-n4` in case `-4` is not widely supported
> 
> All changes make sense.  Will queue.

This version looks good to me too, thanks for working on it Kristoffer

Best Wishes

Phillip

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

[Kristoffer Haugsbakk]

On Tue, Nov 26, 2024, at 12:24, Phillip Wood wrote:
> On 26/11/2024 01:11, Junio C Hamano wrote:
>> kristofferhaugsbakk@fastmail.com writes:
>>
>>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>>
>>> Fix three places where the comment char/string is hardcoded (#) in the
>>> todo list.
>>>
>>> § Changes in v4
>>>
>>> • Use `test_grep`
>>> • Fix commit message (`)
>>> • Don’t need to cat(1)
>>>    • Also use `-n4` in case `-4` is not widely supported
>>
>> All changes make sense.  Will queue.
>
> This version looks good to me too, thanks for working on it Kristoffer

Thanks for the reviews and for finishing the last test coverage in this
series (fixup -c/-C). I was not familiar with that code as a user so I
didn’t know what was expected.

Cheers

-- 
Kristoffer