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

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

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

> 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.

>> +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'.