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

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

[Taylor Blau]

Hi,

Here are some patches that I sent a while ago [1] as an RFC, but are now
ready for an actual review. Not much has changed from the original
patches, but a few things have. Namely:

  - the 'uploadpack.filter.tree.maxDepth' configuration is new,

  - they are rebased forward on top of Christian's various upload-pack
    clean-ups, so they should apply cleanly to 'master' (in fact, they
    were rebased to the tip of master, not just Christian's various
    series), and

  - the patches have been substantially cleaned up and now have
    Signed-off-by trailer

As a reminder, these are the patches that we have been using at GitHub
to allow only certain kinds of object filters when serving partial
clones. If it is of interest, our configuration looks something like the
following:

  [uploadpack]
  	allowAnySHA1InWant = true
  	allowFilter = true
  [uploadpack "filter"]
  	allow = false
  [uploadpack "filter.blob:limit"]
  	allow = true
  [uploadpack "filter.blob:none"]
  	allow = true
  [uploadpack "filter.tree"]
  	allow = true
  	maxDepth = 0

Thanks in advance for your review (and thanks also to Peff, Christian,
Eric, and Junio for their help and discussion already).

[1]: https://lore.kernel.org/git/cover.1584477196.git.me@ttaylorr.com/

Thanks,
Taylor

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 'uploadpack.filter.tree.maxDepth'

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

--
2.27.0.225.g9fa765a71d

