Re: [PATCH v6 2/2] replay: add --revert mode to reverse commit changes
- From
Tian Yuchen <a3205153416@gmail.com>
- Date
- Mar 28, 2026, 04:33 UTC
- 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:
Show 7 quoted lines
> 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);
...
Show 5 quoted lines
> 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,
...
Show 6 quoted lines
> + *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.
Show 7 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
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