| Message ID | 710c5b1a3f6bf8dc112ff13f27a8b2165274488d.1731406513.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sequencer: comment out properly in todo list | expand |
kristofferhaugsbakk@fastmail.com writes: > @@ -2341,8 +2341,8 @@ static int do_pick_commit(struct repository *r, > next = parent; > next_label = msg.parent_label; > if (opts->commit_use_reference) { > - strbuf_addstr(&ctx->message, > - "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); > + strbuf_commented_addf(&ctx->message, comment_line_str, > + "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); With the switch to "commented_addf", we'd terminate this line with LF here, which means ... > @@ -2352,12 +2352,13 @@ static int do_pick_commit(struct repository *r, > !starts_with(orig_subject, "Revert \"")) { > strbuf_addstr(&ctx->message, "Reapply \""); > strbuf_addstr(&ctx->message, orig_subject); > + strbuf_addstr(&ctx->message, "\n"); > } else { > strbuf_addstr(&ctx->message, "Revert \""); > strbuf_addstr(&ctx->message, msg.subject); > - strbuf_addstr(&ctx->message, "\""); > + strbuf_addstr(&ctx->message, "\"\n"); ... we'd want to terminate the line in these two other if/else if/else arms for symmetry, so that ... > } > - strbuf_addstr(&ctx->message, "\n\nThis reverts commit "); > + strbuf_addstr(&ctx->message, "\nThis reverts commit "); ... we can lose the termination of the previous line from here. Makes sense. > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh > index 411027fb58c..26d3cabb608 100755 > --- a/t/t3501-revert-cherry-pick.sh > +++ b/t/t3501-revert-cherry-pick.sh > @@ -228,6 +228,18 @@ test_expect_success 'identification of reverted commit (--reference)' ' > test_cmp expect actual > ' > > +test_expect_success 'git revert --reference with core.commentChar' ' > + test_when_finished "git reset --hard to-ident" && > + git checkout --detach to-ident && > + git -c core.commentChar=% revert \ > + --edit --reference HEAD && > + git log -1 --format=%B HEAD >actual && > + printf "This reverts commit $(git show -s \ > + --pretty=reference HEAD^).\n\n" \ > + >expect && > + test_cmp expect actual > +' I guess this fails by leaving the "# *** SAY WHY" in the resulting message, because the stripspace wants to see '%' to start commented out lines to be stripped? If we inspect with this test what the temporary file we give to the editor looks like to make sure that '%' is used for commenting, that would be a more direct test, but without going that far, at least can we have a comment describing how this is expected to fail without the fix? Thanks.
On 13/11/2024 01:07, Junio C Hamano wrote: > kristofferhaugsbakk@fastmail.com writes: > >> +test_expect_success 'git revert --reference with core.commentChar' ' >> + test_when_finished "git reset --hard to-ident" && >> + git checkout --detach to-ident && >> + git -c core.commentChar=% revert \ >> + --edit --reference HEAD && >> + git log -1 --format=%B HEAD >actual && >> + printf "This reverts commit $(git show -s \ >> + --pretty=reference HEAD^).\n\n" \ >> + >expect && >> + test_cmp expect actual >> +' > > I guess this fails by leaving the "# *** SAY WHY" in the resulting > message, because the stripspace wants to see '%' to start commented > out lines to be stripped? If we inspect with this test what the > temporary file we give to the editor looks like to make sure that > '%' is used for commenting, that would be a more direct test, but > without going that far, at least can we have a comment describing > how this is expected to fail without the fix? For me something like GIT_EDITOR="cat >actual" git -c core.commentChar=% revert \ --edit --reference HEAD && test_grep "^% \*\*\* SAY WHY WE ARE REVERTING THE COMMIT \*\*\*" \ actual Would be a more convincing test as it actually checks that the user sees the line that we expect strbuf_stripspace() to remove from the final message. If we want to check the commit message as well that's fine but I'm not sure its necessary. (if we do we should use test_commit_message like patch 3) Best Wishes Phillip
phillip.wood123@gmail.com writes: >> I guess this fails by leaving the "# *** SAY WHY" in the resulting >> message, because the stripspace wants to see '%' to start commented >> out lines to be stripped? If we inspect with this test what the >> temporary file we give to the editor looks like to make sure that >> '%' is used for commenting, that would be a more direct test, but >> without going that far, at least can we have a comment describing >> how this is expected to fail without the fix? > > For me something like > > GIT_EDITOR="cat >actual" git -c core.commentChar=% revert \ > --edit --reference HEAD && > test_grep "^% \*\*\* SAY WHY WE ARE REVERTING THE COMMIT \*\*\*" \ > actual > > Would be a more convincing test as it actually checks that the user sees > the line that we expect strbuf_stripspace() to remove from the final > message. Yes, it is a more direct test. If we did that, that would of course be preferrable. Thanks.
diff --git a/sequencer.c b/sequencer.c index 1b6fd86f70b..d26299cdea2 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2341,8 +2341,8 @@ static int do_pick_commit(struct repository *r, next = parent; next_label = msg.parent_label; if (opts->commit_use_reference) { - strbuf_addstr(&ctx->message, - "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); + strbuf_commented_addf(&ctx->message, comment_line_str, + "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***"); } else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) && /* * We don't touch pre-existing repeated reverts, because @@ -2352,12 +2352,13 @@ static int do_pick_commit(struct repository *r, !starts_with(orig_subject, "Revert \"")) { strbuf_addstr(&ctx->message, "Reapply \""); strbuf_addstr(&ctx->message, orig_subject); + strbuf_addstr(&ctx->message, "\n"); } else { strbuf_addstr(&ctx->message, "Revert \""); strbuf_addstr(&ctx->message, msg.subject); - strbuf_addstr(&ctx->message, "\""); + strbuf_addstr(&ctx->message, "\"\n"); } - strbuf_addstr(&ctx->message, "\n\nThis reverts commit "); + strbuf_addstr(&ctx->message, "\nThis reverts commit "); refer_to_commit(opts, &ctx->message, commit); if (commit->parents && commit->parents->next) { diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index 411027fb58c..26d3cabb608 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -228,6 +228,18 @@ test_expect_success 'identification of reverted commit (--reference)' ' test_cmp expect actual ' +test_expect_success 'git revert --reference with core.commentChar' ' + test_when_finished "git reset --hard to-ident" && + git checkout --detach to-ident && + git -c core.commentChar=% revert \ + --edit --reference HEAD && + git log -1 --format=%B HEAD >actual && + printf "This reverts commit $(git show -s \ + --pretty=reference HEAD^).\n\n" \ + >expect && + test_cmp expect actual +' + test_expect_success 'identification of reverted commit (revert.reference)' ' git checkout --detach to-ident && git -c revert.reference=true revert --no-edit HEAD &&