Re: [PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 11, 2024, 23:24 UTC
Message-ID: <Ze-SnjgaLNoMvzW5@tanuki>
In-Reply-To: <Ze9Or6vctPJGOV_l@google.com>

On Mon, Mar 11, 2024 at 11:34:23AM -0700, Josh Steadmon wrote:

Show 32 quoted lines

> On 2024.03.05 13:10, Patrick Steinhardt wrote:
> > When creating a new reflog iterator, we first have to reload the stack
> > that the iterator is being created. This is done so that any concurrent
> > writes to the stack are reflected. But `reflog_iterator_for_stack()`
> > always reloads the main stack, which is wrong.
> > 
> > Fix this and reload the correct stack.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  refs/reftable-backend.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> > index 249a618b5a..f04be942ac 100644
> > --- a/refs/reftable-backend.c
> > +++ b/refs/reftable-backend.c
> > @@ -1682,7 +1682,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl
> >  	if (ret)
> >  		goto done;
> >  
> > -	ret = reftable_stack_reload(refs->main_stack);
> > +	ret = reftable_stack_reload(stack);
> >  	if (ret < 0)
> >  		goto done;
> >  
> > -- 
> > 2.44.0
> > 
> 
> Is it possible to write a test to demonstrate the bug that was fixed
> here, or is it too much of a race condition to reliably trigger?

I wouldn't really know how to test for this in a way that is even somewhat reliably, unfortunately. You have to have at least two concurrent commands, one reading and one writing, where the first command loads the worktree stack and then tries to create an iter as above.

Now if this was part of the reftable library we could do it rather easily via t0032, which contains unit tests for the reftable library. But in the reftable backend it's much harder.

Patrick