Re: [PATCH v2 2/2] replay: add --revert mode to reverse commit changes

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Siddarth

I agree with Patrick's comments, I've added a few more of my own below

On 02/12/2025 20:16, Siddharth Asthana wrote:
> The `git replay` command performs server-side history rewriting without
> requiring a working tree. While it currently supports cherry-picking
> commits (--advance) and rebasing (--onto), it lacks the ability to
> revert them.
> 
> At GitLab, we use replay in Gitaly for efficient server-side operations
> on bare repositories. Adding revert functionality enables us to reverse
> problematic commits directly on the server, eliminating client-side
> roundtrips and reducing network overhead.
> 
> Add a `--revert <branch>` mode that reverses the changes introduced by
> the specified commits. Following the architecture of --onto and --advance,
> --revert is a standalone mode that takes a branch argument and updates
> that branch with the revert commits.

s/revert/reverted/?

> diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
> index dcb26e8a8e..eb297c7530 100644
> --- a/Documentation/git-replay.adoc
> +++ b/Documentation/git-replay.adoc
> @@ -9,7 +9,7 @@ git-replay - EXPERIMENTAL: Replay commits on a new base, works with bare repos t
>   SYNOPSIS
>   --------
>   [verse]
> -(EXPERIMENTAL!) 'git replay' ([--contained] --onto <newbase> | --advance <branch>) [--ref-action[=<mode>]] <revision-range>...
> +(EXPERIMENTAL!) 'git replay' ([--contained] --onto <newbase> | --advance <branch> | --revert <branch>) [--ref-action[=<mode>]] <revision-range>...
>   
>   DESCRIPTION
>   -----------
> @@ -42,6 +42,24 @@ The history is replayed on top of the <branch> and <branch> is updated to
>   point at the tip of the resulting history. This is different from `--onto`,
>   which uses the target only as a starting point without updating it.
>   
> +--revert <branch>::
> +	Starting point at which to create the new revert commits; must be a
> +	branch name.
> ++
> +When `--revert` is specified, the commits in the revision range are reverted
> +(their changes are undone) and the revert commits

s/revert/reverted/

> are applied on top of <branch>.
> +The <branch> is then updated to point at the new commits. This is similar to
> +running `git revert` for each commit in the range, but works without a working tree.

"git revert" takes a revision range so it is the same as running "git 
revert <revision-range>" but does not update the working tree.

> ++
> +The commit messages follow `git revert` conventions: prefixed with "Revert" and

s/conventions: prefixed/conventions: they are prefixed/

> +including the original commit SHA.

s/including/include/
s/SHA/hash/

> When reverting a commit whose message starts
> +with "Revert", the new message uses "Reapply" instead. The author of the revert
> +commits is the current user, not the original commit author.
> ++
> +This option is mutually exclusive with `--onto` and `--advance`. It is also
> +incompatible with `--contained` (which is a modifier for `--onto` only).
> +
> +
>   --ref-action[=<mode>]::
>   	Control how references are updated. The mode can be:
>   +
> @@ -141,6 +159,22 @@ all commits they have since `base`, playing them on top of
>   `origin/main`. These three branches may have commits on top of `base`
>   that they have in common, but that does not need to be the case.
>   
> +To revert commits from a branch:
> +
> +------------
> +$ git replay --revert main feature~2..feature
> +------------
> +
> +This reverts the last two commits from 'feature', creating revert commits on
> +top of 'main', and updates 'main' to point at the result. This is useful when
> +commits from 'feature' were previously merged or cherry-picked into 'main' and
> +need to be undone.
> +
> +NOTE: For reverting an entire merge request as a single commit (rather than
> +commit-by-commit), consider using `git merge-tree --merge-base $TIP HEAD $BASE`
> +which can avoid unnecessary merge conflicts.

That's a good suggestion

> +
>   GIT
>   ---
>   Part of the linkgit:git[1] suite
> diff --git a/builtin/replay.c b/builtin/replay.c
> index 6606a2c94b..7660f7412f 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -17,6 +17,7 @@
>   #include "parse-options.h"
>   #include "refs.h"
>   #include "revision.h"
> +#include "sequencer.h"
>   #include "strmap.h"
>   #include <oidset.h>
>   #include <tree.h>
> @@ -26,6 +27,11 @@ enum ref_action_mode {
>   	REF_ACTION_PRINT,
>   };
>   
> +enum replay_action {
> +	REPLAY_PICK,
> +	REPLAY_REVERT,
> +};

