Re: [PATCH v3 10/10] t5325: check both on-disk and in-memory reverse index

From: Jeff King <peff@peff.net>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, dstolee@microsoft.com, gitster@pobox.com,
	jrnieder@gmail.com
Subject: Re: [PATCH v3 10/10] t5325: check both on-disk and in-memory reverse index
Date: Thu, 28 Jan 2021 20:04:00 -0500	[thread overview]
Message-ID: <YBNfAEb+LYAIiPXB@coredump.intra.peff.net> (raw)
In-Reply-To: <38c8afabf25a7f5e144850938cf00b53e9cf25fd.1611617820.git.me@ttaylorr.com>

On Mon, Jan 25, 2021 at 06:37:51PM -0500, Taylor Blau wrote:

> Right now, the test suite can be run with 'GIT_TEST_WRITE_REV_INDEX=1'
> in the environment, which causes all operations which write a pack to
> also write a .rev file.
> 
> To prepare for when that eventually becomes the default, we should
> continue to test the in-memory reverse index, too, in order to avoid
> losing existing coverage. Unfortuantely, explicit existing coverage is
> rather sparse, so only a basic test is added.
> 
> The new test is parameterized over whether or not the .rev file should
> be written, and is run in both modes by t5325 (without having to touch
> GIT_TEST_WRITE_REV_INDEX).

Thanks, I think this is worth having.

> +revindex_tests () {
> +	on_disk="$1"
> +
> +	test_expect_success "setup revindex tests (on_disk=$on_disk)" "
> +		test_oid_cache <<-EOF &&
> +		disklen sha1:47
> +		disklen sha256:59
> +		EOF

These sizes aren't really guaranteed to be stable. I think you've
eliminated most variables by not having the opportunity for deltas,
though we're still subject to the whims of zlib, for example.

I think what we care about most is just that the on-disk and fallback
cases generate the same results. If they do, we can assume those results
are probably sane. If they're not, then a lot of _other_ stuff is going
to be broken (like, say, pack-objects, which is writing out N bytes
based on the sizes claimed by the revindex).

What do you think about just doing this:

  ...setup some commits... &&
  git rev-list --objects --no-object-names --all >objects &&

  git -c pack.writeReverseIndex=false repack -ad &&
  test_path_is_missing .git/objects/pack/*.rev &&
  git cat-file --batch-check="%(objectsize:disk) %(objectname)" \
          <objects >in-core &&

  git -c pack.writeReverseIndex=true repack -ad &&
  test_path_is_file .git/objects/pack/*.rev &&
  git cat-file --batch-check="%(objectsize:disk) %(objectname)" \
          <objects >on-disk &&

  test_cmp on-disk in-core

> +			if test ztrue = \"z$on_disk\"
> +			then
> +				git config pack.writeReverseIndex true
> +			fi &&

Are we autotools now? :) I think we can assume that on_disk contains
something sensible. Perhaps even:

  git config pack.writeReverseIndex $on_disk

though if you take my advice above, all of this goes away.

-Peff