[PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH

[PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH

[Christian Couder]

Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
2024-04-16), lazy fetching has been controlled by the
`GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
nothing" boolean that is set to 'true' by default when calling `git
upload-pack` for security reasons.

Recently the "promisor-remote" capability was added to protocol v2,
allowing servers and clients to agree on the promisor remotes they
can safely use.

This series leverages that capability to implement a pragmatic middle
ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
fetching is allowed only when fetching from promisor remotes that are
both advertised by the server and accepted by the client.

Note that using an environment variable for this is probably not the
best from a usability perspective. An `upload-pack.allowLazyFetch`
configuration variable would likely be better.

Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
things currently work. It would be a much bigger and more invasive
change to implement `upload-pack.allowLazyFetch` in a way that is
compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
backward compatibility. Therefore, transitioning to a configuration
variable is left for future work.

High level overview of the patches
==================================

Patch 1/3: A refactor which separates the fetching logic from the
error handling and validation logic. This might also slightly increase
performance if there are several promisor remotes.

Patch 2/3: A preparatory commit that transitions `GIT_NO_LAZY_FETCH`
from a strict boolean check into an enum that can support multiple
states.

Patch 3/3: Introduces the 'fromAccepted' option, taking advantage of
the previous preparatory commits.

CI tests
========

They all pass, see:

https://github.com/chriscool/git/actions/runs/29078195030


Christian Couder (3):
  promisor-remote: factor out lazy_fetch_objects()
  promisor-remote: introduce enum allow_lazy_fetch
  promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH

 Documentation/git-upload-pack.adoc    |   5 ++
 Documentation/git.adoc                |   6 +-
 promisor-remote.c                     | 110 ++++++++++++++++++--------
 promisor-remote.h                     |  14 ++++
 setup.c                               |   5 +-
 t/t5710-promisor-remote-capability.sh |  49 ++++++++++++
 6 files changed, 154 insertions(+), 35 deletions(-)

-- 
2.55.0.125.g395cd2c8ec.dirty

[PATCH 1/3] promisor-remote: factor out lazy_fetch_objects()

[Christian Couder]

In "promisor-remote.c:fetch_objects()", there is a check to disable
lazy fetching when the `GIT_NO_LAZY_FETCH` environment variable is
set. The fetch_objects() function is called once per promisor remote
though. So the check might be performed more times than necessary.

Also promisor_remote_get_direct() mixes up the logic deciding which
promisor remotes to try with the logic checking that the objects
that could not be fetched are promisor objects.

Let's refactor the lazy fetching logic out of these two functions
into a new lazy_fetch_objects() function. This will make it easier
to extend the lazy fetching logic in following commits.

This is a pure refactoring with no intended behavior change. Two
things shift in ways that are observably equivalent though:

  - the `GIT_NO_LAZY_FETCH` check is now performed once up front,
    instead of once per promisor remote, and

  - promisor_remote_init() is no longer called when lazy fetching
    is disabled, which is fine as nothing downstream of it, like
    is_promisor_object(), needs it in that case.

While at it, let's also convert try_promisor_remotes() to return
'bool' instead of 'int', as it just returns whether all the objects
could be fetched, and document its return value.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 76 ++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 31 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 43505d1e1a..65496c69cf 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -31,15 +31,6 @@ static int fetch_objects(struct repository *repo,
 	FILE *child_in;
 	int quiet;
 
-	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
-		static int warning_shown;
-		if (!warning_shown) {
-			warning_shown = 1;
-			warning(_("lazy fetching disabled; some objects may not be available"));
-		}
-		return -1;
-	}
-
 	child.git_cmd = 1;
 	child.in = -1;
 	if (repo != the_repository)
@@ -270,10 +261,15 @@ static int remove_fetched_oids(struct repository *repo,
 	return remaining_nr;
 }
 
-static int try_promisor_remotes(struct repository *repo,
-				struct object_id **remaining_oids,
-				int *remaining_nr, int *to_free,
-				bool accepted_only)
+/*
+ * Return 'true' if all the objects could be fetched from the
+ * (non-)accepted remotes, 'false' otherwise.
+ */
+static bool try_promisor_remotes(struct repository *repo,
+				 struct object_id **remaining_oids,
+				 int *remaining_nr,
+				 int *to_free,
+				 bool accepted_only)
 {
 	struct promisor_remote *r = repo->promisor_remote_config->promisors;
 
@@ -290,9 +286,37 @@ static int try_promisor_remotes(struct repository *repo,
 				continue;
 			}
 		}
-		return 1; /* all fetched */
+		return true; /* all fetched */
 	}
-	return 0;
+	return false;
+}
+
+/*
+ * Return 'true' if all the objects could be fetched, 'false' otherwise.
+ */
+static bool lazy_fetch_objects(struct repository *repo,
+			       struct object_id **remaining_oids,
+			       int *remaining_nr,
+			       int *to_free)
+{
+	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+		static int warning_shown;
+		if (!warning_shown) {
+			warning_shown = 1;
+			warning(_("lazy fetching disabled; some objects may not be available"));
+		}
+		return false;
+	}
+
+	promisor_remote_init(repo);
+
+	/* Try accepted remotes first (those the server told us to use) */
+	if (try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				 to_free, true))
+		return true;
+
+	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				    to_free, false);
 }
 
 void promisor_remote_get_direct(struct repository *repo,
@@ -302,28 +326,18 @@ void promisor_remote_get_direct(struct repository *repo,
 	struct object_id *remaining_oids = (struct object_id *)oids;
 	int remaining_nr = oid_nr;
 	int to_free = 0;
-	int i;
 
 	if (oid_nr == 0)
 		return;
 
-	promisor_remote_init(repo);
-
-	/* Try accepted remotes first (those the server told us to use) */
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, true))
-		goto all_fetched;
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, false))
-		goto all_fetched;
-
-	for (i = 0; i < remaining_nr; i++) {
-		if (is_promisor_object(repo, &remaining_oids[i]))
-			die(_("could not fetch %s from promisor remote"),
-			    oid_to_hex(&remaining_oids[i]));
+	if (!lazy_fetch_objects(repo, &remaining_oids, &remaining_nr, &to_free)) {
+		for (int i = 0; i < remaining_nr; i++) {
+			if (is_promisor_object(repo, &remaining_oids[i]))
+				die(_("could not fetch %s from promisor remote"),
+				    oid_to_hex(&remaining_oids[i]));
+		}
 	}
 
-all_fetched:
 	if (to_free)
 		free(remaining_oids);
 }
-- 
2.55.0.125.g395cd2c8ec.dirty

[PATCH 2/3] promisor-remote: introduce enum allow_lazy_fetch

[Christian Couder]

The `GIT_NO_LAZY_FETCH` environment variable is currently parsed as
a Boolean, using git_env_bool(), in both "setup.c" and
"promisor-remote.c".

In a following commit, we are going to allow a third value for this
variable, on top of 'true' and 'false'.

To prepare for that, let's introduce an `enum allow_lazy_fetch` with
the possible results of parsing the variable, along with a
parse_allow_lazy_fetch_env() function to parse it, and let's use them
everywhere the variable is parsed.

Note that, as before, an invalid value makes us die(), only the error
message changes from "bad boolean environment value ..." to "bad
environment value ...".

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 24 +++++++++++++++++++++++-
 promisor-remote.h | 13 +++++++++++++
 setup.c           |  5 ++++-
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 65496c69cf..56f57c5267 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -21,6 +21,26 @@ struct promisor_remote_config {
 	struct promisor_remote **promisors_tail;
 };
 
+enum allow_lazy_fetch parse_allow_lazy_fetch_env(void)
+{
+	const char *v = getenv(NO_LAZY_FETCH_ENVIRONMENT);
+	int val;
+
+	if (!v)
+		return LAZY_FETCH_ALL;
+
+	val = git_parse_maybe_bool(v);
+
+	if (!val)
+		return LAZY_FETCH_ALL;
+	if (val > 0)
+		return LAZY_FETCH_NONE;
+
+	die(_("bad environment value '%s' for '%s'; "
+	      "only 'false/0' and 'true/1' are valid"),
+	    v, NO_LAZY_FETCH_ENVIRONMENT);
+}
+
 static int fetch_objects(struct repository *repo,
 			 const char *remote_name,
 			 const struct object_id *oids,
@@ -299,7 +319,9 @@ static bool lazy_fetch_objects(struct repository *repo,
 			       int *remaining_nr,
 			       int *to_free)
 {
-	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+	enum allow_lazy_fetch lf = parse_allow_lazy_fetch_env();
+
+	if (lf == LAZY_FETCH_NONE) {
 		static int warning_shown;
 		if (!warning_shown) {
 			warning_shown = 1;
diff --git a/promisor-remote.h b/promisor-remote.h
index 301f5ac5cb..87fc24c9eb 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -25,6 +25,19 @@ void promisor_remote_clear(struct promisor_remote_config *config);
 struct promisor_remote *repo_promisor_remote_find(struct repository *r, const char *remote_name);
 int repo_has_promisor_remote(struct repository *r);
 
+/* Enum for lazy fetching parsing */
+enum allow_lazy_fetch {
+	LAZY_FETCH_NONE    = 0,  /* No lazy fetching */
+	LAZY_FETCH_ALL           /* Lazy fetch from any promisor remotes */
+};
+
+/*
+ * Parse the NO_LAZY_FETCH_ENVIRONMENT env variable into an
+ * `enum allow_lazy_fetch`.
+ * If parsing fails, then die().
+ */
+enum allow_lazy_fetch parse_allow_lazy_fetch_env(void);
+
 /*
  * Fetches all requested objects from all promisor remotes, trying them one at
  * a time until all objects are fetched.
diff --git a/setup.c b/setup.c
index 0de56a074f..0a81d9f045 100644
--- a/setup.c
+++ b/setup.c
@@ -24,6 +24,7 @@
 #include "trace.h"
 #include "trace2.h"
 #include "worktree.h"
+#include "promisor-remote.h"
 
 enum allowed_bare_repo {
 	ALLOWED_BARE_REPO_EXPLICIT = 0,
@@ -1051,6 +1052,7 @@ static void setup_git_env_internal(struct repository *repo,
 	const char *replace_ref_base;
 	struct set_gitdir_args args = { NULL };
 	struct strvec to_free = STRVEC_INIT;
+	enum allow_lazy_fetch lf;
 
 	args.commondir = getenv_safe(&to_free, GIT_COMMON_DIR_ENVIRONMENT);
 	args.graft_file = getenv_safe(&to_free, GRAFT_ENVIRONMENT);
@@ -1072,7 +1074,8 @@ static void setup_git_env_internal(struct repository *repo,
 	if (shallow_file)
 		set_alternate_shallow_file(repo, shallow_file, 0);
 
-	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0))
+	lf = parse_allow_lazy_fetch_env();
+	if (lf == LAZY_FETCH_NONE)
 		fetch_if_missing = 0;
 }
 
-- 
2.55.0.125.g395cd2c8ec.dirty

[PATCH 3/3] promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH

[Christian Couder]

The `GIT_NO_LAZY_FETCH` environment variable can be set to 'true' or
'false' to enable or disable lazy fetching. By default it is set to
'true' when calling `git upload-pack` to avoid security issues, see
7b70e9efb1 (upload-pack: disable lazy-fetching by default, 2024-04-16).

Recently though, the "promisor-remote" capability was introduced into
protocol v2, which allows a server to advertise some promisor remotes
and clients to accept them or not.

When promisor remotes are advertised by the server and accepted by the
client, it means that they are quite trusted. So the security risks
which come from lazy fetching from them could be considered much more
acceptable.

Let's introduce a 'fromAccepted' option on top of 'true' and 'false'
for `GIT_NO_LAZY_FETCH` to allow lazy fetching only from accepted
promisor remotes.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-upload-pack.adoc    |  5 +++
 Documentation/git.adoc                |  6 ++--
 promisor-remote.c                     | 14 +++++++-
 promisor-remote.h                     |  1 +
 t/t5710-promisor-remote-capability.sh | 49 +++++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-upload-pack.adoc b/Documentation/git-upload-pack.adoc
index 9167a321d0..1c2ed9d7ba 100644
--- a/Documentation/git-upload-pack.adoc
+++ b/Documentation/git-upload-pack.adoc
@@ -71,6 +71,11 @@ This is implemented by having `upload-pack` internally set the
 (because you are fetching from a partial clone, and you are sure
 you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to
 `0`.
++
+`GIT_NO_LAZY_FETCH` can also be set to 'fromAccepted' which allows
+lazy fetching only from remotes that are advertised and accepted using
+the "promisor-remote" protocol v2 capability. See
+linkgit:gitprotocol-v2[5]. This is safer than setting it to `0`.
 
 SECURITY
 --------
diff --git a/Documentation/git.adoc b/Documentation/git.adoc
index 8a5cdd3b3d..14a083bcdb 100644
--- a/Documentation/git.adoc
+++ b/Documentation/git.adoc
@@ -947,9 +947,9 @@ for full details.
 	pathspecs as case-insensitive.
 
 `GIT_NO_LAZY_FETCH`::
-	Setting this Boolean environment variable to true tells Git
-	not to lazily fetch missing objects from the promisor remote
-	on demand.
+	Setting this environment variable controls whether Git is
+	allowed to lazily fetch missing objects from a promisor remote
+	on demand. See linkgit:git-upload-pack[1].
 
 `GIT_REFLOG_ACTION`::
 	When a ref is updated, reflog entries are created to keep
diff --git a/promisor-remote.c b/promisor-remote.c
index 56f57c5267..c80319f966 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -35,9 +35,11 @@ enum allow_lazy_fetch parse_allow_lazy_fetch_env(void)
 		return LAZY_FETCH_ALL;
 	if (val > 0)
 		return LAZY_FETCH_NONE;
+	if (!strcasecmp(v, "fromAccepted"))
+		return LAZY_FETCH_ACCEPTED;
 
 	die(_("bad environment value '%s' for '%s'; "
-	      "only 'false/0' and 'true/1' are valid"),
+	      "only 'false/0', 'true/1' and 'fromAccepted' are valid"),
 	    v, NO_LAZY_FETCH_ENVIRONMENT);
 }
 
@@ -337,6 +339,16 @@ static bool lazy_fetch_objects(struct repository *repo,
 				 to_free, true))
 		return true;
 
+	if (lf == LAZY_FETCH_ACCEPTED) {
+		static int warning_shown;
+		if (!warning_shown) {
+			warning_shown = 1;
+			warning(_("lazy fetching from accepted promisor remotes only; "
+				  "some objects may not be available"));
+		}
+		return false;
+	}
+
 	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
 				    to_free, false);
 }
diff --git a/promisor-remote.h b/promisor-remote.h
index 87fc24c9eb..0d05ff9d84 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -28,6 +28,7 @@ int repo_has_promisor_remote(struct repository *r);
 /* Enum for lazy fetching parsing */
 enum allow_lazy_fetch {
 	LAZY_FETCH_NONE    = 0,  /* No lazy fetching */
+	LAZY_FETCH_ACCEPTED,     /* Lazy fetching only from accepted promisor remotes */
 	LAZY_FETCH_ALL           /* Lazy fetch from any promisor remotes */
 };
 
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 549acff23f..1c61b100b9 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -173,6 +173,55 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
 	initialize_server 1 "$oid"
 '
 
+test_expect_success "clone with GIT_NO_LAZY_FETCH=fromAccepted and accepted promisor remote" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client" &&
+
+	# Clone from server to create a client
+	GIT_NO_LAZY_FETCH=fromAccepted git clone -c remote.lop.promisor=true \
+		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+		-c promisor.acceptfromserver=All \
+		--no-local --filter="blob:limit=5k" server client &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone with GIT_NO_LAZY_FETCH=fromAccepted and no accepted promisor remote" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client" &&
+
+	# Clone from server to create a client
+	# It should fail because the server cannot lazy fetch the missing blob
+	test_must_fail env GIT_NO_LAZY_FETCH=fromAccepted git clone -c remote.lop.promisor=true \
+		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+		-c promisor.acceptfromserver=None \
+		--no-local --filter="blob:limit=5k" server client 2>err &&
+
+	test_grep "lazy fetching from accepted promisor remotes only" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "clone failure with GIT_NO_LAZY_FETCH=bogus" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client" &&
+
+	test_must_fail env GIT_NO_LAZY_FETCH=bogus git clone -c remote.lop.promisor=true \
+		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+		-c remote.lop.url="$TRASH_DIRECTORY_URL/lop" \
+		-c promisor.acceptfromserver=All \
+		--no-local --filter="blob:limit=5k" server client 2>err &&
+
+	test_grep "bad environment value" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
 test_expect_success "init + fetch with promisor.advertise set to 'true'" '
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
-- 
2.55.0.125.g395cd2c8ec.dirty

Re: [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

On 2026-07-10 at 08:51:34, Christian Couder wrote:
> Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
> 2024-04-16), lazy fetching has been controlled by the
> `GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
> nothing" boolean that is set to 'true' by default when calling `git
> upload-pack` for security reasons.
> 
> Recently the "promisor-remote" capability was added to protocol v2,
> allowing servers and clients to agree on the promisor remotes they
> can safely use.
> 
> This series leverages that capability to implement a pragmatic middle
> ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
> fetching is allowed only when fetching from promisor remotes that are
> both advertised by the server and accepted by the client.
> 
> Note that using an environment variable for this is probably not the
> best from a usability perspective. An `upload-pack.allowLazyFetch`
> configuration variable would likely be better.
> 
> Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
> things currently work. It would be a much bigger and more invasive
> change to implement `upload-pack.allowLazyFetch` in a way that is
> compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
> backward compatibility. Therefore, transitioning to a configuration
> variable is left for future work.

I don't think this is a good idea.  We get a lot of reports on the
security list involving various tooling that isn't within the scope of
our threat model.  This substantially increases the amount of code which
is now subject to that threat model and therefore our security
guarantees and I don't think we should do that as it stands, very
especially while so much of our network-facing code is written in C.

The fetch code by default reads lots of configuration information from
the repository, including remote settings and information and we really
want absolutely none of that code running in the context of an untrusted
repository.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 325 bytes --]

Re: [PATCH 0/3] Introduce a 'fromAccepted' option to GIT_NO_LAZY_FETCH

[Christian Couder]

On Fri, Jul 10, 2026 at 9:50 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2026-07-10 at 08:51:34, Christian Couder wrote:
> > Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
> > 2024-04-16), lazy fetching has been controlled by the
> > `GIT_NO_LAZY_FETCH` environment variable. This is currently an "all or
> > nothing" boolean that is set to 'true' by default when calling `git
> > upload-pack` for security reasons.
> >
> > Recently the "promisor-remote" capability was added to protocol v2,
> > allowing servers and clients to agree on the promisor remotes they
> > can safely use.
> >
> > This series leverages that capability to implement a pragmatic middle
> > ground. By setting `GIT_NO_LAZY_FETCH` to 'fromAccepted', lazy
> > fetching is allowed only when fetching from promisor remotes that are
> > both advertised by the server and accepted by the client.
> >
> > Note that using an environment variable for this is probably not the
> > best from a usability perspective. An `upload-pack.allowLazyFetch`
> > configuration variable would likely be better.
> >
> > Unfortunately the `GIT_NO_LAZY_FETCH` environment variable is the way
> > things currently work. It would be a much bigger and more invasive
> > change to implement `upload-pack.allowLazyFetch` in a way that is
> > compatible with `GIT_NO_LAZY_FETCH` which has to stay anyway for
> > backward compatibility. Therefore, transitioning to a configuration
> > variable is left for future work.
>
> I don't think this is a good idea.  We get a lot of reports on the
> security list involving various tooling that isn't within the scope of
> our threat model.  This substantially increases the amount of code which
> is now subject to that threat model and therefore our security
> guarantees and I don't think we should do that as it stands, very
> especially while so much of our network-facing code is written in C.

This small series doesn't change any defaults, especially
GIT_NO_LAZY_FETCH is still set to 1 when calling `git upload-pack` by
default. And the new option is more restrictive than the
GIT_NO_LAZY_FETCH=0 option which already exists.

So I don't think it's fair to say that this _substantially increases_
the amount of code subject to some threat model.

I agree that client acceptance of some promisor remotes doesn't make
the served repo trusted. It's a real concern, but I think it's
addressable by different mechanisms. See below.

> The fetch code by default reads lots of configuration information from
> the repository, including remote settings and information and we really
> want absolutely none of that code running in the context of an untrusted
> repository.

When a promisor remote has been accepted, it means both the client and
the server trust it, so at least the promisor remote is not untrusted.

Now the main security issue on the server side is making sure the
served repo itself is also trusted. And I agree that the operator of
the server should decide and mark that trust, not the client.

I also agree that on GitLab/GitHub-style multi-tenant hosts most
repositories shouldn't be marked as trusted.

However note that:

- The operator of the server is the only actor which can set
GIT_NO_LAZY_FETCH on the server (where it matters).
- In the case of corporate/self-hosted repos, the operator also
controls the repos.
- Different features could be developed (in future work) to improve on
the current state:
    - a way for lazy fetching to work without reading config files,
triggering hooks, or doing potentially sensitive things,
    - an explicit way for operators to mark trusted repos (like
perhaps a server-side config the operator sets per-repo),
    - operator-defined allow/deny rules, or maybe
    - some ways/scripts/commands to scan repos and check configuration
information, remote settings and everything potentially sensitive to
decide if a repo looks safe enough to allow lazy fetching or not.

I would be happy to hear opinions about those potential features or
any other ways to address the issue.

So I agree that this series doesn't fix all the problems on the server
side, but I think it's still valuable to be able to restrict lazy
fetching to accepted promisor remotes.

Also I definitely agree that the current series should have better
documentation about this, and I plan to improve on that in the v2 of
this series.

Thanks for your insightful comments.

[PATCH 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Christian Couder]

Recently the "promisor-remote" capability was added to protocol v2,
allowing servers and clients to agree on the promisor remotes they can
safely use.

The more servers use promisor remotes, the more it is important to
properly control if they can lazy fetch when responding to a clone or
fetch request from the client.

For example, in the context of large object promisors (see
"Documentation/technical/large-object-promisors.adoc"), if a client
clones with a filter set to 100kB while the server has moved all of
the blobs >= 10kB to a promisor remote, the server will not be able to
provide blobs between 10kB and 100kB to the client, which will make
the clone fail.

Even if the `--filter=auto` option is available since ef2f1845ec
(fetch-pack: wire up and enable auto filter logic, 2026-02-16) it's
still a good idea to provide more control over lazy fetching on the
server side to server operators, as lazy fetching on the server side
could be useful in corporate environments.

Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
2024-04-16), lazy fetching has been controlled by the
`GIT_NO_LAZY_FETCH` environment variable. This is a boolean that is
set to 'true' by default when calling `git upload-pack` for security
reasons.

The main security issue on the server side is making sure the served
repo itself is also trusted, as lazily fetching runs `git fetch`,
which may execute arbitrary commands specified in the configuration
and hooks of the served repo. The operator of the server should decide
and mark that trust, not the served repo itself, nor the client.

This series introduces a new 'uploadpack.lazyFetchTrusted' protected
configuration variable similar to 'safe.directory' (see
"Documentation/config/safe.adoc") to mark trusted repos where lazy
fetching is allowed. As it is protected, this config variable will
only take effect if it is set in global or system scope, so only
server operators can control it.

Previous related work
=====================

A previous series called "Introduce a 'fromAccepted' option to
GIT_NO_LAZY_FETCH" [1] took a different approach as it wanted to make
it easier to allow lazy fetching from accepted promisor remotes. But
after brian replied that he didn't think it was a good idea, and after
thinking about this more, my opinion now is that some promisor remotes
being accepted or not is not really relevant to the issue.

In my reply to brian, I said:

"""
Different features could be developed (in future work) to improve on
the current state:
    - a way for lazy fetching to work without reading config files,
triggering hooks, or doing potentially sensitive things,
    - an explicit way for operators to mark trusted repos (like
perhaps a server-side config the operator sets per-repo),
    - operator-defined allow/deny rules, or maybe
    - some ways/scripts/commands to scan repos and check configuration
information, remote settings and everything potentially sensitive to
decide if a repo looks safe enough to allow lazy fetching or not.
"""

So I decided to go with "an explicit way for operators to mark trusted
repos" and this series is an implementation of that.

Note that the feature developed in this series applies to protocol
v0/v1 as well as v2 while the previous one was only related to v2.

[1]: https://lore.kernel.org/git/CAP8UFD0_S9eg_w42tcNRnT9E2ntLr_eHLnzE4c2dSu67DzZoXg@mail.gmail.com/

Overview of the patches
=======================

  - Patch 1/5 is the only patch saved from the "Introduce a
    'fromAccepted' option to GIT_NO_LAZY_FETCH" series. It's not
    necessary for the rest of this series and its main feature to
    work, but I think it's a nice refactoring related to lazy
    fetching, so it might as well be part of this series. There is a
    small change in the commit message (to not mention following
    commits) compared to the version in the previous series.

  - Patches 2/5 and 3/5 extract and modify code used by the
    'safe.directory' config variable in a path_allowlist_apply()
    function, so that this function can be reused to process
    'uploadpack.lazyFetchTrusted' in the next patch.

  - Patch 4/5 actually uses path_allowlist_apply() from a new
    upload_pack_lazy_fetch_trusted() function to process
    'uploadpack.lazyFetchTrusted', but the result from that processing
    isn't actually used to have a practical effect.

  - Patch 5/5 wires up the new upload_pack_lazy_fetch_trusted()
    function to decide if lazy fetching can actually be enabled.

CI tests
========

They all pass, see:

https://github.com/chriscool/git/actions/runs/31171494296

Range diff with previous series
===============================

The range diff with the previous ("Introduce a 'fromAccepted' option
to GIT_NO_LAZY_FETCH") series is not very interesting as only the
first patch has been saved, but anyway here it is:

1:  8dd67ddaca ! 1:  b5b0836d19 promisor-remote: factor out lazy_fetch_objects()
    @@ Commit message
         that could not be fetched are promisor objects.
     
         Let's refactor the lazy fetching logic out of these two functions
    -    into a new lazy_fetch_objects() function. This will make it easier
    -    to extend the lazy fetching logic in following commits.
    +    into a new lazy_fetch_objects() function.
     
         This is a pure refactoring with no intended behavior change. Two
         things shift in ways that are observably equivalent though:
2:  314c61cbbe < -:  ---------- promisor-remote: introduce enum allow_lazy_fetch
3:  cb2f5447e2 < -:  ---------- promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH
-:  ---------- > 2:  879e3a34e3 setup: extract path_allowlist_apply()
-:  ---------- > 3:  98431ab7b3 setup: add 'allow_dot' arg to path_allowlist_apply()
-:  ---------- > 4:  a46f4c1bb8 upload-pack: read uploadpack.lazyFetchTrusted
-:  ---------- > 5:  4063f233aa builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo


Christian Couder (5):
  promisor-remote: factor out lazy_fetch_objects()
  setup: extract path_allowlist_apply()
  setup: add 'allow_dot' arg to path_allowlist_apply()
  upload-pack: read uploadpack.lazyFetchTrusted
  builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo

 Documentation/config/uploadpack.adoc  |  42 ++++++++++
 Documentation/git-upload-pack.adoc    |   5 ++
 Documentation/git.adoc                |   4 +-
 builtin/upload-pack.c                 |  11 +++
 promisor-remote.c                     |  76 ++++++++++--------
 setup.c                               | 108 ++++++++++++++------------
 setup.h                               |  28 +++++++
 t/t5710-promisor-remote-capability.sh |  70 +++++++++++++++++
 upload-pack.c                         |  37 +++++++++
 upload-pack.h                         |   3 +
 10 files changed, 304 insertions(+), 80 deletions(-)

-- 
2.55.0.530.gdb3615d990.dirty

[PATCH 1/5] promisor-remote: factor out lazy_fetch_objects()

[Christian Couder]

In "promisor-remote.c:fetch_objects()", there is a check to disable
lazy fetching when the `GIT_NO_LAZY_FETCH` environment variable is
set. The fetch_objects() function is called once per promisor remote
though. So the check might be performed more times than necessary.

Also promisor_remote_get_direct() mixes up the logic deciding which
promisor remotes to try with the logic checking that the objects
that could not be fetched are promisor objects.

Let's refactor the lazy fetching logic out of these two functions
into a new lazy_fetch_objects() function.

This is a pure refactoring with no intended behavior change. Two
things shift in ways that are observably equivalent though:

  - the `GIT_NO_LAZY_FETCH` check is now performed once up front,
    instead of once per promisor remote, and

  - promisor_remote_init() is no longer called when lazy fetching
    is disabled, which is fine as nothing downstream of it, like
    is_promisor_object(), needs it in that case.

While at it, let's also convert try_promisor_remotes() to return
'bool' instead of 'int', as it just returns whether all the objects
could be fetched, and document its return value.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 76 ++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 31 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 43505d1e1a..65496c69cf 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -31,15 +31,6 @@ static int fetch_objects(struct repository *repo,
 	FILE *child_in;
 	int quiet;
 
-	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
-		static int warning_shown;
-		if (!warning_shown) {
-			warning_shown = 1;
-			warning(_("lazy fetching disabled; some objects may not be available"));
-		}
-		return -1;
-	}
-
 	child.git_cmd = 1;
 	child.in = -1;
 	if (repo != the_repository)
@@ -270,10 +261,15 @@ static int remove_fetched_oids(struct repository *repo,
 	return remaining_nr;
 }
 
-static int try_promisor_remotes(struct repository *repo,
-				struct object_id **remaining_oids,
-				int *remaining_nr, int *to_free,
-				bool accepted_only)
+/*
+ * Return 'true' if all the objects could be fetched from the
+ * (non-)accepted remotes, 'false' otherwise.
+ */
+static bool try_promisor_remotes(struct repository *repo,
+				 struct object_id **remaining_oids,
+				 int *remaining_nr,
+				 int *to_free,
+				 bool accepted_only)
 {
 	struct promisor_remote *r = repo->promisor_remote_config->promisors;
 
@@ -290,9 +286,37 @@ static int try_promisor_remotes(struct repository *repo,
 				continue;
 			}
 		}
-		return 1; /* all fetched */
+		return true; /* all fetched */
 	}
-	return 0;
+	return false;
+}
+
+/*
+ * Return 'true' if all the objects could be fetched, 'false' otherwise.
+ */
+static bool lazy_fetch_objects(struct repository *repo,
+			       struct object_id **remaining_oids,
+			       int *remaining_nr,
+			       int *to_free)
+{
+	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+		static int warning_shown;
+		if (!warning_shown) {
+			warning_shown = 1;
+			warning(_("lazy fetching disabled; some objects may not be available"));
+		}
+		return false;
+	}
+
+	promisor_remote_init(repo);
+
+	/* Try accepted remotes first (those the server told us to use) */
+	if (try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				 to_free, true))
+		return true;
+
+	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				    to_free, false);
 }
 
 void promisor_remote_get_direct(struct repository *repo,
@@ -302,28 +326,18 @@ void promisor_remote_get_direct(struct repository *repo,
 	struct object_id *remaining_oids = (struct object_id *)oids;
 	int remaining_nr = oid_nr;
 	int to_free = 0;
-	int i;
 
 	if (oid_nr == 0)
 		return;
 
-	promisor_remote_init(repo);
-
-	/* Try accepted remotes first (those the server told us to use) */
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, true))
-		goto all_fetched;
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, false))
-		goto all_fetched;
-
-	for (i = 0; i < remaining_nr; i++) {
-		if (is_promisor_object(repo, &remaining_oids[i]))
-			die(_("could not fetch %s from promisor remote"),
-			    oid_to_hex(&remaining_oids[i]));
+	if (!lazy_fetch_objects(repo, &remaining_oids, &remaining_nr, &to_free)) {
+		for (int i = 0; i < remaining_nr; i++) {
+			if (is_promisor_object(repo, &remaining_oids[i]))
+				die(_("could not fetch %s from promisor remote"),
+				    oid_to_hex(&remaining_oids[i]));
+		}
 	}
 
-all_fetched:
 	if (to_free)
 		free(remaining_oids);
 }
-- 
2.55.0.530.gdb3615d990.dirty

[PATCH 2/5] setup: extract path_allowlist_apply()

[Christian Couder]

In a following commit we are going to check whether a repository is
part of an allowlist specified in a config variable.

To prepare for that let's extract existing code from
safe_directory_cb() into a new path_allowlist_apply() helper that will
help with such checks.

While at it let's make the helper's code simpler and more generic.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 setup.c | 107 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 59 insertions(+), 48 deletions(-)

diff --git a/setup.c b/setup.c
index 95909e9603..39dfa1cc5f 100644
--- a/setup.c
+++ b/setup.c
@@ -1339,6 +1339,64 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 	}
 }
 
+static void path_allowlist_apply(const char *key, const char *value,
+				 const char *target_path, int *is_match)
+{
+	char *allowed = NULL;
+	char *normalized = NULL;
+
+	if (!value || !*value) {
+		*is_match = 0;
+		return;
+	}
+
+	if (!strcmp(value, "*")) {
+		*is_match = 1;
+		return;
+	}
+
+	if (git_config_pathname(&allowed, key, value) || !allowed)
+		return;
+
+	/*
+	 * Setting the config variable to a non-absolute path makes
+	 * little sense---it won't be relative to the configuration
+	 * file the item is defined in.  Except for ".", which means
+	 * "if we are at the top level of a repository, then it is
+	 * OK", which is slightly tighter than "*" that allows
+	 * discovery.
+	 */
+	if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
+		warning(_("%s '%s' not absolute"), key, allowed);
+		goto end;
+	}
+
+	/*
+	 * A .gitconfig in $HOME may be shared across different
+	 * machines and the config variable entries may or may not
+	 * exist as paths on all of these machines.  In other words,
+	 * it is not a warning worthy event when there is no such path
+	 * on this machine---the entry may be useful elsewhere.
+	 */
+	normalized = real_pathdup(allowed, 0);
+	if (!normalized)
+		goto end;
+
+	if (ends_with(normalized, "/*")) {
+		size_t len = strlen(normalized);
+		if (!fspathncmp(normalized, target_path, len - 1))
+			*is_match = 1;
+		goto end;
+	}
+
+	if (!fspathcmp(target_path, normalized))
+		*is_match = 1;
+
+end:
+	free(normalized);
+	free(allowed);
+}
+
 struct safe_directory_data {
 	char *path;
 	int is_safe;
@@ -1352,54 +1410,7 @@ static int safe_directory_cb(const char *key, const char *value,
 	if (strcmp(key, "safe.directory"))
 		return 0;
 
-	if (!value || !*value) {
-		data->is_safe = 0;
-	} else if (!strcmp(value, "*")) {
-		data->is_safe = 1;
-	} else {
-		char *allowed = NULL;
-
-		if (!git_config_pathname(&allowed, key, value) && allowed) {
-			char *normalized = NULL;
-
-			/*
-			 * Setting safe.directory to a non-absolute path
-			 * makes little sense---it won't be relative to
-			 * the configuration file the item is defined in.
-			 * Except for ".", which means "if we are at the top
-			 * level of a repository, then it is OK", which is
-			 * slightly tighter than "*" that allows discovery.
-			 */
-			if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
-				warning(_("safe.directory '%s' not absolute"),
-					allowed);
-				goto next;
-			}
-
-			/*
-			 * A .gitconfig in $HOME may be shared across
-			 * different machines and safe.directory entries
-			 * may or may not exist as paths on all of these
-			 * machines.  In other words, it is not a warning
-			 * worthy event when there is no such path on this
-			 * machine---the entry may be useful elsewhere.
-			 */
-			normalized = real_pathdup(allowed, 0);
-			if (!normalized)
-				goto next;
-
-			if (ends_with(normalized, "/*")) {
-				size_t len = strlen(normalized);
-				if (!fspathncmp(normalized, data->path, len - 1))
-					data->is_safe = 1;
-			} else if (!fspathcmp(data->path, normalized)) {
-				data->is_safe = 1;
-			}
-		next:
-			free(normalized);
-			free(allowed);
-		}
-	}
+	path_allowlist_apply(key, value, data->path, &data->is_safe);
 
 	return 0;
 }
-- 
2.55.0.530.gdb3615d990.dirty

[PATCH 3/5] setup: add 'allow_dot' arg to path_allowlist_apply()

[Christian Couder]

A previous commit created path_allowlist_apply() with the goal of later
reusing that function. But when it will be reused in a following commit
this function will need to reject non-absolute paths including those
with a single dot that are currently accepted.

To prepare for reusing path_allowlist_apply(), let's add a
`bool allow_dot` argument to it, and let's export this function.

While at it let's document it properly in "setup.h".

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 setup.c |  9 +++++----
 setup.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 39dfa1cc5f..a09e697e3a 100644
--- a/setup.c
+++ b/setup.c
@@ -1339,8 +1339,9 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 	}
 }
 
-static void path_allowlist_apply(const char *key, const char *value,
-				 const char *target_path, int *is_match)
+void path_allowlist_apply(const char *key, const char *value,
+			  const char *target_path, int *is_match,
+			  bool allow_dot)
 {
 	char *allowed = NULL;
 	char *normalized = NULL;
@@ -1366,7 +1367,7 @@ static void path_allowlist_apply(const char *key, const char *value,
 	 * OK", which is slightly tighter than "*" that allows
 	 * discovery.
 	 */
-	if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
+	if (!is_absolute_path(allowed) && (!allow_dot || strcmp(allowed, "."))) {
 		warning(_("%s '%s' not absolute"), key, allowed);
 		goto end;
 	}
@@ -1410,7 +1411,7 @@ static int safe_directory_cb(const char *key, const char *value,
 	if (strcmp(key, "safe.directory"))
 		return 0;
 
-	path_allowlist_apply(key, value, data->path, &data->is_safe);
+	path_allowlist_apply(key, value, data->path, &data->is_safe, true);
 
 	return 0;
 }
diff --git a/setup.h b/setup.h
index 654f10e059..d4f8af5457 100644
--- a/setup.h
+++ b/setup.h
@@ -304,4 +304,32 @@ struct startup_info {
 extern struct startup_info *startup_info;
 extern const char *tmp_original_cwd;
 
+/*
+ * Apply the path allowlist in 'value' against 'target_path' setting
+ * '*is_match' accordingly.
+ *
+ * `value` is the value of a multi-valued config variable named `key`
+ * that holds an allowlist of paths. `target_path` is the (normalized)
+ * path being tested. `*is_match` is updated in place:
+ *
+ *   - an empty value resets it to 0 (so a later, more specific config
+ *     scope can clear entries from a broader one),
+ *   - "*" sets it to 1 (allow everything),
+ *   - "<path>" sets it to 1 if <path> equals `target_path`,
+ *   - "<path>" + "/" + "*" sets it to 1 if <path> is a leading
+ *     directory of `target_path`,
+ *   - any other (unmatching) value leaves `*is_match` unchanged.
+ *
+ * Non-absolute values are rejected with a warning, except "." when
+ * `allow_dot` is set (used by 'safe.directory' to mean "the top level
+ * of the current repository").
+ *
+ * Callers are expected to invoke this once per config value,
+ * typically from a protected-config callback, so that untrusted
+ * repository config cannot influence the decision.
+ */
+void path_allowlist_apply(const char *key, const char *value,
+			  const char *target_path, int *is_match,
+			  bool allow_dot);
+
 #endif /* SETUP_H */
-- 
2.55.0.530.gdb3615d990.dirty

[PATCH 4/5] upload-pack: read uploadpack.lazyFetchTrusted

[Christian Couder]

Previous commits created and prepared the path_allowlist_apply()
function.

Let's reuse this function for a new "uploadpack.lazyFetchTrusted"
configuration variable.

It allows us to:

  - read an allowlist from that config variable,
  - check if the current repo is in that list, and
  - return the result from a new upload_pack_lazy_fetch_trusted()
    function.

The new function will be used in a following commit.

Note that the new config variable should be read only from protected
configuration files.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 upload-pack.c | 37 +++++++++++++++++++++++++++++++++++++
 upload-pack.h |  3 +++
 2 files changed, 40 insertions(+)

diff --git a/upload-pack.c b/upload-pack.c
index a52856d869..29e700e43b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -34,6 +34,8 @@
 #include "json-writer.h"
 #include "strmap.h"
 #include "promisor-remote.h"
+#include "setup.h"
+#include "abspath.h"
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE	(1u << 11)
@@ -1378,6 +1380,41 @@ static int upload_pack_config(const char *var, const char *value,
 	return parse_hide_refs_config(var, value, "uploadpack", &data->hidden_refs);
 }
 
+struct lazy_fetch_trusted {
+	int trusted;
+	char *repo_path;
+};
+
+static int upload_pack_protected_lazy_fetch_config(const char *var, const char *value,
+						   const struct config_context *ctx UNUSED,
+						   void *cb_data)
+{
+	struct lazy_fetch_trusted *data = cb_data;
+
+	if (!strcmp("uploadpack.lazyfetchtrusted", var)) {
+		path_allowlist_apply(var, value, data->repo_path,
+				     &data->trusted, false);
+		return 0;
+	}
+
+	return 0;
+}
+
+bool upload_pack_lazy_fetch_trusted(struct repository *r)
+{
+	struct lazy_fetch_trusted data = { 0 };
+
+	data.repo_path = real_pathdup(r->worktree ? r->worktree : r->gitdir, 0);
+	if (!data.repo_path)
+		return false;
+
+	git_protected_config(upload_pack_protected_lazy_fetch_config, &data);
+
+	free(data.repo_path);
+
+	return !!data.trusted;
+}
+
 static int upload_pack_protected_config(const char *var, const char *value,
 					const struct config_context *ctx UNUSED,
 					void *cb_data)
diff --git a/upload-pack.h b/upload-pack.h
index d6ee25ea98..b2212992c3 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -12,4 +12,7 @@ struct strbuf;
 int upload_pack_advertise(struct repository *r,
 			  struct strbuf *value);
 
+/* Is this repo trusted for lazy fetching? */
+bool upload_pack_lazy_fetch_trusted(struct repository *r);
+
 #endif /* UPLOAD_PACK_H */
-- 
2.55.0.530.gdb3615d990.dirty

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

[Christian Couder]

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.

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, as the main security issue is that lazily fetching runs `git
fetch`, which may execute arbitrary commands specified in the
configuration and hooks of the served repo.

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.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/uploadpack.adoc  | 42 ++++++++++++++++
 Documentation/git-upload-pack.adoc    |  5 ++
 Documentation/git.adoc                |  4 +-
 builtin/upload-pack.c                 | 11 +++++
 t/t5710-promisor-remote-capability.sh | 70 +++++++++++++++++++++++++++
 5 files changed, 131 insertions(+), 1 deletion(-)

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
+	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.
++
+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`
+entry with an empty value.
++
+A repository is identified by its worktree, or its git directory for a bare
+repository, and the value must be an absolute path. Giving a path with `/*`
+appended to it will trust all repositories under the named directory. To trust
+all served repositories, set `uploadpack.lazyFetchTrusted` to the string `*`.
++
+The value of this setting is interpolated, i.e. `~/<path>` expands to a
+path relative to the home directory and `%(prefix)/<path>` expands to a
+path relative to Git's (runtime) prefix.
++
+Note that this configuration variable is only respected when it is specified
+in protected configuration (see <<SCOPES>>). This prevents untrusted
+repositories from tampering with this value.
diff --git a/Documentation/git-upload-pack.adoc b/Documentation/git-upload-pack.adoc
index 9167a321d0..90c2ba1194 100644
--- a/Documentation/git-upload-pack.adoc
+++ b/Documentation/git-upload-pack.adoc
@@ -71,6 +71,11 @@ This is implemented by having `upload-pack` internally set the
 (because you are fetching from a partial clone, and you are sure
 you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to
 `0`.
++
+Instead of setting `GIT_NO_LAZY_FETCH` to `0` in the environment, a
+server operator can allow lazy fetching on a per-repository basis by
+listing trusted repositories in the `uploadpack.lazyFetchTrusted`
+configuration variable. See linkgit:git-config[1].
 
 SECURITY
 --------
diff --git a/Documentation/git.adoc b/Documentation/git.adoc
index 8a5cdd3b3d..2e763d1f93 100644
--- a/Documentation/git.adoc
+++ b/Documentation/git.adoc
@@ -949,7 +949,9 @@ for full details.
 `GIT_NO_LAZY_FETCH`::
 	Setting this Boolean environment variable to true tells Git
 	not to lazily fetch missing objects from the promisor remote
-	on demand.
+	on demand. On the server side, the `uploadpack.lazyFetchTrusted`
+	configuration variable can control this per-repository. See
+	linkgit:git-upload-pack[1].
 
 `GIT_REFLOG_ACTION`::
 	When a ref is updated, reflog entries are created to keep
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);
 
 	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);
