| Message ID | 1e39d93a45db502280ecff383d53e0294f969719.1710706118.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: introduce `--auto` to pack refs as needed | expand |
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.
On Wed, Mar 20, 2024 at 02:50:43PM -0700, Karthik Nayak wrote: > 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
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; - } err = reftable_addition_add(&add, write_table, arg); if (err < 0)
In `reftable_stack_init_addition()` we call `stack_uptodate()` after having created the lockfile to check whether the stack was modified concurrently, which is indicated by a positive return code from the latter function. If so, we return a `REFTABLE_LOCK_ERROR` to the caller and abort the addition. The error handling has an off-by-one though because we check whether the error code is `> 1` instead of `> 0`. Thus, instead of returning the locking error, we would return a positive value. One of the callers of `reftable_stack_init_addition()` works around this bug by repeating the error code check without the off-by-one. But other callers are subtly broken by this bug. Fix this by checking for `err > 0` instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/stack.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)