{"thread":{"id":"53792","subject":"[PATCH 0/4] upload-pack: custom allowed object filters","startedAt":"2020-07-02T20:06:20Z","lastAt":"2020-07-23T14:13:11Z","messageCount":25,"participants":["Taylor Blau","Jeff King","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"400982","messageId":"cover.1593720075.git.me@ttaylorr.com","threadId":"53792","inReplyTo":null,"subject":"[PATCH 0/4] upload-pack: custom allowed object filters","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-02T20:06:14Z","receivedAt":"2020-07-02T20:06:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere are some patches that I sent a while ago [1] as an RFC, but are now\nready for an actual review. Not much has changed from the original\npatches, but a few things have. Namely:\n\n  - the 'uploadpack.filter.tree.maxDepth' configuration is new,\n\n  - they are rebased forward on top of Christian's various upload-pack\n    clean-ups, so they should apply cleanly to 'master' (in fact, they\n    were rebased to the tip of master, not just Christian's various\n    series), and\n\n  - the patches have been substantially cleaned up and now have\n    Signed-off-by trailer\n\nAs a reminder, these are the patches that we have been using at GitHub\nto allow only certain kinds of object filters when serving partial\nclones. If it is of interest, our configuration looks something like the\nfollowing:\n\n  [uploadpack]\n  \tallowAnySHA1InWant = true\n  \tallowFilter = true\n  [uploadpack \"filter\"]\n  \tallow = false\n  [uploadpack \"filter.blob:limit\"]\n  \tallow = true\n  [uploadpack \"filter.blob:none\"]\n  \tallow = true\n  [uploadpack \"filter.tree\"]\n  \tallow = true\n  \tmaxDepth = 0\n\nThanks in advance for your review (and thanks also to Peff, Christian,\nEric, and Junio for their help and discussion already).\n\n[1]: https://lore.kernel.org/git/cover.1584477196.git.me@ttaylorr.com/\n\nThanks,\nTaylor\n\nTaylor Blau (4):\n  list_objects_filter_options: introduce 'list_object_filter_config_name'\n  upload-pack.c: allow banning certain object filter(s)\n  upload-pack.c: pass 'struct list_objects_filter_options *'\n  upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'\n\n Documentation/config/uploadpack.txt |  22 ++++++\n list-objects-filter-options.c       |  23 ++++++\n list-objects-filter-options.h       |   6 ++\n t/t5616-partial-clone.sh            |  34 +++++++++\n upload-pack.c                       | 104 ++++++++++++++++++++++++++++\n 5 files changed, 189 insertions(+)\n\n--\n2.27.0.225.g9fa765a71d\n"},{"id":"400983","messageId":"738d97be4645435c333111a06560926a701a5bea.1593720075.git.me@ttaylorr.com","threadId":"53792","inReplyTo":"cover.1593720075.git.me@ttaylorr.com","subject":"[PATCH 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-02T20:06:21Z","receivedAt":"2020-07-02T20:06:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a subsequent commit, we will add configuration options that are\nspecific to each kind of object filter, in which case it is handy to\nhave a function that translates between 'enum\nlist_objects_filter_choice' and an appropriate configuration-friendly\nstring.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n list-objects-filter-options.c | 23 +++++++++++++++++++++++\n list-objects-filter-options.h |  6 ++++++\n 2 files changed, 29 insertions(+)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 3553ad7b0a..92b408c0c8 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -15,6 +15,29 @@ static int parse_combine_filter(\n \tconst char *arg,\n \tstruct strbuf *errbuf);\n \n+const char *list_object_filter_config_name(enum list_objects_filter_choice c)\n+{\n+\tswitch (c) {\n+\tcase LOFC_DISABLED:\n+\t\t/* we have no name for \"no filter at all\" */\n+\t\tbreak;\n+\tcase LOFC_BLOB_NONE:\n+\t\treturn \"blob:none\";\n+\tcase LOFC_BLOB_LIMIT:\n+\t\treturn \"blob:limit\";\n+\tcase LOFC_TREE_DEPTH:\n+\t\treturn \"tree\";\n+\tcase LOFC_SPARSE_OID:\n+\t\treturn \"sparse:oid\";\n+\tcase LOFC_COMBINE:\n+\t\treturn \"combine\";\n+\tcase LOFC__COUNT:\n+\t\t/* not a real filter type; just the count of all filters */\n+\t\tbreak;\n+\t}\n+\tBUG(\"list_object_filter_choice_name: invalid argument '%d'\", c);\n+}\n+\n /*\n  * Parse value of the argument to the \"filter\" keyword.\n  * On the command line this looks like:\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex 73fffa4ad7..01767c3c96 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -17,6 +17,12 @@ enum list_objects_filter_choice {\n \tLOFC__COUNT /* must be last */\n };\n \n+/*\n+ * Returns a configuration key suitable for describing the given object filter,\n+ * e.g.: \"blob:none\", \"combine\", etc.\n+ */\n+const char *list_object_filter_config_name(enum list_objects_filter_choice c);\n+\n struct list_objects_filter_options {\n \t/*\n \t * 'filter_spec' is the raw argument value given on the command line\n-- \n2.27.0.225.g9fa765a71d\n\n"},{"id":"400984","messageId":"f0982d24e74155f6c0e405e5e3ae8c3e579f798a.1593720075.git.me@ttaylorr.com","threadId":"53792","inReplyTo":"cover.1593720075.git.me@ttaylorr.com","subject":"[PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-02T20:06:32Z","receivedAt":"2020-07-02T20:06:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Git clients may ask the server for a partial set of objects, where the\nset of objects being requested is refined by one or more object filters.\nServer administrators can configure 'git upload-pack' to allow or ban\nthese filters by setting the 'uploadpack.allowFilter' variable to\n'true' or 'false', respectively.\n\nHowever, administrators using bitmaps may wish to allow certain kinds of\nobject filters, but ban others. Specifically, they may wish to allow\nobject filters that can be optimized by the use of bitmaps, while\nrejecting other object filters which aren't and represent a perceived\nperformance degradation (as well as an increased load factor on the\nserver).\n\nAllow configuring 'git upload-pack' to support object filters on a\ncase-by-case basis by introducing two new configuration variables:\n\n  - 'uploadpack.filter.allow'\n  - 'uploadpack.filter.<kind>.allow'\n\nwhere '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on.\n\nSetting the second configuration variable for any valid value of\n'<kind>' explicitly allows or disallows restricting that kind of object\nfilter.\n\nIf a client requests the object filter <kind> and the respective\nconfiguration value is not set, 'git upload-pack' will default to the\nvalue of 'uploadpack.filter.allow', which itself defaults to 'true' to\nmaintain backwards compatibility. Note that this differs from\n'uploadpack.allowfilter', which controls whether or not the 'filter'\ncapability is advertised.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/uploadpack.txt | 16 ++++++\n t/t5616-partial-clone.sh            | 26 ++++++++++\n upload-pack.c                       | 78 +++++++++++++++++++++++++++++\n 3 files changed, 120 insertions(+)\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex ed1c835695..fd4970306c 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -57,6 +57,22 @@ uploadpack.allowFilter::\n \tIf this option is set, `upload-pack` will support partial\n \tclone and partial fetch object filtering.\n \n+uploadpack.filter.allow::\n+\tProvides a default value for unspecified object filters (see: the\n+\tbelow configuration variable).\n+\tDefaults to `true`.\n+\n+uploadpack.filter.<filter>.allow::\n+\tExplicitly allow or ban the object filter corresponding to\n+\t`<filter>`, where `<filter>` may be one of: `blob:none`,\n+\t`blob:limit`, `tree`, `sparse:oid`, or `combine`. If using\n+\tcombined filters, both `combine` and all of the nested filter\n+\tkinds must be allowed.  Defaults to `uploadpack.filter.allow`.\n++\n+Note that the dot between 'filter' and '<filter>' is both non-standard\n+and intentional. This is done to avoid a parsing ambiguity when\n+specifying this configuration as an argument to Git's top-level `-c`.\n+\n uploadpack.allowRefInWant::\n \tIf this option is set, `upload-pack` will support the `ref-in-want`\n \tfeature of the protocol version 2 `fetch` command.  This feature\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 8a27452a51..5dcd0b5656 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -235,6 +235,32 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' '\n \ttest_cmp unique_types.expected unique_types.actual\n '\n \n+test_expect_success 'upload-pack fails banned object filters' '\n+\t# Test case-insensitivity by intentional use of \"blob:None\" rather than\n+\t# \"blob:none\".\n+\ttest_config -C srv.bare uploadpack.filter.blob:None.allow false &&\n+\ttest_must_fail git clone --no-checkout --filter=blob:none \\\n+\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n+\ttest_i18ngrep \"filter '\\''blob:none'\\'' not supported\" err\n+'\n+\n+test_expect_success 'upload-pack fails banned combine object filters' '\n+\ttest_config -C srv.bare uploadpack.filter.allow false &&\n+\ttest_config -C srv.bare uploadpack.filter.combine.allow true &&\n+\ttest_config -C srv.bare uploadpack.filter.tree.allow true &&\n+\ttest_config -C srv.bare uploadpack.filter.blob:none.allow false &&\n+\ttest_must_fail git clone --no-checkout --filter=tree:1 \\\n+\t\t--filter=blob:none \"file://$(pwd)/srv.bare\" pc3 2>err &&\n+\ttest_i18ngrep \"filter '\\''blob:none'\\'' not supported\" err\n+'\n+\n+test_expect_success 'upload-pack fails banned object filters with fallback' '\n+\ttest_config -C srv.bare uploadpack.filter.allow false &&\n+\ttest_must_fail git clone --no-checkout --filter=blob:none \\\n+\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n+\ttest_i18ngrep \"filter '\\''blob:none'\\'' not supported\" err\n+'\n+\n test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '\n \trm -rf src dst &&\n \tgit init src &&\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 39d0cf00be..a5f56d73cc 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -88,6 +88,7 @@ struct upload_pack_data {\n \tenum allow_uor allow_uor;\n \n \tstruct list_objects_filter_options filter_options;\n+\tstruct string_list allowed_filters;\n \n \tstruct packet_writer writer;\n \n@@ -103,6 +104,7 @@ struct upload_pack_data {\n \tunsigned no_progress : 1;\n \tunsigned use_include_tag : 1;\n \tunsigned allow_filter : 1;\n+\tunsigned allow_filter_fallback : 1;\n \n \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n@@ -120,6 +122,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n \tstruct string_list uri_protocols = STRING_LIST_INIT_DUP;\n \tstruct object_array extra_edge_obj = OBJECT_ARRAY_INIT;\n+\tstruct string_list allowed_filters = STRING_LIST_INIT_DUP;\n \n \tmemset(data, 0, sizeof(*data));\n \tdata->symref = symref;\n@@ -131,6 +134,8 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->deepen_not = deepen_not;\n \tdata->uri_protocols = uri_protocols;\n \tdata->extra_edge_obj = extra_edge_obj;\n+\tdata->allowed_filters = allowed_filters;\n+\tdata->allow_filter_fallback = 1;\n \tpacket_writer_init(&data->writer, 1);\n \n \tdata->keepalive = 5;\n@@ -147,6 +152,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \tstring_list_clear(&data->deepen_not, 0);\n \tobject_array_clear(&data->extra_edge_obj);\n \tlist_objects_filter_release(&data->filter_options);\n+\tstring_list_clear(&data->allowed_filters, 1);\n \n \tfree((char *)data->pack_objects_hook);\n }\n@@ -983,6 +989,47 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,\n \treturn 0;\n }\n \n+static int allows_filter_choice(struct upload_pack_data *data,\n+\t\t\t\tenum list_objects_filter_choice c)\n+{\n+\tconst char *key = list_object_filter_config_name(c);\n+\tstruct string_list_item *item = string_list_lookup(&data->allowed_filters,\n+\t\t\t\t\t\t\t   key);\n+\tif (item)\n+\t\treturn (intptr_t) item->util;\n+\treturn data->allow_filter_fallback;\n+}\n+\n+static struct list_objects_filter_options *banned_filter(\n+\tstruct upload_pack_data *data,\n+\tstruct list_objects_filter_options *opts)\n+{\n+\tsize_t i;\n+\n+\tif (!allows_filter_choice(data, opts->choice))\n+\t\treturn opts;\n+\n+\tif (opts->choice == LOFC_COMBINE)\n+\t\tfor (i = 0; i < opts->sub_nr; i++) {\n+\t\t\tstruct list_objects_filter_options *sub = &opts->sub[i];\n+\t\t\tif (banned_filter(data, sub))\n+\t\t\t\treturn sub;\n+\t\t}\n+\treturn NULL;\n+}\n+\n+static void die_if_using_banned_filter(struct upload_pack_data *data)\n+{\n+\tstruct list_objects_filter_options *banned = banned_filter(data,\n+\t\t\t\t\t\t\t\t   &data->filter_options);\n+\tif (!banned)\n+\t\treturn;\n+\n+\tpacket_writer_error(&data->writer, _(\"filter '%s' not supported\\n\"),\n+\t\t\t    list_object_filter_config_name(banned->choice));\n+\tdie(_(\"git upload-pack: banned object filter requested\"));\n+}\n+\n static void receive_needs(struct upload_pack_data *data,\n \t\t\t  struct packet_reader *reader)\n {\n@@ -1013,6 +1060,7 @@ static void receive_needs(struct upload_pack_data *data,\n \t\t\t\tdie(\"git upload-pack: filtering capability not negotiated\");\n \t\t\tlist_objects_filter_die_if_populated(&data->filter_options);\n \t\t\tparse_list_objects_filter(&data->filter_options, arg);\n+\t\t\tdie_if_using_banned_filter(data);\n \t\t\tcontinue;\n \t\t}\n \n@@ -1169,6 +1217,33 @@ static int find_symref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n+static void parse_object_filter_config(const char *var, const char *value,\n+\t\t\t\t       struct upload_pack_data *data)\n+{\n+\tstruct strbuf spec = STRBUF_INIT;\n+\tconst char *sub, *key;\n+\tsize_t sub_len;\n+\n+\tif (parse_config_key(var, \"uploadpack\", &sub, &sub_len, &key))\n+\t\treturn;\n+\tif (!sub || !skip_prefix(sub, \"filter.\", &sub))\n+\t\treturn;\n+\n+\tif (sub != key)\n+\t\tstrbuf_add(&spec, sub, key - sub - 1);\n+\tstrbuf_tolower(&spec);\n+\n+\tif (!strcmp(key, \"allow\")) {\n+\t\tif (spec.len)\n+\t\t\tstring_list_insert(&data->allowed_filters, spec.buf)->util =\n+\t\t\t\t(void *)(intptr_t)git_config_bool(var, value);\n+\t\telse\n+\t\t\tdata->allow_filter_fallback = git_config_bool(var, value);\n+\t}\n+\n+\tstrbuf_release(&spec);\n+}\n+\n static int upload_pack_config(const char *var, const char *value, void *cb_data)\n {\n \tstruct upload_pack_data *data = cb_data;\n@@ -1208,6 +1283,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)\n \t\t\treturn git_config_string(&data->pack_objects_hook, var, value);\n \t}\n \n+\tparse_object_filter_config(var, value, data);\n+\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n \n@@ -1388,6 +1465,7 @@ static void process_args(struct packet_reader *request,\n \t\tif (data->allow_filter && skip_prefix(arg, \"filter \", &p)) {\n \t\t\tlist_objects_filter_die_if_populated(&data->filter_options);\n \t\t\tparse_list_objects_filter(&data->filter_options, p);\n+\t\t\tdie_if_using_banned_filter(data);\n \t\t\tcontinue;\n \t\t}\n \n-- \n2.27.0.225.g9fa765a71d\n\n"},{"id":"400985","messageId":"3434bd5979499899e24528c9a578288f1c8d2669.1593720075.git.me@ttaylorr.com","threadId":"53792","inReplyTo":"cover.1593720075.git.me@ttaylorr.com","subject":"[PATCH 3/4] upload-pack.c: pass 'struct list_objects_filter_options *'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-02T20:06:36Z","receivedAt":"2020-07-02T20:06:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The 'allows_filter_choice' function used to take an 'enum\nlist_objects_filter_choice', but in a future commit it will be more\nconvenient for it to accept the whole 'struct\nlist_objects_filter_options', for e.g., to inspect the value of\n'->tree_exclude_depth'.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n upload-pack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex a5f56d73cc..a014ae23a9 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -990,9 +990,9 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,\n }\n \n static int allows_filter_choice(struct upload_pack_data *data,\n-\t\t\t\tenum list_objects_filter_choice c)\n+\t\t\t\tstruct list_objects_filter_options *opts)\n {\n-\tconst char *key = list_object_filter_config_name(c);\n+\tconst char *key = list_object_filter_config_name(opts->choice);\n \tstruct string_list_item *item = string_list_lookup(&data->allowed_filters,\n \t\t\t\t\t\t\t   key);\n \tif (item)\n@@ -1006,7 +1006,7 @@ static struct list_objects_filter_options *banned_filter(\n {\n \tsize_t i;\n \n-\tif (!allows_filter_choice(data, opts->choice))\n+\tif (!allows_filter_choice(data, opts))\n \t\treturn opts;\n \n \tif (opts->choice == LOFC_COMBINE)\n-- \n2.27.0.225.g9fa765a71d\n\n"},{"id":"400986","messageId":"9fa765a71d25ef3462ce81cca9754daa9b2579b6.1593720075.git.me@ttaylorr.com","threadId":"53792","inReplyTo":"cover.1593720075.git.me@ttaylorr.com","subject":"[PATCH 4/4] upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-02T20:06:40Z","receivedAt":"2020-07-02T20:06:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In b79cf959b2 (upload-pack.c: allow banning certain object filter(s),\n2020-02-26), we introduced functionality to disallow certain object\nfilters from being chosen from within 'git upload-pack'. Traditionally,\nadministrators use this functionality to disallow filters that are known\nto perform slowly, for e.g., those that do not have bitmap-level\nfiltering.\n\nIn the past, the '--filter=tree:<n>' was one such filter that does not\nhave bitmap-level filtering support, and so was likely to be banned by\nadministrators.\n\nHowever, in the previous couple of commits, we introduced bitmap-level\nfiltering for the case when 'n' is equal to '0', i.e., as if we had a\n'--filter=tree:none' choice.\n\nWhile it would be sufficient to simply write\n\n  $ git config uploadpack.filter.tree.allow true\n\n(since it would allow all values of 'n'), we would like to be able to\nallow this filter for certain values of 'n', i.e., those no greater than\nsome pre-specified maximum.\n\nIn order to do this, introduce a new configuration key, as follows:\n\n  $ git config uploadpack.filter.tree.maxDepth <m>\n\nwhere '<m>' specifies the maximum allowed value of 'n' in the filter\n'tree:n'. Administrators who wish to allow for only the value '0' can\nwrite:\n\n  $ git config uploadpack.filter.tree.allow true\n  $ git config uploadpack.filter.tree.maxDepth 0\n\nwhich allows '--filter=tree:0', but no other values.\n\nUnfortunately, since the tree depth is an unsigned long, we can't use,\nsay, -1 as a sentinel value, and so we must also keep track of \"have we\nset this\" as well as \"to what value\".\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/uploadpack.txt |  6 ++++++\n t/t5616-partial-clone.sh            |  8 ++++++++\n upload-pack.c                       | 32 ++++++++++++++++++++++++++---\n 3 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex fd4970306c..3671b62e4c 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -73,6 +73,12 @@ Note that the dot between 'filter' and '<filter>' is both non-standard\n and intentional. This is done to avoid a parsing ambiguity when\n specifying this configuration as an argument to Git's top-level `-c`.\n \n+uploadpack.filter.tree.maxDepth::\n+\tOnly allow `--filter=tree=<n>` when `n` is no more than the value of\n+\t`uploadpack.filter.tree.maxDepth`. If set, this also implies\n+\t`uploadpack.filter.tree.allow=true`, unless this configuration\n+\tvariable had already been set. Has no effect if unset.\n+\n uploadpack.allowRefInWant::\n \tIf this option is set, `upload-pack` will support the `ref-in-want`\n \tfeature of the protocol version 2 `fetch` command.  This feature\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 5dcd0b5656..8781a24cfe 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -261,6 +261,14 @@ test_expect_success 'upload-pack fails banned object filters with fallback' '\n \ttest_i18ngrep \"filter '\\''blob:none'\\'' not supported\" err\n '\n \n+test_expect_success 'upload-pack limits tree depth filters' '\n+\ttest_config -C srv.bare uploadpack.filter.allow false &&\n+\ttest_config -C srv.bare uploadpack.filter.tree.allow true &&\n+\ttest_config -C srv.bare uploadpack.filter.tree.maxDepth 0 &&\n+\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n+\t\t\"file://$(pwd)/srv.bare\" pc3\n+'\n+\n test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '\n \trm -rf src dst &&\n \tgit init src &&\ndiff --git a/upload-pack.c b/upload-pack.c\nindex a014ae23a9..8db1745b86 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -105,6 +105,7 @@ struct upload_pack_data {\n \tunsigned use_include_tag : 1;\n \tunsigned allow_filter : 1;\n \tunsigned allow_filter_fallback : 1;\n+\tunsigned long tree_filter_max_depth;\n \n \tunsigned done : 1;\t\t\t\t\t/* v2 only */\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n@@ -136,6 +137,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)\n \tdata->extra_edge_obj = extra_edge_obj;\n \tdata->allowed_filters = allowed_filters;\n \tdata->allow_filter_fallback = 1;\n+\tdata->tree_filter_max_depth = ULONG_MAX;\n \tpacket_writer_init(&data->writer, 1);\n \n \tdata->keepalive = 5;\n@@ -995,8 +997,17 @@ static int allows_filter_choice(struct upload_pack_data *data,\n \tconst char *key = list_object_filter_config_name(opts->choice);\n \tstruct string_list_item *item = string_list_lookup(&data->allowed_filters,\n \t\t\t\t\t\t\t   key);\n+\tint allowed = -1;\n \tif (item)\n-\t\treturn (intptr_t) item->util;\n+\t\tallowed = (intptr_t) item->util;\n+\n+\tif (allowed != 0 &&\n+\t    opts->choice == LOFC_TREE_DEPTH &&\n+\t    opts->tree_exclude_depth > data->tree_filter_max_depth)\n+\t\treturn 0;\n+\n+\tif (allowed > -1)\n+\t\treturn allowed;\n \treturn data->allow_filter_fallback;\n }\n \n@@ -1022,11 +1033,22 @@ static void die_if_using_banned_filter(struct upload_pack_data *data)\n {\n \tstruct list_objects_filter_options *banned = banned_filter(data,\n \t\t\t\t\t\t\t\t   &data->filter_options);\n+\tstruct strbuf buf = STRBUF_INIT;\n \tif (!banned)\n \t\treturn;\n \n-\tpacket_writer_error(&data->writer, _(\"filter '%s' not supported\\n\"),\n-\t\t\t    list_object_filter_config_name(banned->choice));\n+\tstrbuf_addf(&buf, _(\"filter '%s' not supported\"),\n+\t\t    list_object_filter_config_name(banned->choice));\n+\tif (banned->choice == LOFC_TREE_DEPTH &&\n+\t    data->tree_filter_max_depth != ULONG_MAX)\n+\t\tstrbuf_addf(&buf, _(\" (maximum depth: %lu, but got: %lu)\"),\n+\t\t\t    data->tree_filter_max_depth,\n+\t\t\t    banned->tree_exclude_depth);\n+\n+\tpacket_writer_error(&data->writer, \"%s\\n\", buf.buf);\n+\n+\tstrbuf_release(&buf);\n+\n \tdie(_(\"git upload-pack: banned object filter requested\"));\n }\n \n@@ -1239,6 +1261,10 @@ static void parse_object_filter_config(const char *var, const char *value,\n \t\t\t\t(void *)(intptr_t)git_config_bool(var, value);\n \t\telse\n \t\t\tdata->allow_filter_fallback = git_config_bool(var, value);\n+\t} else if (!strcmp(spec.buf, \"tree\") && !strcmp(key, \"maxdepth\")) {\n+\t\tstring_list_insert(&data->allowed_filters, \"tree\")->util\n+\t\t\t= (void *) (intptr_t) 1;\n+\t\tdata->tree_filter_max_depth = git_config_ulong(var, value);\n \t}\n \n \tstrbuf_release(&spec);\n-- \n2.27.0.225.g9fa765a71d\n"},{"id":"401181","messageId":"20200708084136.GA2324177@coredump.intra.peff.net","threadId":"53792","inReplyTo":"cover.1593720075.git.me@ttaylorr.com","subject":"Re: [PATCH 0/4] upload-pack: custom allowed object filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-08T08:41:36Z","receivedAt":"2020-07-08T08:41:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 02, 2020 at 04:06:14PM -0400, Taylor Blau wrote:\n\n>   [uploadpack]\n>   \tallowAnySHA1InWant = true\n>   \tallowFilter = true\n>   [uploadpack \"filter\"]\n>   \tallow = false\n>   [uploadpack \"filter.blob:limit\"]\n>   \tallow = true\n>   [uploadpack \"filter.blob:none\"]\n>   \tallow = true\n>   [uploadpack \"filter.tree\"]\n>   \tallow = true\n>   \tmaxDepth = 0\n\nI thought the previous discussion landed on:\n\n  uploadpackfilter.blob:none.allow\n\netc, to avoid the confusing appearance of four-level keys (and the weird\ncase sensitivity implications that would cause).\n\n-Peff\n"},{"id":"401182","messageId":"20200708084527.GB2324177@coredump.intra.peff.net","threadId":"53792","inReplyTo":"f0982d24e74155f6c0e405e5e3ae8c3e579f798a.1593720075.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-08T08:45:27Z","receivedAt":"2020-07-08T08:45:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:\n\n> +static void parse_object_filter_config(const char *var, const char *value,\n> +\t\t\t\t       struct upload_pack_data *data)\n> +{\n> +\tstruct strbuf spec = STRBUF_INIT;\n> +\tconst char *sub, *key;\n> +\tsize_t sub_len;\n> +\n> +\tif (parse_config_key(var, \"uploadpack\", &sub, &sub_len, &key))\n> +\t\treturn;\n> +\tif (!sub || !skip_prefix(sub, \"filter.\", &sub))\n> +\t\treturn;\n\nJust while I'm thinking about the config name and case-sensitivity (from\nthe cover letter): if we did want to use this scheme, then\nskip_iprefix() would make this behave more like a regular part of the\nsection name.\n\nBut I'd prefer to just do away with it by using a scheme that doesn't\nhave the extra layer of dots.\n\n> +\tif (sub != key)\n> +\t\tstrbuf_add(&spec, sub, key - sub - 1);\n> +\tstrbuf_tolower(&spec);\n\nOn the flip side, I'd actually consider _not_ matching the filter name\ncase-insensitively. We don't do so elsewhere (e.g., \"git rev-list\n--filter=BLOB:NONE\" will complain).\n\n-Peff\n"},{"id":"401582","messageId":"20200715100043.GG11341@szeder.dev","threadId":"53792","inReplyTo":"f0982d24e74155f6c0e405e5e3ae8c3e579f798a.1593720075.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-07-15T10:00:43Z","receivedAt":"2020-07-15T10:00:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index 8a27452a51..5dcd0b5656 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -235,6 +235,32 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' '\n>  \ttest_cmp unique_types.expected unique_types.actual\n>  '\n>  \n> +test_expect_success 'upload-pack fails banned object filters' '\n> +\t# Test case-insensitivity by intentional use of \"blob:None\" rather than\n> +\t# \"blob:none\".\n> +\ttest_config -C srv.bare uploadpack.filter.blob:None.allow false &&\n> +\ttest_must_fail git clone --no-checkout --filter=blob:none \\\n> +\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n> +\ttest_i18ngrep \"filter '\\''blob:none'\\'' not supported\" err\n> +'\n> +\n> +test_expect_success 'upload-pack fails banned combine object filters' '\n> +\ttest_config -C srv.bare uploadpack.filter.allow false &&\n> +\ttest_config -C srv.bare uploadpack.filter.combine.allow true &&\n> +\ttest_config -C srv.bare uploadpack.filter.tree.allow true &&\n> +\ttest_config -C srv.bare uploadpack.filter.blob:none.allow false &&\n> +\ttest_must_fail git clone --no-checkout --filter=tree:1 \\\n> +\t\t--filter=blob:none \"file://$(pwd)/srv.bare\" pc3 2>err &&\n> +\ttest_i18ngrep \"filter '\\''blob:none'\\'' not supported\" err\n> +'\n> +\n> +test_expect_success 'upload-pack fails banned object filters with fallback' '\n> +\ttest_config -C srv.bare uploadpack.filter.allow false &&\n> +\ttest_must_fail git clone --no-checkout --filter=blob:none \\\n> +\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n> +\ttest_i18ngrep \"filter '\\''blob:none'\\'' not supported\" err\n> +'\n\nThese three tests are very flaky: 'git upload-pack' can error out\nwhile clone is still sending packets (usually the 'done' line),\nresulting in SIGPIPE and frequent CI failures.  Running this test\nscript with '-r 1,2,17-19 --stress' tends to fail in a couple of\nseconds.\n\nUsing 'test_must_fail ok=sigpipe', as you did in the test in the last\npatch, avoids the test failure caused by SIGPIPE, of course, but,\nunfortunately, all three tests remain flaky, because the expected\nerror message sometimes doesn't make it to 'git clone's stderr, e.g.:\n\n  expecting success of 5616.19 'upload-pack fails banned object filters with fallback': \n          test_config -C srv.bare uploadpack.filter.allow false &&\n          test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \\\n                  \"file://$(pwd)/srv.bare\" pc3 2>err &&\n          test_i18ngrep \"filter 'blob:none' not supported\" err\n  \n  + test_config -C srv.bare uploadpack.filter.allow false\n  + pwd\n  + 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\n  + test_i18ngrep filter 'blob:none' not supported err\n  error: 'grep filter 'blob:none' not supported err' didn't find a match in:\n  Cloning into 'pc3'...\n  fatal: git upload-pack: banned object filter requested\n  error: last command exited with $?=1\n  not ok 19 - upload-pack fails banned object filters with fallback\n\n\nOnce upon a time I had a PoC patch to deal with 'git upload-pack'\naborting while 'git fetch' is still send_request()-ing, by catching\nthe write error to the closed connection and trying read any pending\nERR packets; Christian cleaned it up and submitted it with a proper\ncommit message in\n\n  https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/\n\nbut it haven't been picked up yet.  Disappointingly, that patch\ndoesn't solve these issues...  I haven't looked what's going on\n(perhaps 'git clone' does something differently than 'git fetch'?  no\nidea)\n\n"},{"id":"401583","messageId":"20200715101156.GB22114@szeder.dev","threadId":"53792","inReplyTo":"9fa765a71d25ef3462ce81cca9754daa9b2579b6.1593720075.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-07-15T10:11:56Z","receivedAt":"2020-07-15T10:12:03Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Jul 02, 2020 at 04:06:40PM -0400, Taylor Blau wrote:\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index 5dcd0b5656..8781a24cfe 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -261,6 +261,14 @@ test_expect_success 'upload-pack fails banned object filters with fallback' '\n>  \ttest_i18ngrep \"filter '\\''blob:none'\\'' not supported\" err\n>  '\n>  \n> +test_expect_success 'upload-pack limits tree depth filters' '\n> +\ttest_config -C srv.bare uploadpack.filter.allow false &&\n> +\ttest_config -C srv.bare uploadpack.filter.tree.allow true &&\n> +\ttest_config -C srv.bare uploadpack.filter.tree.maxDepth 0 &&\n> +\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n> +\t\t\"file://$(pwd)/srv.bare\" pc3\n> +'\n\nUnlike in the other three tests added earlier in this series, here you\ndo use 'test_must_fail ok=sigpipe', but you don't check that the\ncommand died with the right error message.  Saving stderr and adding\n\n  test_i18ngrep \"filter '\\''tree'\\'' not supported (maximum depth: 0, but got: 1)\" err\n\nmakes this test flaky, too, like the other three:\n\n  expecting success of 5616.20 'upload-pack limits tree depth filters': \n          test_config -C srv.bare uploadpack.filter.allow false &&\n          test_config -C srv.bare uploadpack.filter.tree.allow true &&\n          test_config -C srv.bare uploadpack.filter.tree.maxDepth 0 &&\n          test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n                  \"file://$(pwd)/srv.bare\" pc3 2>err &&\n          test_i18ngrep \"filter 'tree' not supported (maximum depth: 0, but got: 1)\" err\n  \n  + test_config -C srv.bare uploadpack.filter.allow false\n  + test_config -C srv.bare uploadpack.filter.tree.allow true\n  + test_config -C srv.bare uploadpack.filter.tree.maxDepth 0\n  + pwd\n  + 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\n  + test_i18ngrep filter 'tree' not supported (maximum depth: 0, but got: 1) err\n  error: 'grep filter 'tree' not supported (maximum depth: 0, but got: 1) err' didn't find a match in:\n  Cloning into 'pc3'...\n  fatal: git upload-pack: banned object filter requested\n  error: last command exited with $?=1\n  not ok 20 - upload-pack limits tree depth filters\n\n\n"},{"id":"401584","messageId":"20200715105521.GB3262157@coredump.intra.peff.net","threadId":"53792","inReplyTo":"20200715100043.GG11341@szeder.dev","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-15T10:55:21Z","receivedAt":"2020-07-15T10:55:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 15, 2020 at 12:00:43PM +0200, SZEDER Gábor wrote:\n\n> Once upon a time I had a PoC patch to deal with 'git upload-pack'\n> aborting while 'git fetch' is still send_request()-ing, by catching\n> the write error to the closed connection and trying read any pending\n> ERR packets; Christian cleaned it up and submitted it with a proper\n> commit message in\n> \n>   https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/\n> \n> but it haven't been picked up yet.  Disappointingly, that patch\n> doesn't solve these issues...  I haven't looked what's going on\n> (perhaps 'git clone' does something differently than 'git fetch'?  no\n> idea)\n\nI suspect it is that fetch ignores SIGPIPE, but clone does not. So even\nwhen we see a 141 exit code from fetch, it is probably generated\nsynthetically from exit(141) after we saw EPIPE. And your patch works\nthere because we have a chance to pump the read-side of the pipe,\nwhereas in git-clone we die immediately via the signal.\n\nProbably git-clone should ignore SIGPIPE during the network transfer\nportion of the process for the same reasons given in 143588949c (fetch:\nignore SIGPIPE during network operation, 2019-03-03).\n\n-Peff\n"},{"id":"401784","messageId":"20200720200528.GA91942@syl.lan","threadId":"53792","inReplyTo":"20200708084527.GB2324177@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-20T20:05:28Z","receivedAt":"2020-07-20T20:05:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 08, 2020 at 04:45:27AM -0400, Jeff King wrote:\n> On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:\n>\n> > +static void parse_object_filter_config(const char *var, const char *value,\n> > +\t\t\t\t       struct upload_pack_data *data)\n> > +{\n> > +\tstruct strbuf spec = STRBUF_INIT;\n> > +\tconst char *sub, *key;\n> > +\tsize_t sub_len;\n> > +\n> > +\tif (parse_config_key(var, \"uploadpack\", &sub, &sub_len, &key))\n> > +\t\treturn;\n> > +\tif (!sub || !skip_prefix(sub, \"filter.\", &sub))\n> > +\t\treturn;\n>\n> Just while I'm thinking about the config name and case-sensitivity (from\n> the cover letter): if we did want to use this scheme, then\n> skip_iprefix() would make this behave more like a regular part of the\n> section name.\n>\n> But I'd prefer to just do away with it by using a scheme that doesn't\n> have the extra layer of dots.\n\nYeah. I definitely flip-flopped on this when preparing this for the\nlist. I still feel like 'uploadpackfilter' is gross, so I was hoping to\nsend it with 'uploadpack.filter' (which reads nicely, but forces us to\nwrite some gross code), but both options are frustrating to me for\ndifferent reasons.\n\nPlaying around with it more, though, I think that uploadpackfilter is\nour best bet. I'd hate to introduce a string manipulation bug by munging\nthe reuslt of 'parse_config_key()', which is so clearly designed for\nsomething like 'uploadpackfilter[.<filter>].allow'.\n\n> > +\tif (sub != key)\n> > +\t\tstrbuf_add(&spec, sub, key - sub - 1);\n> > +\tstrbuf_tolower(&spec);\n>\n> On the flip side, I'd actually consider _not_ matching the filter name\n> case-insensitively. We don't do so elsewhere (e.g., \"git rev-list\n> --filter=BLOB:NONE\" will complain).\n\nI dropped the case-insensitive matching in the latest revision. I don't\nthink that we need to be overly accommodating here.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"401785","messageId":"20200720200739.GB91942@syl.lan","threadId":"53792","inReplyTo":"20200715105521.GB3262157@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-20T20:07:39Z","receivedAt":"2020-07-20T20:07:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 15, 2020 at 06:55:21AM -0400, Jeff King wrote:\n> On Wed, Jul 15, 2020 at 12:00:43PM +0200, SZEDER Gábor wrote:\n>\n> > Once upon a time I had a PoC patch to deal with 'git upload-pack'\n> > aborting while 'git fetch' is still send_request()-ing, by catching\n> > the write error to the closed connection and trying read any pending\n> > ERR packets; Christian cleaned it up and submitted it with a proper\n> > commit message in\n> >\n> >   https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/\n> >\n> > but it haven't been picked up yet.  Disappointingly, that patch\n> > doesn't solve these issues...  I haven't looked what's going on\n> > (perhaps 'git clone' does something differently than 'git fetch'?  no\n> > idea)\n>\n> I suspect it is that fetch ignores SIGPIPE, but clone does not. So even\n> when we see a 141 exit code from fetch, it is probably generated\n> synthetically from exit(141) after we saw EPIPE. And your patch works\n> there because we have a chance to pump the read-side of the pipe,\n> whereas in git-clone we die immediately via the signal.\n\nHeh. I was hoping to be rid of those errors with Christian's patches,\nbut it sounds like the problem is coming from outside of 'upload-pack'\nand instead in 'clone'.\n\nThat reasoning seems sound to me, but I'd rather not touch clone in this\npatch series if I don't have to. What I'd rather do is something like:\n\n  - Introduce this patch series with the 'test_must_fail ok=sigpipe',\n    and no error checking.\n\n  - Modify clone to swallow these errors and eat a packet or two.\n\n  - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little\n    bit smarter here.\n\nMaybe more steps than is strictly necessary, but I think it keeps the\nscope of the review on this series reasonable, which is a tradeoff that\nI'm willing to make.\n\n> Probably git-clone should ignore SIGPIPE during the network transfer\n> portion of the process for the same reasons given in 143588949c (fetch:\n> ignore SIGPIPE during network operation, 2019-03-03).\n>\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"401786","messageId":"20200720200934.GC91942@syl.lan","threadId":"53792","inReplyTo":"20200708084136.GA2324177@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] upload-pack: custom allowed object filters","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-20T20:09:34Z","receivedAt":"2020-07-20T20:09:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 08, 2020 at 04:41:36AM -0400, Jeff King wrote:\n> On Thu, Jul 02, 2020 at 04:06:14PM -0400, Taylor Blau wrote:\n>\n> >   [uploadpack]\n> >   \tallowAnySHA1InWant = true\n> >   \tallowFilter = true\n> >   [uploadpack \"filter\"]\n> >   \tallow = false\n> >   [uploadpack \"filter.blob:limit\"]\n> >   \tallow = true\n> >   [uploadpack \"filter.blob:none\"]\n> >   \tallow = true\n> >   [uploadpack \"filter.tree\"]\n> >   \tallow = true\n> >   \tmaxDepth = 0\n>\n> I thought the previous discussion landed on:\n>\n>   uploadpackfilter.blob:none.allow\n>\n> etc, to avoid the confusing appearance of four-level keys (and the weird\n> case sensitivity implications that would cause).\n\nYup, I agree. I added a little bit more detail to this response in\nanother email that I posted shortly above in this thread.\n\nRest assured that I haven't forgotten about this series, I've just been\nbusy and not feeling too pressured to get back to it since we're well\ninto the release-candidate phase, and shouldn't be worrying about this\nuntil after 2.28.0.\n\nIf you're interested, I have what I will eventually call 'v2' of this\nseries at:\n\n  https://github.com/ttaylorr/git/tree/tb/upload-pack-filter-config\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"401787","messageId":"20200720202140.GA2059161@coredump.intra.peff.net","threadId":"53792","inReplyTo":"20200720200739.GB91942@syl.lan","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-20T20:21:40Z","receivedAt":"2020-07-20T20:21:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 20, 2020 at 04:07:39PM -0400, Taylor Blau wrote:\n\n> Heh. I was hoping to be rid of those errors with Christian's patches,\n> but it sounds like the problem is coming from outside of 'upload-pack'\n> and instead in 'clone'.\n\nYes, it's definitely on the client side.\n\n> That reasoning seems sound to me, but I'd rather not touch clone in this\n> patch series if I don't have to. What I'd rather do is something like:\n> \n>   - Introduce this patch series with the 'test_must_fail ok=sigpipe',\n>     and no error checking.\n> \n>   - Modify clone to swallow these errors and eat a packet or two.\n> \n>   - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little\n>     bit smarter here.\n> \n> Maybe more steps than is strictly necessary, but I think it keeps the\n> scope of the review on this series reasonable, which is a tradeoff that\n> I'm willing to make.\n\nI think that's reasonable.\n\nIt doesn't look like the \"pump the read side after EPIPE\" patch is even\non the \"seen\" branch yet, and you'd need to further extend it to clone.\nIt's definitely a big enough change that it's better not to lump it in\nwith this series.\n\n-Peff\n"},{"id":"401820","messageId":"xmqqwo2w7hv4.fsf@gitster.c.googlers.com","threadId":"53792","inReplyTo":"cover.1593720075.git.me@ttaylorr.com","subject":"Re: [PATCH 0/4] upload-pack: custom allowed object filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-21T20:06:39Z","receivedAt":"2020-07-21T20:06:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Taylor Blau (4):\n>   list_objects_filter_options: introduce 'list_object_filter_config_name'\n>   upload-pack.c: allow banning certain object filter(s)\n>   upload-pack.c: pass 'struct list_objects_filter_options *'\n>   upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'\n>\n>  Documentation/config/uploadpack.txt |  22 ++++++\n>  list-objects-filter-options.c       |  23 ++++++\n>  list-objects-filter-options.h       |   6 ++\n>  t/t5616-partial-clone.sh            |  34 +++++++++\n>  upload-pack.c                       | 104 ++++++++++++++++++++++++++++\n>  5 files changed, 189 insertions(+)\n>\n> --\n> 2.27.0.225.g9fa765a71d\n\nWith this series (I do not know which one of them is the culprit) in\nthe 'seen' branch, we seem to consistently get a segfault while\nrunning t5616 on macOS clang build [*1*]\n\n[Footnote]\n\n*1* https://travis-ci.org/github/git/git/jobs/710504820 has the\n    topic at the tip of 'seen' that fails.  Without that merge,\n    https://travis-ci.org/github/git/git/jobs/710342598 seems to\n    pass on all archs.\n\n\n\n\n\n"},{"id":"401822","messageId":"20200721202740.GA34538@syl.lan","threadId":"53792","inReplyTo":"xmqqwo2w7hv4.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/4] upload-pack: custom allowed object filters","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-21T20:27:40Z","receivedAt":"2020-07-21T20:27:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 21, 2020 at 01:06:39PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > Taylor Blau (4):\n> >   list_objects_filter_options: introduce 'list_object_filter_config_name'\n> >   upload-pack.c: allow banning certain object filter(s)\n> >   upload-pack.c: pass 'struct list_objects_filter_options *'\n> >   upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'\n> >\n> >  Documentation/config/uploadpack.txt |  22 ++++++\n> >  list-objects-filter-options.c       |  23 ++++++\n> >  list-objects-filter-options.h       |   6 ++\n> >  t/t5616-partial-clone.sh            |  34 +++++++++\n> >  upload-pack.c                       | 104 ++++++++++++++++++++++++++++\n> >  5 files changed, 189 insertions(+)\n> >\n> > --\n> > 2.27.0.225.g9fa765a71d\n>\n> With this series (I do not know which one of them is the culprit) in\n> the 'seen' branch, we seem to consistently get a segfault while\n> running t5616 on macOS clang build [*1*]\n\nAye. Am I reading that correctly that it's git clone dying with a\nSIGPIPE instead of a segfault? If so, this is what Szeder pointed out a\nlittle lower in the thread (tl;dr is that 'git clone' is not resilient\nto this whereas 'git fetch' is, and so we still need an `ok=sigpipe`\nsomewhere in t5616).\n\nThe new version that I'm preparing has these appropriately, so feel\nfree to discard this until I send a new version your way...\n\n> [Footnote]\n>\n> *1* https://travis-ci.org/github/git/git/jobs/710504820 has the\n>     topic at the tip of 'seen' that fails.  Without that merge,\n>     https://travis-ci.org/github/git/git/jobs/710342598 seems to\n>     pass on all archs.\n\nThanks,\nTaylor\n"},{"id":"401828","messageId":"xmqqd04o7cd6.fsf@gitster.c.googlers.com","threadId":"53792","inReplyTo":"20200721202740.GA34538@syl.lan","subject":"Re: [PATCH 0/4] upload-pack: custom allowed object filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-21T22:05:25Z","receivedAt":"2020-07-21T22:05:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Aye. Am I reading that correctly that it's git clone dying with a\n> SIGPIPE instead of a segfault? If so, this is what Szeder pointed out a\n> little lower in the thread (tl;dr is that 'git clone' is not resilient\n> to this whereas 'git fetch' is, and so we still need an `ok=sigpipe`\n> somewhere in t5616).\n>\n> The new version that I'm preparing has these appropriately, so feel\n> free to discard this until I send a new version your way...\n\nIf it is known and under control, I have nothing to worry about ;-)\n\nThanks.\n"},{"id":"401844","messageId":"20200722091758.GJ11341@szeder.dev","threadId":"53792","inReplyTo":"20200720200739.GB91942@syl.lan","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-07-22T09:17:58Z","receivedAt":"2020-07-22T09:18:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jul 20, 2020 at 04:07:39PM -0400, Taylor Blau wrote:\n> What I'd rather do is something like:\n> \n>   - Introduce this patch series with the 'test_must_fail ok=sigpipe',\n>     and no error checking.\n\ndie_if_using_banned_filter() shows two error messages: a \"fancy\" one\nis sent to the client in the ERR packet, including which particular\nfilter is not supported/allowed, and a simple \n\n  die(_(\"git upload-pack: banned object filter requested\"));\n\nIf this die() were to show the same fancy error message as in the ERR\npacket, then it would always make it to 'git clone's stderr in the\ntests, so the tests could reliably check that 'git upload-pack' died\nfor the expected reason.\n\n>   - Modify clone to swallow these errors and eat a packet or two.\n> \n>   - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little\n>     bit smarter here.\n> \n> Maybe more steps than is strictly necessary, but I think it keeps the\n> scope of the review on this series reasonable, which is a tradeoff that\n> I'm willing to make.\n> \n> > Probably git-clone should ignore SIGPIPE during the network transfer\n> > portion of the process for the same reasons given in 143588949c (fetch:\n> > ignore SIGPIPE during network operation, 2019-03-03).\n> >\n> > -Peff\n> \n> Thanks,\n> Taylor\n"},{"id":"401845","messageId":"20200722092129.GK11341@szeder.dev","threadId":"53792","inReplyTo":"f0982d24e74155f6c0e405e5e3ae8c3e579f798a.1593720075.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-07-22T09:21:29Z","receivedAt":"2020-07-22T09:21:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:\n> +static void die_if_using_banned_filter(struct upload_pack_data *data)\n> +{\n> +\tstruct list_objects_filter_options *banned = banned_filter(data,\n> +\t\t\t\t\t\t\t\t   &data->filter_options);\n> +\tif (!banned)\n> +\t\treturn;\n> +\n> +\tpacket_writer_error(&data->writer, _(\"filter '%s' not supported\\n\"),\n\nShould we really translate the content of the ERR packet?\n\n> +\t\t\t    list_object_filter_config_name(banned->choice));\n> +\tdie(_(\"git upload-pack: banned object filter requested\"));\n> +}\n> +\n>  static void receive_needs(struct upload_pack_data *data,\n>  \t\t\t  struct packet_reader *reader)\n>  {\n"},{"id":"401854","messageId":"20200722201531.GA59352@syl.lan","threadId":"53792","inReplyTo":"20200722091758.GJ11341@szeder.dev","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-22T20:15:31Z","receivedAt":"2020-07-22T20:15:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 22, 2020 at 11:17:58AM +0200, SZEDER Gábor wrote:\n> On Mon, Jul 20, 2020 at 04:07:39PM -0400, Taylor Blau wrote:\n> > What I'd rather do is something like:\n> >\n> >   - Introduce this patch series with the 'test_must_fail ok=sigpipe',\n> >     and no error checking.\n>\n> die_if_using_banned_filter() shows two error messages: a \"fancy\" one\n> is sent to the client in the ERR packet, including which particular\n> filter is not supported/allowed, and a simple\n>\n>   die(_(\"git upload-pack: banned object filter requested\"));\n>\n> If this die() were to show the same fancy error message as in the ERR\n> packet, then it would always make it to 'git clone's stderr in the\n> tests, so the tests could reliably check that 'git upload-pack' died\n> for the expected reason.\n\nBeautiful idea. I changed this in my fork, and I'll send it to this\nthread after 2.28 is out, since I don't want to create a distraction in\nthe meantime.\n\n> >   - Modify clone to swallow these errors and eat a packet or two.\n> >\n> >   - Then, drop the 'ok=sigpipe' from t5616 after 'git clone' is a little\n> >     bit smarter here.\n> >\n> > Maybe more steps than is strictly necessary, but I think it keeps the\n> > scope of the review on this series reasonable, which is a tradeoff that\n> > I'm willing to make.\n> >\n> > > Probably git-clone should ignore SIGPIPE during the network transfer\n> > > portion of the process for the same reasons given in 143588949c (fetch:\n> > > ignore SIGPIPE during network operation, 2019-03-03).\n> > >\n> > > -Peff\n> >\n> > Thanks,\n> > Taylor\n\nThanks,\nTaylor\n"},{"id":"401855","messageId":"20200722201634.GB59352@syl.lan","threadId":"53792","inReplyTo":"20200722092129.GK11341@szeder.dev","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-22T20:16:34Z","receivedAt":"2020-07-22T20:16:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 22, 2020 at 11:21:29AM +0200, SZEDER Gábor wrote:\n> On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:\n> > +static void die_if_using_banned_filter(struct upload_pack_data *data)\n> > +{\n> > +\tstruct list_objects_filter_options *banned = banned_filter(data,\n> > +\t\t\t\t\t\t\t\t   &data->filter_options);\n> > +\tif (!banned)\n> > +\t\treturn;\n> > +\n> > +\tpacket_writer_error(&data->writer, _(\"filter '%s' not supported\\n\"),\n>\n> Should we really translate the content of the ERR packet?\n\ns/ERR packet/dying message after your other piece of advice, but I think\nso. This is eventually going to make its way to users, so translating it\nseems to make sense to me.\n\nI guess they would get it in the localization of the server, not the\nclient, but that doesn't sway me away from this, either.\n\n> > +\t\t\t    list_object_filter_config_name(banned->choice));\n> > +\tdie(_(\"git upload-pack: banned object filter requested\"));\n> > +}\n> > +\n> >  static void receive_needs(struct upload_pack_data *data,\n> >  \t\t\t  struct packet_reader *reader)\n> >  {\n\nThanks,\nTaylor\n"},{"id":"401911","messageId":"xmqqft9j3t4i.fsf@gitster.c.googlers.com","threadId":"53792","inReplyTo":"20200722201531.GA59352@syl.lan","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-23T01:41:33Z","receivedAt":"2020-07-23T01:41:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> If this die() were to show the same fancy error message as in the ERR\n>> packet, then it would always make it to 'git clone's stderr in the\n>> tests, so the tests could reliably check that 'git upload-pack' died\n>> for the expected reason.\n>\n> Beautiful idea. I changed this in my fork, and I'll send it to this\n> thread after 2.28 is out, since I don't want to create a distraction in\n> the meantime.\n\nThanks.  \n\nAs long as everybody understands that \"distraction\" will immediately\ngets backburnered when a regression relevant to the upcoming release\nis found, however, I think performing work as usual is a good thing.\nAfter all, we often find glitches in the current code by trying to\nbuild on it, in addition to using it.\n\n"},{"id":"401916","messageId":"20200723015048.GA83982@syl.lan","threadId":"53792","inReplyTo":"xmqqft9j3t4i.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-23T01:50:48Z","receivedAt":"2020-07-23T01:50:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 22, 2020 at 06:41:33PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> >> If this die() were to show the same fancy error message as in the ERR\n> >> packet, then it would always make it to 'git clone's stderr in the\n> >> tests, so the tests could reliably check that 'git upload-pack' died\n> >> for the expected reason.\n> >\n> > Beautiful idea. I changed this in my fork, and I'll send it to this\n> > thread after 2.28 is out, since I don't want to create a distraction in\n> > the meantime.\n>\n> Thanks.\n>\n> As long as everybody understands that \"distraction\" will immediately\n> gets backburnered when a regression relevant to the upcoming release\n> is found, however, I think performing work as usual is a good thing.\n> After all, we often find glitches in the current code by trying to\n> build on it, in addition to using it.\n\nFair enough ;-). I sent v2 a few minutes ago, but forgot to attach it to\nthis thread as a reply. So, I suppose we can pick up the review there,\ninstead.\n\nThanks,\nTaylor\n"},{"id":"401925","messageId":"20200723075144.GL11341@szeder.dev","threadId":"53792","inReplyTo":"20200722201634.GB59352@syl.lan","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-07-23T07:51:44Z","receivedAt":"2020-07-23T07:51:50Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jul 22, 2020 at 04:16:34PM -0400, Taylor Blau wrote:\n> On Wed, Jul 22, 2020 at 11:21:29AM +0200, SZEDER Gábor wrote:\n> > On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:\n> > > +static void die_if_using_banned_filter(struct upload_pack_data *data)\n> > > +{\n> > > +\tstruct list_objects_filter_options *banned = banned_filter(data,\n> > > +\t\t\t\t\t\t\t\t   &data->filter_options);\n> > > +\tif (!banned)\n> > > +\t\treturn;\n> > > +\n> > > +\tpacket_writer_error(&data->writer, _(\"filter '%s' not supported\\n\"),\n> >\n> > Should we really translate the content of the ERR packet?\n> \n> s/ERR packet/dying message after your other piece of advice, but I think\n> so. This is eventually going to make its way to users, so translating it\n> seems to make sense to me.\n> \n> I guess they would get it in the localization of the server, not the\n> client, but that doesn't sway me away from this, either.\n\nPlease note that the contents of other ERR packets are not translated,\neither.\n\n$ git grep -A1 -h packet_writer_error origin/seen:upload-pack.c\n\t\t\tpacket_writer_error(&data->writer,\n\t\t\t\t\t    \"upload-pack: not our ref %s\",\n--\n\t\t\tpacket_writer_error(&data->writer,\n\t\t\t\t\t    \"upload-pack: not our ref %s\",\n--\n\t\t\tpacket_writer_error(writer,\n\t\t\t\t\t    \"upload-pack: not our ref %s\",\n--\n\t\t\tpacket_writer_error(writer, \"unknown ref %s\", arg);\n\t\t\tdie(\"unknown ref %s\", arg);\n\n\n> > > +\t\t\t    list_object_filter_config_name(banned->choice));\n> > > +\tdie(_(\"git upload-pack: banned object filter requested\"));\n> > > +}\n> > > +\n> > >  static void receive_needs(struct upload_pack_data *data,\n> > >  \t\t\t  struct packet_reader *reader)\n> > >  {\n> \n> Thanks,\n> Taylor\n"},{"id":"401927","messageId":"20200723141306.GA86870@syl.lan","threadId":"53792","inReplyTo":"20200723075144.GL11341@szeder.dev","subject":"Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-23T14:13:06Z","receivedAt":"2020-07-23T14:13:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jul 23, 2020 at 09:51:44AM +0200, SZEDER Gábor wrote:\n> On Wed, Jul 22, 2020 at 04:16:34PM -0400, Taylor Blau wrote:\n> > On Wed, Jul 22, 2020 at 11:21:29AM +0200, SZEDER Gábor wrote:\n> > > On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:\n> > > > +static void die_if_using_banned_filter(struct upload_pack_data *data)\n> > > > +{\n> > > > +\tstruct list_objects_filter_options *banned = banned_filter(data,\n> > > > +\t\t\t\t\t\t\t\t   &data->filter_options);\n> > > > +\tif (!banned)\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tpacket_writer_error(&data->writer, _(\"filter '%s' not supported\\n\"),\n> > >\n> > > Should we really translate the content of the ERR packet?\n> >\n> > s/ERR packet/dying message after your other piece of advice, but I think\n> > so. This is eventually going to make its way to users, so translating it\n> > seems to make sense to me.\n> >\n> > I guess they would get it in the localization of the server, not the\n> > client, but that doesn't sway me away from this, either.\n>\n> Please note that the contents of other ERR packets are not translated,\n> either.\n>\n> $ git grep -A1 -h packet_writer_error origin/seen:upload-pack.c\n> \t\t\tpacket_writer_error(&data->writer,\n> \t\t\t\t\t    \"upload-pack: not our ref %s\",\n> --\n> \t\t\tpacket_writer_error(&data->writer,\n> \t\t\t\t\t    \"upload-pack: not our ref %s\",\n> --\n> \t\t\tpacket_writer_error(writer,\n> \t\t\t\t\t    \"upload-pack: not our ref %s\",\n> --\n> \t\t\tpacket_writer_error(writer, \"unknown ref %s\", arg);\n> \t\t\tdie(\"unknown ref %s\", arg);\n\nFair enough... if there are other changes to make I'll drop the\ntranslation, otherwise this should be easy enough to apply when queuing.\n\nJunio, if you'd rather I reroll the series again, too, that's fine as\nwell.\n\n>\n> > > > +\t\t\t    list_object_filter_config_name(banned->choice));\n> > > > +\tdie(_(\"git upload-pack: banned object filter requested\"));\n> > > > +}\n> > > > +\n> > > >  static void receive_needs(struct upload_pack_data *data,\n> > > >  \t\t\t  struct packet_reader *reader)\n> > > >  {\n> >\n> > Thanks,\n> > Taylor\n\nThanks,\nTaylor\n"}]}