[PATCH 0/1] replay: add --revert option to reverse commit changes

[PATCH 0/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

The `git replay` command currently supports cherry-picking commits for
server-side history rewriting, but lacks the ability to revert them.
This patch adds a `--revert` option to enable reversing commits directly
on bare repositories.

At GitLab, we use replay in Gitaly for efficient server-side operations.
Adding revert functionality enables us to reverse problematic commits
without client-side roundtrips, reducing network overhead.

The implementation leverages the insight that cherry-pick and revert are
essentially the same merge operation with swapped arguments. By swapping
the base and pickme trees when calling `merge_incore_nonrecursive()`, we
effectively reverse the diff direction. The existing conflict handling,
ref updates, and atomic transaction support work unchanged.

The revert message generation logic is extracted into a new shared
`sequencer_format_revert_header()` function in `sequencer.c`, allowing
code reuse between `sequencer.c` and `builtin/replay.c`. The commit
messages follow `git revert` conventions, including "Revert"/"Reapply"
prefixes and the original commit SHA.

This patch includes comprehensive tests covering various scenarios:
bare repositories, --advance mode, conflicts, reapply behavior, and
multiple commits.

Siddharth Asthana (1):
  replay: add --revert option to reverse commit changes

 Documentation/git-replay.adoc |  35 +++++++-
 builtin/replay.c              |  86 ++++++++++++++----
 sequencer.c                   |  23 +++++
 sequencer.h                   |   8 ++
 t/t3650-replay-basics.sh      | 160 ++++++++++++++++++++++++++++++++++
 5 files changed, 295 insertions(+), 17 deletions(-)

-- 
2.51.0

[PATCH 1/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

The `git replay` command performs server-side history rewriting without
requiring a working tree. While it currently supports cherry-picking
commits, 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` option that reverses the changes introduced by the
specified commits. The implementation follows the same approach as
`sequencer.c` (around lines 2358-2390), where cherry-pick and revert
are essentially the same merge operation but with swapped arguments:

  - Cherry-pick: merge(ancestor=parent, ours=current, theirs=commit)
  - Revert: merge(ancestor=commit, ours=current, theirs=parent)

We swap the base and pickme trees when calling
`merge_incore_nonrecursive()`, effectively reversing the diff direction.
The existing conflict handling, ref updates, and atomic transaction
support work unchanged.

The revert message generation logic (handling "Revert" and "Reapply"
cases) is extracted into a new `sequencer_format_revert_header()`
function in `sequencer.c`, which can be shared between `sequencer.c`
and `builtin/replay.c`. The `builtin/replay.c` code calls this shared
function and then appends the commit OID using `oid_to_hex()` directly,
since git replay is designed for simpler server-side operations without
the interactive features and `replay_opts` framework used by
`sequencer.c`.

The commit messages follow `git revert` conventions: prefixed with
"Revert" and including the original commit SHA. When reverting a commit
that itself starts with "Revert", the message uses "Reapply" instead.
Unlike cherry-pick which preserves the original author, revert commits
use the current user as the author, matching the behavior of `git
revert`.

Mark the option as incompatible with `--contained` since reverting
changes across multiple branches simultaneously could lead to
inconsistent repository states.

Helped-by: Christian Couder <christian.couder@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
---
 Documentation/git-replay.adoc |  35 +++++++-
 builtin/replay.c              |  86 ++++++++++++++----
 sequencer.c                   |  23 +++++
 sequencer.h                   |   8 ++
 t/t3650-replay-basics.sh      | 160 ++++++++++++++++++++++++++++++++++
 5 files changed, 295 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
index dcb26e8a8e..ad7dc08622 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>) [--ref-action[=<mode>]] [--revert] <revision-range>...
 
 DESCRIPTION
 -----------
@@ -54,6 +54,18 @@ which uses the target only as a starting point without updating it.
 +
 The default mode can be configured via the `replay.refAction` configuration variable.
 
+--revert::
+	Revert the changes introduced by the commits in the revision range
+	instead of applying them. This reverses the diff direction and creates
+	new commits that undo the changes, similar to `git revert`.
++
+The commit messages are prefixed with "Revert" and include the original
+commit SHA. If reverting a commit whose message starts with "Revert", the new
+message will start with "Reapply" instead. The author of the new commits
+will be the current user, not the original commit author.
++
+This option is incompatible with `--contained`.
+
 <revision-range>::
 	Range of commits to replay. More than one <revision-range> can
 	be passed, but in `--advance <branch>` mode, they should have
@@ -141,6 +153,27 @@ 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 a range of commits:
+
+------------
+$ git replay --revert --onto main feature~3..feature
+------------
+
+This creates new commits on top of 'main' that reverse the changes introduced
+by the last three commits on 'feature'. The 'feature' branch is updated to
+point at the last of these revert commits. The 'main' branch is not updated
+in this case.
+
+To revert commits and advance a branch:
+
+------------
+$ git replay --revert --advance main feature~2..feature
+------------
+
+This reverts the last two commits from 'feature', applies those reverts
+on top of 'main', and updates 'main' to point at the result. The 'feature'
+branch is not updated in this case.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/replay.c b/builtin/replay.c
index 6606a2c94b..7258d0bbc5 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>
@@ -57,10 +58,25 @@ static char *get_author(const char *message)
 	return NULL;
 }
 
+/*
+ * Generates a revert commit message using the shared sequencer function.
+ * We use oid_to_hex() directly instead of refer_to_commit() since git replay
+ * is designed for simpler server-side operations without interactive features.
+ */
+static void generate_revert_message(struct strbuf *msg,
+				    const char *orig_message,
+				    const struct object_id *oid)
+{
+	sequencer_format_revert_header(msg, orig_message);
+	strbuf_addstr(msg, oid_to_hex(oid));
+	strbuf_addstr(msg, ".\n");
+}
+
 static struct commit *create_commit(struct repository *repo,
 				    struct tree *tree,
 				    struct commit *based_on,
-				    struct commit *parent)
+				    struct commit *parent,
+				    int is_revert)
 {
 	struct object_id ret;
 	struct object *obj = NULL;
@@ -78,8 +94,17 @@ 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 (is_revert) {
+		generate_revert_message(&msg, orig_message, &based_on->object.oid);
+		/* For revert, use current user as author */
+		author = NULL;
+	} else {
+		/* Cherry-pick mode: use original commit message and author */
+		strbuf_addstr(&msg, orig_message);
+		author = get_author(message);
+	}
+
 	reset_ident_date();
 	if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
 				 &ret, author, NULL, sign_commit, extra)) {
@@ -261,7 +286,8 @@ static struct commit *pick_regular_commit(struct repository *repo,
 					  kh_oid_map_t *replayed_commits,
 					  struct commit *onto,
 					  struct merge_options *merge_opt,
-					  struct merge_result *result)
+					  struct merge_result *result,
+					  int is_revert)
 {
 	struct commit *base, *replayed_base;
 	struct tree *pickme_tree, *base_tree;
@@ -273,21 +299,41 @@ static struct commit *pick_regular_commit(struct repository *repo,
 	pickme_tree = repo_get_commit_tree(repo, pickme);
 	base_tree = repo_get_commit_tree(repo, base);
 
-	merge_opt->branch1 = short_commit_name(repo, replayed_base);
-	merge_opt->branch2 = short_commit_name(repo, pickme);
-	merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
+	if (is_revert) {
+		/* For revert: swap base and pickme to reverse the diff */
+		merge_opt->branch1 = short_commit_name(repo, replayed_base);
+		merge_opt->branch2 = xstrfmt("parent of %s", short_commit_name(repo, pickme));
+		merge_opt->ancestor = short_commit_name(repo, pickme);
 
-	merge_incore_nonrecursive(merge_opt,
-				  base_tree,
-				  result->tree,
-				  pickme_tree,
-				  result);
+		merge_incore_nonrecursive(merge_opt,
+					  pickme_tree,
+					  result->tree,
+					  base_tree,
+					  result);
+
+		/* branch2 was allocated with xstrfmt, needs freeing */
+		free((char *)merge_opt->branch2);
+	} else {
+		/* For cherry-pick: normal order */
+		merge_opt->branch1 = short_commit_name(repo, replayed_base);
+		merge_opt->branch2 = short_commit_name(repo, pickme);
+		merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
+
+		merge_incore_nonrecursive(merge_opt,
+					  base_tree,
+					  result->tree,
+					  pickme_tree,
+					  result);
+
+		/* ancestor was allocated with xstrfmt, needs freeing */
+		free((char *)merge_opt->ancestor);
+	}
 
-	free((char*)merge_opt->ancestor);
 	merge_opt->ancestor = NULL;
+	merge_opt->branch2 = NULL;
 	if (!result->clean)
 		return NULL;
-	return create_commit(repo, result->tree, pickme, replayed_base);
+	return create_commit(repo, result->tree, pickme, replayed_base, is_revert);
 }
 
 static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source)
@@ -350,6 +396,7 @@ int cmd_replay(int argc,
 	int contained = 0;
 	const char *ref_action = NULL;
 	enum ref_action_mode ref_mode;
+	int is_revert = 0;
 
 	struct rev_info revs;
 	struct commit *last_commit = NULL;
@@ -366,7 +413,7 @@ int cmd_replay(int argc,
 	const char *const replay_usage[] = {
 		N_("(EXPERIMENTAL!) git replay "
 		   "([--contained] --onto <newbase> | --advance <branch>) "
-		   "[--ref-action[=<mode>]] <revision-range>..."),
+		   "[--ref-action[=<mode>]] [--revert] <revision-range>..."),
 		NULL
 	};
 	struct option replay_options[] = {
@@ -381,6 +428,8 @@ int cmd_replay(int argc,
 		OPT_STRING(0, "ref-action", &ref_action,
 			   N_("mode"),
 			   N_("control ref update behavior (update|print)")),
+		OPT_BOOL(0, "revert", &is_revert,
+			 N_("revert commits instead of cherry-picking them")),
 		OPT_END()
 	};
 
@@ -395,6 +444,10 @@ int cmd_replay(int argc,
 	die_for_incompatible_opt2(!!advance_name_opt, "--advance",
 				  contained, "--contained");
 
+	/* --revert is incompatible with --contained */
+	die_for_incompatible_opt2(is_revert, "--revert",
+				  contained, "--contained");
+
 	/* Parse ref action mode from command line or config */
 	ref_mode = get_ref_action_mode(repo, ref_action);
 
@@ -496,7 +549,8 @@ int cmd_replay(int argc,
 			die(_("replaying merge commits is not supported yet!"));
 
 		last_commit = pick_regular_commit(repo, commit, replayed_commits,
-						  onto, &merge_opt, &result);
+						  onto, &merge_opt, &result,
+						  is_revert);
 		if (!last_commit)
 			break;
 
diff --git a/sequencer.c b/sequencer.c
index 5476d39ba9..e6d82c8368 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5572,6 +5572,29 @@ int sequencer_pick_revisions(struct repository *r,
 	return res;
 }
 
+void sequencer_format_revert_header(struct strbuf *out, const char *orig_subject)
+{
+	const char *revert_subject;
+
+	if (skip_prefix(orig_subject, "Revert \"", &revert_subject) &&
+	    /*
+	     * We don't touch pre-existing repeated reverts, because
+	     * theoretically these can be nested arbitrarily deeply,
+	     * thus requiring excessive complexity to deal with.
+	     */
+	    !starts_with(revert_subject, "Revert \"")) {
+		strbuf_addstr(out, "Reapply \"");
+		strbuf_addstr(out, revert_subject);
+		strbuf_addch(out, '\n');
+	} else {
+		strbuf_addstr(out, "Revert \"");
+		strbuf_addstr(out, orig_subject);
+		strbuf_addstr(out, "\"\n");
+	}
+
+	strbuf_addstr(out, "\nThis reverts commit ");
+}
+
 void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
 {
 	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
diff --git a/sequencer.h b/sequencer.h
index 719684c8a9..2d4a2d3fac 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -205,6 +205,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list);
  */
 void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);
 
+/*
+ * Formats a revert commit message header following standard Git conventions.
+ * Handles both regular reverts ("Revert \"<subject>\"") and revert of revert
+ * cases ("Reapply \"<subject>\""). Adds "This reverts commit " at the end.
+ * The caller should append the commit OID after calling this function.
+ */
+void sequencer_format_revert_header(struct strbuf *out, const char *orig_subject);
+
 void append_conflicts_hint(struct index_state *istate,
 		struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode);
 enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index cf3aacf355..5fcd730b54 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -314,4 +314,164 @@ test_expect_success 'invalid replay.refAction value' '
 	test_grep "invalid.*replay.refAction.*value" error
 '
 
+test_expect_success 'using replay with --revert to revert a commit' '
+	# Revert commits D and E from topic2
+	git replay --revert --onto topic1 topic1..topic2 >result &&
+
+	test_line_count = 1 result &&
+	NEW_TOPIC2=$(cut -f 3 -d " " result) &&
+
+	# Verify the result updates the topic2 branch
+	printf "update refs/heads/topic2 " >expect &&
+	printf "%s " $NEW_TOPIC2 >>expect &&
+	git rev-parse topic2 >>expect &&
+
+	test_cmp expect result &&
+
+	# Verify the commit messages contain "Revert"
+	# topic1..topic2 contains D and E, so we get 2 reverts on top of topic1 (which has F, C, B, A)
+	git log --format=%s $NEW_TOPIC2 >actual &&
+	test_line_count = 6 actual &&
+	head -n 1 actual >first-line &&
+	test_grep "^Revert" first-line
+'
+
+test_expect_success 'using replay with --revert on bare repo' '
+	git -C bare replay --revert --onto topic1 topic1..topic2 >result-bare &&
+
+	test_line_count = 1 result-bare &&
+	NEW_COMMIT=$(cut -f 3 -d " " result-bare) &&
+
+	# Verify the commit message contains "Revert"
+	git -C bare log --format=%s $NEW_COMMIT >actual-bare &&
+	test_line_count = 6 actual-bare &&
+	head -n 1 actual-bare >first-line-bare &&
+	test_grep "^Revert" first-line-bare
+'
+
+test_expect_success 'using replay with --revert and --advance' '
+	# Revert commits from topic2 and advance main
+	git replay --revert --advance main topic1..topic2 >result &&
+
+	test_line_count = 1 result &&
+	NEW_MAIN=$(cut -f 3 -d " " result) &&
+
+	# Verify the result updates the main branch
+	printf "update refs/heads/main " >expect &&
+	printf "%s " $NEW_MAIN >>expect &&
+	git rev-parse main >>expect &&
+
+	test_cmp expect result &&
+
+	# Verify the commit message contains "Revert"
+	git log --format=%s $NEW_MAIN >actual &&
+	head -n 1 actual >first-line &&
+	test_grep "^Revert" first-line
+'
+
+test_expect_success 'replay with --revert fails with --contained' '
+	test_must_fail git replay --revert --contained --onto main main..topic3 2>error &&
+	test_grep "revert.*contained.*cannot be used together" error
+'
+
+test_expect_success 'verify revert actually reverses changes' '
+	# Create a branch with a simple change
+	git switch -c revert-test main &&
+	echo "new content" >test-file.txt &&
+	git add test-file.txt &&
+	git commit -m "Add test file" &&
+
+	# Revert the commit
+	git replay --revert --advance revert-test HEAD^..HEAD >result &&
+	REVERTED=$(cut -f 3 -d " " result) &&
+
+	# The file should no longer exist (reverted)
+	test_must_fail git show $REVERTED:test-file.txt
+'
+
+test_expect_success 'revert of a revert creates reapply message' '
+	# Create a commit
+	git switch -c revert-revert main &&
+	echo "content" >revert-test-2.txt &&
+	git add revert-test-2.txt &&
+	git commit -m "Add revert test file" &&
+
+	ORIGINAL=$(git rev-parse HEAD) &&
+
+	# First revert
+	git replay --revert --advance revert-revert HEAD^..HEAD >result1 &&
+	FIRST_REVERT=$(cut -f 3 -d " " result1) &&
+
+	# Check first revert message starts with "Revert"
+	git log --format=%s -1 $FIRST_REVERT >msg1 &&
+	test_grep "^Revert" msg1 &&
+
+	# Now revert the revert
+	git replay --revert --advance revert-revert $ORIGINAL..$FIRST_REVERT >result2 &&
+	REAPPLY=$(cut -f 3 -d " " result2) &&
+
+	# Check second revert message starts with "Reapply"
+	git log --format=%s -1 $REAPPLY >msg2 &&
+	test_grep "^Reapply" msg2 &&
+
+	# The file should exist again (reapplied)
+	git show $REAPPLY:revert-test-2.txt >actual &&
+	echo "content" >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'replay --revert includes commit SHA in message' '
+	git switch -c revert-sha-test main &&
+	echo "test" >sha-test.txt &&
+	git add sha-test.txt &&
+	git commit -m "Test commit for SHA" &&
+
+	COMMIT_SHA=$(git rev-parse HEAD) &&
+	git replay --revert --advance revert-sha-test HEAD^..HEAD >result &&
+	REVERT_COMMIT=$(cut -f 3 -d " " result) &&
+
+	# Check that the commit message includes the original SHA
+	git log --format=%B -1 $REVERT_COMMIT >msg &&
+	test_grep "$COMMIT_SHA" msg
+'
+
+test_expect_success 'replay --revert with conflict' '
+	# Create a conflicting situation
+	git switch -c revert-conflict main &&
+	echo "line1" >conflict-file.txt &&
+	git add conflict-file.txt &&
+	git commit -m "Add conflict file" &&
+
+	git switch -c revert-conflict-branch HEAD^ &&
+	echo "different" >conflict-file.txt &&
+	git add conflict-file.txt &&
+	git commit -m "Different content" &&
+
+	# Try to revert the first commit onto the conflicting branch
+	test_expect_code 1 git replay --revert --onto revert-conflict-branch revert-conflict^..revert-conflict
+'
+
+test_expect_success 'replay --revert handles multiple commits' '
+	# Verify that reverting multiple commits works correctly
+	# The output should show both revert commits in the history
+	git log --format=%s topic2 >topic2-log &&
+	test_write_lines E D C B A >expected-topic2 &&
+	test_cmp expected-topic2 topic2-log &&
+
+	# Revert D and E from topic2, applying the reverts onto topic1
+	git replay --revert --onto topic1 topic1..topic2 >result &&
+
+	test_line_count = 1 result &&
+	FINAL=$(cut -f 3 -d " " result) &&
+
+	# Verify both revert commits appear in the log
+	git log --format=%s $FINAL >log &&
+	head -n 2 log >first-two &&
+	test_grep "^Revert" first-two &&
+
+	# Verify we have both "Revert D" and "Revert E"
+	test_grep "Revert.*E" log &&
+	test_grep "Revert.*D" log
+'
+
 test_done
-- 
2.51.0

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

> The revert message generation logic (handling "Revert" and "Reapply"
> cases) is extracted into a new `sequencer_format_revert_header()`
> function in `sequencer.c`, which can be shared between `sequencer.c`
> and `builtin/replay.c`. The `builtin/replay.c` code calls this shared
> function and then appends the commit OID using `oid_to_hex()` directly,
> since git replay is designed for simpler server-side operations without
> the interactive features and `replay_opts` framework used by
> `sequencer.c`.

When I review a patch that claims to refactor existing logic into a
separate helper function to reuse it in more places, I look at the
diffstat to see how many lines are removed.  The logic for
generating the message does not seem to be "extracted into", but
rather "duplicated to", the new helper function.  It gives the two
message sources opportunity to drift apart over time, which is not
what you want.

In do_pick_commit() where TODO_REVERT command is handled, we find a
code block that is almost identical to what this patch adds to the
new helper function; it should be rewritten to call the new helper
function or perhaps a shared helper function is introduced and
called from there and also from the sequencer_format_revert_header()
function, if there is still some impedance mismatch.  If such a
refactoring is done as a separate preliminary patch in a N-patch
series, the resulting patch series may be easier to follow (and
there may be other opportunities to reuse existing code more).

> Mark the option as incompatible with `--contained` since reverting
> changes across multiple branches simultaneously could lead to
> inconsistent repository states.

This, and the documentation part, does not seem to tell what
"inconsistent state" we are worried about.  Is it just a buggy
design of --revert can be implemented that produces wrong result
when used with --contened, or are these two options inherently try
to achieve contradicting goals?  I am guessing that it is the
latter, but if so, can we make it clear why?

> +--revert::
> +	Revert the changes introduced by the commits in the revision range
> +	instead of applying them. This reverses the diff direction and creates
> +	new commits that undo the changes, similar to `git revert`.
> ++
> +The commit messages are prefixed with "Revert" and include the original
> +commit SHA. If reverting a commit whose message starts with "Revert", the new
> +message will start with "Reapply" instead. The author of the new commits
> +will be the current user, not the original commit author.
> ++
> +This option is incompatible with `--contained`.

I have never used the `--contained` option, but is it so obvious to
those who have why these two have to be made incompatible that the
above statement does not have to be followed by "because ..."?

> @@ -141,6 +153,27 @@ 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 a range of commits:
> +
> +------------
> +$ git replay --revert --onto main feature~3..feature
> +------------
> +
> +This creates new commits on top of 'main' that reverse the changes introduced
> +by the last three commits on 'feature'. The 'feature' branch is updated to
> +point at the last of these revert commits. The 'main' branch is not updated
> +in this case.

Is there any topological requirement between 'main' and 'feature'
branches?  Naïvely, I would expect that it would be perfect if
'feature' branch has been merged to 'main' (then you'd be reverting
the top 3 commits of that branch), but that would be something you
would do to correct 'main', and not 'feature', but the description
explains this is a way to update 'feature' to lose the three topmost
commits, so I am not sure what this example really does and when it
would be useful.

> +To revert commits and advance a branch:
> +
> +------------
> +$ git replay --revert --advance main feature~2..feature
> +------------
> +
> +This reverts the last two commits from 'feature', applies those reverts
> +on top of 'main', and updates 'main' to point at the result. The 'feature'
> +branch is not updated in this case.

The same question.  If I assume that 'main' has merged 'feature'
before, this I can understand and match what I often do quite well
while working on integrating topic branches.  I may merge a topic
that is not yet well cooked enough into 'next', regret that the two
commits at the tip of the topic were premature, and revert these two
commits out of 'next', or something.  This example can be explained
well if there is topological requirement that 'main' has at least
these two commits from 'feature'.

> @@ -261,7 +286,8 @@ static struct commit *pick_regular_commit(struct repository *repo,
>  					  kh_oid_map_t *replayed_commits,
>  					  struct commit *onto,
>  					  struct merge_options *merge_opt,
> -					  struct merge_result *result)
> +					  struct merge_result *result,
> +					  int is_revert)

Are there other ways to pick commit imaginable (if not planned to be
implemented), other than "revert"?  I am wondering if this is better
done as "enum { CHERRY_PICK, REVERT, } pick_variant" for readability
and maintainability.

> @@ -273,21 +299,41 @@ static struct commit *pick_regular_commit(struct repository *repo,
>  	pickme_tree = repo_get_commit_tree(repo, pickme);
>  	base_tree = repo_get_commit_tree(repo, base);
>  
> -	merge_opt->branch1 = short_commit_name(repo, replayed_base);
> -	merge_opt->branch2 = short_commit_name(repo, pickme);
> -	merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
> +	if (is_revert) {

It may be just me, but it would have been easier to follow if
!revert case is given first, as that is the common variant the
pick_regular_commit() function.

> +		/* For revert: swap base and pickme to reverse the diff */
> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
> +		merge_opt->branch2 = xstrfmt("parent of %s", short_commit_name(repo, pickme));

That is an overly long line (sorry, I notice these things when a
line does not even fit in 92-col terminal).

> +		merge_opt->ancestor = short_commit_name(repo, pickme);

> -	merge_incore_nonrecursive(merge_opt,
> -				  base_tree,
> -				  result->tree,
> -				  pickme_tree,
> -				  result);
> +		merge_incore_nonrecursive(merge_opt,
> +					  pickme_tree,
> +					  result->tree,
> +					  base_tree,
> +					  result);

OK.  These are applications of the standard 3-way merge trick to
(ab)use ancestor to implement cherry-pick and revert.  Looking good.

> +
> +		/* branch2 was allocated with xstrfmt, needs freeing */
> +		free((char *)merge_opt->branch2);
> +	} else {
> +		/* For cherry-pick: normal order */
> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
> +		merge_opt->branch2 = short_commit_name(repo, pickme);
> +		merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
> +
> +		merge_incore_nonrecursive(merge_opt,
> +					  base_tree,
> +					  result->tree,
> +					  pickme_tree,
> +					  result);
> +
> +		/* ancestor was allocated with xstrfmt, needs freeing */
> +		free((char *)merge_opt->ancestor);

And the "else" block has the original sequence of statements.

> +	}
>  
> -	free((char*)merge_opt->ancestor);
>  	merge_opt->ancestor = NULL;
> +	merge_opt->branch2 = NULL;

Not a new problem, but what is the point of setting these two (but
not branch1) to NULL?  If a later caller misuses ->ancestor left
behind without setting its own, it would result in an access after
free, but if such a caller misuses ->branch1 left behind without
setting its own, because it is not allocated, it won't be an access
after free, *but* it is nevertheless wrong as the string in ->branch1
is *not* computed suitably for that caller, isn't it?

>  	if (!result->clean)
>  		return NULL;
> -	return create_commit(repo, result->tree, pickme, replayed_base);
> +	return create_commit(repo, result->tree, pickme, replayed_base, is_revert);
>  }


> @@ -350,6 +396,7 @@ int cmd_replay(int argc,
>  	int contained = 0;
>  	const char *ref_action = NULL;
>  	enum ref_action_mode ref_mode;
> +	int is_revert = 0;

Ditto on "revert,cherry-pick".

> diff --git a/sequencer.c b/sequencer.c
> index 5476d39ba9..e6d82c8368 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5572,6 +5572,29 @@ int sequencer_pick_revisions(struct repository *r,
>  	return res;
>  }
>  
> +void sequencer_format_revert_header(struct strbuf *out, const char *orig_subject)
> +{
> +	const char *revert_subject;
> +
> +	if (skip_prefix(orig_subject, "Revert \"", &revert_subject) &&
> +	    /*
> +	     * We don't touch pre-existing repeated reverts, because
> +	     * theoretically these can be nested arbitrarily deeply,
> +	     * thus requiring excessive complexity to deal with.
> +	     */
> +	    !starts_with(revert_subject, "Revert \"")) {
> +		strbuf_addstr(out, "Reapply \"");
> +		strbuf_addstr(out, revert_subject);
> +		strbuf_addch(out, '\n');
> +	} else {
> +		strbuf_addstr(out, "Revert \"");
> +		strbuf_addstr(out, orig_subject);
> +		strbuf_addstr(out, "\"\n");
> +	}
> +
> +	strbuf_addstr(out, "\nThis reverts commit ");
> +}
> +

Dedup with do_pick_commit() where this was taken from.  Possibly in
a separte patch before the main one.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> In do_pick_commit() where TODO_REVERT command is handled, we find a
> code block that is almost identical to what this patch adds to the
> new helper function; it should be rewritten to call the new helper
> function or perhaps a shared helper function is introduced and
> called from there and also from the sequencer_format_revert_header()
> function, if there is still some impedance mismatch.  If such a
> refactoring is done as a separate preliminary patch in a N-patch
> series, the resulting patch series may be easier to follow (and
> there may be other opportunities to reuse existing code more).
> ...
>> diff --git a/sequencer.c b/sequencer.c
>> index 5476d39ba9..e6d82c8368 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -5572,6 +5572,29 @@ int sequencer_pick_revisions(struct repository *r,
>>  	return res;
>>  }
>>  
>> +void sequencer_format_revert_header(struct strbuf *out, const char *orig_subject)
>> +{
>> +	const char *revert_subject;
>> +
>> +	if (skip_prefix(orig_subject, "Revert \"", &revert_subject) &&
>> +	    /*
>> +	     * We don't touch pre-existing repeated reverts, because
>> +	     * theoretically these can be nested arbitrarily deeply,
>> +	     * thus requiring excessive complexity to deal with.
>> +	     */
>> +	    !starts_with(revert_subject, "Revert \"")) {
>> +		strbuf_addstr(out, "Reapply \"");
>> +		strbuf_addstr(out, revert_subject);
>> +		strbuf_addch(out, '\n');
>> +	} else {
>> +		strbuf_addstr(out, "Revert \"");
>> +		strbuf_addstr(out, orig_subject);
>> +		strbuf_addstr(out, "\"\n");
>> +	}
>> +
>> +	strbuf_addstr(out, "\nThis reverts commit ");
>> +}
>> +
>
> Dedup with do_pick_commit() where this was taken from.  Possibly in
> a separte patch before the main one.

Forgot to attach this at the end.  What I meant was that something
along this line may be a good starting point.

 sequencer.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git c/sequencer.c w/sequencer.c
index e6d82c8368..29909952d4 100644
--- c/sequencer.c
+++ w/sequencer.c
@@ -2365,20 +2365,8 @@ static int do_pick_commit(struct repository *r,
 		if (opts->commit_use_reference) {
 			strbuf_commented_addf(&ctx->message, comment_line_str,
 				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
-		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
-			   /*
-			    * We don't touch pre-existing repeated reverts, because
-			    * theoretically these can be nested arbitrarily deeply,
-			    * thus requiring excessive complexity to deal with.
-			    */
-			   !starts_with(orig_subject, "Revert \"")) {
-			strbuf_addstr(&ctx->message, "Reapply \"");
-			strbuf_addstr(&ctx->message, orig_subject);
-			strbuf_addstr(&ctx->message, "\n");
 		} else {
-			strbuf_addstr(&ctx->message, "Revert \"");
-			strbuf_addstr(&ctx->message, msg.subject);
-			strbuf_addstr(&ctx->message, "\"\n");
+			sequencer_format_revert_header(&ctx->message, msg.subject);
 		}
 		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
 		refer_to_commit(opts, &ctx->message, commit);

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Dedup with do_pick_commit() where this was taken from.  Possibly in
>> a separte patch before the main one.
>
> Forgot to attach this at the end.  What I meant was that something
> along this line may be a good starting point.
>
>  sequencer.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git c/sequencer.c w/sequencer.c
> index e6d82c8368..29909952d4 100644
> --- c/sequencer.c
> +++ w/sequencer.c
> @@ -2365,20 +2365,8 @@ static int do_pick_commit(struct repository *r,
>  		if (opts->commit_use_reference) {
>  			strbuf_commented_addf(&ctx->message, comment_line_str,
>  				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> -		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
> -			   /*
> -			    * We don't touch pre-existing repeated reverts, because
> -			    * theoretically these can be nested arbitrarily deeply,
> -			    * thus requiring excessive complexity to deal with.
> -			    */
> -			   !starts_with(orig_subject, "Revert \"")) {
> -			strbuf_addstr(&ctx->message, "Reapply \"");
> -			strbuf_addstr(&ctx->message, orig_subject);
> -			strbuf_addstr(&ctx->message, "\n");
>  		} else {
> -			strbuf_addstr(&ctx->message, "Revert \"");
> -			strbuf_addstr(&ctx->message, msg.subject);
> -			strbuf_addstr(&ctx->message, "\"\n");
> +			sequencer_format_revert_header(&ctx->message, msg.subject);
>  		}
>  		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
>  		refer_to_commit(opts, &ctx->message, commit);

By the way, I probably would not be queuing this version today, as
this has obvious conflict with a large code movement made by
Patrick's "history" series, which itself is expecting a reroll.

Perhaps collect review comments on this iteration a bit more and
wait for that other topic to be rerolled, and if it turns out to be
solid enough, base a v2 of this patch on top of it?

Thanks.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> By the way, I probably would not be queuing this version today, as
> this has obvious conflict with a large code movement made by
> Patrick's "history" series, which itself is expecting a reroll.
>
> Perhaps collect review comments on this iteration a bit more and
> wait for that other topic to be rerolled, and if it turns out to be
> solid enough, base a v2 of this patch on top of it?

While I cannot test it with other topics, I had a chance to run
tests after applying the patch directly on top of 'master':

    $ make CC=clang SANITIZE=address,leak test
    ...
    Test Summary Report
    -------------------
    t3650-replay-basics.sh                           (Wstat: 256 (exited 1) Tests: 31 Failed: 5)
      Failed tests:  23-25, 27, 31
      Non-zero exit status: 1

The first failure was this one

    expecting success of 3650.23 'using replay with --revert to revert a commit': 
            # Revert commits D and E from topic2
            git replay --revert --onto topic1 topic1..topic2 >result &&

            test_line_count = 1 result &&
            NEW_TOPIC2=$(cut -f 3 -d " " result) &&

            # Verify the result updates the topic2 branch
            printf "update refs/heads/topic2 " >expect &&
            printf "%s " $NEW_TOPIC2 >>expect &&
            git rev-parse topic2 >>expect &&

            test_cmp expect result &&

            # Verify the commit messages contain "Revert"
            # topic1..topic2 contains D and E, so we get 2 reverts on top of topic1 (which has F, C, B, A)
            git log --format=%s $NEW_TOPIC2 >actual &&
            test_line_count = 6 actual &&
            head -n 1 actual >first-line &&
            test_grep "^Revert" first-line

    test_line_count: line count for result != 1

The "result" file has 0 bytes (hence 0 lines).

Actually, address or leak sanitizing build is not needed to
reproduce this problem, it seems.

    $ make CC=clang test

Was sufficient to see the same first failure.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

On 26/11/25 01:36, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> By the way, I probably would not be queuing this version today, as
>> this has obvious conflict with a large code movement made by
>> Patrick's "history" series, which itself is expecting a reroll.
>>
>> Perhaps collect review comments on this iteration a bit more and
>> wait for that other topic to be rerolled, and if it turns out to be
>> solid enough, base a v2 of this patch on top of it?
> While I cannot test it with other topics, I had a chance to run
> tests after applying the patch directly on top of 'master':
>
>      $ make CC=clang SANITIZE=address,leak test
>      ...
>      Test Summary Report
>      -------------------
>      t3650-replay-basics.sh                           (Wstat: 256 (exited 1) Tests: 31 Failed: 5)
>        Failed tests:  23-25, 27, 31
>        Non-zero exit status: 1
>
> The first failure was this one
>
>      expecting success of 3650.23 'using replay with --revert to revert a commit':
>              # Revert commits D and E from topic2
>              git replay --revert --onto topic1 topic1..topic2 >result &&
>
>              test_line_count = 1 result &&
>              NEW_TOPIC2=$(cut -f 3 -d " " result) &&
>
>              # Verify the result updates the topic2 branch
>              printf "update refs/heads/topic2 " >expect &&
>              printf "%s " $NEW_TOPIC2 >>expect &&
>              git rev-parse topic2 >>expect &&
>
>              test_cmp expect result &&
>
>              # Verify the commit messages contain "Revert"
>              # topic1..topic2 contains D and E, so we get 2 reverts on top of topic1 (which has F, C, B, A)
>              git log --format=%s $NEW_TOPIC2 >actual &&
>              test_line_count = 6 actual &&
>              head -n 1 actual >first-line &&
>              test_grep "^Revert" first-line
>
>      test_line_count: line count for result != 1
>
> The "result" file has 0 bytes (hence 0 lines).


Ah, this is because my patch was based on a tree that had atomic ref 
updates as the default (REF_ACTION_UPDATE), which produces no stdout 
output. The tests were written for --ref-action=print behavior.

I have fixed the tests to either:
1. Use --ref-action=print explicitly when expecting output, or
2. Check the ref state directly rather than parsing stdout

The test failures you saw should be fixed in v2.

Thanks,
Siddharth


>
> Actually, address or leak sanitizing build is not needed to
> reproduce this problem, it seems.
>
>      $ make CC=clang test
>
> Was sufficient to see the same first failure.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

On 26/11/25 01:09, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> Dedup with do_pick_commit() where this was taken from.  Possibly in
>>> a separte patch before the main one.
>> Forgot to attach this at the end.  What I meant was that something
>> along this line may be a good starting point.
>>
>>   sequencer.c | 14 +-------------
>>   1 file changed, 1 insertion(+), 13 deletions(-)
>>
>> diff --git c/sequencer.c w/sequencer.c
>> index e6d82c8368..29909952d4 100644
>> --- c/sequencer.c
>> +++ w/sequencer.c
>> @@ -2365,20 +2365,8 @@ static int do_pick_commit(struct repository *r,
>>   		if (opts->commit_use_reference) {
>>   			strbuf_commented_addf(&ctx->message, comment_line_str,
>>   				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
>> -		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
>> -			   /*
>> -			    * We don't touch pre-existing repeated reverts, because
>> -			    * theoretically these can be nested arbitrarily deeply,
>> -			    * thus requiring excessive complexity to deal with.
>> -			    */
>> -			   !starts_with(orig_subject, "Revert \"")) {
>> -			strbuf_addstr(&ctx->message, "Reapply \"");
>> -			strbuf_addstr(&ctx->message, orig_subject);
>> -			strbuf_addstr(&ctx->message, "\n");
>>   		} else {
>> -			strbuf_addstr(&ctx->message, "Revert \"");
>> -			strbuf_addstr(&ctx->message, msg.subject);
>> -			strbuf_addstr(&ctx->message, "\"\n");
>> +			sequencer_format_revert_header(&ctx->message, msg.subject);
>>   		}
>>   		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
>>   		refer_to_commit(opts, &ctx->message, commit);
> By the way, I probably would not be queuing this version today, as
> this has obvious conflict with a large code movement made by
> Patrick's "history" series, which itself is expecting a reroll.
>
> Perhaps collect review comments on this iteration a bit more and
> wait for that other topic to be rerolled, and if it turns out to be
> solid enough, base a v2 of this patch on top of it?


Understood. I will wait for Patrick's "history" series to be rerolled 
and base v2 on top of that to avoid conflicts. In the meantime, I will 
address all the review feedback locally.

Thanks,
Siddharth


>
> Thanks.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

On 26/11/25 00:52, Junio C Hamano wrote:
> Siddharth Asthana <siddharthasthana31@gmail.com> writes:
>
>> The revert message generation logic (handling "Revert" and "Reapply"
>> cases) is extracted into a new `sequencer_format_revert_header()`
>> function in `sequencer.c`, which can be shared between `sequencer.c`
>> and `builtin/replay.c`. The `builtin/replay.c` code calls this shared
>> function and then appends the commit OID using `oid_to_hex()` directly,
>> since git replay is designed for simpler server-side operations without
>> the interactive features and `replay_opts` framework used by
>> `sequencer.c`.
> When I review a patch that claims to refactor existing logic into a
> separate helper function to reuse it in more places, I look at the
> diffstat to see how many lines are removed.


You are right - in v1 I added the helper but didn't update 
do_pick_commit() to use it. I have fixed this in my local tree; the 
dedup change is:


     sequencer.c | 14 +-------------
     1 file changed, 1 insertion(+), 13 deletions(-)


> The logic for
> generating the message does not seem to be "extracted into", but
> rather "duplicated to", the new helper function.  It gives the two
> message sources opportunity to drift apart over time, which is not
> what you want.
>
> In do_pick_commit() where TODO_REVERT command is handled, we find a
> code block that is almost identical to what this patch adds to the
> new helper function; it should be rewritten to call the new helper
> function or perhaps a shared helper function is introduced and
> called from there and also from the sequencer_format_revert_header()
> function, if there is still some impedance mismatch.  If such a
> refactoring is done as a separate preliminary patch in a N-patch
> series, the resulting patch series may be easier to follow (and
> there may be other opportunities to reuse existing code more).
>
>> Mark the option as incompatible with `--contained` since reverting
>> changes across multiple branches simultaneously could lead to
>> inconsistent repository states.
> This, and the documentation part, does not seem to tell what
> "inconsistent state" we are worried about.


Elijah's reply clarified this perfectly - `--contained` is a modifier 
for `--onto`, and as he points out, `--revert` should be a new mode 
entirely, not a modifier. Once `--revert` is its own mode (like `--onto` 
and `--advance`), the incompatibility with `--contained` becomes clear:
`--contained` only makes sense with `--onto`.


> Is it just a buggy
> design of --revert can be implemented that produces wrong result
> when used with --contened, or are these two options inherently try
> to achieve contradicting goals?  I am guessing that it is the
> latter, but if so, can we make it clear why?
>
>> +--revert::
>> +	Revert the changes introduced by the commits in the revision range
>> +	instead of applying them. This reverses the diff direction and creates
>> +	new commits that undo the changes, similar to `git revert`.
>> ++
>> +The commit messages are prefixed with "Revert" and include the original
>> +commit SHA. If reverting a commit whose message starts with "Revert", the new
>> +message will start with "Reapply" instead. The author of the new commits
>> +will be the current user, not the original commit author.
>> ++
>> +This option is incompatible with `--contained`.
> I have never used the `--contained` option, but is it so obvious to
> those who have why these two have to be made incompatible that the
> above statement does not have to be followed by "because ..."?
>
>> @@ -141,6 +153,27 @@ 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 a range of commits:
>> +
>> +------------
>> +$ git replay --revert --onto main feature~3..feature
>> +------------
>> +
>> +This creates new commits on top of 'main' that reverse the changes introduced
>> +by the last three commits on 'feature'. The 'feature' branch is updated to
>> +point at the last of these revert commits. The 'main' branch is not updated
>> +in this case.
> Is there any topological requirement between 'main' and 'feature'
> branches?


Yes, and I failed to explain this. For reverts to produce meaningful 
non-empty commits, the commits being reverted should already be in the 
target branch's history. I will clarify the examples to show this 
topology requirement explicitly.


>    Naïvely, I would expect that it would be perfect if
> 'feature' branch has been merged to 'main' (then you'd be reverting
> the top 3 commits of that branch), but that would be something you
> would do to correct 'main', and not 'feature', but the description
> explains this is a way to update 'feature' to lose the three topmost
> commits, so I am not sure what this example really does and when it
> would be useful.
>
>> +To revert commits and advance a branch:
>> +
>> +------------
>> +$ git replay --revert --advance main feature~2..feature
>> +------------
>> +
>> +This reverts the last two commits from 'feature', applies those reverts
>> +on top of 'main', and updates 'main' to point at the result. The 'feature'
>> +branch is not updated in this case.
> The same question.  If I assume that 'main' has merged 'feature'
> before, this I can understand and match what I often do quite well
> while working on integrating topic branches.  I may merge a topic
> that is not yet well cooked enough into 'next', regret that the two
> commits at the tip of the topic were premature, and revert these two
> commits out of 'next', or something.  This example can be explained
> well if there is topological requirement that 'main' has at least
> these two commits from 'feature'.
>
>> @@ -261,7 +286,8 @@ static struct commit *pick_regular_commit(struct repository *repo,
>>   					  kh_oid_map_t *replayed_commits,
>>   					  struct commit *onto,
>>   					  struct merge_options *merge_opt,
>> -					  struct merge_result *result)
>> +					  struct merge_result *result,
>> +					  int is_revert)
> Are there other ways to pick commit imaginable (if not planned to be
> implemented), other than "revert"?  I am wondering if this is better
> done as "enum { CHERRY_PICK, REVERT, } pick_variant" for readability
> and maintainability.


Good point. An enum would be clearer and more maintainable. I wll change 
to `enum replay_action { REPLAY_PICK, REPLAY_REVERT }`.


>
>> @@ -273,21 +299,41 @@ static struct commit *pick_regular_commit(struct repository *repo,
>>   	pickme_tree = repo_get_commit_tree(repo, pickme);
>>   	base_tree = repo_get_commit_tree(repo, base);
>>   
>> -	merge_opt->branch1 = short_commit_name(repo, replayed_base);
>> -	merge_opt->branch2 = short_commit_name(repo, pickme);
>> -	merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
>> +	if (is_revert) {
> It may be just me, but it would have been easier to follow if
> !revert case is given first, as that is the common variant the
> pick_regular_commit() function.


Makes sense - the common case (cherry-pick) should come first. I will 
reorder the if/else.


>
>> +		/* For revert: swap base and pickme to reverse the diff */
>> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
>> +		merge_opt->branch2 = xstrfmt("parent of %s", short_commit_name(repo, pickme));
> That is an overly long line (sorry, I notice these things when a
> line does not even fit in 92-col terminal).


Fixed in my local tree by introducing a `pickme_name` variable.


>
>> +		merge_opt->ancestor = short_commit_name(repo, pickme);
>> -	merge_incore_nonrecursive(merge_opt,
>> -				  base_tree,
>> -				  result->tree,
>> -				  pickme_tree,
>> -				  result);
>> +		merge_incore_nonrecursive(merge_opt,
>> +					  pickme_tree,
>> +					  result->tree,
>> +					  base_tree,
>> +					  result);
> OK.  These are applications of the standard 3-way merge trick to
> (ab)use ancestor to implement cherry-pick and revert.  Looking good.
>
>> +
>> +		/* branch2 was allocated with xstrfmt, needs freeing */
>> +		free((char *)merge_opt->branch2);
>> +	} else {
>> +		/* For cherry-pick: normal order */
>> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
>> +		merge_opt->branch2 = short_commit_name(repo, pickme);
>> +		merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
>> +
>> +		merge_incore_nonrecursive(merge_opt,
>> +					  base_tree,
>> +					  result->tree,
>> +					  pickme_tree,
>> +					  result);
>> +
>> +		/* ancestor was allocated with xstrfmt, needs freeing */
>> +		free((char *)merge_opt->ancestor);
> And the "else" block has the original sequence of statements.
>
>> +	}
>>   
>> -	free((char*)merge_opt->ancestor);
>>   	merge_opt->ancestor = NULL;
>> +	merge_opt->branch2 = NULL;
> Not a new problem, but what is the point of setting these two (but
> not branch1) to NULL?


You're right, this is inconsistent. The intent is to prevent 
use-after-free, but setting only some fields to NULL is incomplete. I 
will either set all three to NULL or add a comment explaining the rationale.


> If a later caller misuses ->ancestor left
> behind without setting its own, it would result in an access after
> free, but if such a caller misuses ->branch1 left behind without
> setting its own, because it is not allocated, it won't be an access
> after free, *but* it is nevertheless wrong as the string in ->branch1
> is *not* computed suitably for that caller, isn't it?
>
>>   	if (!result->clean)
>>   		return NULL;
>> -	return create_commit(repo, result->tree, pickme, replayed_base);
>> +	return create_commit(repo, result->tree, pickme, replayed_base, is_revert);
>>   }
>
>> @@ -350,6 +396,7 @@ int cmd_replay(int argc,
>>   	int contained = 0;
>>   	const char *ref_action = NULL;
>>   	enum ref_action_mode ref_mode;
>> +	int is_revert = 0;
> Ditto on "revert,cherry-pick".
>
>> diff --git a/sequencer.c b/sequencer.c
>> index 5476d39ba9..e6d82c8368 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -5572,6 +5572,29 @@ int sequencer_pick_revisions(struct repository *r,
>>   	return res;
>>   }
>>   
>> +void sequencer_format_revert_header(struct strbuf *out, const char *orig_subject)
>> +{
>> +	const char *revert_subject;
>> +
>> +	if (skip_prefix(orig_subject, "Revert \"", &revert_subject) &&
>> +	    /*
>> +	     * We don't touch pre-existing repeated reverts, because
>> +	     * theoretically these can be nested arbitrarily deeply,
>> +	     * thus requiring excessive complexity to deal with.
>> +	     */
>> +	    !starts_with(revert_subject, "Revert \"")) {
>> +		strbuf_addstr(out, "Reapply \"");
>> +		strbuf_addstr(out, revert_subject);
>> +		strbuf_addch(out, '\n');
>> +	} else {
>> +		strbuf_addstr(out, "Revert \"");
>> +		strbuf_addstr(out, orig_subject);
>> +		strbuf_addstr(out, "\"\n");
>> +	}
>> +
>> +	strbuf_addstr(out, "\nThis reverts commit ");
>> +}
>> +
> Dedup with do_pick_commit() where this was taken from.  Possibly in
> a separte patch before the main one.


I have applied your suggested patch and will split this into a 2-patch 
series: (1) extract and reuse sequencer_format_revert_header(), (2) add 
--revert to replay.

Thanks,
Siddharth

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

>>> +This creates new commits on top of 'main' that reverse the changes introduced
>>> +by the last three commits on 'feature'. The 'feature' branch is updated to
>>> +point at the last of these revert commits. The 'main' branch is not updated
>>> +in this case.
>> Is there any topological requirement between 'main' and 'feature'
>> branches?
>
> Yes, and I failed to explain this. For reverts to produce meaningful 
> non-empty commits, the commits being reverted should already be in the 
> target branch's history. I will clarify the examples to show this 
> topology requirement explicitly.

We need to be a bit careful, though.  Strictly speaking, what we
have is not a requirement on the shape of the history.  If a topic
that was merged to the development branch gets cherry-picked to the
master branch, and then it turns out to be faulty and needs to be
reverted, we can still "revert" the original topic out of the master
branch, even though topologically, the original topic is *not* in
'master'.

>>> +		/* For revert: swap base and pickme to reverse the diff */
>>> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
>>> +		merge_opt->branch2 = xstrfmt("parent of %s", short_commit_name(repo, pickme));
>> That is an overly long line (sorry, I notice these things when a
>> line does not even fit in 92-col terminal).
>
>
> Fixed in my local tree by introducing a `pickme_name` variable.

Just a line-folding at an appropriate column may be sufficient, e.g.,

		merge_opt->branch2 = xstrfmt("parent of %s",
					short_commit_name(repo, pickme));

>>> -	free((char*)merge_opt->ancestor);
>>>   	merge_opt->ancestor = NULL;
>>> +	merge_opt->branch2 = NULL;
>> Not a new problem, but what is the point of setting these two (but
>> not branch1) to NULL?
>
>
> You're right, this is inconsistent. The intent is to prevent 
> use-after-free, but setting only some fields to NULL is incomplete. I 
> will either set all three to NULL or add a comment explaining the rationale.

Is this the only place that resets a subset of merge_opt members for
reuse?  If not, are these multiple places want to reset the same
subset of the members?  Perhaps we can use a helper function to
clarify in such a case.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

On 27/11/25 02:43, Junio C Hamano wrote:
> Siddharth Asthana <siddharthasthana31@gmail.com> writes:
>
>>>> +This creates new commits on top of 'main' that reverse the changes introduced
>>>> +by the last three commits on 'feature'. The 'feature' branch is updated to
>>>> +point at the last of these revert commits. The 'main' branch is not updated
>>>> +in this case.
>>> Is there any topological requirement between 'main' and 'feature'
>>> branches?
>> Yes, and I failed to explain this. For reverts to produce meaningful
>> non-empty commits, the commits being reverted should already be in the
>> target branch's history. I will clarify the examples to show this
>> topology requirement explicitly.
> We need to be a bit careful, though.  Strictly speaking, what we
> have is not a requirement on the shape of the history.


Right - it's the changes that need to exist in the target tree, not the 
commits themselves. Cherry-picked commits can be reverted even without 
topological ancestry. I will fix the documentation.


>    If a topic
> that was merged to the development branch gets cherry-picked to the
> master branch, and then it turns out to be faulty and needs to be
> reverted, we can still "revert" the original topic out of the master
> branch, even though topologically, the original topic is *not* in
> 'master'.
>
>>>> +		/* For revert: swap base and pickme to reverse the diff */
>>>> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
>>>> +		merge_opt->branch2 = xstrfmt("parent of %s", short_commit_name(repo, pickme));
>>> That is an overly long line (sorry, I notice these things when a
>>> line does not even fit in 92-col terminal).
>>
>> Fixed in my local tree by introducing a `pickme_name` variable.
> Just a line-folding at an appropriate column may be sufficient, e.g.,


Will do.


>
> 		merge_opt->branch2 = xstrfmt("parent of %s",
> 					short_commit_name(repo, pickme));
>
>>>> -	free((char*)merge_opt->ancestor);
>>>>    	merge_opt->ancestor = NULL;
>>>> +	merge_opt->branch2 = NULL;
>>> Not a new problem, but what is the point of setting these two (but
>>> not branch1) to NULL?
>>
>> You're right, this is inconsistent. The intent is to prevent
>> use-after-free, but setting only some fields to NULL is incomplete. I
>> will either set all three to NULL or add a comment explaining the rationale.
> Is this the only place that resets a subset of merge_opt members for
> reuse?  If not, are these multiple places want to reset the same
> subset of the members?  Perhaps we can use a helper function to
> clarify in such a case.


This is the only place in replay.c. The reason we only NULL ancestor and 
branch2 is that those are the ones allocated with xstrfmt() - branch1 
points to short_commit_name() which doesn't need freeing.

A helper would make the intent clearer. I will look into adding one.

Thanks,
Siddharth

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Phillip Wood]

Hi Siddharth

On 25/11/2025 17:00, Siddharth Asthana wrote:
> 
> diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
> index dcb26e8a8e..ad7dc08622 100644
> --- a/Documentation/git-replay.adoc
> +++ b/Documentation/git-replay.adoc
> @@ -54,6 +54,18 @@ which uses the target only as a starting point without updating it.
> [...]
> +To revert a range of commits:
> +
> +------------
> +$ git replay --revert --onto main feature~3..feature
> +------------
> +
> +This creates new commits on top of 'main' that reverse the changes introduced
> +by the last three commits on 'feature'. The 'feature' branch is updated to
> +point at the last of these revert commits. The 'main' branch is not updated
> +in this case.

I'm struggling to understand when I'd want to do this. Why would I want 
to update 'feature' to point to the reverted version of its last tree 
commits rebased onto 'main'? In order to understand I ran the first 
tests case which does

	git replay --onto topic1 --revert topic1..topic2

after fixing it by adding --ref-action=print the resulting commit log 
looks like

commit d337fab78e90008835f74e890039b464a0308cbe
Author: author@name <bogus@email@address>
Date:   Thu Apr 7 15:30:13 2005 -0700

     Revert "E
     "

     This reverts commit bceb3acd81ddd36ba0da391fffa48949a1337276.

commit 47f0cc1c1f1911c0047a4d79d79f7c19c6c7151a
Author: author@name <bogus@email@address>
Date:   Thu Apr 7 15:30:13 2005 -0700

     Revert "D
     "

     This reverts commit d953cf2dcc1da8b51934e43fd83dac72d0e267c7.


The commits are empty because the original they are reverting each 
create a new file which is then present in the base revision but not in 
either of the merge heads when we revert. This suggests to me that it is 
not a very realistic test and I'm still scratching my head to see where 
"git replay --onto <commit> --revert" is useful.

If '--revert' does not make sense with '--onto' then perhaps it should 
be a new mode that takes a ref and acts like '--advance' but reverts the 
commits rather than cherry-picking them. When reverting a range of 
commits it would reduce the likelihood of conflicts to revert then in 
reverse order so we should either recommend passing '--reverse' or make 
that the default when '--revert' is given.

As you can see in the log output above the new function to format the 
revert subject lines is buggy. If you had used test_commit_message() to 
check the commit message, rather than just grepping for ^Revert the 
tests would have picked that up.

Thanks

Phillip

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Elijah Newren]

On Wed, Nov 26, 2025 at 3:10 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Siddharth
>
> On 25/11/2025 17:00, Siddharth Asthana wrote:
> >
> > diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
> > index dcb26e8a8e..ad7dc08622 100644
> > --- a/Documentation/git-replay.adoc
> > +++ b/Documentation/git-replay.adoc
> > @@ -54,6 +54,18 @@ which uses the target only as a starting point without updating it.
> > [...]
> > +To revert a range of commits:
> > +
> > +------------
> > +$ git replay --revert --onto main feature~3..feature
> > +------------
> > +
> > +This creates new commits on top of 'main' that reverse the changes introduced
> > +by the last three commits on 'feature'. The 'feature' branch is updated to
> > +point at the last of these revert commits. The 'main' branch is not updated
> > +in this case.
>
> I'm struggling to understand when I'd want to do this. Why would I want
> to update 'feature' to point to the reverted version of its last tree
> commits rebased onto 'main'? In order to understand I ran the first
> tests case which does
>
>         git replay --onto topic1 --revert topic1..topic2
>
> after fixing it by adding --ref-action=print the resulting commit log
> looks like
>
> commit d337fab78e90008835f74e890039b464a0308cbe
> Author: author@name <bogus@email@address>
> Date:   Thu Apr 7 15:30:13 2005 -0700
>
>      Revert "E
>      "
>
>      This reverts commit bceb3acd81ddd36ba0da391fffa48949a1337276.
>
> commit 47f0cc1c1f1911c0047a4d79d79f7c19c6c7151a
> Author: author@name <bogus@email@address>
> Date:   Thu Apr 7 15:30:13 2005 -0700
>
>      Revert "D
>      "
>
>      This reverts commit d953cf2dcc1da8b51934e43fd83dac72d0e267c7.
>
>
> The commits are empty because the original they are reverting each
> create a new file which is then present in the base revision but not in
> either of the merge heads when we revert. This suggests to me that it is
> not a very realistic test and I'm still scratching my head to see where
> "git replay --onto <commit> --revert" is useful.
>
> If '--revert' does not make sense with '--onto' then perhaps it should
> be a new mode that takes a ref and acts like '--advance' but reverts the
> commits rather than cherry-picking them. When reverting a range of
> commits it would reduce the likelihood of conflicts to revert then in
> reverse order so we should either recommend passing '--reverse' or make
> that the default when '--revert' is given.
>
> As you can see in the log output above the new function to format the
> revert subject lines is buggy. If you had used test_commit_message() to
> check the commit message, rather than just grepping for ^Revert the
> tests would have picked that up.
>
> Thanks
>
> Phillip

I was going to say the same thing, but from a different angle.

The sequencer in git is used for three different types of operations:
rebasing, cherry-picking, and reverting a range (with a sequence of
reverts rather than one big revert).  In replay, these correspond to
--onto, --advance, and the new thing you are trying to add.  As such,
it should be its own new mode.

(I do tend to see ranges reverted by a single big revert, the way
Johannes suggested, rather than as a range of individual reverts, so
to me the utility of the new mode looks low, but perhaps others find
more utility in it.  Or maybe the intent is to only use it with a
revision range that is only one commit long?)

Phillip also went into more detail about why "--onto $COMMIT --revert"
specifically doesn't make sense.  I'd also say "--advance $BRANCH
--revert" doesn't read well because to users, "revert" means going
back while "advance" means going forward, so it's a rather confusing
command line to make them wrap their head around.

And yes, Siddharth, you were right that the new mode should be
incompatible with --contained, but that's because --contained is a
special modifier of --onto.  --onto, --advance, and --revert are three
different modes that are incompatible with each other.  Once you've
checked for that incompatibility between the three modes, then you can
either check that whenever --contained is specified, either --onto is
as well, or neither --advance nor --revert are.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> I'm struggling to understand when I'd want to do this. Why would I want
>> to update 'feature' to point to the reverted version of its last tree
>> commits rebased onto 'main'?
>> ...
> I was going to say the same thing, but from a different angle.
>
> The sequencer in git is used for three different types of operations:
> rebasing, cherry-picking, and reverting a range (with a sequence of
> reverts rather than one big revert).  In replay, these correspond to
> --onto, --advance, and the new thing you are trying to add.  As such,
> it should be its own new mode.

This is a great comment that clarifies what the problem is with this.

> And yes, Siddharth, you were right that the new mode should be
> incompatible with --contained, but that's because --contained is a
> special modifier of --onto.  --onto, --advance, and --revert are three
> different modes that are incompatible with each other.

This answers the question I had on the patch perfectly.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Elijah Newren <newren@gmail.com> writes:
>
>>> I'm struggling to understand when I'd want to do this. Why would I want
>>> to update 'feature' to point to the reverted version of its last tree
>>> commits rebased onto 'main'?
>>> ...
>> I was going to say the same thing, but from a different angle.
>>
>> The sequencer in git is used for three different types of operations:
>> rebasing, cherry-picking, and reverting a range (with a sequence of
>> reverts rather than one big revert).  In replay, these correspond to
>> --onto, --advance, and the new thing you are trying to add.  As such,
>> it should be its own new mode.
>
> This is a great comment that clarifies what the problem is with this.

Stepping back a bit, is it just me who thinks that the "--onto"
option is a misnamed "--rebase", and the "--advance" option is a
misnamed "--cherry-pick"?

Perhaps it is already way too late to remedy, but if we ever want to
change it, we should do so while "replay" is still marked experimental.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Elijah Newren]

On Wed, Nov 26, 2025 at 1:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Elijah Newren <newren@gmail.com> writes:
> >
> >>> I'm struggling to understand when I'd want to do this. Why would I want
> >>> to update 'feature' to point to the reverted version of its last tree
> >>> commits rebased onto 'main'?
> >>> ...
> >> I was going to say the same thing, but from a different angle.
> >>
> >> The sequencer in git is used for three different types of operations:
> >> rebasing, cherry-picking, and reverting a range (with a sequence of
> >> reverts rather than one big revert).  In replay, these correspond to
> >> --onto, --advance, and the new thing you are trying to add.  As such,
> >> it should be its own new mode.
> >
> > This is a great comment that clarifies what the problem is with this.
>
> Stepping back a bit, is it just me who thinks that the "--onto"
> option is a misnamed "--rebase", and the "--advance" option is a
> misnamed "--cherry-pick"?

Is the goal to make connections between existing commands for folks
already very familiar with git, at the expense of comprehensibility
for new users and command lines that look somewhat illogical?

For either a rebase or a cherry-pick operation you have: (A) a range
of commits to be transplanted, (B) a base on which to build from, and
(C) the choice of which ref(s) should be updated to point to the
transplanted commits.  cherry-pick assumed HEAD for both (B) and (C).
rebase formed an implicit range instead of letting the user specify
(in a way which has always made it difficult to teach to new users,
IMO, but I digress), which involves HEAD and also used HEAD for (C).
git replay removes all assumptions about HEAD, which means there is
much more freedom for (A), (B), and (C), but I think it also makes it
more important to try to make command lines at least a bit more
self-describing for users to learn.

== Example command lines today ==

  git replay --onto main feature~3..feature

This command replays the commits in the range feature~3..feature onto
main, and updates feature to point at the result.

  git replay --advance main feature~3..feature

This command replays the commits in the range feature~3..feature onto
main, and advances main to point at the result.

(Both replay the same commit range on the same base, they differ only
in which refs are updated at the end.)

== Example command lines from your proposal ==

   git replay --rebase main feature~3..feature

This command to me would suggest that main is being rebased, but it
isn't -- it rebases feature~3..feature onto main while updating
feature to point at the result.  I find the "--rebase main" part of
this command line confusing.

   git replay --cherry-pick main feature~3..feature

This command to me would suggest that main is being cherry-picked, but
it isn't -- it cherry-picks feature~3..feature onto main while
updating main to point at the result.  Again, I find the
"--cherry-pick main" part of this command line confusing.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> == Example command lines from your proposal ==
>
>    git replay --rebase main feature~3..feature
>
> This command to me would suggest that main is being rebased, but it
> isn't -- it rebases feature~3..feature onto main while updating
> feature to point at the result.  I find the "--rebase main" part of
> this command line confusing.
>
>    git replay --cherry-pick main feature~3..feature
>
> This command to me would suggest that main is being cherry-picked, but
> it isn't -- it cherry-picks feature~3..feature onto main while
> updating main to point at the result.  Again, I find the
> "--cherry-pick main" part of this command line confusing.

That only tells us that if you want to help users by limiting the
vocabulary to a single set (i.e. both command names, and mode names
used in replay), you'd need to make sure you have the order of
<branch> and <range> given to the replay command in logical order,
in line with the option name, no?  Of course, if you want to say
"cherry-pick", cherry-picked range would have to come near the
option flag that says "cherry-pick", naturally.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Elijah Newren]

On Wed, Nov 26, 2025 at 3:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > == Example command lines from your proposal ==
> >
> >    git replay --rebase main feature~3..feature
> >
> > This command to me would suggest that main is being rebased, but it
> > isn't -- it rebases feature~3..feature onto main while updating
> > feature to point at the result.  I find the "--rebase main" part of
> > this command line confusing.
> >
> >    git replay --cherry-pick main feature~3..feature
> >
> > This command to me would suggest that main is being cherry-picked, but
> > it isn't -- it cherry-picks feature~3..feature onto main while
> > updating main to point at the result.  Again, I find the
> > "--cherry-pick main" part of this command line confusing.
>
> That only tells us that if you want to help users by limiting the
> vocabulary to a single set (i.e. both command names, and mode names
> used in replay), you'd need to make sure you have the order of
> <branch> and <range> given to the replay command in logical order,
> in line with the option name, no?  Of course, if you want to say
> "cherry-pick", cherry-picked range would have to come near the
> option flag that says "cherry-pick", naturally.

--advance and --onto are flags that require an argument -- in this
case, "main".  So, now you're suggesting more than renaming, in
particular some bigger refactoring such as making these flags now
require the <range> rather than the <base>.  Let's follow that path a
bit further...

Does your proposal assume that <range> is simple, such as
"feature~3..feature" above (i.e. something that an argument parser
would view as a single argument)?  What if the <range> were "^main
feature1 feature2"?  Or ""^$COMMIT --ancestry-path --branches"?  (I
don't see how to have the option parser easily be able to stuff the
arguments to "--rebase ^$COMMIT --ancestry-path --branches" into a
range variable that eats all of "^$COMMIT --ancestry-path
--branches".)  While I use simple ranges to describe the feature, I
specifically built the command to be able to do things like those
other two examples and use it for those.  Those more complicated
examples are things the rebase command just can't do.

Also, just like `git log` allows `git log [<options>] [<revision
range>]`, I wanted git replay to allow `git replay [<options>]
[<revision range>]`.  Instead of doing magic to get an implicit
revision range as rebase does (and with rather limited options because
of that magic), suddenly people can use what they've learned from `git
log` in another place.  But that piece of knowledge only really
transfers if we do similarly to `git log`, i.e. the revision range
comes after other options.

Perhaps one way to avoid the first problem above is to make
`--onto/--advance/--rebase/--cherry-pick" stop requiring (or
accepting) an argument and turn them into simple mode toggles, and
then make both <base> and <range> be positional arguments, with some
well-defined ordering.  However, if <base> comes before <revision>
then we still have the same problem as my previous email, whereas if
it comes after, then we weaken or destroy the connection to `git log`
I made above.  Maybe the connection to `git log` isn't that important.
What I think is important either way, though, is if we use positional
arguments for both things instead of making (at least one) an option,
then I feel we are copying one of the designs of `git rebase` that
makes it hard for even me to use: I hate that it uses multiple
positional arguments to define the operation; despite using the
command heavily for 16-17 years and sending in lots of patches to
improve it, I still can't remember the order of those positional
arguments and have to look it up again when teaching others.  Maybe
that's a personal shortcoming, but I would really rather that either
<base> or <revision> was an option flag.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

On 26/11/25 23:05, Elijah Newren wrote:
> On Wed, Nov 26, 2025 at 3:10 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> Hi Siddharth
>>
>> On 25/11/2025 17:00, Siddharth Asthana wrote:
>>> diff --git a/Documentation/git-replay.adoc b/Documentation/git-replay.adoc
>>> index dcb26e8a8e..ad7dc08622 100644
>>> --- a/Documentation/git-replay.adoc
>>> +++ b/Documentation/git-replay.adoc
>>> @@ -54,6 +54,18 @@ which uses the target only as a starting point without updating it.
>>> [...]
>>> +To revert a range of commits:
>>> +
>>> +------------
>>> +$ git replay --revert --onto main feature~3..feature
>>> +------------
>>> +
>>> +This creates new commits on top of 'main' that reverse the changes introduced
>>> +by the last three commits on 'feature'. The 'feature' branch is updated to
>>> +point at the last of these revert commits. The 'main' branch is not updated
>>> +in this case.
>> I'm struggling to understand when I'd want to do this. Why would I want
>> to update 'feature' to point to the reverted version of its last tree
>> commits rebased onto 'main'? In order to understand I ran the first
>> tests case which does
>>
>>          git replay --onto topic1 --revert topic1..topic2
>>
>> after fixing it by adding --ref-action=print the resulting commit log
>> looks like
>>
>> commit d337fab78e90008835f74e890039b464a0308cbe
>> Author: author@name <bogus@email@address>
>> Date:   Thu Apr 7 15:30:13 2005 -0700
>>
>>       Revert "E
>>       "
>>
>>       This reverts commit bceb3acd81ddd36ba0da391fffa48949a1337276.
>>
>> commit 47f0cc1c1f1911c0047a4d79d79f7c19c6c7151a
>> Author: author@name <bogus@email@address>
>> Date:   Thu Apr 7 15:30:13 2005 -0700
>>
>>       Revert "D
>>       "
>>
>>       This reverts commit d953cf2dcc1da8b51934e43fd83dac72d0e267c7.
>>
>>
>> The commits are empty because the original they are reverting each
>> create a new file which is then present in the base revision but not in
>> either of the merge heads when we revert. This suggests to me that it is
>> not a very realistic test and I'm still scratching my head to see where
>> "git replay --onto <commit> --revert" is useful.
>>
>> If '--revert' does not make sense with '--onto' then perhaps it should
>> be a new mode that takes a ref and acts like '--advance' but reverts the
>> commits rather than cherry-picking them. When reverting a range of
>> commits it would reduce the likelihood of conflicts to revert then in
>> reverse order so we should either recommend passing '--reverse' or make
>> that the default when '--revert' is given.
>>
>> As you can see in the log output above the new function to format the
>> revert subject lines is buggy. If you had used test_commit_message() to
>> check the commit message, rather than just grepping for ^Revert the
>> tests would have picked that up.
>>
>> Thanks
>>
>> Phillip
> I was going to say the same thing, but from a different angle.


Hi Elijah,

thanks for the architectural clarity!


>
> The sequencer in git is used for three different types of operations:
> rebasing, cherry-picking, and reverting a range (with a sequence of
> reverts rather than one big revert).  In replay, these correspond to
> --onto, --advance, and the new thing you are trying to add.  As such,
> it should be its own new mode.


This makes complete sense. I was treating `--revert` as a modifier when 
it should be a third mode alongside `--onto` and `--advance`.

I will restructure so that the user specifies exactly one of:
   --onto <newbase>
   --advance <branch>
   --revert <target>

Where `--revert <target>` applies the reverts on top of <target> and 
updates that ref.


>
> (I do tend to see ranges reverted by a single big revert, the way
> Johannes suggested, rather than as a range of individual reverts,


The commit-by-commit approach is useful when you need:
- Individual revert commits with proper "This reverts commit X" messages
- The ability to later cherry-pick specific reverts
- Clear history showing which commit caused which revert

But I will add documentation noting the `merge-tree` alternative for 
cases where a single combined revert is preferred.

Thanks,
Siddharth


>   so
> to me the utility of the new mode looks low, but perhaps others find
> more utility in it.  Or maybe the intent is to only use it with a
> revision range that is only one commit long?)
>
> Phillip also went into more detail about why "--onto $COMMIT --revert"
> specifically doesn't make sense.  I'd also say



> "--advance $BRANCH
> --revert" doesn't read well because to users, "revert" means going
> back while "advance" means going forward,


Exactly - combining these is semantically confusing even if it could be 
made to work technically.


>   so it's a rather confusing
> command line to make them wrap their head around.
>
> And yes, Siddharth, you were right that the new mode should be
> incompatible with --contained, but that's because --contained is a
> special modifier of --onto.  --onto, --advance, and --revert are three
> different modes that are incompatible with each other.  Once you've
> checked for that incompatibility between the three modes, then you can
> either check that whenever --contained is specified, either --onto is
> as well, or neither --advance nor --revert are.



Right. The check becomes:
1. Exactly one of --onto, --advance, --revert must be specified
2. --contained requires --onto

This is much cleaner than my current approach of pairwise 
incompatibility checks.

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

On 26/11/25 16:40, Phillip Wood wrote:
> Hi Siddharth
>
> On 25/11/2025 17:00, Siddharth Asthana wrote:
>>
>> diff --git a/Documentation/git-replay.adoc 
>> b/Documentation/git-replay.adoc
>> index dcb26e8a8e..ad7dc08622 100644
>> --- a/Documentation/git-replay.adoc
>> +++ b/Documentation/git-replay.adoc
>> @@ -54,6 +54,18 @@ which uses the target only as a starting point 
>> without updating it.
>> [...]
>> +To revert a range of commits:
>> +
>> +------------
>> +$ git replay --revert --onto main feature~3..feature
>> +------------
>> +
>> +This creates new commits on top of 'main' that reverse the changes 
>> introduced
>> +by the last three commits on 'feature'. The 'feature' branch is 
>> updated to
>> +point at the last of these revert commits. The 'main' branch is not 
>> updated
>> +in this case.


Hi Phillip,

Thanks for the detailed analysis!


>
> I'm struggling to understand when I'd want to do this. Why would I 
> want to update 'feature' to point to the reverted version of its last 
> tree commits rebased onto 'main'?


You are absolutely right - the `--onto` example I provided doesn't make 
practical sense. Elijah's reply clarified the architecture: `--revert` 
should be its own mode, not a modifier that combines with `--onto` or 
`--advance`.


The realistic use case is reverting commits from a branch where those 
commits already exist. For example:

     git replay --revert main~3..main

This would revert the last 3 commits on main, creating revert commits on 
top of main.


>  In order to understand I ran the first tests case which does
>
>     git replay --onto topic1 --revert topic1..topic2
>
> after fixing it by adding --ref-action=print the resulting commit log 
> looks like
>
> commit d337fab78e90008835f74e890039b464a0308cbe
> Author: author@name <bogus@email@address>
> Date:   Thu Apr 7 15:30:13 2005 -0700
>
>     Revert "E
>     "
>
>     This reverts commit bceb3acd81ddd36ba0da391fffa48949a1337276.
>
> commit 47f0cc1c1f1911c0047a4d79d79f7c19c6c7151a
> Author: author@name <bogus@email@address>
> Date:   Thu Apr 7 15:30:13 2005 -0700
>
>     Revert "D
>     "
>
>     This reverts commit d953cf2dcc1da8b51934e43fd83dac72d0e267c7.
>
>
> The commits are empty because the original they are reverting each 
> create a new file which is then present in the base revision but not 
> in either of the merge heads when we revert.


This confirms the tests aren't realistic. In v2, I will create tests 
where the commits being reverted are ancestors of the replay target, so 
the reverts produce meaningful diffs.


>  This suggests to me that it is not a very realistic test and I'm 
> still scratching my head to see where "git replay --onto <commit> 
> --revert" is useful.
>
> If '--revert' does not make sense with '--onto' then perhaps it should 
> be a new mode that takes a ref and acts like '--advance' but reverts 
> the commits rather than cherry-picking them. When reverting a range of 
> commits it would reduce the likelihood of conflicts to revert then in 
> reverse order so we should either recommend passing '--reverse' or 
> make that the default when '--revert' is given.
>
> As you can see in the log output above the new function to format the 
> revert subject lines is buggy.


Good catch! The bug is in `generate_revert_message()` - I am passing 
`orig_message` (which points to the full message including body) to 
`sequencer_format_revert_header()`, but that function expects just the 
subject line.

Looking at how sequencer.c does it, they use `msg.subject` which is 
properly extracted. I need to use `commit_subject_length()` to get just 
the subject:

     int subject_len = find_commit_subject(message, &orig_message);
     char *subject = xmemdupz(orig_message, subject_len);
     generate_revert_message(&msg, subject, &based_on->object.oid);
     free(subject);


> If you had used test_commit_message() to check the commit message, 
> rather than just grepping for ^Revert the tests would have picked that 
> up.


You are right. I will use test_commit_message() for proper validation in v2.

Thanks,
Siddharth


>
> Thanks
>
> Phillip
>

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Phillip Wood]

