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

From: Christian Couder <christian.couder@gmail.com>
Date: Sep 30, 2025, 08:23 UTC
Message-ID: <CAP8UFD0POvYDgGtEx8GBhvKkd8XzzWQsy8XxAKL9M3+uz3ka+w@mail.gmail.com>
In-Reply-To: <20250926230838.35870-2-siddharthasthana31@gmail.com>

On Sat, Sep 27, 2025 at 1:09 AM Siddharth Asthana <siddharthasthana31@gmail.com> wrote:

Show 10 quoted lines

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

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

Show 13 quoted lines

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

Show 8 quoted lines

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

Show 5 quoted lines

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