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

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 02/10/25 23:57, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>>    * it provided a natural low-level tool for the suite of hash-object,
>> mktree, commit-tree, mktag, merge-tree, and update-ref, allowing users
>> to have another building block for experimentation and making new
>> tools.
>>
>> I was particularly focused on the last of those items for the intial
>> version at the time, but it should be noted that all three of these
>> are somewhat special cases, and the most common user desire is going
>> to be replaying commits and updating the references at the end.
> Yes.  We could even tweak the stream in the middle with "sed" or
> "grep", I presume? ;-)
>
>> Sure, but it's quite trivial to add, right -- as shown above with the
>> extra "start", "prepare", "commit" directives?
> Very true.
>
> Completely a tangent but, isn't requiring "prepare" at this layer,
> and possibly in the form of ref_transaction_prepare() at the C
> layer, not so ergonomic API design?  Once you "start" a transaction
> and threw a bunch of instruction, "commit" can notice that you are
> in a transaction and should do whatever necessary (including
> whatever "prepare" does).  I am not advocating to simplify the API
> by making end-user/program facing "prepare" a no-op, but just
> wondering why we decided to have "prepare" a so prominent API
> element.


For this patch, I am using the simpler pattern: 
ref_store_transaction_begin()
→ ref_transaction_update() → ref_transaction_commit(). Looking at
builtin/update-ref.c and other code, it seems commit() already handles
whatever prepare does internally when you are not using the explicit stdin
transaction commands.

Should I continue with that pattern, or is there a reason to use prepare()
explicitly even when not doing the stdin command flow?

On the config option you suggested in v1: I will add a config setting so
users can set their preference once. I am thinking either replay.updateRefs
(boolean) or replay.defaultOutput (string: "update"|"commands"). Any
preference on the naming pattern?


>
>> ...
>> Might I suggest a rewrite of the text of the commit message to this point?
> I do think it makes more sense to the reader to know the reasoning
> behind the _current_ design, and what its strengths are.


Thanks Junio. Elijah's rewritten structure is much clearer - I will use it
for v3. It properly explains the trade-offs without the false claims I made
about atomicity.


>
>> =====
>> The git replay command currently outputs update commands that can be
>> piped to update-ref to achieve a rebase, e.g.
>>
>>    git replay --onto main topic1..topic2 | git update-ref --stdin
>>
>> This separation had advantages for three special cases:
>>    * it made testing easy (when state isn't modified from one step to
>> the next, you don't need to make temporary branches or have undo
>> commands, or try to track the changes)
>>    * it provided a natural can-it-rebase-cleanly (and what would it
>> rebase to) capability without automatically updating refs, I guess
>> kind of like a --dry-run
>>    * it provided a natural low-level tool for the suite of hash-object,
>> mktree, commit-tree, mktag, merge-tree, and update-ref, allowing users
>> to have another building block for experimentation and making new
>> tools.
>>
>> However, it should be noted that all three of these are somewhat
>> special cases; users, whether on the client or server side, would
>> almost certainly find it more ergonomical to simply have the updating
>> of refs be the default.  Change the default behavior to update refs
>> directly, and atomically (at least to the extent supported by the refs
>> backend in use).
>> ====
> This reads very well.
>
>> Why is --allow-partial helpful?  You discussed at length why you
>> wanted atomic transactions, but you introduce this option with no
>> rationale and instead just discuss that you implemented it and some
>> design choices once you presuppose that someone wants to use it.
>>
>> Is there a usecase?  I asked for it last time, and suggested
>> discarding the modes without one, but you only discarded one of the
>> extras while leaving this one in.  I'd recommend discarding this one
>> too and just having the two modes -- the output commands that get fed
>> to update-ref, or the automatic transactional update of all or no
>> refs.
> I know there are people who like "best effort", but I too want to
> learn a concrete use case where the "best effort" mode, which
> updates only 3 refs among 30 that were to be updated, would give us
> a better result than "all or none" transaction that fails.


I don't have one. Elijah made the same point - I was trying to anticipate
needs without justification. I am removing --allow-partial from v3, keeping
just the two clear modes: atomic updates (default) or --output-commands
for the traditional pipeline.

Thanks!