Re: [PATCH] bundle-uri.c: Fix double increment in depth

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 21, 2024, 15:24 UTC
Message-ID: <CAP8UFD3jGVaO2JdKE0H22mf4E8YBxrEwuGqJuyeRGBVh==23NQ@mail.gmail.com>
In-Reply-To: <20240621092258.1557258-1-toon@iotcl.com>

On Fri, Jun 21, 2024 at 11:23 AM Toon Claes <toon@iotcl.com> wrote:

Show 5 quoted lines

>
> A bundle URI can serve a gitformat-bundle(5) or a bundle list. This
> plain text file is in the Git config format containing other bundle
> URIs. To avoid these bundle lists to nest too deep, we've set a limit
> with `max_bundle_uri_depth`.

Yeah, max_bundle_uri_depth seems to be hardcoded to 4.

> Although, when walk through the tree of

s/walk/walking/

> bundles, the current depth is incremented in download_bundle_list() and
> then calls download_bundle_to_file(), which also increments the depth.

s/and then calls/which then calls/

> Remove the increment in download_bundle_to_file().

The increment is removed by replacing:

    fetch_bundle_uri_internal( ..., ctx->depth + 1, ...)

with:

    fetch_bundle_uri_internal( ..., ctx->depth, ...)

in download_bundle_to_file(). Ok.

It looks like there is another similar call to that function like this:

fetch_bundle_uri_internal( ... , ctx.depth + 1, ... )

in fetch_bundles_by_token() though.

There ctx.depth is initialized to 0 before the call, so it looks like it could work, but fetch_bundle_uri_internal() can call fetch_bundle_list_in_config_format() which can call download_bundle_list() which, as we saw above, still increases the depth by 1.

So even if download_bundle_list() then calls download_bundle_to_file() without increasing the depth, I am not sure it works well in all cases. At least I think a bit more explanations might be needed.

Show 35 quoted lines

> +test_expect_success 'clone bundle list (file, above max depth)' '
> +       cat >bundle-list-1 <<-EOF &&
> +       [bundle]
> +               version = 1
> +               mode = any
> +
> +       [bundle "bundle-list-2"]
> +               uri = file://$(pwd)/bundle-list-2
> +       EOF
> +
> +       cat >bundle-list-2 <<-EOF &&
> +       [bundle]
> +               version = 1
> +               mode = any
> +
> +       [bundle "bundle-list-3"]
> +               uri = file://$(pwd)/bundle-list-3
> +       EOF
> +
> +       cat >bundle-list-3 <<-EOF &&
> +       [bundle]
> +               version = 1
> +               mode = any
> +
> +       [bundle "bundle-list-4"]
> +               uri = file://$(pwd)/bundle-list-4
> +       EOF
> +
> +       cat >bundle-list-4 <<-EOF &&
> +       [bundle]
> +               version = 1
> +               mode = any
> +
> +       [bundle "bundle-0"]
> +               uri = file://$(pwd)/clone-from/bundle-0.bundle

Is there a reason why it's not more like:

       [bundle "bundle-list-5"]
               uri = file://$(pwd)/bundle-list-5

> +       EOF

It looks like the above is the setup part of the following tests, so it could perhaps be moved into a separate `test_expect_success 'setup deep clone bundle list'` test.

Show 24 quoted lines

> +       git clone --bundle-uri="file://$(pwd)/bundle-list-1" \
> +               clone-from clone-too-deep 2>err &&
> +       ! grep "fatal" err &&
> +       grep "warning: exceeded bundle URI recursion limit" err &&
> +
> +       git -C clone-from for-each-ref --format="%(objectname)" >oids &&
> +       git -C clone-too-deep cat-file --batch-check <oids &&
> +
> +       git -C clone-too-deep for-each-ref --format="%(refname)" >refs &&
> +       ! grep "refs/bundles/" refs
> +'
> +
> +test_expect_success 'clone bundle list (file, below max depth)' '
> +       git clone --bundle-uri="file://$(pwd)/bundle-list-2" \
> +               clone-from clone-max-depth 2>err &&
> +       ! grep "fatal" err &&
> +       ! grep "warning: exceeded bundle URI recursion limit" err &&
> +
> +       git -C clone-from for-each-ref --format="%(objectname)" >oids &&
> +       git -C clone-max-depth cat-file --batch-check <oids &&
> +
> +       git -C clone-max-depth for-each-ref --format="%(refname)" >refs &&
> +       ! grep "refs/bundles/" refs
> +'

Thanks!