| Message ID | 20230821170720.577850-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] sequencer: beautify subject of reverts of reverts | expand |
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > Instead of generating a silly-looking `Revert "Revert "foo""`, make it > a more humane `Reapply "foo"`. Looking good. Will requeue. Thanks.
On Mon, Aug 21, 2023 at 07:07:19PM +0200, Oswald Buddenhagen wrote: > To achieve these goals, the mechanism does not need to be particularly > sophisticated. Therefore, more complicated alternatives which would > "compress more efficiently" have not been implemented. This version is looking good. The main functionality is well-reasoned and straightforwardly implemented. One minor suggestion that you could consider squashing in is some test clean-up like so: --- 8< --- diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index 7011e3a421..4dee71d6d5 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -176,29 +176,27 @@ test_expect_success 'advice from failed revert' ' test_cmp expected actual ' +test_expect_commit_msg () { + echo "$@" >expect && + git log -1 --pretty=%s >actual && + test_cmp expect actual +} + test_expect_success 'title of fresh reverts' ' test_commit --no-tag A file1 && test_commit --no-tag B file1 && git revert --no-edit HEAD && - echo "Revert \"B\"" >expect && - git log -1 --pretty=%s >actual && - test_cmp expect actual && + test_expect_commit_msg "Revert \"B\"" && git revert --no-edit HEAD && - echo "Reapply \"B\"" >expect && - git log -1 --pretty=%s >actual && - test_cmp expect actual && + test_expect_commit_msg "Reapply \"B\"" && git revert --no-edit HEAD && - echo "Revert \"Reapply \"B\"\"" >expect && - git log -1 --pretty=%s >actual && - test_cmp expect actual + test_expect_commit_msg "Revert \"Reapply \"B\"\"" ' test_expect_success 'title of legacy double revert' ' test_commit --no-tag "Revert \"Revert \"B\"\"" file1 && git revert --no-edit HEAD && - echo "Revert \"Revert \"Revert \"B\"\"\"" >expect && - git log -1 --pretty=%s >actual && - test_cmp expect actual + test_expect_commit_msg "Revert \"Revert \"Revert \"B\"\"\"" ' test_expect_success 'identification of reverted commit (default)' ' --- >8 --- To my eyes, it makes checking the subject of our revert commit against an expected value more readable by factoring out the echo, git log, test_cmp pattern. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > This version is looking good. The main functionality is well-reasoned > and straightforwardly implemented. One minor suggestion that you could > consider squashing in is some test clean-up like so: > > --- 8< --- > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh > index 7011e3a421..4dee71d6d5 100755 > --- a/t/t3501-revert-cherry-pick.sh > +++ b/t/t3501-revert-cherry-pick.sh > @@ -176,29 +176,27 @@ test_expect_success 'advice from failed revert' ' > test_cmp expected actual > ' > > +test_expect_commit_msg () { > + echo "$@" >expect && > + git log -1 --pretty=%s >actual && > + test_cmp expect actual > +} > + > test_expect_success 'title of fresh reverts' ' > test_commit --no-tag A file1 && > test_commit --no-tag B file1 && > git revert --no-edit HEAD && > - echo "Revert \"B\"" >expect && > - git log -1 --pretty=%s >actual && > - test_cmp expect actual && > + test_expect_commit_msg "Revert \"B\"" && > git revert --no-edit HEAD && > - echo "Reapply \"B\"" >expect && > - git log -1 --pretty=%s >actual && > - test_cmp expect actual && > + test_expect_commit_msg "Reapply \"B\"" && > git revert --no-edit HEAD && > - echo "Revert \"Reapply \"B\"\"" >expect && > - git log -1 --pretty=%s >actual && > - test_cmp expect actual > + test_expect_commit_msg "Revert \"Reapply \"B\"\"" > ' > > test_expect_success 'title of legacy double revert' ' > test_commit --no-tag "Revert \"Revert \"B\"\"" file1 && > git revert --no-edit HEAD && > - echo "Revert \"Revert \"Revert \"B\"\"\"" >expect && > - git log -1 --pretty=%s >actual && > - test_cmp expect actual > + test_expect_commit_msg "Revert \"Revert \"Revert \"B\"\"\"" > ' > > test_expect_success 'identification of reverted commit (default)' ' > --- >8 --- > > To my eyes, it makes checking the subject of our revert commit against > an expected value more readable by factoring out the echo, git log, > test_cmp pattern. Yeah it does make the test more concise and what is expected stand out more clearly. Good suggestion.
On Wed, Aug 23, 2023 at 02:38:36PM -0700, Junio C Hamano wrote: >Taylor Blau <me@ttaylorr.com> writes: > >> This version is looking good. The main functionality is well-reasoned >> and straightforwardly implemented. One minor suggestion that you could >> consider squashing in is some test clean-up like so: >> > >Yeah it does make the test more concise and what is expected stand >out more clearly. Good suggestion. > agreed. do you want to squash it on your end, or should i reroll? regards
diff --git a/sequencer.c b/sequencer.c index cc9821ece2..12ec158922 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2249,13 +2249,24 @@ static int do_pick_commit(struct repository *r, */ if (command == TODO_REVERT) { + const char *orig_subject; + base = commit; base_label = msg.label; next = parent; next_label = msg.parent_label; if (opts->commit_use_reference) { strbuf_addstr(&msgbuf, "# *** 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 + * theoretically these can be nested arbitrarily deeply, + * thus requiring excessive complexity to deal with. + */ + !starts_with(orig_subject, "Revert \"")) { + strbuf_addstr(&msgbuf, "Reapply \""); + strbuf_addstr(&msgbuf, orig_subject); } else { strbuf_addstr(&msgbuf, "Revert \""); strbuf_addstr(&msgbuf, msg.subject); diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index e2ef619323..7011e3a421 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -176,6 +176,31 @@ test_expect_success 'advice from failed revert' ' test_cmp expected actual ' +test_expect_success 'title of fresh reverts' ' + test_commit --no-tag A file1 && + test_commit --no-tag B file1 && + git revert --no-edit HEAD && + echo "Revert \"B\"" >expect && + git log -1 --pretty=%s >actual && + test_cmp expect actual && + git revert --no-edit HEAD && + echo "Reapply \"B\"" >expect && + git log -1 --pretty=%s >actual && + test_cmp expect actual && + git revert --no-edit HEAD && + echo "Revert \"Reapply \"B\"\"" >expect && + git log -1 --pretty=%s >actual && + test_cmp expect actual +' + +test_expect_success 'title of legacy double revert' ' + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 && + git revert --no-edit HEAD && + echo "Revert \"Revert \"Revert \"B\"\"\"" >expect && + git log -1 --pretty=%s >actual && + test_cmp expect actual +' + test_expect_success 'identification of reverted commit (default)' ' test_commit to-ident && test_when_finished "git reset --hard to-ident" &&
Instead of generating a silly-looking `Revert "Revert "foo""`, make it a more humane `Reapply "foo"`. This is done for two reasons: - To cover the actually common case of just a double revert. - To encourage people to rewrite summaries of recursive reverts by setting an example (a subsequent commit will also do this explicitly in the documentation). To achieve these goals, the mechanism does not need to be particularly sophisticated. Therefore, more complicated alternatives which would "compress more efficiently" have not been implemented. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- v3: - capitulate at first sight of a pre-existing recursive reversion, as handling the edge cases is a bottomless pit - reworked commit message again - moved test into existing file - generalized docu change and factored it out v2: - add discussion to commit message - add paragraph to docu - add test - use skip_prefix() instead of starts_with() - catch pre-existing double reverts Cc: Junio C Hamano <gitster@pobox.com> Cc: Kristoffer Haugsbakk <code@khaugsbakk.name> Cc: Phillip Wood <phillip.wood123@gmail.com> --- sequencer.c | 11 +++++++++++ t/t3501-revert-cherry-pick.sh | 25 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+)