[PATCH v2 0/4] upload-pack: custom allowed object filters

[PATCH v2 0/4] upload-pack: custom allowed object filters

[Taylor Blau]

Here's a much-delayed v2 of my series to teach upload-pack to limit the
kinds of object filters that it is willing to server in a request.

Much is the same since last time, with two notable exceptions:

  - We use the 'uploadpackfilter' top-level configuration key instead of
    pretending that 'uploadpack.filter' is top-level, which greatly
    simplifies the call to 'parse_config_key()'.

  - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,
    which propagates the error through 'git clone' always, and resolves
    a flaky set of tests that used to result in a SIGPIPE.

Thanks for your review in the meantime, and for encouraging me to send
this reroll now instead of later. Maybe I'll start unloading the rest of
my backlog tomorrow ;-).

Taylor Blau (4):
  list_objects_filter_options: introduce
    'list_object_filter_config_name'
  upload-pack.c: allow banning certain object filter(s)
  upload-pack.c: pass 'struct list_objects_filter_options *'
  upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

 Documentation/config/uploadpack.txt |  18 +++++
 list-objects-filter-options.c       |  23 +++++++
 list-objects-filter-options.h       |   6 ++
 t/t5616-partial-clone.sh            |  32 +++++++++
 upload-pack.c                       | 103 ++++++++++++++++++++++++++++
 5 files changed, 182 insertions(+)