Hi Siddharth

On 26/11/2025 19:39, Siddharth Asthana wrote:
> 
> The realistic use case is reverting commits from a branch where those 
> commits already exist. For example:
> 
>      git replay --revert main~3..main
> 
> This would revert the last 3 commits on main, creating revert commits on 
> top of main.

We want to be able to revert an arbitary range of commits. That means we 
need to give --revert a branch name to update in addition to the range 
of commits to revert. The following example would update "main", 
reverting all the commits from the branch "feature"

	git replay --revert main main..feature

Thanks

Phillip

Re: [PATCH 1/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

On 27/11/25 21:51, Phillip Wood wrote:
> Hi Siddharth
>
> On 26/11/2025 19:39, Siddharth Asthana wrote:
>>
>> The realistic use case is reverting commits from a branch where those 
>> commits already exist. For example:
>>
>>      git replay --revert main~3..main
>>
>> This would revert the last 3 commits on main, creating revert commits 
>> on top of main.
>
> We want to be able to revert an arbitary range of commits. That means 
> we need to give --revert a branch name to update in addition to the 
> range of commits to revert. The following example would update "main", 
> reverting all the commits from the branch "feature"
>
>     git replay --revert main main..feature


Makes sense. I will restructure --revert to take a branch argument, 
making it a proper mode alongside --onto and --advance:

     git replay --revert <branch> <revision-range>

This keeps the syntax consistent with the other modes.

Thanks,
Siddharth



>
> Thanks
>
> Phillip
>

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Johannes Schindelin]

