| 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!
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 >
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
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) > ``` >
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
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?
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 &&
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