| Message ID | 20230902072035.652549-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] sequencer: beautify subject of reverts of reverts | expand |
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > --- > v4: > - factor out verification of subject as per taylor's patch, with minor > modifications. The change seems to make the test quite straight-forward to read. Let's mark the topic for 'next'. Thanks. > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh > index e2ef619323..4158590322 100755 > --- a/t/t3501-revert-cherry-pick.sh > +++ b/t/t3501-revert-cherry-pick.sh > @@ -176,6 +176,29 @@ test_expect_success 'advice from failed revert' ' > test_cmp expected actual > ' > > +test_expect_subject () { > + echo "$1" >expect && > + git log -1 --pretty=%s >actual && > + test_cmp expect actual > +} > + > +test_expect_success 'titles of fresh reverts' ' > + test_commit --no-tag A file1 && > + test_commit --no-tag B file1 && > + git revert --no-edit HEAD && > + test_expect_subject "Revert \"B\"" && > + git revert --no-edit HEAD && > + test_expect_subject "Reapply \"B\"" && > + git revert --no-edit HEAD && > + test_expect_subject "Revert \"Reapply \"B\"\"" > +' > + > +test_expect_success 'title of legacy double revert' ' > + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 && > + git revert --no-edit HEAD && > + test_expect_subject "Revert \"Revert \"Revert \"B\"\"\"" > +' > + > test_expect_success 'identification of reverted commit (default)' ' > test_commit to-ident && > test_when_finished "git reset --hard to-ident" &&
On Sat, Sep 2, 2023, at 09:20, Oswald Buddenhagen wrote: > Instead of generating a silly-looking `Revert "Revert "foo""`, make it > a more humane `Reapply "foo"`. Congrats on a nice series. It's very “lean and mean”—focused, not excessive. And I think I will remember the phrase “too nerdy” for a while. ;) Maybe we will get this message template the next time we revert a merge.[1] > If you merge the updated side branch (with D at its tip), none of the > changes made in A or B will be in the result, because they were reverted > by W. That is what Alan saw. > > [...] > > In such a situation, you would want to first revert the previous revert, > which would make the history look like this: ...
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..4158590322 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -176,6 +176,29 @@ test_expect_success 'advice from failed revert' ' test_cmp expected actual ' +test_expect_subject () { + echo "$1" >expect && + git log -1 --pretty=%s >actual && + test_cmp expect actual +} + +test_expect_success 'titles of fresh reverts' ' + test_commit --no-tag A file1 && + test_commit --no-tag B file1 && + git revert --no-edit HEAD && + test_expect_subject "Revert \"B\"" && + git revert --no-edit HEAD && + test_expect_subject "Reapply \"B\"" && + git revert --no-edit HEAD && + test_expect_subject "Revert \"Reapply \"B\"\"" +' + +test_expect_success 'title of legacy double revert' ' + test_commit --no-tag "Revert \"Revert \"B\"\"" file1 && + git revert --no-edit HEAD && + test_expect_subject "Revert \"Revert \"Revert \"B\"\"\"" +' + 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> --- v4: - factor out verification of subject as per taylor's patch, with minor modifications. fwiw, it might make sense to put this into test-lib-functions.sh right after test_commit_message(), then named test_commit_subject(). not sure it would be worth it, given an equally generic implementation would be kinda over-engineered, and the discoverability is kinda poor. 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 | 23 +++++++++++++++++++++++ 2 files changed, 34 insertions(+)