+
 	switch (determine_protocol_version_server()) {
 	case protocol_v2:
 		if (advertise_refs)
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 549acff23f..e6993f2761 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -173,6 +173,76 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
 	initialize_server 1 "$oid"
 '
 
+test_expect_success "clone with uploadpack.lazyFetchTrusted" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Clone without GIT_NO_LAZY_FETCH=0
+	git clone --no-local --filter="blob:limit=5k" server client &&
+
+	# Check that the largest object is not missing on the server
+	# This means the server lazy fetched it
+	check_missing_objects server 0 "" &&
+
+	# Reinitialize server so that the largest object is missing again
+	initialize_server 1 "$oid"
+'
+
+test_expect_success "clone without uploadpack.lazyFetchTrusted fails" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# Note: no uploadpack.lazyFetchTrusted config is set here, so
+	# the served repo is NOT trusted for lazy fetching.
+
+	# Clone without GIT_NO_LAZY_FETCH=0 fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "uploadpack.lazyFetchTrusted is ignored in repo config" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching, but this is
+	# done in the repo config, not in protected config, so this is
+	# ignored.
+	test_config -C server uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Clone without GIT_NO_LAZY_FETCH=0 fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "explicit GIT_NO_LAZY_FETCH overrides uploadpack.lazyFetchTrusted" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# But GIT_NO_LAZY_FETCH=1 disables lazy fetching, so clone fails
+	test_must_fail env GIT_NO_LAZY_FETCH=1 git clone --no-local \
+		--filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
 test_expect_success "init + fetch with promisor.advertise set to 'true'" '
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
-- 
2.55.0.530.gdb3615d990.dirty

Re: [PATCH 1/5] promisor-remote: factor out lazy_fetch_objects()

[Christian Couder]

On Fri, Aug 7, 2026 at 3:55 PM Christian Couder
<christian.couder@gmail.com> wrote:

[...]

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

Sorry I just realized that there is the wrong sign-off email address
again. Will fix it in v2.

Re: [PATCH 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Junio C Hamano]

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

> Range diff with previous series
> ===============================
>
> The range diff with the previous ("Introduce a 'fromAccepted' option
> to GIT_NO_LAZY_FETCH") series is not very interesting as only the
> first patch has been saved, but anyway here it is:
>
> 1:  8dd67ddaca ! 1:  b5b0836d19 promisor-remote: factor out lazy_fetch_objects()
>     @@ Commit message
>          that could not be fetched are promisor objects.
>      
>          Let's refactor the lazy fetching logic out of these two functions
>     -    into a new lazy_fetch_objects() function. This will make it easier
>     -    to extend the lazy fetching logic in following commits.
>     +    into a new lazy_fetch_objects() function.
>      
>          This is a pure refactoring with no intended behavior change. Two
>          things shift in ways that are observably equivalent though:
> 2:  314c61cbbe < -:  ---------- promisor-remote: introduce enum allow_lazy_fetch
> 3:  cb2f5447e2 < -:  ---------- promisor-remote: teach 'fromAccepted' to GIT_NO_LAZY_FETCH
> -:  ---------- > 2:  879e3a34e3 setup: extract path_allowlist_apply()
> -:  ---------- > 3:  98431ab7b3 setup: add 'allow_dot' arg to path_allowlist_apply()
> -:  ---------- > 4:  a46f4c1bb8 upload-pack: read uploadpack.lazyFetchTrusted
> -:  ---------- > 5:  4063f233aa builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo
>
>
> Christian Couder (5):
>   promisor-remote: factor out lazy_fetch_objects()
>   setup: extract path_allowlist_apply()
>   setup: add 'allow_dot' arg to path_allowlist_apply()
>   upload-pack: read uploadpack.lazyFetchTrusted
>   builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo
>
>  Documentation/config/uploadpack.adoc  |  42 ++++++++++
>  Documentation/git-upload-pack.adoc    |   5 ++
>  Documentation/git.adoc                |   4 +-
>  builtin/upload-pack.c                 |  11 +++
>  promisor-remote.c                     |  76 ++++++++++--------
>  setup.c                               | 108 ++++++++++++++------------
>  setup.h                               |  28 +++++++
>  t/t5710-promisor-remote-capability.sh |  70 +++++++++++++++++
>  upload-pack.c                         |  37 +++++++++
>  upload-pack.h                         |   3 +
>  10 files changed, 304 insertions(+), 80 deletions(-)

What's missing is the information on the base.  I tried applying
these patches to 'v2.55.0' and the recent tips of 'master':

    2c78326f81 The 11th batch
    5b2471720c The 10th batch
    a97fcc37c2 The 9th batch
    13c7afec21 The 8th batch
    9a0c4701dc The 7th batch
    5d2e770923 The 6th batch
    48bbf81c29 The 5th batch
    41365c2a9b The 4th batch for Git 2.56
    d35c5399e3 The 3rd batch for Git 2.56
    55526a1826 The 2nd batch for Git 2.56

but the series did not apply to any of them.

It turns out the reason has nothing to do with your choice of
base.  It is because the series structure is not understood by 'b4'.

The cover letter I am responding to is a reply to another series,
but the patches in this round are not marked as 'v2'.  This seems
to cause 'b4' to grab patches from both series and smash them
together, resulting in an inapplicable mess.  It seems you cannot
have your cake and eat it, too 😠.

Next time, please do not thread the two topics together unless you
are marking the newer iteration with a higher 'vN' number.

Thanks.

Re: [PATCH 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Christian Couder]

On Fri, Aug 7, 2026 at 8:31 PM Junio C Hamano <gitster@pobox.com> wrote:

> >  Documentation/config/uploadpack.adoc  |  42 ++++++++++
> >  Documentation/git-upload-pack.adoc    |   5 ++
> >  Documentation/git.adoc                |   4 +-
> >  builtin/upload-pack.c                 |  11 +++
> >  promisor-remote.c                     |  76 ++++++++++--------
> >  setup.c                               | 108 ++++++++++++++------------
> >  setup.h                               |  28 +++++++
> >  t/t5710-promisor-remote-capability.sh |  70 +++++++++++++++++
> >  upload-pack.c                         |  37 +++++++++
> >  upload-pack.h                         |   3 +
> >  10 files changed, 304 insertions(+), 80 deletions(-)
>
> What's missing is the information on the base.  I tried applying
> these patches to 'v2.55.0' and the recent tips of 'master':
>
>     2c78326f81 The 11th batch
>     5b2471720c The 10th batch
>     a97fcc37c2 The 9th batch
>     13c7afec21 The 8th batch
>     9a0c4701dc The 7th batch
>     5d2e770923 The 6th batch
>     48bbf81c29 The 5th batch
>     41365c2a9b The 4th batch for Git 2.56
>     d35c5399e3 The 3rd batch for Git 2.56
>     55526a1826 The 2nd batch for Git 2.56
>
> but the series did not apply to any of them.
>
> It turns out the reason has nothing to do with your choice of
> base.  It is because the series structure is not understood by 'b4'.
>
> The cover letter I am responding to is a reply to another series,
> but the patches in this round are not marked as 'v2'.  This seems
> to cause 'b4' to grab patches from both series and smash them
> together, resulting in an inapplicable mess.  It seems you cannot
> have your cake and eat it, too 😠.

I guess b4 should have, or grow, an option for that, because it's not
uncommon that someone would post an alternative patch or patch series
in reply to some patch(es).

> Next time, please do not thread the two topics together unless you
> are marking the newer iteration with a higher 'vN' number.

Ok, I will not do that. I will start a separate thread. Now I hope it
will work if I send a v2 in reply to the latest series.

Thanks.

Re: [PATCH 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Junio C Hamano]

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

>> It turns out the reason has nothing to do with your choice of
>> base.  It is because the series structure is not understood by 'b4'.
>>
>> The cover letter I am responding to is a reply to another series,
>> but the patches in this round are not marked as 'v2'.  This seems
>> to cause 'b4' to grab patches from both series and smash them
>> together, resulting in an inapplicable mess.  It seems you cannot
>> have your cake and eat it, too 😠.
>
> I guess b4 should have, or grow, an option for that, because it's not
> uncommon that someone would post an alternative patch or patch series
> in reply to some patch(es).

There is an option that tells it not to crawl up the parent article
to find siblings, and it would have worked fine in this case, but
then it would prevent us from noticing that a newer iteration
exists.

But it should not be the norm.

[PATCH v2 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Christian Couder]

Recently the "promisor-remote" capability was added to protocol v2,
allowing servers and clients to agree on the promisor remotes they can
safely use.

The more servers use promisor remotes, the more it is important to
properly control if they can lazy fetch when responding to a clone or
fetch request from the client.

For example, in the context of large object promisors (see
"Documentation/technical/large-object-promisors.adoc"), if a client
clones with a filter set to 100kB while the server has moved all of
the blobs >= 10kB to a promisor remote, the server will not be able to
provide blobs between 10kB and 100kB to the client, which will make
the clone fail.

Even if the `--filter=auto` option is available since ef2f1845ec
(fetch-pack: wire up and enable auto filter logic, 2026-02-16) it's
still a good idea to provide more control over lazy fetching on the
server side to server operators, as lazy fetching on the server side
could be useful in corporate environments.

Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
2024-04-16), lazy fetching has been controlled by the
`GIT_NO_LAZY_FETCH` environment variable. This is a boolean that is
set to 'true' by default when calling `git upload-pack` for security
reasons.

The main security issue on the server side is making sure the served
repo itself is also trusted, as lazily fetching runs `git fetch`,
which may execute arbitrary commands specified in the configuration
and hooks of the served repo. The operator of the server should decide
and mark that trust, not the served repo itself, nor the client.

This series introduces a new 'uploadpack.lazyFetchTrusted' protected
configuration variable similar to 'safe.directory' (see
"Documentation/config/safe.adoc") to mark trusted repos where lazy
fetching is allowed. As it is protected, this config variable will
only take effect if it is set in global or system scope, so only
server operators can control it.

Previous related work
=====================

A previous series called "Introduce a 'fromAccepted' option to
GIT_NO_LAZY_FETCH" [1] took a different approach as it wanted to make
it easier to allow lazy fetching from accepted promisor remotes. But
after brian replied that he didn't think it was a good idea, and after
thinking about this more, my opinion now is that some promisor remotes
being accepted or not is not really relevant to the issue.

In my reply to brian, I said:

"""
Different features could be developed (in future work) to improve on
the current state:
    - a way for lazy fetching to work without reading config files,
triggering hooks, or doing potentially sensitive things,
    - an explicit way for operators to mark trusted repos (like
perhaps a server-side config the operator sets per-repo),
    - operator-defined allow/deny rules, or maybe
    - some ways/scripts/commands to scan repos and check configuration
information, remote settings and everything potentially sensitive to
decide if a repo looks safe enough to allow lazy fetching or not.
"""

So I decided to go with "an explicit way for operators to mark trusted
repos" and this series is an implementation of that.

Note that the feature developed in this series applies to protocol
v0/v1 as well as v2 while the previous one was only related to v2.

[1]: https://lore.kernel.org/git/CAP8UFD0_S9eg_w42tcNRnT9E2ntLr_eHLnzE4c2dSu67DzZoXg@mail.gmail.com/

Overview of the patches
=======================

  - Patch 1/5 is the only patch saved from the "Introduce a
    'fromAccepted' option to GIT_NO_LAZY_FETCH" series. It's not
    necessary for the rest of this series and its main feature to
    work, but I think it's a nice refactoring related to lazy
    fetching, so it might as well be part of this series. There is a
    small change in the commit message (to not mention following
    commits) compared to the version in the previous series.

  - Patches 2/5 and 3/5 extract and modify code used by the
    'safe.directory' config variable in a path_allowlist_apply()
    function, so that this function can be reused to process
    'uploadpack.lazyFetchTrusted' in the next patch.

  - Patch 4/5 actually uses path_allowlist_apply() from a new
    upload_pack_lazy_fetch_trusted() function to process
    'uploadpack.lazyFetchTrusted', but the result from that processing
    isn't actually used to have a practical effect.

  - Patch 5/5 wires up the new upload_pack_lazy_fetch_trusted()
    function to decide if lazy fetching can actually be enabled.

Changes since v1
================

The only change is that the Signed-off-by email address has been fixed
to "christian.couder@gmail.com", which is my primary address in
".mailmap" since 6375b40aea (mailmap: change primary address for
Christian Couder, 2026-08-03).

This version is also sent as a separate 'v2' iteration in reply to v1,
instead of being threaded onto the previous "Introduce a
'fromAccepted' option to GIT_NO_LAZY_FETCH" series, and it now
contains a 'base-commit' trailer, so that 'b4' and other tools can
find the right base and the right patches.

CI tests
========

I didn't run them as only commit messages changed since v1.

Range diff with v1
==================

1:  b5b0836d19 ! 1:  1605740203 promisor-remote: factor out lazy_fetch_objects()
    @@ Commit message
         'bool' instead of 'int', as it just returns whether all the objects
         could be fetched, and document its return value.
     
    -    Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Signed-off-by: Christian Couder <christian.couder@gmail.com>
     
      ## promisor-remote.c ##
     @@ promisor-remote.c: static int fetch_objects(struct repository *repo,
2:  879e3a34e3 ! 2:  5f226b6508 setup: extract path_allowlist_apply()
    @@ Commit message
     
         While at it let's make the helper's code simpler and more generic.
     
    -    Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Signed-off-by: Christian Couder <christian.couder@gmail.com>
     
      ## setup.c ##
     @@ setup.c: static int canonicalize_ceiling_entry(struct string_list_item *item,
3:  98431ab7b3 ! 3:  051aa11fc9 setup: add 'allow_dot' arg to path_allowlist_apply()
    @@ Commit message
     
         While at it let's document it properly in "setup.h".
     
    -    Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Signed-off-by: Christian Couder <christian.couder@gmail.com>
     
      ## setup.c ##
     @@ setup.c: static int canonicalize_ceiling_entry(struct string_list_item *item,
4:  a46f4c1bb8 ! 4:  045b5e647b upload-pack: read uploadpack.lazyFetchTrusted
    @@ Commit message
         Note that the new config variable should be read only from protected
         configuration files.
     
    -    Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Signed-off-by: Christian Couder <christian.couder@gmail.com>
     
      ## upload-pack.c ##
     @@
5:  4063f233aa ! 5:  c116661202 builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo
    @@ Commit message
         Now that "uploadpack.lazyFetchTrusted" is actually doing something,
         let's document it and reference it from GIT_NO_LAZY_FETCH's docs.
     
    -    Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Signed-off-by: Christian Couder <christian.couder@gmail.com>
     
      ## Documentation/config/uploadpack.adoc ##
     @@ Documentation/config/uploadpack.adoc: uploadpack.allowRefInWant::


Christian Couder (5):
  promisor-remote: factor out lazy_fetch_objects()
  setup: extract path_allowlist_apply()
  setup: add 'allow_dot' arg to path_allowlist_apply()
  upload-pack: read uploadpack.lazyFetchTrusted
  builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo

 Documentation/config/uploadpack.adoc  |  42 ++++++++++
 Documentation/git-upload-pack.adoc    |   5 ++
 Documentation/git.adoc                |   4 +-
 builtin/upload-pack.c                 |  11 +++
 promisor-remote.c                     |  76 ++++++++++--------
 setup.c                               | 108 ++++++++++++++------------
 setup.h                               |  28 +++++++
 t/t5710-promisor-remote-capability.sh |  70 +++++++++++++++++
 upload-pack.c                         |  37 +++++++++
 upload-pack.h                         |   3 +
 10 files changed, 304 insertions(+), 80 deletions(-)


base-commit: 745601a9a94110d74769ab605ccd4f61339758d2
-- 
2.55.0.565.gc116661202

[PATCH v2 1/5] promisor-remote: factor out lazy_fetch_objects()

[Christian Couder]

In "promisor-remote.c:fetch_objects()", there is a check to disable
lazy fetching when the `GIT_NO_LAZY_FETCH` environment variable is
set. The fetch_objects() function is called once per promisor remote
though. So the check might be performed more times than necessary.

Also promisor_remote_get_direct() mixes up the logic deciding which
promisor remotes to try with the logic checking that the objects
that could not be fetched are promisor objects.

Let's refactor the lazy fetching logic out of these two functions
into a new lazy_fetch_objects() function.

This is a pure refactoring with no intended behavior change. Two
things shift in ways that are observably equivalent though:

  - the `GIT_NO_LAZY_FETCH` check is now performed once up front,
    instead of once per promisor remote, and

  - promisor_remote_init() is no longer called when lazy fetching
    is disabled, which is fine as nothing downstream of it, like
    is_promisor_object(), needs it in that case.

While at it, let's also convert try_promisor_remotes() to return
'bool' instead of 'int', as it just returns whether all the objects
could be fetched, and document its return value.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 promisor-remote.c | 76 ++++++++++++++++++++++++++++-------------------
 1 file changed, 45 insertions(+), 31 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 43505d1e1a..65496c69cf 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -31,15 +31,6 @@ static int fetch_objects(struct repository *repo,
 	FILE *child_in;
 	int quiet;
 
-	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
-		static int warning_shown;
-		if (!warning_shown) {
-			warning_shown = 1;
-			warning(_("lazy fetching disabled; some objects may not be available"));
-		}
-		return -1;
-	}
-
 	child.git_cmd = 1;
 	child.in = -1;
 	if (repo != the_repository)
@@ -270,10 +261,15 @@ static int remove_fetched_oids(struct repository *repo,
 	return remaining_nr;
 }
 
-static int try_promisor_remotes(struct repository *repo,
-				struct object_id **remaining_oids,
-				int *remaining_nr, int *to_free,
-				bool accepted_only)
+/*
+ * Return 'true' if all the objects could be fetched from the
+ * (non-)accepted remotes, 'false' otherwise.
+ */
+static bool try_promisor_remotes(struct repository *repo,
+				 struct object_id **remaining_oids,
+				 int *remaining_nr,
+				 int *to_free,
+				 bool accepted_only)
 {
 	struct promisor_remote *r = repo->promisor_remote_config->promisors;
 
@@ -290,9 +286,37 @@ static int try_promisor_remotes(struct repository *repo,
 				continue;
 			}
 		}
-		return 1; /* all fetched */
+		return true; /* all fetched */
 	}
-	return 0;
+	return false;
+}
+
+/*
+ * Return 'true' if all the objects could be fetched, 'false' otherwise.
+ */
+static bool lazy_fetch_objects(struct repository *repo,
+			       struct object_id **remaining_oids,
+			       int *remaining_nr,
+			       int *to_free)
+{
+	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+		static int warning_shown;
+		if (!warning_shown) {
+			warning_shown = 1;
+			warning(_("lazy fetching disabled; some objects may not be available"));
+		}
+		return false;
+	}
+
+	promisor_remote_init(repo);
+
+	/* Try accepted remotes first (those the server told us to use) */
+	if (try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				 to_free, true))
+		return true;
+
+	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				    to_free, false);
 }
 
 void promisor_remote_get_direct(struct repository *repo,
@@ -302,28 +326,18 @@ void promisor_remote_get_direct(struct repository *repo,
 	struct object_id *remaining_oids = (struct object_id *)oids;
 	int remaining_nr = oid_nr;
 	int to_free = 0;
-	int i;
 
 	if (oid_nr == 0)
 		return;
 
-	promisor_remote_init(repo);
-
-	/* Try accepted remotes first (those the server told us to use) */
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, true))
-		goto all_fetched;
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, false))
-		goto all_fetched;
-
-	for (i = 0; i < remaining_nr; i++) {
-		if (is_promisor_object(repo, &remaining_oids[i]))
-			die(_("could not fetch %s from promisor remote"),
-			    oid_to_hex(&remaining_oids[i]));
+	if (!lazy_fetch_objects(repo, &remaining_oids, &remaining_nr, &to_free)) {
+		for (int i = 0; i < remaining_nr; i++) {
+			if (is_promisor_object(repo, &remaining_oids[i]))
+				die(_("could not fetch %s from promisor remote"),
+				    oid_to_hex(&remaining_oids[i]));
+		}
 	}
 
-all_fetched:
 	if (to_free)
 		free(remaining_oids);
 }
