Re: [PATCH 01/15] reftable/stack: fix error handling in `reftable_stack_init_addition()`

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 25, 2024, 09:10 UTC
Message-ID: <ZgE_ggdkO6ejjuOG@tanuki>
In-Reply-To: <CAOLa=ZQ5wFCqWZ4yjMzUg5Yz1UeVpwUap0z9Lx9EfRVKnzHttQ@mail.gmail.com>

On Wed, Mar 20, 2024 at 02:50:43PM -0700, Karthik Nayak wrote:

Show 28 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index 1ecf1b9751..92d9a7facb 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -590,8 +590,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
> >  	err = stack_uptodate(st);
> >  	if (err < 0)
> >  		goto done;
> > -
> > -	if (err > 1) {
> > +	if (err > 0) {
> >  		err = REFTABLE_LOCK_ERROR;
> >  		goto done;
> >  	}
> > @@ -713,10 +712,6 @@ static int stack_try_add(struct reftable_stack *st,
> >  	int err = reftable_stack_init_addition(&add, st);
> >  	if (err < 0)
> >  		goto done;
> > -	if (err > 0) {
> > -		err = REFTABLE_LOCK_ERROR;
> > -		goto done;
> > -	}
> 
> This changes the behavior though, since now we skip the `goto done`. It
> would be best to change the previous line to `if (err)`, which is what
> the other function (`reftable_stack_new_addition`) does.

It actually doesn't because `reftable_stack_init_addition()` does not return a positive value anymore. And as it returns `REFTABLE_LOCK_ERROR` in the case where it previously did return a positive value the behaviour is exactly the same because we go into thee `if (err < 0)` branch.

I'll amend the commit message to clariyf.

Patrick