| Message ID | 20230809171531.2564807-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] sequencer: beautify subject of reverts of reverts | expand |
Hi Oswald On 09/08/2023 18:15, Oswald Buddenhagen wrote: > 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. This all looks good to me, it seems quite sensible just to bail out if we see an existing recursive reversion. I'm not suggesting you change these tests but for future reference we now have a test_commit_message() function which was merged a few days ago to simplify tests like these. Thanks for working on it Phillip > 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(+) > > 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" &&
Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Oswald > > On 09/08/2023 18:15, Oswald Buddenhagen wrote: >> 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. > > This all looks good to me, it seems quite sensible just to bail out if > we see an existing recursive reversion. Yes, explicitly refraining from becoming overly cute is a good design decision. >> 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); Being simple-and-stupid to deal only with the most common case, and documenting that it is deliberate that we do not deal with more complex cases in the in-code comment and in the log message, are very good in this case. >> 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 >> +' Presumably the next time this gets reverted we will see a doubled reapply? Isn't that something we care about documenting as a part of this test? i.e. another four-line block after the above? git revert --no-edit HEAD && echo "Reapply \"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 >> +' Good. >> test_expect_success 'identification of reverted commit (default)' ' >> test_commit to-ident && >> test_when_finished "git reset --hard to-ident" &&
On Fri, Aug 11, 2023 at 09:59:34AM -0700, Junio C Hamano wrote: >Presumably the next time this gets reverted we will see a doubled >reapply? > yes >Isn't that something we care about documenting as a part >of this test? i.e. another four-line block after the above? > the third case documents that it's the same as the first case, i.e., "nothing special". so at this point we have full coverage in all regards. going beyond that would be redundant, and we'd again get into the "uh, where do we stop?" situation. 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(+)