From: Siddharth Asthana <siddharthasthana31@gmail•com>
To: Patrick Steinhardt <ps@pks•im>
Cc: git@vger•kernel.org, christian.couder@gmail•com,
newren@gmail•com, gitster@pobox•com, phillip.wood123@gmail•com,
phillip.wood@dunelm•org.uk, karthik.188@gmail•com,
johannes.schindelin@gmx•de, toon@iotcl•com
Subject: Re: [PATCH v2 2/2] replay: add --revert mode to reverse commit changes
Date: Mon, 8 Dec 2025 04:33:02 +0530 [thread overview]
Message-ID: <3f3dd1f1-3127-45a2-9afc-7c452d0418d9@gmail.com> (raw)
In-Reply-To: <aTLDEgGzVnvU45va@pks.im>
On 05/12/25 17:03, Patrick Steinhardt wrote:
> 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?
Good idea. I will add BUG() for unexpected action values to catch any
future additions that aren't properly handled.
>
>> @@ -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.
Will use FREE_AND_NULL() - cleaner and clearer intent.
>
>> @@ -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".
Nice, I wasn't aware of this helper. Will use
die_for_incompatible_opt3() for the three-way mutual exclusivity check.
>
>> @@ -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;
> }
> }
Agreed, this is much cleaner. Will merge the conditionals as you
suggest. Will incorporate all changes in v3.
Thanks for the thorough review!
Siddharth
>
> Patrick
next prev parent reply other threads:[~2025-12-07 23:03 UTC|newest]
Thread overview: 96+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-25 17:00 [PATCH 0/1] replay: add --revert option to reverse commit changes Siddharth Asthana
2025-11-25 17:00 ` [PATCH 1/1] " Siddharth Asthana
2025-11-25 19:22 ` Junio C Hamano
2025-11-25 19:30 ` Junio C Hamano
2025-11-25 19:39 ` Junio C Hamano
2025-11-25 20:06 ` Junio C Hamano
2025-11-26 19:31 ` Siddharth Asthana
2025-11-26 19:28 ` Siddharth Asthana
2025-11-26 19:26 ` Siddharth Asthana
2025-11-26 21:13 ` Junio C Hamano
2025-11-27 19:23 ` Siddharth Asthana
2025-11-26 11:10 ` Phillip Wood
2025-11-26 17:35 ` Elijah Newren
2025-11-26 18:41 ` Junio C Hamano
2025-11-26 21:17 ` Junio C Hamano
2025-11-26 23:06 ` Elijah Newren
2025-11-26 23:14 ` Junio C Hamano
2025-11-26 23:57 ` Elijah Newren
2025-11-26 19:50 ` Siddharth Asthana
2025-11-26 19:39 ` Siddharth Asthana
2025-11-27 16:21 ` Phillip Wood
2025-11-27 19:24 ` Siddharth Asthana
2025-11-25 17:25 ` [PATCH 0/1] " Johannes Schindelin
2025-11-25 18:02 ` Junio C Hamano
2025-11-26 19:18 ` Siddharth Asthana
2025-11-26 21:04 ` Junio C Hamano
2025-11-27 19:21 ` Siddharth Asthana
2025-11-27 20:17 ` Junio C Hamano
2025-11-28 8:07 ` Elijah Newren
2025-11-28 8:24 ` Siddharth Asthana
2025-11-28 16:35 ` Junio C Hamano
2025-11-28 17:07 ` Elijah Newren
2025-11-28 20:50 ` Junio C Hamano
2025-11-28 22:03 ` Elijah Newren
2025-11-29 5:59 ` Junio C Hamano
2025-12-02 20:16 ` [PATCH v2 0/2] replay: add --revert mode " Siddharth Asthana
2025-12-02 20:16 ` [PATCH v2 1/2] sequencer: extract revert message formatting into shared function Siddharth Asthana
2025-12-05 11:33 ` Patrick Steinhardt
2025-12-07 23:00 ` Siddharth Asthana
2025-12-08 7:07 ` Patrick Steinhardt
2026-02-11 13:03 ` Toon Claes
2026-02-11 13:40 ` Patrick Steinhardt
2026-02-11 15:23 ` Kristoffer Haugsbakk
2026-02-11 17:41 ` Junio C Hamano
2026-02-18 22:53 ` Siddharth Asthana
2025-12-02 20:16 ` [PATCH v2 2/2] replay: add --revert mode to reverse commit changes Siddharth Asthana
2025-12-05 11:33 ` Patrick Steinhardt
2025-12-07 23:03 ` Siddharth Asthana [this message]
2025-12-16 16:23 ` Phillip Wood
2026-02-18 23:42 ` [PATCH v3 0/2] " Siddharth Asthana
2026-02-18 23:42 ` [PATCH v3 1/2] sequencer: extract revert message formatting into shared function Siddharth Asthana
2026-02-20 17:01 ` Toon Claes
2026-02-25 21:53 ` Junio C Hamano
2026-03-06 4:55 ` Siddharth Asthana
2026-03-06 4:31 ` Siddharth Asthana
2026-02-26 14:27 ` Phillip Wood
2026-03-06 5:00 ` Siddharth Asthana
2026-02-18 23:42 ` [PATCH v3 2/2] replay: add --revert mode to reverse commit changes Siddharth Asthana
2026-02-20 17:35 ` Toon Claes
2026-02-20 20:23 ` Junio C Hamano
2026-02-23 9:13 ` Christian Couder
2026-02-23 11:23 ` Toon Claes
2026-03-06 5:05 ` Siddharth Asthana
2026-02-26 14:45 ` Phillip Wood
2026-03-06 5:28 ` Siddharth Asthana
2026-03-06 15:52 ` Phillip Wood
2026-03-06 16:20 ` Siddharth Asthana
2026-03-13 5:40 ` [PATCH v4 0/2] " Siddharth Asthana
2026-03-13 5:40 ` [PATCH v4 1/2] sequencer: extract revert message formatting into shared function Siddharth Asthana
2026-03-13 15:53 ` Junio C Hamano
2026-03-16 19:12 ` Toon Claes
2026-03-16 16:57 ` Phillip Wood
2026-03-13 5:40 ` [PATCH v4 2/2] replay: add --revert mode to reverse commit changes Siddharth Asthana
2026-03-16 16:57 ` Phillip Wood
2026-03-16 19:52 ` Toon Claes
2026-03-17 10:11 ` Phillip Wood
2026-03-16 16:59 ` [PATCH v4 0/2] " Phillip Wood
2026-03-16 19:53 ` Toon Claes
2026-03-24 22:03 ` [PATCH v5 " Siddharth Asthana
2026-03-24 22:04 ` [PATCH v5 1/2] sequencer: extract revert message formatting into shared function Siddharth Asthana
2026-03-24 22:04 ` [PATCH v5 2/2] replay: add --revert mode to reverse commit changes Siddharth Asthana
2026-03-25 6:29 ` Junio C Hamano
2026-03-25 15:10 ` Toon Claes
2026-03-25 15:38 ` Siddharth Asthana
2026-03-25 16:44 ` Phillip Wood
2026-03-25 15:36 ` Siddharth Asthana
2026-03-25 20:23 ` [PATCH v6 0/2] " Siddharth Asthana
2026-03-25 20:23 ` [PATCH v6 1/2] sequencer: extract revert message formatting into shared function Siddharth Asthana
2026-03-25 20:23 ` [PATCH v6 2/2] replay: add --revert mode to reverse commit changes Siddharth Asthana
2026-03-28 4:33 ` Tian Yuchen
2026-03-29 16:17 ` Siddharth Asthana
2026-03-30 17:23 ` Tian Yuchen
2026-03-31 8:08 ` Toon Claes
2026-03-31 8:11 ` Toon Claes
2026-03-25 20:23 ` [PATCH v6 1/2] sequencer: extract revert message formatting into shared function Siddharth Asthana
2026-03-25 20:23 ` [PATCH v6 2/2] replay: add --revert mode to reverse commit changes Siddharth Asthana
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3f3dd1f1-3127-45a2-9afc-7c452d0418d9@gmail.com \
--to=siddharthasthana31@gmail$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=johannes.schindelin@gmx$(echo .)de \
--cc=karthik.188@gmail$(echo .)com \
--cc=newren@gmail$(echo .)com \
--cc=phillip.wood123@gmail$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=ps@pks$(echo .)im \
--cc=toon@iotcl$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox