Re: [PATCH 2/8] refs/reftable: handle reloading stacks in the reftable backend

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 6, 2024, 10:43 UTC
Message-ID: <ZytISDjXYp41caZW@pks.im>
In-Reply-To: <CAOLa=ZQwUGzjmvCv8TKuwbeePTG6+HNjU3ngO6hD8=xSOQ5kaQ@mail.gmail.com>

On Tue, Nov 05, 2024 at 05:14:15AM -0600, karthik nayak wrote:

Show 19 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> [snip]
> 
> > @@ -898,27 +915,31 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,
> >  				      struct ref_update *update,
> >  				      struct strbuf *err)
> >  {
> > -	struct reftable_stack *stack = backend_for(refs, update->refname, NULL)->stack;
> >  	struct write_transaction_table_arg *arg = NULL;
> > +	struct reftable_backend *be;
> >  	size_t i;
> >  	int ret;
> >
> > +	ret = backend_for(&be, refs, update->refname, NULL, 0);
> 
> So here, we don't reload the stack, it would be nice to add a comment
> why, in the places we don't. Here, it seems to be because we possibly
> already have an update which would have pushed us to reload the stack.

I'll add comments.

Show 10 quoted lines

> > @@ -2462,7 +2498,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
> >  	arg.refs = refs;
> >  	arg.records = rewritten;
> >  	arg.len = logs_nr;
> > -	arg.stack = stack,
> > +	arg.stack = be->stack,
> >  	arg.refname = refname,
> >
> 
> Shouldn't these lines end with ';'?

Yup, they should. It's harmless, but certainly confusing.

Patrick