[4/4] upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'
diff mbox series

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

Commit Message

Taylor Blau July 2, 2020, 8:06 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 uploadpack.filter.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 uploadpack.filter.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 uploadpack.filter.tree.allow true
  $ git config uploadpack.filter.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            |  8 ++++++++
 upload-pack.c                       | 32 ++++++++++++++++++++++++++---
 3 files changed, 43 insertions(+), 3 deletions(-)

Comments

SZEDER Gábor July 15, 2020, 10:11 a.m. UTC | #1
On Thu, Jul 02, 2020 at 04:06:40PM -0400, Taylor Blau wrote:
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 5dcd0b5656..8781a24cfe 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -261,6 +261,14 @@ 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 uploadpack.filter.allow false &&
> +	test_config -C srv.bare uploadpack.filter.tree.allow true &&
> +	test_config -C srv.bare uploadpack.filter.tree.maxDepth 0 &&
> +	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
> +		"file://$(pwd)/srv.bare" pc3
> +'

Unlike in the other three tests added earlier in this series, here you
do use 'test_must_fail ok=sigpipe', but you don't check that the
command died with the right error message.  Saving stderr and adding

  test_i18ngrep "filter '\''tree'\'' not supported (maximum depth: 0, but got: 1)" err

makes this test flaky, too, like the other three:

  expecting success of 5616.20 'upload-pack limits tree depth filters': 
          test_config -C srv.bare uploadpack.filter.allow false &&
          test_config -C srv.bare uploadpack.filter.tree.allow true &&
          test_config -C srv.bare uploadpack.filter.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_config -C srv.bare uploadpack.filter.allow false
  + test_config -C srv.bare uploadpack.filter.tree.allow true
  + test_config -C srv.bare uploadpack.filter.tree.maxDepth 0
  + pwd
  + test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 file:///home/szeder/src/git/t/trash directory.t5616-partial-clone.stress-4/srv.bare pc3
  + test_i18ngrep filter 'tree' not supported (maximum depth: 0, but got: 1) err
  error: 'grep filter 'tree' not supported (maximum depth: 0, but got: 1) err' didn't find a match in:
  Cloning into 'pc3'...
  fatal: git upload-pack: banned object filter requested
  error: last command exited with $?=1
  not ok 20 - upload-pack limits tree depth filters

Patch
diff mbox series

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index fd4970306c..3671b62e4c 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -73,6 +73,12 @@  Note that the dot between 'filter' and '<filter>' is both non-standard
 and intentional. This is done to avoid a parsing ambiguity when
 specifying this configuration as an argument to Git's top-level `-c`.
 
+uploadpack.filter.tree.maxDepth::
+	Only allow `--filter=tree=<n>` when `n` is no more than the value of
+	`uploadpack.filter.tree.maxDepth`. If set, this also implies
+	`uploadpack.filter.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 5dcd0b5656..8781a24cfe 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -261,6 +261,14 @@  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 uploadpack.filter.allow false &&
+	test_config -C srv.bare uploadpack.filter.tree.allow true &&
+	test_config -C srv.bare uploadpack.filter.tree.maxDepth 0 &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
+		"file://$(pwd)/srv.bare" pc3
+'
+
 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 a014ae23a9..8db1745b86 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;
@@ -995,8 +997,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;
 }
 
@@ -1022,11 +1033,22 @@  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;
 
-	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
-			    list_object_filter_config_name(banned->choice));
+	strbuf_addf(&buf, _("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);
+
+	strbuf_release(&buf);
+
 	die(_("git upload-pack: banned object filter requested"));
 }
 
@@ -1239,6 +1261,10 @@  static void parse_object_filter_config(const char *var, const char *value,
 				(void *)(intptr_t)git_config_bool(var, value);
 		else
 			data->allow_filter_fallback = git_config_bool(var, value);
+	} else if (!strcmp(spec.buf, "tree") && !strcmp(key, "maxdepth")) {
+		string_list_insert(&data->allowed_filters, "tree")->util
+			= (void *) (intptr_t) 1;
+		data->tree_filter_max_depth = git_config_ulong(var, value);
 	}
 
 	strbuf_release(&spec);