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

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 28/03/26 10:03, Tian Yuchen wrote:
> 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.


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


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

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