From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 05 Dec 2025 11:33:38 GMT
Subject: Re: [PATCH v2 2/2] replay: add --revert mode to reverse commit changes
Message-ID: <aTLDEgGzVnvU45va@pks.im>
In-Reply-To: <20251202201611.22137-3-siddharthasthana31@gmail.com>

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