* [BUG]: backfill min batch size test case failure on s390x
@ 2025-04-22 12:31 Pranav P
2025-04-22 12:35 ` Pranav P
2025-04-22 13:53 ` Patrick Steinhardt
0 siblings, 2 replies; 3+ messages in thread
From: Pranav P @ 2025-04-22 12:31 UTC (permalink / raw)
To: git@vger.kernel.org
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
```
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.
Please review the rest of the bug report below.
[System Info]
git version:
git version 2.49.0.391.g4bbb303af6
cpu: s390x
built from commit: 4bbb303af69990ccd05fe3a2eb58a1ce036f8220
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
libcurl: 8.13.0
OpenSSL: OpenSSL 3.4.1 11 Feb 2025
zlib: 1.3.1
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
uname: Linux 6.1.0-31-s390x #1 SMP Debian 6.1.128-1 (2025-02-07) s390x
compiler info: gnuc: 14.2
libc info: glibc: 2.41
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
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.
Thanks,
Pranav
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [BUG]: backfill min batch size test case failure on s390x
2025-04-22 12:31 [BUG]: backfill min batch size test case failure on s390x Pranav P
@ 2025-04-22 12:35 ` Pranav P
2025-04-22 13:53 ` Patrick Steinhardt
1 sibling, 0 replies; 3+ messages in thread
From: Pranav P @ 2025-04-22 12:35 UTC (permalink / raw)
To: git@vger.kernel.org
Hi,
Sorry I had pasted the wrong git diff.
The corrected one is mentioned below.
diff --git a/builtin/backfill.c b/builtin/backfill.c
index 33e1ea2f84..18f9701487 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_INTEGER(0, "min-batch-size", &ctx.min_batch_size,
+ OPT_MAGNITUDE(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")),
--
Thanks,
Pranav
________________________________________
From: Pranav P
Sent: Tuesday, April 22, 2025 6:01 PM
To: git@vger.kernel.org
Subject: [BUG]: backfill min batch size test case failure on s390x
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
```
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.
Please review the rest of the bug report below.
[System Info]
git version:
git version 2.49.0.391.g4bbb303af6
cpu: s390x
built from commit: 4bbb303af69990ccd05fe3a2eb58a1ce036f8220
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
libcurl: 8.13.0
OpenSSL: OpenSSL 3.4.1 11 Feb 2025
zlib: 1.3.1
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
uname: Linux 6.1.0-31-s390x #1 SMP Debian 6.1.128-1 (2025-02-07) s390x
compiler info: gnuc: 14.2
libc info: glibc: 2.41
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
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.
Thanks,
Pranav
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [BUG]: backfill min batch size test case failure on s390x
2025-04-22 12:31 [BUG]: backfill min batch size test case failure on s390x Pranav P
2025-04-22 12:35 ` Pranav P
@ 2025-04-22 13:53 ` Patrick Steinhardt
1 sibling, 0 replies; 3+ messages in thread
From: Patrick Steinhardt @ 2025-04-22 13:53 UTC (permalink / raw)
To: Pranav P; +Cc: git@vger.kernel.org
Hi,
On Tue, Apr 22, 2025 at 12:31:18PM +0000, Pranav P wrote:
> 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.
> ```
> 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/
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-04-22 13:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-22 12:31 [BUG]: backfill min batch size test case failure on s390x Pranav P
2025-04-22 12:35 ` Pranav P
2025-04-22 13:53 ` Patrick Steinhardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).