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

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

On Sat, May 21 2022, Junio C Hamano wrote:

> A typical "git revert" commit uses the full title of the original
> commit in its title, and starts its body of the message with:
>
>     This reverts commit 8fa7f667cf61386257c00d6e954855cc3215ae91.
>
> This does not encourage the best practice of describing not just
> "what" (i.e. "Revert X" on the title says what we did) but "why"
> (i.e. and it does not say why X was undesirable).
>
> We can instead phrase this first line of the body to be more like
>
>     This reverts commit 8fa7f667 (do this and that, 2022-04-25)
>
> so that the title does not have to be
>
>     Revert "do this and that"
>
> We can instead use the title to describe "why" we are reverting the
> original commit.
>
> Introduce the "--reference" option to "git revert", and also the
> revert.reference configuration variable, which defaults to false, to
> tweak the title and the first line of the draft commit message for
> when creating a "revert" commit.

This is fantastic, I've wanted to fix this particular nit for a long
time.

> +static void refer_to_commit(struct replay_opts *opts,
> +			    struct strbuf *msgbuf, struct commit *commit)
> +{
> +	if (opts->commit_use_reference) {
> +		struct pretty_print_context ctx = {
> +			.abbrev = DEFAULT_ABBREV,
> +			.date_mode.type = DATE_SHORT,
> +		};
> +		format_commit_message(commit, "%h (%s, %ad)", msgbuf, &ctx);
> +	} else {
> +		strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));
> +	}
> +}
> +
>  static int do_pick_commit(struct repository *r,
>  			  struct todo_item *item,
>  			  struct replay_opts *opts,
> @@ -2167,14 +2184,19 @@ 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, "Revert \"");
> +			strbuf_addstr(&msgbuf, msg.subject);
> +			strbuf_addstr(&msgbuf, "\"");
> +		} else {
> +			strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE");

Having seen how people use git in the wild, this *WILL* result in a
bunch of history where people don't edit that at all.

If we want them to edit something we should come up with a buffer that's
empty except for suggested #-commented parts, and suggest that they
uncomment/edit some of it.

I also use "git revert --no-edit" in my own workflow, e.g. when
reverting back and forth in WIP code, having the OID is useful, having
this new "reference" would be even better, but having that just in the
body and "DESCRIBE WHY..." in the %s would be much less useful.

> +		}
> +		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);
>  		}
>  		strbuf_addstr(&msgbuf, ".\n");
>  	} else {

This is very POC, but I wish we had something like the WIP diff at the
end of this E-Mail (which is a mess, I was experimenting). I.e. we'd
always emit (when this setting is enabled)

    Revert %h (%s, %ad)

Except that the %s and the rest of the line would use the truncation
feature of "git log" formats, and would make sure the line isn't longer
than so-and-so, e.g. 72 or 50 characters.

Then only if we truncated things there, or we have parents to mention
would we add a body like:

    This reverts commit %h (%s, %ad)

Followed by "reversing changes..."

All of which would be much easier than the below monstrosity I came up
with if:

 1. You could ask "git log" formats to not whitespace pad the
    truncation, I couldn't find a way to do that (but it's the first
    time I use it).

 2. We just had some generic "wrap this buffer" function, maybe we do, I
    didn't look much.

