From: Phillip Wood Date: Mon, 03 Nov 2025 16:25:23 GMT Subject: Re: [PATCH v6 2/3] replay: make atomic ref updates the default behavior Message-ID: In-Reply-To: <20251030191931.30837-3-siddharthasthana31@gmail.com> Hi Siddharth On 30/10/2025 19:19, Siddharth Asthana wrote: > + case REF_ACTION_UPDATE: > + return ref_transaction_update(transaction, refname, new_oid, old_oid, > + NULL, NULL, 0, "git replay", err); I wonder if we should use a more descriptive reflog message here that says what git replay was doing. For example "git replay --onto " could include the new base in the reflog message like "git rebase" does. For "git replay --advance" we could include the commits that have been picked. It would be helpful to test the reflog message in the new tests as well. Thanks Phillip > + default: > + BUG("unknown ref_action_mode %d", mode); > + } > +} > + > int cmd_replay(int argc, > const char **argv, > const char *prefix, > @@ -294,6 +321,8 @@ int cmd_replay(int argc, > struct commit *onto = NULL; > const char *onto_name = NULL; > int contained = 0; > + const char *ref_action_str = NULL; > + enum ref_action_mode ref_action = REF_ACTION_UPDATE; > > struct rev_info revs; > struct commit *last_commit = NULL; > @@ -302,12 +331,14 @@ int cmd_replay(int argc, > struct merge_result result; > struct strset *update_refs = NULL; > kh_oid_map_t *replayed_commits; > + struct ref_transaction *transaction = NULL; > + struct strbuf transaction_err = STRBUF_INIT; > int ret = 0; > > - const char * const replay_usage[] = { > + const char *const replay_usage[] = { > N_("(EXPERIMENTAL!) git replay " > "([--contained] --onto | --advance ) " > - "..."), > + "[--ref-action[=]] ..."), > NULL > }; > struct option replay_options[] = { > @@ -319,6 +350,9 @@ int cmd_replay(int argc, > N_("replay onto given commit")), > OPT_BOOL(0, "contained", &contained, > N_("advance all branches contained in revision-range")), > + OPT_STRING(0, "ref-action", &ref_action_str, > + N_("mode"), > + N_("control ref update behavior (update|print)")), > OPT_END() > }; > > @@ -333,6 +367,18 @@ int cmd_replay(int argc, > die_for_incompatible_opt2(!!advance_name_opt, "--advance", > contained, "--contained"); > > + /* Default to update mode if not specified */ > + if (!ref_action_str) > + ref_action_str = "update"; > + > + /* Parse ref action mode */ > + if (!strcmp(ref_action_str, "update")) > + ref_action = REF_ACTION_UPDATE; > + else if (!strcmp(ref_action_str, "print")) > + ref_action = REF_ACTION_PRINT; > + else > + die(_("unknown --ref-action mode '%s'"), ref_action_str); > + > advance_name = xstrdup_or_null(advance_name_opt); > > repo_init_revisions(repo, &revs, prefix); > @@ -389,6 +435,17 @@ int cmd_replay(int argc, > determine_replay_mode(repo, &revs.cmdline, onto_name, &advance_name, > &onto, &update_refs); > > + /* Initialize ref transaction if using update mode */ > + if (ref_action == REF_ACTION_UPDATE) { > + transaction = ref_store_transaction_begin(get_main_ref_store(repo), > + 0, &transaction_err); > + if (!transaction) { > + ret = error(_("failed to begin ref transaction: %s"), > + transaction_err.buf); > + goto cleanup; > + } > + } > + > if (!onto) /* FIXME: Should handle replaying down to root commit */ > die("Replaying down to root commit is not supported yet!"); > > @@ -434,10 +491,15 @@ int cmd_replay(int argc, > if (decoration->type == DECORATION_REF_LOCAL && > (contained || strset_contains(update_refs, > decoration->name))) { > - printf("update %s %s %s\n", > - decoration->name, > - oid_to_hex(&last_commit->object.oid), > - oid_to_hex(&commit->object.oid)); > + if (handle_ref_update(ref_action, transaction, > + decoration->name, > + &last_commit->object.oid, > + &commit->object.oid, > + &transaction_err) < 0) { > + ret = error(_("failed to update ref '%s': %s"), > + decoration->name, transaction_err.buf); > + goto cleanup; > + } > } > decoration = decoration->next; > } > @@ -445,10 +507,23 @@ int cmd_replay(int argc, > > /* In --advance mode, advance the target ref */ > if (result.clean == 1 && advance_name) { > - printf("update %s %s %s\n", > - advance_name, > - oid_to_hex(&last_commit->object.oid), > - oid_to_hex(&onto->object.oid)); > + if (handle_ref_update(ref_action, transaction, advance_name, > + &last_commit->object.oid, > + &onto->object.oid, > + &transaction_err) < 0) { > + ret = error(_("failed to update ref '%s': %s"), > + advance_name, transaction_err.buf); > + goto cleanup; > + } > + } > + > + /* Commit the ref transaction if we have one */ > + if (transaction && result.clean == 1) { > + if (ref_transaction_commit(transaction, &transaction_err)) { > + ret = error(_("failed to commit ref transaction: %s"), > + transaction_err.buf); > + goto cleanup; > + } > } > > merge_finalize(&merge_opt, &result); > @@ -460,6 +535,9 @@ int cmd_replay(int argc, > ret = result.clean; > > cleanup: > + if (transaction) > + ref_transaction_free(transaction); > + strbuf_release(&transaction_err); > release_revisions(&revs); > free(advance_name); > > diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh > index 58b3759935..123734b49f 100755 > --- a/t/t3650-replay-basics.sh > +++ b/t/t3650-replay-basics.sh > @@ -52,7 +52,7 @@ test_expect_success 'setup bare' ' > ' > > test_expect_success 'using replay to rebase two branches, one on top of other' ' > - git replay --onto main topic1..topic2 >result && > + git replay --ref-action=print --onto main topic1..topic2 >result && > > test_line_count = 1 result && > > @@ -68,7 +68,7 @@ test_expect_success 'using replay to rebase two branches, one on top of other' ' > ' > > test_expect_success 'using replay on bare repo to rebase two branches, one on top of other' ' > - git -C bare replay --onto main topic1..topic2 >result-bare && > + git -C bare replay --ref-action=print --onto main topic1..topic2 >result-bare && > test_cmp expect result-bare > ' > > @@ -86,7 +86,7 @@ test_expect_success 'using replay to perform basic cherry-pick' ' > # 2nd field of result is refs/heads/main vs. refs/heads/topic2 > # 4th field of result is hash for main instead of hash for topic2 > > - git replay --advance main topic1..topic2 >result && > + git replay --ref-action=print --advance main topic1..topic2 >result && > > test_line_count = 1 result && > > @@ -102,7 +102,7 @@ test_expect_success 'using replay to perform basic cherry-pick' ' > ' > > test_expect_success 'using replay on bare repo to perform basic cherry-pick' ' > - git -C bare replay --advance main topic1..topic2 >result-bare && > + git -C bare replay --ref-action=print --advance main topic1..topic2 >result-bare && > test_cmp expect result-bare > ' > > @@ -115,7 +115,7 @@ test_expect_success 'replay fails when both --advance and --onto are omitted' ' > ' > > test_expect_success 'using replay to also rebase a contained branch' ' > - git replay --contained --onto main main..topic3 >result && > + git replay --ref-action=print --contained --onto main main..topic3 >result && > > test_line_count = 2 result && > cut -f 3 -d " " result >new-branch-tips && > @@ -139,12 +139,12 @@ test_expect_success 'using replay to also rebase a contained branch' ' > ' > > test_expect_success 'using replay on bare repo to also rebase a contained branch' ' > - git -C bare replay --contained --onto main main..topic3 >result-bare && > + git -C bare replay --ref-action=print --contained --onto main main..topic3 >result-bare && > test_cmp expect result-bare > ' > > test_expect_success 'using replay to rebase multiple divergent branches' ' > - git replay --onto main ^topic1 topic2 topic4 >result && > + git replay --ref-action=print --onto main ^topic1 topic2 topic4 >result && > > test_line_count = 2 result && > cut -f 3 -d " " result >new-branch-tips && > @@ -168,7 +168,7 @@ test_expect_success 'using replay to rebase multiple divergent branches' ' > ' > > test_expect_success 'using replay on bare repo to rebase multiple divergent branches, including contained ones' ' > - git -C bare replay --contained --onto main ^main topic2 topic3 topic4 >result && > + git -C bare replay --ref-action=print --contained --onto main ^main topic2 topic3 topic4 >result && > > test_line_count = 4 result && > cut -f 3 -d " " result >new-branch-tips && > @@ -217,4 +217,32 @@ test_expect_success 'merge.directoryRenames=false' ' > --onto rename-onto rename-onto..rename-from > ' > > +test_expect_success 'default atomic behavior updates refs directly' ' > + # Store original state for cleanup > + test_when_finished "git branch -f topic2 topic1" && > + > + # Test default atomic behavior (no output, refs updated) > + git replay --onto main topic1..topic2 >output && > + test_must_be_empty output && > + > + # Verify ref was updated > + git log --format=%s topic2 >actual && > + test_write_lines E D M L B A >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'atomic behavior in bare repository' ' > + # Test atomic updates work in bare repo > + git -C bare replay --onto main topic1..topic2 >output && > + test_must_be_empty output && > + > + # Verify ref was updated in bare repo > + git -C bare log --format=%s topic2 >actual && > + test_write_lines E D M L B A >expect && > + test_cmp expect actual && > + > + # Reset for other tests > + git -C bare update-ref refs/heads/topic2 $(git -C bare rev-parse topic1) > +' > + > test_done