Hi Siddharth,

On Tue, 25 Nov 2025, Siddharth Asthana wrote:

> The `git replay` command currently supports cherry-picking commits for
> server-side history rewriting, but lacks the ability to revert them.
> This patch adds a `--revert` option to enable reversing commits directly
> on bare repositories.
> 
> At GitLab, we use replay in Gitaly for efficient server-side operations.
> Adding revert functionality enables us to reverse problematic commits
> without client-side roundtrips, reducing network overhead.
> 
> The implementation leverages the insight that cherry-pick and revert are
> essentially the same merge operation with swapped arguments. By swapping
> the base and pickme trees when calling `merge_incore_nonrecursive()`, we
> effectively reverse the diff direction. The existing conflict handling,
> ref updates, and atomic transaction support work unchanged.

Are you reverting rebased Merge Requests commit by commit? If not, I would
suggest the shortcut to use `merge-tree` directly for the entire Merge
Request. That is, if `$BASE` corresponds to the base branch onto which the
Merge Request was rebased, and `$TIP` corresponds to the Merge Request's
rebased tip commit, then the following will revert that Merge Request:

	git merge-tree --merge-base $TIP HEAD $BASE

The upside is that this can potentially avoid a lot of unnecessary merge
conflicts. The downside is that it does not revert the rebased Merge
Request commit by commit.

