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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 21, 2024 at 11:23 AM Toon Claes <toon@xxxxxxxxx> wrote:
>
> 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.

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

> +       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!





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux