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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 25, 2025, 19:22 UTC
Message-ID: <xmqqwm3drk6m.fsf@gitster.g>
In-Reply-To: <20251125170056.34489-2-siddharthasthana31@gmail.com>

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

Show 8 quoted lines

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

Show 11 quoted lines

> +--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 ..."?

Show 14 quoted lines

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

Show 9 quoted lines

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

Show 7 quoted lines

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

Show 8 quoted lines

> @@ -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);

Show 10 quoted lines

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

Show 17 quoted lines

> +
> +		/* 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.

Show 5 quoted lines

> +	}
>  
> -	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?

Show 5 quoted lines

>  	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);
>  }

Show 5 quoted lines

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

Show 31 quoted lines

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