From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 09 Sep 2025 09:00:44 GMT
Subject: Re: [PATCH 1/2] replay: add --update-refs option
Message-ID: <aL_svO5Ils8r9DkT@pks.im>
In-Reply-To: <c7615356-04cc-47e2-a894-4d24e416e4ad@gmail.com>

On Tue, Sep 09, 2025 at 12:28:29PM +0530, Siddharth Asthana wrote:
> On 08/09/25 15:24, Patrick Steinhardt wrote:
> > Is there a strong reason why a user would want to update refs one by
> > one? If not, let's not add new code to our base that does so. This is
> > known to be inperformant for the reftable backend, but also for the
> > files backend in some cases.
> 
> You are absolutely right about the performance concern. My thinking was to
> provide a simple mode that exactly mimics "git replay | git update-ref
> --stdin" behavior, but I see that's not worth the performance cost.
> 
> I will remove the individual update function and only use batched
> transactions with REF_TRANSACTION_ALLOW_FAILURE when needed.

We can still extend the functionality at a later point if we discover
any use cases for those.

> > > @@ -319,6 +355,12 @@ int cmd_replay(int argc,
> > >   			   N_("replay onto given commit")),
> > >   		OPT_BOOL(0, "contained", &contained,
> > >   			 N_("advance all branches contained in revision-range")),
> > > +		OPT_BOOL(0, "update", &update_directly,
> > > +			 N_("update branches directly instead of outputting update commands")),
> > > +		OPT_BOOL(0, "update-refs", &update_refs_flag,
> > > +			 N_("update branches using ref transactions")),
> > > +		OPT_BOOL(0, "batch", &batch_mode,
> > > +			 N_("allow partial ref updates in batch mode")),
> > >   		OPT_END()
> > >   	};
> > So I think we should reduce this to only accept two flags:
> > `--update-refs` and a flag that accepts a subset of refs failing.o
> > 
> > We might also want to make this something like `--update-refs[=<mode>]`,
> > where `<mode>` could be "allow-failures".
> 
> 
> That make sense. Would you prefer `--update-refs` with `--allow-failures` as
> a separate flag? I am leaning toward that since it's clearer than the
> parameter syntax.

I'd personally prefer `--update-refs[=<mode>]`. The reason is mostly
that it makes it easier to discover what flags are related to the
`--update-refs` infra and you have to worry less about catching any kind
of incompatible flag combinations.

Patrick