From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Thu, 02 Oct 2025 22:20:01 GMT
Subject: Re: [PATCH v2 1/1] replay: make atomic ref updates the default behavior
Message-ID: <9d310bd5-453f-43a4-b477-ba02baa7a664@gmail.com>
In-Reply-To: <9052eccc-1121-442f-ad51-4fe9217024a0@gmail.com>


On 30/09/25 15:35, Phillip Wood wrote:
> Hi Siddharth
>
> 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:
>>
>>      git replay --onto main topic1..topic2 | git update-ref --stdin
>>
>> This design has significant limitations for server-side operations. The
>> two-command pipeline creates coordination complexity, provides no atomic
>> transaction guarantees by default
>
> Are you sure that's true? Maybe I'm missing something but my reading 
> of builtin/update-ref.c is that it when "--stdin" is given it starts a 
> ref transaction, reads the commands from stdin and applies them to 
> that transaction and then commits the transaction which will make the 
> updates atomic.


You are absolutely right, and Karthik confirmed this as well. That was a
significant error in my commit message. git update-ref --stdin IS atomic
by default.


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.


>
>> , and complicates automation in bare
>> repository environments where git replay is primarily used.
>
> How does it complicate automation in bare repositories?
>
> Christian has given detailed feedback on the rest of the commit 
> message so I'll not comment on it further.
>
>> diff --git a/Documentation/git-replay.adoc 
>> b/Documentation/git-replay.adoc
>> index 0b12bf8aa4..e104e0bc03 100644
>> --- a/Documentation/git-replay.adoc
>> +++ b/Documentation/git-replay.adoc
>> @@ -9,16 +9,16 @@ git-replay - EXPERIMENTAL: Replay commits on a new 
>> base, works with bare repos t
>>   SYNOPSIS
>>   --------
>>   [verse]
>> -(EXPERIMENTAL!) 'git replay' ([--contained] --onto <newbase> | 
>> --advance <branch>) <revision-range>...
>> +(EXPERIMENTAL!) 'git replay' ([--contained] --onto <newbase> | 
>> --advance <branch>) [--output-commands | --allow-partial] 
>> <revision-range>...
>
> Please wrap this very long line
>
>> @@ -42,6 +42,20 @@ When `--advance` is specified, the update-ref 
>> command(s) in the output
>>   will update the branch passed as an argument to `--advance` to 
>> point at
>>   the new commits (in other words, this mimics a cherry-pick operation).
>>   +--output-commands::
>> +    Output update-ref commands instead of updating refs directly.
>> +    When this option is used, the output can be piped to `git 
>> update-ref --stdin`
>> +    for successive, relatively slow, ref updates. This is equivalent 
>> to the
>> +    old default behavior.
>> +
>> +--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?


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