| Message ID | 5267b9a9c8cc5cc66979117dc4c1e4d7329e2a03.1729704370.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sequencer: comment checked-out branch properly | expand |
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
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) ```
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
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!
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