-- 
2.55.0.565.gc116661202

[PATCH v2 2/5] setup: extract path_allowlist_apply()

[Christian Couder]

In a following commit we are going to check whether a repository is
part of an allowlist specified in a config variable.

To prepare for that let's extract existing code from
safe_directory_cb() into a new path_allowlist_apply() helper that will
help with such checks.

While at it let's make the helper's code simpler and more generic.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 setup.c | 107 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 59 insertions(+), 48 deletions(-)

diff --git a/setup.c b/setup.c
index 95909e9603..39dfa1cc5f 100644
--- a/setup.c
+++ b/setup.c
@@ -1339,6 +1339,64 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 	}
 }
 
+static void path_allowlist_apply(const char *key, const char *value,
+				 const char *target_path, int *is_match)
+{
+	char *allowed = NULL;
+	char *normalized = NULL;
+
+	if (!value || !*value) {
+		*is_match = 0;
+		return;
+	}
+
+	if (!strcmp(value, "*")) {
+		*is_match = 1;
+		return;
+	}
+
+	if (git_config_pathname(&allowed, key, value) || !allowed)
+		return;
+
+	/*
+	 * Setting the config variable to a non-absolute path makes
+	 * little sense---it won't be relative to the configuration
+	 * file the item is defined in.  Except for ".", which means
+	 * "if we are at the top level of a repository, then it is
+	 * OK", which is slightly tighter than "*" that allows
+	 * discovery.
+	 */
+	if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
+		warning(_("%s '%s' not absolute"), key, allowed);
+		goto end;
+	}
+
+	/*
+	 * A .gitconfig in $HOME may be shared across different
+	 * machines and the config variable entries may or may not
+	 * exist as paths on all of these machines.  In other words,
+	 * it is not a warning worthy event when there is no such path
+	 * on this machine---the entry may be useful elsewhere.
+	 */
+	normalized = real_pathdup(allowed, 0);
+	if (!normalized)
+		goto end;
+
+	if (ends_with(normalized, "/*")) {
+		size_t len = strlen(normalized);
+		if (!fspathncmp(normalized, target_path, len - 1))
+			*is_match = 1;
+		goto end;
+	}
+
+	if (!fspathcmp(target_path, normalized))
+		*is_match = 1;
+
+end:
+	free(normalized);
+	free(allowed);
+}
+
 struct safe_directory_data {
 	char *path;
 	int is_safe;
@@ -1352,54 +1410,7 @@ static int safe_directory_cb(const char *key, const char *value,
 	if (strcmp(key, "safe.directory"))
 		return 0;
 
-	if (!value || !*value) {
-		data->is_safe = 0;
-	} else if (!strcmp(value, "*")) {
-		data->is_safe = 1;
-	} else {
-		char *allowed = NULL;
-
-		if (!git_config_pathname(&allowed, key, value) && allowed) {
-			char *normalized = NULL;
-
-			/*
-			 * Setting safe.directory to a non-absolute path
-			 * makes little sense---it won't be relative to
-			 * the configuration file the item is defined in.
-			 * Except for ".", which means "if we are at the top
-			 * level of a repository, then it is OK", which is
-			 * slightly tighter than "*" that allows discovery.
-			 */
-			if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
-				warning(_("safe.directory '%s' not absolute"),
-					allowed);
-				goto next;
-			}
-
-			/*
-			 * A .gitconfig in $HOME may be shared across
-			 * different machines and safe.directory entries
-			 * may or may not exist as paths on all of these
-			 * machines.  In other words, it is not a warning
-			 * worthy event when there is no such path on this
-			 * machine---the entry may be useful elsewhere.
-			 */
-			normalized = real_pathdup(allowed, 0);
-			if (!normalized)
-				goto next;
-
-			if (ends_with(normalized, "/*")) {
-				size_t len = strlen(normalized);
-				if (!fspathncmp(normalized, data->path, len - 1))
-					data->is_safe = 1;
-			} else if (!fspathcmp(data->path, normalized)) {
-				data->is_safe = 1;
-			}
-		next:
-			free(normalized);
-			free(allowed);
-		}
-	}
+	path_allowlist_apply(key, value, data->path, &data->is_safe);
 
 	return 0;
 }