Range-diff against v1:
  2:  f0982d24e7 ! 134:  9fee52cb6d upload-pack.c: allow banning certain object filter(s)
    @@ Commit message
         Allow configuring 'git upload-pack' to support object filters on a
         case-by-case basis by introducing two new configuration variables:

    -      - 'uploadpack.filter.allow'
    -      - 'uploadpack.filter.<kind>.allow'
    +      - 'uploadpackfilter.allow'
    +      - 'uploadpackfilter.<kind>.allow'

         where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on.

    @@ Commit message

         If a client requests the object filter <kind> and the respective
         configuration value is not set, 'git upload-pack' will default to the
    -    value of 'uploadpack.filter.allow', which itself defaults to 'true' to
    +    value of 'uploadpackfilter.allow', which itself defaults to 'true' to
         maintain backwards compatibility. Note that this differs from
         'uploadpack.allowfilter', which controls whether or not the 'filter'
         capability is advertised.
    @@ Documentation/config/uploadpack.txt: uploadpack.allowFilter::
      	If this option is set, `upload-pack` will support partial
      	clone and partial fetch object filtering.

    -+uploadpack.filter.allow::
    ++uploadpackfilter.allow::
     +	Provides a default value for unspecified object filters (see: the
     +	below configuration variable).
     +	Defaults to `true`.
     +
    -+uploadpack.filter.<filter>.allow::
    ++uploadpackfilter.<filter>.allow::
     +	Explicitly allow or ban the object filter corresponding to
     +	`<filter>`, where `<filter>` may be one of: `blob:none`,
     +	`blob:limit`, `tree`, `sparse:oid`, or `combine`. If using
     +	combined filters, both `combine` and all of the nested filter
    -+	kinds must be allowed.  Defaults to `uploadpack.filter.allow`.
    -++
    -+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`.
    ++	kinds must be allowed. Defaults to `uploadpackfilter.allow`.
     +
      uploadpack.allowRefInWant::
      	If this option is set, `upload-pack` will support the `ref-in-want`
    @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil
      '

     +test_expect_success 'upload-pack fails banned object filters' '
    -+	# Test case-insensitivity by intentional use of "blob:None" rather than
    -+	# "blob:none".
    -+	test_config -C srv.bare uploadpack.filter.blob:None.allow false &&
    ++	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
     +	test_must_fail git clone --no-checkout --filter=blob:none \
     +		"file://$(pwd)/srv.bare" pc3 2>err &&
     +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
     +'
     +
     +test_expect_success 'upload-pack fails banned combine object filters' '
    -+	test_config -C srv.bare uploadpack.filter.allow false &&
    -+	test_config -C srv.bare uploadpack.filter.combine.allow true &&
    -+	test_config -C srv.bare uploadpack.filter.tree.allow true &&
    -+	test_config -C srv.bare uploadpack.filter.blob:none.allow false &&
    ++	test_config -C srv.bare uploadpackfilter.allow false &&
    ++	test_config -C srv.bare uploadpackfilter.combine.allow true &&
    ++	test_config -C srv.bare uploadpackfilter.tree.allow true &&
    ++	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
     +	test_must_fail git clone --no-checkout --filter=tree:1 \
     +		--filter=blob:none "file://$(pwd)/srv.bare" pc3 2>err &&
     +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
     +'
     +
     +test_expect_success 'upload-pack fails banned object filters with fallback' '
    -+	test_config -C srv.bare uploadpack.filter.allow false &&
    ++	test_config -C srv.bare uploadpackfilter.allow false &&
     +	test_must_fail git clone --no-checkout --filter=blob:none \
     +		"file://$(pwd)/srv.bare" pc3 2>err &&
     +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
    @@ upload-pack.c: static int process_deepen_not(const char *line, struct string_lis
     +	if (!banned)
     +		return;
     +
    -+	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
    -+			    list_object_filter_config_name(banned->choice));
    -+	die(_("git upload-pack: banned object filter requested"));
    ++	die(_("git upload-pack: filter '%s' not supported"),
    ++	    list_object_filter_config_name(banned->choice));
     +}
     +
      static void receive_needs(struct upload_pack_data *data,
    @@ upload-pack.c: static int find_symref(const char *refname, const struct object_i
      	return 0;
      }

    -+static void parse_object_filter_config(const char *var, const char *value,
    ++static int parse_object_filter_config(const char *var, const char *value,
     +				       struct upload_pack_data *data)
     +{
    -+	struct strbuf spec = STRBUF_INIT;
    ++	struct strbuf buf = STRBUF_INIT;
     +	const char *sub, *key;
     +	size_t sub_len;
     +
    -+	if (parse_config_key(var, "uploadpack", &sub, &sub_len, &key))
    -+		return;
    -+	if (!sub || !skip_prefix(sub, "filter.", &sub))
    -+		return;
    ++	if (parse_config_key(var, "uploadpackfilter", &sub, &sub_len, &key))
    ++		return 0;
     +
    -+	if (sub != key)
    -+		strbuf_add(&spec, sub, key - sub - 1);
    -+	strbuf_tolower(&spec);
    -+
    -+	if (!strcmp(key, "allow")) {
    -+		if (spec.len)
    -+			string_list_insert(&data->allowed_filters, spec.buf)->util =
    -+				(void *)(intptr_t)git_config_bool(var, value);
    -+		else
    ++	if (!sub) {
    ++		if (!strcmp(key, "allow"))
     +			data->allow_filter_fallback = git_config_bool(var, value);
    ++		return 0;
     +	}
     +
    -+	strbuf_release(&spec);
    ++	strbuf_add(&buf, sub, sub_len);
    ++
    ++	if (!strcmp(key, "allow"))
    ++		string_list_insert(&data->allowed_filters, buf.buf)->util =
    ++			(void *)(intptr_t)git_config_bool(var, value);
    ++
    ++	strbuf_release(&buf);
    ++	return 0;
     +}
     +
      static int upload_pack_config(const char *var, const char *value, void *cb_data)
  3:  3434bd5979 = 135:  550e4e13f1 upload-pack.c: pass 'struct list_objects_filter_options *'
  4:  9fa765a71d ! 136:  bb008f7427 upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'
    @@ Metadata
     Author: Taylor Blau <me@ttaylorr.com>

      ## Commit message ##
    -    upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'
    +    upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

         In b79cf959b2 (upload-pack.c: allow banning certain object filter(s),
         2020-02-26), we introduced functionality to disallow certain object
    @@ Commit message

         While it would be sufficient to simply write

    -      $ git config uploadpack.filter.tree.allow true
    +      $ 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
    @@ Commit message

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

    -      $ git config uploadpack.filter.tree.maxDepth <m>
    +      $ 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 uploadpack.filter.tree.allow true
    -      $ git config uploadpack.filter.tree.maxDepth 0
    +      $ git config uploadpackfilter.tree.allow true
    +      $ git config uploadpackfilter.tree.maxDepth 0

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

    @@ Commit message
         Signed-off-by: Taylor Blau <me@ttaylorr.com>

      ## Documentation/config/uploadpack.txt ##
    -@@ Documentation/config/uploadpack.txt: 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`.
    +@@ Documentation/config/uploadpack.txt: uploadpackfilter.<filter>.allow::
    + 	combined filters, both `combine` and all of the nested filter
    + 	kinds must be allowed. Defaults to `uploadpackfilter.allow`.

    -+uploadpack.filter.tree.maxDepth::
    ++uploadpackfilter.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
    ++	`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::
    @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object f
      '

     +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_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
     +'
    @@ upload-pack.c: static void die_if_using_banned_filter(struct upload_pack_data *d
      	if (!banned)
      		return;

    --	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
    --			    list_object_filter_config_name(banned->choice));
    +-	die(_("git upload-pack: filter '%s' not supported"),
    +-	    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 &&
    @@ upload-pack.c: static void die_if_using_banned_filter(struct upload_pack_data *d
     +		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"));
    ++	die("%s", buf.buf);
      }

    -@@ upload-pack.c: 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;
    + static void receive_needs(struct upload_pack_data *data,
    +@@ upload-pack.c: 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(&spec);
    + 	strbuf_release(&buf);
    + 	return 0;
--
2.28.0.rc1.13.ge78abce653

[PATCH v2 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name'

[Taylor Blau]

In a subsequent commit, we will add configuration options that are
specific to each kind of object filter, in which case it is handy to
have a function that translates between 'enum
list_objects_filter_choice' and an appropriate configuration-friendly
string.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 list-objects-filter-options.c | 23 +++++++++++++++++++++++
 list-objects-filter-options.h |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 3553ad7b0a..92b408c0c8 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -15,6 +15,29 @@ static int parse_combine_filter(
 	const char *arg,
 	struct strbuf *errbuf);
 
+const char *list_object_filter_config_name(enum list_objects_filter_choice c)
+{
+	switch (c) {
+	case LOFC_DISABLED:
+		/* we have no name for "no filter at all" */
+		break;
+	case LOFC_BLOB_NONE:
+		return "blob:none";
+	case LOFC_BLOB_LIMIT:
+		return "blob:limit";
+	case LOFC_TREE_DEPTH:
+		return "tree";
+	case LOFC_SPARSE_OID:
+		return "sparse:oid";
+	case LOFC_COMBINE:
+		return "combine";
+	case LOFC__COUNT:
+		/* not a real filter type; just the count of all filters */
+		break;
+	}
+	BUG("list_object_filter_choice_name: invalid argument '%d'", c);
+}
+
 /*
  * Parse value of the argument to the "filter" keyword.
  * On the command line this looks like:
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index 73fffa4ad7..01767c3c96 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -17,6 +17,12 @@ enum list_objects_filter_choice {
 	LOFC__COUNT /* must be last */
 };
 
+/*
+ * Returns a configuration key suitable for describing the given object filter,
+ * e.g.: "blob:none", "combine", etc.
+ */
+const char *list_object_filter_config_name(enum list_objects_filter_choice c);
+
 struct list_objects_filter_options {
 	/*
 	 * 'filter_spec' is the raw argument value given on the command line
-- 
2.28.0.rc1.13.ge78abce653

[PATCH v2 2/4] upload-pack.c: allow banning certain object filter(s)

[Taylor Blau]

Git clients may ask the server for a partial set of objects, where the
set of objects being requested is refined by one or more object filters.
Server administrators can configure 'git upload-pack' to allow or ban
these filters by setting the 'uploadpack.allowFilter' variable to
'true' or 'false', respectively.

However, administrators using bitmaps may wish to allow certain kinds of
object filters, but ban others. Specifically, they may wish to allow
object filters that can be optimized by the use of bitmaps, while
rejecting other object filters which aren't and represent a perceived
performance degradation (as well as an increased load factor on the
server).

Allow configuring 'git upload-pack' to support object filters on a
case-by-case basis by introducing two new configuration variables:

  - 'uploadpackfilter.allow'
  - 'uploadpackfilter.<kind>.allow'

where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on.

Setting the second configuration variable for any valid value of
'<kind>' explicitly allows or disallows restricting that kind of object
filter.

If a client requests the object filter <kind> and the respective
configuration value is not set, 'git upload-pack' will default to the
value of 'uploadpackfilter.allow', which itself defaults to 'true' to
maintain backwards compatibility. Note that this differs from
'uploadpack.allowfilter', which controls whether or not the 'filter'
capability is advertised.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/uploadpack.txt | 12 +++++
 t/t5616-partial-clone.sh            | 24 +++++++++
 upload-pack.c                       | 76 +++++++++++++++++++++++++++++
 3 files changed, 112 insertions(+)

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index ed1c835695..fffe8ac648 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -57,6 +57,18 @@ uploadpack.allowFilter::
 	If this option is set, `upload-pack` will support partial
 	clone and partial fetch object filtering.
 
+uploadpackfilter.allow::
+	Provides a default value for unspecified object filters (see: the
+	below configuration variable).
+	Defaults to `true`.
+
+uploadpackfilter.<filter>.allow::
+	Explicitly allow or ban the object filter corresponding to
+	`<filter>`, where `<filter>` may be one of: `blob:none`,
+	`blob:limit`, `tree`, `sparse:oid`, or `combine`. If using
+	combined filters, both `combine` and all of the nested filter
+	kinds must be allowed. Defaults to `uploadpackfilter.allow`.
+
 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 8a27452a51..b196ee694c 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -235,6 +235,30 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' '
 	test_cmp unique_types.expected unique_types.actual
 '
 
+test_expect_success 'upload-pack fails banned object filters' '
+	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
+	test_must_fail git clone --no-checkout --filter=blob:none \
+		"file://$(pwd)/srv.bare" pc3 2>err &&
+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
+'
+
+test_expect_success 'upload-pack fails banned combine object filters' '
+	test_config -C srv.bare uploadpackfilter.allow false &&
+	test_config -C srv.bare uploadpackfilter.combine.allow true &&
+	test_config -C srv.bare uploadpackfilter.tree.allow true &&
+	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
+	test_must_fail git clone --no-checkout --filter=tree:1 \
+		--filter=blob:none "file://$(pwd)/srv.bare" pc3 2>err &&
+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
+'
+
+test_expect_success 'upload-pack fails banned object filters with fallback' '
+	test_config -C srv.bare uploadpackfilter.allow false &&
+	test_must_fail git clone --no-checkout --filter=blob:none \
+		"file://$(pwd)/srv.bare" pc3 2>err &&
+	test_i18ngrep "filter '\''blob:none'\'' not supported" 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 951a2b23aa..61929977ab 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -88,6 +88,7 @@ struct upload_pack_data {
 	enum allow_uor allow_uor;
 
 	struct list_objects_filter_options filter_options;
+	struct string_list allowed_filters;
 
 	struct packet_writer writer;
 
@@ -103,6 +104,7 @@ struct upload_pack_data {
 	unsigned no_progress : 1;
 	unsigned use_include_tag : 1;
 	unsigned allow_filter : 1;
+	unsigned allow_filter_fallback : 1;
 
 	unsigned done : 1;					/* v2 only */
 	unsigned allow_ref_in_want : 1;				/* v2 only */
@@ -120,6 +122,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
 	struct string_list uri_protocols = STRING_LIST_INIT_DUP;
 	struct object_array extra_edge_obj = OBJECT_ARRAY_INIT;
+	struct string_list allowed_filters = STRING_LIST_INIT_DUP;
 
 	memset(data, 0, sizeof(*data));
 	data->symref = symref;
@@ -131,6 +134,8 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->deepen_not = deepen_not;
 	data->uri_protocols = uri_protocols;
 	data->extra_edge_obj = extra_edge_obj;
+	data->allowed_filters = allowed_filters;
+	data->allow_filter_fallback = 1;
 	packet_writer_init(&data->writer, 1);
 
 	data->keepalive = 5;
@@ -147,6 +152,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	string_list_clear(&data->deepen_not, 0);
 	object_array_clear(&data->extra_edge_obj);
 	list_objects_filter_release(&data->filter_options);
+	string_list_clear(&data->allowed_filters, 1);
 
 	free((char *)data->pack_objects_hook);
 }
@@ -983,6 +989,46 @@ 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,
+				enum list_objects_filter_choice c)
+{
+	const char *key = list_object_filter_config_name(c);
+	struct string_list_item *item = string_list_lookup(&data->allowed_filters,
+							   key);
+	if (item)
+		return (intptr_t) item->util;
+	return data->allow_filter_fallback;
+}
+
+static struct list_objects_filter_options *banned_filter(
+	struct upload_pack_data *data,
+	struct list_objects_filter_options *opts)
+{
+	size_t i;
+
+	if (!allows_filter_choice(data, opts->choice))
+		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))
+				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);
+	if (!banned)
+		return;
+
+	die(_("git upload-pack: filter '%s' not supported"),
+	    list_object_filter_config_name(banned->choice));
+}
+
 static void receive_needs(struct upload_pack_data *data,
 			  struct packet_reader *reader)
 {
@@ -1013,6 +1059,7 @@ static void receive_needs(struct upload_pack_data *data,
 				die("git upload-pack: filtering capability not negotiated");
 			list_objects_filter_die_if_populated(&data->filter_options);
 			parse_list_objects_filter(&data->filter_options, arg);
+			die_if_using_banned_filter(data);
 			continue;
 		}
 
@@ -1170,6 +1217,32 @@ static int find_symref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
+static int parse_object_filter_config(const char *var, const char *value,
+				       struct upload_pack_data *data)
+{
+	struct strbuf buf = STRBUF_INIT;
+	const char *sub, *key;
+	size_t sub_len;
+
+	if (parse_config_key(var, "uploadpackfilter", &sub, &sub_len, &key))
+		return 0;
+
+	if (!sub) {
+		if (!strcmp(key, "allow"))
+			data->allow_filter_fallback = git_config_bool(var, value);
+		return 0;
+	}
+
+	strbuf_add(&buf, sub, sub_len);
+
+	if (!strcmp(key, "allow"))
+		string_list_insert(&data->allowed_filters, buf.buf)->util =
+			(void *)(intptr_t)git_config_bool(var, value);
+
+	strbuf_release(&buf);
+	return 0;
+}
+
 static int upload_pack_config(const char *var, const char *value, void *cb_data)
 {
 	struct upload_pack_data *data = cb_data;
@@ -1209,6 +1282,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
 			return git_config_string(&data->pack_objects_hook, var, value);
 	}
 
+	parse_object_filter_config(var, value, data);
+
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
@@ -1389,6 +1464,7 @@ static void process_args(struct packet_reader *request,
 		if (data->allow_filter && skip_prefix(arg, "filter ", &p)) {
 			list_objects_filter_die_if_populated(&data->filter_options);
 			parse_list_objects_filter(&data->filter_options, p);
+			die_if_using_banned_filter(data);
 			continue;
 		}
 
-- 
2.28.0.rc1.13.ge78abce653

[PATCH v2 3/4] upload-pack.c: pass 'struct list_objects_filter_options *'

[Taylor Blau]

The 'allows_filter_choice' function used to take an 'enum
list_objects_filter_choice', but in a future commit it will be more
convenient for it to accept the whole 'struct
list_objects_filter_options', for e.g., to inspect the value of
'->tree_exclude_depth'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 upload-pack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 61929977ab..48f10d21f6 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -990,9 +990,9 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,
 }
 
 static int allows_filter_choice(struct upload_pack_data *data,
-				enum list_objects_filter_choice c)
+				struct list_objects_filter_options *opts)
 {
-	const char *key = list_object_filter_config_name(c);
+	const char *key = list_object_filter_config_name(opts->choice);
 	struct string_list_item *item = string_list_lookup(&data->allowed_filters,
 							   key);
 	if (item)
@@ -1006,7 +1006,7 @@ static struct list_objects_filter_options *banned_filter(
 {
 	size_t i;
 
-	if (!allows_filter_choice(data, opts->choice))
+	if (!allows_filter_choice(data, opts))
 		return opts;
 
 	if (opts->choice == LOFC_COMBINE)
-- 
2.28.0.rc1.13.ge78abce653

[PATCH v2 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

[Taylor Blau]

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            |  8 +++++++
 upload-pack.c                       | 33 ++++++++++++++++++++++++++---
 3 files changed, 44 insertions(+), 3 deletions(-)

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 b196ee694c..4292a644d7 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -259,6 +259,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 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
+'
+
 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 48f10d21f6..47cdaae265 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,18 @@ 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;
 
-	die(_("git upload-pack: filter '%s' not supported"),
-	    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);
+	die("%s", buf.buf);
 }
 
 static void receive_needs(struct upload_pack_data *data,
@@ -1238,6 +1256,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;
-- 
2.28.0.rc1.13.ge78abce653

Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

[SZEDER Gábor]

On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:
> Here's a much-delayed v2 of my series to teach upload-pack to limit the
> kinds of object filters that it is willing to server in a request.
> 
> Much is the same since last time, with two notable exceptions:
> 
>   - We use the 'uploadpackfilter' top-level configuration key instead of
>     pretending that 'uploadpack.filter' is top-level, which greatly
>     simplifies the call to 'parse_config_key()'.
> 
>   - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,

To clarify, I only recommended to pass the same message to die() as in
the ERR packet, not dropping the ERR packet, because ...

>     which propagates the error through 'git clone' always,

it does in the new tests when creating a local clone, but does it
really work with all protocols and remote helpers and what not?

>     and resolves
>     a flaky set of tests that used to result in a SIGPIPE.

This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'
can still abort while 'git clone' is still sending packets.  IOW we
still need that 'test_must_fail ok=sigpipe' in all new tests.

Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

[Taylor Blau]

On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:
> On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:
> > Here's a much-delayed v2 of my series to teach upload-pack to limit the
> > kinds of object filters that it is willing to server in a request.
> >
> > Much is the same since last time, with two notable exceptions:
> >
> >   - We use the 'uploadpackfilter' top-level configuration key instead of
> >     pretending that 'uploadpack.filter' is top-level, which greatly
> >     simplifies the call to 'parse_config_key()'.
> >
> >   - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,
>
> To clarify, I only recommended to pass the same message to die() as in
> the ERR packet, not dropping the ERR packet, because ...
>
> >     which propagates the error through 'git clone' always,
>
> it does in the new tests when creating a local clone, but does it
> really work with all protocols and remote helpers and what not?
>
> >     and resolves
> >     a flaky set of tests that used to result in a SIGPIPE.
>
> This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'
> can still abort while 'git clone' is still sending packets.  IOW we
> still need that 'test_must_fail ok=sigpipe' in all new tests.

Let me double check my understanding... I think that you are suggesting
the following three things:

  - Write the same message as an err packet over the wire as we do when
    'die()'ing from inside of upload-pack.c

  - Don't mark said message(s) for translation, matching what we do in
    the rest of upload-pack.c.

  - Re-introduce the 'test_must_fail ok=sigpipe' and stop grepping
    stderr for the right message.

Do I have that right?

Thanks,
Taylor

Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

[Jeff King]

On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:

> Let me double check my understanding... I think that you are suggesting
> the following three things:
> 
>   - Write the same message as an err packet over the wire as we do when
>     'die()'ing from inside of upload-pack.c
> 
>   - Don't mark said message(s) for translation, matching what we do in
>     the rest of upload-pack.c.
> 
>   - Re-introduce the 'test_must_fail ok=sigpipe' and stop grepping
>     stderr for the right message.
> 
> Do I have that right?

I'm not Gábor, but that is the sequence I think is best.

Between the die() and the ERR, the ERR packets are way more useful in
practice, since they actually go back to the client. I'd even suggest we
do away with the die() messages entirely (since they're either redundant
or go nowhere, depending on the protocol), but I think it would make
sense to wait until the raciness issues are fixed (until then, they
_might_ help in the redundant cases, which is what the test here is
relying on).

-Peff

Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

[Taylor Blau]

On Fri, Jul 24, 2020 at 03:51:26PM -0400, Jeff King wrote:
> On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:
>
> > Let me double check my understanding... I think that you are suggesting
> > the following three things:
> >
> >   - Write the same message as an err packet over the wire as we do when
> >     'die()'ing from inside of upload-pack.c
> >
> >   - Don't mark said message(s) for translation, matching what we do in
> >     the rest of upload-pack.c.
> >
> >   - Re-introduce the 'test_must_fail ok=sigpipe' and stop grepping
> >     stderr for the right message.
> >
> > Do I have that right?
>
> I'm not Gábor, but that is the sequence I think is best.
>
> Between the die() and the ERR, the ERR packets are way more useful in
> practice, since they actually go back to the client. I'd even suggest we
> do away with the die() messages entirely (since they're either redundant
> or go nowhere, depending on the protocol), but I think it would make
> sense to wait until the raciness issues are fixed (until then, they
> _might_ help in the redundant cases, which is what the test here is
> relying on).

All sounds good. I'll wait for an ACK from Gábor to make sure that this
is the direction that they want, too.

In the meantime, this is what I have locally. I'll send a series which
has this as its range-diff if Gábor approves of the direction.

1:  6a77af563e = 1:  6a77af563e list_objects_filter_options: introduce 'list_object_filter_config_name'
2:  6dbf58441d ! 2:  80d19481d8 upload-pack.c: allow banning certain object filter(s)
    @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil

     +test_expect_success 'upload-pack fails banned object filters' '
     +	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
    -+	test_must_fail git clone --no-checkout --filter=blob:none \
    -+		"file://$(pwd)/srv.bare" pc3 2>err &&
    -+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
    ++	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
    ++		"file://$(pwd)/srv.bare" pc3
     +'
     +
     +test_expect_success 'upload-pack fails banned combine object filters' '
    @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil
     +	test_config -C srv.bare uploadpackfilter.combine.allow true &&
     +	test_config -C srv.bare uploadpackfilter.tree.allow true &&
     +	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
    -+	test_must_fail git clone --no-checkout --filter=tree:1 \
    -+		--filter=blob:none "file://$(pwd)/srv.bare" pc3 2>err &&
    -+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
    ++	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
    ++		--filter=blob:none "file://$(pwd)/srv.bare" pc3
     +'
     +
     +test_expect_success 'upload-pack fails banned object filters with fallback' '
     +	test_config -C srv.bare uploadpackfilter.allow false &&
    -+	test_must_fail git clone --no-checkout --filter=blob:none \
    -+		"file://$(pwd)/srv.bare" pc3 2>err &&
    -+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
    ++	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
    ++		"file://$(pwd)/srv.bare" pc3
     +'
     +
      test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
    @@ upload-pack.c: static int process_deepen_not(const char *line, struct string_lis
     +{
     +	struct list_objects_filter_options *banned = banned_filter(data,
     +								   &data->filter_options);
    ++	struct strbuf buf = STRBUF_INIT;
     +	if (!banned)
     +		return;
     +
    -+	die(_("git upload-pack: filter '%s' not supported"),
    -+	    list_object_filter_config_name(banned->choice));
    ++	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
    ++		    list_object_filter_config_name(banned->choice));
    ++
    ++	packet_writer_error(&data->writer, "%s\n", buf.buf);
    ++	die("%s", buf.buf);
     +}
     +
      static void receive_needs(struct upload_pack_data *data,
3:  bacdea47d9 = 3:  790552c7c2 upload-pack.c: pass 'struct list_objects_filter_options *'
4:  79af94a41b ! 4:  eb9a81eb1f upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'
    @@ Documentation/config/uploadpack.txt: uploadpackfilter.<filter>.allow::

      ## t/t5616-partial-clone.sh ##
     @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object filters with fallback' '
    - 	test_i18ngrep "filter '\''blob:none'\'' not supported" err
    + 		"file://$(pwd)/srv.bare" pc3
      '

     +test_expect_success 'upload-pack limits tree depth filters' '
    @@ upload-pack.c: static int allows_filter_choice(struct upload_pack_data *data,
      }

     @@ upload-pack.c: 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;

    --	die(_("git upload-pack: filter '%s' not supported"),
    --	    list_object_filter_config_name(banned->choice));
    -+	strbuf_addf(&buf, _("filter '%s' not supported"),
    -+		    list_object_filter_config_name(banned->choice));
    + 	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);
    -+	die("%s", buf.buf);
    - }

    - static void receive_needs(struct upload_pack_data *data,
    + 	packet_writer_error(&data->writer, "%s\n", buf.buf);
    + 	die("%s", buf.buf);
     @@ upload-pack.c: 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 =

> -Peff

Thanks,
Taylor

Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

[SZEDER Gábor]

On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:
> On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:
> > On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:
> > > Here's a much-delayed v2 of my series to teach upload-pack to limit the
> > > kinds of object filters that it is willing to server in a request.
> > >
> > > Much is the same since last time, with two notable exceptions:
> > >
> > >   - We use the 'uploadpackfilter' top-level configuration key instead of
> > >     pretending that 'uploadpack.filter' is top-level, which greatly
> > >     simplifies the call to 'parse_config_key()'.
> > >
> > >   - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,
> >
> > To clarify, I only recommended to pass the same message to die() as in
> > the ERR packet, not dropping the ERR packet, because ...
> >
> > >     which propagates the error through 'git clone' always,
> >
> > it does in the new tests when creating a local clone, but does it
> > really work with all protocols and remote helpers and what not?
> >
> > >     and resolves
> > >     a flaky set of tests that used to result in a SIGPIPE.
> >
> > This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'
> > can still abort while 'git clone' is still sending packets.  IOW we
> > still need that 'test_must_fail ok=sigpipe' in all new tests.
> 
> Let me double check my understanding... I think that you are suggesting
> the following three things:
> 
>   - Write the same message as an err packet over the wire as we do when
>     'die()'ing from inside of upload-pack.c

Yes, though I'm not quite sure that I understand this sentence
correctly, and unless you can convincingly argue in the commit message
that the die() messages make it to the client no matter the
protocol.

>   - Don't mark said message(s) for translation, matching what we do in
>     the rest of upload-pack.c.

Yes.

>   - Re-introduce the 'test_must_fail ok=sigpipe'

Yes.

>      and stop grepping stderr for the right message.

No, please check them, I love those error messages :)

> Do I have that right?
> 
> Thanks,
> Taylor

Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

[Taylor Blau]

On Mon, Jul 27, 2020 at 09:34:32PM +0200, SZEDER Gábor wrote:
> On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:
> > On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:
> > > On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:
> > > > Here's a much-delayed v2 of my series to teach upload-pack to limit the
> > > > kinds of object filters that it is willing to server in a request.
> > > >
> > > > Much is the same since last time, with two notable exceptions:
> > > >
> > > >   - We use the 'uploadpackfilter' top-level configuration key instead of
> > > >     pretending that 'uploadpack.filter' is top-level, which greatly
> > > >     simplifies the call to 'parse_config_key()'.
> > > >
> > > >   - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,
> > >
> > > To clarify, I only recommended to pass the same message to die() as in
> > > the ERR packet, not dropping the ERR packet, because ...
> > >
> > > >     which propagates the error through 'git clone' always,
> > >
> > > it does in the new tests when creating a local clone, but does it
> > > really work with all protocols and remote helpers and what not?
> > >
> > > >     and resolves
> > > >     a flaky set of tests that used to result in a SIGPIPE.
> > >
> > > This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'
> > > can still abort while 'git clone' is still sending packets.  IOW we
> > > still need that 'test_must_fail ok=sigpipe' in all new tests.
> >
> > Let me double check my understanding... I think that you are suggesting
> > the following three things:
> >
> >   - Write the same message as an err packet over the wire as we do when
> >     'die()'ing from inside of upload-pack.c
>
> Yes, though I'm not quite sure that I understand this sentence
> correctly, and unless you can convincingly argue in the commit message
> that the die() messages make it to the client no matter the
> protocol.
>
> >   - Don't mark said message(s) for translation, matching what we do in
> >     the rest of upload-pack.c.
>
> Yes.
>
> >   - Re-introduce the 'test_must_fail ok=sigpipe'
>
> Yes.
>
> >      and stop grepping stderr for the right message.
>
> No, please check them, I love those error messages :)

Isn't the problem that these messages only sometimes make it to the
client depending on what protocol is in use? If so, the right thing to
do would be to not grep for them, since it will make that test flakey.

Maybe I'm misunderstanding you/the problem...

> > Do I have that right?
> >
> > Thanks,
> > Taylor

Thanks,
Taylor

Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

[Jeff King]

On Mon, Jul 27, 2020 at 03:36:57PM -0400, Taylor Blau wrote:

> > >      and stop grepping stderr for the right message.
> >
> > No, please check them, I love those error messages :)
> 
> Isn't the problem that these messages only sometimes make it to the
> client depending on what protocol is in use? If so, the right thing to
> do would be to not grep for them, since it will make that test flakey.
> 
> Maybe I'm misunderstanding you/the problem...

You'd have to omit them for tests over certain protocols, but it would
always be robust to check them over local-filesystem clones and fetches.
So it may be that some tests need them and some do not.

-Peff

Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

[SZEDER Gábor]

On Mon, Jul 27, 2020 at 03:36:57PM -0400, Taylor Blau wrote:
> On Mon, Jul 27, 2020 at 09:34:32PM +0200, SZEDER Gábor wrote:
> > On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:
> > > On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:
> > > > On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:
> > > > > Here's a much-delayed v2 of my series to teach upload-pack to limit the
> > > > > kinds of object filters that it is willing to server in a request.
> > > > >
> > > > > Much is the same since last time, with two notable exceptions:
> > > > >
> > > > >   - We use the 'uploadpackfilter' top-level configuration key instead of
> > > > >     pretending that 'uploadpack.filter' is top-level, which greatly
> > > > >     simplifies the call to 'parse_config_key()'.
> > > > >
> > > > >   - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,
> > > >
> > > > To clarify, I only recommended to pass the same message to die() as in
> > > > the ERR packet, not dropping the ERR packet, because ...
> > > >
> > > > >     which propagates the error through 'git clone' always,
> > > >
> > > > it does in the new tests when creating a local clone, but does it
> > > > really work with all protocols and remote helpers and what not?
> > > >
> > > > >     and resolves
> > > > >     a flaky set of tests that used to result in a SIGPIPE.
> > > >
> > > > This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'
> > > > can still abort while 'git clone' is still sending packets.  IOW we
> > > > still need that 'test_must_fail ok=sigpipe' in all new tests.
> > >
> > > Let me double check my understanding... I think that you are suggesting
> > > the following three things:
> > >
> > >   - Write the same message as an err packet over the wire as we do when
> > >     'die()'ing from inside of upload-pack.c
> >
> > Yes, though I'm not quite sure that I understand this sentence
> > correctly, and unless you can convincingly argue in the commit message
> > that the die() messages make it to the client no matter the
> > protocol.
> >
> > >   - Don't mark said message(s) for translation, matching what we do in
> > >     the rest of upload-pack.c.
> >
> > Yes.
> >
> > >   - Re-introduce the 'test_must_fail ok=sigpipe'
> >
> > Yes.
> >
> > >      and stop grepping stderr for the right message.
> >
> > No, please check them, I love those error messages :)
> 
> Isn't the problem that these messages only sometimes make it to the
> client depending on what protocol is in use? If so, the right thing to
> do would be to not grep for them, since it will make that test flakey.

To my understanding the die() messages always make it to the client's
stderr when cloning from a local repository.  And that's exactly what
all new tests do, i.e they do run 'git clone ... file://', that's why
I recommended to pass those fancy error messages to die(), because
that's easy enough to do, and doesn't require us to make 'git clone'
SIGPIPE-proof first (which, I agree, is beyond the scope of this
series).

> Maybe I'm misunderstanding you/the problem...
> 
> > > Do I have that right?
> > >
> > > Thanks,
> > > Taylor
> 
> Thanks,
> Taylor

Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

[Taylor Blau]

On Mon, Jul 27, 2020 at 09:59:12PM +0200, SZEDER Gábor wrote:
> On Mon, Jul 27, 2020 at 03:36:57PM -0400, Taylor Blau wrote:
> > On Mon, Jul 27, 2020 at 09:34:32PM +0200, SZEDER Gábor wrote:
> > > On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:
> > > > On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:
> > > > > On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:
> > > > > > Here's a much-delayed v2 of my series to teach upload-pack to limit the
> > > > > > kinds of object filters that it is willing to server in a request.
> > > > > >
> > > > > > Much is the same since last time, with two notable exceptions:
> > > > > >
> > > > > >   - We use the 'uploadpackfilter' top-level configuration key instead of
> > > > > >     pretending that 'uploadpack.filter' is top-level, which greatly
> > > > > >     simplifies the call to 'parse_config_key()'.
> > > > > >
> > > > > >   - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,
> > > > >
> > > > > To clarify, I only recommended to pass the same message to die() as in
> > > > > the ERR packet, not dropping the ERR packet, because ...
> > > > >
> > > > > >     which propagates the error through 'git clone' always,
> > > > >
> > > > > it does in the new tests when creating a local clone, but does it
> > > > > really work with all protocols and remote helpers and what not?
> > > > >
> > > > > >     and resolves
> > > > > >     a flaky set of tests that used to result in a SIGPIPE.
> > > > >
> > > > > This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'
> > > > > can still abort while 'git clone' is still sending packets.  IOW we
> > > > > still need that 'test_must_fail ok=sigpipe' in all new tests.
> > > >
> > > > Let me double check my understanding... I think that you are suggesting
> > > > the following three things:
> > > >
> > > >   - Write the same message as an err packet over the wire as we do when
> > > >     'die()'ing from inside of upload-pack.c
> > >
> > > Yes, though I'm not quite sure that I understand this sentence
> > > correctly, and unless you can convincingly argue in the commit message
> > > that the die() messages make it to the client no matter the
> > > protocol.
> > >
> > > >   - Don't mark said message(s) for translation, matching what we do in
> > > >     the rest of upload-pack.c.
> > >
> > > Yes.
> > >
> > > >   - Re-introduce the 'test_must_fail ok=sigpipe'
> > >
> > > Yes.
> > >
> > > >      and stop grepping stderr for the right message.
> > >
> > > No, please check them, I love those error messages :)
> >
> > Isn't the problem that these messages only sometimes make it to the
> > client depending on what protocol is in use? If so, the right thing to
> > do would be to not grep for them, since it will make that test flakey.
>
> To my understanding the die() messages always make it to the client's
> stderr when cloning from a local repository.  And that's exactly what
> all new tests do, i.e they do run 'git clone ... file://', that's why
> I recommended to pass those fancy error messages to die(), because
> that's easy enough to do, and doesn't require us to make 'git clone'
> SIGPIPE-proof first (which, I agree, is beyond the scope of this
> series).

Ah, thanks for your patient explanation of what's going on here. These
tests _would_ be flakey if run over non-local protocols, but they
aren't, so we can (and should) safely grep for the right error message.

So the things to do are the original list, nixing "stop grepping stderr
for ...". Easy ;-).

> > Maybe I'm misunderstanding you/the problem...
> >
> > > > Do I have that right?
> > > >
> > > > Thanks,
> > > > Taylor
> >
> > Thanks,
> > Taylor

Thanks,
Taylor

[PATCH v3 0/4] upload-pack: custom allowed object filters

[Taylor Blau]

Hi,

Here's the third re-roll of my series to teach upload-pack to allow only
certain kinds of object filters. The only thing that has changed since
last time is that we now *do* look for the error messages in t5616.

Normally these error messages may or may not show up (because of the
aforementioned SIGPIPE issue in 'git clone'), but since we are cloning
from a 'file://', we can guarantee that they will appear (and can thusly
be grepped for).

Taylor Blau (4):
  list_objects_filter_options: introduce
    'list_object_filter_config_name'
  upload-pack.c: allow banning certain object filter(s)
  upload-pack.c: pass 'struct list_objects_filter_options *'
  upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

 Documentation/config/uploadpack.txt |  18 +++++
 list-objects-filter-options.c       |  23 ++++++
 list-objects-filter-options.h       |   6 ++
 t/t5616-partial-clone.sh            |  33 +++++++++
 upload-pack.c                       | 105 ++++++++++++++++++++++++++++
 5 files changed, 185 insertions(+)

Range-diff against v2:
[...rebased on the tip of 'master']
 1:  6a77af563e = 80:  b1b3dd7de9 list_objects_filter_options: introduce 'list_object_filter_config_name'
 2:  6dbf58441d ! 81:  a0a0427757 upload-pack.c: allow banning certain object filter(s)
    @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil

     +test_expect_success 'upload-pack fails banned object filters' '
     +	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
    -+	test_must_fail git clone --no-checkout --filter=blob:none \
    ++	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
     +		"file://$(pwd)/srv.bare" pc3 2>err &&
     +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
     +'
    @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil
     +	test_config -C srv.bare uploadpackfilter.combine.allow true &&
     +	test_config -C srv.bare uploadpackfilter.tree.allow true &&
     +	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
    -+	test_must_fail git clone --no-checkout --filter=tree:1 \
    ++	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
     +		--filter=blob:none "file://$(pwd)/srv.bare" pc3 2>err &&
     +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
     +'
     +
     +test_expect_success 'upload-pack fails banned object filters with fallback' '
     +	test_config -C srv.bare uploadpackfilter.allow false &&
    -+	test_must_fail git clone --no-checkout --filter=blob:none \
    ++	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
     +		"file://$(pwd)/srv.bare" pc3 2>err &&
     +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
     +'
    @@ upload-pack.c: static int process_deepen_not(const char *line, struct string_lis
     +{
     +	struct list_objects_filter_options *banned = banned_filter(data,
     +								   &data->filter_options);
    ++	struct strbuf buf = STRBUF_INIT;
     +	if (!banned)
     +		return;
     +
    -+	die(_("git upload-pack: filter '%s' not supported"),
    -+	    list_object_filter_config_name(banned->choice));
    ++	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
    ++		    list_object_filter_config_name(banned->choice));
    ++
    ++	packet_writer_error(&data->writer, "%s\n", buf.buf);
    ++	die("%s", buf.buf);
     +}
     +
      static void receive_needs(struct upload_pack_data *data,
 3:  bacdea47d9 = 82:  ad3f0cce56 upload-pack.c: pass 'struct list_objects_filter_options *'
 4:  79af94a41b ! 83:  c9d71809f4 upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'
    @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object f
     +	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
    ++		"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' '
    @@ upload-pack.c: static int allows_filter_choice(struct upload_pack_data *data,
      }

     @@ upload-pack.c: 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;

    --	die(_("git upload-pack: filter '%s' not supported"),
    --	    list_object_filter_config_name(banned->choice));
    -+	strbuf_addf(&buf, _("filter '%s' not supported"),
    -+		    list_object_filter_config_name(banned->choice));
    + 	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);
    -+	die("%s", buf.buf);
    - }

    - static void receive_needs(struct upload_pack_data *data,
    + 	packet_writer_error(&data->writer, "%s\n", buf.buf);
    + 	die("%s", buf.buf);
     @@ upload-pack.c: 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 =
--
2.28.0.rc1.13.ge78abce653

[PATCH v3 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name'

[Taylor Blau]

In a subsequent commit, we will add configuration options that are
specific to each kind of object filter, in which case it is handy to
have a function that translates between 'enum
list_objects_filter_choice' and an appropriate configuration-friendly
string.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 list-objects-filter-options.c | 23 +++++++++++++++++++++++
 list-objects-filter-options.h |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 3553ad7b0a..92b408c0c8 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -15,6 +15,29 @@ static int parse_combine_filter(
 	const char *arg,
 	struct strbuf *errbuf);
 
+const char *list_object_filter_config_name(enum list_objects_filter_choice c)
+{
+	switch (c) {
+	case LOFC_DISABLED:
+		/* we have no name for "no filter at all" */
+		break;
+	case LOFC_BLOB_NONE:
+		return "blob:none";
+	case LOFC_BLOB_LIMIT:
+		return "blob:limit";
+	case LOFC_TREE_DEPTH:
+		return "tree";
+	case LOFC_SPARSE_OID:
+		return "sparse:oid";
+	case LOFC_COMBINE:
+		return "combine";
+	case LOFC__COUNT:
+		/* not a real filter type; just the count of all filters */
+		break;
+	}
+	BUG("list_object_filter_choice_name: invalid argument '%d'", c);
+}
+
 /*
  * Parse value of the argument to the "filter" keyword.
  * On the command line this looks like:
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index 73fffa4ad7..01767c3c96 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -17,6 +17,12 @@ enum list_objects_filter_choice {
 	LOFC__COUNT /* must be last */
 };
 
+/*
+ * Returns a configuration key suitable for describing the given object filter,
+ * e.g.: "blob:none", "combine", etc.
+ */
+const char *list_object_filter_config_name(enum list_objects_filter_choice c);
+
 struct list_objects_filter_options {
 	/*
 	 * 'filter_spec' is the raw argument value given on the command line
-- 
2.28.0.rc1.13.ge78abce653

[PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s)

[Taylor Blau]

Git clients may ask the server for a partial set of objects, where the
set of objects being requested is refined by one or more object filters.
Server administrators can configure 'git upload-pack' to allow or ban
these filters by setting the 'uploadpack.allowFilter' variable to
'true' or 'false', respectively.

However, administrators using bitmaps may wish to allow certain kinds of
object filters, but ban others. Specifically, they may wish to allow
object filters that can be optimized by the use of bitmaps, while
rejecting other object filters which aren't and represent a perceived
performance degradation (as well as an increased load factor on the
server).

Allow configuring 'git upload-pack' to support object filters on a
case-by-case basis by introducing two new configuration variables:

  - 'uploadpackfilter.allow'
  - 'uploadpackfilter.<kind>.allow'

where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on.

Setting the second configuration variable for any valid value of
'<kind>' explicitly allows or disallows restricting that kind of object
filter.

If a client requests the object filter <kind> and the respective
configuration value is not set, 'git upload-pack' will default to the
value of 'uploadpackfilter.allow', which itself defaults to 'true' to
maintain backwards compatibility. Note that this differs from
'uploadpack.allowfilter', which controls whether or not the 'filter'
capability is advertised.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/uploadpack.txt | 12 +++++
 t/t5616-partial-clone.sh            | 24 +++++++++
 upload-pack.c                       | 80 +++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index ed1c835695..fffe8ac648 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -57,6 +57,18 @@ uploadpack.allowFilter::
 	If this option is set, `upload-pack` will support partial
 	clone and partial fetch object filtering.
 
+uploadpackfilter.allow::
+	Provides a default value for unspecified object filters (see: the
+	below configuration variable).
+	Defaults to `true`.
+
+uploadpackfilter.<filter>.allow::
+	Explicitly allow or ban the object filter corresponding to
+	`<filter>`, where `<filter>` may be one of: `blob:none`,
+	`blob:limit`, `tree`, `sparse:oid`, or `combine`. If using
+	combined filters, both `combine` and all of the nested filter
+	kinds must be allowed. Defaults to `uploadpackfilter.allow`.
+
 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 37de0afb02..0d46b5a2f8 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -235,6 +235,30 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' '
 	test_cmp unique_types.expected unique_types.actual
 '
 
+test_expect_success 'upload-pack fails banned object filters' '
+	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
+		"file://$(pwd)/srv.bare" pc3 2>err &&
+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
+'
+
+test_expect_success 'upload-pack fails banned combine object filters' '
+	test_config -C srv.bare uploadpackfilter.allow false &&
+	test_config -C srv.bare uploadpackfilter.combine.allow true &&
+	test_config -C srv.bare uploadpackfilter.tree.allow true &&
+	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
+		--filter=blob:none "file://$(pwd)/srv.bare" pc3 2>err &&
+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
+'
+
+test_expect_success 'upload-pack fails banned object filters with fallback' '
+	test_config -C srv.bare uploadpackfilter.allow false &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
+		"file://$(pwd)/srv.bare" pc3 2>err &&
+	test_i18ngrep "filter '\''blob:none'\'' not supported" 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 8673741070..ed2098edd0 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -88,6 +88,7 @@ struct upload_pack_data {
 	enum allow_uor allow_uor;
 
 	struct list_objects_filter_options filter_options;
+	struct string_list allowed_filters;
 
 	struct packet_writer writer;
 
@@ -103,6 +104,7 @@ struct upload_pack_data {
 	unsigned no_progress : 1;
 	unsigned use_include_tag : 1;
 	unsigned allow_filter : 1;
+	unsigned allow_filter_fallback : 1;
 
 	unsigned done : 1;					/* v2 only */
 	unsigned allow_ref_in_want : 1;				/* v2 only */
@@ -120,6 +122,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
 	struct string_list uri_protocols = STRING_LIST_INIT_DUP;
 	struct object_array extra_edge_obj = OBJECT_ARRAY_INIT;
+	struct string_list allowed_filters = STRING_LIST_INIT_DUP;
 
 	memset(data, 0, sizeof(*data));
 	data->symref = symref;
@@ -131,6 +134,8 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->deepen_not = deepen_not;
 	data->uri_protocols = uri_protocols;
 	data->extra_edge_obj = extra_edge_obj;
+	data->allowed_filters = allowed_filters;
+	data->allow_filter_fallback = 1;
 	packet_writer_init(&data->writer, 1);
 
 	data->keepalive = 5;
@@ -147,6 +152,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	string_list_clear(&data->deepen_not, 0);
 	object_array_clear(&data->extra_edge_obj);
 	list_objects_filter_release(&data->filter_options);
+	string_list_clear(&data->allowed_filters, 1);
 
 	free((char *)data->pack_objects_hook);
 }
@@ -984,6 +990,50 @@ 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,
+				enum list_objects_filter_choice c)
+{
+	const char *key = list_object_filter_config_name(c);
+	struct string_list_item *item = string_list_lookup(&data->allowed_filters,
+							   key);
+	if (item)
+		return (intptr_t) item->util;
+	return data->allow_filter_fallback;
+}
+
+static struct list_objects_filter_options *banned_filter(
+	struct upload_pack_data *data,
+	struct list_objects_filter_options *opts)
+{
+	size_t i;
+
+	if (!allows_filter_choice(data, opts->choice))
+		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))
+				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;
+	if (!banned)
+		return;
+
+	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
+		    list_object_filter_config_name(banned->choice));
+
+	packet_writer_error(&data->writer, "%s\n", buf.buf);
+	die("%s", buf.buf);
+}
+
 static void receive_needs(struct upload_pack_data *data,
 			  struct packet_reader *reader)
 {
@@ -1014,6 +1064,7 @@ static void receive_needs(struct upload_pack_data *data,
 				die("git upload-pack: filtering capability not negotiated");
 			list_objects_filter_die_if_populated(&data->filter_options);
 			parse_list_objects_filter(&data->filter_options, arg);
+			die_if_using_banned_filter(data);
 			continue;
 		}
 
@@ -1171,6 +1222,32 @@ static int find_symref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
+static int parse_object_filter_config(const char *var, const char *value,
+				       struct upload_pack_data *data)
+{
+	struct strbuf buf = STRBUF_INIT;
+	const char *sub, *key;
+	size_t sub_len;
+
+	if (parse_config_key(var, "uploadpackfilter", &sub, &sub_len, &key))
+		return 0;
+
+	if (!sub) {
+		if (!strcmp(key, "allow"))
+			data->allow_filter_fallback = git_config_bool(var, value);
+		return 0;
+	}
+
+	strbuf_add(&buf, sub, sub_len);
+
+	if (!strcmp(key, "allow"))
+		string_list_insert(&data->allowed_filters, buf.buf)->util =
+			(void *)(intptr_t)git_config_bool(var, value);
+
+	strbuf_release(&buf);
+	return 0;
+}
+
 static int upload_pack_config(const char *var, const char *value, void *cb_data)
 {
 	struct upload_pack_data *data = cb_data;
@@ -1210,6 +1287,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
 			return git_config_string(&data->pack_objects_hook, var, value);
 	}
 
+	parse_object_filter_config(var, value, data);
+
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
@@ -1390,6 +1469,7 @@ static void process_args(struct packet_reader *request,
 		if (data->allow_filter && skip_prefix(arg, "filter ", &p)) {
 			list_objects_filter_die_if_populated(&data->filter_options);
 			parse_list_objects_filter(&data->filter_options, p);
+			die_if_using_banned_filter(data);
 			continue;
 		}
 
-- 
2.28.0.rc1.13.ge78abce653

Re: [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s)

[Jeff King]

On Fri, Jul 31, 2020 at 04:26:31PM -0400, Taylor Blau wrote:

> Git clients may ask the server for a partial set of objects, where the
> set of objects being requested is refined by one or more object filters.
> Server administrators can configure 'git upload-pack' to allow or ban
> these filters by setting the 'uploadpack.allowFilter' variable to
> 'true' or 'false', respectively.
> 
> However, administrators using bitmaps may wish to allow certain kinds of
> object filters, but ban others. Specifically, they may wish to allow
> object filters that can be optimized by the use of bitmaps, while
> rejecting other object filters which aren't and represent a perceived
> performance degradation (as well as an increased load factor on the
> server).
> 
> Allow configuring 'git upload-pack' to support object filters on a
> case-by-case basis by introducing two new configuration variables:
> 
>   - 'uploadpackfilter.allow'
>   - 'uploadpackfilter.<kind>.allow'
> 
> where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on.

Minor nit, but <kind> is "blob:none", "blob:limit", etc. The code and
documentation gets this right; it's just the commit message.

I'm pretty sure this is a casualty of updating the syntax as the series
was developed. One trick I use is to avoid repeating explanations that
are in the documentation from the patch already. I.e., explain "why"
here, but it's OK to let "what" come from the patch itself. That's not
only one less thing to remember to update, but it's less for reviewers
to read through, too.

</meta-patch-advice>

> +test_expect_success 'upload-pack fails banned object filters' '
> +	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
> +	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
> +		"file://$(pwd)/srv.bare" pc3 2>err &&
> +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
> +'

These messages aren't translated now, so we can just use grep, I think.

Ditto in the other tests below.

> +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));
> +
> +	packet_writer_error(&data->writer, "%s\n", buf.buf);
> +	die("%s", buf.buf);
> +}

Hmm, the strbuf was unexpected. I'd have just written it out twice.
After all, the messages don't have to be the same. And perhaps we don't
want them to be the same? A user receiving the ERR packet would see:

  remote error: git upload-pack: filter 'foo' not supported

do we need the "git upload-pack" part there? Other errors say just
"upload-pack". IMHO even that is unnecessarily verbose, and I wouldn't
mind a separate patch to reduce it. But definitely going even longer
doesn't seem like the right direction. :)

I also wondered about the trailing newline. Other callers of
packet_writer_error() don't seem to use it. I think in practice it
doesn't matter much because readers will generally be using
CHOMP_NEWLINE, but it probably makes sense to be consistent.

> [...]

Aside from this nits, this patch looks good to me.

-Peff

Re: [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s)

[Taylor Blau]

On Fri, Jul 31, 2020 at 04:54:34PM -0400, Jeff King wrote:
> On Fri, Jul 31, 2020 at 04:26:31PM -0400, Taylor Blau wrote:
>
> > Git clients may ask the server for a partial set of objects, where the
> > set of objects being requested is refined by one or more object filters.
> > Server administrators can configure 'git upload-pack' to allow or ban
> > these filters by setting the 'uploadpack.allowFilter' variable to
> > 'true' or 'false', respectively.
> >
> > However, administrators using bitmaps may wish to allow certain kinds of
> > object filters, but ban others. Specifically, they may wish to allow
> > object filters that can be optimized by the use of bitmaps, while
> > rejecting other object filters which aren't and represent a perceived
> > performance degradation (as well as an increased load factor on the
> > server).
> >
> > Allow configuring 'git upload-pack' to support object filters on a
> > case-by-case basis by introducing two new configuration variables:
> >
> >   - 'uploadpackfilter.allow'
> >   - 'uploadpackfilter.<kind>.allow'
> >
> > where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on.
>
> Minor nit, but <kind> is "blob:none", "blob:limit", etc. The code and
> documentation gets this right; it's just the commit message.
>
> I'm pretty sure this is a casualty of updating the syntax as the series
> was developed. One trick I use is to avoid repeating explanations that
> are in the documentation from the patch already. I.e., explain "why"
> here, but it's OK to let "what" come from the patch itself. That's not
> only one less thing to remember to update, but it's less for reviewers
> to read through, too.
>
> </meta-patch-advice>

Good advice, and you're right that <kind> is blob:none and similar, not
'blobNone'. In this case, I think the "why" is pretty boring, so I don't
mind repeating myself a little bit in the commit message.
>
> > +test_expect_success 'upload-pack fails banned object filters' '
> > +	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
> > +	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
> > +		"file://$(pwd)/srv.bare" pc3 2>err &&
> > +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
> > +'
>
> These messages aren't translated now, so we can just use grep, I think.
>
> Ditto in the other tests below.

Ack, yep.

>
> > +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));
> > +
> > +	packet_writer_error(&data->writer, "%s\n", buf.buf);
> > +	die("%s", buf.buf);
> > +}
>
> Hmm, the strbuf was unexpected. I'd have just written it out twice.
> After all, the messages don't have to be the same. And perhaps we don't
> want them to be the same? A user receiving the ERR packet would see:
>
>   remote error: git upload-pack: filter 'foo' not supported
>
> do we need the "git upload-pack" part there? Other errors say just
> "upload-pack". IMHO even that is unnecessarily verbose, and I wouldn't
> mind a separate patch to reduce it. But definitely going even longer
> doesn't seem like the right direction. :)

Let's drop 'git upload-pack: ' entirely, and just start the message with
'filter ...'. It looks like you figured out why we use a strbuf here in
your response to the fourth patch.

> I also wondered about the trailing newline. Other callers of
> packet_writer_error() don't seem to use it. I think in practice it
> doesn't matter much because readers will generally be using
> CHOMP_NEWLINE, but it probably makes sense to be consistent.

Dropping the newline should be easy enough.

>
> > [...]
>
> Aside from this nits, this patch looks good to me.
>
> -Peff

Thanks,
Taylor

[PATCH v3 3/4] upload-pack.c: pass 'struct list_objects_filter_options *'

[Taylor Blau]

The 'allows_filter_choice' function used to take an 'enum
list_objects_filter_choice', but in a future commit it will be more
convenient for it to accept the whole 'struct
list_objects_filter_options', for e.g., to inspect the value of
'->tree_exclude_depth'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 upload-pack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index ed2098edd0..5fa22da31f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -991,9 +991,9 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,
 }
 
 static int allows_filter_choice(struct upload_pack_data *data,
-				enum list_objects_filter_choice c)
+				struct list_objects_filter_options *opts)
 {
-	const char *key = list_object_filter_config_name(c);
+	const char *key = list_object_filter_config_name(opts->choice);
 	struct string_list_item *item = string_list_lookup(&data->allowed_filters,
 							   key);
 	if (item)
@@ -1007,7 +1007,7 @@ static struct list_objects_filter_options *banned_filter(
 {
 	size_t i;
 
-	if (!allows_filter_choice(data, opts->choice))
+	if (!allows_filter_choice(data, opts))
 		return opts;
 
 	if (opts->choice == LOFC_COMBINE)
-- 
2.28.0.rc1.13.ge78abce653

[PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

[Taylor Blau]

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(-)

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;
-- 
2.28.0.rc1.13.ge78abce653

Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

[Jeff King]

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

Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

[Jeff King]

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,

Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

[Taylor Blau]

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

Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

[Taylor Blau]

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

Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

[Jeff King]

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

Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

[Jeff King]

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 v4 0/3] upload-pack: custom allowed object filters

[Taylor Blau]

Hi,

Here's what I anticipate to be the final reroll of my series to teach
the new 'uploadpackfilter' configuration section, which allows for more
fine-grained control over which object filters upload-pack is willing to
serve.

Two changes from last time:

  - I adopted Peff's suggestion in beginning in [1], but appropriately
    split it over the existing patch structure.

  - I dropped the old patch 3/4, since it really should have never been
    there in the first place, and just made the refactoring more noisy
    than necessary.

    (For the curious, this patch was written as a preparatory step
    within GitHub's fork in order to add the
    'uploadpackfilter.tree.maxDepth' configuration. This was after the
    initial work when I hadn't yet considered adding such a thing. Now
    that we know the full arc, it makes sense to just pass the right
    parameters from the get-go).

Thanks again for all of the review!

[1] :https://lore.kernel.org/git/20200731210114.GC1440890@coredump.intra.peff.net/

Taylor Blau (3):
  list_objects_filter_options: introduce
    'list_object_filter_config_name'
  upload-pack.c: allow banning certain object filter(s)
  upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

 Documentation/config/uploadpack.txt |  18 +++++
 list-objects-filter-options.c       |  23 ++++++
 list-objects-filter-options.h       |   6 ++
 t/t5616-partial-clone.sh            |  33 +++++++++
 upload-pack.c                       | 104 ++++++++++++++++++++++++++++
 5 files changed, 184 insertions(+)

Range-diff against v3:
-:  ---------- > 1:  21531927e4 Revert "fmt-merge-msg: stop treating `master` specially"
-:  ---------- > 2:  6e6029a82a fmt-merge-msg: allow merge destination to be omitted again
-:  ---------- > 3:  25429fed5c refs: move the logic to add \t to reflog to the files backend
-:  ---------- > 4:  3db796c1c0 t6300: fix issues related to %(contents:size)
-:  ---------- > 5:  85b4e0a6dc Third batch
1:  b1b3dd7de9 = 6:  f4c7771875 list_objects_filter_options: introduce 'list_object_filter_config_name'
2:  a0a0427757 ! 7:  b34f4eaed9 upload-pack.c: allow banning certain object filter(s)
    @@ Commit message
         'uploadpack.allowfilter', which controls whether or not the 'filter'
         capability is advertised.

    +    Helped-by: Jeff King <peff@peff.net>
         Signed-off-by: Taylor Blau <me@ttaylorr.com>

      ## Documentation/config/uploadpack.txt ##
    @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil
     +	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
     +	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
     +		"file://$(pwd)/srv.bare" pc3 2>err &&
    -+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
    ++	grep "filter '\''blob:none'\'' not supported" err
     +'
     +
     +test_expect_success 'upload-pack fails banned combine object filters' '
    @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil
     +	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
     +	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
     +		--filter=blob:none "file://$(pwd)/srv.bare" pc3 2>err &&
    -+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
    ++	grep "filter '\''blob:none'\'' not supported" err
     +'
     +
     +test_expect_success 'upload-pack fails banned object filters with fallback' '
     +	test_config -C srv.bare uploadpackfilter.allow false &&
     +	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
     +		"file://$(pwd)/srv.bare" pc3 2>err &&
    -+	test_i18ngrep "filter '\''blob:none'\'' not supported" err
    ++	grep "filter '\''blob:none'\'' not supported" err
     +'
     +
      test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
    @@ upload-pack.c: static int process_deepen_not(const char *line, struct string_lis
      	return 0;
      }

    -+static int allows_filter_choice(struct upload_pack_data *data,
    -+				enum list_objects_filter_choice c)
    ++NORETURN __attribute__((format(printf,2,3)))
    ++static void send_err_and_die(struct upload_pack_data *data,
    ++			     const char *fmt, ...)
     +{
    -+	const char *key = list_object_filter_config_name(c);
    ++	struct strbuf buf = STRBUF_INIT;
    ++	va_list ap;
    ++
    ++	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;
    ++
     +	if (item)
    -+		return (intptr_t) item->util;
    -+	return data->allow_filter_fallback;
    ++		allowed = (intptr_t)item->util;
    ++	else
    ++		allowed = data->allow_filter_fallback;
    ++
    ++	if (!allowed)
    ++		send_err_and_die(data, "filter '%s' not supported", key);
     +}
     +
    -+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->choice))
    -+		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))
    -+				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;
    -+	if (!banned)
    ++	check_one_filter(data, opts);
    ++	if (opts->choice != LOFC_COMBINE)
     +		return;
     +
    -+	strbuf_addf(&buf, "git upload-pack: filter '%s' not supported",
    -+		    list_object_filter_config_name(banned->choice));
    ++	for (i = 0; i < opts->sub_nr; i++)
    ++		check_filter_recurse(data, &opts->sub[i]);
    ++}
     +
    -+	packet_writer_error(&data->writer, "%s\n", buf.buf);
    -+	die("%s", buf.buf);
    ++static void die_if_using_banned_filter(struct upload_pack_data *data)
    ++{
    ++	check_filter_recurse(data, &data->filter_options);
     +}
     +
      static void receive_needs(struct upload_pack_data *data,
3:  ad3f0cce56 < -:  ---------- upload-pack.c: pass 'struct list_objects_filter_options *'
4:  c9d71809f4 ! 8:  a0e7731a55 upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'
    @@ Commit message

         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 ##
    @@ Documentation/config/uploadpack.txt: uploadpackfilter.<filter>.allow::

      ## t/t5616-partial-clone.sh ##
     @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object filters with fallback' '
    - 	test_i18ngrep "filter '\''blob:none'\'' not supported" err
    + 	grep "filter '\''blob:none'\'' not supported" err
      '

     +test_expect_success 'upload-pack limits tree depth filters' '
    @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object f
     +	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
    ++	grep "tree filter allows max depth 0, but got 1" err
     +'
     +
      test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
    @@ upload-pack.c: static void upload_pack_data_init(struct upload_pack_data *data)
      	packet_writer_init(&data->writer, 1);

      	data->keepalive = 5;
    -@@ upload-pack.c: 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;
    +@@ upload-pack.c: static void check_one_filter(struct upload_pack_data *data,
    +
    + 	if (!allowed)
    + 		send_err_and_die(data, "filter '%s' not supported", key);
     +
    -+	if (allowed != 0 &&
    -+	    opts->choice == LOFC_TREE_DEPTH &&
    ++	if (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;
    ++		send_err_and_die(data,
    ++				 "tree filter allows max depth %lu, but got %lu",
    ++				 data->tree_filter_max_depth,
    ++				 opts->tree_exclude_depth);
      }

    -@@ upload-pack.c: 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);
    + static void check_filter_recurse(struct upload_pack_data *data,
     @@ upload-pack.c: 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 =
--
2.28.0.rc1.13.ge78abce653

[PATCH v4 2/3] upload-pack.c: allow banning certain object filter(s)

[Taylor Blau]

Git clients may ask the server for a partial set of objects, where the
set of objects being requested is refined by one or more object filters.
Server administrators can configure 'git upload-pack' to allow or ban
these filters by setting the 'uploadpack.allowFilter' variable to
'true' or 'false', respectively.

However, administrators using bitmaps may wish to allow certain kinds of
object filters, but ban others. Specifically, they may wish to allow
object filters that can be optimized by the use of bitmaps, while
rejecting other object filters which aren't and represent a perceived
performance degradation (as well as an increased load factor on the
server).

Allow configuring 'git upload-pack' to support object filters on a
case-by-case basis by introducing two new configuration variables:

  - 'uploadpackfilter.allow'
  - 'uploadpackfilter.<kind>.allow'

where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on.

Setting the second configuration variable for any valid value of
'<kind>' explicitly allows or disallows restricting that kind of object
filter.

If a client requests the object filter <kind> and the respective
configuration value is not set, 'git upload-pack' will default to the
value of 'uploadpackfilter.allow', which itself defaults to 'true' to
maintain backwards compatibility. Note that this differs from
'uploadpack.allowfilter', which controls whether or not the 'filter'
capability is advertised.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/uploadpack.txt | 12 ++++
 t/t5616-partial-clone.sh            | 24 ++++++++
 upload-pack.c                       | 86 +++++++++++++++++++++++++++++
 3 files changed, 122 insertions(+)

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index ed1c835695..fffe8ac648 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -57,6 +57,18 @@ uploadpack.allowFilter::
 	If this option is set, `upload-pack` will support partial
 	clone and partial fetch object filtering.
 
+uploadpackfilter.allow::
+	Provides a default value for unspecified object filters (see: the
+	below configuration variable).
+	Defaults to `true`.
+
+uploadpackfilter.<filter>.allow::
+	Explicitly allow or ban the object filter corresponding to
+	`<filter>`, where `<filter>` may be one of: `blob:none`,
+	`blob:limit`, `tree`, `sparse:oid`, or `combine`. If using
+	combined filters, both `combine` and all of the nested filter
+	kinds must be allowed. Defaults to `uploadpackfilter.allow`.
+
 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 37de0afb02..aa68782587 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -235,6 +235,30 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' '
 	test_cmp unique_types.expected unique_types.actual
 '
 
+test_expect_success 'upload-pack fails banned object filters' '
+	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
+		"file://$(pwd)/srv.bare" pc3 2>err &&
+	grep "filter '\''blob:none'\'' not supported" err
+'
+
+test_expect_success 'upload-pack fails banned combine object filters' '
+	test_config -C srv.bare uploadpackfilter.allow false &&
+	test_config -C srv.bare uploadpackfilter.combine.allow true &&
+	test_config -C srv.bare uploadpackfilter.tree.allow true &&
+	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
+		--filter=blob:none "file://$(pwd)/srv.bare" pc3 2>err &&
+	grep "filter '\''blob:none'\'' not supported" err
+'
+
+test_expect_success 'upload-pack fails banned object filters with fallback' '
+	test_config -C srv.bare uploadpackfilter.allow false &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
+		"file://$(pwd)/srv.bare" pc3 2>err &&
+	grep "filter '\''blob:none'\'' not supported" 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 8673741070..2098af8156 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -88,6 +88,7 @@ struct upload_pack_data {
 	enum allow_uor allow_uor;
 
 	struct list_objects_filter_options filter_options;
+	struct string_list allowed_filters;
 
 	struct packet_writer writer;
 
@@ -103,6 +104,7 @@ struct upload_pack_data {
 	unsigned no_progress : 1;
 	unsigned use_include_tag : 1;
 	unsigned allow_filter : 1;
+	unsigned allow_filter_fallback : 1;
 
 	unsigned done : 1;					/* v2 only */
 	unsigned allow_ref_in_want : 1;				/* v2 only */
@@ -120,6 +122,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
 	struct string_list uri_protocols = STRING_LIST_INIT_DUP;
 	struct object_array extra_edge_obj = OBJECT_ARRAY_INIT;
+	struct string_list allowed_filters = STRING_LIST_INIT_DUP;
 
 	memset(data, 0, sizeof(*data));
 	data->symref = symref;
@@ -131,6 +134,8 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->deepen_not = deepen_not;
 	data->uri_protocols = uri_protocols;
 	data->extra_edge_obj = extra_edge_obj;
+	data->allowed_filters = allowed_filters;
+	data->allow_filter_fallback = 1;
 	packet_writer_init(&data->writer, 1);
 
 	data->keepalive = 5;
@@ -147,6 +152,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)
 	string_list_clear(&data->deepen_not, 0);
 	object_array_clear(&data->extra_edge_obj);
 	list_objects_filter_release(&data->filter_options);
+	string_list_clear(&data->allowed_filters, 1);
 
 	free((char *)data->pack_objects_hook);
 }
@@ -984,6 +990,56 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,
 	return 0;
 }
 
+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;
+
+	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;
+
+	if (item)
+		allowed = (intptr_t)item->util;
+	else
+		allowed = data->allow_filter_fallback;
+
+	if (!allowed)
+		send_err_and_die(data, "filter '%s' not supported", key);
+}
+
+static void check_filter_recurse(struct upload_pack_data *data,
+				 struct list_objects_filter_options *opts)
+{
+	size_t i;
+
+	check_one_filter(data, opts);
+	if (opts->choice != LOFC_COMBINE)
+		return;
+
+	for (i = 0; i < opts->sub_nr; i++)
+		check_filter_recurse(data, &opts->sub[i]);
+}
+
+static void die_if_using_banned_filter(struct upload_pack_data *data)
+{
+	check_filter_recurse(data, &data->filter_options);
+}
+
 static void receive_needs(struct upload_pack_data *data,
 			  struct packet_reader *reader)
 {
@@ -1014,6 +1070,7 @@ static void receive_needs(struct upload_pack_data *data,
 				die("git upload-pack: filtering capability not negotiated");
 			list_objects_filter_die_if_populated(&data->filter_options);
 			parse_list_objects_filter(&data->filter_options, arg);
+			die_if_using_banned_filter(data);
 			continue;
 		}
 
@@ -1171,6 +1228,32 @@ static int find_symref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
+static int parse_object_filter_config(const char *var, const char *value,
+				       struct upload_pack_data *data)
+{
+	struct strbuf buf = STRBUF_INIT;
+	const char *sub, *key;
+	size_t sub_len;
+
+	if (parse_config_key(var, "uploadpackfilter", &sub, &sub_len, &key))
+		return 0;
+
+	if (!sub) {
+		if (!strcmp(key, "allow"))
+			data->allow_filter_fallback = git_config_bool(var, value);
+		return 0;
+	}
+
+	strbuf_add(&buf, sub, sub_len);
+
+	if (!strcmp(key, "allow"))
+		string_list_insert(&data->allowed_filters, buf.buf)->util =
+			(void *)(intptr_t)git_config_bool(var, value);
+
+	strbuf_release(&buf);
+	return 0;
+}
+
 static int upload_pack_config(const char *var, const char *value, void *cb_data)
 {
 	struct upload_pack_data *data = cb_data;
@@ -1210,6 +1293,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
 			return git_config_string(&data->pack_objects_hook, var, value);
 	}
 
+	parse_object_filter_config(var, value, data);
+
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
@@ -1390,6 +1475,7 @@ static void process_args(struct packet_reader *request,
 		if (data->allow_filter && skip_prefix(arg, "filter ", &p)) {
 			list_objects_filter_die_if_populated(&data->filter_options);
 			parse_list_objects_filter(&data->filter_options, p);
+			die_if_using_banned_filter(data);
 			continue;
 		}
 
-- 
2.28.0.rc1.13.ge78abce653

[PATCH v4 1/3] list_objects_filter_options: introduce 'list_object_filter_config_name'

[Taylor Blau]

In a subsequent commit, we will add configuration options that are
specific to each kind of object filter, in which case it is handy to
have a function that translates between 'enum
list_objects_filter_choice' and an appropriate configuration-friendly
string.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 list-objects-filter-options.c | 23 +++++++++++++++++++++++
 list-objects-filter-options.h |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 3553ad7b0a..92b408c0c8 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -15,6 +15,29 @@ static int parse_combine_filter(
 	const char *arg,
 	struct strbuf *errbuf);
 
+const char *list_object_filter_config_name(enum list_objects_filter_choice c)
+{
+	switch (c) {
+	case LOFC_DISABLED:
+		/* we have no name for "no filter at all" */
+		break;
+	case LOFC_BLOB_NONE:
+		return "blob:none";
+	case LOFC_BLOB_LIMIT:
+		return "blob:limit";
+	case LOFC_TREE_DEPTH:
+		return "tree";
+	case LOFC_SPARSE_OID:
+		return "sparse:oid";
+	case LOFC_COMBINE:
+		return "combine";
+	case LOFC__COUNT:
+		/* not a real filter type; just the count of all filters */
+		break;
+	}
+	BUG("list_object_filter_choice_name: invalid argument '%d'", c);
+}
+
 /*
  * Parse value of the argument to the "filter" keyword.
  * On the command line this looks like:
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index 73fffa4ad7..01767c3c96 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -17,6 +17,12 @@ enum list_objects_filter_choice {
 	LOFC__COUNT /* must be last */
 };
 
+/*
+ * Returns a configuration key suitable for describing the given object filter,
+ * e.g.: "blob:none", "combine", etc.
+ */
+const char *list_object_filter_config_name(enum list_objects_filter_choice c);
+
 struct list_objects_filter_options {
 	/*
 	 * 'filter_spec' is the raw argument value given on the command line
-- 
2.28.0.rc1.13.ge78abce653

[PATCH v4 3/3] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

[Taylor Blau]

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.

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

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 aa68782587..96ab45feab 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' '
 	grep "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 &&
+	grep "tree filter allows max 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 2098af8156..fbd2db04cd 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;
@@ -1020,6 +1022,13 @@ static void check_one_filter(struct upload_pack_data *data,
 
 	if (!allowed)
 		send_err_and_die(data, "filter '%s' not supported", key);
+
+	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 void check_filter_recurse(struct upload_pack_data *data,
@@ -1249,6 +1258,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;
-- 
2.28.0.rc1.13.ge78abce653

Re: [PATCH v4 0/3] upload-pack: custom allowed object filters

[Jeff King]

On Mon, Aug 03, 2020 at 02:00:04PM -0400, Taylor Blau wrote:

> Here's what I anticipate to be the final reroll of my series to teach
> the new 'uploadpackfilter' configuration section, which allows for more
> fine-grained control over which object filters upload-pack is willing to
> serve.

Thanks, this version looks good to me.

> Two changes from last time:
> 
>   - I adopted Peff's suggestion in beginning in [1], but appropriately
>     split it over the existing patch structure.

The adaptation looks good. The send_err_and_die() helper could possibly
be used for some of the packet_writer_error() callers, too.  Somebody
could convert them on top if they want, but I'm not sure it's even worth
it. IMHO the correct long-term direction is to convert those die() calls
into silent exits and let the ERR packet speak for itself. But we
shouldn't do that until the client side is more robust against
pipe-death races.

>   - I dropped the old patch 3/4, since it really should have never been
>     there in the first place, and just made the refactoring more noisy
>     than necessary.

Yeah, I think folding it in to the first patch as you did makes the
series simpler to follow.

-Peff