From: Christian Couder <christian.couder@gmail.com>
Date: Wed, 09 Sep 2026 10:00:51 GMT
Subject: Re: [PATCH v3 4/5] promisor-remote: prevent infinite recursion when lazy fetching
Message-ID: <CAP8UFD0WUQX4ts_US2Ehdp7hBmEs1_ztjJiGJMYA2ek4awduMg@mail.gmail.com>
In-Reply-To: <xmqqqzj3wr24.fsf@gitster.g>

On Tue, Sep 8, 2026 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > It does not recurse forever in practice, but only because each level
> > adds one more variable to the environment of the child process, so
> > after a while `exec()` fails with:
> >
> >     fatal: cannot exec 'git-upload-pack ...': Argument list too long
> >     fatal: unable to fork
> >
> > To avoid this pathological case altogether, let's use a new
> > `GIT_INTERNAL_LAZY_FETCH_DEPTH` to count the recursion depth, and let's
> > check that it doesn't exceed a MAX_LAZY_FETCH_DEPTH limit (set to 5 for
> > now).
>
> Good.
>
> Does it have to be "unsigned long", though?  Just like oid_nr, I'd
> prefer to see a number whose range or signedness does not matter in
> practice be typed as platform natural "int".  Even though one could
> argue that "anything_nr cannot be negative so it must be unsigned",
> or "int might be too small for some platforms" or "int or ulong have
> different width on different platforms", or even "anything we count
> we should count in size_t", I do not think any of them is a good
> argument against it, especially when the value we start with is 5
> ;-).

I agree that using a plain "int" seems like the most straightforward,
but we don't have git_env_int() while we have git_env_ulong().

So would you be fine with something like:

    int depth = (int)git_env_ulong(LAZY_FETCH_DEPTH_ENVIRONMENT, 0);

which is similar to the following in builtin/pack-objects.c:

    name_hash_version = (int)git_env_ulong("GIT_TEST_NAME_HASH_VERSION", 1);

? Or do you think it's time to introduce git_env_int() in a preparatory patch?