-- 
2.55.0.565.gc116661202

[PATCH v2 3/5] setup: add 'allow_dot' arg to path_allowlist_apply()

[Christian Couder]

A previous commit created path_allowlist_apply() with the goal of later
reusing that function. But when it will be reused in a following commit
this function will need to reject non-absolute paths including those
with a single dot that are currently accepted.

To prepare for reusing path_allowlist_apply(), let's add a
`bool allow_dot` argument to it, and let's export this function.

While at it let's document it properly in "setup.h".

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 setup.c |  9 +++++----
 setup.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 39dfa1cc5f..a09e697e3a 100644
--- a/setup.c
+++ b/setup.c
@@ -1339,8 +1339,9 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 	}
 }
 
-static void path_allowlist_apply(const char *key, const char *value,
-				 const char *target_path, int *is_match)
+void path_allowlist_apply(const char *key, const char *value,
+			  const char *target_path, int *is_match,
+			  bool allow_dot)
 {
 	char *allowed = NULL;
 	char *normalized = NULL;
@@ -1366,7 +1367,7 @@ static void path_allowlist_apply(const char *key, const char *value,
 	 * OK", which is slightly tighter than "*" that allows
 	 * discovery.
 	 */
-	if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
+	if (!is_absolute_path(allowed) && (!allow_dot || strcmp(allowed, "."))) {
 		warning(_("%s '%s' not absolute"), key, allowed);
 		goto end;
 	}
@@ -1410,7 +1411,7 @@ static int safe_directory_cb(const char *key, const char *value,
 	if (strcmp(key, "safe.directory"))
 		return 0;
 
-	path_allowlist_apply(key, value, data->path, &data->is_safe);
+	path_allowlist_apply(key, value, data->path, &data->is_safe, true);
 
 	return 0;
 }
diff --git a/setup.h b/setup.h
index 654f10e059..d4f8af5457 100644
--- a/setup.h
+++ b/setup.h
@@ -304,4 +304,32 @@ struct startup_info {
 extern struct startup_info *startup_info;
 extern const char *tmp_original_cwd;
 
+/*
+ * Apply the path allowlist in 'value' against 'target_path' setting
+ * '*is_match' accordingly.
+ *
+ * `value` is the value of a multi-valued config variable named `key`
+ * that holds an allowlist of paths. `target_path` is the (normalized)
+ * path being tested. `*is_match` is updated in place:
+ *
+ *   - an empty value resets it to 0 (so a later, more specific config
+ *     scope can clear entries from a broader one),
+ *   - "*" sets it to 1 (allow everything),
+ *   - "<path>" sets it to 1 if <path> equals `target_path`,
+ *   - "<path>" + "/" + "*" sets it to 1 if <path> is a leading
+ *     directory of `target_path`,
+ *   - any other (unmatching) value leaves `*is_match` unchanged.
+ *
+ * Non-absolute values are rejected with a warning, except "." when
+ * `allow_dot` is set (used by 'safe.directory' to mean "the top level
+ * of the current repository").
+ *
+ * Callers are expected to invoke this once per config value,
+ * typically from a protected-config callback, so that untrusted
+ * repository config cannot influence the decision.
+ */
+void path_allowlist_apply(const char *key, const char *value,
+			  const char *target_path, int *is_match,
+			  bool allow_dot);
+
 #endif /* SETUP_H */
-- 
2.55.0.565.gc116661202

[PATCH v2 4/5] upload-pack: read uploadpack.lazyFetchTrusted

[Christian Couder]

Previous commits created and prepared the path_allowlist_apply()
function.

Let's reuse this function for a new "uploadpack.lazyFetchTrusted"
configuration variable.

It allows us to:

  - read an allowlist from that config variable,
  - check if the current repo is in that list, and
  - return the result from a new upload_pack_lazy_fetch_trusted()
    function.

The new function will be used in a following commit.

Note that the new config variable should be read only from protected
configuration files.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 upload-pack.c | 37 +++++++++++++++++++++++++++++++++++++
 upload-pack.h |  3 +++
 2 files changed, 40 insertions(+)

diff --git a/upload-pack.c b/upload-pack.c
index a52856d869..29e700e43b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -34,6 +34,8 @@
 #include "json-writer.h"
 #include "strmap.h"
 #include "promisor-remote.h"
+#include "setup.h"
+#include "abspath.h"
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE	(1u << 11)
@@ -1378,6 +1380,41 @@ static int upload_pack_config(const char *var, const char *value,
 	return parse_hide_refs_config(var, value, "uploadpack", &data->hidden_refs);
 }
 
+struct lazy_fetch_trusted {
+	int trusted;
+	char *repo_path;
+};
+
+static int upload_pack_protected_lazy_fetch_config(const char *var, const char *value,
+						   const struct config_context *ctx UNUSED,
+						   void *cb_data)
+{
+	struct lazy_fetch_trusted *data = cb_data;
+
+	if (!strcmp("uploadpack.lazyfetchtrusted", var)) {
+		path_allowlist_apply(var, value, data->repo_path,
+				     &data->trusted, false);
+		return 0;
+	}
+
+	return 0;
+}
+
+bool upload_pack_lazy_fetch_trusted(struct repository *r)
+{
+	struct lazy_fetch_trusted data = { 0 };
+
+	data.repo_path = real_pathdup(r->worktree ? r->worktree : r->gitdir, 0);
+	if (!data.repo_path)
+		return false;
+
+	git_protected_config(upload_pack_protected_lazy_fetch_config, &data);
+
+	free(data.repo_path);
+
+	return !!data.trusted;
+}
+
 static int upload_pack_protected_config(const char *var, const char *value,
 					const struct config_context *ctx UNUSED,
 					void *cb_data)
diff --git a/upload-pack.h b/upload-pack.h
index d6ee25ea98..b2212992c3 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -12,4 +12,7 @@ struct strbuf;
 int upload_pack_advertise(struct repository *r,
 			  struct strbuf *value);
 
+/* Is this repo trusted for lazy fetching? */
+bool upload_pack_lazy_fetch_trusted(struct repository *r);
+
 #endif /* UPLOAD_PACK_H */
-- 
2.55.0.565.gc116661202

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

[Christian Couder]

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.

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, as the main security issue is that lazily fetching runs `git
fetch`, which may execute arbitrary commands specified in the
configuration and hooks of the served repo.

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.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 Documentation/config/uploadpack.adoc  | 42 ++++++++++++++++
 Documentation/git-upload-pack.adoc    |  5 ++
 Documentation/git.adoc                |  4 +-
 builtin/upload-pack.c                 | 11 +++++
 t/t5710-promisor-remote-capability.sh | 70 +++++++++++++++++++++++++++
 5 files changed, 131 insertions(+), 1 deletion(-)

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
+	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.
++
+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`
+entry with an empty value.
++
+A repository is identified by its worktree, or its git directory for a bare
+repository, and the value must be an absolute path. Giving a path with `/*`
+appended to it will trust all repositories under the named directory. To trust
+all served repositories, set `uploadpack.lazyFetchTrusted` to the string `*`.
++
+The value of this setting is interpolated, i.e. `~/<path>` expands to a
+path relative to the home directory and `%(prefix)/<path>` expands to a
+path relative to Git's (runtime) prefix.
++
+Note that this configuration variable is only respected when it is specified
+in protected configuration (see <<SCOPES>>). This prevents untrusted
+repositories from tampering with this value.
diff --git a/Documentation/git-upload-pack.adoc b/Documentation/git-upload-pack.adoc
index 9167a321d0..90c2ba1194 100644
--- a/Documentation/git-upload-pack.adoc
+++ b/Documentation/git-upload-pack.adoc
@@ -71,6 +71,11 @@ This is implemented by having `upload-pack` internally set the
 (because you are fetching from a partial clone, and you are sure
 you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to
 `0`.
++
+Instead of setting `GIT_NO_LAZY_FETCH` to `0` in the environment, a
+server operator can allow lazy fetching on a per-repository basis by
+listing trusted repositories in the `uploadpack.lazyFetchTrusted`
+configuration variable. See linkgit:git-config[1].
 
 SECURITY
 --------
diff --git a/Documentation/git.adoc b/Documentation/git.adoc
index 8a5cdd3b3d..2e763d1f93 100644
--- a/Documentation/git.adoc
+++ b/Documentation/git.adoc
@@ -949,7 +949,9 @@ for full details.
 `GIT_NO_LAZY_FETCH`::
 	Setting this Boolean environment variable to true tells Git
 	not to lazily fetch missing objects from the promisor remote
-	on demand.
+	on demand. On the server side, the `uploadpack.lazyFetchTrusted`
+	configuration variable can control this per-repository. See
+	linkgit:git-upload-pack[1].
 
 `GIT_REFLOG_ACTION`::
 	When a ref is updated, reflog entries are created to keep
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);
 
 	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);
+
 	switch (determine_protocol_version_server()) {
 	case protocol_v2:
 		if (advertise_refs)
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 549acff23f..e6993f2761 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -173,6 +173,76 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
 	initialize_server 1 "$oid"
 '
 
+test_expect_success "clone with uploadpack.lazyFetchTrusted" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Clone without GIT_NO_LAZY_FETCH=0
+	git clone --no-local --filter="blob:limit=5k" server client &&
+
+	# Check that the largest object is not missing on the server
+	# This means the server lazy fetched it
+	check_missing_objects server 0 "" &&
+
+	# Reinitialize server so that the largest object is missing again
+	initialize_server 1 "$oid"
+'
+
+test_expect_success "clone without uploadpack.lazyFetchTrusted fails" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# Note: no uploadpack.lazyFetchTrusted config is set here, so
+	# the served repo is NOT trusted for lazy fetching.
+
+	# Clone without GIT_NO_LAZY_FETCH=0 fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "uploadpack.lazyFetchTrusted is ignored in repo config" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching, but this is
+	# done in the repo config, not in protected config, so this is
+	# ignored.
+	test_config -C server uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Clone without GIT_NO_LAZY_FETCH=0 fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "explicit GIT_NO_LAZY_FETCH overrides uploadpack.lazyFetchTrusted" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# But GIT_NO_LAZY_FETCH=1 disables lazy fetching, so clone fails
+	test_must_fail env GIT_NO_LAZY_FETCH=1 git clone --no-local \
+		--filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
 test_expect_success "init + fetch with promisor.advertise set to 'true'" '
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
-- 
2.55.0.565.gc116661202

Re: [PATCH v2 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Junio C Hamano]

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

> Recently the "promisor-remote" capability was added to protocol v2,
> allowing servers and clients to agree on the promisor remotes they can
> safely use.

I do not know what you did, but it seems that an attempt to futz
with the thread structure i.e.,

    This version is also sent as a separate 'v2' iteration in reply to v1,
    instead of being threaded onto the previous "Introduce a
    'fromAccepted' option to GIT_NO_LAZY_FETCH" series, and it now
    contains a 'base-commit' trailer, so that 'b4' and other tools can
    find the right base and the right patches.

made the patches totally unusable.

This [v2 0/5] has

    Message-ID: <20260813154748.2378747-1-christian.couder@gmail.com>
    In-reply-to: <20260807135511.1818458-1-christian.couder@gmail.com>

which is *correct*.  It is a reply to [0/5] of the original thread.
However, [v2 1/5] says:

    Message-ID: <20260813154748.2378747-2-christian.couder@gmail.com>
    In-reply-to: <20260807135511.1818458-1-christian.couder@gmail.com>

which is quite wrong.  [v2 1/5] should be a reply to the cover
letter of the v2 iteration.  [v2 2/5], [v2 3/5], ... should also be
replies to the cover letter of the v2 iteration.

I have never seen either plain vanilla send-email or GitGitGadget
produce misthreaded series like this one.  Do you have some custom
settings to send things out?

Here is _one_ way to create a thread of the right shape:

 1. Develop this 5-patch series.

 2. Run

    $ git format-patch -v2 --cover-letter -5

    to grab 0000-cover-letter.patch to 0005-builtin-upload-...patch
    files.  You may also want to pass --range-diff option.

 3. Edit 0000-cover-letter.patch to your satisfaction.  Do not futz
    with In-Reply-To or References or Message-Id yourself there;
    the tool will do this part better than manual editing.

 4. Run

    $ git send-email --no-chain-reply-to \
      --in-reply-to='<20260807135511.1818458-1-christian.couder@gmail.com>' \
      000[0-5]-*.patch

    This will make the initial message (which is the cover letter of
    this iteration) a reply to the named message (which is the cover
    letter of the v1 iteration), and then the remainder replies to
    the initial message, which is what we want to see.

There surely are other right ways to do so.  As long as the end
result would look like

    * vN (1 < N) cover letter is a reply to v1 cover letter
    * vN patch M (0 < M) is a reply to vN cover letter

things will flow more smoothly.

HTH.

Re: [PATCH v2 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Christian Couder]

On Thu, Aug 13, 2026 at 10:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > Recently the "promisor-remote" capability was added to protocol v2,
> > allowing servers and clients to agree on the promisor remotes they can
> > safely use.
>
> I do not know what you did, but it seems that an attempt to futz
> with the thread structure i.e.,
>
>     This version is also sent as a separate 'v2' iteration in reply to v1,
>     instead of being threaded onto the previous "Introduce a
>     'fromAccepted' option to GIT_NO_LAZY_FETCH" series, and it now
>     contains a 'base-commit' trailer, so that 'b4' and other tools can
>     find the right base and the right patches.
>
> made the patches totally unusable.
>
> This [v2 0/5] has
>
>     Message-ID: <20260813154748.2378747-1-christian.couder@gmail.com>
>     In-reply-to: <20260807135511.1818458-1-christian.couder@gmail.com>
>
> which is *correct*.  It is a reply to [0/5] of the original thread.
> However, [v2 1/5] says:
>
>     Message-ID: <20260813154748.2378747-2-christian.couder@gmail.com>
>     In-reply-to: <20260807135511.1818458-1-christian.couder@gmail.com>
>
> which is quite wrong.  [v2 1/5] should be a reply to the cover
> letter of the v2 iteration.  [v2 2/5], [v2 3/5], ... should also be
> replies to the cover letter of the v2 iteration.

Yeah, sorry. I tried to pass `--in-reply-to` to `git format-patch`
instead of `git send-email` but it looks like they don't behave the
same. Maybe because I have no `format.thread` set, so `git
format-patch` defaults to `--no-thread`, and in that case
`--in-reply-to` applies to every mail, not just the cover letter. I
should have checked more carefully and not tried to improve too many
things at once in my setup.

> I have never seen either plain vanilla send-email or GitGitGadget
> produce misthreaded series like this one.  Do you have some custom
> settings to send things out?

No, I just use `git format-patch` and `git send-email` without special
configuration.

> Here is _one_ way to create a thread of the right shape:
>
>  1. Develop this 5-patch series.
>
>  2. Run
>
>     $ git format-patch -v2 --cover-letter -5
>
>     to grab 0000-cover-letter.patch to 0005-builtin-upload-...patch
>     files.  You may also want to pass --range-diff option.
>
>  3. Edit 0000-cover-letter.patch to your satisfaction.  Do not futz
>     with In-Reply-To or References or Message-Id yourself there;
>     the tool will do this part better than manual editing.
>
>  4. Run
>
>     $ git send-email --no-chain-reply-to \
>       --in-reply-to='<20260807135511.1818458-1-christian.couder@gmail.com>' \
>       000[0-5]-*.patch

Yeah, that's pretty much what I usually do. I don't use
`--no-chain-reply-to` though, but I will.

>     This will make the initial message (which is the cover letter of
>     this iteration) a reply to the named message (which is the cover
>     letter of the v1 iteration), and then the remainder replies to
>     the initial message, which is what we want to see.
>
> There surely are other right ways to do so.  As long as the end
> result would look like
>
>     * vN (1 < N) cover letter is a reply to v1 cover letter
>     * vN patch M (0 < M) is a reply to vN cover letter

Yeah I should have checked more carefully before sending. Thanks.

> things will flow more smoothly.
>
> HTH.

Re: [PATCH v2 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Junio C Hamano]

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

>> I have never seen either plain vanilla send-email or GitGitGadget
>> produce misthreaded series like this one.  Do you have some custom
>> settings to send things out?
>
> No, I just use `git format-patch` and `git send-email` without special
> configuration.

Perhaps you are contaminating format-patch output with In-Reply-To:
and other message-id related headers (perhaps using its options)?  I
don't, and I suspect your mentees probably do not, as their patches
do not have this issue, either.

> Yeah I should have checked more carefully before sending. Thanks.

Thanks.

Re: [PATCH v2 1/5] promisor-remote: factor out lazy_fetch_objects()

[Junio C Hamano]

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

> +/*
> + * Return 'true' if all the objects could be fetched, 'false' otherwise.
> + */
> +static bool lazy_fetch_objects(struct repository *repo,
> +			       struct object_id **remaining_oids,
> +			       int *remaining_nr,
> +			       int *to_free)
> +{
> +	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
> +		static int warning_shown;
> +		if (!warning_shown) {
> +			warning_shown = 1;
> +			warning(_("lazy fetching disabled; some objects may not be available"));
> +		}
> +		return false;
> +	}
> +
> +	promisor_remote_init(repo);
> +
> +	/* Try accepted remotes first (those the server told us to use) */
> +	if (try_promisor_remotes(repo, remaining_oids, remaining_nr,
> +				 to_free, true))
> +		return true;
> +
> +	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
> +				    to_free, false);
>  }

Perhaps writing it this way would make it easier to tell what is
going on.  We try the preferred ones first, and then fall back to
the other ones.

        return (try_promisor_remotes(..., true) ||
                try_promisor_remotes(..., false));

But more importantly, I wonder if keeping the list of missing object
names in memory will later turn out to be problematic in real-life
applications.  Without knowing much about how the current code for
bulk dehydrating promisor objects is structured, I expected an API
that looks more like:

 - bulk_download_begin(): performs the early part of
   fetch_objects(), sets up connections to the promisor remote(s),
   and calls start_command() on the child process.

 - bulk_download_this(): after calling the _begin() function above,
   it runs around and collects missing objects that it needs to do
   its work.  For each such missing object it discovers, this
   function is called, which sends the object name down the
   '--stdin' file descriptor.

 - bulk_download_done(): tells the child process that we are done
   feeding object names.

but that is not what I am seeing.  I guess the current arrangement
cannot be avoided, because we are going to fetch from more than one
promisor remote.  Under such constraints, the way to deal with a
massive number of missing objects will not be "streaming" like I
imagined above, but needs to be done differently, like spooling to a
file or something silly like that.

In any case, except that this avoids checking the environment
variable multiple times, I can see that it is a no-op refactoring of
the existing code.

Nice and cleanly done.

Thanks.

Re: [PATCH v2 2/5] setup: extract path_allowlist_apply()

[Junio C Hamano]

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

> In a following commit we are going to check whether a repository is
> part of an allowlist specified in a config variable.
>
> To prepare for that let's extract existing code from
> safe_directory_cb() into a new path_allowlist_apply() helper that will
> help with such checks.
>
> While at it let's make the helper's code simpler and more generic.
>
> Signed-off-by: Christian Couder <christian.couder@gmail.com>
> ---
>  setup.c | 107 +++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 59 insertions(+), 48 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 95909e9603..39dfa1cc5f 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1339,6 +1339,64 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
>  	}
>  }
>  
> +static void path_allowlist_apply(const char *key, const char *value,
> +				 const char *target_path, int *is_match)
> +{
> +	char *allowed = NULL;
> +	char *normalized = NULL;
> +
> +	if (!value || !*value) {
> +		*is_match = 0;
> +		return;
> +	}
> +
> +	if (!strcmp(value, "*")) {
> +		*is_match = 1;
> +		return;
> +	}
> +
> +	if (git_config_pathname(&allowed, key, value) || !allowed)
> +		return;

The inversion of the polarity from the original here is a nice
touch.  We no longer have to look at deeply indented block to tell
immediately that nothing will happen when the configuration variable
is not set.

> +	/*
> +	 * Setting the config variable to a non-absolute path makes
> +	 * little sense---it won't be relative to the configuration
> +	 * file the item is defined in.  Except for ".", which means
> +	 * "if we are at the top level of a repository, then it is
> +	 * OK", which is slightly tighter than "*" that allows
> +	 * discovery.
> +	 */
> +	if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
> +		warning(_("%s '%s' not absolute"), key, allowed);
> +		goto end;
> +	}
> +
> +	/*
> +	 * A .gitconfig in $HOME may be shared across different
> +	 * machines and the config variable entries may or may not
> +	 * exist as paths on all of these machines.  In other words,
> +	 * it is not a warning worthy event when there is no such path
> +	 * on this machine---the entry may be useful elsewhere.
> +	 */
> +	normalized = real_pathdup(allowed, 0);
> +	if (!normalized)
> +		goto end;
> +
> +	if (ends_with(normalized, "/*")) {
> +		size_t len = strlen(normalized);
> +		if (!fspathncmp(normalized, target_path, len - 1))
> +			*is_match = 1;
> +		goto end;
> +	}
> +
> +	if (!fspathcmp(target_path, normalized))
> +		*is_match = 1;
> +
> +end:
> +	free(normalized);
> +	free(allowed);
> +}

