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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Siddarth

On 03/11/2025 19:32, Siddharth Asthana wrote:
> 
> I looked at how `git rebase` constructs its reflog messages (it uses
> something like "rebase (finish): refs/heads/feature onto abc123def")
> and I'm thinking of using a simpler format for replay:
> 
>      "replay --onto main"
>      "replay --advance main"
> 
> This shows the mode and target in a way that mirrors what the user
> typed.

That makes sense

> I chose to use the symbolic name (e.g., "main") rather than
> the commit SHA because it seems more user-friendly, though I notice
> git rebase uses oid_to_hex().

One thing to note is that using oid_to_hex() tells us which commit we 
rebased on to. Using "main" means it is hard to find exactly which 
commit was used because you have to dig through the reflog for "main" to 
find where it was pointing at the time that "replay" was run.

> Regarding "include the commits that have been picked" for --advance
> mode - would you prefer:
>    - The revision range as specified by the user (e.g., "topic1..topic2")?
>    - Just the target branch like I have above?
> 
> The revision range would provide more context, but it might make the
> reflog message quite long if the user specified something complex. I'm
> happy to include it if that's what you think would be most useful.

The full revision range could certainly get quite long. "git 
cherry-pick" creates one reflog entry per picked commit which avoids 
that problem. I don't think we necessarily want "git replay" to create 
masses of reflog entries though so perhaps we should use the revision 
range if it is simple like "a..b" and something more general if it is 
more complex than that?

> I'll add tests for the reflog messages in both modes.

That's great

Thanks

Phillip

> Thanks,
> Siddharth
> 
> 
>>
>>
>>> +    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
>>