The patch itself looks fine to me, if a bit too extensive on the side of
adding tests: Remember, a nimble test suite that catches a bug once is
better than a long-running test suite that would catch a bug several times
_iff_ it didn't tax the developer's patience so much that it is
interrupted and aborted. You probably agree that Git's CI runtimes are
already counter-productively long.

Ciao,
Johannes

> The revert message generation logic is extracted into a new shared
> `sequencer_format_revert_header()` function in `sequencer.c`, allowing
> code reuse between `sequencer.c` and `builtin/replay.c`. The commit
> messages follow `git revert` conventions, including "Revert"/"Reapply"
> prefixes and the original commit SHA.
> 
> This patch includes comprehensive tests covering various scenarios:
> bare repositories, --advance mode, conflicts, reapply behavior, and
> multiple commits.
> 
> Siddharth Asthana (1):
>   replay: add --revert option to reverse commit changes
> 
>  Documentation/git-replay.adoc |  35 +++++++-
>  builtin/replay.c              |  86 ++++++++++++++----
>  sequencer.c                   |  23 +++++
>  sequencer.h                   |   8 ++
>  t/t3650-replay-basics.sh      | 160 ++++++++++++++++++++++++++++++++++
>  5 files changed, 295 insertions(+), 17 deletions(-)
> 
> -- 
> 2.51.0
> 
> 

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The patch itself looks fine to me, if a bit too extensive on the side of
> adding tests: Remember, a nimble test suite that catches a bug once is
> better than a long-running test suite that would catch a bug several times
> _iff_ it didn't tax the developer's patience so much that it is
> interrupted and aborted. You probably agree that Git's CI runtimes are
> already counter-productively long.

I am not sure about some of the negations in the above, but it is
very good to point out that tests want to cover widely but without
overlap.  Two tests that try to see the tool works well under
identical scenarios can be better done as a single test.  We do not
need to catch the same bug in multiple tests, as people tend to see
test breakages, update the code to fix the first one, and continue,
wanting to fix more and different kind of breakages.

Thanks.

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

On 25/11/25 22:55, Johannes Schindelin wrote:
> Hi Siddharth,
>
> On Tue, 25 Nov 2025, Siddharth Asthana wrote:
>
>> The `git replay` command currently supports cherry-picking commits for
>> server-side history rewriting, but lacks the ability to revert them.
>> This patch adds a `--revert` option to enable reversing commits directly
>> on bare repositories.
>>
>> At GitLab, we use replay in Gitaly for efficient server-side operations.
>> Adding revert functionality enables us to reverse problematic commits
>> without client-side roundtrips, reducing network overhead.
>>
>> The implementation leverages the insight that cherry-pick and revert are
>> essentially the same merge operation with swapped arguments. By swapping
>> the base and pickme trees when calling `merge_incore_nonrecursive()`, we
>> effectively reverse the diff direction. The existing conflict handling,
>> ref updates, and atomic transaction support work unchanged.


Hi Johannes,
Thanks for the review!


> Are you reverting rebased Merge Requests commit by commit? If not, I would
> suggest the shortcut to use `merge-tree` directly for the entire Merge
> Request.


That's a great point. At GitLab, we have use cases for both approaches:

