On Fri, Mar 28, 2025 at 11:37:25AM -0400, Todd Zullinger wrote:

I wrote:

quoted

I don't have a lot of time to poke at this today, but I'll
make another test run on an s390x build host without that
patch, but where I can save the output and post it
somewhere.

For the Fedora packaging, it will be something like this:

    make -C t all || {
        (cd t && ./t5620-backfill.sh -ix);
        ./print-failed-test-output;
    }

The matches file is empty.

    $ ls -lhn batch-trace matches 
    -rw-r--r--. 1 1000 1000 31K Mar 28 11:09 batch-trace
    -rw-r--r--. 1 1000 1000   0 Mar 28 11:09 matches

The only match in batch-trace for promisor fetch_count is
from the previous test:

    $ grep -e '"category":"promisor","key":"fetch_count","value":' batch-trace
    {"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"}

The trash directory for the test run is here, in case anyone
wants to poke at it:

    https://tmz.fedorapeople.org/t5620-backfill-trash-dir.tar.gz

The full build log is available as well:

    https://tmz.fedorapeople.org/git-2.49.0-s390x-build.log

If you search for 'BEGIN BASE64 MESSAGE' in that, it
provides a command which can be used to extract the full
test-results directory.  That's used to get the output from
the build hosts where shell access isn't available.  I don't
know that it's got anything which isn't in the trash
directory tarball which I already extracted, but it's there
just in case.

Thanks for the additional information!

One thing I stumbled over: the `--min-batch-size` parameter is parsed
using `OPT_INTEGER()`, which expects the value pointer to point to an
integer. But we pass `struct backfill_context::min_batch_size`, which is
of type `size_t`. Maybe that's causing us to end up with an invalid
value?

Could you please check whether the below diff fixes the issue for you?
If so I can turn it into a proper patch.

Patrick

-- >8 --

diff --git a/builtin/backfill.c b/builtin/backfill.c
index 33e1ea2f84f..1dd0d746538 100644
--- a/builtin/backfill.c
+++ b/builtin/backfill.c

@@ -119,11 +119,11 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
 	struct backfill_context ctx = {
 		.repo = repo,
 		.current_batch = OID_ARRAY_INIT,
-		.min_batch_size = 50000,
 		.sparse = 0,
 	};
+	unsigned long min_batch_size = 50000;
 	struct option options[] = {
-		OPT_INTEGER(0, "min-batch-size", &ctx.min_batch_size,
+		OPT_MAGNITUDE(0, "min-batch-size", &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")),

@@ -140,6 +140,7 @@ int cmd_backfill(int argc, const char **argv, const char *prefix, struct reposit
 
 	if (ctx.sparse < 0)
 		ctx.sparse = core_apply_sparse_checkout;
+	ctx.min_batch_size = min_batch_size;
 
 	result = do_backfill(&ctx);
 	backfill_context_clear(&ctx);