The name "is_match" somehow feels a bit awkward.  How about calling
it 

    *matches = true/false;

instead?

Re: [PATCH v2 3/5] setup: add 'allow_dot' arg to path_allowlist_apply()

[Junio C Hamano]

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

> A previous commit created path_allowlist_apply() with the goal of later
> reusing that function. But when it will be reused in a following commit
> this function will need to reject non-absolute paths including those
> with a single dot that are currently accepted.
>
> To prepare for reusing path_allowlist_apply(), let's add a
> `bool allow_dot` argument to it, and let's export this function.
>
> While at it let's document it properly in "setup.h".

If this is just "I want to add an extra caller that has specific
need and do not care about others in the future", this may be OK but
as a public function, this is a bit disappointing API design.  I
expected, as a generally useful function, you would instead add a
callback function to allow replacing the use of is_absoute_path()
plus the warning there, i.e.

void path_allowlist_apply(const char *key, const char *value,
			  const char *target_path, bool *matches,
			  bool (*allow_path)(const char *path))
{
	...

	if (!allow_path(allowed))
		goto end;

Also to avoid limiting this to configuration callback, I might
recommend to have it be more like this:

void path_allowlist_apply(const char *allowed, const char *target_path,
			  bool *matches,
			  bool (*allow_path)(const char *path, void *cbdata),
			  void *allow_path_cbdata)

where the original safe-directory thing may call
git_config_pathname() to compute allowed before calling this helper,
and pass the address of something like:

	struct { const char *key, *value } cbdata = {
		.key = key, .value = value;
	};

as the cbdata, and pass something like this

	static bool allow_safe_dir(const char *path, void *cbdata_)
	{
		struct { const char *key, *value } *cbdata = _cbdata;
		if (is_absoute_path(path) || !strcmp(path, ".")
			return true; /* ok */

		warning(_("%s '%s' not absolute"), cbdata->key, path);
		return false;
	}

as the allow_path callback function.  IOW warning, or insisting on
it being absolute, etc., does not have to be carved in stone.

Thanks.

Re: [PATCH v2 4/5] upload-pack: read uploadpack.lazyFetchTrusted

[Junio C Hamano]

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

> Previous commits created and prepared the path_allowlist_apply()
> function.
>
> Let's reuse this function for a new "uploadpack.lazyFetchTrusted"
> configuration variable.
>
> It allows us to:
>
>   - read an allowlist from that config variable,
>   - check if the current repo is in that list, and
>   - return the result from a new upload_pack_lazy_fetch_trusted()
>     function.
>
> The new function will be used in a following commit.
>
> Note that the new config variable should be read only from protected
> configuration files.
>
> Signed-off-by: Christian Couder <christian.couder@gmail.com>
> ---

OK.

I am not sure if the idea of configuration variable is truly sound,
but if it were, I agree that this is a reasonable implementation for
it.

Thanks.

>  upload-pack.c | 37 +++++++++++++++++++++++++++++++++++++
>  upload-pack.h |  3 +++
>  2 files changed, 40 insertions(+)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index a52856d869..29e700e43b 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -34,6 +34,8 @@
>  #include "json-writer.h"
>  #include "strmap.h"
>  #include "promisor-remote.h"
> +#include "setup.h"
> +#include "abspath.h"
>  
>  /* Remember to update object flag allocation in object.h */
>  #define THEY_HAVE	(1u << 11)
> @@ -1378,6 +1380,41 @@ static int upload_pack_config(const char *var, const char *value,
>  	return parse_hide_refs_config(var, value, "uploadpack", &data->hidden_refs);
>  }
>  
> +struct lazy_fetch_trusted {
> +	int trusted;
> +	char *repo_path;
> +};
> +
> +static int upload_pack_protected_lazy_fetch_config(const char *var, const char *value,
> +						   const struct config_context *ctx UNUSED,
> +						   void *cb_data)
> +{
> +	struct lazy_fetch_trusted *data = cb_data;
> +
> +	if (!strcmp("uploadpack.lazyfetchtrusted", var)) {
> +		path_allowlist_apply(var, value, data->repo_path,
> +				     &data->trusted, false);
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
> +bool upload_pack_lazy_fetch_trusted(struct repository *r)
> +{
> +	struct lazy_fetch_trusted data = { 0 };
> +
> +	data.repo_path = real_pathdup(r->worktree ? r->worktree : r->gitdir, 0);
> +	if (!data.repo_path)
> +		return false;
> +
> +	git_protected_config(upload_pack_protected_lazy_fetch_config, &data);
> +
> +	free(data.repo_path);
> +
> +	return !!data.trusted;
> +}
> +
>  static int upload_pack_protected_config(const char *var, const char *value,
>  					const struct config_context *ctx UNUSED,
>  					void *cb_data)
> diff --git a/upload-pack.h b/upload-pack.h
> index d6ee25ea98..b2212992c3 100644
> --- a/upload-pack.h
> +++ b/upload-pack.h
> @@ -12,4 +12,7 @@ struct strbuf;
>  int upload_pack_advertise(struct repository *r,
>  			  struct strbuf *value);
>  
> +/* Is this repo trusted for lazy fetching? */
> +bool upload_pack_lazy_fetch_trusted(struct repository *r);
> +
>  #endif /* UPLOAD_PACK_H */

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

[Junio C Hamano]

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

[PATCH v3 0/5] Introduce 'uploadpack.lazyFetchTrusted'

[Christian Couder]

Recently the "promisor-remote" capability was added to protocol v2,
allowing servers and clients to agree on the promisor remotes they can
safely use.

The more servers use promisor remotes, the more it is important to
properly control if they can lazy fetch when responding to a clone or
fetch request from the client.

For example, in the context of large object promisors (see
"Documentation/technical/large-object-promisors.adoc"), if a client
clones with a filter set to 100kB while the server has moved all of
the blobs >= 10kB to a promisor remote, the server will not be able to
provide blobs between 10kB and 100kB to the client, which will make
the clone fail.

Even if the `--filter=auto` option is available since ef2f1845ec
(fetch-pack: wire up and enable auto filter logic, 2026-02-16) it's
still a good idea to provide more control over lazy fetching on the
server side to server operators, as lazy fetching on the server side
could be useful in corporate environments.

Since 7b70e9efb1 (upload-pack: disable lazy-fetching by default,
2024-04-16), lazy fetching has been controlled by the
`GIT_NO_LAZY_FETCH` environment variable. This is a boolean that is
set to 'true' by default when calling `git upload-pack` for security
reasons.

The main security issue on the server side is making sure the served
repo itself is also trusted, as lazily fetching runs `git fetch`,
which may execute arbitrary commands specified in the configuration
and hooks of the served repo. The operator of the server should decide
and mark that trust, not the served repo itself, nor the client.

This series introduces a new 'uploadpack.lazyFetchTrusted' protected
configuration variable similar to 'safe.directory' (see
"Documentation/config/safe.adoc") to mark trusted repos where lazy
fetching is allowed. As it is protected, this config variable will
only take effect if it is set in global or system scope, so only
server operators can control it.

Previous related work
=====================

A previous series called "Introduce a 'fromAccepted' option to
GIT_NO_LAZY_FETCH" [1] took a different approach as it wanted to make
it easier to allow lazy fetching from accepted promisor remotes. But
after brian replied that he didn't think it was a good idea, and after
thinking about this more, my opinion now is that some promisor remotes
being accepted or not is not really relevant to the issue.

In my reply to brian, I said:

"""
Different features could be developed (in future work) to improve on
the current state:
    - a way for lazy fetching to work without reading config files,
triggering hooks, or doing potentially sensitive things,
    - an explicit way for operators to mark trusted repos (like
perhaps a server-side config the operator sets per-repo),
    - operator-defined allow/deny rules, or maybe
    - some ways/scripts/commands to scan repos and check configuration
information, remote settings and everything potentially sensitive to
decide if a repo looks safe enough to allow lazy fetching or not.
"""

So I decided to go with "an explicit way for operators to mark trusted
repos" and this series is an implementation of that.

Note that the feature developed in this series applies to protocol
v0/v1 as well as v2 while the previous one was only related to v2.

[1]: https://lore.kernel.org/git/CAP8UFD0_S9eg_w42tcNRnT9E2ntLr_eHLnzE4c2dSu67DzZoXg@mail.gmail.com/

Overview of the patches
=======================

  - Patch 1/5 is the only patch saved from the "Introduce a
    'fromAccepted' option to GIT_NO_LAZY_FETCH" series. It's not
    necessary for the rest of this series and its main feature to
    work, but I think it's a nice refactoring related to lazy
    fetching, so it might as well be part of this series. There is a
    small change in the commit message (to not mention following
    commits) compared to the version in the previous series.

  - Patch 2/5 extracts and modifies code used by the 'safe.directory'
    config variable in new path_allowlist_config_apply() and
    path_allowlist_apply() functions, so that these functions can be
    reused to process 'uploadpack.lazyFetchTrusted' in the next patch.

  - Patch 3/5 uses the new functions from the previous patch in a new
    upload_pack_lazy_fetch_trusted() function to process
    'uploadpack.lazyFetchTrusted', but the result from that processing
    isn't actually used to have a practical effect.

  - Patch 4/5, which is new in this v3, prevents infinite lazy fetch
    recursions that the following patch would otherwise make possible.
    If a repo is allowed to lazy fetch and one of its promisor remotes
    resolves back to it, for example if it is its own promisor remote
    as Junio noticed when reviewing v2, each nested `upload-pack`
    inherits `GIT_NO_LAZY_FETCH=0` and fetches again.

  - Patch 5/5 wires up the new upload_pack_lazy_fetch_trusted()
    function to decide if lazy fetching can actually be enabled.

Changes since v2
================

Thanks to Junio for reviewing the previous version.

Rebased on top of 3cb9185f65 (The 22nd batch, 2026-09-02) as the
previous version was based on a quite old commit: 745601a9a9 (mailmap:
map Elijah Newren's current and previous work addresses, 2026-08-12)
and I wanted to avoid possible merge issues.

 - Patch 1/5 has a small simplification in how try_promisor_remotes()
   is called first with its last argument set to 'true', and then with
   it set to 'false'. Both calls are now chained with `||`.

 - Patches 2/5 and 3/5 have been squashed together and reworked
   completely into the new patch 2/5, especially:

   - path_allowlist_apply() now has a
     `bool (*allow_path)(const char *path, void *cbdata)` argument so
     that callers can customize which paths they accept.

   - A new path_allowlist_config_apply() wrapper around
     path_allowlist_apply() has been added to avoid code duplication
     in the callers.

   - The `int *is_match` argument of path_allowlist_apply() has been
     changed to `bool *matches` and `int is_safe` in
     `struct safe_directory_data` has been changed to `bool safe`
     accordingly.

 - Patch 3/5 (previously 4/5) has a number of changes:

   - Its commit message has been improved and adapted to the 2 other
     big changes below.

   - It defines its own allow_trusted_path() function to customize the
     paths it accepts and pass that new function to the new functions
     from the previous commit that it uses.

   - The code and commit message have been changed so that a served
     repository is identified only by its git dir. We wrongly used to
     say that it could also be identified by its worktree, but
     `upload-pack` actually uses enter_repo(), so it doesn't know
     about worktrees.

   - `int trusted` has been changed to `bool trusted` and moved after
     the other field in `struct lazy_fetch_trusted`. This matches the
     changes to `bool *matches` and `bool safe` in the "setup.c" code.

 - Patch 4/5 is new and prevents infinite lazy fetch recursions, using
   a new `GIT_INTERNAL_LAZY_FETCH_DEPTH` environment variable to limit
   the nesting depth. See the patch 4/5 description above.

 - Patch 5/5 has a few changes:

   - The `uploadpack.lazyFetchTrusted` doc has been clarified, typo
     fixed, reorganized, and completed with information related to the
     changes in this v3, especially:
       - the fact that repos are identified by their git dir, and
       - that configuring a repo as its own remote is not a good idea.

   - Two tests have been added to make sure
     `uploadpack.lazyFetchTrusted` doesn't make infinite lazy fetch
     recursion possible, and to show that repos are identified by
     their git dir, and cannot be identified by a worktree.

CI tests
========

They all pass except for the "debian-11" one which keeps failing at
the "install git in container" step with the following error:

```
E: Release file for http://deb.debian.org/debian-security/dists/bullseye-security/InRelease
is expired (invalid since 18h 34min 49s). Updates for this repository will not be applied.
Error: Process completed with exit code 100.
```

so it is very likely unrelated to this series.

See: https://github.com/chriscool/git/actions/runs/34232995230

Range-diff compared to v2
=========================

1:  1605740203 ! 1:  9403597855 promisor-remote: factor out lazy_fetch_objects()
    @@ promisor-remote.c: static int try_promisor_remotes(struct repository *repo,
     +	promisor_remote_init(repo);
     +
     +	/* Try accepted remotes first (those the server told us to use) */
    -+	if (try_promisor_remotes(repo, remaining_oids, remaining_nr,
    -+				 to_free, true))
    -+		return true;
    -+
     +	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
    -+				    to_free, false);
    ++				    to_free, true) ||
    ++		try_promisor_remotes(repo, remaining_oids, remaining_nr,
    ++				     to_free, false);
      }
      
      void promisor_remote_get_direct(struct repository *repo,
2:  5f226b6508 < -:  ---------- setup: extract path_allowlist_apply()
3:  051aa11fc9 < -:  ---------- setup: add 'allow_dot' arg to path_allowlist_apply()
-:  ---------- > 2:  2155c4202d setup: extract path_allowlist_apply()
4:  045b5e647b ! 3:  37043ffeaf upload-pack: read uploadpack.lazyFetchTrusted
    @@ Commit message
         upload-pack: read uploadpack.lazyFetchTrusted
     
         Previous commits created and prepared the path_allowlist_apply()
    -    function.
    +    and path_allowlist_config_apply() functions, but used them only for the
    +    "safe.directory" configuration variable.
     
    -    Let's reuse this function for a new "uploadpack.lazyFetchTrusted"
    +    Let's reuse these functions for a new "uploadpack.lazyFetchTrusted"
         configuration variable.
     
         It allows us to:
    @@ Commit message
           - return the result from a new upload_pack_lazy_fetch_trusted()
             function.
     
    -    The new function will be used in a following commit.
    +    As path_allowlist_config_apply() lets each caller decide which paths
    +    it is willing to accept using a callback, let's pass it a new
    +    allow_trusted_path() callback. Unlike the "safe.directory" callback, it
    +    accepts only absolute paths, and not ".", as `upload-pack` always
    +    serves a repository given by an absolute path, so there is no "current
    +    repository" for "." to refer to.
    +
    +    Note that a served repository is identified by its git directory, and
    +    not by its worktree. This is because `upload-pack` uses enter_repo()
    +    instead of the usual repository discovery, so it never learns about a
    +    worktree and `r->worktree` is always NULL there. In practice this
    +    means that a non-bare repository served as "/srv/repo" has to be
    +    allowlisted as "/srv/repo/.git".
    +
    +    The new upload_pack_lazy_fetch_trusted() function will be used in a
    +    following commit.
     
         Note that the new config variable should be read only from protected
         configuration files.
    @@ upload-pack.c: static int upload_pack_config(const char *var, const char *value,
      	return parse_hide_refs_config(var, value, "uploadpack", &data->hidden_refs);
      }
      
    ++/*
    ++ * Only absolute paths make sense here. Unlike 'safe.directory', "."
    ++ * is not accepted, as the served repository is always identified by
    ++ * an absolute path.
    ++ */
    ++static bool allow_trusted_path(const char *path, void *cbdata_)
    ++{
    ++	struct path_allowlist_cb_data *cbdata = cbdata_;
    ++
    ++	if (is_absolute_path(path))
    ++		return true;
    ++
    ++	warning(_("%s '%s' not absolute"), cbdata->key, path);
    ++	return false;
    ++}
    ++
     +struct lazy_fetch_trusted {
    -+	int trusted;
     +	char *repo_path;
    ++	bool trusted;
     +};
     +
     +static int upload_pack_protected_lazy_fetch_config(const char *var, const char *value,
    @@ upload-pack.c: static int upload_pack_config(const char *var, const char *value,
     +						   void *cb_data)
     +{
     +	struct lazy_fetch_trusted *data = cb_data;
    ++	struct path_allowlist_cb_data cbdata = { .key = var };
     +
    -+	if (!strcmp("uploadpack.lazyfetchtrusted", var)) {
    -+		path_allowlist_apply(var, value, data->repo_path,
    -+				     &data->trusted, false);
    ++	if (strcmp("uploadpack.lazyfetchtrusted", var))
     +		return 0;
    -+	}
    ++
    ++	path_allowlist_config_apply(var, value, data->repo_path, &data->trusted,
    ++				    allow_trusted_path, &cbdata);
     +
     +	return 0;
     +}
    @@ upload-pack.c: static int upload_pack_config(const char *var, const char *value,
     +{
     +	struct lazy_fetch_trusted data = { 0 };
     +
    -+	data.repo_path = real_pathdup(r->worktree ? r->worktree : r->gitdir, 0);
    ++	/*
    ++	 * A served repository is identified by its git directory, as
    ++	 * `upload-pack` uses enter_repo() instead of the usual repository
    ++	 * discovery, so its worktree, if any, is never known here.
    ++	 */
    ++	data.repo_path = real_pathdup(r->gitdir, 0);
     +	if (!data.repo_path)
     +		return false;
     +
-:  ---------- > 4:  38fc060999 promisor-remote: prevent infinite recursion when lazy fetching
5:  c116661202 ! 5:  8cb97230e5 builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo
    @@ Documentation/config/uploadpack.adoc: uploadpack.allowRefInWant::
      	replication delay.
     +
     +uploadpack.lazyFetchTrusted::
    -+	These config entries specify repositories that `upload-pack` is
    -+	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.
    ++	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.
    +++
    ++A repository is identified by its git directory, i.e. the `.git`
    ++directory of a repository that has a worktree, or the repository itself
    ++if it is bare. So a non-bare repository served as `/srv/repo` has to be
    ++allowlisted as `/srv/repo/.git`. Giving a path with `/*` appended to it
    ++will trust all repositories under the named directory. To trust all
    ++served repositories, set `uploadpack.lazyFetchTrusted` to the string
    ++`*`.
    +++
    ++The value of this setting is interpolated, i.e. `~/<path>` expands to a
    ++path relative to the home directory and `%(prefix)/<path>` expands to a
    ++path relative to Git's (runtime) prefix.
    +++
    ++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.
     ++
     +Note that this allows lazy fetching from any promisor remote
     +configured in the served repository, not only from the promisor
    @@ Documentation/config/uploadpack.adoc: uploadpack.allowRefInWant::
     +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.
    ++repository are also trustworthy. In particular, a trusted repository
    ++should not be configured as its own promisor remote, as `upload-pack`
    ++would then try to lazily fetch missing objects from the repository
    ++itself, which is pointless.
     ++
    -+This is a multi-valued setting, i.e. you can add more than one
    ++As this is a multi-valued setting, 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`
    ++specified in the system config), add an `uploadpack.lazyFetchTrusted`
     +entry with an empty value.
     ++
    -+A repository is identified by its worktree, or its git directory for a bare
    -+repository, and the value must be an absolute path. Giving a path with `/*`
    -+appended to it will trust all repositories under the named directory. To trust
    -+all served repositories, set `uploadpack.lazyFetchTrusted` to the string `*`.
    -++
    -+The value of this setting is interpolated, i.e. `~/<path>` expands to a
    -+path relative to the home directory and `%(prefix)/<path>` expands to a
    -+path relative to Git's (runtime) prefix.
    -++
    -+Note that this configuration variable is only respected when it is specified
    -+in protected configuration (see <<SCOPES>>). This prevents untrusted
    -+repositories from tampering with this value.
    ++Note that this configuration variable is only respected when it is
    ++specified in protected configuration (see <<SCOPES>>). This prevents
    ++untrusted repositories from tampering with this value.
     
      ## Documentation/git-upload-pack.adoc ##
     @@ Documentation/git-upload-pack.adoc: This is implemented by having `upload-pack` internally set the
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
     +	# Check that the largest object is still missing on the server
     +	check_missing_objects server 1 "$oid"
     +'
    ++
    ++test_expect_success "trusted repo as its own promisor remote does not recurse" '
    ++	# No promisors are advertised
    ++	git -C server config promisor.advertise false &&
    ++	test_when_finished "rm -rf client" &&
    ++
    ++	# Add itself as its own remote
    ++	git -C server remote add self "$TRASH_DIRECTORY_URL/server" &&
    ++	git -C server config remote.self.promisor true &&
    ++	test_when_finished "git -C server remote remove self" &&
    ++
    ++	# Make "self" the only promisor remote of the server, so that it
    ++	# cannot get the missing object from "lop". Note that
    ++	# "remote.lop.partialCloneFilter" also makes "lop" a promisor
    ++	# remote, so it has to be unset too.
    ++	git -C server config --unset remote.lop.promisor &&
    ++	test_when_finished "git -C server config remote.lop.promisor true" &&
    ++	lop_filter="$(git -C server config remote.lop.partialCloneFilter)" &&
    ++	git -C server config --unset remote.lop.partialCloneFilter &&
    ++	test_when_finished "git -C server config remote.lop.partialCloneFilter \"$lop_filter\"" &&
    ++
    ++	# Allow lazy fetching from itself
    ++	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
    ++
    ++	# Check that lazy fetching fails
    ++	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
    ++	test_grep "too many nested lazy fetches" err &&
    ++
    ++	# Check that the largest object is still missing on the server
    ++	check_missing_objects server 1 "$oid"
    ++'
    ++
    ++test_expect_success "uploadpack.lazyFetchTrusted needs the git dir of a non-bare repo" '
    ++	test_when_finished "rm -rf nonbare client client2" &&
    ++
    ++	# Create a non-bare repo, without any worktree content, so that
    ++	# its largest object can be filtered out below
    ++	git init nonbare &&
    ++	git -C nonbare remote add origin "$TRASH_DIRECTORY_URL/template" &&
    ++	git -C nonbare fetch origin &&
    ++	git -C nonbare update-ref HEAD FETCH_HEAD &&
    ++
    ++	git -C nonbare remote add lop "$TRASH_DIRECTORY_URL/lop" &&
    ++	git -C nonbare config remote.lop.promisor true &&
    ++	git -C nonbare config uploadpack.allowFilter true &&
    ++	git -C nonbare config uploadpack.allowAnySHA1InWant true &&
    ++	git -C nonbare config promisor.advertise false &&
    ++
    ++	# Repack everything, then repack without the largest object and
    ++	# create a promisor pack, like initialize_server() does
    ++	git -C nonbare -c repack.writebitmaps=false repack -a -d &&
    ++	rm -f nonbare/.git/objects/pack/*.promisor &&
    ++	git -C nonbare -c repack.writebitmaps=false repack -a -d \
    ++		--filter=blob:limit=5k --filter-to="$(pwd)/nonbare-pack" &&
    ++	promisor_file=$(ls nonbare/.git/objects/pack/*.pack | sed "s/\.pack/.promisor/") &&
    ++	>"$promisor_file" &&
    ++	check_missing_objects nonbare 1 "$oid" &&
    ++
    ++	# The worktree path does not identify the repo, so it is not
    ++	# trusted and the clone fails
    ++	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/nonbare" &&
    ++	test_must_fail git clone --no-local --filter="blob:limit=1k" \
    ++		nonbare client 2>err &&
    ++	test_grep "lazy fetching disabled" err &&
    ++	check_missing_objects nonbare 1 "$oid" &&
    ++
    ++	# The git dir identifies the repo, so it is trusted and the
    ++	# clone succeeds
    ++	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/nonbare/.git" &&
    ++	git clone --no-local --filter="blob:limit=1k" nonbare client2 &&
    ++	check_missing_objects nonbare 0 ""
    ++'
     +
      test_expect_success "init + fetch with promisor.advertise set to 'true'" '
      	git -C server config promisor.advertise true &&


Christian Couder (5):
  promisor-remote: factor out lazy_fetch_objects()
  setup: extract path_allowlist_apply()
  upload-pack: read uploadpack.lazyFetchTrusted
  promisor-remote: prevent infinite recursion when lazy fetching
  builtin/upload-pack: set GIT_NO_LAZY_FETCH to 0 on trusted repo

 Documentation/config/uploadpack.adoc  |  49 +++++++++
 Documentation/git-upload-pack.adoc    |   5 +
 Documentation/git.adoc                |   4 +-
 builtin/upload-pack.c                 |  11 ++
 environment.h                         |   8 ++
 promisor-remote.c                     |  96 +++++++++++------
 setup.c                               | 138 ++++++++++++++++---------
 setup.h                               |  50 +++++++++
 t/t0410-partial-clone.sh              |  33 ++++++
 t/t5710-promisor-remote-capability.sh | 142 ++++++++++++++++++++++++++
 upload-pack.c                         |  59 +++++++++++
 upload-pack.h                         |   3 +
 12 files changed, 514 insertions(+), 84 deletions(-)


base-commit: 3cb9185f65410273787f74333cc027d2ea5daada
-- 
2.55.0.792.ged91fccac1.dirty

[PATCH v3 1/5] promisor-remote: factor out lazy_fetch_objects()

[Christian Couder]

In "promisor-remote.c:fetch_objects()", there is a check to disable
lazy fetching when the `GIT_NO_LAZY_FETCH` environment variable is
set. The fetch_objects() function is called once per promisor remote
though. So the check might be performed more times than necessary.

Also promisor_remote_get_direct() mixes up the logic deciding which
promisor remotes to try with the logic checking that the objects
that could not be fetched are promisor objects.

Let's refactor the lazy fetching logic out of these two functions
into a new lazy_fetch_objects() function.

This is a pure refactoring with no intended behavior change. Two
things shift in ways that are observably equivalent though:

  - the `GIT_NO_LAZY_FETCH` check is now performed once up front,
    instead of once per promisor remote, and

  - promisor_remote_init() is no longer called when lazy fetching
    is disabled, which is fine as nothing downstream of it, like
    is_promisor_object(), needs it in that case.

While at it, let's also convert try_promisor_remotes() to return
'bool' instead of 'int', as it just returns whether all the objects
could be fetched, and document its return value.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 promisor-remote.c | 74 +++++++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 31 deletions(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 43505d1e1a..df17fec3bb 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -31,15 +31,6 @@ static int fetch_objects(struct repository *repo,
 	FILE *child_in;
 	int quiet;
 
-	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
-		static int warning_shown;
-		if (!warning_shown) {
-			warning_shown = 1;
-			warning(_("lazy fetching disabled; some objects may not be available"));
-		}
-		return -1;
-	}
-
 	child.git_cmd = 1;
 	child.in = -1;
 	if (repo != the_repository)
@@ -270,10 +261,15 @@ static int remove_fetched_oids(struct repository *repo,
 	return remaining_nr;
 }
 
-static int try_promisor_remotes(struct repository *repo,
-				struct object_id **remaining_oids,
-				int *remaining_nr, int *to_free,
-				bool accepted_only)
+/*
+ * Return 'true' if all the objects could be fetched from the
+ * (non-)accepted remotes, 'false' otherwise.
+ */
+static bool try_promisor_remotes(struct repository *repo,
+				 struct object_id **remaining_oids,
+				 int *remaining_nr,
+				 int *to_free,
+				 bool accepted_only)
 {
 	struct promisor_remote *r = repo->promisor_remote_config->promisors;
 
@@ -290,9 +286,35 @@ static int try_promisor_remotes(struct repository *repo,
 				continue;
 			}
 		}
-		return 1; /* all fetched */
+		return true; /* all fetched */
 	}
-	return 0;
+	return false;
+}
+
+/*
+ * Return 'true' if all the objects could be fetched, 'false' otherwise.
+ */
+static bool lazy_fetch_objects(struct repository *repo,
+			       struct object_id **remaining_oids,
+			       int *remaining_nr,
+			       int *to_free)
+{
+	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
+		static int warning_shown;
+		if (!warning_shown) {
+			warning_shown = 1;
+			warning(_("lazy fetching disabled; some objects may not be available"));
+		}
+		return false;
+	}
+
+	promisor_remote_init(repo);
+
+	/* Try accepted remotes first (those the server told us to use) */
+	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				    to_free, true) ||
+		try_promisor_remotes(repo, remaining_oids, remaining_nr,
+				     to_free, false);
 }
 
 void promisor_remote_get_direct(struct repository *repo,
@@ -302,28 +324,18 @@ void promisor_remote_get_direct(struct repository *repo,
 	struct object_id *remaining_oids = (struct object_id *)oids;
 	int remaining_nr = oid_nr;
 	int to_free = 0;
-	int i;
 
 	if (oid_nr == 0)
 		return;
 
-	promisor_remote_init(repo);
-
-	/* Try accepted remotes first (those the server told us to use) */
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, true))
-		goto all_fetched;
-	if (try_promisor_remotes(repo, &remaining_oids, &remaining_nr,
-				 &to_free, false))
-		goto all_fetched;
-
-	for (i = 0; i < remaining_nr; i++) {
-		if (is_promisor_object(repo, &remaining_oids[i]))
-			die(_("could not fetch %s from promisor remote"),
-			    oid_to_hex(&remaining_oids[i]));
+	if (!lazy_fetch_objects(repo, &remaining_oids, &remaining_nr, &to_free)) {
+		for (int i = 0; i < remaining_nr; i++) {
+			if (is_promisor_object(repo, &remaining_oids[i]))
+				die(_("could not fetch %s from promisor remote"),
+				    oid_to_hex(&remaining_oids[i]));
+		}
 	}
 
-all_fetched:
 	if (to_free)
 		free(remaining_oids);
 }
-- 
2.55.0.792.ged91fccac1.dirty

[PATCH v3 2/5] setup: extract path_allowlist_apply()

[Christian Couder]

In a following commit we are going to check whether a repository is
part of an allowlist specified in a config variable.

To prepare for that let's extract existing code from
safe_directory_cb() into a new path_allowlist_apply() helper that will
help with such checks.

While at it let's make the helper's code simpler and more generic, by
passing it a `bool (*allow_path)(const char *path, void *cbdata)`
function that decides if a path is acceptable by the caller.

To further simplify how to reuse that new helper, and avoid duplicating
the config-value handling in a future commit, let's also introduce a
path_allowlist_config_apply() helper.

For clarity, let's change the `int is_safe` to `bool safe` in
`struct safe_directory_data`.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 setup.c | 138 ++++++++++++++++++++++++++++++++++++--------------------
 setup.h |  50 ++++++++++++++++++++
 2 files changed, 138 insertions(+), 50 deletions(-)

diff --git a/setup.c b/setup.c
index dfe05d9a03..366a7dc5c0 100644
--- a/setup.c
+++ b/setup.c
@@ -1338,67 +1338,105 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 	}
 }
 
+void path_allowlist_apply(const char *allowed, const char *target_path,
+			  bool *matches,
+			  bool (*allow_path)(const char *path, void *cbdata),
+			  void *allow_path_cbdata)
+{
+	char *normalized = NULL;
+
+	if (!allowed || !*allowed) {
+		*matches = false;
+		return;
+	}
+
+	if (!strcmp(allowed, "*")) {
+		*matches = true;
+		return;
+	}
+
+	if (!allow_path(allowed, allow_path_cbdata))
+		return;
+
+	/*
+	 * A .gitconfig in $HOME may be shared across different
+	 * machines and the config variable entries may or may not
+	 * exist as paths on all of these machines.  In other words,
+	 * it is not a warning worthy event when there is no such path
+	 * on this machine---the entry may be useful elsewhere.
+	 */
+	normalized = real_pathdup(allowed, 0);
+	if (!normalized)
+		return;
+
+	if (ends_with(normalized, "/*")) {
+		size_t len = strlen(normalized);
+		if (!fspathncmp(normalized, target_path, len - 1))
+			*matches = true;
+	} else if (!fspathcmp(target_path, normalized)) {
+		*matches = true;
+	}
+
+	free(normalized);
+}
+
+void path_allowlist_config_apply(const char *key, const char *value,
+				 const char *target_path, bool *matches,
+				 bool (*allow_path)(const char *path, void *cbdata),
+				 void *allow_path_cbdata)
+{
+	char *allowed = NULL;
+
+	if (!value || !*value || !strcmp(value, "*")) {
+		path_allowlist_apply(value, target_path, matches,
+				     allow_path, allow_path_cbdata);
+		return;
+	}
+
+	if (git_config_pathname(&allowed, key, value) || !allowed)
+		return;
+
+	path_allowlist_apply(allowed, target_path, matches,
+			     allow_path, allow_path_cbdata);
+
+	free(allowed);
+}
+
+/*
+ * Setting the config variable to a non-absolute path makes
+ * little sense---it won't be relative to the configuration
+ * file the item is defined in.  Except for ".", which means
+ * "if we are at the top level of a repository, then it is
+ * OK", which is slightly tighter than "*" that allows
+ * discovery.
+ */
+static bool allow_safe_dir(const char *path, void *cbdata_)
+{
+	struct path_allowlist_cb_data *cbdata = cbdata_;
+
+	if (is_absolute_path(path) || !strcmp(path, "."))
+		return true;
+
+	warning(_("%s '%s' not absolute"), cbdata->key, path);
+	return false;
+}
+
 struct safe_directory_data {
 	char *path;
-	int is_safe;
+	bool safe;
 };
 
 static int safe_directory_cb(const char *key, const char *value,
 			     const struct config_context *ctx UNUSED, void *d)
 {
 	struct safe_directory_data *data = d;
+	struct path_allowlist_cb_data cbdata = { .key = key };
 
 	if (strcmp(key, "safe.directory"))
 		return 0;
 
-	if (!value || !*value) {
-		data->is_safe = 0;
-	} else if (!strcmp(value, "*")) {
-		data->is_safe = 1;
-	} else {
-		char *allowed = NULL;
-
-		if (!git_config_pathname(&allowed, key, value) && allowed) {
-			char *normalized = NULL;
-
-			/*
-			 * Setting safe.directory to a non-absolute path
-			 * makes little sense---it won't be relative to
-			 * the configuration file the item is defined in.
-			 * Except for ".", which means "if we are at the top
-			 * level of a repository, then it is OK", which is
-			 * slightly tighter than "*" that allows discovery.
-			 */
-			if (!is_absolute_path(allowed) && strcmp(allowed, ".")) {
-				warning(_("safe.directory '%s' not absolute"),
-					allowed);
-				goto next;
-			}
-
-			/*
-			 * A .gitconfig in $HOME may be shared across
-			 * different machines and safe.directory entries
-			 * may or may not exist as paths on all of these
-			 * machines.  In other words, it is not a warning
-			 * worthy event when there is no such path on this
-			 * machine---the entry may be useful elsewhere.
-			 */
-			normalized = real_pathdup(allowed, 0);
-			if (!normalized)
-				goto next;
-
-			if (ends_with(normalized, "/*")) {
-				size_t len = strlen(normalized);
-				if (!fspathncmp(normalized, data->path, len - 1))
-					data->is_safe = 1;
-			} else if (!fspathcmp(data->path, normalized)) {
-				data->is_safe = 1;
-			}
-		next:
-			free(normalized);
-			free(allowed);
-		}
-	}
+	path_allowlist_config_apply(key, value, data->path, &data->safe,
+				    allow_safe_dir, &cbdata);
 
 	return 0;
 }
@@ -1440,7 +1478,7 @@ static int ensure_valid_ownership(const char *gitfile,
 	git_protected_config(safe_directory_cb, &data);
 
 	free(data.path);
-	return data.is_safe;
+	return data.safe;
 }
 
 void die_upon_dubious_ownership(const char *gitfile, const char *worktree,
diff --git a/setup.h b/setup.h
index 763fd384e8..6b84fbe507 100644
--- a/setup.h
+++ b/setup.h
@@ -304,4 +304,54 @@ struct startup_info {
 extern struct startup_info *startup_info;
 extern const char *tmp_original_cwd;
 
+/* Path allowlist */
+
+struct path_allowlist_cb_data {
+	const char *key;
+};
+
+/*
+ * Check the allowlist entry in `allowed` against `target_path`,
+ * updating `*matches` accordingly.
+ *
+ * `allowed` is a single entry of an allowlist of paths, typically one
+ * value of a multi-valued config variable, already expanded by
+ * git_config_pathname(). `target_path` is the (normalized) path being
+ * tested. `*matches` is updated in place:
+ *
+ *   - an empty `allowed` resets it to 'false' (so a later, more
+ *     specific config scope can clear entries from a broader one),
+ *   - "*" sets it to 'true' (allow everything),
+ *   - "<path>" sets it to 'true' if <path> equals `target_path`,
+ *   - "<path>" + "/" + "*" sets it to 'true' if <path> is a leading
+ *     directory of `target_path`,
+ *   - anything else leaves `*matches` unchanged.
+ *
+ * `allow_path` is called with `allowed` and `allow_path_cbdata`, and
+ * should return 'true' if the entry is acceptable to the caller. It
+ * lets each caller decide which paths it is willing to consider, and
+ * whether to warn about the ones it rejects. Returning 'false' leaves
+ * `*matches` unchanged.
+ *
+ * Callers are expected to invoke this once per allowlist entry,
+ * typically from a protected-config callback, so that untrusted
+ * repository config cannot influence the decision.
+ */
+void path_allowlist_apply(const char *allowed, const char *target_path,
+			  bool *matches,
+			  bool (*allow_path)(const char *path, void *cbdata),
+			  void *allow_path_cbdata);
+
+/*
+ * Apply one value of a multi-valued config variable holding an
+ * allowlist of paths, expanding it with git_config_pathname() before
+ * checking it against `target_path`. Empty and "*" values are passed
+ * through without expansion, as interpolating them is not
+ * meaningful. See path_allowlist_apply().
+ */
+void path_allowlist_config_apply(const char *key, const char *value,
+				 const char *target_path, bool *matches,
+				 bool (*allow_path)(const char *path, void *cbdata),
+				 void *allow_path_cbdata);
+
 #endif /* SETUP_H */
-- 
2.55.0.792.ged91fccac1.dirty

[PATCH v3 3/5] upload-pack: read uploadpack.lazyFetchTrusted

[Christian Couder]

Previous commits created and prepared the path_allowlist_apply()
and path_allowlist_config_apply() functions, but used them only for the
"safe.directory" configuration variable.

Let's reuse these functions for a new "uploadpack.lazyFetchTrusted"
configuration variable.

It allows us to:

  - read an allowlist from that config variable,
  - check if the current repo is in that list, and
  - return the result from a new upload_pack_lazy_fetch_trusted()
    function.

As path_allowlist_config_apply() lets each caller decide which paths
it is willing to accept using a callback, let's pass it a new
allow_trusted_path() callback. Unlike the "safe.directory" callback, it
accepts only absolute paths, and not ".", as `upload-pack` always
serves a repository given by an absolute path, so there is no "current
repository" for "." to refer to.

Note that a served repository is identified by its git directory, and
not by its worktree. This is because `upload-pack` uses enter_repo()
instead of the usual repository discovery, so it never learns about a
worktree and `r->worktree` is always NULL there. In practice this
means that a non-bare repository served as "/srv/repo" has to be
allowlisted as "/srv/repo/.git".

The new upload_pack_lazy_fetch_trusted() function will be used in a
following commit.

Note that the new config variable should be read only from protected
configuration files.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 upload-pack.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
 upload-pack.h |  3 +++
 2 files changed, 62 insertions(+)

diff --git a/upload-pack.c b/upload-pack.c
index 22573ad365..a300870fa9 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -34,6 +34,8 @@
 #include "json-writer.h"
 #include "strmap.h"
 #include "promisor-remote.h"
+#include "setup.h"
+#include "abspath.h"
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE	(1u << 11)
@@ -1343,6 +1345,63 @@ static int upload_pack_config(const char *var, const char *value,
 	return parse_hide_refs_config(var, value, "uploadpack", &data->hidden_refs);
 }
 
+/*
+ * Only absolute paths make sense here. Unlike 'safe.directory', "."
+ * is not accepted, as the served repository is always identified by
+ * an absolute path.
+ */
+static bool allow_trusted_path(const char *path, void *cbdata_)
+{
+	struct path_allowlist_cb_data *cbdata = cbdata_;
+
+	if (is_absolute_path(path))
+		return true;
+
+	warning(_("%s '%s' not absolute"), cbdata->key, path);
+	return false;
+}
+
+struct lazy_fetch_trusted {
+	char *repo_path;
+	bool trusted;
+};
+
+static int upload_pack_protected_lazy_fetch_config(const char *var, const char *value,
+						   const struct config_context *ctx UNUSED,
+						   void *cb_data)
+{
+	struct lazy_fetch_trusted *data = cb_data;
+	struct path_allowlist_cb_data cbdata = { .key = var };
+
+	if (strcmp("uploadpack.lazyfetchtrusted", var))
+		return 0;
+
+	path_allowlist_config_apply(var, value, data->repo_path, &data->trusted,
+				    allow_trusted_path, &cbdata);
+
+	return 0;
+}
+
+bool upload_pack_lazy_fetch_trusted(struct repository *r)
+{
+	struct lazy_fetch_trusted data = { 0 };
+
+	/*
+	 * A served repository is identified by its git directory, as
+	 * `upload-pack` uses enter_repo() instead of the usual repository
+	 * discovery, so its worktree, if any, is never known here.
+	 */
+	data.repo_path = real_pathdup(r->gitdir, 0);
+	if (!data.repo_path)
+		return false;
+
+	git_protected_config(upload_pack_protected_lazy_fetch_config, &data);
+
+	free(data.repo_path);
+
+	return !!data.trusted;
+}
+
 static int upload_pack_protected_config(const char *var, const char *value,
 					const struct config_context *ctx UNUSED,
 					void *cb_data)
diff --git a/upload-pack.h b/upload-pack.h
index d6ee25ea98..b2212992c3 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -12,4 +12,7 @@ struct strbuf;
 int upload_pack_advertise(struct repository *r,
 			  struct strbuf *value);
 
+/* Is this repo trusted for lazy fetching? */
+bool upload_pack_lazy_fetch_trusted(struct repository *r);
+
 #endif /* UPLOAD_PACK_H */
-- 
2.55.0.792.ged91fccac1.dirty

[PATCH v3 4/5] promisor-remote: prevent infinite recursion when lazy fetching

[Christian Couder]

If a repository R is configured to lazy fetch from a promisor remote P
which is also configured to in turn lazy fetch from R, there is an
infinite recursion: R asks P for a missing object, P asks R for it,
and so on. The simplest case of this is a repository configured as its
own promisor remote.

This is not reachable when serving a repository by default, as
`upload-pack` sets `GIT_NO_LAZY_FETCH` to 1, which makes the nested
`upload-pack` refuse to lazily fetch. A following commit will let
server operators allow lazy fetching for repositories they trust
though, and as `GIT_NO_LAZY_FETCH` is then set to 0 and passed down to
child processes, nothing stops the recursion anymore.

It does not recurse forever in practice, but only because each level
adds one more variable to the environment of the child process, so
after a while `exec()` fails with:

    fatal: cannot exec 'git-upload-pack ...': Argument list too long
    fatal: unable to fork

To avoid this pathological case altogether, let's use a new
`GIT_INTERNAL_LAZY_FETCH_DEPTH` to count the recursion depth, and let's
check that it doesn't exceed a MAX_LAZY_FETCH_DEPTH limit (set to 5 for
now).

Note that some nesting is legitimate: when `git fetch` runs
`index-pack`, it can lazily fetch REF_DELTA bases that are missing
locally, so the limit should not be 1.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 environment.h            |  8 ++++++++
 promisor-remote.c        | 26 ++++++++++++++++++++++----
 t/t0410-partial-clone.sh | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/environment.h b/environment.h
index e7ec5b0437..f2833be9fe 100644
--- a/environment.h
+++ b/environment.h
@@ -52,6 +52,14 @@
  */
 #define GIT_ADVICE_ENVIRONMENT "GIT_ADVICE"
 
+/*
+ * Environment variable used to detect that a lazy fetch is already in
+ * progress in a parent process, to prevent infinite recursion when a
+ * promisor remote resolves back to the repository being served.
+ * This is an internal variable that should not be set by the user.
+ */
+#define LAZY_FETCH_DEPTH_ENVIRONMENT "GIT_INTERNAL_LAZY_FETCH_DEPTH"
+
 /*
  * Environment variable used in handshaking the wire protocol.
  * Contains a colon ':' separated list of keys with optional values
diff --git a/promisor-remote.c b/promisor-remote.c
index df17fec3bb..e9c5b413f1 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -24,7 +24,7 @@ struct promisor_remote_config {
 static int fetch_objects(struct repository *repo,
 			 const char *remote_name,
 			 const struct object_id *oids,
-			 int oid_nr)
+			 int oid_nr, unsigned long depth)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 	int i;
@@ -41,6 +41,7 @@ static int fetch_objects(struct repository *repo,
 		     "--filter=blob:none", "--stdin", NULL);
 	if (!repo_config_get_bool(repo, "promisor.quiet", &quiet) && quiet)
 		strvec_push(&child.args, "--quiet");
+	strvec_pushf(&child.env, "%s=%lu", LAZY_FETCH_DEPTH_ENVIRONMENT, depth + 1);
 	if (start_command(&child))
 		die(_("promisor-remote: unable to fork off fetch subprocess"));
 	child_in = xfdopen(child.in, "w");
@@ -269,6 +270,7 @@ static bool try_promisor_remotes(struct repository *repo,
 				 struct object_id **remaining_oids,
 				 int *remaining_nr,
 				 int *to_free,
+				 unsigned long depth,
 				 bool accepted_only)
 {
 	struct promisor_remote *r = repo->promisor_remote_config->promisors;
@@ -276,7 +278,8 @@ static bool try_promisor_remotes(struct repository *repo,
 	for (; r; r = r->next) {
 		if (accepted_only != r->accepted)
 			continue;
-		if (fetch_objects(repo, r->name, *remaining_oids, *remaining_nr) < 0) {
+		if (fetch_objects(repo, r->name,
+				  *remaining_oids, *remaining_nr, depth) < 0) {
 			if (*remaining_nr == 1)
 				continue;
 			*remaining_nr = remove_fetched_oids(repo, remaining_oids,
@@ -291,6 +294,8 @@ static bool try_promisor_remotes(struct repository *repo,
 	return false;
 }
 
+#define MAX_LAZY_FETCH_DEPTH 5
+
 /*
  * Return 'true' if all the objects could be fetched, 'false' otherwise.
  */
@@ -299,6 +304,8 @@ static bool lazy_fetch_objects(struct repository *repo,
 			       int *remaining_nr,
 			       int *to_free)
 {
+	unsigned long depth = git_env_ulong(LAZY_FETCH_DEPTH_ENVIRONMENT, 0);
+
 	if (git_env_bool(NO_LAZY_FETCH_ENVIRONMENT, 0)) {
 		static int warning_shown;
 		if (!warning_shown) {
@@ -308,13 +315,24 @@ static bool lazy_fetch_objects(struct repository *repo,
 		return false;
 	}
 
+	if (depth >= MAX_LAZY_FETCH_DEPTH) {
+		static int warning_shown;
+		if (!warning_shown) {
+			warning_shown = 1;
+			warning(_("too many nested lazy fetches (%lu); "
+				  "is a promisor remote pointing at the repository itself?"),
+				depth);
+		}
+		return false;
+	}
+
 	promisor_remote_init(repo);
 
 	/* Try accepted remotes first (those the server told us to use) */
 	return try_promisor_remotes(repo, remaining_oids, remaining_nr,
-				    to_free, true) ||
+				    to_free, depth, true) ||
 		try_promisor_remotes(repo, remaining_oids, remaining_nr,
-				     to_free, false);
+				     to_free, depth, false);
 }
 
 void promisor_remote_get_direct(struct repository *repo,
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 788e9a1631..a54685e3c7 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -709,6 +709,39 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '
 	test_grep ! "[?]$FILE_HASH" out
 '
 
+test_expect_success 'lazy-fetch does not recurse infinitely between two promisor remotes' '
+	rm -rf full partial1.git partial2.git &&
+
+	# Create a repo with a blob
+	test_create_repo full &&
+	test_config -C full uploadpack.allowfilter 1 &&
+	test_config -C full uploadpack.allowanysha1inwant 1 &&
+	test_commit -C full create-a-file file.txt &&
+	FILE_HASH=$(git -C full rev-parse HEAD:file.txt) &&
+
+	# Create partial clone repos without blobs
+	git clone --filter=blob:none --bare "file://$(pwd)/full" partial1.git &&
+	git clone --filter=blob:none --bare "file://$(pwd)/full" partial2.git &&
+	test_config -C partial1.git uploadpack.allowfilter 1 &&
+	test_config -C partial1.git uploadpack.allowanysha1inwant 1 &&
+	test_config -C partial2.git uploadpack.allowfilter 1 &&
+	test_config -C partial2.git uploadpack.allowanysha1inwant 1 &&
+
+	# Configure the partial repos as remotes of each other
+	git -C partial2.git remote set-url origin "file://$(pwd)/partial1.git" &&
+	git -C partial1.git remote set-url origin "file://$(pwd)/partial2.git" &&
+
+	# Make sure lazy fetching fails
+	test_must_fail env GIT_TRACE="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 \
+		git -C partial1.git cat-file -e "$FILE_HASH" 2>err &&
+	test_grep "too many nested lazy fetches" err &&
+
+	# Make sure the recursion was bounded, i.e. that only
+	# MAX_LAZY_FETCH_DEPTH "git fetch" subprocesses were spawned
+	grep "run_command: GIT_INTERNAL_LAZY_FETCH_DEPTH" trace >fetches &&
+	test_line_count = 5 fetches
+'
+
 test_expect_success 'push should not fetch new commit objects' '
 	rm -rf server client &&
 	test_create_repo server &&
-- 
2.55.0.792.ged91fccac1.dirty

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

[Christian Couder]

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.

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, as the main security issue is that lazily fetching runs `git
fetch`, which may execute arbitrary commands specified in the
configuration and hooks of the served repo.

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.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 Documentation/config/uploadpack.adoc  |  49 +++++++++
 Documentation/git-upload-pack.adoc    |   5 +
 Documentation/git.adoc                |   4 +-
 builtin/upload-pack.c                 |  11 ++
 t/t5710-promisor-remote-capability.sh | 142 ++++++++++++++++++++++++++
 5 files changed, 210 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/uploadpack.adoc b/Documentation/config/uploadpack.adoc
index 0e1dda944a..e143de93aa 100644
--- a/Documentation/config/uploadpack.adoc
+++ b/Documentation/config/uploadpack.adoc
@@ -86,3 +86,52 @@ 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::
+	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.
++
+A repository is identified by its git directory, i.e. the `.git`
+directory of a repository that has a worktree, or the repository itself
+if it is bare. So a non-bare repository served as `/srv/repo` has to be
+allowlisted as `/srv/repo/.git`. Giving a path with `/*` appended to it
+will trust all repositories under the named directory. To trust all
+served repositories, set `uploadpack.lazyFetchTrusted` to the string
+`*`.
++
+The value of this setting is interpolated, i.e. `~/<path>` expands to a
+path relative to the home directory and `%(prefix)/<path>` expands to a
+path relative to Git's (runtime) prefix.
++
+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.
++
+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. In particular, a trusted repository
+should not be configured as its own promisor remote, as `upload-pack`
+would then try to lazily fetch missing objects from the repository
+itself, which is pointless.
++
+As this is a multi-valued setting, 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 an `uploadpack.lazyFetchTrusted`
+entry with an empty value.
++
+Note that this configuration variable is only respected when it is
+specified in protected configuration (see <<SCOPES>>). This prevents
+untrusted repositories from tampering with this value.
diff --git a/Documentation/git-upload-pack.adoc b/Documentation/git-upload-pack.adoc
index 9167a321d0..90c2ba1194 100644
--- a/Documentation/git-upload-pack.adoc
+++ b/Documentation/git-upload-pack.adoc
@@ -71,6 +71,11 @@ This is implemented by having `upload-pack` internally set the
 (because you are fetching from a partial clone, and you are sure
 you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to
 `0`.
++
+Instead of setting `GIT_NO_LAZY_FETCH` to `0` in the environment, a
+server operator can allow lazy fetching on a per-repository basis by
+listing trusted repositories in the `uploadpack.lazyFetchTrusted`
+configuration variable. See linkgit:git-config[1].
 
 SECURITY
 --------
diff --git a/Documentation/git.adoc b/Documentation/git.adoc
index 8a5cdd3b3d..2e763d1f93 100644
--- a/Documentation/git.adoc
+++ b/Documentation/git.adoc
@@ -949,7 +949,9 @@ for full details.
 `GIT_NO_LAZY_FETCH`::
 	Setting this Boolean environment variable to true tells Git
 	not to lazily fetch missing objects from the promisor remote
-	on demand.
+	on demand. On the server side, the `uploadpack.lazyFetchTrusted`
+	configuration variable can control this per-repository. See
+	linkgit:git-upload-pack[1].
 
 `GIT_REFLOG_ACTION`::
 	When a ref is updated, reflog entries are created to keep
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);
 
 	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);
+
 	switch (determine_protocol_version_server()) {
 	case protocol_v2:
 		if (advertise_refs)
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 549acff23f..62f4b56006 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -173,6 +173,148 @@ test_expect_success "clone with promisor.acceptfromserver set to 'None'" '
 	initialize_server 1 "$oid"
 '
 
+test_expect_success "clone with uploadpack.lazyFetchTrusted" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Clone without GIT_NO_LAZY_FETCH=0
+	git clone --no-local --filter="blob:limit=5k" server client &&
+
+	# Check that the largest object is not missing on the server
+	# This means the server lazy fetched it
+	check_missing_objects server 0 "" &&
+
+	# Reinitialize server so that the largest object is missing again
+	initialize_server 1 "$oid"
+'
+
+test_expect_success "clone without uploadpack.lazyFetchTrusted fails" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# Note: no uploadpack.lazyFetchTrusted config is set here, so
+	# the served repo is NOT trusted for lazy fetching.
+
+	# Clone without GIT_NO_LAZY_FETCH=0 fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "uploadpack.lazyFetchTrusted is ignored in repo config" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching, but this is
+	# done in the repo config, not in protected config, so this is
+	# ignored.
+	test_config -C server uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Clone without GIT_NO_LAZY_FETCH=0 fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "explicit GIT_NO_LAZY_FETCH overrides uploadpack.lazyFetchTrusted" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# The served repo is trusted for lazy fetching
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# But GIT_NO_LAZY_FETCH=1 disables lazy fetching, so clone fails
+	test_must_fail env GIT_NO_LAZY_FETCH=1 git clone --no-local \
+		--filter="blob:limit=5k" server client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "trusted repo as its own promisor remote does not recurse" '
+	# No promisors are advertised
+	git -C server config promisor.advertise false &&
+	test_when_finished "rm -rf client" &&
+
+	# Add itself as its own remote
+	git -C server remote add self "$TRASH_DIRECTORY_URL/server" &&
+	git -C server config remote.self.promisor true &&
+	test_when_finished "git -C server remote remove self" &&
+
+	# Make "self" the only promisor remote of the server, so that it
+	# cannot get the missing object from "lop". Note that
+	# "remote.lop.partialCloneFilter" also makes "lop" a promisor
+	# remote, so it has to be unset too.
+	git -C server config --unset remote.lop.promisor &&
+	test_when_finished "git -C server config remote.lop.promisor true" &&
+	lop_filter="$(git -C server config remote.lop.partialCloneFilter)" &&
+	git -C server config --unset remote.lop.partialCloneFilter &&
+	test_when_finished "git -C server config remote.lop.partialCloneFilter \"$lop_filter\"" &&
+
+	# Allow lazy fetching from itself
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/server" &&
+
+	# Check that lazy fetching fails
+	test_must_fail git clone --no-local --filter="blob:limit=5k" server client 2>err &&
+	test_grep "too many nested lazy fetches" err &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "uploadpack.lazyFetchTrusted needs the git dir of a non-bare repo" '
+	test_when_finished "rm -rf nonbare client client2" &&
+
+	# Create a non-bare repo, without any worktree content, so that
+	# its largest object can be filtered out below
+	git init nonbare &&
+	git -C nonbare remote add origin "$TRASH_DIRECTORY_URL/template" &&
+	git -C nonbare fetch origin &&
+	git -C nonbare update-ref HEAD FETCH_HEAD &&
+
+	git -C nonbare remote add lop "$TRASH_DIRECTORY_URL/lop" &&
+	git -C nonbare config remote.lop.promisor true &&
+	git -C nonbare config uploadpack.allowFilter true &&
+	git -C nonbare config uploadpack.allowAnySHA1InWant true &&
+	git -C nonbare config promisor.advertise false &&
+
+	# Repack everything, then repack without the largest object and
+	# create a promisor pack, like initialize_server() does
+	git -C nonbare -c repack.writebitmaps=false repack -a -d &&
+	rm -f nonbare/.git/objects/pack/*.promisor &&
+	git -C nonbare -c repack.writebitmaps=false repack -a -d \
+		--filter=blob:limit=5k --filter-to="$(pwd)/nonbare-pack" &&
+	promisor_file=$(ls nonbare/.git/objects/pack/*.pack | sed "s/\.pack/.promisor/") &&
+	>"$promisor_file" &&
+	check_missing_objects nonbare 1 "$oid" &&
+
+	# The worktree path does not identify the repo, so it is not
+	# trusted and the clone fails
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/nonbare" &&
+	test_must_fail git clone --no-local --filter="blob:limit=1k" \
+		nonbare client 2>err &&
+	test_grep "lazy fetching disabled" err &&
+	check_missing_objects nonbare 1 "$oid" &&
+
+	# The git dir identifies the repo, so it is trusted and the
+	# clone succeeds
+	test_config_global uploadpack.lazyFetchTrusted "$(pwd)/nonbare/.git" &&
+	git clone --no-local --filter="blob:limit=1k" nonbare client2 &&
+	check_missing_objects nonbare 0 ""
+'
+
 test_expect_success "init + fetch with promisor.advertise set to 'true'" '
 	git -C server config promisor.advertise true &&
 	test_when_finished "rm -rf client" &&
-- 
2.55.0.792.ged91fccac1.dirty

Re: [PATCH v2 2/5] setup: extract path_allowlist_apply()

[Christian Couder]

On Fri, Aug 14, 2026 at 7:56 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > diff --git a/setup.c b/setup.c
> > index 95909e9603..39dfa1cc5f 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -1339,6 +1339,64 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,

[...]

> > +
> > +     if (ends_with(normalized, "/*")) {
> > +             size_t len = strlen(normalized);
> > +             if (!fspathncmp(normalized, target_path, len - 1))
> > +                     *is_match = 1;
> > +             goto end;
> > +     }
> > +
> > +     if (!fspathcmp(target_path, normalized))
> > +             *is_match = 1;
> > +
> > +end:
> > +     free(normalized);
> > +     free(allowed);
> > +}
>
> The name "is_match" somehow feels a bit awkward.  How about calling
> it
>
>     *matches = true/false;
>
> instead?

It was `int is_match` to match with `int is_safe` in `struct
safe_directory_data`, as the function is called this way:

path_allowlist_apply(key, value, data->path, &data->is_safe);

But OK, I have changed both `int is_match` and `int is_safe` to `bool
matches` and `bool safe` in the v3 I just sent.

Thanks.

Re: [PATCH v2 3/5] setup: add 'allow_dot' arg to path_allowlist_apply()

[Christian Couder]

On Fri, Aug 14, 2026 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:

> If this is just "I want to add an extra caller that has specific
> need and do not care about others in the future", this may be OK but
> as a public function, this is a bit disappointing API design.

I thought that flags might be enough at least for some time, but I
agree that it could soon make the code difficult to reason about,
which is not a good thing for this kind of code.

> I expected, as a generally useful function, you would instead add a
> callback function to allow replacing the use of is_absoute_path()
> plus the warning there, i.e.
>
> void path_allowlist_apply(const char *key, const char *value,
>                           const char *target_path, bool *matches,
>                           bool (*allow_path)(const char *path))
> {
>         ...
>
>         if (!allow_path(allowed))
>                 goto end;
>
> Also to avoid limiting this to configuration callback, I might
> recommend to have it be more like this:
>
> void path_allowlist_apply(const char *allowed, const char *target_path,
>                           bool *matches,
>                           bool (*allow_path)(const char *path, void *cbdata),
>                           void *allow_path_cbdata)
>
> where the original safe-directory thing may call
> git_config_pathname() to compute allowed before calling this helper,
> and pass the address of something like:
>
>         struct { const char *key, *value } cbdata = {
>                 .key = key, .value = value;
>         };
>
> as the cbdata, and pass something like this
>
>         static bool allow_safe_dir(const char *path, void *cbdata_)
>         {
>                 struct { const char *key, *value } *cbdata = _cbdata;
>                 if (is_absoute_path(path) || !strcmp(path, ".")
>                         return true; /* ok */
>
>                 warning(_("%s '%s' not absolute"), cbdata->key, path);
>                 return false;
>         }
>
> as the allow_path callback function.  IOW warning, or insisting on
> it being absolute, etc., does not have to be carved in stone.

I have tried to implement it like you suggest in the v3 I just sent.

Thanks.

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

[Christian Couder]

On Fri, Aug 14, 2026 at 9:35 PM Junio C Hamano <gitster@pobox.com> wrote:

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

I have used that in the v3 I just sent.

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

The v3 has improved regarding this as I think it makes it clearer that
repos are identified by having their git dir, or a parent directory of
it, in this config variable.

> > +     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 😛).

Actually it happens that it could recursively lazy fetch in v2, but
this has been fixed with a new patch and a few tests in v3. Thanks for
the suggestion.

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

Fixed in v3.

Thanks.

Re: [PATCH v2 1/5] promisor-remote: factor out lazy_fetch_objects()

[Christian Couder]

On Fri, Aug 14, 2026 at 7:49 PM Junio C Hamano <gitster@pobox.com> wrote:

> Perhaps writing it this way would make it easier to tell what is
> going on.  We try the preferred ones first, and then fall back to
> the other ones.
>
>         return (try_promisor_remotes(..., true) ||
>                 try_promisor_remotes(..., false));

Yes, this is used in v3.

> But more importantly, I wonder if keeping the list of missing object
> names in memory will later turn out to be problematic in real-life
> applications.  Without knowing much about how the current code for
> bulk dehydrating promisor objects is structured, I expected an API
> that looks more like:
>
>  - bulk_download_begin(): performs the early part of
>    fetch_objects(), sets up connections to the promisor remote(s),
>    and calls start_command() on the child process.
>
>  - bulk_download_this(): after calling the _begin() function above,
>    it runs around and collects missing objects that it needs to do
>    its work.  For each such missing object it discovers, this
>    function is called, which sends the object name down the
>    '--stdin' file descriptor.
>
>  - bulk_download_done(): tells the child process that we are done
>    feeding object names.
>
> but that is not what I am seeing.  I guess the current arrangement
> cannot be avoided, because we are going to fetch from more than one
> promisor remote.  Under such constraints, the way to deal with a
> massive number of missing objects will not be "streaming" like I
> imagined above, but needs to be done differently, like spooling to a
> file or something silly like that.
>
> In any case, except that this avoids checking the environment
> variable multiple times, I can see that it is a no-op refactoring of
> the existing code.

Yeah, I prefer to avoid working on a big refactoring in this area
until we have evidence showing that there is a bottleneck here.

> Nice and cleanly done.

Thanks.

Re: [PATCH v3 1/5] promisor-remote: factor out lazy_fetch_objects()

[Junio C Hamano]

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

> In "promisor-remote.c:fetch_objects()", there is a check to disable
> lazy fetching when the `GIT_NO_LAZY_FETCH` environment variable is
> set. The fetch_objects() function is called once per promisor remote
> though. So the check might be performed more times than necessary.
>
> Also promisor_remote_get_direct() mixes up the logic deciding which
> promisor remotes to try with the logic checking that the objects
> that could not be fetched are promisor objects.
>
> Let's refactor the lazy fetching logic out of these two functions
> into a new lazy_fetch_objects() function.
>
> This is a pure refactoring with no intended behavior change. Two
> things shift in ways that are observably equivalent though:
>
>   - the `GIT_NO_LAZY_FETCH` check is now performed once up front,
>     instead of once per promisor remote, and
>
>   - promisor_remote_init() is no longer called when lazy fetching
>     is disabled, which is fine as nothing downstream of it, like
>     is_promisor_object(), needs it in that case.

Yeah, I too noticed these while reading the patch.  The latter
change may be a very good thing, in that the calling sequence around
promisor_remote_init() seems to be anybody who needs to access the
promisor remote information is expected to _init() the system
beforehand.  If it were "call _init() once at the very beginning and
then do random things on promisor remotes", then moving its callsite
may have to be done more carefully, but with the "user makes sure it
is initialized beforehand" convention, the postimage of this patch
follows the pattern exactly.

> While at it, let's also convert try_promisor_remotes() to return
> 'bool' instead of 'int', as it just returns whether all the objects
> could be fetched, and document its return value.

Meh.

> +/*
> + * Return 'true' if all the objects could be fetched from the
> + * (non-)accepted remotes, 'false' otherwise.
> + */

The comment was not quite understandable, at least to me,
especially around "from the (non-)accepted" part of the sentence.

Also "could be fetched" made it sound as if this were dry-run but
isn't this function actually doing the fetching and reporting if
everything got fetched or there are still objects remaining to be
fetched?

    /*
     * fetch remaining objects (given in remaining_oids) from
     * the known promisor remotes.  If accepted_only is true,
     * ignore promisor remotes with .accepted member unset.
     * return true when all requested objects have been fetched,
     * false otherwise.
     */

The above only mentions half of how the remaining_oids parameter is
used (i.e., only on the input side), but if we are adding a comment,
we should document how remaining_oids and to_free are used as well.

The semantics of to_free in the entire callchain is especially
tricky to describe correctly, I am afraid.

Re: [PATCH v3 2/5] setup: extract path_allowlist_apply()

[Junio C Hamano]

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

> For clarity, let's change the `int is_safe` to `bool safe` in
> `struct safe_directory_data`.

I am not sure if this clarifies, though.

> diff --git a/setup.c b/setup.c
> index dfe05d9a03..366a7dc5c0 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1338,67 +1338,105 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
>  	}
>  }
>  
> +void path_allowlist_apply(const char *allowed, const char *target_path,
> +			  bool *matches,
> +			  bool (*allow_path)(const char *path, void *cbdata),
> +			  void *allow_path_cbdata)
> +{
> +	char *normalized = NULL;
> +
> +	if (!allowed || !*allowed) {
> +		*matches = false;
> +		return;
> +	}
> +
> +	if (!strcmp(allowed, "*")) {
> +		*matches = true;
> +		return;
> +	}
> +
> +	if (!allow_path(allowed, allow_path_cbdata))
> +		return;
> +
> +	/*
> +	 * A .gitconfig in $HOME may be shared across different
> +	 * machines and the config variable entries may or may not
> +	 * exist as paths on all of these machines.  In other words,
> +	 * it is not a warning worthy event when there is no such path
> +	 * on this machine---the entry may be useful elsewhere.
> +	 */

This is inherited from the preimage and not something you would want
to fix in this patch, but I do not think ignoring missing path like
this is healthy.  You do not know if the path given is missing by
design (i.e., the set of paths is union of paths that could exist)
or if it is missing due to an error (i.e., a filesystem that should
have been mounted is not mounted).  In the latter case, ignoring it
may make the system behave in a way that the user did not intend to.


> +	normalized = real_pathdup(allowed, 0);
> +	if (!normalized)
> +		return;
> +
> +	if (ends_with(normalized, "/*")) {
> +		size_t len = strlen(normalized);
> +		if (!fspathncmp(normalized, target_path, len - 1))
> +			*matches = true;
> +	} else if (!fspathcmp(target_path, normalized)) {
> +		*matches = true;
> +	}
> +
> +	free(normalized);
> +}

