From: Siddharth Asthana Date: Wed, 08 Oct 2025 20:09:00 GMT Subject: Re: [PATCH v2 1/1] replay: make atomic ref updates the default behavior Message-ID: <1bfffc20-7e25-4633-a0b8-6660913a74dd@gmail.com> In-Reply-To: On 08/10/25 19:31, Phillip Wood wrote: > Hi Siddharth > > On 02/10/2025 23:20, Siddharth Asthana wrote: >> On 30/09/25 15:35, Phillip Wood wrote: >>> On 27/09/2025 00:08, Siddharth Asthana wrote: >>>> The git replay command currently outputs update commands that must be >>>> piped to git update-ref --stdin to actually update references: >> >> The actual advantages of the new default aren't about atomicity (that >> already exists), but rather: >> - Eliminating the pipeline for the common case >> - Better ergonomics for users who just want refs updated >> - Simpler server-side automation >> >> I will rewrite the commit message to accurately reflect this. Elijah >> provided a good suggested structure that captures the real trade-offs >> without false claims. > > That's great. I agree that having replay update the refs itself is a > useful improvement. > >>>> +--allow-partial:: >>>> +    Allow some ref updates to succeed even if others fail. By >>>> default, >>>> +    ref updates are atomic (all succeed or all fail). With this >>>> option, >>>> +    failed updates are reported as warnings rather than causing >>>> the entire >>>> +    command to fail. The command exits with code 0 only if all >>>> updates >>>> +    succeed; any failures result in exit code 1. Cannot be used with >>>> +    `--output-commands`. >>> >>> Rather than having two incompatible options perhaps we could have a >>> single "--update-refs=(yes|print|allow-partial-updates)" argument. I >>> think the name "--allow-partial" is rather ambiguous as it does not >>> say what it is allowing to be partial. >> >> After thinking about this and Elijah's feedback, I am leaning toward >> dropping --allow-partial entirely since I don't have a concrete use case >> for it. That simplifies things to just: default atomic updates vs >> --output-commands for the traditional pipeline. >> >> Would you still prefer a --update-refs= style, or is the simpler >> --output-commands flag sufficient given that --allow-partial is going >> away? > > The advantage of --update-refs= is that it allows for future > extensions such as adding support for partial in a way that does not > add conflicting options. That's a good point about extensibility. Elijah suggested --[no-]update-refs which is simpler but less extensible. Between: - --[no-]update-refs (simple, covers current needs) - --update-refs= (extensible for future modes) I am inclined toward the simpler --[no-]update-refs for now since we don't have concrete plans for other modes. But if you think the extensibility is important, I can go with the = style. What do you think? > > Thanks > > Phillip >  > >>> >>>> +static int add_ref_to_transaction(struct ref_transaction >>>> *transaction, >>>> +                  const char *refname, >>>> +                  const struct object_id *new_oid, >>>> +                  const struct object_id *old_oid, >>>> +                  struct strbuf *err) >>>> +{ >>>> +    return ref_transaction_update(transaction, refname, new_oid, >>>> old_oid, >>>> +                      NULL, NULL, 0, "git replay", err); >>>> +} >>> >>> I'm not sure this function adds much value. I think it would be >>> better to instead have a helper function that updates refs or prints >>> the ref updates so that we do not duplicate that code in the two >>> places below. >> >> >> ood point. I will extract a helper like: >> >>      static int handle_ref_update(int output_commands, >>                                   struct ref_transaction *transaction, >>                                   const char *refname, >>                                   const struct object_id *new_oid, >>                                   const struct object_id *old_oid, >>                                   struct strbuf *err) >> >> This eliminates the duplication and fixes the over-long lines you >> pointed >> out at both call sites. >> >> Thanks! >> >> >>> >>>> @@ -434,10 +481,18 @@ 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 (output_commands) { >>>> +                    printf("update %s %s %s\n", >>>> +                           decoration->name, >>>> + oid_to_hex(&last_commit->object.oid), >>>> + oid_to_hex(&commit->object.oid)); >>>> +                } else if (add_ref_to_transaction(transaction, >>>> decoration->name, >>>> + &last_commit->object.oid, >>>> + &commit->object.oid, >>>> +                                  &transaction_err) < 0) { >>>> +                    ret = error(_("failed to add ref update to >>>> transaction: %s"), transaction_err.buf); >>>> +                    goto cleanup; >>>> +                } >>>>               } >>> >>> The lines here are very long due to the indentation, having a >>> separate function to update the refs or print the ref updates would >>> be much more readable. >>> >>>>               decoration = decoration->next; >>>>           } >>>> @@ -445,10 +500,33 @@ 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 (output_commands) { >>>> +            printf("update %s %s %s\n", >>>> +                   advance_name, >>>> + oid_to_hex(&last_commit->object.oid), >>>> +                   oid_to_hex(&onto->object.oid)); >>>> +        } else if (add_ref_to_transaction(transaction, advance_name, >>>> +                          &last_commit->object.oid, >>>> +                          &onto->object.oid, >>>> +                          &transaction_err) < 0) { >>>> +            ret = error(_("failed to add ref update to >>>> transaction: %s"), transaction_err.buf); >>>> +            goto cleanup; >>>> +        } >>>> +    } >>> >>> Putting the code to update the refs or print the ref updates into a >>> single function would avoid this duplication and over-long lines. >>> >>> Thanks >>> >>> Phillip >>> >>>> +    /* Commit the ref transaction if we have one */ >>>> +    if (transaction && result.clean == 1) { >>>> +        if (ref_transaction_commit(transaction, &transaction_err)) { >>>> +            if (allow_partial) { >>>> +                warning(_("some ref updates failed: %s"), >>>> transaction_err.buf); >>>> + ref_transaction_for_each_rejected_update(transaction, >>>> +                                     print_rejected_update, NULL); >>>> +                ret = 0; /* Set failure even with allow_partial */ >>>> +            } else { >>>> +                ret = error(_("failed to update refs: %s"), >>>> transaction_err.buf); >>>> +                goto cleanup; >>>> +            } >>>> +        } >>>>       } >>>>         merge_finalize(&merge_opt, &result); >>>> @@ -457,9 +535,17 @@ int cmd_replay(int argc, >>>>           strset_clear(update_refs); >>>>           free(update_refs); >>>>       } >>>> -    ret = result.clean; >>>> + >>>> +    /* Handle empty ranges: if no commits were processed, treat as >>>> success */ >>>> +    if (!commits_processed) >>>> +        ret = 1; /* Success - no commits to replay is not an error */ >>>> +    else >>>> +        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..8b4301e227 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 --output-commands --onto main topic1..topic2 >>>> >result && >>>>         test_line_count = 1 result && >>>>   @@ -67,9 +67,30 @@ test_expect_success 'using replay to rebase >>>> two branches, one on top of other' ' >>>>       test_cmp expect result >>>>   ' >>>>   +test_expect_success 'using replay with default atomic behavior >>>> (no output)' ' >>>> +    # Create a test branch that wont interfere with others >>>> +    git branch atomic-test topic2 && >>>> +    git rev-parse atomic-test >atomic-test-old && >>>> + >>>> +    # Default behavior: atomic ref updates (no output) >>>> +    git replay --onto main topic1..atomic-test >output && >>>> +    test_must_be_empty output && >>>> + >>>> +    # Verify the branch was updated >>>> +    git rev-parse atomic-test >atomic-test-new && >>>> +    ! test_cmp atomic-test-old atomic-test-new && >>>> + >>>> +    # Verify the history is correct >>>> +    git log --format=%s atomic-test >actual && >>>> +    test_write_lines E D M L B A >expect && >>>> +    test_cmp expect actual >>>> +' >>>> + >>>>   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 && >>>> -    test_cmp expect result-bare >>>> +    git -C bare replay --output-commands --onto main >>>> topic1..topic2 >result-bare && >>>> + >>>> +    # The result should match what we got from the regular repo >>>> +    test_cmp result result-bare >>>>   ' >>>>     test_expect_success 'using replay to rebase with a conflict' ' >>>> @@ -86,7 +107,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 --output-commands --advance main topic1..topic2 >>>> >result && >>>>         test_line_count = 1 result && >>>>   @@ -102,7 +123,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 --output-commands --advance main >>>> topic1..topic2 >result-bare && >>>>       test_cmp expect result-bare >>>>   ' >>>>   @@ -115,7 +136,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 --output-commands --contained --onto main >>>> main..topic3 >result && >>>>         test_line_count = 2 result && >>>>       cut -f 3 -d " " result >new-branch-tips && >>>> @@ -139,12 +160,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 --output-commands --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 --output-commands --onto main ^topic1 topic2 topic4 >>>> >result && >>>>         test_line_count = 2 result && >>>>       cut -f 3 -d " " result >new-branch-tips && >>>> @@ -168,7 +189,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 --output-commands --contained --onto main >>>> ^main topic2 topic3 topic4 >result && >>>>         test_line_count = 4 result && >>>>       cut -f 3 -d " " result >new-branch-tips && >>>> @@ -217,4 +238,131 @@ test_expect_success >>>> 'merge.directoryRenames=false' ' >>>>           --onto rename-onto rename-onto..rename-from >>>>   ' >>>>   +# Tests for new default atomic behavior and options> > >>>> +test_expect_success 'replay default behavior should not produce >>> output when successful' ' >>>> +    git replay --onto main topic1..topic3 >output && >>>> +    test_must_be_empty output >>>> +' >>>> + >>>> +test_expect_success 'replay with --output-commands produces >>>> traditional output' ' >>>> +    git replay --output-commands --onto main topic1..topic3 >>>> >output && >>>> +    test_line_count = 1 output && >>>> +    grep "^update refs/heads/topic3 " output >>>> +' >>>> + >>>> +test_expect_success 'replay with --allow-partial should not >>>> produce output when successful' ' >>>> +    git replay --allow-partial --onto main topic1..topic3 >output && >>>> +    test_must_be_empty output >>>> +' >>>> + >>>> +test_expect_success 'replay fails when --output-commands and -- >>>> allow-partial are used together' ' >>>> +    test_must_fail git replay --output-commands --allow-partial -- >>>> onto main topic1..topic2 2>error && >>>> +    grep "cannot be used together" error >>>> +' >>>> + >>>> +test_expect_success 'replay with --contained updates multiple >>>> branches atomically' ' >>>> +    # Create fresh test branches based on the original structure >>>> +    # contained-topic1 should be contained within the range to >>>> contained-topic3 >>>> +    git branch contained-base main && >>>> +    git checkout -b contained-topic1 contained-base && >>>> +    test_commit ContainedC && >>>> +    git checkout -b contained-topic3 contained-topic1 && >>>> +    test_commit ContainedG && >>>> +    test_commit ContainedH && >>>> +    git checkout main && >>>> + >>>> +    # Store original states >>>> +    git rev-parse contained-topic1 >contained-topic1-old && >>>> +    git rev-parse contained-topic3 >contained-topic3-old && >>>> + >>>> +    # Use --contained to update multiple branches - this should >>>> update both >>>> +    git replay --contained --onto main contained-base..contained- >>>> topic3 && >>>> + >>>> +    # Verify both branches were updated >>>> +    git rev-parse contained-topic1 >contained-topic1-new && >>>> +    git rev-parse contained-topic3 >contained-topic3-new && >>>> +    ! test_cmp contained-topic1-old contained-topic1-new && >>>> +    ! test_cmp contained-topic3-old contained-topic3-new >>>> +' >>>> + >>>> +test_expect_success 'replay atomic behavior: all refs updated or >>>> none' ' >>>> +    # Store original state >>>> +    git rev-parse topic4 >topic4-old && >>>> + >>>> +    # Default atomic behavior >>>> +    git replay --onto main main..topic4 && >>>> + >>>> +    # Verify ref was updated >>>> +    git rev-parse topic4 >topic4-new && >>>> +    ! test_cmp topic4-old topic4-new && >>>> + >>>> +    # Verify no partial state >>>> +    git log --format=%s topic4 >actual && >>>> +    test_write_lines J I M L B A >expect && >>>> +    test_cmp expect actual >>>> +' >>>> + >>>> +test_expect_success 'replay works correctly with bare repositories' ' >>>> +    # Test atomic behavior in bare repo (important for Gitaly) >>>> +    git checkout -b bare-test topic1 && >>>> +    test_commit BareTest && >>>> + >>>> +    # Test with bare repo - replay the commits from >>>> main..bare-test to get the full history >>>> +    git -C bare fetch .. bare-test:bare-test && >>>> +    git -C bare replay --onto main main..bare-test && >>>> + >>>> +    # Verify the bare repo was updated correctly (no output) >>>> +    git -C bare log --format=%s bare-test >actual && >>>> +    test_write_lines BareTest F C M L B A >expect && >>>> +    test_cmp expect actual >>>> +' >>>> + >>>> +test_expect_success 'replay --allow-partial with no failures >>>> produces no output' ' >>>> +    git checkout -b partial-test topic1 && >>>> +    test_commit PartialTest && >>>> + >>>> +    # Should succeed silently even with partial mode >>>> +    git replay --allow-partial --onto main topic1..partial-test >>>> >output && >>>> +    test_must_be_empty output >>>> +' >>>> + >>>> +test_expect_success 'replay maintains ref update consistency' ' >>>> +    # Test that traditional vs atomic produce equivalent results >>>> +    git checkout -b method1-test topic2 && >>>> +    git checkout -b method2-test topic2 && >>>> + >>>> +    # Both methods should update refs to point to the same >>>> replayed commits >>>> +    git replay --output-commands --onto main topic1..method1-test >>>> >update-commands && >>>> +    git update-ref --stdin >>> +    git log --format=%s method1-test >traditional-result && >>>> + >>>> +    # Direct atomic method should produce same commit history >>>> +    git replay --onto main topic1..method2-test && >>>> +    git log --format=%s method2-test >atomic-result && >>>> + >>>> +    # Both methods should produce identical commit histories >>>> +    test_cmp traditional-result atomic-result >>>> +' >>>> + >>>> +test_expect_success 'replay error messages are helpful and clear' ' >>>> +    # Test that error messages are clear >>>> +    test_must_fail git replay --output-commands --allow-partial -- >>>> onto main topic1..topic2 2>error && >>>> +    grep "cannot be used together" error >>>> +' >>>> + >>>> +test_expect_success 'replay with empty range produces no output >>>> and no changes' ' >>>> +    # Create a test branch for empty range testing >>>> +    git checkout -b empty-test topic1 && >>>> +    git rev-parse empty-test >empty-test-before && >>>> + >>>> +    # Empty range should succeed but do nothing >>>> +    git replay --onto main empty-test..empty-test >output && >>>> +    test_must_be_empty output && >>>> + >>>> +    # Branch should be unchanged >>>> +    git rev-parse empty-test >empty-test-after && >>>> +    test_cmp empty-test-before empty-test-after >>>> +' >>>> + >>>>   test_done >>> >