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

From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Mar 29, 2026, 16:17 UTC
Message-ID: <6427d088-e41c-47ff-ab6e-4d7679e85d3c@gmail.com>
In-Reply-To: <05959eb8-4b8a-421e-bf5f-9e6f0b59a313@gmail.com>

On 28/03/26 10:03, Tian Yuchen wrote:

Show 33 quoted lines

> 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?

Yeah, you are right that regular git revert doesn't carry over extra headers from the original commit. In practice this doesn't bite us today since extra headers are mostly mergetag, and replay already rejects merge commits. But it's still worth cleaning up. I will send a follow-up patch for it rather than rerolling the whole series.

Show 27 quoted lines

> 
> 
> 
> 
>> +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.

Yes -- positive_refexprs counts each position tip, so that gives 2 and the > 1 check catches it.

Show 15 quoted lines

> 
> 
> 
> 
>> +    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

Interesting idea. git replay is still experimental so the interface could evolve. Worth considering as a follow-up but I would keep it out of this series for now.

Thanks for the review!

Show 10 quoted lines

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