From: Siddharth Asthana Date: Sun, 07 Dec 2025 23:03:02 GMT Subject: Re: [PATCH v2 2/2] replay: add --revert mode to reverse commit changes Message-ID: <3f3dd1f1-3127-45a2-9afc-7c452d0418d9@gmail.com> In-Reply-To: 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