Re: [PATCH v2 2/5] setup: extract path_allowlist_apply()

[Junio C Hamano]

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

> path_allowlist_apply(key, value, data->path, &data->is_safe);
>
> But OK, I have changed both `int is_match` and `int is_safe` to `bool
> matches` and `bool safe` in the v3 I just sent.

I hate to say this but I think is_safe was perfectly good.  is_match
was not quite grammatrical (it is either "matches" ir "is_a_match"),
but "is_safe" is perfectly fine.

Re: [PATCH v3 4/5] promisor-remote: prevent infinite recursion when lazy fetching

[Junio C Hamano]

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

> It does not recurse forever in practice, but only because each level
> adds one more variable to the environment of the child process, so
> after a while `exec()` fails with:
>
>     fatal: cannot exec 'git-upload-pack ...': Argument list too long
>     fatal: unable to fork
>
> To avoid this pathological case altogether, let's use a new
> `GIT_INTERNAL_LAZY_FETCH_DEPTH` to count the recursion depth, and let's
> check that it doesn't exceed a MAX_LAZY_FETCH_DEPTH limit (set to 5 for
> now).

Good.

Does it have to be "unsigned long", though?  Just like oid_nr, I'd
prefer to see a number whose range or signedness does not matter in
practice be typed as platform natural "int".  Even though one could
argue that "anything_nr cannot be negative so it must be unsigned",
or "int might be too small for some platforms" or "int or ulong have
different width on different platforms", or even "anything we count
we should count in size_t", I do not think any of them is a good
argument against it, especially when the value we start with is 5
;-).

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

