Re: [PATCH] sequencer: comment checked-out branch properly
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:
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) ```
