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

[Junio C Hamano <gitster@pobox.com>]

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.