[Junio C Hamano]

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.

Re: [PATCH v3 4/5] promisor-remote: prevent infinite recursion when lazy fetching

[Christian Couder]

On Tue, Sep 8, 2026 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > It does not recurse forever in practice, but only because each level
> > adds one more variable to the environment of the child process, so
> > after a while `exec()` fails with:
> >
> >     fatal: cannot exec 'git-upload-pack ...': Argument list too long
> >     fatal: unable to fork
> >
> > To avoid this pathological case altogether, let's use a new
> > `GIT_INTERNAL_LAZY_FETCH_DEPTH` to count the recursion depth, and let's
> > check that it doesn't exceed a MAX_LAZY_FETCH_DEPTH limit (set to 5 for
> > now).
>
> Good.
>
> Does it have to be "unsigned long", though?  Just like oid_nr, I'd
> prefer to see a number whose range or signedness does not matter in
> practice be typed as platform natural "int".  Even though one could
> argue that "anything_nr cannot be negative so it must be unsigned",
> or "int might be too small for some platforms" or "int or ulong have
> different width on different platforms", or even "anything we count
> we should count in size_t", I do not think any of them is a good
> argument against it, especially when the value we start with is 5
> ;-).

I agree that using a plain "int" seems like the most straightforward,
but we don't have git_env_int() while we have git_env_ulong().