1.  For quick undoing an entire MR, the `merge-tree` approach you 
suggest is indeed more efficient and avoids unnecessary intermediate 
conflicts.

2. For commit-by-commit reverts, we need individual revert commits with 
proper attribution (which commit is being reverted) for auditability and 
history clarity. This is particularly useful when only specific commits 
from a merged branch need to be reverted.


I will add a note in the documentation mentioning the `merge-tree` 
alternative for whole-MR reverts.


> That is, if `$BASE` corresponds to the base branch onto which the
> Merge Request was rebased, and `$TIP` corresponds to the Merge Request's
> rebased tip commit, then the following will revert that Merge Request:
>
> 	git merge-tree --merge-base $TIP HEAD $BASE
>
> The upside is that this can potentially avoid a lot of unnecessary merge
> conflicts. The downside is that it does not revert the rebased Merge
> Request commit by commit.
>
> The patch itself looks fine to me, if a bit too extensive on the side of
> adding tests


Agreed. Looking at the tests again, I can consolidate several of them:
- The bare repo test can be merged with the basic revert test
- The multiple commits test overlaps with the basic functionality

I will trim down to essential coverage in v2: basic revert, conflict 
handling, and Reapply behavior.

Thanks,
Siddharth


> : Remember, a nimble test suite that catches a bug once is
> better than a long-running test suite that would catch a bug several times
> _iff_ it didn't tax the developer's patience so much that it is
> interrupted and aborted. You probably agree that Git's CI runtimes are
> already counter-productively long.
>
> Ciao,
> Johannes
>
>> The revert message generation logic is extracted into a new shared
>> `sequencer_format_revert_header()` function in `sequencer.c`, allowing
>> code reuse between `sequencer.c` and `builtin/replay.c`. The commit
>> messages follow `git revert` conventions, including "Revert"/"Reapply"
>> prefixes and the original commit SHA.
>>
>> This patch includes comprehensive tests covering various scenarios:
>> bare repositories, --advance mode, conflicts, reapply behavior, and
>> multiple commits.
>>
>> Siddharth Asthana (1):
>>    replay: add --revert option to reverse commit changes
>>
>>   Documentation/git-replay.adoc |  35 +++++++-
>>   builtin/replay.c              |  86 ++++++++++++++----
>>   sequencer.c                   |  23 +++++
>>   sequencer.h                   |   8 ++
>>   t/t3650-replay-basics.sh      | 160 ++++++++++++++++++++++++++++++++++
>>   5 files changed, 295 insertions(+), 17 deletions(-)
>>
>> -- 
>> 2.51.0
>>
>>

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

> 1. For quick undoing an entire MR, the `merge-tree` approach you 
> suggest is indeed more efficient and avoids unnecessary intermediate 
> conflicts.
>
> 2. For commit-by-commit reverts, we need individual revert commits with 
> proper attribution (which commit is being reverted) for auditability and 
> history clarity. This is particularly useful when only specific commits 
> from a merged branch need to be reverted.

These are both good workflows with appropriate uses.  To make the
tool useful for #2, it needs to be able to allow "I have merged a
topic with 7 commits, but the first commit and the fourth commit are
faulty and I need to revert them", i.e., not just a range (like
"rebase" and "cherry-pick" workflows take), but a set of commits
that are potentially disconnected.  The current command line
arguments "git replay" supports, or "git revert A..B" for that
matter, are not exactly a good fit for such a use case, although the
user can of course run two single-commit revert operations in a row.

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

On 27/11/25 02:34, Junio C Hamano wrote:
> Siddharth Asthana <siddharthasthana31@gmail.com> writes:
>
>> 1. For quick undoing an entire MR, the `merge-tree` approach you
>> suggest is indeed more efficient and avoids unnecessary intermediate
>> conflicts.
>>
>> 2. For commit-by-commit reverts, we need individual revert commits with
>> proper attribution (which commit is being reverted) for auditability and
>> history clarity. This is particularly useful when only specific commits
>> from a merged branch need to be reverted.
> These are both good workflows with appropriate uses.  To make the
> tool useful for #2, it needs to be able to allow "I have merged a
> topic with 7 commits, but the first commit and the fourth commit are
> faulty and I need to revert them", i.e., not just a range


Since replay uses the same rev-list machinery as `git log`, users can 
already specify disconnected commits:

     git replay --revert <target> <commit1> <commit4>

I will add a test to verify this works and document the capability.

Thanks,
Siddharth


> (like
> "rebase" and "cherry-pick" workflows take), but a set of commits
> that are potentially disconnected.  The current command line
> arguments "git replay" supports, or "git revert A..B" for that
> matter, are not exactly a good fit for such a use case, although the
> user can of course run two single-commit revert operations in a row.
>

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

>> These are both good workflows with appropriate uses.  To make the
>> tool useful for #2, it needs to be able to allow "I have merged a
>> topic with 7 commits, but the first commit and the fourth commit are
>> faulty and I need to revert them", i.e., not just a range
>
>
> Since replay uses the same rev-list machinery as `git log`, users can 
> already specify disconnected commits:
>
>      git replay --revert <target> <commit1> <commit4>

Ah, OK.

Yes, you can use setup_revisions() command line parsing, intercept
the result of parsing before prepare_revilimit_list() reduces that
to a contiguous set of commits that are reachable from some of the
positive refs and are not reachable from any of the negative ones,
so it certainly is doable.

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Elijah Newren]

On Thu, Nov 27, 2025 at 11:21 AM Siddharth Asthana
<siddharthasthana31@gmail.com> wrote:
>
> On 27/11/25 02:34, Junio C Hamano wrote:
> > Siddharth Asthana <siddharthasthana31@gmail.com> writes:
> >
> >> 1. For quick undoing an entire MR, the `merge-tree` approach you
> >> suggest is indeed more efficient and avoids unnecessary intermediate
> >> conflicts.
> >>
> >> 2. For commit-by-commit reverts, we need individual revert commits with
> >> proper attribution (which commit is being reverted) for auditability and
> >> history clarity. This is particularly useful when only specific commits
> >> from a merged branch need to be reverted.
> > These are both good workflows with appropriate uses.  To make the
> > tool useful for #2, it needs to be able to allow "I have merged a
> > topic with 7 commits, but the first commit and the fourth commit are
> > faulty and I need to revert them", i.e., not just a range
>
>
> Since replay uses the same rev-list machinery as `git log`, users can
> already specify disconnected commits:
>
>      git replay --revert <target> <commit1> <commit4>

No, this command does not specify disconnected commits.  A <range> of
"<commit1> <commit4>" specifies all commits in the history of either
<commit1> or <commit4>.  Thus, this example command line would be
asking to revert all commits in the history of either <commit1> or
<commit4> (all the way back to the initial commit), rather than just
reverting those two commits.  This is just like how
   git log <commit1> <commit4>
shows all commits in the history of either <commit1> or <commit4>
instead of just showing those two commits.

There isn't really a mechanism in replay right now to handle a
disconnected set of commits for either --advance or --revert.  If
there were, it'd probably look like

   git replay --advance <branch> --no-walk <commit1> <commit4>

but the code isn't set up to check whether you specified --no-walk,
and thinks "Um, you specified multiple branches here and it's not
clear the order in which to cherry-pick them" so it throws an error:

$ git replay --advance main --no-walk Commit1 Commit7
fatal: cannot advance target with multiple sources because ordering
would be ill-defined

If you comment out the relevant check which dies with that error, then
you end up in some codepath that segfaults instead (not-properly
initialized commit/tree objects or something?).  I'm sure that could
be fixed, but "users can already specify disconnected commits" is just
not accurate.

> I will add a test to verify this works and document the capability.

Supporting --no-walk so that folks can do disconnected commits for
both --advance and --revert may be nice, but given that it's missing
for --advance already, it might be considered a separate change from
your current submission.  I'll leave that up to you.

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Siddharth Asthana]

On 28/11/25 13:37, Elijah Newren wrote:
> On Thu, Nov 27, 2025 at 11:21 AM Siddharth Asthana
> <siddharthasthana31@gmail.com> wrote:
>> On 27/11/25 02:34, Junio C Hamano wrote:
>>> Siddharth Asthana <siddharthasthana31@gmail.com> writes:
>>>
>>>> 1. For quick undoing an entire MR, the `merge-tree` approach you
>>>> suggest is indeed more efficient and avoids unnecessary intermediate
>>>> conflicts.
>>>>
>>>> 2. For commit-by-commit reverts, we need individual revert commits with
>>>> proper attribution (which commit is being reverted) for auditability and
>>>> history clarity. This is particularly useful when only specific commits
>>>> from a merged branch need to be reverted.
>>> These are both good workflows with appropriate uses.  To make the
>>> tool useful for #2, it needs to be able to allow "I have merged a
>>> topic with 7 commits, but the first commit and the fourth commit are
>>> faulty and I need to revert them", i.e., not just a range
>>
>> Since replay uses the same rev-list machinery as `git log`, users can
>> already specify disconnected commits:
>>
>>       git replay --revert <target> <commit1> <commit4>


Hi Elijah,


> No, this command does not specify disconnected commits.  A <range> of
> "<commit1> <commit4>" specifies all commits in the history of either
> <commit1> or <commit4>.


You are right, I misspoke. I was conflating the command-line syntax with 
what the revision machinery actually produces after prepare_revision_walk().


>    Thus, this example command line would be
> asking to revert all commits in the history of either <commit1> or
> <commit4> (all the way back to the initial commit), rather than just
> reverting those two commits.  This is just like how
>     git log <commit1> <commit4>
> shows all commits in the history of either <commit1> or <commit4>
> instead of just showing those two commits.
>
> There isn't really a mechanism in replay right now to handle a
> disconnected set of commits for either --advance or --revert.


Right, the check at line 190 in replay.c:

     if (rinfo.positive_refexprs > 1)
         die(_("cannot advance target with multiple sources..."));

fires before we even get to the revision walk.


> If there were, it'd probably look like
>
>     git replay --advance <branch> --no-walk <commit1> <commit4>
>
> but the code isn't set up to check whether you specified --no-walk,
> and thinks "Um, you specified multiple branches here and it's not
> clear the order in which to cherry-pick them" so it throws an error:
>
> $ git replay --advance main --no-walk Commit1 Commit7
> fatal: cannot advance target with multiple sources because ordering
> would be ill-defined
>
> If you comment out the relevant check which dies with that error, then
> you end up in some codepath that segfaults instead (not-properly
> initialized commit/tree objects or something?).  I'm sure that could
> be fixed, but "users can already specify disconnected commits" is just
> not accurate.
>
>> I will add a test to verify this works and document the capability.
> Supporting --no-walk so that folks can do disconnected commits for
> both --advance and --revert may be nice, but given that it's missing
> for --advance already, it might be considered a separate change from
> your current submission.  I'll leave that up to you.



Agreed. I will keep the current submission focused on basic --revert 
functionality. Supporting --no-walk for disconnected commits (benefiting 
both --advance and --revert) would make a nice follow-up series.

Thanks for the correction.

Siddharth

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

> Agreed. I will keep the current submission focused on basic --revert 
> functionality. Supporting --no-walk for disconnected commits (benefiting 
> both --advance and --revert) would make a nice follow-up series.

If we want to have a useful support for disconnected set of commits,
"--no-walk" is not the way to go, I would say.

Imagine "among the 7-patch topic merged, the second commit (i.e.,
topic~5) and the final 3 (i.e., topic~3..topic) need to go".  You'd
want to be able to say (without going into details of the syntax)

    revert topic~5 topic~3..topic

The setup_revisions() parser is still the right thing to use to
parse the command line arguments and pick out "topic~5" and
"topic~3..topic", but instead of letting prepare_revision_walk()
turn them into a single contiguous set of revisions, you'd need to
check revs->cmdline->rev[] and

 (1) treat singleton as its own disconnected island that require no
     walking,

 (2) treat A..B as a range and independently walk them, and

 (3) dedup the result from cmdline->rev[] elements into a set of
     commits that are potentially disconnected.

I agree 100% that this topic should not attempt to deal with a
disconnected set of commits.  That can and should be done as a
separate series.

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Elijah Newren]

On Fri, Nov 28, 2025 at 8:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Siddharth Asthana <siddharthasthana31@gmail.com> writes:
>
> > Agreed. I will keep the current submission focused on basic --revert
> > functionality. Supporting --no-walk for disconnected commits (benefiting
> > both --advance and --revert) would make a nice follow-up series.
>
> If we want to have a useful support for disconnected set of commits,
> "--no-walk" is not the way to go, I would say.
>
> Imagine "among the 7-patch topic merged, the second commit (i.e.,
> topic~5) and the final 3 (i.e., topic~3..topic) need to go".  You'd
> want to be able to say (without going into details of the syntax)
>
>     revert topic~5 topic~3..topic
>
> The setup_revisions() parser is still the right thing to use to
> parse the command line arguments and pick out "topic~5" and
> "topic~3..topic", but instead of letting prepare_revision_walk()
> turn them into a single contiguous set of revisions, you'd need to
> check revs->cmdline->rev[] and
>
>  (1) treat singleton as its own disconnected island that require no
>      walking,
>
>  (2) treat A..B as a range and independently walk them, and
>
>  (3) dedup the result from cmdline->rev[] elements into a set of
>      commits that are potentially disconnected.
>
> I agree 100% that this topic should not attempt to deal with a
> disconnected set of commits.  That can and should be done as a
> separate series.

How does one distinguish the "topic~5" in the range "topic~5
topic~3..topic" from
  * the topic~5 in "^topic~7 topic~5"
  * the "topic1" and "topic2" in "^$OLD_COMMIT --ancestry-path topic1 topic2"
?
I kind of think we still need some kind of flag (possibly implied by
default for --advance and --revert but not for --onto?), though I
agree it'd need to be a new one rather than --no-walk for your example
to work.

And if one can do this, should this flag also be added to other
commands, so that e.g. `git log <someflag> topic~5 topic~3..topic"
would also show the commits in topic~3..topic plus topic~5?

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> How does one distinguish the "topic~5" in the range "topic~5
> topic~3..topic" from
>   * the topic~5 in "^topic~7 topic~5"

Two answers.

(1) You don't have to.  When you scan cmdline->rev[], you can notice
    the ^topic-7 form and reject, saying "we accept A..B but not ^A B."

(2) Or you design and document the interpretation you implement when
    you see a negative commit while you scan over cmdline->rev[].
    Perhaps you may make "^topic-7" to require a positive commit
    after it and convert "^topic-7 topic5" as if the user gave you a
    single "topic~7..topic~5".  Or you may do something else.

My assumption has been (1).


>   * the "topic1" and "topic2" in "^$OLD_COMMIT --ancestry-path topic1 topic2"

I haven't thought it through, but doesn't ancetry-path imply you are
really interested in the traditional connected set of commits?  The
path is a connected subset inside those commits after all, no?

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Elijah Newren]

On Fri, Nov 28, 2025 at 12:50 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > How does one distinguish the "topic~5" in the range "topic~5
> > topic~3..topic" from
> >   * the topic~5 in "^topic~7 topic~5"
>
> Two answers.
>
> (1) You don't have to.  When you scan cmdline->rev[], you can notice
>     the ^topic-7 form and reject, saying "we accept A..B but not ^A B."

Using revision ranges like ^A B C with --onto (rebasing several
branches at once) was one of the major usecases for git replay.
Because of that, even when I only have one branch, I often use ^A B
over A..B.

It's also called out pretty explicitly in the manual:
```
When calling `git replay`, one does not need to specify a range of
commits to replay using the syntax `A..B`; any range expression will
do:

------------
$ git replay --onto origin/main ^base branch1 branch2 branch3
------------

This will simultaneously rebase `branch1`, `branch2`, and `branch3`,
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.
```

Granted, unlike --onto, using --advance (or --revert) with multiple
branches at once doesn't make sense because cherry-picking/reverting
multiple branches into one has an ill-defined ordering problem.  Since
only one branch is allowed, maybe we could make special rules for
--advance and --revert, but it would feel a little weird to have a
revision range not actually be a revision range for some modes of the
command while also having them be an actual revision range for the
other mode.

> (2) Or you design and document the interpretation you implement when
>     you see a negative commit while you scan over cmdline->rev[].
>     Perhaps you may make "^topic-7" to require a positive commit
>     after it and convert "^topic-7 topic5" as if the user gave you a
>     single "topic~7..topic~5".  Or you may do something else.

Part of the point of replay was to "use the same revision ranges that
git log allows".  I agree that having a way to specify "topic~3..topic
plus the commit topic~5" is useful for this command, but it's equally
likely to be useful for `git log`; people have asked for it before.
However, there is currently no way to specify that set of revisions to
`git log`.  Perhaps we should add some flag to revision walking that
allows that kind of alternate rule?

If we do that, then both log and replay would benefit.  We could even
consider making that flag be implied by both --advance and --revert
(but NOT by --onto).  If we do that, we would also need a way to
negate that flag, since someone will probably want a way to
cherry-pick some disconnected branch down to the root commit, meaning
they really would want "topic~5" to be treated as a traditional range
and not as a single commit.

> >   * the "topic1" and "topic2" in "^$OLD_COMMIT --ancestry-path topic1 topic2"
>
> I haven't thought it through, but doesn't ancetry-path imply you are
> really interested in the traditional connected set of commits?  The
> path is a connected subset inside those commits after all, no?

Yes, I believe --ancestry-path makes it clear that a range is wanted.
It's not clear to me that we can always correctly guess
old-style-range vs. new-style-range from the existing command line
arguments alone, particularly given the example above of single commit
vs. commits back to the root.  I think an explicit flag is needed, if
we want to go this route.

(Also, this feels like a fair amount of work, when people can easily
just invoke replay multiple times...)

Re: [PATCH 0/1] replay: add --revert option to reverse commit changes

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> It's also called out pretty explicitly in the manual:
> ```
> When calling `git replay`, one does not need to specify a range of
> commits to replay using the syntax `A..B`; any range expression will
> do:

When we use the phrase "any range expression" to refer to what
prepare_revision_walk() produces, it by definition means we only
deal with a connected set.

It often is very handy to allow saying "master..topic1 topic2" or
"^master topic1 topic2" or "topic1 topic2 --not master" to mean
"commits on these branches", of course, and there are many such
useful use cases that do not require disjoint set (and that is why
we survived without any disjoint set support on "git log" side,
except for individually specifying commits and say "--no-walk",
which is still technically a "set" but not very useful one when the
number of commits you individually have to specify becomes more than
a handful).

It only means that the documentation needs to be updated if we ever
want to introduce an extended form of the command line syntax that
allows users to specify an unconnected set of commits.

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

[Siddharth Asthana]

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.

The implementation follows the same approach as sequencer.c where
cherry-pick and revert are the same merge operation but with swapped
arguments. For cherry-pick we merge(ancestor=parent, ours=current,
theirs=commit), while for revert we merge(ancestor=commit, ours=current,
theirs=parent). By swapping the base and pickme trees when calling
merge_incore_nonrecursive(), we effectively reverse the diff direction.

The series is structured as follows:

Patch 1 extracts the revert message formatting logic into a shared
sequencer_format_revert_header() function, eliminating code duplication
between sequencer.c and the upcoming replay code. This follows Junio's
suggestion to split the changes.

Patch 2 adds the --revert <branch> mode to git replay. Following the
architectural pattern suggested by Elijah and Phillip, --revert is a
standalone mode (like --onto and --advance) that takes a branch argument
and updates that branch with the revert commits.

The series is based on top of f0ef5b6d9b (The fifth batch, 2025-11-29).

CI: https://gitlab.com/gitlab-org/git/-/pipelines/2191734674
The static-analysis and Windows CI failures are pre-existing
infrastructure issues unrelated to this series.

Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
---
Changes in v2:
- Split into two patches as Junio suggested
- Changed --revert from a modifier flag to a standalone mode
  (--revert <branch>) that is mutually exclusive with --onto/--advance
- Used enum replay_action instead of int is_revert
- Fixed subject extraction bug in generate_revert_message()
- Put common (cherry-pick) case first in if/else block
- Fixed repo_unuse_commit_buffer() to use repo parameter consistently
- Improved tests with test_commit_message() and atomic ref update handling
- Link to v1: https://lore.kernel.org/git/20251125170055.64991-1-siddharthasthana31@gmail.com/

Siddharth Asthana (2):
  sequencer: extract revert message formatting into shared function
  replay: add --revert mode to reverse commit changes

 Documentation/git-replay.adoc |  36 ++++++++-
 builtin/replay.c              | 145 ++++++++++++++++++++++++++++------
 sequencer.c                   |  39 +++++----
 sequencer.h                   |   8 ++
 t/t3650-replay-basics.sh      | 111 ++++++++++++++++++++++++++
 5 files changed, 300 insertions(+), 39 deletions(-)


