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

> 
> Changes in v4:
> - Replaced sequencer_format_revert_header() with a more complete
>    sequencer_format_revert_message() that handles everything: subject
>    prefix, commit reference via refer_to_commit(), and merge-parent
>    references -- per Phillip
> - Updated refer_to_commit() signature to take (struct repository *r,
>    bool use_commit_reference) instead of (struct replay_opts *opts)
> - Reverts are now newest-first (revs.reverse = 0 for --revert),
>    chaining on last_commit rather than the parent mapping
> - Changed doc example to cross-branch scenario and restored the
>    merge-tree NOTE
> - Updated error message format to "'--revert' cannot be used with
>    multiple revision ranges..." (and same for --advance)
> - Empty revert commits are now dropped, consistent with cherry-pick

> - Link to v3: https://public-inbox.org/git/20260218234215.89326-1-siddharthasthana31@gmail.com/
> - Link to v2: https://public-inbox.org/git/20251202201611.22137-1-siddharthasthana31@gmail.com/
> - Link to v1: https://public-inbox.org/git/20251125170056.34489-1-siddharthasthana31@gmail.com/
> 
> Thanks,
> Siddharth
> 
> ---
> Siddharth Asthana (2):
>    sequencer: extract revert message formatting into shared function
>    replay: add --revert mode to reverse commit changes
> 
>   Documentation/git-replay.adoc |  43 ++++++++-
>   builtin/replay.c              |  46 ++++++----
>   replay.c                      | 165 ++++++++++++++++++++++++----------
>   replay.h                      |  11 ++-
>   sequencer.c                   |  78 +++++++++-------
>   sequencer.h                   |  14 +++
>   t/t3650-replay-basics.sh      | 114 +++++++++++++++++++++--
>   7 files changed, 364 insertions(+), 107 deletions(-)
> 
> Range-diff versus v3:
> 
> 1:  9d686bcdfe ! 1:  bdc710b265 sequencer: extract revert message formatting into shared function
>      @@ Commit message
>           sequencer: extract revert message formatting into shared function
>       
>           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.
>      +    "Reapply" cases, appending "This reverts commit <ref>.", and handling
>      +    merge-parent references) currently lives inline in do_pick_commit().
>      +    The upcoming replay --revert mode needs to reuse this logic.
>       
>      -    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>"").
>      -    When an oid is provided, the function appends the full commit hash and
>      -    period; otherwise the caller should append the commit reference.
>      +    Extract all of this into a new sequencer_format_revert_message()
>      +    function. The function takes a repository, the subject line, commit,
>      +    parent, a use_commit_reference flag, and the output strbuf. It handles
>      +    both regular reverts ("Revert "<subject>"") and revert-of-revert cases
>      +    ("Reapply "<subject>""), and uses refer_to_commit() internally to
>      +    format the commit reference.
>       
>      -    Update do_pick_commit() to use the new helper, eliminating code
>      -    duplication while preserving the special handling for commit_use_reference.
>      +    Update refer_to_commit() to take a struct repository parameter instead
>      +    of relying on the_repository, and a bool instead of reading from
>      +    replay_opts directly. This makes it usable from the new shared function
>      +    without pulling in sequencer-specific state.
>       
>           Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
>       
>        ## sequencer.c ##
>      +@@ sequencer.c: static int should_edit(struct replay_opts *opts) {
>      + 	return opts->edit;
>      + }
>      +
>      +-static void refer_to_commit(struct replay_opts *opts,
>      +-			    struct strbuf *msgbuf, struct commit *commit)
>      ++static void refer_to_commit(struct repository *r, struct strbuf *msgbuf,
>      ++			    const struct commit *commit,
>      ++			    bool use_commit_reference)
>      + {
>      +-	if (opts->commit_use_reference) {
>      ++	if (use_commit_reference) {
>      + 		struct pretty_print_context ctx = {
>      + 			.abbrev = DEFAULT_ABBREV,
>      + 			.date_mode.type = DATE_SHORT,
>      + 		};
>      +-		repo_format_commit_message(the_repository, commit,
>      ++		repo_format_commit_message(r, commit,
>      + 					   "%h (%s, %ad)", msgbuf, &ctx);
>      + 	} else {
>      + 		strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));
>       @@ sequencer.c: static int do_pick_commit(struct repository *r,
>        	 */
>        
>      @@ sequencer.c: static int do_pick_commit(struct repository *r,
>        		base = commit;
>        		base_label = msg.label;
>        		next = parent;
>      -@@ 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 ***");
>      + 		next_label = msg.parent_label;
>      +-		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
>      @@ sequencer.c: static int do_pick_commit(struct repository *r,
>       -			strbuf_addstr(&ctx->message, "Reapply \"");
>       -			strbuf_addstr(&ctx->message, orig_subject);
>       -			strbuf_addstr(&ctx->message, "\n");
>      -+			strbuf_addstr(&ctx->message, "\nThis reverts commit ");
>      - 		} else {
>      +-		} 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, NULL);
>      - 		}
>      +-		}
>       -		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
>      - 		refer_to_commit(opts, &ctx->message, commit);
>      +-		refer_to_commit(opts, &ctx->message, commit);
>      +-
>      +-		if (commit->parents && commit->parents->next) {
>      +-			strbuf_addstr(&ctx->message, ", reversing\nchanges made to ");
>      +-			refer_to_commit(opts, &ctx->message, parent);
>      +-		}
>      +-		strbuf_addstr(&ctx->message, ".\n");
>      ++		sequencer_format_revert_message(r, msg.subject, commit,
>      ++						parent,
>      ++						opts->commit_use_reference,
>      ++						&ctx->message);
>      + 	} else {
>      + 		const char *p;
>        
>      - 		if (commit->parents && commit->parents->next) {
>       @@ sequencer.c: int sequencer_pick_revisions(struct repository *r,
>        	return res;
>        }
>        
>      -+void sequencer_format_revert_header(struct strbuf *out,
>      -+				    const char *orig_subject,
>      -+				    const struct object_id *oid)
>      ++void sequencer_format_revert_message(struct repository *r,
>      ++				     const char *subject,
>      ++				     const struct commit *commit,
>      ++				     const struct commit *parent,
>      ++				     bool use_commit_reference,
>      ++				     struct strbuf *message)
>       +{
>      -+	const char *revert_subject;
>      ++	const char *orig_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');
>      ++	if (use_commit_reference) {
>      ++		strbuf_commented_addf(message, comment_line_str,
>      ++				      "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
>      ++	} else if (skip_prefix(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(message, "Reapply \"");
>      ++		strbuf_addstr(message, orig_subject);
>      ++		strbuf_addstr(message, "\n");
>       +	} else {
>      -+		strbuf_addstr(out, "Revert \"");
>      -+		strbuf_addstr(out, orig_subject);
>      -+		strbuf_addstr(out, "\"\n");
>      ++		strbuf_addstr(message, "Revert \"");
>      ++		strbuf_addstr(message, subject);
>      ++		strbuf_addstr(message, "\"\n");
>       +	}
>      ++	strbuf_addstr(message, "\nThis reverts commit ");
>      ++	refer_to_commit(r, message, commit, use_commit_reference);
>       +
>      -+	strbuf_addstr(out, "\nThis reverts commit ");
>      -+	if (oid) {
>      -+		strbuf_addstr(out, oid_to_hex(oid));
>      -+		strbuf_addstr(out, ".\n");
>      ++	if (commit->parents && commit->parents->next) {
>      ++		strbuf_addstr(message, ", reversing\nchanges made to ");
>      ++		refer_to_commit(r, message, parent, use_commit_reference);
>       +	}
>      ++	strbuf_addstr(message, ".\n");
>       +}
>       +
>        void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)
>      @@ sequencer.h: int sequencer_determine_whence(struct repository *r, enum commit_wh
>        int sequencer_get_update_refs_state(const char *wt_dir, struct string_list *refs);
>        
>       +/*
>      -+ * Formats a revert commit message following standard Git conventions.
>      -+ * Handles both regular reverts ("Revert \"<subject>\"") and revert of revert
>      -+ * cases ("Reapply \"<subject>\""). Adds "This reverts commit <oid>." if oid
>      -+ * is provided, otherwise just adds "This reverts commit " and the caller
>      -+ * should append the commit reference.
>      ++ * Formats a complete revert commit message following standard Git conventions.
>      ++ * Handles regular reverts ("Revert \"<subject>\""), revert of revert cases
>      ++ * ("Reapply \"<subject>\""), and the --reference style. Appends "This reverts
>      ++ * commit <ref>." using either the abbreviated or full commit reference
>      ++ * depending on use_commit_reference. Also handles merge-parent references.
>       + */
>      -+void sequencer_format_revert_header(struct strbuf *out,
>      -+				    const char *orig_subject,
>      -+				    const struct object_id *oid);
>      ++void sequencer_format_revert_message(struct repository *r,
>      ++				     const char *subject,
>      ++				     const struct commit *commit,
>      ++				     const struct commit *parent,
>      ++				     bool use_commit_reference,
>      ++				     struct strbuf *message);
>       +
>        #endif /* SEQUENCER_H */
> 2:  066269706e ! 2:  bea6229575 replay: add --revert mode to reverse commit changes
>      @@ Commit message
>           We swap the base and pickme trees passed to merge_incore_nonrecursive()
>           to reverse the diff direction.
>       
>      +    Reverts are processed newest-first (matching git revert behavior) to
>      +    reduce conflicts by peeling off changes from the top. Each revert
>      +    builds on the result of the previous one via the last_commit fallback
>      +    in the main replay loop, rather than relying on the parent-mapping
>      +    used for cherry-pick.
>      +
>           Revert commit messages follow the usual git revert conventions: prefixed
>           with "Revert" (or "Reapply" when reverting a revert), and including
>           "This reverts commit <hash>.". The author is set to the current user
>      @@ Commit message
>           Helped-by: Phillip Wood <phillip.wood123@gmail.com>
>           Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>           Helped-by: Junio C Hamano <gitster@pobox.com>
>      +    Helped-by: Toon Claes <toon@iotcl.com>
>           Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
>       
>        ## Documentation/git-replay.adoc ##
>      @@ Documentation/git-replay.adoc: all commits they have since `base`, playing them
>       +To revert commits on a branch:
>       +
>       +------------
>      -+$ git replay --revert main main~2..main
>      ++$ git replay --revert main topic~2..topic
>       +------------
>       +
>      -+This reverts the last two commits on `main`, creating two revert commits
>      -+on top of `main`, and updates `main` to point at the result.
>      ++This reverts the last two commits from `topic`, creating revert commits on
>      ++top of `main`, and updates `main` to point at the result. This is useful when
>      ++commits from `topic` 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
>        ---
>      @@ builtin/replay.c: int cmd_replay(int argc,
>        
>        	/* Parse ref action mode from command line or config */
>        	ref_mode = get_ref_action_mode(repo, ref_action);
>      +@@ builtin/replay.c: int cmd_replay(int argc,
>      + 	 * some options changing these values if we think they could
>      + 	 * be useful.
>      + 	 */
>      +-	revs.reverse = 1;
>      ++	/*
>      ++	 * Cherry-pick/rebase need oldest-first ordering so that each
>      ++	 * replayed commit can build on its already-replayed parent.
>      ++	 * Revert needs newest-first ordering (like git revert) to
>      ++	 * reduce conflicts by peeling off changes from the top.
>      ++	 */
>      ++	revs.reverse = opts.revert ? 0 : 1;
>      + 	revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
>      + 	revs.topo_order = 1;
>      + 	revs.simplify_history = 0;
>      +@@ builtin/replay.c: int cmd_replay(int argc,
>      + 	 * Detect and warn if we override some user specified rev
>      + 	 * walking options.
>      + 	 */
>      +-	if (revs.reverse != 1) {
>      +-		warning(_("some rev walking options will be overridden as "
>      +-			  "'%s' bit in 'struct rev_info' will be forced"),
>      +-			"reverse");
>      +-		revs.reverse = 1;
>      ++	{
>      ++		int desired_reverse = opts.revert ? 0 : 1;
>      ++		if (revs.reverse != desired_reverse) {
>      ++			warning(_("some rev walking options will be overridden as "
>      ++				  "'%s' bit in 'struct rev_info' will be forced"),
>      ++				"reverse");
>      ++			revs.reverse = desired_reverse;
>      ++		}
>      + 	}
>      + 	if (revs.sort_order != REV_SORT_IN_GRAPH_ORDER) {
>      + 		warning(_("some rev walking options will be overridden as "
>       @@ builtin/replay.c: int cmd_replay(int argc,
>        		goto cleanup;
>        
>      @@ replay.c
>        #include "strmap.h"
>        #include "tree.h"
>        
>      +-/*
>      +- * We technically need USE_THE_REPOSITORY_VARIABLE for DEFAULT_ABBREV, but
>      +- * do not want to use the_repository.
>      +- */
>      +-#define the_repository DO_NOT_USE_THE_REPOSITORY
>       +enum replay_mode {
>       +	REPLAY_MODE_PICK,
>       +	REPLAY_MODE_REVERT,
>       +};
>      -+
>      +
>        static const char *short_commit_name(struct repository *repo,
>        				     struct commit *commit)
>      - {
>       @@ replay.c: static char *get_author(const char *message)
>        	return NULL;
>        }
>      @@ replay.c: static char *get_author(const char *message)
>       +	subject_len = find_commit_subject(message, &subject_start);
>       +	subject = xmemdupz(subject_start, subject_len);
>       +
>      -+	sequencer_format_revert_header(msg, subject, &commit->object.oid);
>      ++	sequencer_format_revert_message(repo, subject, commit,
>      ++					commit->parents ? commit->parents->item : NULL,
>      ++					false, msg);
>       +
>       +	free(subject);
>       +	repo_unuse_commit_buffer(repo, commit, message);
>      @@ replay.c: static void get_ref_information(struct repository *repo,
>       +	}
>       +	*onto = peel_committish(repo, *branch_name, option_name);
>       +	if (rinfo->positive_refexprs > 1)
>      -+		die(_("cannot %s target with multiple sources because ordering would be ill-defined"),
>      -+		    option_name + 2); /* skip "--" prefix */
>      ++		die(_("'%s' cannot be used with multiple revision ranges "
>      ++		      "because the ordering would be ill-defined"),
>      ++		    option_name);
>       +}
>       +
>        static void set_up_replay_mode(struct repository *repo,
>      @@ replay.c: static struct commit *pick_regular_commit(struct repository *repo,
>       +	merge_opt->branch2 = NULL;
>        	if (!result->clean)
>        		return NULL;
>      --	/* Drop commits that become empty */
>      --	if (oideq(&replayed_base_tree->object.oid, &result->tree->object.oid) &&
>      -+	/* Drop commits that become empty (only for picks) */
>      -+	if (mode == REPLAY_MODE_PICK &&
>      -+	    oideq(&replayed_base_tree->object.oid, &result->tree->object.oid) &&
>      + 	/* Drop commits that become empty */
>      + 	if (oideq(&replayed_base_tree->object.oid, &result->tree->object.oid) &&
>        	    !oideq(&pickme_tree->object.oid, &base_tree->object.oid))
>        		return replayed_base;
>       -	return create_commit(repo, result->tree, pickme, replayed_base);
>      @@ replay.c: int replay_revisions(struct rev_info *revs,
>        
>        		last_commit = pick_regular_commit(revs->repo, commit, replayed_commits,
>       -						  onto, &merge_opt, &result);
>      -+						  onto, &merge_opt, &result, mode);
>      ++						  mode == REPLAY_MODE_REVERT ? last_commit : onto,
>      ++						  &merge_opt, &result, mode);
>        		if (!last_commit)
>        			break;
>        
>      @@ t/t3650-replay-basics.sh: test_expect_success 'no base or negative ref gives no-
>        	test_must_fail git replay --advance=main --contained \
>        		topic1..topic2 2>actual &&
>        	test_cmp expect actual
>      + '
>      +
>      + test_expect_success 'cannot advance target ... ordering would be ill-defined' '
>      +-	echo "fatal: cannot advance target with multiple sources because ordering would be ill-defined" >expect &&
>      ++	cat >expect <<-\EOF &&
>      ++	fatal: '"'"'--advance'"'"' cannot be used with multiple revision ranges because the ordering would be ill-defined
>      ++	EOF
>      + 	test_must_fail git replay --advance=main main topic1 topic2 2>actual &&
>      + 	test_cmp expect actual
>      + '
>       @@ t/t3650-replay-basics.sh: test_expect_success 'invalid replay.refAction value' '
>        	test_grep "invalid.*replay.refAction.*value" error
>        '
>      @@ t/t3650-replay-basics.sh: test_expect_success 'invalid replay.refAction value' '
>       +'
>       +
>       +test_expect_success 'cannot revert with multiple sources' '
>      -+	echo "fatal: cannot revert target with multiple sources because ordering would be ill-defined" >expect &&
>      ++	cat >expect <<-\EOF &&
>      ++	fatal: '"'"'--revert'"'"' cannot be used with multiple revision ranges because the ordering would be ill-defined
>      ++	EOF
>       +	test_must_fail git replay --revert main main topic1 topic2 2>actual &&
>       +	test_cmp expect actual
>       +'
>      @@ t/t3650-replay-basics.sh: test_expect_success 'invalid replay.refAction value' '
>       +	# Revert commits I and J
>       +	git replay --revert topic4 topic4~2..topic4 &&
>       +
>      -+	# Verify the revert commits were created
>      ++	# Verify the revert commits were created (newest-first ordering
>      ++	# means J is reverted first, then I on top)
>       +	git log --format=%s -4 topic4 >actual &&
>       +	cat >expect <<-\EOF &&
>      -+	Revert "J"
>       +	Revert "I"
>      ++	Revert "J"
>       +	J
>       +	I
>       +	EOF
>       +	test_cmp expect actual &&
>       +
>      -+	# Verify commit message format includes hash
>      ++	# Verify commit message format includes hash (tip is Revert "I")
>       +	test_commit_message topic4 <<-EOF &&
>      -+	Revert "J"
>      ++	Revert "I"
>       +
>      -+	This reverts commit $(git rev-parse J).
>      ++	This reverts commit $(git rev-parse I).
>       +	EOF
>       +
>       +	# Verify reflog message
> 
> 
> base-commit: d181b9354cf85b44455ce3ca9e6af0b9559e0ae2
> 
>