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

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