Re: [PATCH v2 1/1] replay: make atomic ref updates the default behavior

> 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=<mode> style, or is the simpler
>> --output-commands flag sufficient given that --allow-partial is going 
>> away?
>
> The advantage of --update-refs=<mode> is that it allows for future 
> extensions such as adding support for partial in a way that does not 
> add conflicting options.

>
> 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 <update-commands &&
>>>> +    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
>>>
>