| Message ID | 10598a56d64f5c2b4d8d05d7e7b09a18ef254f88.1698728953.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: remove use of hardcoded comment char | expand |
Hi Tony On 31/10/2023 05:09, Tony Tung via GitGitGadget wrote: > From: Tony Tung <tonytung@merly.org> > > Instead of using the hardcoded `# `, use the > user-defined comment_line_char. Adds a test > to prevent regressions. Well spotted and thanks for fixing this. Normally we wrap the commit message at ~72 chars. > Signed-off-by: Tony Tung <tonytung@merly.org> > --- > sequencer.c | 5 +++-- > t/t3404-rebase-interactive.sh | 39 +++++++++++++++++++++++++++++++++++ > 2 files changed, 42 insertions(+), 2 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index d584cac8ed9..8c6666d5e43 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -6082,8 +6082,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_char, > + "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/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 8ea2bf13026..076dca87871 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1839,6 +1839,45 @@ test_expect_success '--update-refs adds label and update-ref commands' ' > ) > ' Thank you for taking the time to add a test. I think it could be simplified though as all we really need to do is check that the expected comment is present in the todo list. Something like (untested) test_expect_success '--update-refs works with core.commentChar' ' git worktree add new-branch && test_when_finished "git worktree remove new-branch" && test_config core.commentchar : && write_script fake-editor.sh <<-\EOF && grep "^: Ref refs/heads/new-branch checked out at .*new-branch" "$1" && # no need to rebase >"$1" EOF ( test_set_editor "$(pwd)/fake-editor.sh" && git rebase -i --update-refs HEAD^ ) ' Best Wishes Phillip > +test_expect_success '--update-refs works with core.commentChar' ' > + git checkout -b update-refs-with-commentchar no-conflict-branch && > + test_config core.commentChar : && > + git branch -f base HEAD~4 && > + git branch -f first HEAD~3 && > + git branch -f second HEAD~3 && > + git branch -f third HEAD~1 && > + git commit --allow-empty --fixup=third && > + git branch -f is-not-reordered && > + git commit --allow-empty --fixup=HEAD~4 && > + git branch -f shared-tip && > + git checkout update-refs && > + ( > + write_script fake-editor.sh <<-\EOF && > + grep "^[^:]" "$1" > + exit 1 > + EOF > + test_set_editor "$(pwd)/fake-editor.sh" && > + > + cat >expect <<-EOF && > + pick $(git log -1 --format=%h J) J > + fixup $(git log -1 --format=%h update-refs) fixup! J : empty > + update-ref refs/heads/second > + update-ref refs/heads/first > + pick $(git log -1 --format=%h K) K > + pick $(git log -1 --format=%h L) L > + fixup $(git log -1 --format=%h is-not-reordered) fixup! L : empty > + update-ref refs/heads/third > + pick $(git log -1 --format=%h M) M > + update-ref refs/heads/no-conflict-branch > + update-ref refs/heads/is-not-reordered > + update-ref refs/heads/update-refs-with-commentchar > + EOF > + > + test_must_fail git rebase -i --autosquash --update-refs primary shared-tip >todo && > + test_cmp expect todo > + ) > +' > + > test_expect_success '--update-refs adds commands with --rebase-merges' ' > git checkout -b update-refs-with-merge no-conflict-branch && > git branch -f base HEAD~4 &&
"Tony Tung via GitGitGadget" <gitgitgadget@gmail.com> writes: > Subject: Re: [PATCH v2 1/2] sequencer: remove use of comment character The patch does not seem to be doing that, though. It may have removed '#' in "# Ref", but still uses comment_line_char, so it does not remove use at all (and we do not want to, of course). "use the core.commentchar consistently" > From: Tony Tung <tonytung@merly.org> > > Instead of using the hardcoded `# `, use the > user-defined comment_line_char. Adds a test > to prevent regressions. Overly short lines. The readers cannot tell where in the output the hardcoded # appears with the above description. I am guessing that it is in comments in the sequencer/todo file that mark commits that are at the tip of branches that are checked out, but there may be more specific circumstances in which the comment is used, like "when rebase -i is used with the --update-refs option", if so that also need to be told to the readers. Describe the condition well enough so that readers can easily see the defect the patch attempts to fix. > - strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n", > - decoration->name, path); > + strbuf_commented_addf(ctx->buf, comment_line_char, > + "Ref %s checked out at '%s'\n", > + decoration->name, path); OK.
diff --git a/sequencer.c b/sequencer.c index d584cac8ed9..8c6666d5e43 100644 --- a/sequencer.c +++ b/sequencer.c @@ -6082,8 +6082,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_char, + "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/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 8ea2bf13026..076dca87871 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1839,6 +1839,45 @@ test_expect_success '--update-refs adds label and update-ref commands' ' ) ' +test_expect_success '--update-refs works with core.commentChar' ' + git checkout -b update-refs-with-commentchar no-conflict-branch && + test_config core.commentChar : && + git branch -f base HEAD~4 && + git branch -f first HEAD~3 && + git branch -f second HEAD~3 && + git branch -f third HEAD~1 && + git commit --allow-empty --fixup=third && + git branch -f is-not-reordered && + git commit --allow-empty --fixup=HEAD~4 && + git branch -f shared-tip && + git checkout update-refs && + ( + write_script fake-editor.sh <<-\EOF && + grep "^[^:]" "$1" + exit 1 + EOF + test_set_editor "$(pwd)/fake-editor.sh" && + + cat >expect <<-EOF && + pick $(git log -1 --format=%h J) J + fixup $(git log -1 --format=%h update-refs) fixup! J : empty + update-ref refs/heads/second + update-ref refs/heads/first + pick $(git log -1 --format=%h K) K + pick $(git log -1 --format=%h L) L + fixup $(git log -1 --format=%h is-not-reordered) fixup! L : empty + update-ref refs/heads/third + pick $(git log -1 --format=%h M) M + update-ref refs/heads/no-conflict-branch + update-ref refs/heads/is-not-reordered + update-ref refs/heads/update-refs-with-commentchar + EOF + + test_must_fail git rebase -i --autosquash --update-refs primary shared-tip >todo && + test_cmp expect todo + ) +' + test_expect_success '--update-refs adds commands with --rebase-merges' ' git checkout -b update-refs-with-merge no-conflict-branch && git branch -f base HEAD~4 &&