From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 08 Oct 2025 14:01:13 GMT
Subject: Re: [PATCH v2 1/1] replay: make atomic ref updates the default behavior
Message-ID: <d9764c7b-8de2-4b54-8c44-a4bd7f5860e8@gmail.com>
In-Reply-To: <9d310bd5-453f-43a4-b477-ba02baa7a664@gmail.com>

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
>>