Re: [PATCH v2 8/9] reftable/stack: fix corruption on concurrent compaction

> Patrick Steinhardt <ps@pks.im> writes:
> > +			/*
> > +			 * We have found the first entry. Verify that all the
> > +			 * subsequent tables we have compacted still exist in
> > +			 * the modified stack in the exact same order as we
> > +			 * have compacted them.
> > +			 */
> > +			for (size_t j = 1; j < last - first + 1; j++) {
> > +				const char *old = first + j < st->merged->stack_len ?
> > +					st->readers[first + j]->name : NULL;
> > +				const char *new = names[i + j];
> > +
> > +				/*
> > +				 * If some entries are missing or in case the tables
> > +				 * have changed then we need to bail out. Again, this
> > +				 * shouldn't ever happen because we have locked the
> > +				 * tables we are compacting.
> > +				 */
> 
> Okay, this is exactly what I was saying above. It still does makes sense
> to keep this check to ensure future versions don't break it.

> > +				if (!old || !new || strcmp(old, new)) {
> > +					err = REFTABLE_OUTDATED_ERROR;
> > +					goto done;
> > +				}
> > +			}
> > +
> > +			new_offset = i;
> > +			break;
> > +		}
> > +
> > +		/*
> > +		 * In case we didn't find our compacted tables in the stack we
> > +		 * need to bail out. In theory, this should have never happened
> > +		 * because we locked the tables we are compacting.
> > +		 */
> > +		if (new_offset < 0) {
> > +			err = REFTABLE_OUTDATED_ERROR;
> > +			goto done;
> > +		}
> > +
> > +		/*
> > +		 * We have found the new range that we want to replace, so
> > +		 * let's update the range of tables that we want to replace.
> > +		 */
> > +		last_to_replace = last + (new_offset - first);
> > +		first_to_replace = new_offset;
> 
> Nit: might be easier to read as
> 
>   first_to_replace = new_offset;
>   last_to_replace = first_to_replace + (last - first);