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

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 30/09/25 13:53, Christian Couder wrote:
> On Sat, Sep 27, 2025 at 1:09 AM Siddharth Asthana
> <siddharthasthana31@gmail.com> 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, and complicates automation in bare
>> repository environments where git replay is primarily used.
> Yeah, right.
>
>> During extensive mailing list discussion, multiple maintainers identified
>> that the current approach
> When you say "current approach" we first think we are talking about
> the behavior you described above when you said "The git replay command
> currently ..."
>
>> forces users to opt-in to atomic behavior rather
>> than defaulting to the safer, more reliable option.
> But here you are actually talking about what the previous version of
> this patch did.
>
>> Elijah Newren noted
>> that the experimental status explicitly allows such behavior changes, while
>> Patrick Steinhardt highlighted performance concerns with individual ref
>> updates in the reftable backend.
> Also the commit message is not the right place to describe what
> happened during discussions of the previous version(s) of a patch.
> It's not the right place to talk about previous version(s) of a patch
> in general. Those things should go into the cover letter.
>
> If you want to talk about an option that was considered but rejected,
> you can say something like the following instead of the whole
> paragraph:
>
> "To address this limitation, adding an option named for example
> `--atomic-update` was considered. With such an option `git replay
> --atomic-update --onto main topic1..topic2` would atomically update
> all the refs without having to use a separate `git update-ref --stdin`
> command. The issue is that this would force users to opt-in to the
> atomic behavior rather than have it as the default safer, faster and
> more reliable option.
>
> Fortunately the experimental status of the `git replay` command
> explicitly allows behavior changes, so we are allowed to make the
> command atomically update all the refs by default.
> "


Hi Christian,

Thanks for the detailed commit message review. You are absolutely right - I
was mixing the patch rationale with v1→v2 changelog, which belongs in the
cover letter.

Your suggested framing about considering an --atomic-update option but
rejecting it in favor of making it default is much clearer than my
approach. I will use that structure.

For v3:
- Move all "since v1" discussion to cover letter
- Use imperative mood ("Let's change" not "This patch changes")
- Be explicit that --output-commands and --allow-partial are new options
- Add full stops to the implementation details list
- Will add Helped-by trailers for Elijah, Patrick and you ofcourse as 
suggested.

Quick question: for the C89 compliance mention, should I drop it entirely
or briefly note "uses 'int' instead of 'bool' for C89 compatibility"? I
want to acknowledge the bool→int change but not belabor it.

Thanks again!


>
>> The core issue is that git replay was designed around command output rather
>> than direct action. This made sense for a plumbing tool, but creates barriers
>> for the primary use case: server-side operations that need reliable, atomic
>> ref updates without pipeline complexity.
> I think this paragraph should go just before the "Fortunately the
> experimental status of the `git replay` command explicitly ..." that I
> suggest above.
>
>> This patch changes the default behavior to update refs directly using Git's
> s/This patch changes/Let's change/
>
> (See our SubmittingPatches documentation where it suggests using
> imperative mood to describe the changes we make.)
>
>> ref transaction API:
>>
>>      git replay --onto main topic1..topic2
>>      # No output; all refs updated atomically or none
>>
>> The implementation uses ref_store_transaction_begin() with atomic mode by
>> default, ensuring all ref updates succeed or all fail as a single operation.
>> This leverages git replay's existing server-side strengths (in-memory operation,
>> no work tree requirement) while adding the atomic guarantees that server
>> operations require.
>>
>> For users needing the traditional pipeline workflow, --output-commands
>> preserves the original behavior:
> I think something like:
>
> "For users needing the traditional pipeline workflow, let's add a new
> `--output-commands`option that preserves the original behavior:"
>
> is more explicit and makes it clear that it's a new option added by
> this patch and not an existing option.
>
>>      git replay --output-commands --onto main topic1..topic2 | git update-ref --stdin
>>
>> The --allow-partial option enables partial failure tolerance.
> In the same way, something like:
>
> "Let's also add a new `--allow-partial` option that enables partial
> failure tolerance."
>
>> However, following
>> maintainer feedback, it implements a "strict success" model: the command exits
> I think you can remove "following maintainer feedback" here. The cover
> letter or a trailer like "Helped-by: ..." at the end of the commit
> message (but Junio will add his "Signed-off-by: ..." anyway so adding
> an Helped-by: ... about him is redundant) are the right place to
> mention people who helped or suggested changes.
>
>> with code 0 only if ALL ref updates succeed, and exits with code 1 if ANY
>> updates fail. This ensures that --allow-partial changes error reporting style
>> (warnings vs hard errors) but not success criteria, handling edge cases like
>> "no updates needed" cleanly.
>>
>> Implementation details:
>> - Empty commit ranges now return success (exit code 0) rather than failure,
>>    as no commits to replay is a valid successful operation
> Nit: as all the sentences in this "Implementation details" list start
> with an uppercase, I think they should end with a full stop.
>
>> - Added comprehensive test coverage with 12 new tests covering atomic behavior,
>>    option validation, bare repository support, and edge cases
>> - Fixed test isolation issues to prevent branch state contamination between tests
>> - Maintains C89 compliance and follows Git's established coding conventions
> I am not sure this one is worth mentioning here, at least not like
> this. You may want to say in the cover letter that compared to the
> previous version this patch doesn't use 'bool' anymore and explain
> why. Or maybe you want to explain here that using the 'bool' type was
> considered but rejected for some reason. But in both cases, you should
> be explicit about the reason.
>
>> - Refactored option validation to use die_for_incompatible_opt2() for both
>>    --advance/--contained and --allow-partial/--output-commands conflicts,
>>    providing consistent error reporting
>> - Fixed --allow-partial exit code behavior to implement "strict success" model
>>    where any ref update failures result in exit code 1, even with partial tolerance
> This should probably go to the cover letter, as we should not talk in
> the commit message about changes since a previous version of the
> commit.
>
>> - Updated documentation with proper line wrapping, consistent terminology using
>>    "old default behavior", performance context, and reorganized examples for clarity
> This also sounds like a change compared to the previous version of the patch.
>
>> - Eliminates individual ref updates (refs_update_ref calls) that perform
>>    poorly with reftable backend
> This also sounds like a change compared to the previous version of the patch.
>
>> - Uses only batched ref transactions for optimal performance across all
>>    ref backends
> I think you can remove "only" in the sentence as in the
> --output-commands case no transaction is used.
>
>> - Avoids naming collision with git rebase --update-refs by using distinct
>>    option names
> This also sounds like a change compared to the previous version of the patch.
>
>> - Defaults to atomic behavior while preserving pipeline compatibility
> This has been discussed above. It doesn't look like an implementation
> detail to me.
>
>> The result is a command that works better for its primary use case (server-side
>> operations) while maintaining full backward compatibility for existing workflows.
>>
>> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
> Adding "Helped-by: ..." trailers for at least Elijah and Patrick would be nice.