Re: [PATCH v3] revert: optionally refer to commit in the "reference" format

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2022, 21:52 UTC
Message-ID: <xmqqmtewoxu9.fsf@gitster.g>
In-Reply-To: <220601.86zgiwz9uk.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 17 quoted lines

> I think a good solution to that would be to e.g. emit:
>
>     # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***
>
>     Reverts commit <git reference>
>
>     This revert of a merge reverts changes made to <git reference 2>.
>
> Instead of what you have, which is:
>
>     # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***
>
>     This reverts commit <git reference>, reversing
>     changes made to <git reference 2>.
>
> It's sharing a bit less code between the two, but I think the message is
> suffering for it now.

We shouldn't be making it inconvenient for our primary intended audience.

The real first line is designed to be usable without editing for them. Those who forgets to write the title and ends up with "This reverts ..." as the title can still identify such a commit for the purpose of "rebase -i" and "commit --fixup", and that is good enough for them.

But your version will force the intended audience to remove the "Reverts ..." line, that strikes the balance at the wrong place.

Show 11 quoted lines

>> +test_expect_success 'identification of reverted commit (revert.reference)' '
>> +	git checkout --detach to-ident &&
>> +	git -c revert.reference=true revert --no-edit HEAD &&
>> +	git cat-file commit HEAD >actual.raw &&
>> +	grep "^This reverts " actual.raw >actual &&
>> +	echo "This reverts commit $(git show -s --pretty=reference HEAD^)." >expect &&
>> +	test_cmp expect actual
>
> Also (probably mentioned) I'd find this much easier to read/review if it
> was using test_cmp, now you need to carefully parse the code to see what
> the outputs are like exactly, but if we compared the full output...

We are using test_cmp. 'actual' has what we care about (i.e. what does the line that begin with "This reverts " say?) and compares it with what we expect to see in 'expect'.