Re: [PATCH v3 5/5] builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2026, 18:34 UTC
Message-ID: <xmqqmrtrwq0k.fsf@gitster.g>
In-Reply-To: <20260908164129.560396-6-christian.couder@gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 9 quoted lines

> A previous commit added a new "uploadpack.lazyFetchTrusted" protected
> config variable that can contain an allowlist of repos, as well as
> functions to check if the current repo is in that list. But when the
> current repo is in that list, we currently do nothing.
>
> Let's instead set `GIT_NO_LAZY_FETCH` to `0`, which allows
> `upload-pack` and its `pack-objects` child process to lazily fetch the
> objects they need to serve a client, for example when the filter used
> by the client and the one used by the server don't match.

While I agree that it is a good idea to make it more lenient to work with remotes that are explicitly marked as trusted, it somehow feels a bit unnatural for a configuration variable, or a conclusion derived from the setting of a configuration variable, overriding an environment variable. Who is setting this environment variable in the first place?

If NO_LAZY_FETCH is what server operators set and export, I strongly suspect that not honoring it merely because the new variable could be used to give them a finer-grained control would be very surprising experience for them.

If the answer is "this never comes from the end-user or the server operator. We used to automatically set NO_LAZY_FETCH from the process that spawns uploadpack because we trusted nobody", then I'd imagine that we would prefer to see that code that automatically sets NO_LAZY_FETCH to inspect the configuration variable and to decide not to do so.

And I think that is what the code is doing (in other words, from a cursory read, I think the new code is doing the right thing and it is just the way how the above is explained that I found it iffy). We used to say "when serving a client, we do not lazy fetch what we are missing from our promisor remotes by setting NO_LAZY_FETCH" and it was unconditional.

I think what we want to happen is:

 * If the server operator has NO_LAZY_FETCH set, we honor it and do
   not do anything.

 * If the server operator does not have NO_LAZY_FETCH set, then we
   see if the configuration variable is there, and if there is, we
   let it take care of which promisor remote to allow by not futzing
   with NO_LAZY_FETCH ourselves.

 * Otherwise, we set and export NO_LAZY_FETCH just we used to.

and what you have in the patch is close enough to that (you left the historical "disable lazy fetch upfront" so worst case you export the thing twice which is not necessary).

> This allows server operators to properly control lazy fetching. It is
> their responsibility, not the client's, to decide if the served repo is
> trusted,

If "the served repo" refers to where the client is fetching from, trusting that repository or not is up to the client; if they do not trust it, they should not be coming to you.

I may be misunderstanding what you are trying to say here, but what is up to the server operator to decide is if the promisor remotes, which the repo that is serving the client uses, is trustworthy, right?

Show 6 quoted lines

> As `GIT_NO_LAZY_FETCH` is passed down to child processes through the
> environment, this works for `pack-objects`, which performs the lazy
> fetch when serving a client, without any further plumbing.
>
> Now that "uploadpack.lazyFetchTrusted" is actually doing something,
> let's document it and reference it from GIT_NO_LAZY_FETCH's docs.

Show 16 quoted lines

> diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
> index 32831fb879..8b531ca724 100644
> --- a/builtin/upload-pack.c
> +++ b/builtin/upload-pack.c
> @@ -42,10 +42,13 @@ int cmd_upload_pack(int argc,
>  		OPT_END()
>  	};
>  	unsigned enter_repo_flags = ENTER_REPO_ANY_OWNER_OK;
> +	bool no_lazy_fetch_set;
>  
>  	packet_trace_identity("upload-pack");
>  	disable_replace_refs();
>  	save_commit_buffer = 0;
> +
> +	no_lazy_fetch_set = !!getenv(NO_LAZY_FETCH_ENVIRONMENT);
>  	xsetenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 0);

I am not seeing what is in the postcontext of this hunk and in the precontext of the next hunk, but I wonder if we can just remove this xsetenv (without "no_lazy_fetch_set" variable at all) here ...

Show 12 quoted lines

>  	argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);
> @@ -62,6 +65,14 @@ int cmd_upload_pack(int argc,
>  	if (!enter_repo(the_repository, dir, enter_repo_flags))
>  		die("'%s' does not appear to be a git repository", dir);
>  
> +	/*
> +	 * Relax the GIT_NO_LAZY_FETCH=1 default if the served repo is in
> +	 * the "uploadpack.lazyFetchTrusted" protected allowlist and
> +	 * GIT_NO_LAZY_FETCH was not already set explicitly.
> +	 */
> +	if (!no_lazy_fetch_set && upload_pack_lazy_fetch_trusted(the_repository))
> +		xsetenv(NO_LAZY_FETCH_ENVIRONMENT, "0", 1);

... and instead check the existing environment here, and do the choice from three possibilities I listed above here.

Other than that, this is a great endgame of the series.

Thanks.