Re: [PATCH v6 2/3] replay: make atomic ref updates the default behavior

> 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 
> <new-base> <revs>" 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 <newbase> | --advance <branch>) "
>> -           "<revision-range>..."),
>> +           "[--ref-action[=<mode>]] <revision-range>..."),
>>           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
>