{"thread":{"id":"63202","subject":"Testsuite failure on s390x and sparc64 after 6840fe9ee2","startedAt":"2025-03-26T20:42:58Z","lastAt":"2025-04-01T15:04:13Z","messageCount":14,"participants":["John Paul Adrian Glaubitz","Todd Zullinger","Patrick Steinhardt","SZEDER Gábor","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"515123","messageId":"89257ab82cd60d135cce02d51eacee7ec35c1c37.camel@physik.fu-berlin.de","threadId":"63202","inReplyTo":null,"subject":"Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"John Paul Adrian Glaubitz","fromEmail":"glaubitz@physik.fu-berlin.de","sentAt":"2025-03-26T20:42:55Z","receivedAt":"2025-03-26T20:42:58Z","isPatch":false,"sender":{"key":"glaubitz@physik.fu-berlin.de","avatar":"https://avatars.githubusercontent.com/u/1647645?v=4"},"body":"Hi,\n\nthe following commit:\n\ncommit 6840fe9ee29ab51ffd7d924c624dc62da22c50bf\nAuthor: Derrick Stolee <derrickstolee@github.com>\nDate:   Mon Feb 3 17:11:05 2025 +0000\n\n    backfill: add --min-batch-size=<n> option\n    \n    Users may want to specify a minimum batch size for their needs. This is only\n    a minimum: the path-walk API provides a list of OIDs that correspond to the\n    same path, and thus it is optimal to allow delta compression across those\n    objects in a single server request.\n    \n    We could consider limiting the request to have a maximum batch size in the\n    future. For now, we let the path-walk API batches determine the\n    boundaries.\n(...)\n\nbroke the testsuite on s390x [1] and sparc64 [2]. The following test fails:\n\nnot ok 4 - do partial clone 2, backfill min batch size\n\nCC'ing the author which is Derrick Stolee.\n\nThanks,\nAdrian\n\n> [1] https://buildd.debian.org/status/fetch.php?pkg=git&arch=s390x&ver=1%3A2.49.0-1&stamp=1742165887&raw=0\n> [2] https://buildd.debian.org/status/fetch.php?pkg=git&arch=sparc64&ver=1%3A2.49.0-1&stamp=1742674659&raw=0\n\n-- \n .''`.  John Paul Adrian Glaubitz\n: :' :  Debian Developer\n`. `'   Physicist\n  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913\n"},{"id":"515125","messageId":"Z-R_Zmr6kxCPLm-O@teonanacatl.net","threadId":"63202","inReplyTo":"89257ab82cd60d135cce02d51eacee7ec35c1c37.camel@physik.fu-berlin.de","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2025-03-26T22:27:50Z","receivedAt":"2025-03-26T22:27:52Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"John Paul Adrian Glaubitz wrote:\n> the following commit:\n> \n> commit 6840fe9ee29ab51ffd7d924c624dc62da22c50bf\n> Author: Derrick Stolee <derrickstolee@github.com>\n> Date:   Mon Feb 3 17:11:05 2025 +0000\n> \n>     backfill: add --min-batch-size=<n> option\n>     \n>     Users may want to specify a minimum batch size for their needs. This is only\n>     a minimum: the path-walk API provides a list of OIDs that correspond to the\n>     same path, and thus it is optimal to allow delta compression across those\n>     objects in a single server request.\n>     \n>     We could consider limiting the request to have a maximum batch size in the\n>     future. For now, we let the path-walk API batches determine the\n>     boundaries.\n> (...)\n> \n> broke the testsuite on s390x [1] and sparc64 [2]. The following test fails:\n> \n> not ok 4 - do partial clone 2, backfill min batch size\n> \n> CC'ing the author which is Derrick Stolee.\n\nI reported this during the rc period.  I didn't hear back on\nit, but hopefully your message will arrive at a more\nconvenient time. :)\n\nhttps://lore.kernel.org/git/Z8HW6petWuMRWSXf@teonanacatl.net/\n\n-- \nTodd\n"},{"id":"515224","messageId":"Z-Zr7BZL1UGqVxKu@pks.im","threadId":"63202","inReplyTo":"Z-R_Zmr6kxCPLm-O@teonanacatl.net","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-28T09:29:16Z","receivedAt":"2025-03-28T09:29:20Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 26, 2025 at 06:27:50PM -0400, Todd Zullinger wrote:\n> John Paul Adrian Glaubitz wrote:\n> > the following commit:\n> > \n> > commit 6840fe9ee29ab51ffd7d924c624dc62da22c50bf\n> > Author: Derrick Stolee <derrickstolee@github.com>\n> > Date:   Mon Feb 3 17:11:05 2025 +0000\n> > \n> >     backfill: add --min-batch-size=<n> option\n> >     \n> >     Users may want to specify a minimum batch size for their needs. This is only\n> >     a minimum: the path-walk API provides a list of OIDs that correspond to the\n> >     same path, and thus it is optimal to allow delta compression across those\n> >     objects in a single server request.\n> >     \n> >     We could consider limiting the request to have a maximum batch size in the\n> >     future. For now, we let the path-walk API batches determine the\n> >     boundaries.\n> > (...)\n> > \n> > broke the testsuite on s390x [1] and sparc64 [2]. The following test fails:\n> > \n> > not ok 4 - do partial clone 2, backfill min batch size\n> > \n> > CC'ing the author which is Derrick Stolee.\n> \n> I reported this during the rc period.  I didn't hear back on\n> it, but hopefully your message will arrive at a more\n> convenient time. :)\n> \n> https://lore.kernel.org/git/Z8HW6petWuMRWSXf@teonanacatl.net/\n\nCopy-pasting the test logs from that mail:\n\n    expecting success of 5620.4 'do partial clone 2, backfill min batch size':\n            git clone --no-checkout --filter=blob:none      \\\n                    --single-branch --branch=main           \\\n                    \"file://$(pwd)/srv.bare\" backfill2 &&\n            GIT_TRACE2_EVENT=\"$(pwd)/batch-trace\" git \\\n                    -C backfill2 backfill --min-batch-size=20 &&\n            # Batches were used\n            test_trace2_data promisor fetch_count 20 <batch-trace >matches &&\n            test_line_count = 2 matches &&\n            test_trace2_data promisor fetch_count 8 <batch-trace &&\n            # No more missing objects!\n            git -C backfill2 rev-list --quiet --objects --missing=print HEAD >revs2 &&\n            test_line_count = 0 revs2\n    +++ pwd\n    ++ git clone --no-checkout --filter=blob:none --single-branch --branch=main 'file:///tmp/git-t.sYdo/trash directory.t5620-backfill/srv.bare' backfill2\n    Cloning into 'backfill2'...\n    +++ pwd\n    ++ GIT_TRACE2_EVENT='/tmp/git-t.sYdo/trash directory.t5620-backfill/batch-trace'\n    ++ git -C backfill2 backfill --min-batch-size=20\n    ++ test_trace2_data promisor fetch_count 20\n    ++ grep -e '\"category\":\"promisor\",\"key\":\"fetch_count\",\"value\":\"20\"'\n    error: last command exited with $?=1\n    not ok 4 - do partial clone 2, backfill min batch size\n\nIt would be nice to learn what the file contains instead of the expected\nstring, which might give us a bit more of a hint what's wrong. You can\nfor example apply the following patch:\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 79377bc0fc2..197494cd28c 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1975,7 +1975,7 @@ test_region () {\n #\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" git pack-objects ... &&\n #\ttest_trace2_data pack-objects reused N <trace2.txt\n test_trace2_data () {\n-\tgrep -e '\"category\":\"'\"$1\"'\",\"key\":\"'\"$2\"'\",\"value\":\"'\"$3\"'\"'\n+\ttest_grep -e '\"category\":\"'\"$1\"'\",\"key\":\"'\"$2\"'\",\"value\":\"'\"$3\"'\"'\n }\n \n # Given a GIT_TRACE2_EVENT log over stdin, writes to stdout a list of URLs\n\nIf you then re-run the test with `-ix` we should end up printing the\ncontents of that non-matching file.\n\nThanks!\n\nPatrick\n"},{"id":"515225","messageId":"Z-ZsUsaSw2pQwlYb@pks.im","threadId":"63202","inReplyTo":"Z-Zr7BZL1UGqVxKu@pks.im","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-28T09:30:58Z","receivedAt":"2025-03-28T09:31:02Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Also Cc'ing Stolee's current mail address instead of the GitHub one.\n\nOn Fri, Mar 28, 2025 at 10:29:16AM +0100, Patrick Steinhardt wrote:\n> On Wed, Mar 26, 2025 at 06:27:50PM -0400, Todd Zullinger wrote:\n> > John Paul Adrian Glaubitz wrote:\n> > > the following commit:\n> > > \n> > > commit 6840fe9ee29ab51ffd7d924c624dc62da22c50bf\n> > > Author: Derrick Stolee <derrickstolee@github.com>\n> > > Date:   Mon Feb 3 17:11:05 2025 +0000\n> > > \n> > >     backfill: add --min-batch-size=<n> option\n> > >     \n> > >     Users may want to specify a minimum batch size for their needs. This is only\n> > >     a minimum: the path-walk API provides a list of OIDs that correspond to the\n> > >     same path, and thus it is optimal to allow delta compression across those\n> > >     objects in a single server request.\n> > >     \n> > >     We could consider limiting the request to have a maximum batch size in the\n> > >     future. For now, we let the path-walk API batches determine the\n> > >     boundaries.\n> > > (...)\n> > > \n> > > broke the testsuite on s390x [1] and sparc64 [2]. The following test fails:\n> > > \n> > > not ok 4 - do partial clone 2, backfill min batch size\n> > > \n> > > CC'ing the author which is Derrick Stolee.\n> > \n> > I reported this during the rc period.  I didn't hear back on\n> > it, but hopefully your message will arrive at a more\n> > convenient time. :)\n> > \n> > https://lore.kernel.org/git/Z8HW6petWuMRWSXf@teonanacatl.net/\n> \n> Copy-pasting the test logs from that mail:\n> \n>     expecting success of 5620.4 'do partial clone 2, backfill min batch size':\n>             git clone --no-checkout --filter=blob:none      \\\n>                     --single-branch --branch=main           \\\n>                     \"file://$(pwd)/srv.bare\" backfill2 &&\n>             GIT_TRACE2_EVENT=\"$(pwd)/batch-trace\" git \\\n>                     -C backfill2 backfill --min-batch-size=20 &&\n>             # Batches were used\n>             test_trace2_data promisor fetch_count 20 <batch-trace >matches &&\n>             test_line_count = 2 matches &&\n>             test_trace2_data promisor fetch_count 8 <batch-trace &&\n>             # No more missing objects!\n>             git -C backfill2 rev-list --quiet --objects --missing=print HEAD >revs2 &&\n>             test_line_count = 0 revs2\n>     +++ pwd\n>     ++ git clone --no-checkout --filter=blob:none --single-branch --branch=main 'file:///tmp/git-t.sYdo/trash directory.t5620-backfill/srv.bare' backfill2\n>     Cloning into 'backfill2'...\n>     +++ pwd\n>     ++ GIT_TRACE2_EVENT='/tmp/git-t.sYdo/trash directory.t5620-backfill/batch-trace'\n>     ++ git -C backfill2 backfill --min-batch-size=20\n>     ++ test_trace2_data promisor fetch_count 20\n>     ++ grep -e '\"category\":\"promisor\",\"key\":\"fetch_count\",\"value\":\"20\"'\n>     error: last command exited with $?=1\n>     not ok 4 - do partial clone 2, backfill min batch size\n> \n> It would be nice to learn what the file contains instead of the expected\n> string, which might give us a bit more of a hint what's wrong. You can\n> for example apply the following patch:\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 79377bc0fc2..197494cd28c 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1975,7 +1975,7 @@ test_region () {\n>  #\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" git pack-objects ... &&\n>  #\ttest_trace2_data pack-objects reused N <trace2.txt\n>  test_trace2_data () {\n> -\tgrep -e '\"category\":\"'\"$1\"'\",\"key\":\"'\"$2\"'\",\"value\":\"'\"$3\"'\"'\n> +\ttest_grep -e '\"category\":\"'\"$1\"'\",\"key\":\"'\"$2\"'\",\"value\":\"'\"$3\"'\"'\n>  }\n>  \n>  # Given a GIT_TRACE2_EVENT log over stdin, writes to stdout a list of URLs\n> \n> If you then re-run the test with `-ix` we should end up printing the\n> contents of that non-matching file.\n> \n> Thanks!\n> \n> Patrick\n> \n"},{"id":"515226","messageId":"4276c8d0b72f11f325482756d3bc251327d0ac47.camel@physik.fu-berlin.de","threadId":"63202","inReplyTo":"Z-Zr7BZL1UGqVxKu@pks.im","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"John Paul Adrian Glaubitz","fromEmail":"glaubitz@physik.fu-berlin.de","sentAt":"2025-03-28T09:38:51Z","receivedAt":"2025-03-28T09:39:00Z","isPatch":false,"sender":{"key":"glaubitz@physik.fu-berlin.de","avatar":"https://avatars.githubusercontent.com/u/1647645?v=4"},"body":"Hi Patrick,\n\nOn Fri, 2025-03-28 at 10:29 +0100, Patrick Steinhardt wrote:\n> > I reported this during the rc period.  I didn't hear back on\n> > it, but hopefully your message will arrive at a more\n> > convenient time. :)\n> > \n> > https://lore.kernel.org/git/Z8HW6petWuMRWSXf@teonanacatl.net/\n> \n> Copy-pasting the test logs from that mail:\n> \n>     expecting success of 5620.4 'do partial clone 2, backfill min batch size':\n>             git clone --no-checkout --filter=blob:none      \\\n>                     --single-branch --branch=main           \\\n>                     \"file://$(pwd)/srv.bare\" backfill2 &&\n>             GIT_TRACE2_EVENT=\"$(pwd)/batch-trace\" git \\\n>                     -C backfill2 backfill --min-batch-size=20 &&\n>             # Batches were used\n>             test_trace2_data promisor fetch_count 20 <batch-trace >matches &&\n>             test_line_count = 2 matches &&\n>             test_trace2_data promisor fetch_count 8 <batch-trace &&\n>             # No more missing objects!\n>             git -C backfill2 rev-list --quiet --objects --missing=print HEAD >revs2 &&\n>             test_line_count = 0 revs2\n>     +++ pwd\n>     ++ git clone --no-checkout --filter=blob:none --single-branch --branch=main 'file:///tmp/git-t.sYdo/trash directory.t5620-backfill/srv.bare' backfill2\n>     Cloning into 'backfill2'...\n>     +++ pwd\n>     ++ GIT_TRACE2_EVENT='/tmp/git-t.sYdo/trash directory.t5620-backfill/batch-trace'\n>     ++ git -C backfill2 backfill --min-batch-size=20\n>     ++ test_trace2_data promisor fetch_count 20\n>     ++ grep -e '\"category\":\"promisor\",\"key\":\"fetch_count\",\"value\":\"20\"'\n>     error: last command exited with $?=1\n>     not ok 4 - do partial clone 2, backfill min batch size\n> \n> It would be nice to learn what the file contains instead of the expected\n> string, which might give us a bit more of a hint what's wrong. You can\n> for example apply the following patch:\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 79377bc0fc2..197494cd28c 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1975,7 +1975,7 @@ test_region () {\n>  #\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" git pack-objects ... &&\n>  #\ttest_trace2_data pack-objects reused N <trace2.txt\n>  test_trace2_data () {\n> -\tgrep -e '\"category\":\"'\"$1\"'\",\"key\":\"'\"$2\"'\",\"value\":\"'\"$3\"'\"'\n> +\ttest_grep -e '\"category\":\"'\"$1\"'\",\"key\":\"'\"$2\"'\",\"value\":\"'\"$3\"'\"'\n>  }\n>  \n>  # Given a GIT_TRACE2_EVENT log over stdin, writes to stdout a list of URLs\n> \n> If you then re-run the test with `-ix` we should end up printing the\n> contents of that non-matching file.\n\nCould you please post the complete command line? I have no clue where to pass \"-ix\".\n\nI was previously running the tests with \"make test\".\n\nThanks,\nAdrian\n\n-- \n .''`.  John Paul Adrian Glaubitz\n: :' :  Debian Developer\n`. `'   Physicist\n  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913\n"},{"id":"515234","messageId":"Z-atRMGXHilZRTEL@teonanacatl.net","threadId":"63202","inReplyTo":"4276c8d0b72f11f325482756d3bc251327d0ac47.camel@physik.fu-berlin.de","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2025-03-28T14:08:04Z","receivedAt":"2025-03-28T14:08:07Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"John Paul Adrian Glaubitz wrote:\n> Hi Patrick,\n> \n> On Fri, 2025-03-28 at 10:29 +0100, Patrick Steinhardt wrote:\n>>> I reported this during the rc period.  I didn't hear back on\n>>> it, but hopefully your message will arrive at a more\n>>> convenient time. :)\n>>> \n>>> https://lore.kernel.org/git/Z8HW6petWuMRWSXf@teonanacatl.net/\n>> \n>> Copy-pasting the test logs from that mail:\n>> \n>>     expecting success of 5620.4 'do partial clone 2, backfill min batch size':\n>>             git clone --no-checkout --filter=blob:none      \\\n>>                     --single-branch --branch=main           \\\n>>                     \"file://$(pwd)/srv.bare\" backfill2 &&\n>>             GIT_TRACE2_EVENT=\"$(pwd)/batch-trace\" git \\\n>>                     -C backfill2 backfill --min-batch-size=20 &&\n>>             # Batches were used\n>>             test_trace2_data promisor fetch_count 20 <batch-trace >matches &&\n>>             test_line_count = 2 matches &&\n>>             test_trace2_data promisor fetch_count 8 <batch-trace &&\n>>             # No more missing objects!\n>>             git -C backfill2 rev-list --quiet --objects --missing=print HEAD >revs2 &&\n>>             test_line_count = 0 revs2\n>>     +++ pwd\n>>     ++ git clone --no-checkout --filter=blob:none --single-branch --branch=main 'file:///tmp/git-t.sYdo/trash directory.t5620-backfill/srv.bare' backfill2\n>>     Cloning into 'backfill2'...\n>>     +++ pwd\n>>     ++ GIT_TRACE2_EVENT='/tmp/git-t.sYdo/trash directory.t5620-backfill/batch-trace'\n>>     ++ git -C backfill2 backfill --min-batch-size=20\n>>     ++ test_trace2_data promisor fetch_count 20\n>>     ++ grep -e '\"category\":\"promisor\",\"key\":\"fetch_count\",\"value\":\"20\"'\n>>     error: last command exited with $?=1\n>>     not ok 4 - do partial clone 2, backfill min batch size\n>> \n>> It would be nice to learn what the file contains instead of the expected\n>> string, which might give us a bit more of a hint what's wrong. You can\n>> for example apply the following patch:\n>> \n>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n>> index 79377bc0fc2..197494cd28c 100644\n>> --- a/t/test-lib-functions.sh\n>> +++ b/t/test-lib-functions.sh\n>> @@ -1975,7 +1975,7 @@ test_region () {\n>>  #\tGIT_TRACE2_EVENT=\"$(pwd)/trace.txt\" git pack-objects ... &&\n>>  #\ttest_trace2_data pack-objects reused N <trace2.txt\n>>  test_trace2_data () {\n>> -\tgrep -e '\"category\":\"'\"$1\"'\",\"key\":\"'\"$2\"'\",\"value\":\"'\"$3\"'\"'\n>> +\ttest_grep -e '\"category\":\"'\"$1\"'\",\"key\":\"'\"$2\"'\",\"value\":\"'\"$3\"'\"'\n>>  }\n>>  \n>>  # Given a GIT_TRACE2_EVENT log over stdin, writes to stdout a list of URLs\n>> \n>> If you then re-run the test with `-ix` we should end up printing the\n>> contents of that non-matching file.\n> \n> Could you please post the complete command line? I have no clue where to pass \"-ix\".\n> \n> I was previously running the tests with \"make test\".\n\nYou'd do something like:\n\n    cd t && ./t5620-backfill.sh -ix\n\nThough the patch to change grep to test_grep is incomplete,\nI believe.  Using that, you get an error:\n\n    error: bug in the test script: test_grep requires a file\n    to read as the last parameter\n\nI don't have a lot of time to poke at this today, but I'll\nmake another test run on an s390x build host without that\npatch, but where I can save the output and post it\nsomewhere.\n\nFor the Fedora packaging, it will be something like this:\n\n    make -C t all || {\n        (cd t && ./t5620-backfill.sh -ix);\n        ./print-failed-test-output;\n    }\n\nWhere print-failed-test-output is a script¹ which snarfs up\nthe output files in t/test-results and the test directory,\nsince there is not direct shell access to the build host(s).\n\n¹ https://src.fedoraproject.org/rpms/git/raw/0af3adf/f/print-failed-test-output\n\n-- \nTodd\n"},{"id":"515237","messageId":"Z-bCNdOOLrM2Chb8@teonanacatl.net","threadId":"63202","inReplyTo":"Z-atRMGXHilZRTEL@teonanacatl.net","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2025-03-28T15:37:25Z","receivedAt":"2025-03-28T15:37:28Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"I wrote:\n> I don't have a lot of time to poke at this today, but I'll\n> make another test run on an s390x build host without that\n> patch, but where I can save the output and post it\n> somewhere.\n> \n> For the Fedora packaging, it will be something like this:\n> \n>     make -C t all || {\n>         (cd t && ./t5620-backfill.sh -ix);\n>         ./print-failed-test-output;\n>     }\n\nThe matches file is empty.\n\n    $ ls -lhn batch-trace matches \n    -rw-r--r--. 1 1000 1000 31K Mar 28 11:09 batch-trace\n    -rw-r--r--. 1 1000 1000   0 Mar 28 11:09 matches\n\nThe only match in batch-trace for promisor fetch_count is\nfrom the previous test:\n\n    $ grep -e '\"category\":\"promisor\",\"key\":\"fetch_count\",\"value\":' batch-trace\n    {\"event\":\"data\",\"sid\":\"20250328T150939.623820Z-H9aa15b67-P0008f613\",\"thread\":\"main\",\"time\":\"2025-03-28T15:09:39.625484Z\",\"file\":\"promisor-remote.c\",\"line\":55,\"repo\":1,\"t_abs\":0.001777,\"t_rel\":0.001777,\"nesting\":1,\"category\":\"promisor\",\"key\":\"fetch_count\",\"value\":\"48\"}\n\nThe trash directory for the test run is here, in case anyone\nwants to poke at it:\n\n    https://tmz.fedorapeople.org/t5620-backfill-trash-dir.tar.gz\n\nThe full build log is available as well:\n\n    https://tmz.fedorapeople.org/git-2.49.0-s390x-build.log\n\nIf you search for 'BEGIN BASE64 MESSAGE' in that, it\nprovides a command which can be used to extract the full\ntest-results directory.  That's used to get the output from\nthe build hosts where shell access isn't available.  I don't\nknow that it's got anything which isn't in the trash\ndirectory tarball which I already extracted, but it's there\njust in case.\n\n-- \nTodd\n"},{"id":"515361","messageId":"Z-qKGqpbdaW9WCrP@pks.im","threadId":"63202","inReplyTo":"Z-bCNdOOLrM2Chb8@teonanacatl.net","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-31T12:27:06Z","receivedAt":"2025-03-31T12:27:14Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 28, 2025 at 11:37:25AM -0400, Todd Zullinger wrote:\n> I wrote:\n> > I don't have a lot of time to poke at this today, but I'll\n> > make another test run on an s390x build host without that\n> > patch, but where I can save the output and post it\n> > somewhere.\n> > \n> > For the Fedora packaging, it will be something like this:\n> > \n> >     make -C t all || {\n> >         (cd t && ./t5620-backfill.sh -ix);\n> >         ./print-failed-test-output;\n> >     }\n> \n> The matches file is empty.\n> \n>     $ ls -lhn batch-trace matches \n>     -rw-r--r--. 1 1000 1000 31K Mar 28 11:09 batch-trace\n>     -rw-r--r--. 1 1000 1000   0 Mar 28 11:09 matches\n> \n> The only match in batch-trace for promisor fetch_count is\n> from the previous test:\n> \n>     $ grep -e '\"category\":\"promisor\",\"key\":\"fetch_count\",\"value\":' batch-trace\n>     {\"event\":\"data\",\"sid\":\"20250328T150939.623820Z-H9aa15b67-P0008f613\",\"thread\":\"main\",\"time\":\"2025-03-28T15:09:39.625484Z\",\"file\":\"promisor-remote.c\",\"line\":55,\"repo\":1,\"t_abs\":0.001777,\"t_rel\":0.001777,\"nesting\":1,\"category\":\"promisor\",\"key\":\"fetch_count\",\"value\":\"48\"}\n> \n> The trash directory for the test run is here, in case anyone\n> wants to poke at it:\n> \n>     https://tmz.fedorapeople.org/t5620-backfill-trash-dir.tar.gz\n> \n> The full build log is available as well:\n> \n>     https://tmz.fedorapeople.org/git-2.49.0-s390x-build.log\n> \n> If you search for 'BEGIN BASE64 MESSAGE' in that, it\n> provides a command which can be used to extract the full\n> test-results directory.  That's used to get the output from\n> the build hosts where shell access isn't available.  I don't\n> know that it's got anything which isn't in the trash\n> directory tarball which I already extracted, but it's there\n> just in case.\n\nThanks for the additional information!\n\nOne thing I stumbled over: the `--min-batch-size` parameter is parsed\nusing `OPT_INTEGER()`, which expects the value pointer to point to an\ninteger. But we pass `struct backfill_context::min_batch_size`, which is\nof type `size_t`. Maybe that's causing us to end up with an invalid\nvalue?\n\nCould you please check whether the below diff fixes the issue for you?\nIf so I can turn it into a proper patch.\n\nPatrick\n\n-- >8 --\n\ndiff --git a/builtin/backfill.c b/builtin/backfill.c\nindex 33e1ea2f84f..1dd0d746538 100644\n--- a/builtin/backfill.c\n+++ b/builtin/backfill.c\n@@ -119,11 +119,11 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit\n \tstruct backfill_context ctx = {\n \t\t.repo = repo,\n \t\t.current_batch = OID_ARRAY_INIT,\n-\t\t.min_batch_size = 50000,\n \t\t.sparse = 0,\n \t};\n+\tunsigned long min_batch_size = 50000;\n \tstruct option options[] = {\n-\t\tOPT_INTEGER(0, \"min-batch-size\", &ctx.min_batch_size,\n+\t\tOPT_MAGNITUDE(0, \"min-batch-size\", &min_batch_size,\n \t\t\t    N_(\"Minimum number of objects to request at a time\")),\n \t\tOPT_BOOL(0, \"sparse\", &ctx.sparse,\n \t\t\t N_(\"Restrict the missing objects to the current sparse-checkout\")),\n@@ -140,6 +140,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit\n \n \tif (ctx.sparse < 0)\n \t\tctx.sparse = core_apply_sparse_checkout;\n+\tctx.min_batch_size = min_batch_size;\n \n \tresult = do_backfill(&ctx);\n \tbackfill_context_clear(&ctx);\n"},{"id":"515377","messageId":"Z-q5aOIahoUKSyBi@teonanacatl.net","threadId":"63202","inReplyTo":"Z-qKGqpbdaW9WCrP@pks.im","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2025-03-31T15:48:56Z","receivedAt":"2025-03-31T15:48:59Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"[cc: fixed Derrick's address]\n\nHi Patrick,\n\nPatrick Steinhardt wrote:\n> Thanks for the additional information!\n\nThank you for looking into it and providing a patch!\n\n> One thing I stumbled over: the `--min-batch-size` parameter is parsed\n> using `OPT_INTEGER()`, which expects the value pointer to point to an\n> integer. But we pass `struct backfill_context::min_batch_size`, which is\n> of type `size_t`. Maybe that's causing us to end up with an invalid\n> value?\n> \n> Could you please check whether the below diff fixes the issue for you?\n> If so I can turn it into a proper patch.\n\nIt does indeed lead to a successful test run:\n\nt5620-backfill.sh ..................................\nok 1 - setup repo for object creation\nok 2 - setup bare clone for server\nok 3 - do partial clone 1, backfill gets all objects\nok 4 - do partial clone 2, backfill min batch size\nok 5 - backfill --sparse without sparse-checkout fails\nok 6 - backfill --sparse\nok 7 - backfill --sparse without cone mode (positive)\nok 8 - backfill --sparse without cone mode (negative)\nok 9 - create a partial clone over HTTP\nok 10 - backfilling over HTTP succeeds\n# passed all 10 test(s)\n1..10\n\nSource: https://kojipkgs.fedoraproject.org//work/tasks/3947/130943947/build.log\n\nI tested it against the other common architectures the\nFedora build system provides as well, to be sure no others\nregressed, although there aren't as many as the Debian build\nsystem :).\n\n-- \nTodd\n"},{"id":"515394","messageId":"Z+rcVY7KqEuF1wFw@szeder.dev","threadId":"63202","inReplyTo":"Z-qKGqpbdaW9WCrP@pks.im","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2025-03-31T18:17:57Z","receivedAt":"2025-03-31T18:18:00Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Mar 31, 2025 at 02:27:06PM +0200, Patrick Steinhardt wrote:\n> One thing I stumbled over: the `--min-batch-size` parameter is parsed\n> using `OPT_INTEGER()`, which expects the value pointer to point to an\n> integer. But we pass `struct backfill_context::min_batch_size`, which is\n> of type `size_t`. Maybe that's causing us to end up with an invalid\n> value?\n\nWe could teach parse-options to verify at compile time that it got a\n'value' pointer to an appropriately sized variable with a simple\ntrick:\n\ndiff --git a/parse-options.h b/parse-options.h\nindex 997ffbee80..ac63f9548a 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -213,7 +213,7 @@ struct option {\n \t.type = OPTION_INTEGER, \\\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n-\t.value = (v), \\\n+\t.value = (v) + 0/(sizeof(*(v)) == sizeof(int)), \\\n \t.argh = N_(\"n\"), \\\n \t.help = (h), \\\n \t.flags = (f), \\\n\nThis bug would then cause a compiler error like this:\n\n      CC builtin/backfill.o\n  In file included from builtin/backfill.c:7:\n  builtin/backfill.c: In function ‘cmd_backfill’:\n  ./parse-options.h:216:25: error: division by zero [-Werror=div-by-zero]\n    216 |         .value = (v) + 0/(sizeof(*v) == sizeof(int)), \\\n        |                         ^\n  ./parse-options.h:272:37: note: in expansion of macro ‘OPT_INTEGER_F’\n    272 | #define OPT_INTEGER(s, l, v, h)     OPT_INTEGER_F(s, l, v, h, 0)\n        |                                     ^~~~~~~~~~~~~\n  builtin/backfill.c:126:17: note: in expansion of macro ‘OPT_INTEGER’\n    126 |                 OPT_INTEGER(0, \"min-batch-size\", &ctx.min_batch_size,\n        |                 ^~~~~~~~~~~\n  cc1: all warnings being treated as errors\n  make: *** [Makefile:2811: builtin/backfill.o] Error 1\n\nAlas, the change is ugly (and we should do the same for many other\nOPT_* macros as well) and the error message is far from\nto-the-point...  Turning this into something usable would require a\nmore clever trick, and that's more than I can devote to this issue.\n\n"},{"id":"515405","messageId":"20250401023358.GA1087913@coredump.intra.peff.net","threadId":"63202","inReplyTo":"Z+rcVY7KqEuF1wFw@szeder.dev","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-01T02:33:58Z","receivedAt":"2025-04-01T02:34:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 31, 2025 at 08:17:57PM +0200, SZEDER Gábor wrote:\n\n> On Mon, Mar 31, 2025 at 02:27:06PM +0200, Patrick Steinhardt wrote:\n> > One thing I stumbled over: the `--min-batch-size` parameter is parsed\n> > using `OPT_INTEGER()`, which expects the value pointer to point to an\n> > integer. But we pass `struct backfill_context::min_batch_size`, which is\n> > of type `size_t`. Maybe that's causing us to end up with an invalid\n> > value?\n> \n> We could teach parse-options to verify at compile time that it got a\n> 'value' pointer to an appropriately sized variable with a simple\n> trick:\n\nThat would be nice. I think we've discussed type safety for\nparse-options before, but IIRC none of the solutions were very\nsatisfying. But this sounds like a relatively low-effort approach that\nbuys us something, at least. I wonder if it could even be extended to\nuse __builtin_types_compatible() on platforms that support it.\n\n+cc René as our resident expert on gross C hacks. ;)\n\n> This bug would then cause a compiler error like this:\n> \n>       CC builtin/backfill.o\n>   In file included from builtin/backfill.c:7:\n>   builtin/backfill.c: In function ‘cmd_backfill’:\n>   ./parse-options.h:216:25: error: division by zero [-Werror=div-by-zero]\n>     216 |         .value = (v) + 0/(sizeof(*v) == sizeof(int)), \\\n>         |                         ^\n>   ./parse-options.h:272:37: note: in expansion of macro ‘OPT_INTEGER_F’\n>     272 | #define OPT_INTEGER(s, l, v, h)     OPT_INTEGER_F(s, l, v, h, 0)\n>         |                                     ^~~~~~~~~~~~~\n>   builtin/backfill.c:126:17: note: in expansion of macro ‘OPT_INTEGER’\n>     126 |                 OPT_INTEGER(0, \"min-batch-size\", &ctx.min_batch_size,\n>         |                 ^~~~~~~~~~~\n>   cc1: all warnings being treated as errors\n>   make: *** [Makefile:2811: builtin/backfill.o] Error 1\n> \n> Alas, the change is ugly (and we should do the same for many other\n> OPT_* macros as well) and the error message is far from\n> to-the-point...  Turning this into something usable would require a\n> more clever trick, and that's more than I can devote to this issue.\n\nWe do have BUILD_ASSERT_OR_ZERO(). It produces similarly arcane errors,\nbut at least the presence of the macro name helps a bit. E.g., doing\nthis:\n\ndiff --git a/parse-options.h b/parse-options.h\nindex 997ffbee80..5303ad6bcf 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -213,7 +213,7 @@ struct option {\n \t.type = OPTION_INTEGER, \\\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n-\t.value = (v), \\\n+\t.value = (v) + BUILD_ASSERT_OR_ZERO(sizeof(*v) == sizeof(int)), \\\n \t.argh = N_(\"n\"), \\\n \t.help = (h), \\\n \t.flags = (f), \\\n\nyields:\n\n      CC builtin/backfill.o\n  In file included from ./builtin.h:4,\n                   from builtin/backfill.c:4:\n  builtin/backfill.c: In function ‘cmd_backfill’:\n  ./git-compat-util.h:103:22: error: size of unnamed array is negative\n    103 |         (sizeof(char [1 - 2*!(cond)]) - 1)\n        |                      ^\n  ./parse-options.h:216:24: note: in expansion of macro ‘BUILD_ASSERT_OR_ZERO’\n    216 |         .value = (v) + BUILD_ASSERT_OR_ZERO(sizeof(*v) == sizeof(int)), \\\n        |                        ^~~~~~~~~~~~~~~~~~~~\n  ./parse-options.h:272:37: note: in expansion of macro ‘OPT_INTEGER_F’\n    272 | #define OPT_INTEGER(s, l, v, h)     OPT_INTEGER_F(s, l, v, h, 0)\n        |                                     ^~~~~~~~~~~~~\n  builtin/backfill.c:126:17: note: in expansion of macro ‘OPT_INTEGER’\n    126 |                 OPT_INTEGER(0, \"min-batch-size\", &ctx.min_batch_size,\n        |                 ^~~~~~~~~~~\n  make: *** [Makefile:2810: builtin/backfill.o] Error 1\n\n-Peff\n"},{"id":"515410","messageId":"20250401031030.GB1087913@coredump.intra.peff.net","threadId":"63202","inReplyTo":"20250401023358.GA1087913@coredump.intra.peff.net","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-01T03:10:30Z","receivedAt":"2025-04-01T03:10:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 31, 2025 at 10:33:58PM -0400, Jeff King wrote:\n\n> That would be nice. I think we've discussed type safety for\n> parse-options before, but IIRC none of the solutions were very\n> satisfying. But this sounds like a relatively low-effort approach that\n> buys us something, at least. I wonder if it could even be extended to\n> use __builtin_types_compatible() on platforms that support it.\n\nSo here's a slightly fancier version that uses the gcc builtin when it's\navailable:\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 8560c89374..7bcbe0b4ac 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -110,11 +110,17 @@ DISABLE_WARNING(-Wsign-compare)\n # define BARF_UNLESS_COPYABLE(dst, src) \\\n \tBUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(__typeof__(*(dst)), \\\n \t\t\t\t\t\t\t  __typeof__(*(src))))\n+\n+# define BARF_UNLESS_TYPE_MATCH(var, type) \\\n+\tBUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(__typeof__(*(var)), type))\n+\n #else\n # define BARF_UNLESS_AN_ARRAY(arr) 0\n # define BARF_UNLESS_COPYABLE(dst, src) \\\n \tBUILD_ASSERT_OR_ZERO(0 ? ((*(dst) = *(src)), 0) : \\\n \t\t\t\t sizeof(*(dst)) == sizeof(*(src)))\n+# define BARF_UNLESS_TYPE_MATCH(var, type) \\\n+\tBUILD_ASSERT_OR_ZERO(sizeof(*(var)) == sizeof(type))\n #endif\n /*\n  * ARRAY_SIZE - get the number of elements in a visible array\ndiff --git a/parse-options.h b/parse-options.h\nindex 997ffbee80..b38a852a8b 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -213,7 +213,7 @@ struct option {\n \t.type = OPTION_INTEGER, \\\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n-\t.value = (v), \\\n+\t.value = (v) + BARF_UNLESS_TYPE_MATCH((v), int), \\\n \t.argh = N_(\"n\"), \\\n \t.help = (h), \\\n \t.flags = (f), \\\n\nThat turns up several more hits, which all seem to be related to\nsigned-ness (mostly passing a pointer to unsigned). E.g.:\n\n  git grep --after-context=-1 foo -- builtin/checkout.c\n\nends up assigning \"-1\" to an \"unsigned\" via pointer casting. I think\nthat's probably technically undefined behavior, but works OK in practice\nto give you UINT_MAX.  I'd have thought that would give you infinite\ncontext, so it might even be doing something useful (or at least\nsomething that users might rely upon), but strangely it doesn't seem to\n(I'd guess very large context values aren't handled in the grep code\nsomehow).\n\nSo it might be possible to clean these up without hurting anything else.\n\n-Peff\n"},{"id":"515423","messageId":"Z-vRQ-FNv7WD02hl@pks.im","threadId":"63202","inReplyTo":"20250401031030.GB1087913@coredump.intra.peff.net","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-01T11:43:37Z","receivedAt":"2025-04-01T11:43:46Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 31, 2025 at 11:10:30PM -0400, Jeff King wrote:\n> On Mon, Mar 31, 2025 at 10:33:58PM -0400, Jeff King wrote:\n> \n> > That would be nice. I think we've discussed type safety for\n> > parse-options before, but IIRC none of the solutions were very\n> > satisfying. But this sounds like a relatively low-effort approach that\n> > buys us something, at least. I wonder if it could even be extended to\n> > use __builtin_types_compatible() on platforms that support it.\n> \n> So here's a slightly fancier version that uses the gcc builtin when it's\n> available:\n\nThanks for these! I'd also like to spin this even further: right now we\ndon't really care about the precision of the underlying integer types at\nall. While we could force all users to the same type via your mechanism,\nI think that'd ultimately be quite awkward. Another way would be to use\none macro per underlying integer type, but that would quickly explode in\nscope.\n\nI'll instead try to extend the parse-options interface so that we track\nthe precision of the underlying integer and then produce an error when\nthe parsed integer exceeds that precision.\n\nI'll send a patch series later this week.\n\nPatrick\n"},{"id":"515440","messageId":"Z-wAYoYBv-ge10I7@pks.im","threadId":"63202","inReplyTo":"Z-vRQ-FNv7WD02hl@pks.im","subject":"Re: Testsuite failure on s390x and sparc64 after 6840fe9ee2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-01T15:04:02Z","receivedAt":"2025-04-01T15:04:13Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 01, 2025 at 01:43:37PM +0200, Patrick Steinhardt wrote:\n> I'll send a patch series later this week.\n\nSent now via [1].\n\nPatrick\n\n[1]: <20250401-b4-pks-parse-options-integers-v1-0-a628ad40c3b4@pks.im>\n"}]}