Re: Test breakage with zlib-ng

From: Jeff King <peff@peff.net>
Date: Dec 12, 2023, 20:01 UTC
Message-ID: <20231212200153.GB1127366@coredump.intra.peff.net>
In-Reply-To: <9feeb6cf-aabf-4002-917f-3f6c27547bc8@web.de>

On Tue, Dec 12, 2023 at 06:04:55PM +0100, René Scharfe wrote:

Show 14 quoted lines

> Subject: [PATCH] t6300: avoid hard-coding object sizes
> 
> f4ee22b526 (ref-filter: add tests for objectsize:disk, 2018-12-24)
> hard-coded the expected object sizes.  Coincidentally the size of commit
> and tag is the same with zlib at the default compression level.
> 
> 1f5f8f3e85 (t6300: abstract away SHA-1-specific constants, 2020-02-22)
> encoded the sizes as a single value, which coincidentally also works
> with sha256.
> 
> Different compression libraries like zlib-ng may arrive at different
> values.  Get them from the file system instead of hard-coding them to
> make switching the compression library (or changing the compression
> level) easier.

Yeah, this is definitely the right solution here. I'm surprised the hard-coded values didn't cause problems before now. ;)

The patch looks good to me, but a few small comments:

Show 5 quoted lines

> +test_object_file_size () {
> +	oid=$(git rev-parse "$1")
> +	path=".git/objects/$(test_oid_to_path $oid)"
> +	test_file_size "$path"
> +}

Here we're assuming the objects are loose. I think that's probably OK (and certainly the test will notice if that changes).

We're covering the formatting code paths along with the underlying implementation that fills in object_info->disk_sizep for loose objects. Which I think is plenty for this particular script, which is about for-each-ref.

It would be nice to have coverage of the packed_object_info() code path, though. Back when it was added in a4ac106178 (cat-file: add %(objectsize:disk) format atom, 2013-07-10), I cowardly punted on this, writing:

  This patch does not include any tests, as the exact numbers
  returned are volatile and subject to zlib and packing
  decisions. We cannot even reliably guarantee that the
  on-disk size is smaller than the object content (though in
  general this should be the case for non-trivial objects).

I don't think it's that big a deal, but I guess we could do something like:

  prev=
  git show-index <$pack_idx |
  sort -n |
  grep -A1 $oid |
  while read ofs oid csum
  do
    test -n "$prev" && echo "$((ofs - prev))"
    prev=$ofs
  done

It feels a little redundant with what Git is doing under the hood, but at least is exercising the code (and we're using the idx directly, so we're confirming that the revindex is right).

Anyway, that is all way beyond the scope of your patch, but I wonder if it's worth doing on top.

Show 6 quoted lines

> @@ -129,7 +129,7 @@ test_atom head push:strip=1 remotes/myfork/main
>  test_atom head push:strip=-1 main
>  test_atom head objecttype commit
>  test_atom head objectsize $((131 + hexlen))
> -test_atom head objectsize:disk $disklen
> +test_atom head objectsize:disk $(test_object_file_size refs/heads/main)

These test_object_file_size calls are happening outside of any test_expect_* block, so we'd miss failing exit codes (and also the helper is not &&-chained), and any stderr would leak to the output. That's probably OK in practice, though (if something goes wrong then the expected value output will be bogus and the test itself will fail).

-Peff