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

From: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Date: Oct 23, 2024, 19:53 UTC
Message-ID: <20cca28c-cc0f-4bfa-bc1c-6a3dd6bc25a8@app.fastmail.com>
In-Reply-To: <ZxlEJ+44M8z03VOj@nand.local>

On Wed, Oct 23, 2024, at 20:44, Taylor Blau wrote:

Show 6 quoted lines

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

Show 22 quoted lines

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

Show 19 quoted lines

>> […]
>> +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:

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

edited 1

Signed-off-by: Rebase Committer <rebase.committer@example.com>

% This is the commit message #4:

% amend! amend! 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

edited 1

edited 2

edited 3 squashed ok 13 - sequence of fixup, fixup -C & squash --signoff works (but with commentChar) ```