[v3,4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'
diff mbox series

Message ID c9d71809f49dc1b600b75769e0b6aae17382bf4e.1596227003.git.me@ttaylorr.com
State New
Headers show
Series
  • upload-pack: custom allowed object filters
Related show

Commit Message

Taylor Blau July 31, 2020, 8:26 p.m. UTC
In b79cf959b2 (upload-pack.c: allow banning certain object filter(s),
2020-02-26), we introduced functionality to disallow certain object
filters from being chosen from within 'git upload-pack'. Traditionally,
administrators use this functionality to disallow filters that are known
to perform slowly, for e.g., those that do not have bitmap-level
filtering.

In the past, the '--filter=tree:<n>' was one such filter that does not
have bitmap-level filtering support, and so was likely to be banned by
administrators.

However, in the previous couple of commits, we introduced bitmap-level
filtering for the case when 'n' is equal to '0', i.e., as if we had a
'--filter=tree:none' choice.

While it would be sufficient to simply write

  $ git config uploadpackfilter.tree.allow true

(since it would allow all values of 'n'), we would like to be able to
allow this filter for certain values of 'n', i.e., those no greater than
some pre-specified maximum.

In order to do this, introduce a new configuration key, as follows:

  $ git config uploadpackfilter.tree.maxDepth <m>

where '<m>' specifies the maximum allowed value of 'n' in the filter
'tree:n'. Administrators who wish to allow for only the value '0' can
write:

  $ git config uploadpackfilter.tree.allow true
  $ git config uploadpackfilter.tree.maxDepth 0

which allows '--filter=tree:0', but no other values.

Unfortunately, since the tree depth is an unsigned long, we can't use,
say, -1 as a sentinel value, and so we must also keep track of "have we
set this" as well as "to what value".

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/uploadpack.txt |  6 ++++++
 t/t5616-partial-clone.sh            |  9 +++++++++
 upload-pack.c                       | 27 ++++++++++++++++++++++++++-
 3 files changed, 41 insertions(+), 1 deletion(-)

Comments

Jeff King July 31, 2020, 9:01 p.m. UTC | #1
On Fri, Jul 31, 2020 at 04:26:39PM -0400, Taylor Blau wrote:

> +test_expect_success 'upload-pack limits tree depth filters' '
> +	test_config -C srv.bare uploadpackfilter.allow false &&
> +	test_config -C srv.bare uploadpackfilter.tree.allow true &&
> +	test_config -C srv.bare uploadpackfilter.tree.maxDepth 0 &&
> +	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
> +		"file://$(pwd)/srv.bare" pc3 2>err &&
> +	test_i18ngrep "filter '\''tree'\'' not supported (maximum depth: 0, but got: 1)" err
> +'

Same i18ngrep comment as in the earlier patch (i.e., we can use grep
here).

> @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data)
>  
>  	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
>  		    list_object_filter_config_name(banned->choice));
> +	if (banned->choice == LOFC_TREE_DEPTH &&
> +	    data->tree_filter_max_depth != ULONG_MAX)
> +		strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"),
> +			    data->tree_filter_max_depth,
> +			    banned->tree_exclude_depth);

Hmm. So I see now why you wanted to go with the strbuf in the earlier
patch. This does still feel awkward, though. You check "is it allowed"
in an earlier function, we get "nope, it's not allowed", and now we have
to reimplement the check here. That seems like a maintenance burden.

