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

Christian Couder [off-list ref] writes:

On Fri, Jun 21, 2024 at 11:23 AM Toon Claes [off-list ref] wrote:

quoted

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.

quoted

Although, when walk through the tree of

s/walk/walking/

While you are typofixing ...

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

... also "Fix" -> "fix".

quoted

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/

quoted

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.

I have to wonder if the code should pass the whole ctx around,
instead of passing depth separately, and increment it at the single
place that matters, in order to reduce the chance of similar problem
happening.  The place that matters the recursion depth can be the
download_bundle_list() function---that is the one that controls the
recursion, and it is incrementing the depth for the calls it makes
(via the for_all_* callback mechanism).  Alternatively, it can be
the fetch_bundle_uri_internal() function where actual copying, for
which we do want to enforce the depth limit, happens.  The function
even has the code for depth limit, so having an increment next to it
may make it more readable and understandable.

So instead of taking ctx->r, ctx->depth+1, and ctx->list separately,
shouldn't fetch_bundle_uri_internal() take the whole ctx and use
ctx->depth (not +1---incrementing it is not its business) and the
whole (current and future) problem like this goes away, no?