{"thread":{"id":"60605","subject":"Test breakage with zlib-ng","startedAt":"2023-12-12T14:16:40Z","lastAt":"2023-12-24T09:30:50Z","messageCount":17,"participants":["Ondrej Pohorelsky","René Scharfe","Jeff King","brian m. carlson","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"485577","messageId":"CA+B51BEpSh1wT627Efpysw3evVocpiDCoQ3Xaza6jKE3B62yig@mail.gmail.com","threadId":"60605","inReplyTo":null,"subject":"Test breakage with zlib-ng","fromName":"Ondrej Pohorelsky","fromEmail":"opohorel@redhat.com","sentAt":"2023-12-12T14:16:26Z","receivedAt":"2023-12-12T14:16:40Z","isPatch":false,"sender":{"key":"opohorel@redhat.com","avatar":"https://avatars.githubusercontent.com/u/35430604?v=4"},"body":"Hi everyone,\n\nAs some might have heard, there is a proposal for Fedora 40 to\ntransition from zlib to zlib-ng[0]. Because of this, there has been a\nrebuild of all packages to ensure every package works under zlib-ng.\n\nGit test suit has three breakages in t6300-for-each-ref.sh.\nTo be precise, it is:\n\nnot ok 35 - basic atom: refs/heads/main objectsize:disk\nnot ok 107 - basic atom: refs/tags/testtag objectsize:disk\nnot ok 108 - basic atom: refs/tags/testtag *objectsize:disk\n\n\nAll of these tests are atomic, and they compare the result against\n$disklen. I discussed it with Tulio Magno Quites Machado Filho, who\nran the tests and is owner of the proposal.\nIt seems like the compression of zlib-ng is shaving/adding some bytes\nto the actual output, which then fails the comparison.\n\nHere is an example:\n\n```\nexpecting success of 6300.35 'basic atom: refs/heads/main objectsize:disk':\ngit for-each-ref --format=\"%($format)\" \"$ref\" >actual &&\nsanitize_pgp <actual >actual.clean &&\ntest_cmp expected actual.clean\n\n\n++ git for-each-ref '--format=%(objectsize:disk)' refs/heads/main\n++ sanitize_pgp\n++ perl -ne '\n/^-----END PGP/ and $in_pgp = 0;\nprint unless $in_pgp;\n/^-----BEGIN PGP/ and $in_pgp = 1;\n'\n++ command /usr/bin/perl -ne '\n/^-----END PGP/ and $in_pgp = 0;\nprint unless $in_pgp;\n/^-----BEGIN PGP/ and $in_pgp = 1;\n'\n++ test_cmp expected actual.clean\n++ test 2 -ne 2\n++ eval 'diff -u' '\"$@\"'\n+++ diff -u expected actual.clean\n--- expected 2023-12-06 21:06:07.808849497 +0000\n+++ actual.clean 2023-12-06 21:06:07.812849541 +0000\n@@ -1 +1 @@\n-138\n+137\nerror: last command exited with $?=1\nnot ok 35 - basic atom: refs/heads/main objectsize:disk\n```\n\nThe whole build log can be found here[1].\n\nI can easily patch these tests in Fedora to be compatible with zlib-ng\nonly by not comparing to $disklen, but a concrete value, however I\nwould like to have a universal solution that works with both zlib and\nzlib-ng. So if anyone has an idea on how to do it, please let me know.\nThanks\n\n\n[0]https://discussion.fedoraproject.org/t/f40-change-proposal-transitioning-to-zlib-ng-as-a-compatible-replacement-for-zlib-system-wide/95807\n[1]https://download.copr.fedorainfracloud.org/results/tuliom/zlib-ng-compat-mpb/fedora-rawhide-x86_64/06729801-git/builder-live.log.gz\n\nCheers,\nOndřej Pohořelský\n\n"},{"id":"485583","messageId":"9feeb6cf-aabf-4002-917f-3f6c27547bc8@web.de","threadId":"60605","inReplyTo":"CA+B51BEpSh1wT627Efpysw3evVocpiDCoQ3Xaza6jKE3B62yig@mail.gmail.com","subject":"Re: Test breakage with zlib-ng","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-12T17:04:55Z","receivedAt":"2023-12-12T17:05:22Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.12.23 um 15:16 schrieb Ondrej Pohorelsky:\n> Hi everyone,\n>\n> As some might have heard, there is a proposal for Fedora 40 to\n> transition from zlib to zlib-ng[0]. Because of this, there has been a\n> rebuild of all packages to ensure every package works under zlib-ng.\n>\n> Git test suit has three breakages in t6300-for-each-ref.sh.\n> To be precise, it is:\n>\n> not ok 35 - basic atom: refs/heads/main objectsize:disk\n> not ok 107 - basic atom: refs/tags/testtag objectsize:disk\n> not ok 108 - basic atom: refs/tags/testtag *objectsize:disk\n>\n>\n> All of these tests are atomic, and they compare the result against\n> $disklen.\n\nWhy do these three objects (HEAD commit of main, testtag and testtag\ntarget) have the same size?  Half of the answer is that testtag points\nto the HEAD of main.  But the other half is pure coincidence as far as I\ncan see.\n\n> I can easily patch these tests in Fedora to be compatible with zlib-ng\n> only by not comparing to $disklen, but a concrete value, however I\n> would like to have a universal solution that works with both zlib and\n> zlib-ng. So if anyone has an idea on how to do it, please let me know.\n\nThe test stores the expected values at the top, in the following lines,\nfor the two possible repository formats:\n\n\ttest_expect_success setup '\n\t\ttest_oid_cache <<-EOF &&\n\t\tdisklen sha1:138\n\t\tdisklen sha256:154\n\t\tEOF\n\nSo it's using hard-coded values already, which breaks when the\ncompression rate changes.\n\nWe could set core.compression to 0 to take compression out of the\npicture.\n\nOr we could get the sizes of the objects by checking their files,\nwhich would not require  hard-coding anymore.  Patch below.\n\n--- >8 ---\nSubject: [PATCH] t6300: avoid hard-coding object sizes\n\nf4ee22b526 (ref-filter: add tests for objectsize:disk, 2018-12-24)\nhard-coded the expected object sizes.  Coincidentally the size of commit\nand tag is the same with zlib at the default compression level.\n\n1f5f8f3e85 (t6300: abstract away SHA-1-specific constants, 2020-02-22)\nencoded the sizes as a single value, which coincidentally also works\nwith sha256.\n\nDifferent compression libraries like zlib-ng may arrive at different\nvalues.  Get them from the file system instead of hard-coding them to\nmake switching the compression library (or changing the compression\nlevel) easier.\n\nReported-by: Ondrej Pohorelsky <opohorel@redhat.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/t6300-for-each-ref.sh | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 54e2281259..843a7fe143 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -20,12 +20,13 @@ setdate_and_increment () {\n     export GIT_COMMITTER_DATE GIT_AUTHOR_DATE\n }\n\n-test_expect_success setup '\n-\ttest_oid_cache <<-EOF &&\n-\tdisklen sha1:138\n-\tdisklen sha256:154\n-\tEOF\n+test_object_file_size () {\n+\toid=$(git rev-parse \"$1\")\n+\tpath=\".git/objects/$(test_oid_to_path $oid)\"\n+\ttest_file_size \"$path\"\n+}\n\n+test_expect_success setup '\n \t# setup .mailmap\n \tcat >.mailmap <<-EOF &&\n \tA Thor <athor@example.com> A U Thor <author@example.com>\n@@ -94,7 +95,6 @@ test_atom () {\n }\n\n hexlen=$(test_oid hexsz)\n-disklen=$(test_oid disklen)\n\n test_atom head refname refs/heads/main\n test_atom head refname: refs/heads/main\n@@ -129,7 +129,7 @@ test_atom head push:strip=1 remotes/myfork/main\n test_atom head push:strip=-1 main\n test_atom head objecttype commit\n test_atom head objectsize $((131 + hexlen))\n-test_atom head objectsize:disk $disklen\n+test_atom head objectsize:disk $(test_object_file_size refs/heads/main)\n test_atom head deltabase $ZERO_OID\n test_atom head objectname $(git rev-parse refs/heads/main)\n test_atom head objectname:short $(git rev-parse --short refs/heads/main)\n@@ -203,8 +203,8 @@ test_atom tag upstream ''\n test_atom tag push ''\n test_atom tag objecttype tag\n test_atom tag objectsize $((114 + hexlen))\n-test_atom tag objectsize:disk $disklen\n-test_atom tag '*objectsize:disk' $disklen\n+test_atom tag objectsize:disk $(test_object_file_size refs/tags/testtag)\n+test_atom tag '*objectsize:disk' $(test_object_file_size refs/heads/main)\n test_atom tag deltabase $ZERO_OID\n test_atom tag '*deltabase' $ZERO_OID\n test_atom tag objectname $(git rev-parse refs/tags/testtag)\n--\n2.43.0\n\n"},{"id":"485589","messageId":"20231212200153.GB1127366@coredump.intra.peff.net","threadId":"60605","inReplyTo":"9feeb6cf-aabf-4002-917f-3f6c27547bc8@web.de","subject":"Re: Test breakage with zlib-ng","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-12T20:01:53Z","receivedAt":"2023-12-12T20:01:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 12, 2023 at 06:04:55PM +0100, René Scharfe wrote:\n\n> Subject: [PATCH] t6300: avoid hard-coding object sizes\n> \n> f4ee22b526 (ref-filter: add tests for objectsize:disk, 2018-12-24)\n> hard-coded the expected object sizes.  Coincidentally the size of commit\n> and tag is the same with zlib at the default compression level.\n> \n> 1f5f8f3e85 (t6300: abstract away SHA-1-specific constants, 2020-02-22)\n> encoded the sizes as a single value, which coincidentally also works\n> with sha256.\n> \n> Different compression libraries like zlib-ng may arrive at different\n> values.  Get them from the file system instead of hard-coding them to\n> make switching the compression library (or changing the compression\n> level) easier.\n\nYeah, this is definitely the right solution here. I'm surprised the\nhard-coded values didn't cause problems before now. ;)\n\nThe patch looks good to me, but a few small comments:\n\n> +test_object_file_size () {\n> +\toid=$(git rev-parse \"$1\")\n> +\tpath=\".git/objects/$(test_oid_to_path $oid)\"\n> +\ttest_file_size \"$path\"\n> +}\n\nHere we're assuming the objects are loose. I think that's probably OK\n(and certainly the test will notice if that changes).\n\nWe're covering the formatting code paths along with the underlying\nimplementation that fills in object_info->disk_sizep for loose objects.\nWhich I think is plenty for this particular script, which is about\nfor-each-ref.\n\nIt would be nice to have coverage of the packed_object_info() code path,\nthough. Back when it was added in a4ac106178 (cat-file: add\n%(objectsize:disk) format atom, 2013-07-10), I cowardly punted on this,\nwriting:\n\n  This patch does not include any tests, as the exact numbers\n  returned are volatile and subject to zlib and packing\n  decisions. We cannot even reliably guarantee that the\n  on-disk size is smaller than the object content (though in\n  general this should be the case for non-trivial objects).\n\nI don't think it's that big a deal, but I guess we could do something\nlike:\n\n  prev=\n  git show-index <$pack_idx |\n  sort -n |\n  grep -A1 $oid |\n  while read ofs oid csum\n  do\n    test -n \"$prev\" && echo \"$((ofs - prev))\"\n    prev=$ofs\n  done\n\nIt feels a little redundant with what Git is doing under the hood, but\nat least is exercising the code (and we're using the idx directly, so\nwe're confirming that the revindex is right).\n\nAnyway, that is all way beyond the scope of your patch, but I wonder if\nit's worth doing on top.\n\n> @@ -129,7 +129,7 @@ test_atom head push:strip=1 remotes/myfork/main\n>  test_atom head push:strip=-1 main\n>  test_atom head objecttype commit\n>  test_atom head objectsize $((131 + hexlen))\n> -test_atom head objectsize:disk $disklen\n> +test_atom head objectsize:disk $(test_object_file_size refs/heads/main)\n\nThese test_object_file_size calls are happening outside of any\ntest_expect_* block, so we'd miss failing exit codes (and also the\nhelper is not &&-chained), and any stderr would leak to the output.\nThat's probably OK in practice, though (if something goes wrong then the\nexpected value output will be bogus and the test itself will fail).\n\n-Peff\n"},{"id":"485595","messageId":"ZXjcOtQ8s60X8FEQ@tapette.crustytoothpaste.net","threadId":"60605","inReplyTo":"9feeb6cf-aabf-4002-917f-3f6c27547bc8@web.de","subject":"Re: Test breakage with zlib-ng","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-12-12T22:18:34Z","receivedAt":"2023-12-12T22:18:38Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-12-12 at 17:04:55, René Scharfe wrote:\n> --- >8 ---\n> Subject: [PATCH] t6300: avoid hard-coding object sizes\n> \n> f4ee22b526 (ref-filter: add tests for objectsize:disk, 2018-12-24)\n> hard-coded the expected object sizes.  Coincidentally the size of commit\n> and tag is the same with zlib at the default compression level.\n> \n> 1f5f8f3e85 (t6300: abstract away SHA-1-specific constants, 2020-02-22)\n> encoded the sizes as a single value, which coincidentally also works\n> with sha256.\n> \n> Different compression libraries like zlib-ng may arrive at different\n> values.  Get them from the file system instead of hard-coding them to\n> make switching the compression library (or changing the compression\n> level) easier.\n\nThis definitely seems like the right thing to do.  I was a bit lazy at\nthe time and probably could have improved it then, but it's at least\ngood that we're doing it now.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"485597","messageId":"xmqqv893oxyp.fsf@gitster.g","threadId":"60605","inReplyTo":"9feeb6cf-aabf-4002-917f-3f6c27547bc8@web.de","subject":"Re: Test breakage with zlib-ng","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-12T22:30:38Z","receivedAt":"2023-12-12T22:30:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Or we could get the sizes of the objects by checking their files,\n> which would not require  hard-coding anymore.  Patch below.\n\nThat was my first reaction to seeing the original report.  It is a\nbit surprising that the necessary fix is so small and makes me wonder\nwhy we initially did the hardcoded values, which feels more work to\ninitially write the test vector.\n\nLooking good.  Thanks.\n\n> --- >8 ---\n> Subject: [PATCH] t6300: avoid hard-coding object sizes\n>\n> f4ee22b526 (ref-filter: add tests for objectsize:disk, 2018-12-24)\n> hard-coded the expected object sizes.  Coincidentally the size of commit\n> and tag is the same with zlib at the default compression level.\n>\n> 1f5f8f3e85 (t6300: abstract away SHA-1-specific constants, 2020-02-22)\n> encoded the sizes as a single value, which coincidentally also works\n> with sha256.\n>\n> Different compression libraries like zlib-ng may arrive at different\n> values.  Get them from the file system instead of hard-coding them to\n> make switching the compression library (or changing the compression\n> level) easier.\n>\n> Reported-by: Ondrej Pohorelsky <opohorel@redhat.com>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  t/t6300-for-each-ref.sh | 18 +++++++++---------\n>  1 file changed, 9 insertions(+), 9 deletions(-)\n>\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index 54e2281259..843a7fe143 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -20,12 +20,13 @@ setdate_and_increment () {\n>      export GIT_COMMITTER_DATE GIT_AUTHOR_DATE\n>  }\n>\n> -test_expect_success setup '\n> -\ttest_oid_cache <<-EOF &&\n> -\tdisklen sha1:138\n> -\tdisklen sha256:154\n> -\tEOF\n> +test_object_file_size () {\n> +\toid=$(git rev-parse \"$1\")\n> +\tpath=\".git/objects/$(test_oid_to_path $oid)\"\n> +\ttest_file_size \"$path\"\n> +}\n>\n> +test_expect_success setup '\n>  \t# setup .mailmap\n>  \tcat >.mailmap <<-EOF &&\n>  \tA Thor <athor@example.com> A U Thor <author@example.com>\n> @@ -94,7 +95,6 @@ test_atom () {\n>  }\n>\n>  hexlen=$(test_oid hexsz)\n> -disklen=$(test_oid disklen)\n>\n>  test_atom head refname refs/heads/main\n>  test_atom head refname: refs/heads/main\n> @@ -129,7 +129,7 @@ test_atom head push:strip=1 remotes/myfork/main\n>  test_atom head push:strip=-1 main\n>  test_atom head objecttype commit\n>  test_atom head objectsize $((131 + hexlen))\n> -test_atom head objectsize:disk $disklen\n> +test_atom head objectsize:disk $(test_object_file_size refs/heads/main)\n>  test_atom head deltabase $ZERO_OID\n>  test_atom head objectname $(git rev-parse refs/heads/main)\n>  test_atom head objectname:short $(git rev-parse --short refs/heads/main)\n> @@ -203,8 +203,8 @@ test_atom tag upstream ''\n>  test_atom tag push ''\n>  test_atom tag objecttype tag\n>  test_atom tag objectsize $((114 + hexlen))\n> -test_atom tag objectsize:disk $disklen\n> -test_atom tag '*objectsize:disk' $disklen\n> +test_atom tag objectsize:disk $(test_object_file_size refs/tags/testtag)\n> +test_atom tag '*objectsize:disk' $(test_object_file_size refs/heads/main)\n>  test_atom tag deltabase $ZERO_OID\n>  test_atom tag '*deltabase' $ZERO_OID\n>  test_atom tag objectname $(git rev-parse refs/tags/testtag)\n> --\n> 2.43.0\n"},{"id":"485599","messageId":"ff735aac-b60b-4d52-a6dc-180ab504fc8d@web.de","threadId":"60605","inReplyTo":"20231212200153.GB1127366@coredump.intra.peff.net","subject":"Re: Test breakage with zlib-ng","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-12T22:54:34Z","receivedAt":"2023-12-12T22:54:51Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.12.23 um 21:01 schrieb Jeff King:\n> On Tue, Dec 12, 2023 at 06:04:55PM +0100, René Scharfe wrote:\n>\n>> Subject: [PATCH] t6300: avoid hard-coding object sizes\n>>\n>> f4ee22b526 (ref-filter: add tests for objectsize:disk, 2018-12-24)\n>> hard-coded the expected object sizes.  Coincidentally the size of commit\n>> and tag is the same with zlib at the default compression level.\n>>\n>> 1f5f8f3e85 (t6300: abstract away SHA-1-specific constants, 2020-02-22)\n>> encoded the sizes as a single value, which coincidentally also works\n>> with sha256.\n>>\n>> Different compression libraries like zlib-ng may arrive at different\n>> values.  Get them from the file system instead of hard-coding them to\n>> make switching the compression library (or changing the compression\n>> level) easier.\n>\n> Yeah, this is definitely the right solution here. I'm surprised the\n> hard-coded values didn't cause problems before now. ;)\n>\n> The patch looks good to me, but a few small comments:\n>\n>> +test_object_file_size () {\n>> +\toid=$(git rev-parse \"$1\")\n>> +\tpath=\".git/objects/$(test_oid_to_path $oid)\"\n>> +\ttest_file_size \"$path\"\n>> +}\n>\n> Here we're assuming the objects are loose. I think that's probably OK\n> (and certainly the test will notice if that changes).\n>\n> We're covering the formatting code paths along with the underlying\n> implementation that fills in object_info->disk_sizep for loose objects.\n> Which I think is plenty for this particular script, which is about\n> for-each-ref.\n>\n> It would be nice to have coverage of the packed_object_info() code path,\n> though. Back when it was added in a4ac106178 (cat-file: add\n> %(objectsize:disk) format atom, 2013-07-10), I cowardly punted on this,\n> writing:\n>\n>   This patch does not include any tests, as the exact numbers\n>   returned are volatile and subject to zlib and packing\n>   decisions. We cannot even reliably guarantee that the\n>   on-disk size is smaller than the object content (though in\n>   general this should be the case for non-trivial objects).\n>\n> I don't think it's that big a deal, but I guess we could do something\n> like:\n>\n>   prev=\n>   git show-index <$pack_idx |\n>   sort -n |\n>   grep -A1 $oid |\n>   while read ofs oid csum\n>   do\n>     test -n \"$prev\" && echo \"$((ofs - prev))\"\n>     prev=$ofs\n>   done\n>\n> It feels a little redundant with what Git is doing under the hood, but\n> at least is exercising the code (and we're using the idx directly, so\n> we're confirming that the revindex is right).\n\nA generic object size function based on both methods could live in the\ntest lib and be used for e.g. cat-file tests as well.  Getting such a\nfunction polished and library-worthy is probably more work than I\nnaively imagine, however -- due to our shunning of pipes alone.\n\n> Anyway, that is all way beyond the scope of your patch, but I wonder if\n> it's worth doing on top.\n>\n>> @@ -129,7 +129,7 @@ test_atom head push:strip=1 remotes/myfork/main\n>>  test_atom head push:strip=-1 main\n>>  test_atom head objecttype commit\n>>  test_atom head objectsize $((131 + hexlen))\n>> -test_atom head objectsize:disk $disklen\n>> +test_atom head objectsize:disk $(test_object_file_size refs/heads/main)\n>\n> These test_object_file_size calls are happening outside of any\n> test_expect_* block, so we'd miss failing exit codes (and also the\n> helper is not &&-chained), and any stderr would leak to the output.\n> That's probably OK in practice, though (if something goes wrong then the\n> expected value output will be bogus and the test itself will fail).\n\nRight.  We could also set variables during setup, though, to put\nreaders' minds at rest.\n\nRené\n"},{"id":"485613","messageId":"65557f2d-9de0-49ae-a858-80476aa52b68@web.de","threadId":"60605","inReplyTo":"ff735aac-b60b-4d52-a6dc-180ab504fc8d@web.de","subject":"[PATCH 2/1] test-lib-functions: add object size functions","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-13T12:28:56Z","receivedAt":"2023-12-13T12:29:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add test_object_size and its helpers test_loose_object_size and\ntest_packed_object_size, which allow determining the size of a Git\nobject using only the low-level Git commands rev-parse and show-index.\n\nUse it in t6300 to replace the bare-bones function test_object_file_size\nas a motivating example.  There it provides the expected output of the\nhigh-level Git command for-each-ref.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nSo how about this?  I'm a bit nervous about all the rules about output\ndescriptors and error propagation and whatnot in the test library, but\nthis implementation seems simple enough and might be useful in more than\none test.  No idea how to add support for alternate object directories,\nbut I doubt we'll ever need it.\n---\n t/t6300-for-each-ref.sh | 16 ++++++--------\n t/test-lib-functions.sh | 47 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 843a7fe143..4687660f38 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -20,12 +20,6 @@ setdate_and_increment () {\n     export GIT_COMMITTER_DATE GIT_AUTHOR_DATE\n }\n\n-test_object_file_size () {\n-\toid=$(git rev-parse \"$1\")\n-\tpath=\".git/objects/$(test_oid_to_path $oid)\"\n-\ttest_file_size \"$path\"\n-}\n-\n test_expect_success setup '\n \t# setup .mailmap\n \tcat >.mailmap <<-EOF &&\n@@ -40,7 +34,11 @@ test_expect_success setup '\n \tgit branch -M main &&\n \tsetdate_and_increment &&\n \tgit tag -a -m \"Tagging at $datestamp\" testtag &&\n+\ttesttag_oid=$(git rev-parse refs/tags/testtag) &&\n+\ttesttag_disksize=$(test_object_size $testtag_oid) &&\n \tgit update-ref refs/remotes/origin/main main &&\n+\tcommit_oid=$(git rev-parse refs/heads/main) &&\n+\tcommit_disksize=$(test_object_size $commit_oid) &&\n \tgit remote add origin nowhere &&\n \tgit config branch.main.remote origin &&\n \tgit config branch.main.merge refs/heads/main &&\n@@ -129,7 +127,7 @@ test_atom head push:strip=1 remotes/myfork/main\n test_atom head push:strip=-1 main\n test_atom head objecttype commit\n test_atom head objectsize $((131 + hexlen))\n-test_atom head objectsize:disk $(test_object_file_size refs/heads/main)\n+test_atom head objectsize:disk $commit_disksize\n test_atom head deltabase $ZERO_OID\n test_atom head objectname $(git rev-parse refs/heads/main)\n test_atom head objectname:short $(git rev-parse --short refs/heads/main)\n@@ -203,8 +201,8 @@ test_atom tag upstream ''\n test_atom tag push ''\n test_atom tag objecttype tag\n test_atom tag objectsize $((114 + hexlen))\n-test_atom tag objectsize:disk $(test_object_file_size refs/tags/testtag)\n-test_atom tag '*objectsize:disk' $(test_object_file_size refs/heads/main)\n+test_atom tag objectsize:disk $testtag_disksize\n+test_atom tag '*objectsize:disk' $commit_disksize\n test_atom tag deltabase $ZERO_OID\n test_atom tag '*deltabase' $ZERO_OID\n test_atom tag objectname $(git rev-parse refs/tags/testtag)\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 9c3cf12b26..9b49645f77 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1733,6 +1733,53 @@ test_oid_to_path () {\n \techo \"${1%$basename}/$basename\"\n }\n\n+test_loose_object_size () {\n+\ttest \"$#\" -ne 1 && BUG \"1 param\"\n+\tlocal path=$(test_oid_to_path \"$1\")\n+\ttest_file_size \"$(git rev-parse --git-path \"objects/$path\")\" 2>&4\n+}\n+\n+test_packed_object_size () {\n+\ttest \"$#\" -ne 2 && BUG \"2 params\"\n+\tlocal oid=$1 idx=$2 packsize rawsz end\n+\n+\tpacksize=$(test_file_size \"${idx%.idx}.pack\")\n+\trawsz=$(test_oid rawsz)\n+\tend=$(($packsize - $rawsz))\n+\n+\tgit show-index <\"$idx\" |\n+\tawk -v oid=\"$oid\" -v end=\"$end\" '\n+\t\t$2 == oid {start = $1}\n+\t\t{offsets[$1] = 1}\n+\t\tEND {\n+\t\t\tif (!start || start >= end)\n+\t\t\t\texit 1\n+\t\t\tfor (o in offsets)\n+\t\t\t\tif (start < o && o < end)\n+\t\t\t\t\tend = o\n+\t\t\tprint end - start\n+\t\t}\n+\t' && return 0\n+\n+\techo >&4 \"error: '$oid' not found in '$idx'\"\n+\treturn 1\n+}\n+\n+test_object_size () {\n+\ttest \"$#\" -ne 1 && BUG \"1 param\"\n+\tlocal oid=$1\n+\n+\ttest_loose_object_size \"$oid\" 4>/dev/null && return 0\n+\n+\tfor idx in \"$(git rev-parse --git-path objects/pack)\"/pack-*.idx\n+\tdo\n+\t\ttest_packed_object_size \"$oid\" \"$idx\" 4>/dev/null && return 0\n+\tdone\n+\n+\techo >&4 \"error: '$oid' not found\"\n+\treturn 1\n+}\n+\n # Parse oids from git ls-files --staged output\n test_parse_ls_files_stage_oids () {\n \tawk '{print $2}' -\n--\n2.43.0\n"},{"id":"485664","messageId":"20231214205936.GA2272813@coredump.intra.peff.net","threadId":"60605","inReplyTo":"65557f2d-9de0-49ae-a858-80476aa52b68@web.de","subject":"Re: [PATCH 2/1] test-lib-functions: add object size functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-14T20:59:36Z","receivedAt":"2023-12-14T21:06:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 13, 2023 at 01:28:56PM +0100, René Scharfe wrote:\n\n> Add test_object_size and its helpers test_loose_object_size and\n> test_packed_object_size, which allow determining the size of a Git\n> object using only the low-level Git commands rev-parse and show-index.\n> \n> Use it in t6300 to replace the bare-bones function test_object_file_size\n> as a motivating example.  There it provides the expected output of the\n> high-level Git command for-each-ref.\n\nThis adds a packed-object function, but I doubt anybody actually calls\nit. If we're going to do that, it's probably worth adding some tests for\n\"cat-file --batch-check\" or similar.\n\nAt which point I wonder if rather than having a function for a single\nobject, we are better off just testing the result of:\n\n  git cat-file --batch-all-objects --unordered --batch-check='%(objectsize:disk)'\n\nagainst a single post-processed \"show-index\" invocation.\n\n> So how about this?  I'm a bit nervous about all the rules about output\n> descriptors and error propagation and whatnot in the test library, but\n> this implementation seems simple enough and might be useful in more than\n> one test.  No idea how to add support for alternate object directories,\n> but I doubt we'll ever need it.\n\nI'm not sure that we need to do anything special with output\nredirection. Shouldn't these functions just send errors to stderr as\nusual? If they are run inside a test_expect block, that goes to\ndescriptor 4 (which is either /dev/null or the original stderr,\ndepending on whether \"-v\" was used).\n\n> +test_loose_object_size () {\n> +\ttest \"$#\" -ne 1 && BUG \"1 param\"\n> +\tlocal path=$(test_oid_to_path \"$1\")\n> +\ttest_file_size \"$(git rev-parse --git-path \"objects/$path\")\" 2>&4\n> +}\n\nOK. We lose the exit code from \"rev-parse\" but that is probably OK for\nour purposes.\n\n> +test_packed_object_size () {\n> +\ttest \"$#\" -ne 2 && BUG \"2 params\"\n> +\tlocal oid=$1 idx=$2 packsize rawsz end\n> +\n> +\tpacksize=$(test_file_size \"${idx%.idx}.pack\")\n> +\trawsz=$(test_oid rawsz)\n> +\tend=$(($packsize - $rawsz))\n\nOK, this $end is the magic required for the final entry. Makes sense.\n\n> +\tgit show-index <\"$idx\" |\n> +\tawk -v oid=\"$oid\" -v end=\"$end\" '\n> +\t\t$2 == oid {start = $1}\n> +\t\t{offsets[$1] = 1}\n> +\t\tEND {\n> +\t\t\tif (!start || start >= end)\n> +\t\t\t\texit 1\n> +\t\t\tfor (o in offsets)\n> +\t\t\t\tif (start < o && o < end)\n> +\t\t\t\t\tend = o\n> +\t\t\tprint end - start\n> +\t\t}\n> +\t' && return 0\n\nI was confused at first, because I didn't see any sorting happening. But\nif I understand correctly, you're just looking for the smallest \"end\"\nthat comes after the start of the object we're looking for. Which I\nthink works.\n\n-Peff\n"},{"id":"485818","messageId":"6750c93c-78d0-46b5-bfc2-0774156ed2ed@web.de","threadId":"60605","inReplyTo":"20231214205936.GA2272813@coredump.intra.peff.net","subject":"Re: [PATCH 2/1] test-lib-functions: add object size functions","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-19T16:42:39Z","receivedAt":"2023-12-19T16:43:02Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.12.23 um 21:59 schrieb Jeff King:\n> On Wed, Dec 13, 2023 at 01:28:56PM +0100, René Scharfe wrote:\n>\n>> Add test_object_size and its helpers test_loose_object_size and\n>> test_packed_object_size, which allow determining the size of a Git\n>> object using only the low-level Git commands rev-parse and show-index.\n>>\n>> Use it in t6300 to replace the bare-bones function test_object_file_size\n>> as a motivating example.  There it provides the expected output of the\n>> high-level Git command for-each-ref.\n>\n> This adds a packed-object function, but I doubt anybody actually calls\n> it. If we're going to do that, it's probably worth adding some tests for\n> \"cat-file --batch-check\" or similar.\n\nYes, and I was assuming that someone else would be eager to add such\ntests. *ahem*\n\n> At which point I wonder if rather than having a function for a single\n> object, we are better off just testing the result of:\n>\n>   git cat-file --batch-all-objects --unordered --batch-check='%(objectsize:disk)'\n>\n> against a single post-processed \"show-index\" invocation.\n\nSure, we might want to optimize for bulk-processing and possibly end up\nonly checking the size of single objects in t6300, making new library\nfunctions unnecessary.\n\nWhen dumping size information of multiple objects, it's probably a good\nidea to include \"%(objectname)\" as well in the format.\n\nYou'd need one show-index call for each .idx file.  A simple test would\nonly have a single one; a library function might need to handle multiple\npacks.\n\n>> So how about this?  I'm a bit nervous about all the rules about output\n>> descriptors and error propagation and whatnot in the test library, but\n>> this implementation seems simple enough and might be useful in more than\n>> one test.  No idea how to add support for alternate object directories,\n>> but I doubt we'll ever need it.\n>\n> I'm not sure that we need to do anything special with output\n> redirection. Shouldn't these functions just send errors to stderr as\n> usual? If they are run inside a test_expect block, that goes to\n> descriptor 4 (which is either /dev/null or the original stderr,\n> depending on whether \"-v\" was used).\n\nGood point.  My bad excuse is that I copied the redirection to fd 4 from\ntest_grep.\n\n>> +\tgit show-index <\"$idx\" |\n>> +\tawk -v oid=\"$oid\" -v end=\"$end\" '\n>> +\t\t$2 == oid {start = $1}\n>> +\t\t{offsets[$1] = 1}\n>> +\t\tEND {\n>> +\t\t\tif (!start || start >= end)\n>> +\t\t\t\texit 1\n>> +\t\t\tfor (o in offsets)\n>> +\t\t\t\tif (start < o && o < end)\n>> +\t\t\t\t\tend = o\n>> +\t\t\tprint end - start\n>> +\t\t}\n>> +\t' && return 0\n>\n> I was confused at first, because I didn't see any sorting happening. But\n> if I understand correctly, you're just looking for the smallest \"end\"\n> that comes after the start of the object we're looking for. Which I\n> think works.\n\nYes, calculating the minimum offset suffices when handling a single\nobject -- no sorting required.  For bulk mode we'd better sort, of\ncourse:\n\n\tgit show-index <\"$idx\" |\n\tsort -n |\n\tawk -v end=\"$end\" '\n\t\tNR > 1 {print oid, $1 - start}\n\t\t{start = $1; oid = $2}\n\t\tEND {print oid, end - start}\n\t'\n\nNo idea how to make such a thing robust against malformed or truncated\noutput from show-index, but perhaps that's not necessary, depending on\nhow the result is used.\n\nRené\n\n"},{"id":"485912","messageId":"20231221094722.GA570888@coredump.intra.peff.net","threadId":"60605","inReplyTo":"6750c93c-78d0-46b5-bfc2-0774156ed2ed@web.de","subject":"[PATCH] t1006: add tests for %(objectsize:disk)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-21T09:47:22Z","receivedAt":"2023-12-21T09:47:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 19, 2023 at 05:42:39PM +0100, René Scharfe wrote:\n\n> > This adds a packed-object function, but I doubt anybody actually calls\n> > it. If we're going to do that, it's probably worth adding some tests for\n> > \"cat-file --batch-check\" or similar.\n> \n> Yes, and I was assuming that someone else would be eager to add such\n> tests. *ahem*\n\n:P OK, here it is. This can be its own topic, or go on top of the\nrs/t6300-compressed-size-fix branch.\n\n> > At which point I wonder if rather than having a function for a single\n> > object, we are better off just testing the result of:\n> >\n> >   git cat-file --batch-all-objects --unordered --batch-check='%(objectsize:disk)'\n> >\n> > against a single post-processed \"show-index\" invocation.\n> \n> Sure, we might want to optimize for bulk-processing and possibly end up\n> only checking the size of single objects in t6300, making new library\n> functions unnecessary.\n\nSo yeah, I think the approach here makes library functions unnecessary\n(and I see you already asked Junio to drop your patch 2).\n\n> When dumping size information of multiple objects, it's probably a good\n> idea to include \"%(objectname)\" as well in the format.\n\nYep, definitely.\n\n-- >8 --\nSubject: [PATCH] t1006: add tests for %(objectsize:disk)\n\nBack when we added this placeholder in a4ac106178 (cat-file: add\n%(objectsize:disk) format atom, 2013-07-10), there were no tests,\nclaiming \"[...]the exact numbers returned are volatile and subject to\nzlib and packing decisions\".\n\nBut we can use a little shell hackery to get the expected numbers\nourselves. To a certain degree this is just re-implementing what Git is\ndoing under the hood, but it is still worth doing. It makes sure we\nexercise the %(objectsize:disk) code at all, and having the two\nimplementations agree gives us more confidence.\n\nNote that our shell code assumes that no object appears twice (either in\ntwo packs, or as both loose and packed), as then the results really are\nundefined. That's OK for our purposes, and the test will notice if that\nassumption is violated (the shell version would produce duplicate lines\nthat Git's output does not have).\n\nHelped-by: René Scharfe <l.s.r@web.de>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI stole a bit of your awk. You can tell because I'd have written it in\nperl. ;)\n\n t/t1006-cat-file.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 271c5e4fd3..21915be308 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -1100,6 +1100,40 @@ test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will wor\n \tcmp expect actual\n '\n \n+test_expect_success 'cat-file %(objectsize:disk) with --batch-all-objects' '\n+\t# our state has both loose and packed objects,\n+\t# so find both for our expected output\n+\t{\n+\t\tfind .git/objects/?? -type f |\n+\t\tawk -F/ \"{ print \\$0, \\$3\\$4 }\" |\n+\t\twhile read path oid\n+\t\tdo\n+\t\t\tsize=$(test_file_size \"$path\") &&\n+\t\t\techo \"$oid $size\" ||\n+\t\t\treturn 1\n+\t\tdone &&\n+\t\trawsz=$(test_oid rawsz) &&\n+\t\tfind .git/objects/pack -name \"*.idx\" |\n+\t\twhile read idx\n+\t\tdo\n+\t\t\tgit show-index <\"$idx\" >idx.raw &&\n+\t\t\tsort -n <idx.raw >idx.sorted &&\n+\t\t\tpacksz=$(test_file_size \"${idx%.idx}.pack\") &&\n+\t\t\tend=$((packsz - rawsz)) &&\n+\t\t\tawk -v end=\"$end\" \"\n+\t\t\t  NR > 1 { print oid, \\$1 - start }\n+\t\t\t  { start = \\$1; oid = \\$2 }\n+\t\t\t  END { print oid, end - start }\n+\t\t\t\" idx.sorted ||\n+\t\t\treturn 1\n+\t\tdone\n+\t} >expect.raw &&\n+\tsort <expect.raw >expect &&\n+\tgit cat-file --batch-all-objects \\\n+\t\t--batch-check=\"%(objectname) %(objectsize:disk)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up replacement object' '\n \torig=$(git rev-parse HEAD) &&\n \tgit cat-file commit $orig >orig &&\n-- \n2.43.0.430.gaf21263e5d\n\n"},{"id":"485928","messageId":"d44cb8e7-ffce-4184-b9b5-6bb56705dcd1@web.de","threadId":"60605","inReplyTo":"20231221094722.GA570888@coredump.intra.peff.net","subject":"Re: [PATCH] t1006: add tests for %(objectsize:disk)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-21T12:19:53Z","receivedAt":"2023-12-21T12:20:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.12.23 um 10:47 schrieb Jeff King:\n> On Tue, Dec 19, 2023 at 05:42:39PM +0100, René Scharfe wrote:\n>\n>>> This adds a packed-object function, but I doubt anybody actually calls\n>>> it. If we're going to do that, it's probably worth adding some tests for\n>>> \"cat-file --batch-check\" or similar.\n>>\n>> Yes, and I was assuming that someone else would be eager to add such\n>> tests. *ahem*\n>\n> :P OK, here it is. This can be its own topic, or go on top of the\n> rs/t6300-compressed-size-fix branch.\n\nGreat, thank you!\n\n> -- >8 --\n> Subject: [PATCH] t1006: add tests for %(objectsize:disk)\n>\n> Back when we added this placeholder in a4ac106178 (cat-file: add\n> %(objectsize:disk) format atom, 2013-07-10), there were no tests,\n> claiming \"[...]the exact numbers returned are volatile and subject to\n> zlib and packing decisions\".\n>\n> But we can use a little shell hackery to get the expected numbers\n> ourselves. To a certain degree this is just re-implementing what Git is\n> doing under the hood, but it is still worth doing. It makes sure we\n> exercise the %(objectsize:disk) code at all, and having the two\n> implementations agree gives us more confidence.\n>\n> Note that our shell code assumes that no object appears twice (either in\n> two packs, or as both loose and packed), as then the results really are\n> undefined. That's OK for our purposes, and the test will notice if that\n> assumption is violated (the shell version would produce duplicate lines\n> that Git's output does not have).\n>\n> Helped-by: René Scharfe <l.s.r@web.de>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I stole a bit of your awk. You can tell because I'd have written it in\n> perl. ;)\n\nI think we can do it even in shell, especially if...\n\n>\n>  t/t1006-cat-file.sh | 34 ++++++++++++++++++++++++++++++++++\n>  1 file changed, 34 insertions(+)\n>\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 271c5e4fd3..21915be308 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -1100,6 +1100,40 @@ test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will wor\n>  \tcmp expect actual\n>  '\n>\n> +test_expect_success 'cat-file %(objectsize:disk) with --batch-all-objects' '\n> +\t# our state has both loose and packed objects,\n> +\t# so find both for our expected output\n> +\t{\n> +\t\tfind .git/objects/?? -type f |\n> +\t\tawk -F/ \"{ print \\$0, \\$3\\$4 }\" |\n> +\t\twhile read path oid\n> +\t\tdo\n> +\t\t\tsize=$(test_file_size \"$path\") &&\n> +\t\t\techo \"$oid $size\" ||\n> +\t\t\treturn 1\n> +\t\tdone &&\n> +\t\trawsz=$(test_oid rawsz) &&\n> +\t\tfind .git/objects/pack -name \"*.idx\" |\n> +\t\twhile read idx\n> +\t\tdo\n> +\t\t\tgit show-index <\"$idx\" >idx.raw &&\n> +\t\t\tsort -n <idx.raw >idx.sorted &&\n> +\t\t\tpacksz=$(test_file_size \"${idx%.idx}.pack\") &&\n> +\t\t\tend=$((packsz - rawsz)) &&\n> +\t\t\tawk -v end=\"$end\" \"\n> +\t\t\t  NR > 1 { print oid, \\$1 - start }\n> +\t\t\t  { start = \\$1; oid = \\$2 }\n> +\t\t\t  END { print oid, end - start }\n> +\t\t\t\" idx.sorted ||\n\n... we stop slicing the data against the grain.  Let's reverse the order\n(sort -r), then we don't need to carry the oid forward:\n\n\t\t\tsort -nr <idx.raw >idx.sorted &&\n\t\t\tpacksz=$(test_file_size \"${idx%.idx}.pack\") &&\n\t\t\tend=$((packsz - rawsz)) &&\n\t\t\tawk -v end=\"$end\" \"\n\t\t\t  { print \\$2, end - \\$1; end = \\$1 }\n\t\t\t\" idx.sorted ||\n\nAnd at that point it should be easy to use a shell loop instead of awk:\n\n\t\t\twhile read start oid rest\n\t\t\tdo\n\t\t\t\tsize=$((end - start)) &&\n\t\t\t\tend=$start &&\n\t\t\t\techo \"$oid $size\" ||\n\t\t\t\treturn 1\n\t\t\tdone <idx.sorted\n\n> +\t\t\treturn 1\n> +\t\tdone\n> +\t} >expect.raw &&\n> +\tsort <expect.raw >expect &&\n\nThe reversal above becomes irrelevant with that line, so the result in\nexpect stays the same.\n\nShould we deduplicate here, like cat-file does (i.e. use \"sort -u\")?\nHaving the same object in multiple places for whatever reason would not\nbe a cause for reporting an error in this test, I would think.\n\n> +\tgit cat-file --batch-all-objects \\\n> +\t\t--batch-check=\"%(objectname) %(objectsize:disk)\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'set up replacement object' '\n>  \torig=$(git rev-parse HEAD) &&\n>  \tgit cat-file commit $orig >orig &&\n\nOne more thing: We can do the work of the first awk invocation in the\nalready existing loop as well:\n\n> +test_expect_success 'cat-file %(objectsize:disk) with --batch-all-objects' '\n> +\t# our state has both loose and packed objects,\n> +\t# so find both for our expected output\n> +\t{\n> +\t\tfind .git/objects/?? -type f |\n> +\t\tawk -F/ \"{ print \\$0, \\$3\\$4 }\" |\n> +\t\twhile read path oid\n> +\t\tdo\n> +\t\t\tsize=$(test_file_size \"$path\") &&\n> +\t\t\techo \"$oid $size\" ||\n> +\t\t\treturn 1\n> +\t\tdone &&\n\n... but the substitutions are a bit awkward:\n\n\t\tfind .git/objects/?? -type f |\n\t\twhile read path\n\t\tdo\n\t\t\tbasename=${path##*/} &&\n\t\t\tdirname=${path%/$basename} &&\n\t\t\toid=\"${dirname#.git/objects/}${basename}\" &&\n\t\t\tsize=$(test_file_size \"$path\") &&\n\t\t\techo \"$oid $size\" ||\n\t\t\treturn 1\n\t\tdone &&\n\nThe avoided awk invocation might be worth the trouble, though.\n\nRené\n"},{"id":"485959","messageId":"20231221213034.GB1446091@coredump.intra.peff.net","threadId":"60605","inReplyTo":"d44cb8e7-ffce-4184-b9b5-6bb56705dcd1@web.de","subject":"Re: [PATCH] t1006: add tests for %(objectsize:disk)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-21T21:30:34Z","receivedAt":"2023-12-21T21:30:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 21, 2023 at 01:19:53PM +0100, René Scharfe wrote:\n\n> I think we can do it even in shell, especially if...\n> [...]\n\nYeah, your conversion looks accurate. I do wonder if it is worth golfing\nfurther, though. If it were a process invocation per object, I'd\ndefinitely say the efficiency gain is worth it. But dropping one process\nfrom the whole test isn't that exciting either way.\n\n> (sort -r), then we don't need to carry the oid forward:\n> \n> \t\t\tsort -nr <idx.raw >idx.sorted &&\n> \t\t\tpacksz=$(test_file_size \"${idx%.idx}.pack\") &&\n> \t\t\tend=$((packsz - rawsz)) &&\n> \t\t\tawk -v end=\"$end\" \"\n> \t\t\t  { print \\$2, end - \\$1; end = \\$1 }\n> \t\t\t\" idx.sorted ||\n> \n> And at that point it should be easy to use a shell loop instead of awk:\n> \n> \t\t\twhile read start oid rest\n> \t\t\tdo\n> \t\t\t\tsize=$((end - start)) &&\n> \t\t\t\tend=$start &&\n> \t\t\t\techo \"$oid $size\" ||\n> \t\t\t\treturn 1\n> \t\t\tdone <idx.sorted\n\nThe one thing I do like is that we don't have to escape anything inside\nan awk program that is forced to use double-quotes. ;)\n\n> Should we deduplicate here, like cat-file does (i.e. use \"sort -u\")?\n> Having the same object in multiple places for whatever reason would not\n> be a cause for reporting an error in this test, I would think.\n\nNo, for the reasons I said in the commit message: if an object exists in\nmultiple places the test is already potentially invalid, as Git does not\npromise which version it will use. So it might work racily, or it might\nwork for now but be fragile. By not de-duplicating, we make sure the\ntest's assumption holds.\n\n> One more thing: We can do the work of the first awk invocation in the\n> already existing loop as well:\n> [...]\n> ... but the substitutions are a bit awkward:\n> \n> \t\tfind .git/objects/?? -type f |\n> \t\twhile read path\n> \t\tdo\n> \t\t\tbasename=${path##*/} &&\n> \t\t\tdirname=${path%/$basename} &&\n> \t\t\toid=\"${dirname#.git/objects/}${basename}\" &&\n> \t\t\tsize=$(test_file_size \"$path\") &&\n> \t\t\techo \"$oid $size\" ||\n> \t\t\treturn 1\n> \t\tdone &&\n> \n> The avoided awk invocation might be worth the trouble, though.\n\nYeah, I briefly considered whether it would be possible in pure shell,\nbut didn't get very far before assuming it was going to be ugly. Thank\nyou for confirming. ;)\n\nAgain, if we were doing one awk per object, I'd try to avoid it. But\nsince we can cover all objects in a single pass, I think it's OK.\n\n-Peff\n"},{"id":"485968","messageId":"120b3194-5eee-47ed-b2d8-bc6731b71a6b@web.de","threadId":"60605","inReplyTo":"20231221213034.GB1446091@coredump.intra.peff.net","subject":"Re: [PATCH] t1006: add tests for %(objectsize:disk)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-21T23:13:10Z","receivedAt":"2023-12-21T23:13:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.12.23 um 22:30 schrieb Jeff King:\n> On Thu, Dec 21, 2023 at 01:19:53PM +0100, René Scharfe wrote:\n>\n>> I think we can do it even in shell, especially if...\n>> [...]\n>\n> Yeah, your conversion looks accurate. I do wonder if it is worth golfing\n> further, though. If it were a process invocation per object, I'd\n> definitely say the efficiency gain is worth it. But dropping one process\n> from the whole test isn't that exciting either way.\n\nFair enough.\n\n>\n>> (sort -r), then we don't need to carry the oid forward:\n>>\n>> \t\t\tsort -nr <idx.raw >idx.sorted &&\n>> \t\t\tpacksz=$(test_file_size \"${idx%.idx}.pack\") &&\n>> \t\t\tend=$((packsz - rawsz)) &&\n>> \t\t\tawk -v end=\"$end\" \"\n>> \t\t\t  { print \\$2, end - \\$1; end = \\$1 }\n>> \t\t\t\" idx.sorted ||\n>>\n>> And at that point it should be easy to use a shell loop instead of awk:\n>>\n>> \t\t\twhile read start oid rest\n>> \t\t\tdo\n>> \t\t\t\tsize=$((end - start)) &&\n>> \t\t\t\tend=$start &&\n>> \t\t\t\techo \"$oid $size\" ||\n>> \t\t\t\treturn 1\n>> \t\t\tdone <idx.sorted\n>\n> The one thing I do like is that we don't have to escape anything inside\n> an awk program that is forced to use double-quotes. ;)\n\nFor me it's processing the data in the \"correct\" order (descending, i.e.\nstarting at the end, which we have to calculate first anyway based on the\nsize).\n\n>> Should we deduplicate here, like cat-file does (i.e. use \"sort -u\")?\n>> Having the same object in multiple places for whatever reason would not\n>> be a cause for reporting an error in this test, I would think.\n>\n> No, for the reasons I said in the commit message: if an object exists in\n> multiple places the test is already potentially invalid, as Git does not\n> promise which version it will use. So it might work racily, or it might\n> work for now but be fragile. By not de-duplicating, we make sure the\n> test's assumption holds.\n\nOh, skipped that paragraph.  Still I don't see how a duplicate object\nwould necessarily invalidate t1006.  The comment for the test \"cat-file\n--batch-all-objects shows all objects\" a few lines above indicates that\nit's picky about the provenance of objects, but it uses a separate\nrepository.  I can't infer the same requirement for the root repo, but\nwe already established that I can't read.\n\nAnyway, if someone finds a use for git repack without -d or\ngit unpack-objects or whatever else causes duplicates in the root\nrepository of t1006 then they can try to reverse your ban with concrete\narguments.\n\nRené\n"},{"id":"485987","messageId":"20231223100905.GB2016274@coredump.intra.peff.net","threadId":"60605","inReplyTo":"120b3194-5eee-47ed-b2d8-bc6731b71a6b@web.de","subject":"[PATCH v2] t1006: add tests for %(objectsize:disk)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-23T10:09:05Z","receivedAt":"2023-12-23T10:09:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 22, 2023 at 12:13:10AM +0100, René Scharfe wrote:\n\n> >> \t\t\twhile read start oid rest\n> >> \t\t\tdo\n> >> \t\t\t\tsize=$((end - start)) &&\n> >> \t\t\t\tend=$start &&\n> >> \t\t\t\techo \"$oid $size\" ||\n> >> \t\t\t\treturn 1\n> >> \t\t\tdone <idx.sorted\n> >\n> > The one thing I do like is that we don't have to escape anything inside\n> > an awk program that is forced to use double-quotes. ;)\n> \n> For me it's processing the data in the \"correct\" order (descending, i.e.\n> starting at the end, which we have to calculate first anyway based on the\n> size).\n\nThat was one thing that I thought made it more complicated. The obvious\norder to me is start-to-end in the pack. But I do agree that going in\nreverse order makes things much simpler, as we compute the size of each\nentry as we see it (and so there are fewer special cases).\n\nSo I'm convinced that it's worth switching. Here's a v2 with your\nsuggestion.\n\n-- >8 --\nSubject: t1006: add tests for %(objectsize:disk)\n\nBack when we added this placeholder in a4ac106178 (cat-file: add\n%(objectsize:disk) format atom, 2013-07-10), there were no tests,\nclaiming \"[...]the exact numbers returned are volatile and subject to\nzlib and packing decisions\".\n\nBut we can use a little shell hackery to get the expected numbers\nourselves. To a certain degree this is just re-implementing what Git is\ndoing under the hood, but it is still worth doing. It makes sure we\nexercise the %(objectsize:disk) code at all, and having the two\nimplementations agree gives us more confidence.\n\nNote that our shell code assumes that no object appears twice (either in\ntwo packs, or as both loose and packed), as then the results really are\nundefined. That's OK for our purposes, and the test will notice if that\nassumption is violated (the shell version would produce duplicate lines\nthat Git's output does not have).\n\nHelped-by: René Scharfe <l.s.r@web.de>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t1006-cat-file.sh | 36 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 36 insertions(+)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 271c5e4fd3..e0c6482797 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -1100,6 +1100,42 @@ test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will wor\n \tcmp expect actual\n '\n \n+test_expect_success 'cat-file %(objectsize:disk) with --batch-all-objects' '\n+\t# our state has both loose and packed objects,\n+\t# so find both for our expected output\n+\t{\n+\t\tfind .git/objects/?? -type f |\n+\t\tawk -F/ \"{ print \\$0, \\$3\\$4 }\" |\n+\t\twhile read path oid\n+\t\tdo\n+\t\t\tsize=$(test_file_size \"$path\") &&\n+\t\t\techo \"$oid $size\" ||\n+\t\t\treturn 1\n+\t\tdone &&\n+\t\trawsz=$(test_oid rawsz) &&\n+\t\tfind .git/objects/pack -name \"*.idx\" |\n+\t\twhile read idx\n+\t\tdo\n+\t\t\tgit show-index <\"$idx\" >idx.raw &&\n+\t\t\tsort -nr <idx.raw >idx.sorted &&\n+\t\t\tpacksz=$(test_file_size \"${idx%.idx}.pack\") &&\n+\t\t\tend=$((packsz - rawsz)) &&\n+\t\t\twhile read start oid rest\n+\t\t\tdo\n+\t\t\t\tsize=$((end - start)) &&\n+\t\t\t\tend=$start &&\n+\t\t\t\techo \"$oid $size\" ||\n+\t\t\t\treturn 1\n+\t\t\tdone <idx.sorted ||\n+\t\t\treturn 1\n+\t\tdone\n+\t} >expect.raw &&\n+\tsort <expect.raw >expect &&\n+\tgit cat-file --batch-all-objects \\\n+\t\t--batch-check=\"%(objectname) %(objectsize:disk)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up replacement object' '\n \torig=$(git rev-parse HEAD) &&\n \tgit cat-file commit $orig >orig &&\n-- \n2.43.0.448.g93112243fb\n\n"},{"id":"485988","messageId":"20231223101853.GC2016274@coredump.intra.peff.net","threadId":"60605","inReplyTo":"120b3194-5eee-47ed-b2d8-bc6731b71a6b@web.de","subject":"Re: [PATCH] t1006: add tests for %(objectsize:disk)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-23T10:18:53Z","receivedAt":"2023-12-23T10:18:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 22, 2023 at 12:13:10AM +0100, René Scharfe wrote:\n\n> >> Should we deduplicate here, like cat-file does (i.e. use \"sort -u\")?\n> >> Having the same object in multiple places for whatever reason would not\n> >> be a cause for reporting an error in this test, I would think.\n> >\n> > No, for the reasons I said in the commit message: if an object exists in\n> > multiple places the test is already potentially invalid, as Git does not\n> > promise which version it will use. So it might work racily, or it might\n> > work for now but be fragile. By not de-duplicating, we make sure the\n> > test's assumption holds.\n> \n> Oh, skipped that paragraph.  Still I don't see how a duplicate object\n> would necessarily invalidate t1006.  The comment for the test \"cat-file\n> --batch-all-objects shows all objects\" a few lines above indicates that\n> it's picky about the provenance of objects, but it uses a separate\n> repository.  I can't infer the same requirement for the root repo, but\n> we already established that I can't read.\n\nThe cat-file documentation explicitly calls this situation out:\n\n  Note also that multiple copies of an object may be present in the\n  object database; in this case, it is undefined which copy’s size or\n  delta base will be reported.\n\nSo if t1006 were to grow such a duplicate object, what will happen? If\nwe de-dup in the new test, then we might end up mentioning the same copy\n(and the test passes), or we might not (and the test fails). But much\nworse, the results might be racy (depending on how cat-file happens to\ndecide which one to use). By no de-duping, then the test will reliably\nfail and the author can decide how to handle it then.\n\nIOW it is about failing immediately and predictably rather than letting\na future change to sneak a race or other accident-waiting-to-happen into\nt1006.\n\n> Anyway, if someone finds a use for git repack without -d or\n> git unpack-objects or whatever else causes duplicates in the root\n> repository of t1006 then they can try to reverse your ban with concrete\n> arguments.\n\nIn the real world, the most common way to get a duplicate is to fetch or\npush into a repository, such that:\n\n  1. There are enough objects to retain the pack (100 by default)\n\n  2. There's a thin delta in the on-the-wire pack (i.e., a delta against\n     a base that the sender knows the receiver has, but which isn't\n     itself sent).\n\nThen \"index-pack --fix-thin\" will complete the on-disk pack by storing a\ncopy of the base object in it. And now we have it in two packs (and if\nit's a delta or loose in the original, the size will be different).\n\n-Peff\n"},{"id":"486014","messageId":"62dc66ef-d669-4486-be61-8d50bf469627@web.de","threadId":"60605","inReplyTo":"20231223100905.GB2016274@coredump.intra.peff.net","subject":"Re: [PATCH v2] t1006: add tests for %(objectsize:disk)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-24T09:30:21Z","receivedAt":"2023-12-24T09:30:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.12.23 um 11:09 schrieb Jeff King:\n>\n> ---\n>  t/t1006-cat-file.sh | 36 ++++++++++++++++++++++++++++++++++++\n>  1 file changed, 36 insertions(+)\n>\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 271c5e4fd3..e0c6482797 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -1100,6 +1100,42 @@ test_expect_success 'cat-file --batch=\"batman\" with --batch-all-objects will wor\n>  \tcmp expect actual\n>  '\n>\n> +test_expect_success 'cat-file %(objectsize:disk) with --batch-all-objects' '\n> +\t# our state has both loose and packed objects,\n> +\t# so find both for our expected output\n> +\t{\n> +\t\tfind .git/objects/?? -type f |\n> +\t\tawk -F/ \"{ print \\$0, \\$3\\$4 }\" |\n> +\t\twhile read path oid\n> +\t\tdo\n> +\t\t\tsize=$(test_file_size \"$path\") &&\n> +\t\t\techo \"$oid $size\" ||\n> +\t\t\treturn 1\n> +\t\tdone &&\n> +\t\trawsz=$(test_oid rawsz) &&\n> +\t\tfind .git/objects/pack -name \"*.idx\" |\n> +\t\twhile read idx\n> +\t\tdo\n> +\t\t\tgit show-index <\"$idx\" >idx.raw &&\n> +\t\t\tsort -nr <idx.raw >idx.sorted &&\n> +\t\t\tpacksz=$(test_file_size \"${idx%.idx}.pack\") &&\n> +\t\t\tend=$((packsz - rawsz)) &&\n> +\t\t\twhile read start oid rest\n> +\t\t\tdo\n> +\t\t\t\tsize=$((end - start)) &&\n> +\t\t\t\tend=$start &&\n> +\t\t\t\techo \"$oid $size\" ||\n> +\t\t\t\treturn 1\n> +\t\t\tdone <idx.sorted ||\n> +\t\t\treturn 1\n> +\t\tdone\n> +\t} >expect.raw &&\n> +\tsort <expect.raw >expect &&\n> +\tgit cat-file --batch-all-objects \\\n> +\t\t--batch-check=\"%(objectname) %(objectsize:disk)\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'set up replacement object' '\n>  \torig=$(git rev-parse HEAD) &&\n>  \tgit cat-file commit $orig >orig &&\n\nLooks good to me.\n\nRené\n"},{"id":"486015","messageId":"290b20f6-ce3b-49b3-a61d-69277fd8c430@web.de","threadId":"60605","inReplyTo":"20231223101853.GC2016274@coredump.intra.peff.net","subject":"Re: [PATCH] t1006: add tests for %(objectsize:disk)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-12-24T09:30:17Z","receivedAt":"2023-12-24T09:30:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.12.23 um 11:18 schrieb Jeff King:\n> On Fri, Dec 22, 2023 at 12:13:10AM +0100, René Scharfe wrote:\n>\n>>>> Should we deduplicate here, like cat-file does (i.e. use \"sort -u\")?\n>>>> Having the same object in multiple places for whatever reason would not\n>>>> be a cause for reporting an error in this test, I would think.\n>>>\n>>> No, for the reasons I said in the commit message: if an object exists in\n>>> multiple places the test is already potentially invalid, as Git does not\n>>> promise which version it will use. So it might work racily, or it might\n>>> work for now but be fragile. By not de-duplicating, we make sure the\n>>> test's assumption holds.\n>>\n>> Oh, skipped that paragraph.  Still I don't see how a duplicate object\n>> would necessarily invalidate t1006.  The comment for the test \"cat-file\n>> --batch-all-objects shows all objects\" a few lines above indicates that\n>> it's picky about the provenance of objects, but it uses a separate\n>> repository.  I can't infer the same requirement for the root repo, but\n>> we already established that I can't read.\n>\n> The cat-file documentation explicitly calls this situation out:\n>\n>   Note also that multiple copies of an object may be present in the\n>   object database; in this case, it is undefined which copy’s size or\n>   delta base will be reported.\n>\n> So if t1006 were to grow such a duplicate object, what will happen? If\n> we de-dup in the new test, then we might end up mentioning the same copy\n> (and the test passes), or we might not (and the test fails). But much\n> worse, the results might be racy (depending on how cat-file happens to\n> decide which one to use). By no de-duping, then the test will reliably\n> fail and the author can decide how to handle it then.\n>\n> IOW it is about failing immediately and predictably rather than letting\n> a future change to sneak a race or other accident-waiting-to-happen into\n> t1006.\n>\n>> Anyway, if someone finds a use for git repack without -d or\n>> git unpack-objects or whatever else causes duplicates in the root\n>> repository of t1006 then they can try to reverse your ban with concrete\n>> arguments.\n>\n> In the real world, the most common way to get a duplicate is to fetch or\n> push into a repository, such that:\n>\n>   1. There are enough objects to retain the pack (100 by default)\n>\n>   2. There's a thin delta in the on-the-wire pack (i.e., a delta against\n>      a base that the sender knows the receiver has, but which isn't\n>      itself sent).\n>\n> Then \"index-pack --fix-thin\" will complete the on-disk pack by storing a\n> copy of the base object in it. And now we have it in two packs (and if\n> it's a delta or loose in the original, the size will be different).\n\nI think I get it now.  The size possibly being different is crucial.\ncat-file deduplicates based on object ID alone.  sort -u in t1006 would\ndeduplicate based on object ID and size, meaning that it would only\nremove duplicates of the same size.  Emulating the deduplication of\ncat-file is also possible, but would introduce the race you mentioned.\n\nHowever, even removing only same-size duplicates is unreliable because\nthere is no guarantee that the same object has the same size in\ndifferent packs.  Adding a new object that is a better delta base would\nchange the size.\n\nSo, deduplicating based on object ID and size is sound for any\nparticular run, but sizes are not stable and thus we need to know if\nthe tests do something that adds duplicates of any size.\n\nRené\n"}]}