I think a more natural flow would be either:

  - the "is it allowed" functions calls immediately into the function
    that sends the error and dies (this might need a conditional if
    there's a caller who doesn't want to die; I didn't check)

or

  - on failure it populates an error buffer itself, which the caller can
    then pass along as it sees fit

-Peff
Jeff King July 31, 2020, 9:22 p.m. UTC | #2
On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote:

> Hmm. So I see now why you wanted to go with the strbuf in the earlier
> patch. This does still feel awkward, though. You check "is it allowed"
> in an earlier function, we get "nope, it's not allowed", and now we have
> to reimplement the check here. That seems like a maintenance burden.
> 
> I think a more natural flow would be either:
> 
>   - the "is it allowed" functions calls immediately into the function
>     that sends the error and dies (this might need a conditional if
>     there's a caller who doesn't want to die; I didn't check)
> 
> or
> 
>   - on failure it populates an error buffer itself, which the caller can
>     then pass along as it sees fit

The first one is easy to do, because there's no other caller. Worth it?

-- >8 --
diff --git a/upload-pack.c b/upload-pack.c
index 131445b212..574a447d5c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -992,62 +992,63 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,
 	return 0;
 }
 
-static int allows_filter_choice(struct upload_pack_data *data,
-				struct list_objects_filter_options *opts)
+/* probably this helper could be used in lots more places */
+NORETURN __attribute__((format(printf,2,3)))
+static void send_err_and_die(struct upload_pack_data *data,
+			     const char *fmt, ...)
+{
+	struct strbuf buf = STRBUF_INIT;
+	va_list ap;
+
+	/* yuck, buf not necessary if we had va_list versions of our helpers */
+	va_start(ap, fmt);
+	strbuf_vaddf(&buf, fmt, ap);
+	va_end(ap);
+
+	packet_writer_error(&data->writer, "%s", buf.buf);
+	die("%s", buf.buf);
+}
+
+static void check_one_filter(struct upload_pack_data *data,
+			     struct list_objects_filter_options *opts)
 {
 	const char *key = list_object_filter_config_name(opts->choice);
 	struct string_list_item *item = string_list_lookup(&data->allowed_filters,
 							   key);
-	int allowed = -1;
+	int allowed;
 	if (item)
 		allowed = (intptr_t) item->util;
+	else
+		allowed = data->allow_filter_fallback;
 
-	if (allowed != 0 &&
-	    opts->choice == LOFC_TREE_DEPTH &&
-	    opts->tree_exclude_depth > data->tree_filter_max_depth)
-		return 0;
+	if (!allowed)
+		send_err_and_die(data, "filter '%s' not supported",
+				 list_object_filter_config_name(opts->choice));
 
-	if (allowed > -1)
-		return allowed;
-	return data->allow_filter_fallback;
+	if (opts->choice == LOFC_TREE_DEPTH &&
+	    opts->tree_exclude_depth > data->tree_filter_max_depth)
+		send_err_and_die(data,
+				 "tree filter allows max depth %lu, but got %lu",
+				 data->tree_filter_max_depth,
+				 opts->tree_exclude_depth);
 }
 
-static struct list_objects_filter_options *banned_filter(
-	struct upload_pack_data *data,
-	struct list_objects_filter_options *opts)
+static void check_filter_recurse(struct upload_pack_data *data,
+				 struct list_objects_filter_options *opts)
 {
 	size_t i;
 
-	if (!allows_filter_choice(data, opts))
-		return opts;
+	check_one_filter(data, opts);
 
 	if (opts->choice == LOFC_COMBINE)
 		for (i = 0; i < opts->sub_nr; i++) {
-			struct list_objects_filter_options *sub = &opts->sub[i];
-			if (banned_filter(data, sub))
-				return sub;
+			check_filter_recurse(data, &opts->sub[i]);
 		}
-	return NULL;
 }
 
 static void die_if_using_banned_filter(struct upload_pack_data *data)
 {
-	struct list_objects_filter_options *banned = banned_filter(data,
-								   &data->filter_options);
-	struct strbuf buf = STRBUF_INIT;
-	if (!banned)
-		return;
-
-	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
-		    list_object_filter_config_name(banned->choice));
-	if (banned->choice == LOFC_TREE_DEPTH &&
-	    data->tree_filter_max_depth != ULONG_MAX)
-		strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"),
-			    data->tree_filter_max_depth,
-			    banned->tree_exclude_depth);
-
-	packet_writer_error(&data->writer, "%s\n", buf.buf);
-	die("%s", buf.buf);
+	check_filter_recurse(data, &data->filter_options);
 }
 
 static void receive_needs(struct upload_pack_data *data,
Taylor Blau July 31, 2020, 9:29 p.m. UTC | #3
On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote:
> On Fri, Jul 31, 2020 at 04:26:39PM -0400, Taylor Blau wrote:
>
> > +test_expect_success 'upload-pack limits tree depth filters' '
> > +	test_config -C srv.bare uploadpackfilter.allow false &&
> > +	test_config -C srv.bare uploadpackfilter.tree.allow true &&
> > +	test_config -C srv.bare uploadpackfilter.tree.maxDepth 0 &&
> > +	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
> > +		"file://$(pwd)/srv.bare" pc3 2>err &&
> > +	test_i18ngrep "filter '\''tree'\'' not supported (maximum depth: 0, but got: 1)" err
> > +'
>
> Same i18ngrep comment as in the earlier patch (i.e., we can use grep
> here).
>
> > @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data)
> >
> >  	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
> >  		    list_object_filter_config_name(banned->choice));
> > +	if (banned->choice == LOFC_TREE_DEPTH &&
> > +	    data->tree_filter_max_depth != ULONG_MAX)
> > +		strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"),
> > +			    data->tree_filter_max_depth,
> > +			    banned->tree_exclude_depth);
>
> Hmm. So I see now why you wanted to go with the strbuf in the earlier
> patch. This does still feel awkward, though. You check "is it allowed"
> in an earlier function, we get "nope, it's not allowed", and now we have
> to reimplement the check here. That seems like a maintenance burden.

