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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, May 26 2022, Junio C Hamano wrote:

> When this option is in use, the first line of the pre-filled editor
> buffer becomes a comment line that tells the user to say _why_.  If
> the user exits the editor without touching this line by mistake,
> what we prepare to become the first line of the body, i.e. "This
> reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to
> be the title of the resulting commit.  This behaviour is designed to
> help such a user to identify such a revert in "git log --oneline"
> easily so that it can be further reworded with "git rebase -i" later.

This is a good trade-off, and means that the --no-edit case is also
preserved. However...

> @@ -2167,14 +2184,20 @@ static int do_pick_commit(struct repository *r,
>  		base_label = msg.label;
>  		next = parent;
>  		next_label = msg.parent_label;
> -		strbuf_addstr(&msgbuf, "Revert \"");
> -		strbuf_addstr(&msgbuf, msg.subject);
> -		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
> -		strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
> +		if (opts->commit_use_reference) {
> +			strbuf_addstr(&msgbuf,
> +				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> +		} else {
> +			strbuf_addstr(&msgbuf, "Revert \"");
> +			strbuf_addstr(&msgbuf, msg.subject);
> +			strbuf_addstr(&msgbuf, "\"");
> +		}
> +		strbuf_addstr(&msgbuf, "\n\nThis reverts commit ");
> +		refer_to_commit(opts, &msgbuf, commit);
>  
>  		if (commit->parents && commit->parents->next) {
>  			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
> -			strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));
> +			refer_to_commit(opts, &msgbuf, parent);
>  		}

...the way this is implemented means that we end up with a much more
verbose subject line in the case of reverts, which doesn't seem to be
intended (or at least not called out in the commit message).

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.

I.e. the "Revert <reference>" change to "This reverts commit
<reference>" doesn't per-se seem intentional, but just a side-effect of
the optional "reference" revert's default "subject" line piggy-backing
on the non-"reference" body.

> +test_expect_success 'identification of reverted commit (default)' '
> +	test_commit to-ident &&
> +	test_when_finished "git reset --hard to-ident" &&
> +	git checkout --detach to-ident &&
> +	git revert --no-edit HEAD &&
> +	git cat-file commit HEAD >actual.raw &&
> +	grep "^This reverts " actual.raw >actual &&
> +	echo "This reverts commit $(git rev-parse HEAD^)." >expect &&

This pattern hides git exit codes & segfaults (I don't remember if I
mentioned that in a previous round, I think so...).

> +test_expect_success 'identification of reverted commit (--reference)' '
> +	git checkout --detach to-ident &&
> +	git revert --reference --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
> +'
> +
> +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...