{"thread":{"id":"63328","subject":"[BUG]: backfill min batch size test case failure on s390x","startedAt":"2025-04-22T12:31:23Z","lastAt":"2025-04-22T13:53:54Z","messageCount":3,"participants":["Pranav P","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"516492","messageId":"BY5PR15MB35396BEBCAB2C559080F08C6A0BB2@BY5PR15MB3539.namprd15.prod.outlook.com","threadId":"63328","inReplyTo":null,"subject":"[BUG]: backfill min batch size test case failure on s390x","fromName":"Pranav P","fromEmail":"pranav.p7@ibm.com","sentAt":"2025-04-22T12:31:18Z","receivedAt":"2025-04-22T12:31:23Z","isPatch":false,"sender":{"key":"pranav.p7@ibm.com","avatar":null},"body":"Hi, \n \nWhen running `make test` on an s390x machine in Debian it is failing on 'do partial clone 2, backfill min batch size' \nReference: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1102106 \n \nAfter processing the command line arguments structure member min_batch_size should have had the value 20 \n \nInstead of having the value 20 (--min-batch-size=20) it was having a very large value \n \nmin_batch_size in `struct backfill_context` is of type `size_t` and since in the function cmd_backfill, in the \noptions struct it is passed on to OPT_INTEGER, which eventually causes \n \n``` \n*(int *)opt->value = strtol(arg, (char **)&s, 10); \n``` \nin parse-options.c line 188. This is writing the data in the first 4 bytes of min_batch_size and on big endian \nsystems this will lead min_batch_size to be a big number. This issue is immediately visible in little endian systems. \n \nChanging OPT_INTEGER to OPT_MAGNITUDE seems to be working on x86 and s390x \n \n``` \ndiff --git a/builtin/backfill.c b/builtin/backfill.c \nindex 18f9701487..33e1ea2f84 100644 \n--- a/builtin/backfill.c \n+++ b/builtin/backfill.c \n@@ -123,7 +123,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit \n .sparse = 0, \n }; \n struct option options[] = { \n- OPT_MAGNITUDE(0, \"min-batch-size\", &ctx.min_batch_size, \n+ OPT_INTEGER(0, \"min-batch-size\", &ctx.min_batch_size, \n N_(\"Minimum number of objects to request at a time\")), \n OPT_BOOL(0, \"sparse\", &ctx.sparse, \n N_(\"Restrict the missing objects to the current sparse-checkout\")), \n``` \n \nBut on systems where size_t which not be unsigned long, this might lead to an issue. \nSo, one other suggestion I have is to change the data type of min_batch_size from size_t to int. But I am not able to \ndetermine whether a practical upper bound for min_batch_size would exceed what an int variable can store. \nWith that clarification, I can a raise patch for the issue. \n \nPlease review the rest of the bug report below.\n\n[System Info] \ngit version: \ngit version 2.49.0.391.g4bbb303af6 \ncpu: s390x \nbuilt from commit: 4bbb303af69990ccd05fe3a2eb58a1ce036f8220 \nsizeof-long: 8 \nsizeof-size_t: 8 \nshell-path: /bin/sh \nlibcurl: 8.13.0 \nOpenSSL: OpenSSL 3.4.1 11 Feb 2025 \nzlib: 1.3.1 \nSHA-1: SHA1_DC \nSHA-256: SHA256_BLK \nuname: Linux 6.1.0-31-s390x #1 SMP Debian 6.1.128-1 (2025-02-07) s390x \ncompiler info: gnuc: 14.2 \nlibc info: glibc: 2.41 \n$SHELL (typically, interactive shell): /bin/bash \n \n \n[Enabled Hooks]\n\nI am fairly new to opensource and was following the `git bugreport`. So I am extremely sorry for any lack of clarity in the report.\n\nThanks,\nPranav"},{"id":"516493","messageId":"BY5PR15MB35398BF51EAFC520421163AFA0BB2@BY5PR15MB3539.namprd15.prod.outlook.com","threadId":"63328","inReplyTo":"BY5PR15MB35396BEBCAB2C559080F08C6A0BB2@BY5PR15MB3539.namprd15.prod.outlook.com","subject":"Re: [BUG]: backfill min batch size test case failure on s390x","fromName":"Pranav P","fromEmail":"pranav.p7@ibm.com","sentAt":"2025-04-22T12:35:01Z","receivedAt":"2025-04-22T12:35:05Z","isPatch":false,"sender":{"key":"pranav.p7@ibm.com","avatar":null},"body":"Hi,\n\nSorry I had pasted the wrong git diff.\nThe corrected one is mentioned below.\n\ndiff --git a/builtin/backfill.c b/builtin/backfill.c\nindex 33e1ea2f84..18f9701487 100644\n--- a/builtin/backfill.c\n+++ b/builtin/backfill.c\n@@ -123,7 +123,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit\n .sparse = 0,\n };\n struct option options[] = {\n- OPT_INTEGER(0, \"min-batch-size\", &ctx.min_batch_size,\n+ OPT_MAGNITUDE(0, \"min-batch-size\", &ctx.min_batch_size,\n N_(\"Minimum number of objects to request at a time\")),\n OPT_BOOL(0, \"sparse\", &ctx.sparse,\n N_(\"Restrict the missing objects to the current sparse-checkout\")),\n--\n\nThanks,\nPranav\n\n________________________________________\nFrom: Pranav P\nSent: Tuesday, April 22, 2025 6:01 PM\nTo: git@vger.kernel.org\nSubject: [BUG]: backfill min batch size test case failure on s390x\n\nHi,\n\nWhen running `make test` on an s390x machine in Debian it is failing on 'do partial clone 2, backfill min batch size'\nReference: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1102106\n\nAfter processing the command line arguments structure member min_batch_size should have had the value 20\n\nInstead of having the value 20 (--min-batch-size=20) it was having a very large value\n\nmin_batch_size in `struct backfill_context` is of type `size_t` and since in the function cmd_backfill, in the\noptions struct it is passed on to OPT_INTEGER, which eventually causes\n\n```\n*(int *)opt->value = strtol(arg, (char **)&s, 10);\n```\nin parse-options.c line 188. This is writing the data in the first 4 bytes of min_batch_size and on big endian\nsystems this will lead min_batch_size to be a big number. This issue is immediately visible in little endian systems.\n\nChanging OPT_INTEGER to OPT_MAGNITUDE seems to be working on x86 and s390x\n\n```\ndiff --git a/builtin/backfill.c b/builtin/backfill.c\nindex 18f9701487..33e1ea2f84 100644\n--- a/builtin/backfill.c\n+++ b/builtin/backfill.c\n@@ -123,7 +123,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit\n .sparse = 0,\n };\n struct option options[] = {\n- OPT_MAGNITUDE(0, \"min-batch-size\", &ctx.min_batch_size,\n+ OPT_INTEGER(0, \"min-batch-size\", &ctx.min_batch_size,\n N_(\"Minimum number of objects to request at a time\")),\n OPT_BOOL(0, \"sparse\", &ctx.sparse,\n N_(\"Restrict the missing objects to the current sparse-checkout\")),\n```\n\nBut on systems where size_t which not be unsigned long, this might lead to an issue.\nSo, one other suggestion I have is to change the data type of min_batch_size from size_t to int. But I am not able to\ndetermine whether a practical upper bound for min_batch_size would exceed what an int variable can store.\nWith that clarification, I can a raise patch for the issue.\n\nPlease review the rest of the bug report below.\n\n[System Info]\ngit version:\ngit version 2.49.0.391.g4bbb303af6\ncpu: s390x\nbuilt from commit: 4bbb303af69990ccd05fe3a2eb58a1ce036f8220\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nlibcurl: 8.13.0\nOpenSSL: OpenSSL 3.4.1 11 Feb 2025\nzlib: 1.3.1\nSHA-1: SHA1_DC\nSHA-256: SHA256_BLK\nuname: Linux 6.1.0-31-s390x #1 SMP Debian 6.1.128-1 (2025-02-07) s390x\ncompiler info: gnuc: 14.2\nlibc info: glibc: 2.41\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n\nI am fairly new to opensource and was following the `git bugreport`. So I am extremely sorry for any lack of clarity in the report.\n\nThanks,\nPranav\n"},{"id":"516495","messageId":"aAefbU-MmY_734oB@pks.im","threadId":"63328","inReplyTo":"BY5PR15MB35396BEBCAB2C559080F08C6A0BB2@BY5PR15MB3539.namprd15.prod.outlook.com","subject":"Re: [BUG]: backfill min batch size test case failure on s390x","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-04-22T13:53:49Z","receivedAt":"2025-04-22T13:53:54Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nOn Tue, Apr 22, 2025 at 12:31:18PM +0000, Pranav P wrote:\n> Hi,\n>\n> When running `make test` on an s390x machine in Debian it is failing on 'do partial clone 2, backfill min batch size'\n> Reference: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1102106\n>\n> After processing the command line arguments structure member min_batch_size should have had the value 20\n>\n> Instead of having the value 20 (--min-batch-size=20) it was having a very large value\n>\n> min_batch_size in `struct backfill_context` is of type `size_t` and since in the function cmd_backfill, in the\n> options struct it is passed on to OPT_INTEGER, which eventually causes\n>\n> ```\n> *(int *)opt->value = strtol(arg, (char **)&s, 10);\n> ```\n> in parse-options.c line 188. This is writing the data in the first 4 bytes of min_batch_size and on big endian\n> systems this will lead min_batch_size to be a big number. This issue is immediately visible in little endian systems.\n>\n> Changing OPT_INTEGER to OPT_MAGNITUDE seems to be working on x86 and s390x\n\nYup, indeed.\n\n> ```\n> diff --git a/builtin/backfill.c b/builtin/backfill.c\n> index 18f9701487..33e1ea2f84 100644\n> --- a/builtin/backfill.c\n> +++ b/builtin/backfill.c\n> @@ -123,7 +123,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit\n> .sparse = 0,\n> };\n> struct option options[] = {\n> - OPT_MAGNITUDE(0, \"min-batch-size\", &ctx.min_batch_size,\n> + OPT_INTEGER(0, \"min-batch-size\", &ctx.min_batch_size,\n> N_(\"Minimum number of objects to request at a time\")),\n> OPT_BOOL(0, \"sparse\", &ctx.sparse,\n> N_(\"Restrict the missing objects to the current sparse-checkout\")),\n> ```\n>\n> But on systems where size_t which not be unsigned long, this might lead to an issue.\n> So, one other suggestion I have is to change the data type of min_batch_size from size_t to int. But I am not able to\n> determine whether a practical upper bound for min_batch_size would exceed what an int variable can store.\n> With that clarification, I can a raise patch for the issue.\n\nTrue, as well. Overall the current state is a bit unfortunate because\nit's so easy to get this wrong, and the compiler won't even spit out a\nwarning.\n\n> I am fairly new to opensource and was following the `git bugreport`. So I am extremely sorry for any lack of clarity in the report.\n\nNo need to be sorry, this bug report contains all the details we'd need\nand is of quite high quality :) The only thing is that we've already got\nthis bug reported via [1].\n\nThe underlying issue is addressed via [2], which has already been merged\nto `next`. It also fixes the underlying issue you observed with\ndifferent integer widths as well as with signedness.\n\nThanks!\n\nPatrick\n\n[1]: https://lore.kernel.org/git/89257ab82cd60d135cce02d51eacee7ec35c1c37.camel@physik.fu-berlin.de/\n[2]: https://lore.kernel.org/git/20250401-b4-pks-parse-options-integers-v1-0-a628ad40c3b4@pks.im/\n"}]}