I'm not sure that I follow. Is the earlier function that you're
referring to 'banned_filter'? If so, the only use of that function is
within 'die_if_using_banned_filter'. 'banned_filter' exists only insofar
as to answer the question "return me the first banned filter, if one
exists, or NULL otherwise".

Then, dying here is as simple as (1) lookup the banned filter, and (2)
check if it's NULL or not.

If you're referring to 'allows_filter_choice', I guess I see what you're
getting it, but to be honest I'm not sure if I'm buying it. If we were
to combine 'allows_filter_choice', 'banned_filter', and
'die_if_using_banned_filter' into one function that traversed the filter
tree and 'die()'d as soon as it got to a banned one, that function would
have to know how to:

  1. Recurse through the tree when it hits a LOFC_COMBINE node.

  2. At each node, translate the filter->choice into the appropriate key
  name, look it up, and then figure out how to interpret its allowed
  status (including falling back to the default if unspecified).

  3. And, it would have to figure out how to format the message at each
  step.

(3) I think is made easier, since we know what message to format based
on whether or not we're in the 'opts->choice == LOFC_TREE_DEPTH' arm or
not. But, there are two more things that we now have to cram into that
same function.

Maybe I'm being too strict an adherent to having simpler functions, but
I'm failing to see how to combine these in a way that's cleaner than
what's written here.

