From: Christian Couder Date: Mon, 28 Sep 2026 13:42:45 GMT Subject: Re: [PATCH v3 5/5] builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo Message-ID: In-Reply-To: On Tue, Sep 8, 2026 at 8:34 PM Junio C Hamano wrote: > > Christian Couder writes: > > > 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). I have tried to improve on that in v4 by rewording the title and commit message. > 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 should be fixed in v4, see below. > > 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? I think that by listing a repo in uploadpack.lazyFetchTrusted, the operator vouches for the following: - the repo's configuration and hooks, because git fetch will execute them, - the promisor remotes it is configured to lazily fetch from, because objects will come from there. I have tried to clarify this in v4 with the following in the commit message: + Note that what a server operator vouches for by listing a repo there + is that the promisor remotes this repo is configured to lazily fetch + from, as well as its configuration and hooks, are trustworthy. Whether + a client trusts the repo it fetches from is a separate matter, and up + to the client. > > 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. > > > 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 ... > > > 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. Yes, that's what is implemented in v4. The three possibilities are also listed in the commit message now. > Other than that, this is a great endgame of the series. Thanks.