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

[Junio C Hamano <gitster@pobox.com>]

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.

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

Thanks.