Re: [PATCH 21/20] t5319: make corrupted large-offset test more robust

From: Jeff King <peff@peff.net>
Date: Oct 15, 2023, 03:17 UTC
Message-ID: <20231015031732.GB554702@coredump.intra.peff.net>
In-Reply-To: <xmqq4jitt2ie.fsf@gitster.g>

On Sat, Oct 14, 2023 at 12:42:01PM -0700, Junio C Hamano wrote:

Show 5 quoted lines

> > The set of objects created in the test is deterministic. But the delta
> > selection seems not to be (which is not too surprising, as it is
> > multi-threaded).
> 
> So, the offsets of the objects are also not deterministic?

Hmm, yeah, you're right. The pack clearly is not deterministic, and so neither will its offsets be.

And thus...

Show 11 quoted lines

> >   b. The "objects64" setup could use --delta-base-offset. This would fix
> >      our problem, but earlier tests have many hard-coded offsets. Using
> >      OFS_DELTA would change the locations of objects in the pack (this
> >      might even be OK because I think most of the offsets are within the
> >      .idx file, but it seems brittle and I'm afraid to touch it).
> 
> I am not sure I follow, as it does not sound a solution to anything
> if the offsets are not deterministic (and "earlier tests" that have
> hard coded offsets are broken no matter what, which is not a problem
> this patch introduces).  Puzzled, but not curious enough to think
> about it further, as you have already rejected this approach ;-)

...yes, my "this might even be OK" is true. So it would work to solve the problem by using --delta-base-offset. Those earlier tests are actually OK (they munge only the idx and the midx, not the pack). And if we have delta base offsets, then walking delta chains never requires looking at the pack idx.

I'm not sure if that is any less subtle than the solution I did come up with, though.

The most unsubtle thing is cleaning up the broken .idx immediately after generating the midx. I didn't want to do that because it disrupts the state of the objects64 directory. But we could always turn its setup into a function or something. I dunno. It is probably not worth spending too many brain cycles on. My hope is that nobody ever has to touch this test ever again. ;)

Show 14 quoted lines

> > @@ -1129,8 +1129,10 @@ test_expect_success 'reader bounds-checks large offset table' '
> >  		git multi-pack-index --object-dir=../objects64 write &&
> >  		midx=../objects64/pack/multi-pack-index &&
> >  		corrupt_chunk_file $midx LOFF clear &&
> > -		test_must_fail git cat-file \
> > -			--batch-check --batch-all-objects 2>err &&
> > +		# using only %(objectsize) is important here; see the commit
> > +		# message for more details
> > +		test_must_fail git cat-file --batch-all-objects \
> > +			--batch-check="%(objectsize)" 2>err &&
> 
> A rather unfriendly message to readers, as it is unclear which
> commit you are talking about, and a fun thing is that you cannot
> say which one.

Yeah, I had a similar thought process. I sort of assume anybody working on git.git is capable of turning to "git blame" in a situation like this. But maybe:

  # using only %(objectsize) is important here; run "git blame"
  # on these lines for more details

would spell it out more clearly.

-Peff