Re: [PATCH 8/8] reftable/stack: handle locked tables during auto-compaction

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]

On Fri, Aug 02, 2024 at 04:24:42PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > +			} else if (errno == EEXIST) {
> >  				err = REFTABLE_LOCK_ERROR;
> > -			else
> > +				goto done;
> > +			} else {
> >  				err = REFTABLE_IO_ERROR;
> > -			goto done;
> > +				goto done;
> > +			}
> >  		}
> >  
> >  		/*
> > @@ -1270,7 +1308,7 @@ static int stack_compact_range(struct reftable_stack *st,
> >  	 * delete the files after we closed them on Windows, so this needs to
> >  	 * happen first.
> >  	 */
> > -	err = reftable_stack_reload_maybe_reuse(st, first < last);
> > +	err = reftable_stack_reload_maybe_reuse(st, first_to_replace < last_to_replace);
> 
> What is this change about?
> 
> No code changed that computes first_to_replace and last_to_replace?
> Perhaps before this step, using first/last and using
> first_to_replace/last_to_replace did not make any difference,
> because we never dealt with a case where we failed to lock any of
> the tables?
> 
> I am wondering if this would be helped by a no-op clarification
> before the actual behaviour change, similar to how step 4/8 added
> the nlocks variable.

As we know that we always want to replace the same set of tables, just
with different offsets, it follows that `first - last` is always equal
to `first_to_replace - last_to_replace`.

So this is a no-op change, let me just drop it.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]