So would you be fine with something like:

    int depth = (int)git_env_ulong(LAZY_FETCH_DEPTH_ENVIRONMENT, 0);

which is similar to the following in builtin/pack-objects.c:

    name_hash_version = (int)git_env_ulong("GIT_TEST_NAME_HASH_VERSION", 1);

? Or do you think it's time to introduce git_env_int() in a preparatory patch?

Re: [PATCH v3 4/5] promisor-remote: prevent infinite recursion when lazy fetching

[Junio C Hamano]

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

> I agree that using a plain "int" seems like the most straightforward,
> but we don't have git_env_int() while we have git_env_ulong().
>
> So would you be fine with something like:
>
>     int depth = (int)git_env_ulong(LAZY_FETCH_DEPTH_ENVIRONMENT, 0);
>
> which is similar to the following in builtin/pack-objects.c:
>
>     name_hash_version = (int)git_env_ulong("GIT_TEST_NAME_HASH_VERSION", 1);
>
> ? Or do you think it's time to introduce git_env_int() in a preparatory patch?

There are 13 existing callers, among which one that you found
explicitly casts to int, but many others make assignments with
implicit cast (e.g., members of bloom_settings used in
commit-graph.c are of type uint32_t), and config.c reads
GIT_TEST_INDEX_THREADS into an "int val" with implicit cast.
progress.c:get_defalut_delay() does the same.

So I would say that it is up to you to pile on existing technical
debt by mimicking config.c:repo_config_get_index_threads() and
progress.c:get_default_delay(), or audit all callers of
git_env_ulong() and migrate appropriate ones among them to use
git_env_int().  From my cursory survey, I suspect that not many
callers of git_get_ulong() would survive.

Thanks.