Range-diff against v1:
-:  ---------- > 1:  bfd75484b4 sequencer: extract revert message formatting into shared function
1:  dd47a89a5b ! 2:  a2f99bc8c2 replay: add --revert option to reverse commit changes
    @@ Metadata
     Author: Siddharth Asthana <siddharthasthana31@gmail.com>
     
      ## Commit message ##
    -    replay: add --revert option to reverse commit changes
    +    replay: add --revert mode to reverse commit changes
     
         The `git replay` command performs server-side history rewriting without
         requiring a working tree. While it currently supports cherry-picking
    -    commits, it lacks the ability to revert them.
    +    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` option that reverses the changes introduced by the
    -    specified commits. The implementation follows the same approach as
    -    `sequencer.c` (around lines 2358-2390), where cherry-pick and revert
    -    are essentially the same merge operation but with swapped arguments:
    +    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.
    +
    +    The implementation follows the same approach as sequencer.c (lines
    +    2360-2399), where cherry-pick and revert are the same merge operation
    +    but with swapped arguments:
     
           - Cherry-pick: merge(ancestor=parent, ours=current, theirs=commit)
           - Revert: merge(ancestor=commit, ours=current, theirs=parent)
     
         We swap the base and pickme trees when calling
    -    `merge_incore_nonrecursive()`, effectively reversing the diff direction.
    +    merge_incore_nonrecursive(), effectively reversing the diff direction.
         The existing conflict handling, ref updates, and atomic transaction
         support work unchanged.
     
    -    The revert message generation logic (handling "Revert" and "Reapply"
    -    cases) is extracted into a new `sequencer_format_revert_header()`
    -    function in `sequencer.c`, which can be shared between `sequencer.c`
    -    and `builtin/replay.c`. The existing code in `do_pick_commit()` is
    -    updated to use this shared function, eliminating code duplication.
    -    The `builtin/replay.c` code calls this shared function and then appends
    -    the commit OID using `oid_to_hex()` directly, since git replay is
    -    designed for simpler server-side operations without the interactive
    -    features and `replay_opts` framework used by `sequencer.c`.
    -
    -    The commit messages follow `git revert` conventions: prefixed with
    +    The commit messages follow git revert conventions: prefixed with
         "Revert" and including the original commit SHA. When reverting a commit
         that itself starts with "Revert", the message uses "Reapply" instead.
         Unlike cherry-pick which preserves the original author, revert commits
    -    use the current user as the author, matching the behavior of `git
    -    revert`.
    -
    -    Mark the option as incompatible with `--contained` because reverting
    -    changes across multiple branches simultaneously could produce unexpected
    -    results when branches have interdependencies or shared history.
    +    use the current user as the author, matching the behavior of git revert.
     
         Helped-by: Christian Couder <christian.couder@gmail.com>
         Helped-by: Patrick Steinhardt <ps@pks.im>
    +    Helped-by: Elijah Newren <newren@gmail.com>
    +    Helped-by: Phillip Wood <phillip.wood123@gmail.com>
    +    Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
     
      ## Documentation/git-replay.adoc ##
    @@ Documentation/git-replay.adoc: git-replay - EXPERIMENTAL: Replay commits on a ne
      --------
      [verse]
     -(EXPERIMENTAL!) 'git replay' ([--contained] --onto <newbase> | --advance <branch>) [--ref-action[=<mode>]] <revision-range>...
    -+(EXPERIMENTAL!) 'git replay' ([--contained] --onto <newbase> | --advance <branch>) [--ref-action[=<mode>]] [--revert] <revision-range>...
    ++(EXPERIMENTAL!) 'git replay' ([--contained] --onto <newbase> | --advance <branch> | --revert <branch>) [--ref-action[=<mode>]] <revision-range>...
      
      DESCRIPTION
      -----------
    -@@ Documentation/git-replay.adoc: which uses the target only as a starting point without updating it.
    - +
    - The default mode can be configured via the `replay.refAction` configuration variable.
    +@@ Documentation/git-replay.adoc: 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::
    -+	Revert the changes introduced by the commits in the revision range
    -+	instead of applying them. This reverses the diff direction and creates
    -+	new commits that undo the changes, similar to `git revert`.
    ++--revert <branch>::
    ++	Starting point at which to create the new revert commits; must be a
    ++	branch name.
     ++
    -+The commit messages are prefixed with "Revert" and include the original
    -+commit SHA. If reverting a commit whose message starts with "Revert", the new
    -+message will start with "Reapply" instead. The author of the new commits
    -+will be the current user, not the original commit author.
    ++When `--revert` is specified, the commits in the revision range are reverted
    ++(their changes are undone) and the revert commits 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.
     ++
    -+This option is incompatible with `--contained` because reverting changes
    -+across multiple branches simultaneously could produce unexpected results
    -+when branches have interdependencies or shared history.
    ++The commit messages follow `git revert` conventions: prefixed with "Revert" and
    ++including the original commit SHA. 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).
    ++
     +
    - <revision-range>::
    - 	Range of commits to replay. More than one <revision-range> can
    - 	be passed, but in `--advance <branch>` mode, they should have
    + --ref-action[=<mode>]::
    + 	Control how references are updated. The mode can be:
    + +
     @@ Documentation/git-replay.adoc: 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 a range of commits:
    ++To revert commits from a branch:
     +
     +------------
    -+$ git replay --revert --onto main feature~3..feature
    ++$ git replay --revert main feature~2..feature
     +------------
     +
    -+This creates new commits on top of 'main' that reverse the changes introduced
    -+by the last three commits on 'feature'. The 'feature' branch is updated to
    -+point at the last of these revert commits. The 'main' branch is not updated
    -+in this case.
    ++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.
     +
    -+To revert commits and advance a branch:
    ++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.
     +
    -+------------
    -+$ git replay --revert --advance main feature~2..feature
    -+------------
    -+
    -+This reverts the last two commits from 'feature', applies those reverts
    -+on top of 'main', and updates 'main' to point at the result. The 'feature'
    -+branch is not updated in this case.
     +
      GIT
      ---
    @@ builtin/replay.c
      #include "strmap.h"
      #include <oidset.h>
      #include <tree.h>
    +@@ builtin/replay.c: enum ref_action_mode {
    + 	REF_ACTION_PRINT,
    + };
    + 
    ++enum replay_action {
    ++	REPLAY_PICK,
    ++	REPLAY_REVERT,
    ++};
    ++
    + static const char *short_commit_name(struct repository *repo,
    + 				     struct commit *commit)
    + {
     @@ builtin/replay.c: static char *get_author(const char *message)
      	return NULL;
      }
      
    -+/*
    -+ * Generates a revert commit message using the shared sequencer function.
    -+ * We use oid_to_hex() directly instead of refer_to_commit() since git replay
    -+ * is designed for simpler server-side operations without interactive features.
    -+ */
     +static void generate_revert_message(struct strbuf *msg,
    -+				    const char *orig_message,
    -+				    const struct object_id *oid)
    ++				    struct commit *commit,
    ++				    struct repository *repo)
     +{
    -+	sequencer_format_revert_header(msg, orig_message);
    -+	strbuf_addstr(msg, oid_to_hex(oid));
    ++	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");
    ++
    ++	free(subject);
    ++	repo_unuse_commit_buffer(repo, commit, message);
     +}
     +
      static struct commit *create_commit(struct repository *repo,
    @@ builtin/replay.c: static char *get_author(const char *message)
      				    struct commit *based_on,
     -				    struct commit *parent)
     +				    struct commit *parent,
    -+				    int is_revert)
    ++				    enum replay_action action)
      {
      	struct object_id ret;
      	struct object *obj = NULL;
     @@ builtin/replay.c: 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);
    +-	find_commit_subject(message, &orig_message);
     -	strbuf_addstr(&msg, orig_message);
     -	author = get_author(message);
    -+
    -+	if (is_revert) {
    -+		generate_revert_message(&msg, orig_message, &based_on->object.oid);
    -+		/* For revert, use current user as author */
    -+		author = NULL;
    ++	if (action == REPLAY_REVERT) {
    ++		generate_revert_message(&msg, based_on, repo);
    ++		author = xstrdup(git_author_info(IDENT_STRICT));
     +	} else {
    -+		/* Cherry-pick mode: use original commit message and author */
    ++		find_commit_subject(message, &orig_message);
     +		strbuf_addstr(&msg, orig_message);
     +		author = get_author(message);
     +	}
    -+
      	reset_ident_date();
      	if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
      				 &ret, author, NULL, sign_commit, extra)) {
    +@@ builtin/replay.c: static struct commit *create_commit(struct repository *repo,
    + 	obj = parse_object(repo, &ret);
    + 
    + out:
    +-	repo_unuse_commit_buffer(the_repository, based_on, message);
    ++	repo_unuse_commit_buffer(repo, based_on, message);
    + 	free_commit_extra_headers(extra);
    + 	free_commit_list(parents);
    + 	strbuf_release(&msg);
    +@@ builtin/replay.c: static void determine_replay_mode(struct repository *repo,
    + 				  struct rev_cmdline_info *cmd_info,
    + 				  const char *onto_name,
    + 				  char **advance_name,
    ++				  char **revert_name,
    + 				  struct commit **onto,
    + 				  struct strset **update_refs)
    + {
    +@@ builtin/replay.c: 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"));
    + 	} else {
    + 		int positive_refs_complete = (
    + 			rinfo.positive_refexprs ==
     @@ builtin/replay.c: static struct commit *pick_regular_commit(struct repository *repo,
      					  kh_oid_map_t *replayed_commits,
      					  struct commit *onto,
      					  struct merge_options *merge_opt,
     -					  struct merge_result *result)
     +					  struct merge_result *result,
    -+					  int is_revert)
    ++					  enum replay_action action)
      {
      	struct commit *base, *replayed_base;
      	struct tree *pickme_tree, *base_tree;
    @@ builtin/replay.c: static struct commit *pick_regular_commit(struct repository *r
     -	merge_opt->branch1 = short_commit_name(repo, replayed_base);
     -	merge_opt->branch2 = short_commit_name(repo, pickme);
     -	merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
    --
    ++	if (action == REPLAY_PICK) {
    ++		/* Cherry-pick: normal order */
    ++		merge_opt->branch1 = short_commit_name(repo, replayed_base);
    ++		merge_opt->branch2 = short_commit_name(repo, pickme);
    ++		merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
    + 
     -	merge_incore_nonrecursive(merge_opt,
     -				  base_tree,
     -				  result->tree,
     -				  pickme_tree,
     -				  result);
    -+	if (is_revert) {
    -+		/* For revert: swap base and pickme to reverse the diff */
    ++		merge_incore_nonrecursive(merge_opt,
    ++					  base_tree,
    ++					  result->tree,
    ++					  pickme_tree,
    ++					  result);
    + 
    +-	free((char*)merge_opt->ancestor);
    ++		free((char *)merge_opt->ancestor);
    ++	} else {
    ++		/* Revert: swap base and pickme to reverse the diff */
     +		const char *pickme_name = short_commit_name(repo, pickme);
     +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
     +		merge_opt->branch2 = xstrfmt("parent of %s", pickme_name);
    @@ builtin/replay.c: static struct commit *pick_regular_commit(struct repository *r
     +					  base_tree,
     +					  result);
     +
    -+		/* branch2 was allocated with xstrfmt, needs freeing */
     +		free((char *)merge_opt->branch2);
    -+	} else {
    -+		/* For cherry-pick: normal order */
    -+		merge_opt->branch1 = short_commit_name(repo, replayed_base);
    -+		merge_opt->branch2 = short_commit_name(repo, pickme);
    -+		merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
    -+
    -+		merge_incore_nonrecursive(merge_opt,
    -+					  base_tree,
    -+					  result->tree,
    -+					  pickme_tree,
    -+					  result);
    -+
    -+		/* ancestor was allocated with xstrfmt, needs freeing */
    -+		free((char *)merge_opt->ancestor);
     +	}
    - 
    --	free((char*)merge_opt->ancestor);
      	merge_opt->ancestor = NULL;
     +	merge_opt->branch2 = NULL;
      	if (!result->clean)
      		return NULL;
     -	return create_commit(repo, result->tree, pickme, replayed_base);
    -+	return create_commit(repo, result->tree, pickme, replayed_base, is_revert);
    ++	return create_commit(repo, result->tree, pickme, replayed_base, action);
      }
      
      static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source)
     @@ builtin/replay.c: int cmd_replay(int argc,
    + {
    + 	const char *advance_name_opt = NULL;
    + 	char *advance_name = NULL;
    ++	const char *revert_name_opt = NULL;
    ++	char *revert_name = NULL;
    ++	enum replay_action action = REPLAY_PICK;
    + 	struct commit *onto = NULL;
    + 	const char *onto_name = NULL;
      	int contained = 0;
    - 	const char *ref_action = NULL;
    - 	enum ref_action_mode ref_mode;
    -+	int is_revert = 0;
    - 
    - 	struct rev_info revs;
    - 	struct commit *last_commit = NULL;
     @@ builtin/replay.c: int cmd_replay(int argc,
    + 
      	const char *const replay_usage[] = {
      		N_("(EXPERIMENTAL!) git replay "
    - 		   "([--contained] --onto <newbase> | --advance <branch>) "
    --		   "[--ref-action[=<mode>]] <revision-range>..."),
    -+		   "[--ref-action[=<mode>]] [--revert] <revision-range>..."),
    +-		   "([--contained] --onto <newbase> | --advance <branch>) "
    ++		   "([--contained] --onto <newbase> | --advance <branch> | --revert <branch>) "
    + 		   "[--ref-action[=<mode>]] <revision-range>..."),
      		NULL
      	};
    - 	struct option replay_options[] = {
     @@ builtin/replay.c: int cmd_replay(int argc,
    + 			   N_("replay onto given commit")),
    + 		OPT_BOOL(0, "contained", &contained,
    + 			 N_("advance all branches contained in revision-range")),
    ++		OPT_STRING(0, "revert", &revert_name_opt,
    ++			   N_("branch"),
    ++			   N_("revert commits onto given branch")),
      		OPT_STRING(0, "ref-action", &ref_action,
      			   N_("mode"),
      			   N_("control ref update behavior (update|print)")),
    -+		OPT_BOOL(0, "revert", &is_revert,
    -+			 N_("revert commits instead of cherry-picking them")),
    - 		OPT_END()
    - 	};
    - 
     @@ builtin/replay.c: int cmd_replay(int argc,
    + 	argc = parse_options(argc, argv, prefix, replay_options, replay_usage,
    + 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);
    + 
    +-	if (!onto_name && !advance_name_opt) {
    +-		error(_("option --onto or --advance is mandatory"));
    ++	/* Exactly one mode must be specified */
    ++	if (!onto_name && !advance_name_opt && !revert_name_opt) {
    ++		error(_("exactly one of --onto, --advance, or --revert is required"));
    + 		usage_with_options(replay_usage, replay_options);
    + 	}
    + 
      	die_for_incompatible_opt2(!!advance_name_opt, "--advance",
    - 				  contained, "--contained");
    +-				  contained, "--contained");
    ++				  !!onto_name, "--onto");
    ++	die_for_incompatible_opt2(!!revert_name_opt, "--revert",
    ++				  !!onto_name, "--onto");
    ++	die_for_incompatible_opt2(!!revert_name_opt, "--revert",
    ++				  !!advance_name_opt, "--advance");
    ++	die_for_incompatible_opt2(contained, "--contained",
    ++				  !onto_name, "requires --onto");
      
    -+	/* --revert is incompatible with --contained */
    -+	die_for_incompatible_opt2(is_revert, "--revert",
    -+				  contained, "--contained");
    -+
      	/* Parse ref action mode from command line or config */
      	ref_mode = get_ref_action_mode(repo, ref_action);
      
    + 	advance_name = xstrdup_or_null(advance_name_opt);
    ++	revert_name = xstrdup_or_null(revert_name_opt);
    ++	if (revert_name)
    ++		action = REPLAY_REVERT;
    + 
    + 	repo_init_revisions(repo, &revs, prefix);
    + 
    +@@ builtin/replay.c: int cmd_replay(int argc,
    + 	}
    + 
    + 	determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
    ++			      &revert_name,
    + 			      &onto, &update_refs);
    + 
    + 	/* Build reflog message */
    +-	if (advance_name_opt)
    ++	if (revert_name_opt)
    ++		strbuf_addf(&reflog_msg, "replay --revert %s", revert_name_opt);
    ++	else if (advance_name_opt)
    + 		strbuf_addf(&reflog_msg, "replay --advance %s", advance_name_opt);
    + 	else
    + 		strbuf_addf(&reflog_msg, "replay --onto %s",
     @@ builtin/replay.c: int cmd_replay(int argc,
      			die(_("replaying merge commits is not supported yet!"));
      
      		last_commit = pick_regular_commit(repo, commit, replayed_commits,
     -						  onto, &merge_opt, &result);
    -+						  onto, &merge_opt, &result,
    -+						  is_revert);
    ++						  onto, &merge_opt, &result, action);
      		if (!last_commit)
      			break;
      
    -
    - ## sequencer.c ##
    -@@ sequencer.c: static int do_pick_commit(struct repository *r,
    - 		if (opts->commit_use_reference) {
    - 			strbuf_commented_addf(&ctx->message, comment_line_str,
    - 				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
    --		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
    --			   /*
    --			    * We don't touch pre-existing repeated reverts, because
    --			    * theoretically these can be nested arbitrarily deeply,
    --			    * thus requiring excessive complexity to deal with.
    --			    */
    --			   !starts_with(orig_subject, "Revert \"")) {
    --			strbuf_addstr(&ctx->message, "Reapply \"");
    --			strbuf_addstr(&ctx->message, orig_subject);
    --			strbuf_addstr(&ctx->message, "\n");
    -+			strbuf_addstr(&ctx->message, "\nThis reverts commit ");
    - 		} else {
    --			strbuf_addstr(&ctx->message, "Revert \"");
    --			strbuf_addstr(&ctx->message, msg.subject);
    --			strbuf_addstr(&ctx->message, "\"\n");
    -+			sequencer_format_revert_header(&ctx->message, msg.subject);
    - 		}
    --		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
    - 		refer_to_commit(opts, &ctx->message, commit);
    +@@ builtin/replay.c: int cmd_replay(int argc,
    + 		kh_value(replayed_commits, pos) = last_commit;
      
    - 		if (commit->parents && commit->parents->next) {
    -@@ sequencer.c: int sequencer_pick_revisions(struct repository *r,
    - 	return res;
    - }
    + 		/* Update any necessary branches */
    +-		if (advance_name)
    ++		if (advance_name || revert_name)
    + 			continue;
    + 		decoration = get_name_decoration(&commit->object);
    + 		if (!decoration)
    +@@ builtin/replay.c: int cmd_replay(int argc,
    + 		}
    + 	}
      
    -+void sequencer_format_revert_header(struct strbuf *out, const char *orig_subject)
    -+{
    -+	const char *revert_subject;
    -+
    -+	if (skip_prefix(orig_subject, "Revert \"", &revert_subject) &&
    -+	    /*
    -+	     * We don't touch pre-existing repeated reverts, because
    -+	     * theoretically these can be nested arbitrarily deeply,
    -+	     * thus requiring excessive complexity to deal with.
    -+	     */
    -+	    !starts_with(revert_subject, "Revert \"")) {
    -+		strbuf_addstr(out, "Reapply \"");
    -+		strbuf_addstr(out, revert_subject);
    -+		strbuf_addch(out, '\n');
    -+	} else {
    -+		strbuf_addstr(out, "Revert \"");
    -+		strbuf_addstr(out, orig_subject);
    -+		strbuf_addstr(out, "\"\n");
    +-	/* In --advance mode, advance the target ref */
    ++	/* In --advance or --revert mode, update the target ref */
    + 	if (result.clean == 1 && advance_name) {
    + 		if (handle_ref_update(ref_mode, transaction, advance_name,
    + 				      &last_commit->object.oid,
    +@@ builtin/replay.c: int cmd_replay(int argc,
    + 			goto cleanup;
    + 		}
    + 	}
    ++	if (result.clean == 1 && revert_name) {
    ++		if (handle_ref_update(ref_mode, transaction, revert_name,
    ++				      &last_commit->object.oid,
    ++				      &onto->object.oid,
    ++				      reflog_msg.buf,
    ++				      &transaction_err) < 0) {
    ++			ret = error(_("failed to update ref '%s': %s"),
    ++				    revert_name, transaction_err.buf);
    ++			goto cleanup;
    ++		}
     +	}
    -+
    -+	strbuf_addstr(out, "\nThis reverts commit ");
    -+}
    -+
    - void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
    - {
    - 	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
    -
    - ## sequencer.h ##
    -@@ sequencer.h: int todo_list_rearrange_squash(struct todo_list *todo_list);
    -  */
    - void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);
      
    -+/*
    -+ * Formats a revert commit message header following standard Git conventions.
    -+ * Handles both regular reverts ("Revert \"<subject>\"") and revert of revert
    -+ * cases ("Reapply \"<subject>\""). Adds "This reverts commit " at the end.
    -+ * The caller should append the commit OID after calling this function.
    -+ */
    -+void sequencer_format_revert_header(struct strbuf *out, const char *orig_subject);
    -+
    - void append_conflicts_hint(struct index_state *istate,
    - 		struct strbuf *msgbuf, enum commit_msg_cleanup_mode cleanup_mode);
    - enum commit_msg_cleanup_mode get_cleanup_mode(const char *cleanup_arg,
    + 	/* Commit the ref transaction if we have one */
    + 	if (transaction && result.clean == 1) {
     
      ## t/t3650-replay-basics.sh ##
     @@ t/t3650-replay-basics.sh: test_expect_success 'invalid replay.refAction value' '
      	test_grep "invalid.*replay.refAction.*value" error
      '
      
    -+test_expect_success 'using replay with --revert to revert a commit' '
    -+	# Revert commits D and E from topic2
    -+	git replay --revert --onto topic1 topic1..topic2 >result &&
    -+
    -+	test_must_be_empty result &&
    -+
    -+	# Verify the commit messages contain "Revert"
    -+	# topic1..topic2 contains D and E, so we get 2 reverts on top of topic1 (which has F, C, B, A)
    -+	git log --format=%s topic2 >actual &&
    -+	test_line_count = 6 actual &&
    -+	head -n 1 actual >first-line &&
    -+	test_grep "^Revert" first-line
    ++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
     +'
     +
    -+test_expect_success 'using replay with --revert on bare repo' '
    -+	git -C bare replay --revert --onto topic1 topic1..topic2 >result-bare &&
    -+
    -+	test_must_be_empty result-bare &&
    -+
    -+	# Verify the commit message contains "Revert"
    -+	git -C bare log --format=%s topic2 >actual-bare &&
    -+	test_line_count = 6 actual-bare &&
    -+	head -n 1 actual-bare >first-line-bare &&
    -+	test_grep "^Revert" first-line-bare
    ++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_expect_success 'using replay with --revert and --advance' '
    -+	# Revert commits from topic2 and advance main
    -+	git replay --revert --advance main topic1..topic2 >result &&
    ++test_expect_success 'git replay --revert with --ref-action=print' '
    ++	# Store original state
    ++	START=$(git rev-parse revert-test) &&
    ++	test_when_finished "git branch -f revert-test $START" &&
     +
    -+	test_must_be_empty result &&
    ++	git replay --ref-action=print --revert revert-test revert-test~2..revert-test >result &&
    ++	test_line_count = 1 result &&
     +
    -+	# Verify the commit message contains "Revert"
    -+	git log --format=%s main >actual &&
    -+	head -n 1 actual >first-line &&
    -+	test_grep "^Revert" first-line
    ++	# 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 'replay with --revert fails with --contained' '
    -+	test_must_fail git replay --revert --contained --onto main main..topic3 2>error &&
    -+	test_grep "revert.*contained.*cannot be used together" error
    -+'
    ++test_expect_success 'git replay --revert reapply behavior' '
    ++	# Store original state
    ++	START=$(git rev-parse revert-test) &&
    ++	test_when_finished "git branch -f revert-test $START" &&
     +
    -+test_expect_success 'verify revert actually reverses changes' '
    -+	# Create a branch with a simple change
    -+	git switch -c revert-test main &&
    -+	echo "new content" >test-file.txt &&
    -+	git add test-file.txt &&
    -+	git commit -m "Add test file" &&
    ++	# First revert R3
    ++	git replay --revert revert-test revert-test~1..revert-test &&
    ++	REVERT_R3=$(git rev-parse revert-test) &&
     +
    -+	# Revert the commit
    -+	git replay --revert --advance revert-test HEAD^..HEAD >result &&
    ++	# Now revert the revert (should create "Reapply" message)
    ++	git replay --revert revert-test revert-test~1..revert-test >output &&
    ++	test_must_be_empty output &&
     +
    -+	test_must_be_empty result &&
    ++	# Verify Reapply message
    ++	test_commit_message revert-test -m "Reapply \"R3\"
     +
    -+	# The file should no longer exist (reverted)
    -+	test_must_fail git show revert-test:test-file.txt
    ++This reverts commit $(git rev-parse $REVERT_R3)."
     +'
     +
    -+test_expect_success 'revert of a revert creates reapply message' '
    -+	# Create a commit
    -+	git switch -c revert-revert main &&
    -+	echo "content" >revert-test-2.txt &&
    -+	git add revert-test-2.txt &&
    -+	git commit -m "Add revert test file" &&
    -+
    -+	ORIGINAL=$(git rev-parse HEAD) &&
    -+
    -+	# First revert
    -+	git replay --revert --advance revert-revert HEAD^..HEAD >result1 &&
    -+
    -+	test_must_be_empty result1 &&
    -+
    -+	# Check first revert message starts with "Revert"
    -+	git log --format=%s -1 revert-revert >msg1 &&
    -+	test_grep "^Revert" msg1 &&
    -+
    -+	FIRST_REVERT=$(git rev-parse revert-revert) &&
    -+
    -+	# Now revert the revert
    -+	git replay --revert --advance revert-revert $ORIGINAL..$FIRST_REVERT >result2 &&
    -+
    -+	test_must_be_empty result2 &&
    -+
    -+	# Check second revert message starts with "Reapply"
    -+	git log --format=%s -1 revert-revert >msg2 &&
    -+	test_grep "^Reapply" msg2 &&
    -+
    -+	# The file should exist again (reapplied)
    -+	git show revert-revert:revert-test-2.txt >actual &&
    -+	echo "content" >expected &&
    -+	test_cmp expected actual
    ++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 'replay --revert includes commit SHA in message' '
    -+	git switch -c revert-sha-test main &&
    -+	echo "test" >sha-test.txt &&
    -+	git add sha-test.txt &&
    -+	git commit -m "Test commit for SHA" &&
    -+
    -+	COMMIT_SHA=$(git rev-parse HEAD) &&
    -+	git replay --revert --advance revert-sha-test HEAD^..HEAD >result &&
    ++test_expect_success 'git replay --revert reflog message' '
    ++	# Store original state
    ++	START=$(git rev-parse revert-test) &&
    ++	test_when_finished "git branch -f revert-test $START" &&
     +
    -+	test_must_be_empty result &&
    ++	git replay --revert revert-test revert-test~1..revert-test >output &&
    ++	test_must_be_empty output &&
     +
    -+	# Check that the commit message includes the original SHA
    -+	git log --format=%B -1 revert-sha-test >msg &&
    -+	test_grep "$COMMIT_SHA" msg
    ++	# 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 'replay --revert with conflict' '
    -+	# Create a conflicting situation
    -+	git switch -c revert-conflict main &&
    -+	echo "line1" >conflict-file.txt &&
    -+	git add conflict-file.txt &&
    -+	git commit -m "Add conflict file" &&
    -+
    -+	git switch -c revert-conflict-branch HEAD^ &&
    -+	echo "different" >conflict-file.txt &&
    -+	git add conflict-file.txt &&
    -+	git commit -m "Different content" &&
    -+
    -+	# Try to revert the first commit onto the conflicting branch
    -+	test_expect_code 1 git replay --revert --onto revert-conflict-branch revert-conflict^..revert-conflict
    ++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 'replay --revert handles multiple commits' '
    -+	# Save the original topic2 state
    -+	ORIG_TOPIC2=$(git rev-parse topic2) &&
    -+	test_when_finished "git branch -f topic2 $ORIG_TOPIC2" &&
    -+
    -+	# Revert D and E from topic2, applying the reverts onto topic1
    -+	git replay --revert --onto topic1 topic1..topic2 >result &&
    -+
    -+	test_must_be_empty result &&
    -+
    -+	# Verify both revert commits appear in the log
    -+	git log --format=%s topic2 >log &&
    -+	head -n 2 log >first-two &&
    -+	test_grep "^Revert" first-two &&
    ++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
    ++'
     +
    -+	# Verify we have both "Revert D" and "Revert E"
    -+	test_grep "Revert.*E" log &&
    -+	test_grep "Revert.*D" log
    ++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
     +'
     +
      test_done

-- 
2.51.0

base-commit: f0ef5b6d9bcc258e4cbef93839d1b7465d5212b9

Thanks
- Siddharth

[PATCH v2 1/2] sequencer: extract revert message formatting into shared function

[Siddharth Asthana]

The logic for formatting revert commit messages (handling "Revert" and
"Reapply" cases) is currently duplicated between sequencer.c and will be
needed by builtin/replay.c.

Extract this logic into a new sequencer_format_revert_header() function
that can be shared. The function handles both regular reverts ("Revert
"<subject>"") and revert-of-revert cases ("Reapply "<subject>"").
Update do_pick_commit() to use the new helper, eliminating code
duplication while preserving the special handling for commit_use_reference.

Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
---
 sequencer.c | 39 +++++++++++++++++++++++++--------------
 sequencer.h |  8 ++++++++
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5476d39ba9..9f621aef4b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2365,22 +2365,10 @@ static int do_pick_commit(struct repository *r,
 		if (opts->commit_use_reference) {
 			strbuf_commented_addf(&ctx->message, comment_line_str,
 				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
-		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
-			   /*
-			    * We don't touch pre-existing repeated reverts, because
-			    * theoretically these can be nested arbitrarily deeply,
-			    * thus requiring excessive complexity to deal with.
-			    */
-			   !starts_with(orig_subject, "Revert \"")) {
-			strbuf_addstr(&ctx->message, "Reapply \"");
-			strbuf_addstr(&ctx->message, orig_subject);
-			strbuf_addstr(&ctx->message, "\n");
+			strbuf_addstr(&ctx->message, "\nThis reverts commit ");
 		} else {
-			strbuf_addstr(&ctx->message, "Revert \"");
-			strbuf_addstr(&ctx->message, msg.subject);
-			strbuf_addstr(&ctx->message, "\"\n");
+			sequencer_format_revert_header(&ctx->message, msg.subject);
 		}
-		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
 		refer_to_commit(opts, &ctx->message, commit);
 
 		if (commit->parents && commit->parents->next) {
@@ -5572,6 +5560,29 @@ int sequencer_pick_revisions(struct repository *r,
 	return res;
 }
 
+void sequencer_format_revert_header(struct strbuf *out, const char *orig_subject)
+{
+	const char *revert_subject;
+
+	if (skip_prefix(orig_subject, "Revert \"", &revert_subject) &&
+	    /*
+	     * We don't touch pre-existing repeated reverts, because
+	     * theoretically these can be nested arbitrarily deeply,
+	     * thus requiring excessive complexity to deal with.
+	     */
+	    !starts_with(revert_subject, "Revert \"")) {
+		strbuf_addstr(out, "Reapply \"");
+		strbuf_addstr(out, revert_subject);
+		strbuf_addch(out, '\n');
+	} else {
+		strbuf_addstr(out, "Revert \"");
+		strbuf_addstr(out, orig_subject);
+		strbuf_addstr(out, "\"\n");
+	}
+
+	strbuf_addstr(out, "\nThis reverts commit ");
+}
+
 void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
 {
 	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
diff --git a/sequencer.h b/sequencer.h
index 719684c8a9..114c5d2449 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -271,4 +271,12 @@ int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
  */
 int sequencer_get_update_refs_state(const char *wt_dir, struct string_list *refs);
 
+/*
+ * Formats a revert commit message header following standard Git conventions.
+ * Handles both regular reverts ("Revert \"<subject>\"") and revert of revert
+ * cases ("Reapply \"<subject>\""). Adds "This reverts commit " at the end.
+ * The caller should append the commit OID after calling this function.
+ */
+void sequencer_format_revert_header(struct strbuf *out, const char *orig_subject);
+
 #endif /* SEQUENCER_H */
-- 
2.51.0

Re: [PATCH v2 1/2] sequencer: extract revert message formatting into shared function

[Patrick Steinhardt]

On Wed, Dec 03, 2025 at 01:46:10AM +0530, Siddharth Asthana wrote:
> diff --git a/sequencer.c b/sequencer.c
> index 5476d39ba9..9f621aef4b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2365,22 +2365,10 @@ static int do_pick_commit(struct repository *r,
>  		if (opts->commit_use_reference) {
>  			strbuf_commented_addf(&ctx->message, comment_line_str,
>  				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> -		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
> -			   /*
> -			    * We don't touch pre-existing repeated reverts, because
> -			    * theoretically these can be nested arbitrarily deeply,
> -			    * thus requiring excessive complexity to deal with.
> -			    */
> -			   !starts_with(orig_subject, "Revert \"")) {
> -			strbuf_addstr(&ctx->message, "Reapply \"");
> -			strbuf_addstr(&ctx->message, orig_subject);
> -			strbuf_addstr(&ctx->message, "\n");
> +			strbuf_addstr(&ctx->message, "\nThis reverts commit ");
>  		} else {
> -			strbuf_addstr(&ctx->message, "Revert \"");
> -			strbuf_addstr(&ctx->message, msg.subject);
> -			strbuf_addstr(&ctx->message, "\"\n");
> +			sequencer_format_revert_header(&ctx->message, msg.subject);
>  		}
> -		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
>  		refer_to_commit(opts, &ctx->message, commit);
>  
>  		if (commit->parents && commit->parents->next) {

Is there any reason why we don't also handle `refer_to_commit()` in that
new function?

Patrick

Re: [PATCH v2 1/2] sequencer: extract revert message formatting into shared function

[Siddharth Asthana]

On 05/12/25 17:03, Patrick Steinhardt wrote:
> On Wed, Dec 03, 2025 at 01:46:10AM +0530, Siddharth Asthana wrote:
>> diff --git a/sequencer.c b/sequencer.c
>> index 5476d39ba9..9f621aef4b 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2365,22 +2365,10 @@ static int do_pick_commit(struct repository *r,
>>   		if (opts->commit_use_reference) {
>>   			strbuf_commented_addf(&ctx->message, comment_line_str,
>>   				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
>> -		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
>> -			   /*
>> -			    * We don't touch pre-existing repeated reverts, because
>> -			    * theoretically these can be nested arbitrarily deeply,
>> -			    * thus requiring excessive complexity to deal with.
>> -			    */
>> -			   !starts_with(orig_subject, "Revert \"")) {
>> -			strbuf_addstr(&ctx->message, "Reapply \"");
>> -			strbuf_addstr(&ctx->message, orig_subject);
>> -			strbuf_addstr(&ctx->message, "\n");
>> +			strbuf_addstr(&ctx->message, "\nThis reverts commit ");
>>   		} else {
>> -			strbuf_addstr(&ctx->message, "Revert \"");
>> -			strbuf_addstr(&ctx->message, msg.subject);
>> -			strbuf_addstr(&ctx->message, "\"\n");
>> +			sequencer_format_revert_header(&ctx->message, msg.subject);
>>   		}
>> -		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
>>   		refer_to_commit(opts, &ctx->message, commit);
>>   
>>   		if (commit->parents && commit->parents->next) {
> Is there any reason why we don't also handle `refer_to_commit()` in that
> new function?


The `refer_to_commit()` function depends on `struct replay_opts` and its 
`commit_use_reference` flag, which controls whether to use abbreviated 
commit info ("%h (%s, %ad)") or the full OID. This is specific to 
sequencer.c's interactive workflow where users can choose the reference 
style via --reference.

In replay.c, we always use the full OID via `oid_to_hex()` since it's 
designed for non-interactive server-side operations without the 
`replay_opts` framework. Including `refer_to_commit()` would require 
either passing `replay_opts` to the shared function (leaking sequencer 
internals) or adding a format parameter which feels like 
over-engineering for current needs.

Happy to reconsider if you think there's a cleaner way to share this.

Thanks,
Siddharth


>
> Patrick

Re: [PATCH v2 1/2] sequencer: extract revert message formatting into shared function

[Patrick Steinhardt]

On Mon, Dec 08, 2025 at 04:30:58AM +0530, Siddharth Asthana wrote:
> 
> On 05/12/25 17:03, Patrick Steinhardt wrote:
> > On Wed, Dec 03, 2025 at 01:46:10AM +0530, Siddharth Asthana wrote:
> > > diff --git a/sequencer.c b/sequencer.c
> > > index 5476d39ba9..9f621aef4b 100644
> > > --- a/sequencer.c
> > > +++ b/sequencer.c
> > > @@ -2365,22 +2365,10 @@ static int do_pick_commit(struct repository *r,
> > >   		if (opts->commit_use_reference) {
> > >   			strbuf_commented_addf(&ctx->message, comment_line_str,
> > >   				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> > > -		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
> > > -			   /*
> > > -			    * We don't touch pre-existing repeated reverts, because
> > > -			    * theoretically these can be nested arbitrarily deeply,
> > > -			    * thus requiring excessive complexity to deal with.
> > > -			    */
> > > -			   !starts_with(orig_subject, "Revert \"")) {
> > > -			strbuf_addstr(&ctx->message, "Reapply \"");
> > > -			strbuf_addstr(&ctx->message, orig_subject);
> > > -			strbuf_addstr(&ctx->message, "\n");
> > > +			strbuf_addstr(&ctx->message, "\nThis reverts commit ");
> > >   		} else {
> > > -			strbuf_addstr(&ctx->message, "Revert \"");
> > > -			strbuf_addstr(&ctx->message, msg.subject);
> > > -			strbuf_addstr(&ctx->message, "\"\n");
> > > +			sequencer_format_revert_header(&ctx->message, msg.subject);
> > >   		}
> > > -		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
> > >   		refer_to_commit(opts, &ctx->message, commit);
> > >   		if (commit->parents && commit->parents->next) {
> > Is there any reason why we don't also handle `refer_to_commit()` in that
> > new function?
> 
> 
> The `refer_to_commit()` function depends on `struct replay_opts` and its
> `commit_use_reference` flag, which controls whether to use abbreviated
> commit info ("%h (%s, %ad)") or the full OID. This is specific to
> sequencer.c's interactive workflow where users can choose the reference
> style via --reference.
> 
> In replay.c, we always use the full OID via `oid_to_hex()` since it's
> designed for non-interactive server-side operations without the
> `replay_opts` framework. Including `refer_to_commit()` would require either
> passing `replay_opts` to the shared function (leaking sequencer internals)
> or adding a format parameter which feels like over-engineering for current
> needs.
> 
> Happy to reconsider if you think there's a cleaner way to share this.

A simple alternative might be to convert the `struct replay_opts`
parameter into a `flags` field that tells the function whether it is
expected to use the object ID or whether it should try using the
abbreviated commit info instead.

Patrick

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

[Siddharth Asthana]

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.

The implementation follows the same approach as sequencer.c (lines
2360-2399), where cherry-pick and revert are the same merge operation
but with swapped arguments:

  - Cherry-pick: merge(ancestor=parent, ours=current, theirs=commit)
  - Revert: merge(ancestor=commit, ours=current, theirs=parent)

We swap the base and pickme trees when calling
merge_incore_nonrecursive(), effectively reversing the diff direction.
The existing conflict handling, ref updates, and atomic transaction
support work unchanged.

The commit messages follow git revert conventions: prefixed with
"Revert" and including the original commit SHA. When reverting a commit
that itself starts with "Revert", the message uses "Reapply" instead.
Unlike cherry-pick which preserves the original author, revert commits
use the current user as the author, matching the behavior of git revert.

Helped-by: Christian Couder <christian.couder@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Elijah Newren <newren@gmail.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
---
 Documentation/git-replay.adoc |  36 ++++++++-
 builtin/replay.c              | 145 ++++++++++++++++++++++++++++------
 t/t3650-replay-basics.sh      | 111 ++++++++++++++++++++++++++
 3 files changed, 267 insertions(+), 25 deletions(-)

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 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.
++
+The commit messages follow `git revert` conventions: prefixed with "Revert" and
+including the original commit SHA. 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.
+
+
 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,
+};
+
 static const char *short_commit_name(struct repository *repo,
 				     struct commit *commit)
 {
@@ -57,10 +63,32 @@ static char *get_author(const char *message)
 	return NULL;
 }
 
+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");
+
+	free(subject);
+	repo_unuse_commit_buffer(repo, commit, message);
+}
+
 static struct commit *create_commit(struct repository *repo,
 				    struct tree *tree,
 				    struct commit *based_on,
-				    struct commit *parent)
+				    struct commit *parent,
+				    enum replay_action action)
 {
 	struct object_id ret;
 	struct object *obj = NULL;
@@ -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));
+	} else {
+		find_commit_subject(message, &orig_message);
+		strbuf_addstr(&msg, orig_message);
+		author = get_author(message);
+	}
 	reset_ident_date();
 	if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
 				 &ret, author, NULL, sign_commit, extra)) {
@@ -90,7 +123,7 @@ static struct commit *create_commit(struct repository *repo,
 	obj = parse_object(repo, &ret);
 
 out:
-	repo_unuse_commit_buffer(the_repository, based_on, message);
+	repo_unuse_commit_buffer(repo, based_on, message);
 	free_commit_extra_headers(extra);
 	free_commit_list(parents);
 	strbuf_release(&msg);
@@ -166,6 +199,7 @@ static void determine_replay_mode(struct repository *repo,
 				  struct rev_cmdline_info *cmd_info,
 				  const char *onto_name,
 				  char **advance_name,
+				  char **revert_name,
 				  struct commit **onto,
 				  struct strset **update_refs)
 {
@@ -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"));
 	} else {
 		int positive_refs_complete = (
 			rinfo.positive_refexprs ==
@@ -261,7 +309,8 @@ static struct commit *pick_regular_commit(struct repository *repo,
 					  kh_oid_map_t *replayed_commits,
 					  struct commit *onto,
 					  struct merge_options *merge_opt,
-					  struct merge_result *result)
+					  struct merge_result *result,
+					  enum replay_action action)
 {
 	struct commit *base, *replayed_base;
 	struct tree *pickme_tree, *base_tree;
@@ -273,21 +322,39 @@ static struct commit *pick_regular_commit(struct repository *repo,
 	pickme_tree = repo_get_commit_tree(repo, pickme);
 	base_tree = repo_get_commit_tree(repo, base);
 
-	merge_opt->branch1 = short_commit_name(repo, replayed_base);
-	merge_opt->branch2 = short_commit_name(repo, pickme);
-	merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
+	if (action == REPLAY_PICK) {
+		/* Cherry-pick: normal order */
+		merge_opt->branch1 = short_commit_name(repo, replayed_base);
+		merge_opt->branch2 = short_commit_name(repo, pickme);
+		merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
 
-	merge_incore_nonrecursive(merge_opt,
-				  base_tree,
-				  result->tree,
-				  pickme_tree,
-				  result);
+		merge_incore_nonrecursive(merge_opt,
+					  base_tree,
+					  result->tree,
+					  pickme_tree,
+					  result);
 
-	free((char*)merge_opt->ancestor);
+		free((char *)merge_opt->ancestor);
+	} else {
+		/* Revert: swap base and pickme to reverse the diff */
+		const char *pickme_name = short_commit_name(repo, pickme);
+		merge_opt->branch1 = short_commit_name(repo, replayed_base);
+		merge_opt->branch2 = xstrfmt("parent of %s", pickme_name);
+		merge_opt->ancestor = pickme_name;
+
+		merge_incore_nonrecursive(merge_opt,
+					  pickme_tree,
+					  result->tree,
+					  base_tree,
+					  result);
+
+		free((char *)merge_opt->branch2);
+	}
 	merge_opt->ancestor = NULL;
+	merge_opt->branch2 = NULL;
 	if (!result->clean)
 		return NULL;
-	return create_commit(repo, result->tree, pickme, replayed_base);
+	return create_commit(repo, result->tree, pickme, replayed_base, action);
 }
 
 static enum ref_action_mode parse_ref_action_mode(const char *ref_action, const char *source)
@@ -345,6 +412,9 @@ int cmd_replay(int argc,
 {
 	const char *advance_name_opt = NULL;
 	char *advance_name = NULL;
+	const char *revert_name_opt = NULL;
+	char *revert_name = NULL;
+	enum replay_action action = REPLAY_PICK;
 	struct commit *onto = NULL;
 	const char *onto_name = NULL;
 	int contained = 0;
@@ -365,7 +435,7 @@ int cmd_replay(int argc,
 
 	const char *const replay_usage[] = {
 		N_("(EXPERIMENTAL!) git replay "
-		   "([--contained] --onto <newbase> | --advance <branch>) "
+		   "([--contained] --onto <newbase> | --advance <branch> | --revert <branch>) "
 		   "[--ref-action[=<mode>]] <revision-range>..."),
 		NULL
 	};
@@ -378,6 +448,9 @@ int cmd_replay(int argc,
 			   N_("replay onto given commit")),
 		OPT_BOOL(0, "contained", &contained,
 			 N_("advance all branches contained in revision-range")),
+		OPT_STRING(0, "revert", &revert_name_opt,
+			   N_("branch"),
+			   N_("revert commits onto given branch")),
 		OPT_STRING(0, "ref-action", &ref_action,
 			   N_("mode"),
 			   N_("control ref update behavior (update|print)")),
@@ -387,18 +460,28 @@ int cmd_replay(int argc,
 	argc = parse_options(argc, argv, prefix, replay_options, replay_usage,
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);
 
-	if (!onto_name && !advance_name_opt) {
-		error(_("option --onto or --advance is mandatory"));
+	/* Exactly one mode must be specified */
+	if (!onto_name && !advance_name_opt && !revert_name_opt) {
+		error(_("exactly one of --onto, --advance, or --revert is required"));
 		usage_with_options(replay_usage, replay_options);
 	}
 
 	die_for_incompatible_opt2(!!advance_name_opt, "--advance",
-				  contained, "--contained");
+				  !!onto_name, "--onto");
+	die_for_incompatible_opt2(!!revert_name_opt, "--revert",
+				  !!onto_name, "--onto");
+	die_for_incompatible_opt2(!!revert_name_opt, "--revert",
+				  !!advance_name_opt, "--advance");
+	die_for_incompatible_opt2(contained, "--contained",
+				  !onto_name, "requires --onto");
 
 	/* Parse ref action mode from command line or config */
 	ref_mode = get_ref_action_mode(repo, ref_action);
 
 	advance_name = xstrdup_or_null(advance_name_opt);
+	revert_name = xstrdup_or_null(revert_name_opt);
+	if (revert_name)
+		action = REPLAY_REVERT;
 
 	repo_init_revisions(repo, &revs, prefix);
 
@@ -452,10 +535,13 @@ int cmd_replay(int argc,
 	}
 
 	determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name,
+			      &revert_name,
 			      &onto, &update_refs);
 
 	/* Build reflog message */
-	if (advance_name_opt)
+	if (revert_name_opt)
+		strbuf_addf(&reflog_msg, "replay --revert %s", revert_name_opt);
+	else if (advance_name_opt)
 		strbuf_addf(&reflog_msg, "replay --advance %s", advance_name_opt);
 	else
 		strbuf_addf(&reflog_msg, "replay --onto %s",
@@ -496,7 +582,7 @@ int cmd_replay(int argc,
 			die(_("replaying merge commits is not supported yet!"));
 
 		last_commit = pick_regular_commit(repo, commit, replayed_commits,
-						  onto, &merge_opt, &result);
+						  onto, &merge_opt, &result, action);
 		if (!last_commit)
 			break;
 
@@ -508,7 +594,7 @@ int cmd_replay(int argc,
 		kh_value(replayed_commits, pos) = last_commit;
 
 		/* Update any necessary branches */
-		if (advance_name)
+		if (advance_name || revert_name)
 			continue;
 		decoration = get_name_decoration(&commit->object);
 		if (!decoration)
@@ -532,7 +618,7 @@ int cmd_replay(int argc,
 		}
 	}
 
-	/* In --advance mode, advance the target ref */
+	/* In --advance or --revert mode, update the target ref */
 	if (result.clean == 1 && advance_name) {
 		if (handle_ref_update(ref_mode, transaction, advance_name,
 				      &last_commit->object.oid,
@@ -544,6 +630,17 @@ int cmd_replay(int argc,
 			goto cleanup;
 		}
 	}
+	if (result.clean == 1 && revert_name) {
+		if (handle_ref_update(ref_mode, transaction, revert_name,
+				      &last_commit->object.oid,
+				      &onto->object.oid,
+				      reflog_msg.buf,
+				      &transaction_err) < 0) {
+			ret = error(_("failed to update ref '%s': %s"),
+				    revert_name, transaction_err.buf);
+			goto cleanup;
+		}
+	}
 
 	/* Commit the ref transaction if we have one */
 	if (transaction && result.clean == 1) {
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
+'
+
+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_expect_success 'git replay --revert with --ref-action=print' '
+	# 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' '
+	# 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' '
+	# 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
+'
+
 test_done
-- 
2.51.0

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

[Patrick Steinhardt]

On Wed, Dec 03, 2025 at 01:46:11AM +0530, Siddharth Asthana wrote:
> diff --git a/builtin/replay.c b/builtin/replay.c
> index 6606a2c94b..7660f7412f 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -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));
> +	} else {
> +		find_commit_subject(message, &orig_message);
> +		strbuf_addstr(&msg, orig_message);
> +		author = get_author(message);
> +	}
>  	reset_ident_date();
>  	if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
>  				 &ret, author, NULL, sign_commit, extra)) {

Do we want to be defensive in those if-chains and verify that `action ==
REPLAY_PICK` in the other case, and `BUG()` if it's not?

> @@ -273,21 +322,39 @@ static struct commit *pick_regular_commit(struct repository *repo,
>  	pickme_tree = repo_get_commit_tree(repo, pickme);
>  	base_tree = repo_get_commit_tree(repo, base);
>  
> -	merge_opt->branch1 = short_commit_name(repo, replayed_base);
> -	merge_opt->branch2 = short_commit_name(repo, pickme);
> -	merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
> +	if (action == REPLAY_PICK) {
> +		/* Cherry-pick: normal order */
> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
> +		merge_opt->branch2 = short_commit_name(repo, pickme);
> +		merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
>  
> -	merge_incore_nonrecursive(merge_opt,
> -				  base_tree,
> -				  result->tree,
> -				  pickme_tree,
> -				  result);
> +		merge_incore_nonrecursive(merge_opt,
> +					  base_tree,
> +					  result->tree,
> +					  pickme_tree,
> +					  result);
>  
> -	free((char*)merge_opt->ancestor);
> +		free((char *)merge_opt->ancestor);
> +	} else {
> +		/* Revert: swap base and pickme to reverse the diff */
> +		const char *pickme_name = short_commit_name(repo, pickme);
> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
> +		merge_opt->branch2 = xstrfmt("parent of %s", pickme_name);
> +		merge_opt->ancestor = pickme_name;
> +
> +		merge_incore_nonrecursive(merge_opt,
> +					  pickme_tree,
> +					  result->tree,
> +					  base_tree,
> +					  result);
> +
> +		free((char *)merge_opt->branch2);
> +	}
>  	merge_opt->ancestor = NULL;
> +	merge_opt->branch2 = NULL;

We can `FREE_AND_NULL()` instead of manually unsetting these.

> @@ -387,18 +460,28 @@ int cmd_replay(int argc,
>  	argc = parse_options(argc, argv, prefix, replay_options, replay_usage,
>  			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);
>  
> -	if (!onto_name && !advance_name_opt) {
> -		error(_("option --onto or --advance is mandatory"));
> +	/* Exactly one mode must be specified */
> +	if (!onto_name && !advance_name_opt && !revert_name_opt) {
> +		error(_("exactly one of --onto, --advance, or --revert is required"));
>  		usage_with_options(replay_usage, replay_options);
>  	}
>  
>  	die_for_incompatible_opt2(!!advance_name_opt, "--advance",
> -				  contained, "--contained");
> +				  !!onto_name, "--onto");
> +	die_for_incompatible_opt2(!!revert_name_opt, "--revert",
> +				  !!onto_name, "--onto");
> +	die_for_incompatible_opt2(!!revert_name_opt, "--revert",
> +				  !!advance_name_opt, "--advance");
> +	die_for_incompatible_opt2(contained, "--contained",
> +				  !onto_name, "requires --onto");

We have `die_for_incompatible_opt3()` that can be used here to check for
mutual exclusivity of "--revert", "--advance" and "--onto".

> @@ -508,7 +594,7 @@ int cmd_replay(int argc,
>  		kh_value(replayed_commits, pos) = last_commit;
>  
>  		/* Update any necessary branches */
> -		if (advance_name)
> +		if (advance_name || revert_name)
>  			continue;
>  		decoration = get_name_decoration(&commit->object);
>  		if (!decoration)
> @@ -532,7 +618,7 @@ int cmd_replay(int argc,
>  		}
>  	}
>  
> -	/* In --advance mode, advance the target ref */
> +	/* In --advance or --revert mode, update the target ref */
>  	if (result.clean == 1 && advance_name) {
>  		if (handle_ref_update(ref_mode, transaction, advance_name,
>  				      &last_commit->object.oid,
> @@ -544,6 +630,17 @@ int cmd_replay(int argc,
>  			goto cleanup;
>  		}
>  	}
> +	if (result.clean == 1 && revert_name) {
> +		if (handle_ref_update(ref_mode, transaction, revert_name,
> +				      &last_commit->object.oid,
> +				      &onto->object.oid,
> +				      reflog_msg.buf,
> +				      &transaction_err) < 0) {
> +			ret = error(_("failed to update ref '%s': %s"),
> +				    revert_name, transaction_err.buf);
> +			goto cleanup;
> +		}
> +	}

This conditional and the one beforehand are the exact same, except that
we use either `revert_name` or `advance_name`. Let's merge them:

    if (result.clean == 1 && (revert_name || advance_name)) {
        const char *ref = revert_name ? revert_name : advance_name;
        if (handle_ref_update(ref_mode, transaction, ref,
                      &last_commit->object.oid,
                      &onto->object.oid,
                      reflog_msg.buf,
                      &transaction_err) < 0) {
            ret = error(_("failed to update ref '%s': %s"),
                    revert_name, transaction_err.buf);
            goto cleanup;
        }
    }

Patrick

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

[Siddharth Asthana]

On 05/12/25 17:03, Patrick Steinhardt wrote:
> On Wed, Dec 03, 2025 at 01:46:11AM +0530, Siddharth Asthana wrote:
>> diff --git a/builtin/replay.c b/builtin/replay.c
>> index 6606a2c94b..7660f7412f 100644
>> --- a/builtin/replay.c
>> +++ b/builtin/replay.c
>> @@ -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));
>> +	} else {
>> +		find_commit_subject(message, &orig_message);
>> +		strbuf_addstr(&msg, orig_message);
>> +		author = get_author(message);
>> +	}
>>   	reset_ident_date();
>>   	if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
>>   				 &ret, author, NULL, sign_commit, extra)) {
> Do we want to be defensive in those if-chains and verify that `action ==
> REPLAY_PICK` in the other case, and `BUG()` if it's not?


Good idea. I will add BUG() for unexpected action values to catch any 
future additions that aren't properly handled.


>
>> @@ -273,21 +322,39 @@ static struct commit *pick_regular_commit(struct repository *repo,
>>   	pickme_tree = repo_get_commit_tree(repo, pickme);
>>   	base_tree = repo_get_commit_tree(repo, base);
>>   
>> -	merge_opt->branch1 = short_commit_name(repo, replayed_base);
>> -	merge_opt->branch2 = short_commit_name(repo, pickme);
>> -	merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
>> +	if (action == REPLAY_PICK) {
>> +		/* Cherry-pick: normal order */
>> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
>> +		merge_opt->branch2 = short_commit_name(repo, pickme);
>> +		merge_opt->ancestor = xstrfmt("parent of %s", merge_opt->branch2);
>>   
>> -	merge_incore_nonrecursive(merge_opt,
>> -				  base_tree,
>> -				  result->tree,
>> -				  pickme_tree,
>> -				  result);
>> +		merge_incore_nonrecursive(merge_opt,
>> +					  base_tree,
>> +					  result->tree,
>> +					  pickme_tree,
>> +					  result);
>>   
>> -	free((char*)merge_opt->ancestor);
>> +		free((char *)merge_opt->ancestor);
>> +	} else {
>> +		/* Revert: swap base and pickme to reverse the diff */
>> +		const char *pickme_name = short_commit_name(repo, pickme);
>> +		merge_opt->branch1 = short_commit_name(repo, replayed_base);
>> +		merge_opt->branch2 = xstrfmt("parent of %s", pickme_name);
>> +		merge_opt->ancestor = pickme_name;
>> +
>> +		merge_incore_nonrecursive(merge_opt,
>> +					  pickme_tree,
>> +					  result->tree,
>> +					  base_tree,
>> +					  result);
>> +
>> +		free((char *)merge_opt->branch2);
>> +	}
>>   	merge_opt->ancestor = NULL;
>> +	merge_opt->branch2 = NULL;
> We can `FREE_AND_NULL()` instead of manually unsetting these.


Will use FREE_AND_NULL() - cleaner and clearer intent.


>
>> @@ -387,18 +460,28 @@ int cmd_replay(int argc,
>>   	argc = parse_options(argc, argv, prefix, replay_options, replay_usage,
>>   			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);
>>   
>> -	if (!onto_name && !advance_name_opt) {
>> -		error(_("option --onto or --advance is mandatory"));
>> +	/* Exactly one mode must be specified */
>> +	if (!onto_name && !advance_name_opt && !revert_name_opt) {
>> +		error(_("exactly one of --onto, --advance, or --revert is required"));
>>   		usage_with_options(replay_usage, replay_options);
>>   	}
>>   
>>   	die_for_incompatible_opt2(!!advance_name_opt, "--advance",
>> -				  contained, "--contained");
>> +				  !!onto_name, "--onto");
>> +	die_for_incompatible_opt2(!!revert_name_opt, "--revert",
>> +				  !!onto_name, "--onto");
>> +	die_for_incompatible_opt2(!!revert_name_opt, "--revert",
>> +				  !!advance_name_opt, "--advance");
>> +	die_for_incompatible_opt2(contained, "--contained",
>> +				  !onto_name, "requires --onto");
> We have `die_for_incompatible_opt3()` that can be used here to check for
> mutual exclusivity of "--revert", "--advance" and "--onto".


Nice, I wasn't aware of this helper. Will use 
die_for_incompatible_opt3() for the three-way mutual exclusivity check.


>
>> @@ -508,7 +594,7 @@ int cmd_replay(int argc,
>>   		kh_value(replayed_commits, pos) = last_commit;
>>   
>>   		/* Update any necessary branches */
>> -		if (advance_name)
>> +		if (advance_name || revert_name)
>>   			continue;
>>   		decoration = get_name_decoration(&commit->object);
>>   		if (!decoration)
>> @@ -532,7 +618,7 @@ int cmd_replay(int argc,
>>   		}
>>   	}
>>   
>> -	/* In --advance mode, advance the target ref */
>> +	/* In --advance or --revert mode, update the target ref */
>>   	if (result.clean == 1 && advance_name) {
>>   		if (handle_ref_update(ref_mode, transaction, advance_name,
>>   				      &last_commit->object.oid,
>> @@ -544,6 +630,17 @@ int cmd_replay(int argc,
>>   			goto cleanup;
>>   		}
>>   	}
>> +	if (result.clean == 1 && revert_name) {
>> +		if (handle_ref_update(ref_mode, transaction, revert_name,
>> +				      &last_commit->object.oid,
>> +				      &onto->object.oid,
>> +				      reflog_msg.buf,
>> +				      &transaction_err) < 0) {
>> +			ret = error(_("failed to update ref '%s': %s"),
>> +				    revert_name, transaction_err.buf);
>> +			goto cleanup;
>> +		}
>> +	}
> This conditional and the one beforehand are the exact same, except that
> we use either `revert_name` or `advance_name`. Let's merge them:
>
>      if (result.clean == 1 && (revert_name || advance_name)) {
>          const char *ref = revert_name ? revert_name : advance_name;
>          if (handle_ref_update(ref_mode, transaction, ref,
>                        &last_commit->object.oid,
>                        &onto->object.oid,
>                        reflog_msg.buf,
>                        &transaction_err) < 0) {
>              ret = error(_("failed to update ref '%s': %s"),
>                      revert_name, transaction_err.buf);
>              goto cleanup;
>          }
>      }


Agreed, this is much cleaner. Will merge the conditionals as you 
suggest. Will incorporate all changes in v3.

Thanks for the thorough review!

Siddharth


>
> Patrick

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

[Phillip Wood]

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