sequencer.h already defines enum replay_action with an extra member so 
this is a bit confusing, maybe we should use a different name?

> +static void generate_revert_message(struct strbuf *msg,
> +				    struct commit *commit,
> +				    struct repository *repo)
> +{
> +	const char *out_enc = get_commit_output_encoding();
> +	const char *message = repo_logmsg_reencode(repo, commit, NULL, out_enc);
> +	const char *subject_start;
> +	int subject_len;
> +	char *subject;
> +
> +	subject_len = find_commit_subject(message, &subject_start);
> +	subject = xmemdupz(subject_start, subject_len);
> +
> +	sequencer_format_revert_header(msg, subject);
> +	strbuf_addstr(msg, oid_to_hex(&commit->object.oid));
> +	strbuf_addstr(msg, ".\n");

It's a bit odd that sequencer_format_revert_header() actually adds the 
beginning of the body but we have to add the commit oid ourselves. It 
would be nicer if we could pass the oid to that function and have it 
format the message for us. It's a bit tricky because the sequencer needs 
to handle merges as well but it shouldn't be too difficult.

The function name is also a bit strange as header normally refers to the 
commit metadata not the subject line.

> @@ -77,9 +105,14 @@ static struct commit *create_commit(struct repository *repo,
>   
>   	commit_list_insert(parent, &parents);
>   	extra = read_commit_extra_headers(based_on, exclude_gpgsig);
> -	find_commit_subject(message, &orig_message);
> -	strbuf_addstr(&msg, orig_message);
> -	author = get_author(message);
> +	if (action == REPLAY_REVERT) {
> +		generate_revert_message(&msg, based_on, repo);
> +		author = xstrdup(git_author_info(IDENT_STRICT));

write_commit_tree() will look up the author for us if we just pass NULL 
so I would just set author = NULL here or delete this line and 
initialize author to NULL at the beginning of this function.

> +	} else {
> +		find_commit_subject(message, &orig_message);
> +		strbuf_addstr(&msg, orig_message);
> +		author = get_author(message);
> +	}

This matches the deleted lines - good

> @@ -196,6 +230,20 @@ static void determine_replay_mode(struct repository *repo,
>   		}
>   		if (rinfo.positive_refexprs > 1)
>   			die(_("cannot advance target with multiple sources because ordering would be ill-defined"));
> +	} else if (*revert_name) {
> +		struct object_id oid;
> +		char *fullname = NULL;
> +
> +		*onto = peel_committish(repo, *revert_name);
> +		if (repo_dwim_ref(repo, *revert_name, strlen(*revert_name),
> +				  &oid, &fullname, 0) == 1) {
> +			free(*revert_name);
> +			*revert_name = fullname;
> +		} else {
> +			die(_("argument to --revert must be a reference"));
> +		}
> +		if (rinfo.positive_refexprs > 1)
> +			die(_("cannot revert with multiple sources because ordering would be ill-defined"));

This is a copy of what we do with --advance but with a different option 
name - can be factor this out into a common function that's called for 
both options?

> @@ -452,10 +535,13 @@ int cmd_replay(int argc,
>   	}
>   
>   	determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
> +			      &revert_name,
>   			      &onto, &update_refs);

Let's not fold the line after "revert_name"

> diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
> index cf3aacf355..1c4e1cb666 100755
> --- a/t/t3650-replay-basics.sh
> +++ b/t/t3650-replay-basics.sh
> @@ -314,4 +314,115 @@ test_expect_success 'invalid replay.refAction value' '
>   	test_grep "invalid.*replay.refAction.*value" error
>   '
>   
> +test_expect_success 'setup for revert tests' '
> +	git switch -c revert-test main &&
> +	test_commit R1 &&
> +	test_commit R2 &&
> +	test_commit R3 &&
> +	git switch main
> +'

Why do we need a new branch for this? We already have topic3 built on 
top of topic1 so we can test reverting commits with

	git replay --revert=topic3 main..topic1

which should revert C and F

> +test_expect_success 'git replay --revert reverts commits' '
> +	# Store original state
> +	START=$(git rev-parse revert-test) &&
> +	test_when_finished "git branch -f revert-test $START" &&
> +
> +	git replay --revert revert-test revert-test~2..revert-test >output &&
> +	test_must_be_empty output &&
> +
> +	# Verify revert-test was updated with revert commits
> +	git log --format=%s -n 5 revert-test >actual &&
> +	cat >expect <<-\EOF &&
> +	Revert "R3"
> +	Revert "R2"
> +	R3
> +	R2
> +	R1
> +	EOF
> +	test_cmp expect actual &&
> +
> +	# Verify commit message format
> +	test_commit_message revert-test -m "Revert \"R3\"
> +
> +This reverts commit $(git rev-parse R3)."

test_commit_message accepts the expected message on stdin so you can write

	test_commit_message revert-test <<-EOF
	Revert "R3"

	This reverts commit $(git rev-parse R3)
	EOF

which is cleaner as we don't have to escape the double quotes and the 
message is nicely indented.

> +'
> +
> +test_expect_success 'git replay --revert with --ref-action=print' '

Given the ref updating code is independent of --advance, --revert etc 
I'm not sure what extra coverage this test adds. If the previous test 
passes what is a plausible scenario where this one fails?

> +	# Store original state
> +	START=$(git rev-parse revert-test) &&
> +	test_when_finished "git branch -f revert-test $START" &&
> +
> +	git replay --ref-action=print --revert revert-test revert-test~2..revert-test >result &&
> +	test_line_count = 1 result &&
> +
> +	# Verify output format: update refs/heads/revert-test <new> <old>
> +	cut -f 3 -d " " result >new-tip &&
> +	printf "update refs/heads/revert-test " >expect &&
> +	printf "%s " $(cat new-tip) >>expect &&
> +	printf "%s\n" $START >>expect &&
> +	test_cmp expect result
> +'
> +
> +test_expect_success 'git replay --revert reapply behavior' '

Good idea

> +	# Store original state
> +	START=$(git rev-parse revert-test) &&
> +	test_when_finished "git branch -f revert-test $START" &&
> +
> +	# First revert R3
> +	git replay --revert revert-test revert-test~1..revert-test &&
> +	REVERT_R3=$(git rev-parse revert-test) &&
> +
> +	# Now revert the revert (should create "Reapply" message)
> +	git replay --revert revert-test revert-test~1..revert-test >output &&
> +	test_must_be_empty output &&
> +
> +	# Verify Reapply message
> +	test_commit_message revert-test -m "Reapply \"R3\"
> +
> +This reverts commit $(git rev-parse $REVERT_R3)."
> +'
> +
> +test_expect_success 'git replay --revert with conflict' '
> +	# Create a conflicting scenario
> +	git switch -c revert-conflict main &&
> +	test_commit C1 &&
> +	echo conflict >C1.t &&
> +	test_commit C2 C1.t &&
> +	git switch main &&
> +	echo different >C1.t &&
> +	test_commit C3 C1.t &&
> +
> +	# Try to revert C2 onto main (which has conflicting C3)
> +	test_expect_code 1 git replay --revert main revert-conflict~1..revert-conflict
> +'
> +
> +test_expect_success 'git replay --revert reflog message' '

I think we should just check the reflog message in one of the earlier tests.

> +	# Store original state
> +	START=$(git rev-parse revert-test) &&
> +	test_when_finished "git branch -f revert-test $START" &&
> +
> +	git replay --revert revert-test revert-test~1..revert-test >output &&
> +	test_must_be_empty output &&
> +
> +	# Verify reflog message includes --revert and branch name
> +	git reflog revert-test -1 --format=%gs >reflog-msg &&
> +	echo "replay --revert revert-test" >expect-reflog &&
> +	test_cmp expect-reflog reflog-msg
> +'
> +
> +test_expect_success 'git replay --revert incompatible with --contained' '
> +	test_must_fail git replay --revert revert-test --contained revert-test~1..revert-test 2>error &&
> +	test_grep "requires --onto" error
> +'
> +
> +test_expect_success 'git replay --revert incompatible with --onto' '
> +	test_must_fail git replay --revert revert-test --onto main revert-test~1..revert-test 2>error &&
> +	test_grep "cannot be used together" error
> +'
> +
> +test_expect_success 'git replay --revert incompatible with --advance' '
> +	test_must_fail git replay --revert revert-test --advance main revert-test~1..revert-test 2>error &&
> +	test_grep "cannot be used together" error
> +'

These last three look good.

Thanks

Phillip