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

Christian Couder <[email protected]> writes:

> diff --git a/Documentation/config/uploadpack.adoc b/Documentation/config/uploadpack.adoc
> index 0e1dda944a..e960879c16 100644
> --- a/Documentation/config/uploadpack.adoc
> +++ b/Documentation/config/uploadpack.adoc
> @@ -86,3 +86,45 @@ uploadpack.allowRefInWant::
>  	is intended for the benefit of load-balanced servers which may
>  	not have the same view of what OIDs their refs point to due to
>  	replication delay.
> +
> +uploadpack.lazyFetchTrusted::
> +	These config entries specify repositories that `upload-pack` is

To somebody who designed this mechanism, it may have been clear that
you are talking about multi-valued configuration variable, i.e.,

	[uploadpack]
		lazyFetchTrusted = repo1
		lazyFetchTrusted = repo2
		...
		lazyFetchTrusted = repoN
		
but the "config entries specify repositories" can be misread to mean

	[uploadpack]
		lazyFetchTrusted = repo1 repo2 ... repoN

especially combined with the use of verb "list" in "Listing a
repository here tells..." we see below.

	A multi-valued configuration variable, each of which names a
	repository that `upload-pack` is allowed to ...

or something, perhaps.  Say that upfront to make sure readers won't
waste their time wondering what the syntax is.

Also, how would one specify a repository?  A URL?  Remote nickname
used in

	[remote "nick"] url = ...

configuration?  Local directory that houses another repository?
Something else?

> +	allowed to lazily fetch missing objects for. By default,
> +	`upload-pack` refuses to lazily fetch (see the description of the
> +	`GIT_NO_LAZY_FETCH` environment variable in
> +	linkgit:git-upload-pack[1]), because doing so would run `git fetch`,
> +	which may execute arbitrary commands specified in the configuration
> +	and hooks of the served repository. Listing a repository here tells
> +	`upload-pack` that it is trusted, so lazy fetching from the promisor
> +	remotes configured in it is allowed. This is equivalent to setting
> +	`GIT_NO_LAZY_FETCH` to `0` for the matching repositories. An
> +	explicitly set `GIT_NO_LAZY_FETCH` takes precedence over this
> +	setting.

It would be interesting to set it to point at itself.  A client asks
you to serve a pack, you find some objects you yourself do not have
because you fetched lazily from the upstream, and you end up asking
you if you have that object (U+1F61B Face with Stuck-Out Tongue 😛).

> +Note that this allows lazy fetching from any promisor remote
> +configured in the served repository, not only from the promisor
> +remotes that the client accepted using the "promisor-remote" protocol
> +v2 capability (see linkgit:gitprotocol-v2[5]). The served repository
> +is trusted as a whole, including its configuration, so the promisor
> +remotes it configures are trusted too. It is the server operator's
> +responsibility to make sure that the promisor remotes of a trusted
> +repository are also trustworthy.
> ++
> +This is a multi-valued setting, i.e. you can add more than one
> +repository via `git config (--global|--system) --add`. To reset the
> +list of trusted repositories (e.g. to override any such repositories
> +specified in the system config), add a `uploadpack.lazyFetchTrusted`

a -> an before `uploadpack.lazyFetchTrusted`.

> +entry with an empty value.