git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [BUG]: backfill min batch size test case failure on s390x

From
Patrick Steinhardt <ps@pks.im>
Date
Apr 22, 2025, 13:53 UTC
Message-ID
<aAefbU-MmY_734oB@pks.im>
In-Reply-To
<BY5PR15MB35396BEBCAB2C559080F08C6A0BB2@BY5PR15MB3539.namprd15.prod.outlook.com>
Hi,
On Tue, Apr 22, 2025 at 12:31:18PM +0000, Pranav P wrote:
Show 19 quoted lines
> Hi,
>
> When running `make test` on an s390x machine in Debian it is failing on 'do partial clone 2, backfill min batch size'
> Reference: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1102106
>
> After processing the command line arguments structure member min_batch_size should have had the value 20
>
> Instead of having the value 20 (--min-batch-size=20) it was having a very large value
>
> min_batch_size in `struct backfill_context` is of type `size_t` and since in the function cmd_backfill, in the
> options struct it is passed on to OPT_INTEGER, which eventually causes
>
> ```
> *(int *)opt->value = strtol(arg, (char **)&s, 10);
> ```
> in parse-options.c line 188. This is writing the data in the first 4 bytes of min_batch_size and on big endian
> systems this will lead min_batch_size to be a big number. This issue is immediately visible in little endian systems.
>
> Changing OPT_INTEGER to OPT_MAGNITUDE seems to be working on x86 and s390x
Yup, indeed.
Show 20 quoted lines
> ```
> diff --git a/builtin/backfill.c b/builtin/backfill.c
> index 18f9701487..33e1ea2f84 100644
> --- a/builtin/backfill.c
> +++ b/builtin/backfill.c
> @@ -123,7 +123,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
>                 .sparse = 0,
>         };
>         struct option options[] = {
> -               OPT_MAGNITUDE(0, "min-batch-size", &ctx.min_batch_size,
> +               OPT_INTEGER(0, "min-batch-size", &ctx.min_batch_size,
>                             N_("Minimum number of objects to request at a time")),
>                 OPT_BOOL(0, "sparse", &ctx.sparse,
>                          N_("Restrict the missing objects to the current sparse-checkout")),
> ```
>
> But on systems where size_t which not be unsigned long, this might lead to an issue.
> 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
> determine whether a practical upper bound for min_batch_size would exceed what an int variable can store.
> With that clarification, I can a raise patch for the issue.

True, as well. Overall the current state is a bit unfortunate because it's so easy to get this wrong, and the compiler won't even spit out a warning.

> 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.

No need to be sorry, this bug report contains all the details we'd need and is of quite high quality :) The only thing is that we've already got this bug reported via [1].

The underlying issue is addressed via [2], which has already been merged to `next`. It also fixes the underlying issue you observed with different integer widths as well as with signedness.

Thanks!
Patrick

[1]: https://lore.kernel.org/git/89257ab82cd60d135cce02d51eacee7ec35c1c37.camel@physik.fu-berlin.de/ [2]: https://lore.kernel.org/git/20250401-b4-pks-parse-options-integers-v1-0-a628ad40c3b4@pks.im/

Previous: Pranav P
Message 3 of 3 in “[BUG]: backfill min batch size test case failure on s390x”
  1. Pranav PApr 22, 2025
  2. Pranav PApr 22, 2025
  3. Patrick SteinhardtApr 22, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.