> I think a more natural flow would be either:
>
>   - the "is it allowed" functions calls immediately into the function
>     that sends the error and dies (this might need a conditional if
>     there's a caller who doesn't want to die; I didn't check)
>
> or
>
>   - on failure it populates an error buffer itself, which the caller can
>     then pass along as it sees fit

I guess; I'm not a huge fan of the side-effecting nature, but maybe it's
cleaner.

I dunno. I appreciate your review, but I feel like we're in a bikeshed.

> -Peff

Thanks,
Taylor
Taylor Blau July 31, 2020, 9:30 p.m. UTC | #4
On Fri, Jul 31, 2020 at 05:22:43PM -0400, Jeff King wrote:
> On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote:
>
> > Hmm. So I see now why you wanted to go with the strbuf in the earlier
> > patch. This does still feel awkward, though. You check "is it allowed"
> > in an earlier function, we get "nope, it's not allowed", and now we have
> > to reimplement the check here. That seems like a maintenance burden.
> >
> > I think a more natural flow would be either:
> >
> >   - the "is it allowed" functions calls immediately into the function
> >     that sends the error and dies (this might need a conditional if
> >     there's a caller who doesn't want to die; I didn't check)
> >
> > or
> >
> >   - on failure it populates an error buffer itself, which the caller can
> >     then pass along as it sees fit
>
> The first one is easy to do, because there's no other caller. Worth it?
>
> -- >8 --
> diff --git a/upload-pack.c b/upload-pack.c
> index 131445b212..574a447d5c 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -992,62 +992,63 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,
>  	return 0;
>  }
>
> -static int allows_filter_choice(struct upload_pack_data *data,
> -				struct list_objects_filter_options *opts)
> +/* probably this helper could be used in lots more places */
> +NORETURN __attribute__((format(printf,2,3)))
> +static void send_err_and_die(struct upload_pack_data *data,
> +			     const char *fmt, ...)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	va_list ap;
> +
> +	/* yuck, buf not necessary if we had va_list versions of our helpers */
> +	va_start(ap, fmt);
> +	strbuf_vaddf(&buf, fmt, ap);
> +	va_end(ap);
> +
> +	packet_writer_error(&data->writer, "%s", buf.buf);
> +	die("%s", buf.buf);
> +}
> +
> +static void check_one_filter(struct upload_pack_data *data,
> +			     struct list_objects_filter_options *opts)
>  {
>  	const char *key = list_object_filter_config_name(opts->choice);
>  	struct string_list_item *item = string_list_lookup(&data->allowed_filters,
>  							   key);
> -	int allowed = -1;
> +	int allowed;
>  	if (item)
>  		allowed = (intptr_t) item->util;
> +	else
> +		allowed = data->allow_filter_fallback;
>
> -	if (allowed != 0 &&
> -	    opts->choice == LOFC_TREE_DEPTH &&
> -	    opts->tree_exclude_depth > data->tree_filter_max_depth)
> -		return 0;
> +	if (!allowed)
> +		send_err_and_die(data, "filter '%s' not supported",
> +				 list_object_filter_config_name(opts->choice));
>
> -	if (allowed > -1)
> -		return allowed;
> -	return data->allow_filter_fallback;
> +	if (opts->choice == LOFC_TREE_DEPTH &&
> +	    opts->tree_exclude_depth > data->tree_filter_max_depth)
> +		send_err_and_die(data,
> +				 "tree filter allows max depth %lu, but got %lu",
> +				 data->tree_filter_max_depth,
> +				 opts->tree_exclude_depth);
>  }
>
> -static struct list_objects_filter_options *banned_filter(
> -	struct upload_pack_data *data,
> -	struct list_objects_filter_options *opts)
> +static void check_filter_recurse(struct upload_pack_data *data,
> +				 struct list_objects_filter_options *opts)
>  {
>  	size_t i;
>
> -	if (!allows_filter_choice(data, opts))
> -		return opts;
> +	check_one_filter(data, opts);
>
>  	if (opts->choice == LOFC_COMBINE)
>  		for (i = 0; i < opts->sub_nr; i++) {
> -			struct list_objects_filter_options *sub = &opts->sub[i];
> -			if (banned_filter(data, sub))
> -				return sub;
> +			check_filter_recurse(data, &opts->sub[i]);
>  		}
> -	return NULL;
>  }
>
>  static void die_if_using_banned_filter(struct upload_pack_data *data)
>  {
> -	struct list_objects_filter_options *banned = banned_filter(data,
> -								   &data->filter_options);
> -	struct strbuf buf = STRBUF_INIT;
> -	if (!banned)
> -		return;
> -
> -	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
> -		    list_object_filter_config_name(banned->choice));
> -	if (banned->choice == LOFC_TREE_DEPTH &&
> -	    data->tree_filter_max_depth != ULONG_MAX)
> -		strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"),
> -			    data->tree_filter_max_depth,
> -			    banned->tree_exclude_depth);
> -
> -	packet_writer_error(&data->writer, "%s\n", buf.buf);
> -	die("%s", buf.buf);
> +	check_filter_recurse(data, &data->filter_options);
>  }
>
>  static void receive_needs(struct upload_pack_data *data,

I think that we crossed emails, but I'm still not convinced that this is
any cleaner than what I wrote. Yes, it's a maintenance problem if we add
more filter-specific logic like what we have in the LOFC_TREE_DEPTH
case, but I feel like we're bending over backwards in the meantime to
accommodate a problem that we don't have.

Thanks,
Taylor
Jeff King July 31, 2020, 9:36 p.m. UTC | #5
On Fri, Jul 31, 2020 at 05:29:05PM -0400, Taylor Blau wrote:

> > > @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data)
> > >
> > >  	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
> > >  		    list_object_filter_config_name(banned->choice));
> > > +	if (banned->choice == LOFC_TREE_DEPTH &&
> > > +	    data->tree_filter_max_depth != ULONG_MAX)
> > > +		strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"),
> > > +			    data->tree_filter_max_depth,
> > > +			    banned->tree_exclude_depth);
> >
> > Hmm. So I see now why you wanted to go with the strbuf in the earlier
> > patch. This does still feel awkward, though. You check "is it allowed"
> > in an earlier function, we get "nope, it's not allowed", and now we have
> > to reimplement the check here. That seems like a maintenance burden.
> 
> I'm not sure that I follow. Is the earlier function that you're
> referring to 'banned_filter'? If so, the only use of that function is
> within 'die_if_using_banned_filter'. 'banned_filter' exists only insofar
> as to answer the question "return me the first banned filter, if one
> exists, or NULL otherwise".
> 
> Then, dying here is as simple as (1) lookup the banned filter, and (2)
> check if it's NULL or not.
> 
> If you're referring to 'allows_filter_choice', I guess I see what you're
> getting it, but to be honest I'm not sure if I'm buying it.

