Re: [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts

[Junio C Hamano <gitster@pobox.com>]

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" &&