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

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 12, 2024, 09:05 UTC
Message-ID: <ZzMaX8HCj3GO5JUJ@pks.im>
In-Reply-To: <xmqqo72l54yb.fsf@gitster.g>

On Tue, Nov 12, 2024 at 03:41:48PM +0900, Junio C Hamano wrote:

Show 37 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> > -	ret = read_ref_without_reload(refs, stack, refname, oid, referent, type);
> > +	ret = read_ref_without_reload(refs, be->stack, refname, oid, referent, type);
> 
> 
> The following bit is curious.
> 
> > +	ret = backend_for(&be, refs, update->refname, NULL, 0);
> > +	if (ret)
> > +		return ret;
> > +
> 
> We locate one without reloading, and ...
> 
> >  	/*
> >  	 * Search for a preexisting stack update. If there is one then we add
> >  	 * the update to it, otherwise we set up a new stack update.
> >  	 */
> >  	for (i = 0; !arg && i < tx_data->args_nr; i++)
> > -		if (tx_data->args[i].stack == stack)
> > +		if (tx_data->args[i].be == be)
> >  			arg = &tx_data->args[i];
> >  	if (!arg) {
> 
> ... only when we cannot reuse preexisting one, ...
> 
> >  		struct reftable_addition *addition;
> >  
> > -		ret = reftable_stack_reload(stack);
> > +		ret = backend_for(&be, refs, update->refname, NULL, 1);
> >  		if (ret)
> >  			return ret;
> 
> ... instead of directly doing reload on the instance we already
> have, we do another _for() to locate one, this time reload set to 1.
> 
> That looks like doing some redundant work?  I am confused.

It indeed is redundant work, yes. And in fact it is redundant work that isn't really required anymore. My first iteration didn't yet have the `reftable_write_options::on_reload()` callback, and I instead tried to catch reloads via `backend_for()`, so it was required to reload via that function.

But now that we do have the callback that isn't needed anymore, and thus we don't have to call `backend_for()` a second time here. I'll adapt this accordingly.

Patrick