Yeah, it's allows_filter_choice() that knows "if it's a tree we must
check the depth". And now die_if_using_banned_filter() needs to know
that, too. The policy is implemented twice.

I do appreciate that the way you've written it means that if somebody
forgets to update die_if_using_banned_filter() to match the logic in
allows_filter_choice(), we'd at least still die, just with a less good
error message. But it seems better still not to require the two to match
in the first place.

> If we were
> to combine 'allows_filter_choice', 'banned_filter', and
> 'die_if_using_banned_filter' into one function that traversed the filter
> tree and 'die()'d as soon as it got to a banned one, that function would
> have to know how to:
> 
>   1. Recurse through the tree when it hits a LOFC_COMBINE node.
> 
>   2. At each node, translate the filter->choice into the appropriate key
>   name, look it up, and then figure out how to interpret its allowed
>   status (including falling back to the default if unspecified).
> 
>   3. And, it would have to figure out how to format the message at each
>   step.
> 
> (3) I think is made easier, since we know what message to format based
> on whether or not we're in the 'opts->choice == LOFC_TREE_DEPTH' arm or
> not. But, there are two more things that we now have to cram into that
> same function.

You can still split those things into functions; see the patch I posted.

> Maybe I'm being too strict an adherent to having simpler functions, but
> I'm failing to see how to combine these in a way that's cleaner than
> what's written here.

To me this is less about "clean" and more about "don't ever duplicate
policy code". I don't mind duplicating boilerplate, but introducing a
place where somebody touching function X must remember to also touch Y
(and gets no compiler support to remind them) is a bad thing. I guess
you can call that "clean", but I'd take longer or more functions as a
tradeoff to avoid that.

My suggested patch does introduce more side effects. I think that's OK
because there really is only a single caller here. But if you wanted it
cleaner, then I think having allows_filter_choice() fill out an error
strbuf would eliminate my concern without drastically altering the flow
of your code.

-Peff
Jeff King July 31, 2020, 9:43 p.m. UTC | #6
On Fri, Jul 31, 2020 at 05:36:04PM -0400, Jeff King wrote:

