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

>
> Christian Couder <christian.couder@gmail.com> 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).

> 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?

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