Re: [PATCH v4 5/5] builtin/upload-pack: don't disable lazy fetching on trusted repo

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

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

>  Documentation/config/uploadpack.adoc  |  49 +++++++++
>  Documentation/git-upload-pack.adoc    |   5 +
>  Documentation/git.adoc                |   4 +-
>  builtin/upload-pack.c                 |  19 +++-
>  t/t5710-promisor-remote-capability.sh | 142 ++++++++++++++++++++++++++
>  5 files changed, 217 insertions(+), 2 deletions(-)

The diffstat above is pleasing to see, with ample documentation to
help users, tests with (hopefully) reasonable coverage, and a
minimal amount of actual code changes to enable the feature, thanks
to the preparatory work done in earlier steps.

> +uploadpack.lazyFetchTrusted::
> +	A multi-valued configuration variable, each of which contains the
> +	absolute local path of a repository that `upload-pack` is allowed to
> +	lazily fetch missing objects for.

"each of which" lacks a plural noun to modify.  Perhaps

	each value of which specifies the absolute local path of a
	repository from which upload-pack is allowed to lazily fetch
	missing objects.

> ++
> +A repository is identified by its git directory, i.e. the `.git`

"i.e." -> "i.e.," (similarly "e.g." -> "e.g.," below).

> diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
> index 32831fb879..53e76deb23 100644
> --- a/builtin/upload-pack.c
> +++ b/builtin/upload-pack.c
> @@ -46,7 +46,6 @@ int cmd_upload_pack(int argc,
>  	packet_trace_identity("upload-pack");
>  	disable_replace_refs();
>  	save_commit_buffer = 0;
> -	xsetenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 0);
>  
>  	argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0);
>  
> @@ -62,6 +61,24 @@ 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);
>  
> +	/*
> +	 * Lazily fetching while serving a client would run `git fetch`,
> +	 * which may execute arbitrary commands from the configuration
> +	 * and hooks of the served repo, so we disable it by default as
> +	 * we trust nobody. There are two ways for a server operator to
> +	 * allow it though:
> +	 *
> +	 *   - if GIT_NO_LAZY_FETCH is already set, we leave it alone and
> +	 *     honor whatever the operator put there,
> +	 *
> +	 *   - otherwise, if the served repo is in the
> +	 *     "uploadpack.lazyFetchTrusted" protected allowlist, we
> +	 *     don't disable lazy fetching.
> +	 */
> +	if (!getenv(NO_LAZY_FETCH_ENVIRONMENT) &&
> +	    !upload_pack_lazy_fetch_trusted(the_repository))
> +		xsetenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 1);
> +

OK, the logic is so trivially obvious and clear that it wouldn't
even need the above comment.  Very nice.