> My suggested patch does introduce more side effects. I think that's OK
> because there really is only a single caller here. But if you wanted it
> cleaner, then I think having allows_filter_choice() fill out an error
> strbuf would eliminate my concern without drastically altering the flow
> of your code.

That could be something like the patch below. banned_filter() could
switch to returning a simple boolean, but I didn't do that here.

I did (as with my other patch) reorder the logic in allows_filter_choice(),
as I think it's much easier to follow by resolving "allowed" to the
fallback earlier. That lets you handle "not allowed at all" first and
early-return (or die) before dealing with type-specific checks.

diff --git a/upload-pack.c b/upload-pack.c
index 131445b212..86786f0e13 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -993,59 +993,60 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,
 }
 
 static int allows_filter_choice(struct upload_pack_data *data,
-				struct list_objects_filter_options *opts)
+				struct list_objects_filter_options *opts,
+				struct strbuf *err)
 {
 	const char *key = list_object_filter_config_name(opts->choice);
 	struct string_list_item *item = string_list_lookup(&data->allowed_filters,
 							   key);
-	int allowed = -1;
+	int allowed;
 	if (item)
 		allowed = (intptr_t) item->util;
+	else
+		allowed = data->allow_filter_fallback;
 
-	if (allowed != 0 &&
-	    opts->choice == LOFC_TREE_DEPTH &&
-	    opts->tree_exclude_depth > data->tree_filter_max_depth)
+	if (!allowed) {
+		strbuf_addf(err, "filter '%s' not supported", key);
 		return 0;
+	}
+
+	if (opts->choice == LOFC_TREE_DEPTH &&
+	    opts->tree_exclude_depth > data->tree_filter_max_depth) {
+		strbuf_addf(err, "tree filter maximum depth is %lu, but got %lu",
+			    data->tree_filter_max_depth, opts->tree_exclude_depth);
+		return 0;
+	}
 
-	if (allowed > -1)
-		return allowed;
-	return data->allow_filter_fallback;
+	return 1;
 }
 
 static struct list_objects_filter_options *banned_filter(
 	struct upload_pack_data *data,
-	struct list_objects_filter_options *opts)
+	struct list_objects_filter_options *opts,
+	struct strbuf *err)
 {
 	size_t i;
 
-	if (!allows_filter_choice(data, opts))
+	if (!allows_filter_choice(data, opts, err))
 		return opts;
 
 	if (opts->choice == LOFC_COMBINE)
 		for (i = 0; i < opts->sub_nr; i++) {
 			struct list_objects_filter_options *sub = &opts->sub[i];
-			if (banned_filter(data, sub))
+			if (banned_filter(data, sub, err))
 				return sub;
 		}
 	return NULL;
 }
 
 static void die_if_using_banned_filter(struct upload_pack_data *data)
 {
-	struct list_objects_filter_options *banned = banned_filter(data,
-								   &data->filter_options);
 	struct strbuf buf = STRBUF_INIT;
+	struct list_objects_filter_options *banned =
+		banned_filter(data, &data->filter_options, &buf);
 	if (!banned)
 		return;
 
-	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
-		    list_object_filter_config_name(banned->choice));
-	if (banned->choice == LOFC_TREE_DEPTH &&
-	    data->tree_filter_max_depth != ULONG_MAX)
-		strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"),
-			    data->tree_filter_max_depth,
-			    banned->tree_exclude_depth);
-
 	packet_writer_error(&data->writer, "%s\n", buf.buf);
 	die("%s", buf.buf);
 }

Patch
diff mbox series

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index fffe8ac648..ee7b3ac94f 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -69,6 +69,12 @@  uploadpackfilter.<filter>.allow::
 	combined filters, both `combine` and all of the nested filter
 	kinds must be allowed. Defaults to `uploadpackfilter.allow`.
 