> diff --git a/sequencer.h b/sequencer.h
> index da64473636..698599fe4e 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -49,6 +49,7 @@ struct replay_opts {
>  	int reschedule_failed_exec;
>  	int committer_date_is_author_date;
>  	int ignore_date;
> +	int commit_use_reference;
>  
>  	int mainline;
>  
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 8617efaaf1..36f9565b92 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -159,6 +159,7 @@ test_expect_success 'cherry-pick works with dirty renamed file' '
>  '
>  
>  test_expect_success 'advice from failed revert' '
> +	test_when_finished "git reset --hard" &&
>  	test_commit --no-tag "add dream" dream dream &&
>  	dream_oid=$(git rev-parse --short HEAD) &&
>  	cat <<-EOF >expected &&
> @@ -174,4 +175,34 @@ test_expect_success 'advice from failed revert' '
>  	test_must_fail git revert HEAD^ 2>actual &&
>  	test_cmp expected actual
>  '
> +
> +test_expect_success 'identification of reverted commit (vanilla)' '
> +	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 &&
> +	test_cmp expect actual
> +'
> +
> +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 &&

This hides the exit code of the embedded $() git command.

Re the suggested changes below: let's just test_cmp the whole thing,
subject+body instead?

> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'identification of reverted commit (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
> +'
> +
>  test_done


diff --git a/sequencer.c b/sequencer.c
index 4abb77add09..a43444e2339 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2061,18 +2061,14 @@ static int should_edit(struct replay_opts *opts) {
 	return opts->edit;
 }
 
-static void refer_to_commit(struct replay_opts *opts,
-			    struct strbuf *msgbuf, struct commit *commit)
-{
-	if (opts->commit_use_reference) {
-		struct pretty_print_context ctx = {
-			.abbrev = DEFAULT_ABBREV,
-			.date_mode.type = DATE_SHORT,
-		};
-		format_commit_message(commit, "%h (%s, %ad)", msgbuf, &ctx);
-	} else {
-		strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));
-	}
+static void refer_to_commit(struct strbuf *sb, struct commit *commit,
+			    const char *const fmt)
+{
+	struct pretty_print_context ctx = {
+		.abbrev = DEFAULT_ABBREV,
+		.date_mode.type = DATE_SHORT,
+	};
+	format_commit_message(commit, fmt, sb, &ctx);
 }
 
 static int do_pick_commit(struct repository *r,
@@ -2183,19 +2179,63 @@ static int do_pick_commit(struct repository *r,
 		base_label = msg.label;
 		next = parent;
 		next_label = msg.parent_label;
-		if (!opts->commit_use_reference) {
+		if (opts->commit_use_reference) {
+			struct strbuf sb_b = STRBUF_INIT;
+			struct strbuf sb_h = STRBUF_INIT;
+			struct strbuf sb_ad = STRBUF_INIT;
+			struct strbuf sb_s = STRBUF_INIT;
+			struct strbuf sb_s_t = STRBUF_INIT;
+			const size_t wraplen = 72;
+			char *fmt;
+			size_t len = wraplen;
+			int truncated;
+
+			refer_to_commit(&sb_h, commit, "Revert %h (");
+			len -= sb_h.len;
+			refer_to_commit(&sb_ad, commit, "%ad)");
+			len -= sb_ad.len;
+			len -= 2 ; /* The ", " in: Revert %h (%s, %ad) */
+			/* vanilla %h (%s, %ad) */
+			refer_to_commit(&sb_s, commit, "%s");
+
+			/* truncated %h (%s, %ad) */
+			fmt = xstrfmt("%%<(%d,trunc)%%s", (int)len);
+			refer_to_commit(&sb_s_t, commit, fmt);
+			free(fmt);
+			strbuf_rtrim(&sb_s_t);
+
+			truncated = strcmp(sb_s.buf, sb_s_t.buf);
+
+			strbuf_addbuf(&msgbuf, &sb_h);
+			strbuf_addbuf(&msgbuf, &sb_s_t);
+			strbuf_addbuf(&msgbuf, &sb_ad);
+			strbuf_addstr(&msgbuf, "\n\n");
+
+			if (truncated) {
+				const char *pfx = "This reverts commit ";
+				size_t len = wraplen - strlen(pfx); /* TODO: Consider %h etc. */
+				char *fmt;
+				
+				fmt = xstrfmt("%s%%h (%%w(%d,0,0)%%s, %%ad)",
+					      pfx, (int)len);
+				refer_to_commit(&sb_b, commit, fmt);
+				free(fmt);
+				strbuf_addbuf(&msgbuf, &sb_b);
+			} else {
+				/* TODO: The not-truncated version ... */
+			}
+		} else {
 			strbuf_addstr(&msgbuf, "Revert \"");
 			strbuf_addstr(&msgbuf, msg.subject);
 			strbuf_addstr(&msgbuf, "\"");
-		} else {
-			strbuf_addstr(&msgbuf, "DESCRIBE WHY WE ARE REVERTING HERE");
-		}
-		strbuf_addstr(&msgbuf, "\n\nThis reverts commit ");
-		refer_to_commit(opts, &msgbuf, commit);
+			strbuf_addstr(&msgbuf, "\n\nThis reverts commit ");
+			strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
 
-		if (commit->parents && commit->parents->next) {
-			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
-			refer_to_commit(opts, &msgbuf, parent);
+			if (commit->parents && commit->parents->next) {
+				/* TODO: Handle the wrapping of this too... */
+				strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
+				strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));
+			}
 		}
 		strbuf_addstr(&msgbuf, ".\n");
 	} else {