[PATCH 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.27.0.225.g9fa765a71d

[PATCH 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:

  - 'uploadpack.filter.allow'
  - 'uploadpack.filter.<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 'uploadpack.filter.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 | 16 ++++++
 t/t5616-partial-clone.sh            | 26 ++++++++++
 upload-pack.c                       | 78 +++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+)

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index ed1c835695..fd4970306c 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -57,6 +57,22 @@ uploadpack.allowFilter::
 	If this option is set, `upload-pack` will support partial
 	clone and partial fetch object filtering.
 
+uploadpack.filter.allow::
+	Provides a default value for unspecified object filters (see: the
+	below configuration variable).
+	Defaults to `true`.
+
+uploadpack.filter.<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`.
+
 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..5dcd0b5656 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -235,6 +235,32 @@ 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 case-insensitivity by intentional use of "blob:None" rather than
+	# "blob:none".
+	test_config -C srv.bare uploadpack.filter.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_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_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 39d0cf00be..a5f56d73cc 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,47 @@ 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;
+
+	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"));
+}
+
 static void receive_needs(struct upload_pack_data *data,
 			  struct packet_reader *reader)
 {
@@ -1013,6 +1060,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;
 		}
 
@@ -1169,6 +1217,33 @@ static int find_symref(const char *refname, const struct object_id *oid,
 	return 0;
 }
 
+static void parse_object_filter_config(const char *var, const char *value,
+				       struct upload_pack_data *data)
+{
+	struct strbuf spec = 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 (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
+			data->allow_filter_fallback = git_config_bool(var, value);
+	}
+
+	strbuf_release(&spec);
+}
+
 static int upload_pack_config(const char *var, const char *value, void *cb_data)
 {
 	struct upload_pack_data *data = cb_data;
@@ -1208,6 +1283,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");
 }
 
@@ -1388,6 +1465,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.27.0.225.g9fa765a71d

[PATCH 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 a5f56d73cc..a014ae23a9 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.27.0.225.g9fa765a71d

[PATCH 4/4] upload-pack.c: introduce 'uploadpack.filter.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 uploadpack.filter.tree.allow true

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

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

  $ git config uploadpack.filter.tree.maxDepth <m>

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

  $ git config uploadpack.filter.tree.allow true
  $ git config uploadpack.filter.tree.maxDepth 0

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

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

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

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index fd4970306c..3671b62e4c 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -73,6 +73,12 @@ Note that the dot between 'filter' and '<filter>' is both non-standard
 and intentional. This is done to avoid a parsing ambiguity when
 specifying this configuration as an argument to Git's top-level `-c`.
 
+uploadpack.filter.tree.maxDepth::
+	Only allow `--filter=tree=<n>` when `n` is no more than the value of
+	`uploadpack.filter.tree.maxDepth`. If set, this also implies
+	`uploadpack.filter.tree.allow=true`, unless this configuration
+	variable had already been set. Has no effect if unset.
+
 uploadpack.allowRefInWant::
 	If this option is set, `upload-pack` will support the `ref-in-want`
 	feature of the protocol version 2 `fetch` command.  This feature
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 5dcd0b5656..8781a24cfe 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -261,6 +261,14 @@ test_expect_success 'upload-pack fails banned object filters with fallback' '
 	test_i18ngrep "filter '\''blob:none'\'' not supported" err
 '
 
+test_expect_success 'upload-pack limits tree depth filters' '
+	test_config -C srv.bare uploadpack.filter.allow false &&
+	test_config -C srv.bare uploadpack.filter.tree.allow true &&
+	test_config -C srv.bare uploadpack.filter.tree.maxDepth 0 &&
+	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
+		"file://$(pwd)/srv.bare" pc3
+'
+
 test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '
 	rm -rf src dst &&
 	git init src &&
diff --git a/upload-pack.c b/upload-pack.c
index a014ae23a9..8db1745b86 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -105,6 +105,7 @@ struct upload_pack_data {
 	unsigned use_include_tag : 1;
 	unsigned allow_filter : 1;
 	unsigned allow_filter_fallback : 1;
+	unsigned long tree_filter_max_depth;
 
 	unsigned done : 1;					/* v2 only */
 	unsigned allow_ref_in_want : 1;				/* v2 only */
@@ -136,6 +137,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
 	data->extra_edge_obj = extra_edge_obj;
 	data->allowed_filters = allowed_filters;
 	data->allow_filter_fallback = 1;
+	data->tree_filter_max_depth = ULONG_MAX;
 	packet_writer_init(&data->writer, 1);
 
 	data->keepalive = 5;
@@ -995,8 +997,17 @@ static int allows_filter_choice(struct upload_pack_data *data,
 	const char *key = list_object_filter_config_name(opts->choice);
 	struct string_list_item *item = string_list_lookup(&data->allowed_filters,
 							   key);
+	int allowed = -1;
 	if (item)
-		return (intptr_t) item->util;
+		allowed = (intptr_t) item->util;
+
+	if (allowed != 0 &&
+	    opts->choice == LOFC_TREE_DEPTH &&
+	    opts->tree_exclude_depth > data->tree_filter_max_depth)
+		return 0;
+
+	if (allowed > -1)
+		return allowed;
 	return data->allow_filter_fallback;
 }
 
@@ -1022,11 +1033,22 @@ static void die_if_using_banned_filter(struct upload_pack_data *data)
 {
 	struct list_objects_filter_options *banned = banned_filter(data,
 								   &data->filter_options);
+	struct strbuf buf = STRBUF_INIT;
 	if (!banned)
 		return;
 
-	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
-			    list_object_filter_config_name(banned->choice));
+	strbuf_addf(&buf, _("filter '%s' not supported"),
+		    list_object_filter_config_name(banned->choice));
+	if (banned->choice == LOFC_TREE_DEPTH &&
+	    data->tree_filter_max_depth != ULONG_MAX)
+		strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"),
+			    data->tree_filter_max_depth,
+			    banned->tree_exclude_depth);
+
+	packet_writer_error(&data->writer, "%s\n", buf.buf);
+
+	strbuf_release(&buf);
+
 	die(_("git upload-pack: banned object filter requested"));
 }
 
@@ -1239,6 +1261,10 @@ static void parse_object_filter_config(const char *var, const char *value,
 				(void *)(intptr_t)git_config_bool(var, value);
 		else
 			data->allow_filter_fallback = git_config_bool(var, value);
+	} else if (!strcmp(spec.buf, "tree") && !strcmp(key, "maxdepth")) {
+		string_list_insert(&data->allowed_filters, "tree")->util
+			= (void *) (intptr_t) 1;
+		data->tree_filter_max_depth = git_config_ulong(var, value);
 	}
 
 	strbuf_release(&spec);
-- 
2.27.0.225.g9fa765a71d

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

[Jeff King]

On Thu, Jul 02, 2020 at 04:06:14PM -0400, Taylor Blau wrote:

>   [uploadpack]
>   	allowAnySHA1InWant = true
>   	allowFilter = true
>   [uploadpack "filter"]
>   	allow = false
>   [uploadpack "filter.blob:limit"]
>   	allow = true
>   [uploadpack "filter.blob:none"]
>   	allow = true
>   [uploadpack "filter.tree"]
>   	allow = true
>   	maxDepth = 0

I thought the previous discussion landed on:

  uploadpackfilter.blob:none.allow

etc, to avoid the confusing appearance of four-level keys (and the weird
case sensitivity implications that would cause).

-Peff

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

[Jeff King]

On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:

> +static void parse_object_filter_config(const char *var, const char *value,
> +				       struct upload_pack_data *data)
> +{
> +	struct strbuf spec = 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;

Just while I'm thinking about the config name and case-sensitivity (from
the cover letter): if we did want to use this scheme, then
skip_iprefix() would make this behave more like a regular part of the
section name.

But I'd prefer to just do away with it by using a scheme that doesn't
have the extra layer of dots.

> +	if (sub != key)
> +		strbuf_add(&spec, sub, key - sub - 1);
> +	strbuf_tolower(&spec);

On the flip side, I'd actually consider _not_ matching the filter name
case-insensitively. We don't do so elsewhere (e.g., "git rev-list
--filter=BLOB:NONE" will complain).

-Peff

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

[SZEDER Gábor]

On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 8a27452a51..5dcd0b5656 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -235,6 +235,32 @@ 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 case-insensitivity by intentional use of "blob:None" rather than
> +	# "blob:none".
> +	test_config -C srv.bare uploadpack.filter.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_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_must_fail git clone --no-checkout --filter=blob:none \
> +		"file://$(pwd)/srv.bare" pc3 2>err &&
> +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
> +'

These three tests are very flaky: 'git upload-pack' can error out
while clone is still sending packets (usually the 'done' line),
resulting in SIGPIPE and frequent CI failures.  Running this test
script with '-r 1,2,17-19 --stress' tends to fail in a couple of
seconds.

Using 'test_must_fail ok=sigpipe', as you did in the test in the last
patch, avoids the test failure caused by SIGPIPE, of course, but,
unfortunately, all three tests remain flaky, because the expected
error message sometimes doesn't make it to 'git clone's stderr, e.g.:

  expecting success of 5616.19 'upload-pack fails banned object filters with fallback': 
          test_config -C srv.bare uploadpack.filter.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_config -C srv.bare uploadpack.filter.allow false
  + pwd
  + test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none file:///home/szeder/src/git/t/trash directory.t5616-partial-clone.stress-2/srv.bare pc3
  + test_i18ngrep filter 'blob:none' not supported err
  error: 'grep filter 'blob:none' not supported err' didn't find a match in:
  Cloning into 'pc3'...
  fatal: git upload-pack: banned object filter requested
  error: last command exited with $?=1
  not ok 19 - upload-pack fails banned object filters with fallback


Once upon a time I had a PoC patch to deal with 'git upload-pack'
aborting while 'git fetch' is still send_request()-ing, by catching
the write error to the closed connection and trying read any pending
ERR packets; Christian cleaned it up and submitted it with a proper
commit message in

  https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/

but it haven't been picked up yet.  Disappointingly, that patch
doesn't solve these issues...  I haven't looked what's going on
(perhaps 'git clone' does something differently than 'git fetch'?  no
idea)

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

[SZEDER Gábor]

On Thu, Jul 02, 2020 at 04:06:40PM -0400, Taylor Blau wrote:
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 5dcd0b5656..8781a24cfe 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -261,6 +261,14 @@ test_expect_success 'upload-pack fails banned object filters with fallback' '
>  	test_i18ngrep "filter '\''blob:none'\'' not supported" err
>  '
>  
> +test_expect_success 'upload-pack limits tree depth filters' '
> +	test_config -C srv.bare uploadpack.filter.allow false &&
> +	test_config -C srv.bare uploadpack.filter.tree.allow true &&
> +	test_config -C srv.bare uploadpack.filter.tree.maxDepth 0 &&
> +	test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
> +		"file://$(pwd)/srv.bare" pc3
> +'

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

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

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

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

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

[Jeff King]

On Wed, Jul 15, 2020 at 12:00:43PM +0200, SZEDER Gábor wrote:

> Once upon a time I had a PoC patch to deal with 'git upload-pack'
> aborting while 'git fetch' is still send_request()-ing, by catching
> the write error to the closed connection and trying read any pending
> ERR packets; Christian cleaned it up and submitted it with a proper
> commit message in
> 
>   https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/
> 
> but it haven't been picked up yet.  Disappointingly, that patch
> doesn't solve these issues...  I haven't looked what's going on
> (perhaps 'git clone' does something differently than 'git fetch'?  no
> idea)

I suspect it is that fetch ignores SIGPIPE, but clone does not. So even
when we see a 141 exit code from fetch, it is probably generated
synthetically from exit(141) after we saw EPIPE. And your patch works
there because we have a chance to pump the read-side of the pipe,
whereas in git-clone we die immediately via the signal.

Probably git-clone should ignore SIGPIPE during the network transfer
portion of the process for the same reasons given in 143588949c (fetch:
ignore SIGPIPE during network operation, 2019-03-03).

-Peff

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

[Taylor Blau]

On Wed, Jul 08, 2020 at 04:45:27AM -0400, Jeff King wrote:
> On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
>
> > +static void parse_object_filter_config(const char *var, const char *value,
> > +				       struct upload_pack_data *data)
> > +{
> > +	struct strbuf spec = 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;
>
> Just while I'm thinking about the config name and case-sensitivity (from
> the cover letter): if we did want to use this scheme, then
> skip_iprefix() would make this behave more like a regular part of the
> section name.
>
> But I'd prefer to just do away with it by using a scheme that doesn't
> have the extra layer of dots.

Yeah. I definitely flip-flopped on this when preparing this for the
list. I still feel like 'uploadpackfilter' is gross, so I was hoping to
send it with 'uploadpack.filter' (which reads nicely, but forces us to
write some gross code), but both options are frustrating to me for
different reasons.

Playing around with it more, though, I think that uploadpackfilter is
our best bet. I'd hate to introduce a string manipulation bug by munging
the reuslt of 'parse_config_key()', which is so clearly designed for
something like 'uploadpackfilter[.<filter>].allow'.

> > +	if (sub != key)
> > +		strbuf_add(&spec, sub, key - sub - 1);
> > +	strbuf_tolower(&spec);
>
> On the flip side, I'd actually consider _not_ matching the filter name
> case-insensitively. We don't do so elsewhere (e.g., "git rev-list
> --filter=BLOB:NONE" will complain).

I dropped the case-insensitive matching in the latest revision. I don't
think that we need to be overly accommodating here.

> -Peff

Thanks,
Taylor

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

[Taylor Blau]

On Wed, Jul 15, 2020 at 06:55:21AM -0400, Jeff King wrote:
> On Wed, Jul 15, 2020 at 12:00:43PM +0200, SZEDER Gábor wrote:
>
> > Once upon a time I had a PoC patch to deal with 'git upload-pack'
> > aborting while 'git fetch' is still send_request()-ing, by catching
> > the write error to the closed connection and trying read any pending
> > ERR packets; Christian cleaned it up and submitted it with a proper
> > commit message in
> >
> >   https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/
> >
> > but it haven't been picked up yet.  Disappointingly, that patch
> > doesn't solve these issues...  I haven't looked what's going on
> > (perhaps 'git clone' does something differently than 'git fetch'?  no
> > idea)
>
> I suspect it is that fetch ignores SIGPIPE, but clone does not. So even
> when we see a 141 exit code from fetch, it is probably generated
> synthetically from exit(141) after we saw EPIPE. And your patch works
> there because we have a chance to pump the read-side of the pipe,
> whereas in git-clone we die immediately via the signal.

Heh. I was hoping to be rid of those errors with Christian's patches,
but it sounds like the problem is coming from outside of 'upload-pack'
and instead in 'clone'.

That reasoning seems sound to me, but I'd rather not touch clone in this
patch series if I don't have to. What I'd rather do is something like:

  - Introduce this patch series with the 'test_must_fail ok=sigpipe',
    and no error checking.

  - Modify clone to swallow these errors and eat a packet or two.

  - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little
    bit smarter here.

Maybe more steps than is strictly necessary, but I think it keeps the
scope of the review on this series reasonable, which is a tradeoff that
I'm willing to make.

> Probably git-clone should ignore SIGPIPE during the network transfer
> portion of the process for the same reasons given in 143588949c (fetch:
> ignore SIGPIPE during network operation, 2019-03-03).
>
> -Peff

Thanks,
Taylor

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

[Taylor Blau]

On Wed, Jul 08, 2020 at 04:41:36AM -0400, Jeff King wrote:
> On Thu, Jul 02, 2020 at 04:06:14PM -0400, Taylor Blau wrote:
>
> >   [uploadpack]
> >   	allowAnySHA1InWant = true
> >   	allowFilter = true
> >   [uploadpack "filter"]
> >   	allow = false
> >   [uploadpack "filter.blob:limit"]
> >   	allow = true
> >   [uploadpack "filter.blob:none"]
> >   	allow = true
> >   [uploadpack "filter.tree"]
> >   	allow = true
> >   	maxDepth = 0
>
> I thought the previous discussion landed on:
>
>   uploadpackfilter.blob:none.allow
>
> etc, to avoid the confusing appearance of four-level keys (and the weird
> case sensitivity implications that would cause).

Yup, I agree. I added a little bit more detail to this response in
another email that I posted shortly above in this thread.

Rest assured that I haven't forgotten about this series, I've just been
busy and not feeling too pressured to get back to it since we're well
into the release-candidate phase, and shouldn't be worrying about this
until after 2.28.0.

If you're interested, I have what I will eventually call 'v2' of this
series at:

  https://github.com/ttaylorr/git/tree/tb/upload-pack-filter-config

> -Peff

Thanks,
Taylor

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

[Jeff King]

On Mon, Jul 20, 2020 at 04:07:39PM -0400, Taylor Blau wrote:

> Heh. I was hoping to be rid of those errors with Christian's patches,
> but it sounds like the problem is coming from outside of 'upload-pack'
> and instead in 'clone'.

Yes, it's definitely on the client side.

> That reasoning seems sound to me, but I'd rather not touch clone in this
> patch series if I don't have to. What I'd rather do is something like:
> 
>   - Introduce this patch series with the 'test_must_fail ok=sigpipe',
>     and no error checking.
> 
>   - Modify clone to swallow these errors and eat a packet or two.
> 
>   - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little
>     bit smarter here.
> 
> Maybe more steps than is strictly necessary, but I think it keeps the
> scope of the review on this series reasonable, which is a tradeoff that
> I'm willing to make.

I think that's reasonable.

It doesn't look like the "pump the read side after EPIPE" patch is even
on the "seen" branch yet, and you'd need to further extend it to clone.
It's definitely a big enough change that it's better not to lump it in
with this series.

-Peff

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> 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 'uploadpack.filter.tree.maxDepth'
>
>  Documentation/config/uploadpack.txt |  22 ++++++
>  list-objects-filter-options.c       |  23 ++++++
>  list-objects-filter-options.h       |   6 ++
>  t/t5616-partial-clone.sh            |  34 +++++++++
>  upload-pack.c                       | 104 ++++++++++++++++++++++++++++
>  5 files changed, 189 insertions(+)
>
> --
> 2.27.0.225.g9fa765a71d

With this series (I do not know which one of them is the culprit) in
the 'seen' branch, we seem to consistently get a segfault while
running t5616 on macOS clang build [*1*]

[Footnote]

*1* https://travis-ci.org/github/git/git/jobs/710504820 has the
    topic at the tip of 'seen' that fails.  Without that merge,
    https://travis-ci.org/github/git/git/jobs/710342598 seems to
    pass on all archs.

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

[Taylor Blau]

On Tue, Jul 21, 2020 at 01:06:39PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > 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 'uploadpack.filter.tree.maxDepth'
> >
> >  Documentation/config/uploadpack.txt |  22 ++++++
> >  list-objects-filter-options.c       |  23 ++++++
> >  list-objects-filter-options.h       |   6 ++
> >  t/t5616-partial-clone.sh            |  34 +++++++++
> >  upload-pack.c                       | 104 ++++++++++++++++++++++++++++
> >  5 files changed, 189 insertions(+)
> >
> > --
> > 2.27.0.225.g9fa765a71d
>
> With this series (I do not know which one of them is the culprit) in
> the 'seen' branch, we seem to consistently get a segfault while
> running t5616 on macOS clang build [*1*]

Aye. Am I reading that correctly that it's git clone dying with a
SIGPIPE instead of a segfault? If so, this is what Szeder pointed out a
little lower in the thread (tl;dr is that 'git clone' is not resilient
to this whereas 'git fetch' is, and so we still need an `ok=sigpipe`
somewhere in t5616).

The new version that I'm preparing has these appropriately, so feel
free to discard this until I send a new version your way...

> [Footnote]
>
> *1* https://travis-ci.org/github/git/git/jobs/710504820 has the
>     topic at the tip of 'seen' that fails.  Without that merge,
>     https://travis-ci.org/github/git/git/jobs/710342598 seems to
>     pass on all archs.

Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Aye. Am I reading that correctly that it's git clone dying with a
> SIGPIPE instead of a segfault? If so, this is what Szeder pointed out a
> little lower in the thread (tl;dr is that 'git clone' is not resilient
> to this whereas 'git fetch' is, and so we still need an `ok=sigpipe`
> somewhere in t5616).
>
> The new version that I'm preparing has these appropriately, so feel
> free to discard this until I send a new version your way...

If it is known and under control, I have nothing to worry about ;-)

Thanks.

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

[SZEDER Gábor]

On Mon, Jul 20, 2020 at 04:07:39PM -0400, Taylor Blau wrote:
> What I'd rather do is something like:
> 
>   - Introduce this patch series with the 'test_must_fail ok=sigpipe',
>     and no error checking.

die_if_using_banned_filter() shows two error messages: a "fancy" one
is sent to the client in the ERR packet, including which particular
filter is not supported/allowed, and a simple 

  die(_("git upload-pack: banned object filter requested"));

If this die() were to show the same fancy error message as in the ERR
packet, then it would always make it to 'git clone's stderr in the
tests, so the tests could reliably check that 'git upload-pack' died
for the expected reason.

>   - Modify clone to swallow these errors and eat a packet or two.
> 
>   - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little
>     bit smarter here.
> 
> Maybe more steps than is strictly necessary, but I think it keeps the
> scope of the review on this series reasonable, which is a tradeoff that
> I'm willing to make.
> 
> > Probably git-clone should ignore SIGPIPE during the network transfer
> > portion of the process for the same reasons given in 143588949c (fetch:
> > ignore SIGPIPE during network operation, 2019-03-03).
> >
> > -Peff
> 
> Thanks,
> Taylor

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

[SZEDER Gábor]

On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> +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;
> +
> +	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),

Should we really translate the content of the ERR packet?

> +			    list_object_filter_config_name(banned->choice));
> +	die(_("git upload-pack: banned object filter requested"));
> +}
> +
>  static void receive_needs(struct upload_pack_data *data,
>  			  struct packet_reader *reader)
>  {

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

[Taylor Blau]

On Wed, Jul 22, 2020 at 11:17:58AM +0200, SZEDER Gábor wrote:
> On Mon, Jul 20, 2020 at 04:07:39PM -0400, Taylor Blau wrote:
> > What I'd rather do is something like:
> >
> >   - Introduce this patch series with the 'test_must_fail ok=sigpipe',
> >     and no error checking.
>
> die_if_using_banned_filter() shows two error messages: a "fancy" one
> is sent to the client in the ERR packet, including which particular
> filter is not supported/allowed, and a simple
>
>   die(_("git upload-pack: banned object filter requested"));
>
> If this die() were to show the same fancy error message as in the ERR
> packet, then it would always make it to 'git clone's stderr in the
> tests, so the tests could reliably check that 'git upload-pack' died
> for the expected reason.

Beautiful idea. I changed this in my fork, and I'll send it to this
thread after 2.28 is out, since I don't want to create a distraction in
the meantime.

> >   - Modify clone to swallow these errors and eat a packet or two.
> >
> >   - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little
> >     bit smarter here.
> >
> > Maybe more steps than is strictly necessary, but I think it keeps the
> > scope of the review on this series reasonable, which is a tradeoff that
> > I'm willing to make.
> >
> > > Probably git-clone should ignore SIGPIPE during the network transfer
> > > portion of the process for the same reasons given in 143588949c (fetch:
> > > ignore SIGPIPE during network operation, 2019-03-03).
> > >
> > > -Peff
> >
> > Thanks,
> > Taylor

Thanks,
Taylor

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

[Taylor Blau]

On Wed, Jul 22, 2020 at 11:21:29AM +0200, SZEDER Gábor wrote:
> On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> > +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;
> > +
> > +	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
>
> Should we really translate the content of the ERR packet?

s/ERR packet/dying message after your other piece of advice, but I think
so. This is eventually going to make its way to users, so translating it
seems to make sense to me.

I guess they would get it in the localization of the server, not the
client, but that doesn't sway me away from this, either.

> > +			    list_object_filter_config_name(banned->choice));
> > +	die(_("git upload-pack: banned object filter requested"));
> > +}
> > +
> >  static void receive_needs(struct upload_pack_data *data,
> >  			  struct packet_reader *reader)
> >  {

Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> If this die() were to show the same fancy error message as in the ERR
>> packet, then it would always make it to 'git clone's stderr in the
>> tests, so the tests could reliably check that 'git upload-pack' died
>> for the expected reason.
>
> Beautiful idea. I changed this in my fork, and I'll send it to this
> thread after 2.28 is out, since I don't want to create a distraction in
> the meantime.

Thanks.  

As long as everybody understands that "distraction" will immediately
gets backburnered when a regression relevant to the upcoming release
is found, however, I think performing work as usual is a good thing.
After all, we often find glitches in the current code by trying to
build on it, in addition to using it.

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

[Taylor Blau]

On Wed, Jul 22, 2020 at 06:41:33PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >> If this die() were to show the same fancy error message as in the ERR
> >> packet, then it would always make it to 'git clone's stderr in the
> >> tests, so the tests could reliably check that 'git upload-pack' died
> >> for the expected reason.
> >
> > Beautiful idea. I changed this in my fork, and I'll send it to this
> > thread after 2.28 is out, since I don't want to create a distraction in
> > the meantime.
>
> Thanks.
>
> As long as everybody understands that "distraction" will immediately
> gets backburnered when a regression relevant to the upcoming release
> is found, however, I think performing work as usual is a good thing.
> After all, we often find glitches in the current code by trying to
> build on it, in addition to using it.

Fair enough ;-). I sent v2 a few minutes ago, but forgot to attach it to
this thread as a reply. So, I suppose we can pick up the review there,
instead.

Thanks,
Taylor

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

[SZEDER Gábor]

On Wed, Jul 22, 2020 at 04:16:34PM -0400, Taylor Blau wrote:
> On Wed, Jul 22, 2020 at 11:21:29AM +0200, SZEDER Gábor wrote:
> > On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> > > +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;
> > > +
> > > +	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
> >
> > Should we really translate the content of the ERR packet?
> 
> s/ERR packet/dying message after your other piece of advice, but I think
> so. This is eventually going to make its way to users, so translating it
> seems to make sense to me.
> 
> I guess they would get it in the localization of the server, not the
> client, but that doesn't sway me away from this, either.

Please note that the contents of other ERR packets are not translated,
either.

$ git grep -A1 -h packet_writer_error origin/seen:upload-pack.c
			packet_writer_error(&data->writer,
					    "upload-pack: not our ref %s",
--
			packet_writer_error(&data->writer,
					    "upload-pack: not our ref %s",
--
			packet_writer_error(writer,
					    "upload-pack: not our ref %s",
--
			packet_writer_error(writer, "unknown ref %s", arg);
			die("unknown ref %s", arg);


> > > +			    list_object_filter_config_name(banned->choice));
> > > +	die(_("git upload-pack: banned object filter requested"));
> > > +}
> > > +
> > >  static void receive_needs(struct upload_pack_data *data,
> > >  			  struct packet_reader *reader)
> > >  {
> 
> Thanks,
> Taylor

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

[Taylor Blau]

On Thu, Jul 23, 2020 at 09:51:44AM +0200, SZEDER Gábor wrote:
> On Wed, Jul 22, 2020 at 04:16:34PM -0400, Taylor Blau wrote:
> > On Wed, Jul 22, 2020 at 11:21:29AM +0200, SZEDER Gábor wrote:
> > > On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> > > > +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;
> > > > +
> > > > +	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
> > >
> > > Should we really translate the content of the ERR packet?
> >
> > s/ERR packet/dying message after your other piece of advice, but I think
> > so. This is eventually going to make its way to users, so translating it
> > seems to make sense to me.
> >
> > I guess they would get it in the localization of the server, not the
> > client, but that doesn't sway me away from this, either.
>
> Please note that the contents of other ERR packets are not translated,
> either.
>
> $ git grep -A1 -h packet_writer_error origin/seen:upload-pack.c
> 			packet_writer_error(&data->writer,
> 					    "upload-pack: not our ref %s",
> --
> 			packet_writer_error(&data->writer,
> 					    "upload-pack: not our ref %s",
> --
> 			packet_writer_error(writer,
> 					    "upload-pack: not our ref %s",
> --
> 			packet_writer_error(writer, "unknown ref %s", arg);
> 			die("unknown ref %s", arg);

Fair enough... if there are other changes to make I'll drop the
translation, otherwise this should be easy enough to apply when queuing.

Junio, if you'd rather I reroll the series again, too, that's fine as
well.

>
> > > > +			    list_object_filter_config_name(banned->choice));
> > > > +	die(_("git upload-pack: banned object filter requested"));
> > > > +}
> > > > +
> > > >  static void receive_needs(struct upload_pack_data *data,
> > > >  			  struct packet_reader *reader)
> > > >  {
> >
> > Thanks,
> > Taylor

Thanks,
Taylor