+uploadpackfilter.tree.maxDepth::
+	Only allow `--filter=tree=<n>` when `n` is no more than the value of
+	`uploadpackfilter.tree.maxDepth`. If set, this also implies
+	`uploadpackfilter.tree.allow=true`, unless this configuration
+	variable had already been set. Has no effect if unset.
+
 uploadpack.allowRefInWant::
 	If this option is set, `upload-pack` will support the `ref-in-want`
 	feature of the protocol version 2 `fetch` command.  This feature
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 0d46b5a2f8..35cb6a34a3 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -259,6 +259,15 @@  test_expect_success 'upload-pack fails banned object filters with fallback' '
 	test_i18ngrep "filter '\''blob:none'\'' not supported" err
 '
 
+test_expect_success 'upload-pack limits tree depth filters' '
+	test_config -C srv.bare uploadpackfilter.allow false &&
+	test_config -C srv.bare uploadpackfilter.tree.allow true &&
+	test_config -C srv.bare uploadpackfilter.tree.maxDepth 0 &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
+		"file://$(pwd)/srv.bare" pc3 2>err &&
+	test_i18ngrep "filter '\''tree'\'' not supported (maximum depth: 0, but got: 1)" err
+'
+
 test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
 	rm -rf src dst &&
 	git init src &&
diff --git a/upload-pack.c b/upload-pack.c
index 5fa22da31f..131445b212 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -105,6 +105,7 @@  struct upload_pack_data {
 	unsigned use_include_tag : 1;
 	unsigned allow_filter : 1;
 	unsigned allow_filter_fallback : 1;
+	unsigned long tree_filter_max_depth;
 
 	unsigned done : 1;					/* v2 only */
 	unsigned allow_ref_in_want : 1;				/* v2 only */
@@ -136,6 +137,7 @@  static void upload_pack_data_init(struct upload_pack_data *data)
 	data->extra_edge_obj = extra_edge_obj;
 	data->allowed_filters = allowed_filters;
 	data->allow_filter_fallback = 1;
+	data->tree_filter_max_depth = ULONG_MAX;
 	packet_writer_init(&data->writer, 1);
 
 	data->keepalive = 5;
@@ -996,8 +998,17 @@  static int allows_filter_choice(struct upload_pack_data *data,
 	const char *key = list_object_filter_config_name(opts->choice);
 	struct string_list_item *item = string_list_lookup(&data->allowed_filters,
 							   key);
+	int allowed = -1;
 	if (item)
-		return (intptr_t) item->util;
+		allowed = (intptr_t) item->util;
+
+	if (allowed != 0 &&
+	    opts->choice == LOFC_TREE_DEPTH &&
+	    opts->tree_exclude_depth > data->tree_filter_max_depth)
+		return 0;
+
+	if (allowed > -1)
+		return allowed;
 	return data->allow_filter_fallback;
 }
 
@@ -1029,6 +1040,11 @@  static void die_if_using_banned_filter(struct upload_pack_data *data)
 
 	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
 		    list_object_filter_config_name(banned->choice));
+	if (banned->choice == LOFC_TREE_DEPTH &&
+	    data->tree_filter_max_depth != ULONG_MAX)
+		strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"),
+			    data->tree_filter_max_depth,
+			    banned->tree_exclude_depth);
 
 	packet_writer_error(&data->writer, "%s\n", buf.buf);
 	die("%s", buf.buf);
@@ -1243,6 +1259,15 @@  static int parse_object_filter_config(const char *var, const char *value,
 	if (!strcmp(key, "allow"))
 		string_list_insert(&data->allowed_filters, buf.buf)->util =
 			(void *)(intptr_t)git_config_bool(var, value);
+	else if (!strcmp(buf.buf, "tree") && !strcmp(key, "maxdepth")) {
+		if (!value) {
+			strbuf_release(&buf);
+			return config_error_nonbool(var);
+		}
+		string_list_insert(&data->allowed_filters, buf.buf)->util =
+			(void *)(intptr_t)1;
+		data->tree_filter_max_depth = git_config_ulong(var, value);
+	}
 
 	strbuf_release(&buf);
 	return 0;