From: Tian Yuchen <a3205153416@gmail.com>
Date: Sat, 28 Mar 2026 04:33:49 GMT
Subject: Re: [PATCH v6 2/2] replay: add --revert mode to reverse commit changes
Message-ID: <05959eb8-4b8a-421e-bf5f-9e6f0b59a313@gmail.com>
In-Reply-To: <20260325202354.10628-3-siddharthasthana31@gmail.com>

Hi Siddharth,

The patch itself looks pretty good to me, but I have some reservations 
about its functionality:

On 3/26/26 04:23, Siddharth Asthana wrote:
>   static struct commit *create_commit(struct repository *repo,
>   				    struct tree *tree,
>   				    struct commit *based_on,
> -				    struct commit *parent)
> +				    struct commit *parent,
> +				    enum replay_mode mode)
>   {

...

>   	extra = read_commit_extra_headers(based_on, exclude_gpgsig);

...


>   	if (commit_tree_extended(msg.buf, msg.len, &tree->object.oid, parents,
>   				 &ret, author, NULL, sign_commit, extra)) {
> @@ -153,11 +188,35 @@ static void get_ref_information(struct repository *repo,
>   	}
>   }

It seems there isn’t a distinction made here between how 'cherry-pick' 
and 'revert' handle the `extra` header. But doesn’t the 'revert' 
operation actually create a *new* commit with the *current time* and 
*current author*? Is it appropriate to inherit the 'extra' header?




> +static void set_up_branch_mode(struct repository *repo,

...

> +	*onto = peel_committish(repo, *branch_name, option_name);
> +	if (rinfo->positive_refexprs > 1)
> +		die(_("'%s' cannot be used with multiple revision ranges "
> +		      "because the ordering would be ill-defined"),
> +		    option_name);
> +}

This is a fail-safe design intended to prevent users from entering 
commands like:

	git replay --revert main f1 f2

This operation is indeed undefined which should be intercepted. However, 
considering:
	
	git replay --revert main HEAD~5..HEAD~3 HEAD~1..HEAD

Is this operation also intercepted? I think the reason is that the 
condition 'rinfo->positive_refexprs > 1' is a bit too simplistic.




> +	if (repo_dwim_ref(repo, *branch_name, strlen(*branch_name),
> +			  &oid, &fullname, 0) == 1) {
> +		free(*branch_name);
> +		*branch_name = fullname;
> +	} else {
> +		die(_("argument to %s must be a reference"), option_name);
> +	}

I think it would be great if a low-level command supported something like:

	git replay --revert new-branch HEAD~3..HEAD

Even if it just saves the step of creating a new branch ;)




These are just my thoughts on the matter. Hope to spark discussion.

Regards, Yuchen