Re: [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 11, 2023, 16:59 UTC
- Message-ID
- <xmqq1qg9v7k9.fsf@gitster.g>
- In-Reply-To
- <dba3f15a-3575-e4f9-2291-c5a342cfed43@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
Show 17 quoted lines
> 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.
Show 25 quoted lines
>> 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.
Show 23 quoted lines
>> 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
Show 7 quoted lines
>> +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" &&