{"thread":{"id":"53910","subject":"[PATCH v2 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name'","startedAt":"2020-07-23T01:49:00Z","lastAt":"2020-08-04T00:37:24Z","messageCount":32,"participants":["Taylor Blau","SZEDER Gábor","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"401917","messageId":"cover.1595468657.git.me@ttaylorr.com","threadId":"53910","inReplyTo":null,"subject":"[PATCH v2 0/4] upload-pack: custom allowed object filters","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-23T01:48:54Z","receivedAt":"2020-07-23T01:49:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here's a much-delayed v2 of my series to teach upload-pack to limit the\nkinds of object filters that it is willing to server in a request.\n\nMuch is the same since last time, with two notable exceptions:\n\n - We use the 'uploadpackfilter' top-level configuration key instead of\n pretending that 'uploadpack.filter' is top-level, which greatly\n simplifies the call to 'parse_config_key()'.\n\n - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,\n which propagates the error through 'git clone' always, and resolves\n a flaky set of tests that used to result in a SIGPIPE.\n\nThanks for your review in the meantime, and for encouraging me to send\nthis reroll now instead of later. Maybe I'll start unloading the rest of\nmy backlog tomorrow ;-).\n\nTaylor Blau (4):\n list_objects_filter_options: introduce\n '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 'uploadpackfilter.tree.maxDepth'\n\n Documentation/config/uploadpack.txt | 18 +++++\n list-objects-filter-options.c | 23 +++++++\n list-objects-filter-options.h | 6 ++\n t/t5616-partial-clone.sh | 32 +++++++++\n upload-pack.c | 103 ++++++++++++++++++++++++++++\n 5 files changed, 182 insertions(+)\n\nRange-diff against v1:\n 2: f0982d24e7 ! 134: 9fee52cb6d upload-pack.c: allow banning certain object filter(s)\n @@ Commit message\n Allow configuring 'git upload-pack' to support object filters on a\n case-by-case basis by introducing two new configuration variables:\n\n - - 'uploadpack.filter.allow'\n - - 'uploadpack.filter..allow'\n + - 'uploadpackfilter.allow'\n + - 'uploadpackfilter..allow'\n\n where '' may be one of 'blobNone', 'blobLimit', 'tree', and so on.\n\n @@ Commit message\n\n If a client requests the object filter and the respective\n configuration value is not set, 'git upload-pack' will default to the\n - value of 'uploadpack.filter.allow', which itself defaults to 'true' to\n + value of 'uploadpackfilter.allow', which itself defaults to 'true' to\n maintain backwards compatibility. Note that this differs from\n 'uploadpack.allowfilter', which controls whether or not the 'filter'\n capability is advertised.\n @@ Documentation/config/uploadpack.txt: 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 ++uploadpackfilter.allow::\n +\tProvides a default value for unspecified object filters (see: the\n +\tbelow configuration variable).\n +\tDefaults to `true`.\n +\n -+uploadpack.filter..allow::\n ++uploadpackfilter..allow::\n +\tExplicitly allow or ban the object filter corresponding to\n +\t``, where `` 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 '' 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 ++\tkinds must be allowed. Defaults to `uploadpackfilter.allow`.\n +\n uploadpack.allowRefInWant::\n \tIf this option is set, `upload-pack` will support the `ref-in-want`\n @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil\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_config -C srv.bare uploadpackfilter.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_config -C srv.bare uploadpackfilter.allow false &&\n ++\ttest_config -C srv.bare uploadpackfilter.combine.allow true &&\n ++\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n ++\ttest_config -C srv.bare uploadpackfilter.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_config -C srv.bare uploadpackfilter.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 @@ upload-pack.c: static int process_deepen_not(const char *line, struct string_lis\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 ++\tdie(_(\"git upload-pack: filter '%s' not supported\"),\n ++\t list_object_filter_config_name(banned->choice));\n +}\n +\n static void receive_needs(struct upload_pack_data *data,\n @@ upload-pack.c: static int find_symref(const char *refname, const struct object_i\n \treturn 0;\n }\n\n -+static void parse_object_filter_config(const char *var, const char *value,\n ++static int 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 ++\tstruct strbuf buf = 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 ++\tif (parse_config_key(var, \"uploadpackfilter\", &sub, &sub_len, &key))\n ++\t\treturn 0;\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 ++\tif (!sub) {\n ++\t\tif (!strcmp(key, \"allow\"))\n +\t\t\tdata->allow_filter_fallback = git_config_bool(var, value);\n ++\t\treturn 0;\n +\t}\n +\n -+\tstrbuf_release(&spec);\n ++\tstrbuf_add(&buf, sub, sub_len);\n ++\n ++\tif (!strcmp(key, \"allow\"))\n ++\t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n ++\t\t\t(void *)(intptr_t)git_config_bool(var, value);\n ++\n ++\tstrbuf_release(&buf);\n ++\treturn 0;\n +}\n +\n static int upload_pack_config(const char *var, const char *value, void *cb_data)\n 3: 3434bd5979 = 135: 550e4e13f1 upload-pack.c: pass 'struct list_objects_filter_options *'\n 4: 9fa765a71d ! 136: bb008f7427 upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'\n @@ Metadata\n Author: Taylor Blau \n\n ## Commit message ##\n - upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'\n + upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'\n\n In b79cf959b2 (upload-pack.c: allow banning certain object filter(s),\n 2020-02-26), we introduced functionality to disallow certain object\n @@ Commit message\n\n While it would be sufficient to simply write\n\n - $ git config uploadpack.filter.tree.allow true\n + $ git config uploadpackfilter.tree.allow true\n\n (since it would allow all values of 'n'), we would like to be able to\n allow this filter for certain values of 'n', i.e., those no greater than\n @@ Commit message\n\n In order to do this, introduce a new configuration key, as follows:\n\n - $ git config uploadpack.filter.tree.maxDepth \n + $ git config uploadpackfilter.tree.maxDepth \n\n where '' specifies the maximum allowed value of 'n' in the filter\n 'tree:n'. Administrators who wish to allow for only the value '0' can\n write:\n\n - $ git config uploadpack.filter.tree.allow true\n - $ git config uploadpack.filter.tree.maxDepth 0\n + $ git config uploadpackfilter.tree.allow true\n + $ git config uploadpackfilter.tree.maxDepth 0\n\n which allows '--filter=tree:0', but no other values.\n\n @@ Commit message\n Signed-off-by: Taylor Blau \n\n ## Documentation/config/uploadpack.txt ##\n -@@ Documentation/config/uploadpack.txt: Note that the dot between 'filter' and '' 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 +@@ Documentation/config/uploadpack.txt: uploadpackfilter..allow::\n + \tcombined filters, both `combine` and all of the nested filter\n + \tkinds must be allowed. Defaults to `uploadpackfilter.allow`.\n\n -+uploadpack.filter.tree.maxDepth::\n ++uploadpackfilter.tree.maxDepth::\n +\tOnly allow `--filter=tree=` 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 ++\t`uploadpackfilter.tree.maxDepth`. If set, this also implies\n ++\t`uploadpackfilter.tree.allow=true`, unless this configuration\n +\tvariable had already been set. Has no effect if unset.\n +\n uploadpack.allowRefInWant::\n @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object f\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_config -C srv.bare uploadpackfilter.allow false &&\n ++\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n ++\ttest_config -C srv.bare uploadpackfilter.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 @@ upload-pack.c: static void die_if_using_banned_filter(struct upload_pack_data *d\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: filter '%s' not supported\"),\n +-\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 @@ upload-pack.c: static void die_if_using_banned_filter(struct upload_pack_data *d\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 ++\tdie(\"%s\", buf.buf);\n }\n\n -@@ upload-pack.c: 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 + static void receive_needs(struct upload_pack_data *data,\n +@@ upload-pack.c: static int parse_object_filter_config(const char *var, const char *value,\n + \tif (!strcmp(key, \"allow\"))\n + \t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n + \t\t\t(void *)(intptr_t)git_config_bool(var, value);\n ++\telse if (!strcmp(buf.buf, \"tree\") && !strcmp(key, \"maxdepth\")) {\n ++\t\tif (!value) {\n ++\t\t\tstrbuf_release(&buf);\n ++\t\t\treturn config_error_nonbool(var);\n ++\t\t}\n ++\t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n ++\t\t\t(void *)(intptr_t)1;\n +\t\tdata->tree_filter_max_depth = git_config_ulong(var, value);\n - \t}\n ++\t}\n\n - \tstrbuf_release(&spec);\n + \tstrbuf_release(&buf);\n + \treturn 0;\n--\n2.28.0.rc1.13.ge78abce653\n"},{"id":"401912","messageId":"69605299bb38a678338365cf05178ce0735c7c67.1595468659.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1595468657.git.me@ttaylorr.com","subject":"[PATCH v2 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-23T01:48:59Z","receivedAt":"2020-07-23T01:49:03Z","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 \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.28.0.rc1.13.ge78abce653\n\n"},{"id":"401915","messageId":"9fee52cb6dbc5eb7157a145eb6764b35b6a0e2f3.1595468659.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1595468657.git.me@ttaylorr.com","subject":"[PATCH v2 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-23T01:49:03Z","receivedAt":"2020-07-23T01:49:08Z","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 - 'uploadpackfilter.allow'\n - 'uploadpackfilter..allow'\n\nwhere '' may be one of 'blobNone', 'blobLimit', 'tree', and so on.\n\nSetting the second configuration variable for any valid value of\n'' explicitly allows or disallows restricting that kind of object\nfilter.\n\nIf a client requests the object filter and the respective\nconfiguration value is not set, 'git upload-pack' will default to the\nvalue of 'uploadpackfilter.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 \n---\n Documentation/config/uploadpack.txt | 12 +++++\n t/t5616-partial-clone.sh | 24 +++++++++\n upload-pack.c | 76 +++++++++++++++++++++++++++++\n 3 files changed, 112 insertions(+)\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex ed1c835695..fffe8ac648 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -57,6 +57,18 @@ uploadpack.allowFilter::\n \tIf this option is set, `upload-pack` will support partial\n \tclone and partial fetch object filtering.\n \n+uploadpackfilter.allow::\n+\tProvides a default value for unspecified object filters (see: the\n+\tbelow configuration variable).\n+\tDefaults to `true`.\n+\n+uploadpackfilter..allow::\n+\tExplicitly allow or ban the object filter corresponding to\n+\t``, where `` 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 `uploadpackfilter.allow`.\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..b196ee694c 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -235,6 +235,30 @@ 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+\ttest_config -C srv.bare uploadpackfilter.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 uploadpackfilter.allow false &&\n+\ttest_config -C srv.bare uploadpackfilter.combine.allow true &&\n+\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n+\ttest_config -C srv.bare uploadpackfilter.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 uploadpackfilter.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 951a2b23aa..61929977ab 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,46 @@ 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+\tdie(_(\"git upload-pack: filter '%s' not supported\"),\n+\t list_object_filter_config_name(banned->choice));\n+}\n+\n static void receive_needs(struct upload_pack_data *data,\n \t\t\t struct packet_reader *reader)\n {\n@@ -1013,6 +1059,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@@ -1170,6 +1217,32 @@ static int find_symref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n+static int parse_object_filter_config(const char *var, const char *value,\n+\t\t\t\t struct upload_pack_data *data)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *sub, *key;\n+\tsize_t sub_len;\n+\n+\tif (parse_config_key(var, \"uploadpackfilter\", &sub, &sub_len, &key))\n+\t\treturn 0;\n+\n+\tif (!sub) {\n+\t\tif (!strcmp(key, \"allow\"))\n+\t\t\tdata->allow_filter_fallback = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n+\tstrbuf_add(&buf, sub, sub_len);\n+\n+\tif (!strcmp(key, \"allow\"))\n+\t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n+\t\t\t(void *)(intptr_t)git_config_bool(var, value);\n+\n+\tstrbuf_release(&buf);\n+\treturn 0;\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@@ -1209,6 +1282,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@@ -1389,6 +1464,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.28.0.rc1.13.ge78abce653\n\n"},{"id":"401913","messageId":"550e4e13f16016b219445376fdcd75da87d60c65.1595468659.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1595468657.git.me@ttaylorr.com","subject":"[PATCH v2 3/4] upload-pack.c: pass 'struct list_objects_filter_options *'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-23T01:49:07Z","receivedAt":"2020-07-23T01:49:11Z","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 \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 61929977ab..48f10d21f6 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.28.0.rc1.13.ge78abce653\n\n"},{"id":"401914","messageId":"bb008f7427cfe4f0a5c85af6c6f821f48a534360.1595468659.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1595468657.git.me@ttaylorr.com","subject":"[PATCH v2 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-23T01:49:11Z","receivedAt":"2020-07-23T01:49:15Z","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:' 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 uploadpackfilter.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 uploadpackfilter.tree.maxDepth \n\nwhere '' 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 uploadpackfilter.tree.allow true\n $ git config uploadpackfilter.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 \n---\n Documentation/config/uploadpack.txt | 6 ++++++\n t/t5616-partial-clone.sh | 8 +++++++\n upload-pack.c | 33 ++++++++++++++++++++++++++---\n 3 files changed, 44 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex fffe8ac648..ee7b3ac94f 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -69,6 +69,12 @@ uploadpackfilter..allow::\n \tcombined filters, both `combine` and all of the nested filter\n \tkinds must be allowed. Defaults to `uploadpackfilter.allow`.\n \n+uploadpackfilter.tree.maxDepth::\n+\tOnly allow `--filter=tree=` when `n` is no more than the value of\n+\t`uploadpackfilter.tree.maxDepth`. If set, this also implies\n+\t`uploadpackfilter.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 b196ee694c..4292a644d7 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -259,6 +259,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 uploadpackfilter.allow false &&\n+\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n+\ttest_config -C srv.bare uploadpackfilter.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 48f10d21f6..47cdaae265 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,18 @@ 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-\tdie(_(\"git upload-pack: filter '%s' not supported\"),\n-\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+\tdie(\"%s\", buf.buf);\n }\n \n static void receive_needs(struct upload_pack_data *data,\n@@ -1238,6 +1256,15 @@ static int parse_object_filter_config(const char *var, const char *value,\n \tif (!strcmp(key, \"allow\"))\n \t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n \t\t\t(void *)(intptr_t)git_config_bool(var, value);\n+\telse if (!strcmp(buf.buf, \"tree\") && !strcmp(key, \"maxdepth\")) {\n+\t\tif (!value) {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn config_error_nonbool(var);\n+\t\t}\n+\t\tstring_list_insert(&data->allowed_filters, buf.buf)->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(&buf);\n \treturn 0;\n-- \n2.28.0.rc1.13.ge78abce653\n"},{"id":"401961","messageId":"20200723204325.GM11341@szeder.dev","threadId":"53910","inReplyTo":"cover.1595468657.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/4] upload-pack: custom allowed object filters","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-07-23T20:43:25Z","receivedAt":"2020-07-23T20:43:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:\n> Here's a much-delayed v2 of my series to teach upload-pack to limit the\n> kinds of object filters that it is willing to server in a request.\n> \n> Much is the same since last time, with two notable exceptions:\n> \n> - We use the 'uploadpackfilter' top-level configuration key instead of\n> pretending that 'uploadpack.filter' is top-level, which greatly\n> simplifies the call to 'parse_config_key()'.\n> \n> - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,\n\nTo clarify, I only recommended to pass the same message to die() as in\nthe ERR packet, not dropping the ERR packet, because ...\n\n> which propagates the error through 'git clone' always,\n\nit does in the new tests when creating a local clone, but does it\nreally work with all protocols and remote helpers and what not?\n\n> and resolves\n> a flaky set of tests that used to result in a SIGPIPE.\n\nThis doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'\ncan still abort while 'git clone' is still sending packets. IOW we\nstill need that 'test_must_fail ok=sigpipe' in all new tests.\n\n"},{"id":"402006","messageId":"20200724165133.GA15287@syl.lan","threadId":"53910","inReplyTo":"20200723204325.GM11341@szeder.dev","subject":"Re: [PATCH v2 0/4] upload-pack: custom allowed object filters","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-24T16:51:33Z","receivedAt":"2020-07-24T16:51:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:\n> On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:\n> > Here's a much-delayed v2 of my series to teach upload-pack to limit the\n> > kinds of object filters that it is willing to server in a request.\n> >\n> > Much is the same since last time, with two notable exceptions:\n> >\n> > - We use the 'uploadpackfilter' top-level configuration key instead of\n> > pretending that 'uploadpack.filter' is top-level, which greatly\n> > simplifies the call to 'parse_config_key()'.\n> >\n> > - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,\n>\n> To clarify, I only recommended to pass the same message to die() as in\n> the ERR packet, not dropping the ERR packet, because ...\n>\n> > which propagates the error through 'git clone' always,\n>\n> it does in the new tests when creating a local clone, but does it\n> really work with all protocols and remote helpers and what not?\n>\n> > and resolves\n> > a flaky set of tests that used to result in a SIGPIPE.\n>\n> This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'\n> can still abort while 'git clone' is still sending packets. IOW we\n> still need that 'test_must_fail ok=sigpipe' in all new tests.\n\nLet me double check my understanding... I think that you are suggesting\nthe following three things:\n\n - Write the same message as an err packet over the wire as we do when\n 'die()'ing from inside of upload-pack.c\n\n - Don't mark said message(s) for translation, matching what we do in\n the rest of upload-pack.c.\n\n - Re-introduce the 'test_must_fail ok=sigpipe' and stop grepping\n stderr for the right message.\n\nDo I have that right?\n\nThanks,\nTaylor\n"},{"id":"402016","messageId":"20200724195126.GB4013174@coredump.intra.peff.net","threadId":"53910","inReplyTo":"20200724165133.GA15287@syl.lan","subject":"Re: [PATCH v2 0/4] upload-pack: custom allowed object filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-24T19:51:26Z","receivedAt":"2020-07-24T19:51:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:\n\n> Let me double check my understanding... I think that you are suggesting\n> the following three things:\n> \n> - Write the same message as an err packet over the wire as we do when\n> 'die()'ing from inside of upload-pack.c\n> \n> - Don't mark said message(s) for translation, matching what we do in\n> the rest of upload-pack.c.\n> \n> - Re-introduce the 'test_must_fail ok=sigpipe' and stop grepping\n> stderr for the right message.\n> \n> Do I have that right?\n\nI'm not Gábor, but that is the sequence I think is best.\n\nBetween the die() and the ERR, the ERR packets are way more useful in\npractice, since they actually go back to the client. I'd even suggest we\ndo away with the die() messages entirely (since they're either redundant\nor go nowhere, depending on the protocol), but I think it would make\nsense to wait until the raciness issues are fixed (until then, they\n_might_ help in the redundant cases, which is what the test here is\nrelying on).\n\n-Peff\n"},{"id":"402132","messageId":"20200727142511.GA62919@syl.lan","threadId":"53910","inReplyTo":"20200724195126.GB4013174@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/4] upload-pack: custom allowed object filters","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-27T14:25:11Z","receivedAt":"2020-07-27T14:25:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 24, 2020 at 03:51:26PM -0400, Jeff King wrote:\n> On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:\n>\n> > Let me double check my understanding... I think that you are suggesting\n> > the following three things:\n> >\n> > - Write the same message as an err packet over the wire as we do when\n> > 'die()'ing from inside of upload-pack.c\n> >\n> > - Don't mark said message(s) for translation, matching what we do in\n> > the rest of upload-pack.c.\n> >\n> > - Re-introduce the 'test_must_fail ok=sigpipe' and stop grepping\n> > stderr for the right message.\n> >\n> > Do I have that right?\n>\n> I'm not Gábor, but that is the sequence I think is best.\n>\n> Between the die() and the ERR, the ERR packets are way more useful in\n> practice, since they actually go back to the client. I'd even suggest we\n> do away with the die() messages entirely (since they're either redundant\n> or go nowhere, depending on the protocol), but I think it would make\n> sense to wait until the raciness issues are fixed (until then, they\n> _might_ help in the redundant cases, which is what the test here is\n> relying on).\n\nAll sounds good. I'll wait for an ACK from Gábor to make sure that this\nis the direction that they want, too.\n\nIn the meantime, this is what I have locally. I'll send a series which\nhas this as its range-diff if Gábor approves of the direction.\n\n1: 6a77af563e = 1: 6a77af563e list_objects_filter_options: introduce 'list_object_filter_config_name'\n2: 6dbf58441d ! 2: 80d19481d8 upload-pack.c: allow banning certain object filter(s)\n @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil\n\n +test_expect_success 'upload-pack fails banned object filters' '\n +\ttest_config -C srv.bare uploadpackfilter.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 ++\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \\\n ++\t\t\"file://$(pwd)/srv.bare\" pc3\n +'\n +\n +test_expect_success 'upload-pack fails banned combine object filters' '\n @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil\n +\ttest_config -C srv.bare uploadpackfilter.combine.allow true &&\n +\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n +\ttest_config -C srv.bare uploadpackfilter.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 ++\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n ++\t\t--filter=blob:none \"file://$(pwd)/srv.bare\" pc3\n +'\n +\n +test_expect_success 'upload-pack fails banned object filters with fallback' '\n +\ttest_config -C srv.bare uploadpackfilter.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 ++\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \\\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 @@ upload-pack.c: static int process_deepen_not(const char *line, struct string_lis\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 -+\tdie(_(\"git upload-pack: filter '%s' not supported\"),\n -+\t list_object_filter_config_name(banned->choice));\n ++\tstrbuf_addf(&buf, \"git upload-pack: filter '%s' not supported\",\n ++\t\t list_object_filter_config_name(banned->choice));\n ++\n ++\tpacket_writer_error(&data->writer, \"%s\\n\", buf.buf);\n ++\tdie(\"%s\", buf.buf);\n +}\n +\n static void receive_needs(struct upload_pack_data *data,\n3: bacdea47d9 = 3: 790552c7c2 upload-pack.c: pass 'struct list_objects_filter_options *'\n4: 79af94a41b ! 4: eb9a81eb1f upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'\n @@ Documentation/config/uploadpack.txt: uploadpackfilter..allow::\n\n ## t/t5616-partial-clone.sh ##\n @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object filters with fallback' '\n - \ttest_i18ngrep \"filter '\\''blob:none'\\'' not supported\" err\n + \t\t\"file://$(pwd)/srv.bare\" pc3\n '\n\n +test_expect_success 'upload-pack limits tree depth filters' '\n @@ upload-pack.c: static int allows_filter_choice(struct upload_pack_data *data,\n }\n\n @@ upload-pack.c: 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 --\tdie(_(\"git upload-pack: filter '%s' not supported\"),\n --\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 + \tstrbuf_addf(&buf, \"git upload-pack: 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 -+\tdie(\"%s\", buf.buf);\n - }\n\n - static void receive_needs(struct upload_pack_data *data,\n + \tpacket_writer_error(&data->writer, \"%s\\n\", buf.buf);\n + \tdie(\"%s\", buf.buf);\n @@ upload-pack.c: static int parse_object_filter_config(const char *var, const char *value,\n \tif (!strcmp(key, \"allow\"))\n \t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"402153","messageId":"20200727193432.GO11341@szeder.dev","threadId":"53910","inReplyTo":"20200724165133.GA15287@syl.lan","subject":"Re: [PATCH v2 0/4] upload-pack: custom allowed object filters","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-07-27T19:34:32Z","receivedAt":"2020-07-27T19:34:39Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:\n> On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:\n> > On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:\n> > > Here's a much-delayed v2 of my series to teach upload-pack to limit the\n> > > kinds of object filters that it is willing to server in a request.\n> > >\n> > > Much is the same since last time, with two notable exceptions:\n> > >\n> > > - We use the 'uploadpackfilter' top-level configuration key instead of\n> > > pretending that 'uploadpack.filter' is top-level, which greatly\n> > > simplifies the call to 'parse_config_key()'.\n> > >\n> > > - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,\n> >\n> > To clarify, I only recommended to pass the same message to die() as in\n> > the ERR packet, not dropping the ERR packet, because ...\n> >\n> > > which propagates the error through 'git clone' always,\n> >\n> > it does in the new tests when creating a local clone, but does it\n> > really work with all protocols and remote helpers and what not?\n> >\n> > > and resolves\n> > > a flaky set of tests that used to result in a SIGPIPE.\n> >\n> > This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'\n> > can still abort while 'git clone' is still sending packets. IOW we\n> > still need that 'test_must_fail ok=sigpipe' in all new tests.\n> \n> Let me double check my understanding... I think that you are suggesting\n> the following three things:\n> \n> - Write the same message as an err packet over the wire as we do when\n> 'die()'ing from inside of upload-pack.c\n\nYes, though I'm not quite sure that I understand this sentence\ncorrectly, and unless you can convincingly argue in the commit message\nthat the die() messages make it to the client no matter the\nprotocol.\n\n> - Don't mark said message(s) for translation, matching what we do in\n> the rest of upload-pack.c.\n\nYes.\n\n> - Re-introduce the 'test_must_fail ok=sigpipe'\n\nYes.\n\n> and stop grepping stderr for the right message.\n\nNo, please check them, I love those error messages :)\n\n> Do I have that right?\n> \n> Thanks,\n> Taylor\n"},{"id":"402155","messageId":"20200727193657.GC62919@syl.lan","threadId":"53910","inReplyTo":"20200727193432.GO11341@szeder.dev","subject":"Re: [PATCH v2 0/4] upload-pack: custom allowed object filters","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-27T19:36:57Z","receivedAt":"2020-07-27T19:37:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jul 27, 2020 at 09:34:32PM +0200, SZEDER Gábor wrote:\n> On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:\n> > On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:\n> > > On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:\n> > > > Here's a much-delayed v2 of my series to teach upload-pack to limit the\n> > > > kinds of object filters that it is willing to server in a request.\n> > > >\n> > > > Much is the same since last time, with two notable exceptions:\n> > > >\n> > > > - We use the 'uploadpackfilter' top-level configuration key instead of\n> > > > pretending that 'uploadpack.filter' is top-level, which greatly\n> > > > simplifies the call to 'parse_config_key()'.\n> > > >\n> > > > - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,\n> > >\n> > > To clarify, I only recommended to pass the same message to die() as in\n> > > the ERR packet, not dropping the ERR packet, because ...\n> > >\n> > > > which propagates the error through 'git clone' always,\n> > >\n> > > it does in the new tests when creating a local clone, but does it\n> > > really work with all protocols and remote helpers and what not?\n> > >\n> > > > and resolves\n> > > > a flaky set of tests that used to result in a SIGPIPE.\n> > >\n> > > This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'\n> > > can still abort while 'git clone' is still sending packets. IOW we\n> > > still need that 'test_must_fail ok=sigpipe' in all new tests.\n> >\n> > Let me double check my understanding... I think that you are suggesting\n> > the following three things:\n> >\n> > - Write the same message as an err packet over the wire as we do when\n> > 'die()'ing from inside of upload-pack.c\n>\n> Yes, though I'm not quite sure that I understand this sentence\n> correctly, and unless you can convincingly argue in the commit message\n> that the die() messages make it to the client no matter the\n> protocol.\n>\n> > - Don't mark said message(s) for translation, matching what we do in\n> > the rest of upload-pack.c.\n>\n> Yes.\n>\n> > - Re-introduce the 'test_must_fail ok=sigpipe'\n>\n> Yes.\n>\n> > and stop grepping stderr for the right message.\n>\n> No, please check them, I love those error messages :)\n\nIsn't the problem that these messages only sometimes make it to the\nclient depending on what protocol is in use? If so, the right thing to\ndo would be to not grep for them, since it will make that test flakey.\n\nMaybe I'm misunderstanding you/the problem...\n\n> > Do I have that right?\n> >\n> > Thanks,\n> > Taylor\n\nThanks,\nTaylor\n"},{"id":"402156","messageId":"20200727194215.GB795313@coredump.intra.peff.net","threadId":"53910","inReplyTo":"20200727193657.GC62919@syl.lan","subject":"Re: [PATCH v2 0/4] upload-pack: custom allowed object filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-27T19:42:15Z","receivedAt":"2020-07-27T19:42:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 27, 2020 at 03:36:57PM -0400, Taylor Blau wrote:\n\n> > > and stop grepping stderr for the right message.\n> >\n> > No, please check them, I love those error messages :)\n> \n> Isn't the problem that these messages only sometimes make it to the\n> client depending on what protocol is in use? If so, the right thing to\n> do would be to not grep for them, since it will make that test flakey.\n> \n> Maybe I'm misunderstanding you/the problem...\n\nYou'd have to omit them for tests over certain protocols, but it would\nalways be robust to check them over local-filesystem clones and fetches.\nSo it may be that some tests need them and some do not.\n\n-Peff\n"},{"id":"402157","messageId":"20200727195912.GP11341@szeder.dev","threadId":"53910","inReplyTo":"20200727193657.GC62919@syl.lan","subject":"Re: [PATCH v2 0/4] upload-pack: custom allowed object filters","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-07-27T19:59:12Z","receivedAt":"2020-07-27T19:59:19Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jul 27, 2020 at 03:36:57PM -0400, Taylor Blau wrote:\n> On Mon, Jul 27, 2020 at 09:34:32PM +0200, SZEDER Gábor wrote:\n> > On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:\n> > > On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:\n> > > > On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:\n> > > > > Here's a much-delayed v2 of my series to teach upload-pack to limit the\n> > > > > kinds of object filters that it is willing to server in a request.\n> > > > >\n> > > > > Much is the same since last time, with two notable exceptions:\n> > > > >\n> > > > > - We use the 'uploadpackfilter' top-level configuration key instead of\n> > > > > pretending that 'uploadpack.filter' is top-level, which greatly\n> > > > > simplifies the call to 'parse_config_key()'.\n> > > > >\n> > > > > - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,\n> > > >\n> > > > To clarify, I only recommended to pass the same message to die() as in\n> > > > the ERR packet, not dropping the ERR packet, because ...\n> > > >\n> > > > > which propagates the error through 'git clone' always,\n> > > >\n> > > > it does in the new tests when creating a local clone, but does it\n> > > > really work with all protocols and remote helpers and what not?\n> > > >\n> > > > > and resolves\n> > > > > a flaky set of tests that used to result in a SIGPIPE.\n> > > >\n> > > > This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'\n> > > > can still abort while 'git clone' is still sending packets. IOW we\n> > > > still need that 'test_must_fail ok=sigpipe' in all new tests.\n> > >\n> > > Let me double check my understanding... I think that you are suggesting\n> > > the following three things:\n> > >\n> > > - Write the same message as an err packet over the wire as we do when\n> > > 'die()'ing from inside of upload-pack.c\n> >\n> > Yes, though I'm not quite sure that I understand this sentence\n> > correctly, and unless you can convincingly argue in the commit message\n> > that the die() messages make it to the client no matter the\n> > protocol.\n> >\n> > > - Don't mark said message(s) for translation, matching what we do in\n> > > the rest of upload-pack.c.\n> >\n> > Yes.\n> >\n> > > - Re-introduce the 'test_must_fail ok=sigpipe'\n> >\n> > Yes.\n> >\n> > > and stop grepping stderr for the right message.\n> >\n> > No, please check them, I love those error messages :)\n> \n> Isn't the problem that these messages only sometimes make it to the\n> client depending on what protocol is in use? If so, the right thing to\n> do would be to not grep for them, since it will make that test flakey.\n\nTo my understanding the die() messages always make it to the client's\nstderr when cloning from a local repository. And that's exactly what\nall new tests do, i.e they do run 'git clone ... file://', that's why\nI recommended to pass those fancy error messages to die(), because\nthat's easy enough to do, and doesn't require us to make 'git clone'\nSIGPIPE-proof first (which, I agree, is beyond the scope of this\nseries).\n\n> Maybe I'm misunderstanding you/the problem...\n> \n> > > Do I have that right?\n> > >\n> > > Thanks,\n> > > Taylor\n> \n> Thanks,\n> Taylor\n"},{"id":"402158","messageId":"20200727200355.GD62919@syl.lan","threadId":"53910","inReplyTo":"20200727195912.GP11341@szeder.dev","subject":"Re: [PATCH v2 0/4] upload-pack: custom allowed object filters","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-27T20:03:55Z","receivedAt":"2020-07-27T20:04:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jul 27, 2020 at 09:59:12PM +0200, SZEDER Gábor wrote:\n> On Mon, Jul 27, 2020 at 03:36:57PM -0400, Taylor Blau wrote:\n> > On Mon, Jul 27, 2020 at 09:34:32PM +0200, SZEDER Gábor wrote:\n> > > On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:\n> > > > On Thu, Jul 23, 2020 at 10:43:25PM +0200, SZEDER Gábor wrote:\n> > > > > On Wed, Jul 22, 2020 at 09:48:54PM -0400, Taylor Blau wrote:\n> > > > > > Here's a much-delayed v2 of my series to teach upload-pack to limit the\n> > > > > > kinds of object filters that it is willing to server in a request.\n> > > > > >\n> > > > > > Much is the same since last time, with two notable exceptions:\n> > > > > >\n> > > > > > - We use the 'uploadpackfilter' top-level configuration key instead of\n> > > > > > pretending that 'uploadpack.filter' is top-level, which greatly\n> > > > > > simplifies the call to 'parse_config_key()'.\n> > > > > >\n> > > > > > - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,\n> > > > >\n> > > > > To clarify, I only recommended to pass the same message to die() as in\n> > > > > the ERR packet, not dropping the ERR packet, because ...\n> > > > >\n> > > > > > which propagates the error through 'git clone' always,\n> > > > >\n> > > > > it does in the new tests when creating a local clone, but does it\n> > > > > really work with all protocols and remote helpers and what not?\n> > > > >\n> > > > > > and resolves\n> > > > > > a flaky set of tests that used to result in a SIGPIPE.\n> > > > >\n> > > > > This doesn't resolve the SIGPIPE flakiness, because 'git upload-pack'\n> > > > > can still abort while 'git clone' is still sending packets. IOW we\n> > > > > still need that 'test_must_fail ok=sigpipe' in all new tests.\n> > > >\n> > > > Let me double check my understanding... I think that you are suggesting\n> > > > the following three things:\n> > > >\n> > > > - Write the same message as an err packet over the wire as we do when\n> > > > 'die()'ing from inside of upload-pack.c\n> > >\n> > > Yes, though I'm not quite sure that I understand this sentence\n> > > correctly, and unless you can convincingly argue in the commit message\n> > > that the die() messages make it to the client no matter the\n> > > protocol.\n> > >\n> > > > - Don't mark said message(s) for translation, matching what we do in\n> > > > the rest of upload-pack.c.\n> > >\n> > > Yes.\n> > >\n> > > > - Re-introduce the 'test_must_fail ok=sigpipe'\n> > >\n> > > Yes.\n> > >\n> > > > and stop grepping stderr for the right message.\n> > >\n> > > No, please check them, I love those error messages :)\n> >\n> > Isn't the problem that these messages only sometimes make it to the\n> > client depending on what protocol is in use? If so, the right thing to\n> > do would be to not grep for them, since it will make that test flakey.\n>\n> To my understanding the die() messages always make it to the client's\n> stderr when cloning from a local repository. And that's exactly what\n> all new tests do, i.e they do run 'git clone ... file://', that's why\n> I recommended to pass those fancy error messages to die(), because\n> that's easy enough to do, and doesn't require us to make 'git clone'\n> SIGPIPE-proof first (which, I agree, is beyond the scope of this\n> series).\n\nAh, thanks for your patient explanation of what's going on here. These\ntests _would_ be flakey if run over non-local protocols, but they\naren't, so we can (and should) safely grep for the right error message.\n\nSo the things to do are the original list, nixing \"stop grepping stderr\nfor ...\". Easy ;-).\n\n> > Maybe I'm misunderstanding you/the problem...\n> >\n> > > > Do I have that right?\n> > > >\n> > > > Thanks,\n> > > > Taylor\n> >\n> > Thanks,\n> > Taylor\n\nThanks,\nTaylor\n"},{"id":"402589","messageId":"cover.1596227003.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1595468657.git.me@ttaylorr.com","subject":"[PATCH v3 0/4] upload-pack: custom allowed object filters","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-31T20:26:21Z","receivedAt":"2020-07-31T20:26:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere's the third re-roll of my series to teach upload-pack to allow only\ncertain kinds of object filters. The only thing that has changed since\nlast time is that we now *do* look for the error messages in t5616.\n\nNormally these error messages may or may not show up (because of the\naforementioned SIGPIPE issue in 'git clone'), but since we are cloning\nfrom a 'file://', we can guarantee that they will appear (and can thusly\nbe grepped for).\n\nTaylor Blau (4):\n list_objects_filter_options: introduce\n '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 'uploadpackfilter.tree.maxDepth'\n\n Documentation/config/uploadpack.txt | 18 +++++\n list-objects-filter-options.c | 23 ++++++\n list-objects-filter-options.h | 6 ++\n t/t5616-partial-clone.sh | 33 +++++++++\n upload-pack.c | 105 ++++++++++++++++++++++++++++\n 5 files changed, 185 insertions(+)\n\nRange-diff against v2:\n[...rebased on the tip of 'master']\n 1: 6a77af563e = 80: b1b3dd7de9 list_objects_filter_options: introduce 'list_object_filter_config_name'\n 2: 6dbf58441d ! 81: a0a0427757 upload-pack.c: allow banning certain object filter(s)\n @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil\n\n +test_expect_success 'upload-pack fails banned object filters' '\n +\ttest_config -C srv.bare uploadpackfilter.blob:none.allow false &&\n -+\ttest_must_fail git clone --no-checkout --filter=blob:none \\\n ++\ttest_must_fail ok=sigpipe 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 @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil\n +\ttest_config -C srv.bare uploadpackfilter.combine.allow true &&\n +\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n +\ttest_config -C srv.bare uploadpackfilter.blob:none.allow false &&\n -+\ttest_must_fail git clone --no-checkout --filter=tree:1 \\\n ++\ttest_must_fail ok=sigpipe 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 uploadpackfilter.allow false &&\n -+\ttest_must_fail git clone --no-checkout --filter=blob:none \\\n ++\ttest_must_fail ok=sigpipe 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 @@ upload-pack.c: static int process_deepen_not(const char *line, struct string_lis\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 -+\tdie(_(\"git upload-pack: filter '%s' not supported\"),\n -+\t list_object_filter_config_name(banned->choice));\n ++\tstrbuf_addf(&buf, \"git upload-pack: filter '%s' not supported\",\n ++\t\t list_object_filter_config_name(banned->choice));\n ++\n ++\tpacket_writer_error(&data->writer, \"%s\\n\", buf.buf);\n ++\tdie(\"%s\", buf.buf);\n +}\n +\n static void receive_needs(struct upload_pack_data *data,\n 3: bacdea47d9 = 82: ad3f0cce56 upload-pack.c: pass 'struct list_objects_filter_options *'\n 4: 79af94a41b ! 83: c9d71809f4 upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'\n @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object f\n +\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n +\ttest_config -C srv.bare uploadpackfilter.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 ++\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n ++\ttest_i18ngrep \"filter '\\''tree'\\'' not supported (maximum depth: 0, but got: 1)\" err\n +'\n +\n test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '\n @@ upload-pack.c: static int allows_filter_choice(struct upload_pack_data *data,\n }\n\n @@ upload-pack.c: 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 --\tdie(_(\"git upload-pack: filter '%s' not supported\"),\n --\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 + \tstrbuf_addf(&buf, \"git upload-pack: 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 -+\tdie(\"%s\", buf.buf);\n - }\n\n - static void receive_needs(struct upload_pack_data *data,\n + \tpacket_writer_error(&data->writer, \"%s\\n\", buf.buf);\n + \tdie(\"%s\", buf.buf);\n @@ upload-pack.c: static int parse_object_filter_config(const char *var, const char *value,\n \tif (!strcmp(key, \"allow\"))\n \t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n--\n2.28.0.rc1.13.ge78abce653\n"},{"id":"402590","messageId":"b1b3dd7de985dd12911ba2086048623097e96e3d.1596227003.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1596227003.git.me@ttaylorr.com","subject":"[PATCH v3 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-31T20:26:26Z","receivedAt":"2020-07-31T20:26:32Z","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 \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.28.0.rc1.13.ge78abce653\n\n"},{"id":"402591","messageId":"a0a04277578333daa0b38a8e06e1738b920f5ac3.1596227003.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1596227003.git.me@ttaylorr.com","subject":"[PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-31T20:26:31Z","receivedAt":"2020-07-31T20:26: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 - 'uploadpackfilter.allow'\n - 'uploadpackfilter..allow'\n\nwhere '' may be one of 'blobNone', 'blobLimit', 'tree', and so on.\n\nSetting the second configuration variable for any valid value of\n'' explicitly allows or disallows restricting that kind of object\nfilter.\n\nIf a client requests the object filter and the respective\nconfiguration value is not set, 'git upload-pack' will default to the\nvalue of 'uploadpackfilter.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 \n---\n Documentation/config/uploadpack.txt | 12 +++++\n t/t5616-partial-clone.sh | 24 +++++++++\n upload-pack.c | 80 +++++++++++++++++++++++++++++\n 3 files changed, 116 insertions(+)\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex ed1c835695..fffe8ac648 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -57,6 +57,18 @@ uploadpack.allowFilter::\n \tIf this option is set, `upload-pack` will support partial\n \tclone and partial fetch object filtering.\n \n+uploadpackfilter.allow::\n+\tProvides a default value for unspecified object filters (see: the\n+\tbelow configuration variable).\n+\tDefaults to `true`.\n+\n+uploadpackfilter..allow::\n+\tExplicitly allow or ban the object filter corresponding to\n+\t``, where `` 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 `uploadpackfilter.allow`.\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 37de0afb02..0d46b5a2f8 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -235,6 +235,30 @@ 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+\ttest_config -C srv.bare uploadpackfilter.blob:none.allow false &&\n+\ttest_must_fail ok=sigpipe 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 uploadpackfilter.allow false &&\n+\ttest_config -C srv.bare uploadpackfilter.combine.allow true &&\n+\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n+\ttest_config -C srv.bare uploadpackfilter.blob:none.allow false &&\n+\ttest_must_fail ok=sigpipe 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 uploadpackfilter.allow false &&\n+\ttest_must_fail ok=sigpipe 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 8673741070..ed2098edd0 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@@ -984,6 +990,50 @@ 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+\tstruct strbuf buf = STRBUF_INIT;\n+\tif (!banned)\n+\t\treturn;\n+\n+\tstrbuf_addf(&buf, \"git upload-pack: filter '%s' not supported\",\n+\t\t list_object_filter_config_name(banned->choice));\n+\n+\tpacket_writer_error(&data->writer, \"%s\\n\", buf.buf);\n+\tdie(\"%s\", buf.buf);\n+}\n+\n static void receive_needs(struct upload_pack_data *data,\n \t\t\t struct packet_reader *reader)\n {\n@@ -1014,6 +1064,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@@ -1171,6 +1222,32 @@ static int find_symref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n+static int parse_object_filter_config(const char *var, const char *value,\n+\t\t\t\t struct upload_pack_data *data)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *sub, *key;\n+\tsize_t sub_len;\n+\n+\tif (parse_config_key(var, \"uploadpackfilter\", &sub, &sub_len, &key))\n+\t\treturn 0;\n+\n+\tif (!sub) {\n+\t\tif (!strcmp(key, \"allow\"))\n+\t\t\tdata->allow_filter_fallback = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n+\tstrbuf_add(&buf, sub, sub_len);\n+\n+\tif (!strcmp(key, \"allow\"))\n+\t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n+\t\t\t(void *)(intptr_t)git_config_bool(var, value);\n+\n+\tstrbuf_release(&buf);\n+\treturn 0;\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@@ -1210,6 +1287,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@@ -1390,6 +1469,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.28.0.rc1.13.ge78abce653\n\n"},{"id":"402592","messageId":"ad3f0cce56bf205ae718b0f3be46b91ef0285492.1596227003.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1596227003.git.me@ttaylorr.com","subject":"[PATCH v3 3/4] upload-pack.c: pass 'struct list_objects_filter_options *'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-31T20:26:35Z","receivedAt":"2020-07-31T20:26:40Z","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 \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 ed2098edd0..5fa22da31f 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -991,9 +991,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@@ -1007,7 +1007,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.28.0.rc1.13.ge78abce653\n\n"},{"id":"402593","messageId":"c9d71809f49dc1b600b75769e0b6aae17382bf4e.1596227003.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1596227003.git.me@ttaylorr.com","subject":"[PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-31T20:26:39Z","receivedAt":"2020-07-31T20:26:45Z","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:' 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 uploadpackfilter.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 uploadpackfilter.tree.maxDepth \n\nwhere '' 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 uploadpackfilter.tree.allow true\n $ git config uploadpackfilter.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 \n---\n Documentation/config/uploadpack.txt | 6 ++++++\n t/t5616-partial-clone.sh | 9 +++++++++\n upload-pack.c | 27 ++++++++++++++++++++++++++-\n 3 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex fffe8ac648..ee7b3ac94f 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -69,6 +69,12 @@ uploadpackfilter..allow::\n \tcombined filters, both `combine` and all of the nested filter\n \tkinds must be allowed. Defaults to `uploadpackfilter.allow`.\n \n+uploadpackfilter.tree.maxDepth::\n+\tOnly allow `--filter=tree=` when `n` is no more than the value of\n+\t`uploadpackfilter.tree.maxDepth`. If set, this also implies\n+\t`uploadpackfilter.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 0d46b5a2f8..35cb6a34a3 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -259,6 +259,15 @@ 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 uploadpackfilter.allow false &&\n+\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n+\ttest_config -C srv.bare uploadpackfilter.tree.maxDepth 0 &&\n+\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n+\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n+\ttest_i18ngrep \"filter '\\''tree'\\'' not supported (maximum depth: 0, but got: 1)\" 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 5fa22da31f..131445b212 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@@ -996,8 +998,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@@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data)\n \n \tstrbuf_addf(&buf, \"git upload-pack: 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 \tdie(\"%s\", buf.buf);\n@@ -1243,6 +1259,15 @@ static int parse_object_filter_config(const char *var, const char *value,\n \tif (!strcmp(key, \"allow\"))\n \t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n \t\t\t(void *)(intptr_t)git_config_bool(var, value);\n+\telse if (!strcmp(buf.buf, \"tree\") && !strcmp(key, \"maxdepth\")) {\n+\t\tif (!value) {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn config_error_nonbool(var);\n+\t\t}\n+\t\tstring_list_insert(&data->allowed_filters, buf.buf)->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(&buf);\n \treturn 0;\n-- \n2.28.0.rc1.13.ge78abce653\n"},{"id":"402598","messageId":"20200731205434.GB1440890@coredump.intra.peff.net","threadId":"53910","inReplyTo":"a0a04277578333daa0b38a8e06e1738b920f5ac3.1596227003.git.me@ttaylorr.com","subject":"Re: [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-31T20:54:34Z","receivedAt":"2020-07-31T20:54:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 31, 2020 at 04:26:31PM -0400, Taylor Blau wrote:\n\n> Git clients may ask the server for a partial set of objects, where the\n> set of objects being requested is refined by one or more object filters.\n> Server administrators can configure 'git upload-pack' to allow or ban\n> these filters by setting the 'uploadpack.allowFilter' variable to\n> 'true' or 'false', respectively.\n> \n> However, administrators using bitmaps may wish to allow certain kinds of\n> object filters, but ban others. Specifically, they may wish to allow\n> object filters that can be optimized by the use of bitmaps, while\n> rejecting other object filters which aren't and represent a perceived\n> performance degradation (as well as an increased load factor on the\n> server).\n> \n> Allow configuring 'git upload-pack' to support object filters on a\n> case-by-case basis by introducing two new configuration variables:\n> \n> - 'uploadpackfilter.allow'\n> - 'uploadpackfilter..allow'\n> \n> where '' may be one of 'blobNone', 'blobLimit', 'tree', and so on.\n\nMinor nit, but is \"blob:none\", \"blob:limit\", etc. The code and\ndocumentation gets this right; it's just the commit message.\n\nI'm pretty sure this is a casualty of updating the syntax as the series\nwas developed. One trick I use is to avoid repeating explanations that\nare in the documentation from the patch already. I.e., explain \"why\"\nhere, but it's OK to let \"what\" come from the patch itself. That's not\nonly one less thing to remember to update, but it's less for reviewers\nto read through, too.\n\n\n\n> +test_expect_success 'upload-pack fails banned object filters' '\n> +\ttest_config -C srv.bare uploadpackfilter.blob:none.allow false &&\n> +\ttest_must_fail ok=sigpipe 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 messages aren't translated now, so we can just use grep, I think.\n\nDitto in the other tests below.\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> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tif (!banned)\n> +\t\treturn;\n> +\n> +\tstrbuf_addf(&buf, \"git upload-pack: filter '%s' not supported\",\n> +\t\t list_object_filter_config_name(banned->choice));\n> +\n> +\tpacket_writer_error(&data->writer, \"%s\\n\", buf.buf);\n> +\tdie(\"%s\", buf.buf);\n> +}\n\nHmm, the strbuf was unexpected. I'd have just written it out twice.\nAfter all, the messages don't have to be the same. And perhaps we don't\nwant them to be the same? A user receiving the ERR packet would see:\n\n remote error: git upload-pack: filter 'foo' not supported\n\ndo we need the \"git upload-pack\" part there? Other errors say just\n\"upload-pack\". IMHO even that is unnecessarily verbose, and I wouldn't\nmind a separate patch to reduce it. But definitely going even longer\ndoesn't seem like the right direction. :)\n\nI also wondered about the trailing newline. Other callers of\npacket_writer_error() don't seem to use it. I think in practice it\ndoesn't matter much because readers will generally be using\nCHOMP_NEWLINE, but it probably makes sense to be consistent.\n\n> [...]\n\nAside from this nits, this patch looks good to me.\n\n-Peff\n"},{"id":"402599","messageId":"20200731210114.GC1440890@coredump.intra.peff.net","threadId":"53910","inReplyTo":"c9d71809f49dc1b600b75769e0b6aae17382bf4e.1596227003.git.me@ttaylorr.com","subject":"Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-31T21:01:14Z","receivedAt":"2020-07-31T21:01:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 31, 2020 at 04:26:39PM -0400, Taylor Blau wrote:\n\n> +test_expect_success 'upload-pack limits tree depth filters' '\n> +\ttest_config -C srv.bare uploadpackfilter.allow false &&\n> +\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n> +\ttest_config -C srv.bare uploadpackfilter.tree.maxDepth 0 &&\n> +\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n> +\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n> +\ttest_i18ngrep \"filter '\\''tree'\\'' not supported (maximum depth: 0, but got: 1)\" err\n> +'\n\nSame i18ngrep comment as in the earlier patch (i.e., we can use grep\nhere).\n\n> @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data)\n> \n> \tstrbuf_addf(&buf, \"git upload-pack: 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\nHmm. So I see now why you wanted to go with the strbuf in the earlier\npatch. This does still feel awkward, though. You check \"is it allowed\"\nin an earlier function, we get \"nope, it's not allowed\", and now we have\nto reimplement the check here. That seems like a maintenance burden.\n\nI think a more natural flow would be either:\n\n - the \"is it allowed\" functions calls immediately into the function\n that sends the error and dies (this might need a conditional if\n there's a caller who doesn't want to die; I didn't check)\n\nor\n\n - on failure it populates an error buffer itself, which the caller can\n then pass along as it sees fit\n\n-Peff\n"},{"id":"402600","messageId":"20200731212032.GD3409@syl.lan","threadId":"53910","inReplyTo":"20200731205434.GB1440890@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-31T21:20:32Z","receivedAt":"2020-07-31T21:20:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 31, 2020 at 04:54:34PM -0400, Jeff King wrote:\n> On Fri, Jul 31, 2020 at 04:26:31PM -0400, Taylor Blau wrote:\n>\n> > Git clients may ask the server for a partial set of objects, where the\n> > set of objects being requested is refined by one or more object filters.\n> > Server administrators can configure 'git upload-pack' to allow or ban\n> > these filters by setting the 'uploadpack.allowFilter' variable to\n> > 'true' or 'false', respectively.\n> >\n> > However, administrators using bitmaps may wish to allow certain kinds of\n> > object filters, but ban others. Specifically, they may wish to allow\n> > object filters that can be optimized by the use of bitmaps, while\n> > rejecting other object filters which aren't and represent a perceived\n> > performance degradation (as well as an increased load factor on the\n> > server).\n> >\n> > Allow configuring 'git upload-pack' to support object filters on a\n> > case-by-case basis by introducing two new configuration variables:\n> >\n> > - 'uploadpackfilter.allow'\n> > - 'uploadpackfilter..allow'\n> >\n> > where '' may be one of 'blobNone', 'blobLimit', 'tree', and so on.\n>\n> Minor nit, but is \"blob:none\", \"blob:limit\", etc. The code and\n> documentation gets this right; it's just the commit message.\n>\n> I'm pretty sure this is a casualty of updating the syntax as the series\n> was developed. One trick I use is to avoid repeating explanations that\n> are in the documentation from the patch already. I.e., explain \"why\"\n> here, but it's OK to let \"what\" come from the patch itself. That's not\n> only one less thing to remember to update, but it's less for reviewers\n> to read through, too.\n>\n> \n\nGood advice, and you're right that is blob:none and similar, not\n'blobNone'. In this case, I think the \"why\" is pretty boring, so I don't\nmind repeating myself a little bit in the commit message.\n>\n> > +test_expect_success 'upload-pack fails banned object filters' '\n> > +\ttest_config -C srv.bare uploadpackfilter.blob:none.allow false &&\n> > +\ttest_must_fail ok=sigpipe 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> These messages aren't translated now, so we can just use grep, I think.\n>\n> Ditto in the other tests below.\n\nAck, yep.\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> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tif (!banned)\n> > +\t\treturn;\n> > +\n> > +\tstrbuf_addf(&buf, \"git upload-pack: filter '%s' not supported\",\n> > +\t\t list_object_filter_config_name(banned->choice));\n> > +\n> > +\tpacket_writer_error(&data->writer, \"%s\\n\", buf.buf);\n> > +\tdie(\"%s\", buf.buf);\n> > +}\n>\n> Hmm, the strbuf was unexpected. I'd have just written it out twice.\n> After all, the messages don't have to be the same. And perhaps we don't\n> want them to be the same? A user receiving the ERR packet would see:\n>\n> remote error: git upload-pack: filter 'foo' not supported\n>\n> do we need the \"git upload-pack\" part there? Other errors say just\n> \"upload-pack\". IMHO even that is unnecessarily verbose, and I wouldn't\n> mind a separate patch to reduce it. But definitely going even longer\n> doesn't seem like the right direction. :)\n\nLet's drop 'git upload-pack: ' entirely, and just start the message with\n'filter ...'. It looks like you figured out why we use a strbuf here in\nyour response to the fourth patch.\n\n> I also wondered about the trailing newline. Other callers of\n> packet_writer_error() don't seem to use it. I think in practice it\n> doesn't matter much because readers will generally be using\n> CHOMP_NEWLINE, but it probably makes sense to be consistent.\n\nDropping the newline should be easy enough.\n\n>\n> > [...]\n>\n> Aside from this nits, this patch looks good to me.\n>\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"402601","messageId":"20200731212243.GA1443086@coredump.intra.peff.net","threadId":"53910","inReplyTo":"20200731210114.GC1440890@coredump.intra.peff.net","subject":"Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-31T21:22:43Z","receivedAt":"2020-07-31T21:22:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote:\n\n> Hmm. So I see now why you wanted to go with the strbuf in the earlier\n> patch. This does still feel awkward, though. You check \"is it allowed\"\n> in an earlier function, we get \"nope, it's not allowed\", and now we have\n> to reimplement the check here. That seems like a maintenance burden.\n> \n> I think a more natural flow would be either:\n> \n> - the \"is it allowed\" functions calls immediately into the function\n> that sends the error and dies (this might need a conditional if\n> there's a caller who doesn't want to die; I didn't check)\n> \n> or\n> \n> - on failure it populates an error buffer itself, which the caller can\n> then pass along as it sees fit\n\nThe first one is easy to do, because there's no other caller. Worth it?\n\n-- >8 --\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 131445b212..574a447d5c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -992,62 +992,63 @@ 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\tstruct list_objects_filter_options *opts)\n+/* probably this helper could be used in lots more places */\n+NORETURN __attribute__((format(printf,2,3)))\n+static void send_err_and_die(struct upload_pack_data *data,\n+\t\t\t const char *fmt, ...)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tva_list ap;\n+\n+\t/* yuck, buf not necessary if we had va_list versions of our helpers */\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(&buf, fmt, ap);\n+\tva_end(ap);\n+\n+\tpacket_writer_error(&data->writer, \"%s\", buf.buf);\n+\tdie(\"%s\", buf.buf);\n+}\n+\n+static void check_one_filter(struct upload_pack_data *data,\n+\t\t\t struct list_objects_filter_options *opts)\n {\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+\tint allowed;\n \tif (item)\n \t\tallowed = (intptr_t) item->util;\n+\telse\n+\t\tallowed = data->allow_filter_fallback;\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+\tif (!allowed)\n+\t\tsend_err_and_die(data, \"filter '%s' not supported\",\n+\t\t\t\t list_object_filter_config_name(opts->choice));\n \n-\tif (allowed > -1)\n-\t\treturn allowed;\n-\treturn data->allow_filter_fallback;\n+\tif (opts->choice == LOFC_TREE_DEPTH &&\n+\t opts->tree_exclude_depth > data->tree_filter_max_depth)\n+\t\tsend_err_and_die(data,\n+\t\t\t\t \"tree filter allows max depth %lu, but got %lu\",\n+\t\t\t\t data->tree_filter_max_depth,\n+\t\t\t\t opts->tree_exclude_depth);\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+static void check_filter_recurse(struct upload_pack_data *data,\n+\t\t\t\t struct list_objects_filter_options *opts)\n {\n \tsize_t i;\n \n-\tif (!allows_filter_choice(data, opts))\n-\t\treturn opts;\n+\tcheck_one_filter(data, 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\tcheck_filter_recurse(data, &opts->sub[i]);\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-\tstruct strbuf buf = STRBUF_INIT;\n-\tif (!banned)\n-\t\treturn;\n-\n-\tstrbuf_addf(&buf, \"git upload-pack: 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-\tdie(\"%s\", buf.buf);\n+\tcheck_filter_recurse(data, &data->filter_options);\n }\n \n static void receive_needs(struct upload_pack_data *data,\n"},{"id":"402602","messageId":"20200731212905.GE3409@syl.lan","threadId":"53910","inReplyTo":"20200731210114.GC1440890@coredump.intra.peff.net","subject":"Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-31T21:29:05Z","receivedAt":"2020-07-31T21:29:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote:\n> On Fri, Jul 31, 2020 at 04:26:39PM -0400, Taylor Blau wrote:\n>\n> > +test_expect_success 'upload-pack limits tree depth filters' '\n> > +\ttest_config -C srv.bare uploadpackfilter.allow false &&\n> > +\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n> > +\ttest_config -C srv.bare uploadpackfilter.tree.maxDepth 0 &&\n> > +\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n> > +\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n> > +\ttest_i18ngrep \"filter '\\''tree'\\'' not supported (maximum depth: 0, but got: 1)\" err\n> > +'\n>\n> Same i18ngrep comment as in the earlier patch (i.e., we can use grep\n> here).\n>\n> > @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data)\n> >\n> > \tstrbuf_addf(&buf, \"git upload-pack: 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> Hmm. So I see now why you wanted to go with the strbuf in the earlier\n> patch. This does still feel awkward, though. You check \"is it allowed\"\n> in an earlier function, we get \"nope, it's not allowed\", and now we have\n> to reimplement the check here. That seems like a maintenance burden.\n\nI'm not sure that I follow. Is the earlier function that you're\nreferring to 'banned_filter'? If so, the only use of that function is\nwithin 'die_if_using_banned_filter'. 'banned_filter' exists only insofar\nas to answer the question \"return me the first banned filter, if one\nexists, or NULL otherwise\".\n\nThen, dying here is as simple as (1) lookup the banned filter, and (2)\ncheck if it's NULL or not.\n\nIf you're referring to 'allows_filter_choice', I guess I see what you're\ngetting it, but to be honest I'm not sure if I'm buying it. If we were\nto combine 'allows_filter_choice', 'banned_filter', and\n'die_if_using_banned_filter' into one function that traversed the filter\ntree and 'die()'d as soon as it got to a banned one, that function would\nhave to know how to:\n\n 1. Recurse through the tree when it hits a LOFC_COMBINE node.\n\n 2. At each node, translate the filter->choice into the appropriate key\n name, look it up, and then figure out how to interpret its allowed\n status (including falling back to the default if unspecified).\n\n 3. And, it would have to figure out how to format the message at each\n step.\n\n(3) I think is made easier, since we know what message to format based\non whether or not we're in the 'opts->choice == LOFC_TREE_DEPTH' arm or\nnot. But, there are two more things that we now have to cram into that\nsame function.\n\nMaybe I'm being too strict an adherent to having simpler functions, but\nI'm failing to see how to combine these in a way that's cleaner than\nwhat's written here.\n\n> I think a more natural flow would be either:\n>\n> - the \"is it allowed\" functions calls immediately into the function\n> that sends the error and dies (this might need a conditional if\n> there's a caller who doesn't want to die; I didn't check)\n>\n> or\n>\n> - on failure it populates an error buffer itself, which the caller can\n> then pass along as it sees fit\n\nI guess; I'm not a huge fan of the side-effecting nature, but maybe it's\ncleaner.\n\nI dunno. I appreciate your review, but I feel like we're in a bikeshed.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"402603","messageId":"20200731213038.GF3409@syl.lan","threadId":"53910","inReplyTo":"20200731212243.GA1443086@coredump.intra.peff.net","subject":"Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-07-31T21:30:38Z","receivedAt":"2020-07-31T21:30:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 31, 2020 at 05:22:43PM -0400, Jeff King wrote:\n> On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote:\n>\n> > Hmm. So I see now why you wanted to go with the strbuf in the earlier\n> > patch. This does still feel awkward, though. You check \"is it allowed\"\n> > in an earlier function, we get \"nope, it's not allowed\", and now we have\n> > to reimplement the check here. That seems like a maintenance burden.\n> >\n> > I think a more natural flow would be either:\n> >\n> > - the \"is it allowed\" functions calls immediately into the function\n> > that sends the error and dies (this might need a conditional if\n> > there's a caller who doesn't want to die; I didn't check)\n> >\n> > or\n> >\n> > - on failure it populates an error buffer itself, which the caller can\n> > then pass along as it sees fit\n>\n> The first one is easy to do, because there's no other caller. Worth it?\n>\n> -- >8 --\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 131445b212..574a447d5c 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -992,62 +992,63 @@ 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\tstruct list_objects_filter_options *opts)\n> +/* probably this helper could be used in lots more places */\n> +NORETURN __attribute__((format(printf,2,3)))\n> +static void send_err_and_die(struct upload_pack_data *data,\n> +\t\t\t const char *fmt, ...)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tva_list ap;\n> +\n> +\t/* yuck, buf not necessary if we had va_list versions of our helpers */\n> +\tva_start(ap, fmt);\n> +\tstrbuf_vaddf(&buf, fmt, ap);\n> +\tva_end(ap);\n> +\n> +\tpacket_writer_error(&data->writer, \"%s\", buf.buf);\n> +\tdie(\"%s\", buf.buf);\n> +}\n> +\n> +static void check_one_filter(struct upload_pack_data *data,\n> +\t\t\t struct list_objects_filter_options *opts)\n> {\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> +\tint allowed;\n> \tif (item)\n> \t\tallowed = (intptr_t) item->util;\n> +\telse\n> +\t\tallowed = data->allow_filter_fallback;\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> +\tif (!allowed)\n> +\t\tsend_err_and_die(data, \"filter '%s' not supported\",\n> +\t\t\t\t list_object_filter_config_name(opts->choice));\n>\n> -\tif (allowed > -1)\n> -\t\treturn allowed;\n> -\treturn data->allow_filter_fallback;\n> +\tif (opts->choice == LOFC_TREE_DEPTH &&\n> +\t opts->tree_exclude_depth > data->tree_filter_max_depth)\n> +\t\tsend_err_and_die(data,\n> +\t\t\t\t \"tree filter allows max depth %lu, but got %lu\",\n> +\t\t\t\t data->tree_filter_max_depth,\n> +\t\t\t\t opts->tree_exclude_depth);\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> +static void check_filter_recurse(struct upload_pack_data *data,\n> +\t\t\t\t struct list_objects_filter_options *opts)\n> {\n> \tsize_t i;\n>\n> -\tif (!allows_filter_choice(data, opts))\n> -\t\treturn opts;\n> +\tcheck_one_filter(data, 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\tcheck_filter_recurse(data, &opts->sub[i]);\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> -\tstruct strbuf buf = STRBUF_INIT;\n> -\tif (!banned)\n> -\t\treturn;\n> -\n> -\tstrbuf_addf(&buf, \"git upload-pack: 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> -\tdie(\"%s\", buf.buf);\n> +\tcheck_filter_recurse(data, &data->filter_options);\n> }\n>\n> static void receive_needs(struct upload_pack_data *data,\n\nI think that we crossed emails, but I'm still not convinced that this is\nany cleaner than what I wrote. Yes, it's a maintenance problem if we add\nmore filter-specific logic like what we have in the LOFC_TREE_DEPTH\ncase, but I feel like we're bending over backwards in the meantime to\naccommodate a problem that we don't have.\n\nThanks,\nTaylor\n"},{"id":"402604","messageId":"20200731213604.GA1457058@coredump.intra.peff.net","threadId":"53910","inReplyTo":"20200731212905.GE3409@syl.lan","subject":"Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-31T21:36:04Z","receivedAt":"2020-07-31T21:36:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 31, 2020 at 05:29:05PM -0400, Taylor Blau wrote:\n\n> > > @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data)\n> > >\n> > > \tstrbuf_addf(&buf, \"git upload-pack: 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> > Hmm. So I see now why you wanted to go with the strbuf in the earlier\n> > patch. This does still feel awkward, though. You check \"is it allowed\"\n> > in an earlier function, we get \"nope, it's not allowed\", and now we have\n> > to reimplement the check here. That seems like a maintenance burden.\n> \n> I'm not sure that I follow. Is the earlier function that you're\n> referring to 'banned_filter'? If so, the only use of that function is\n> within 'die_if_using_banned_filter'. 'banned_filter' exists only insofar\n> as to answer the question \"return me the first banned filter, if one\n> exists, or NULL otherwise\".\n> \n> Then, dying here is as simple as (1) lookup the banned filter, and (2)\n> check if it's NULL or not.\n> \n> If you're referring to 'allows_filter_choice', I guess I see what you're\n> getting it, but to be honest I'm not sure if I'm buying it.\n\nYeah, it's allows_filter_choice() that knows \"if it's a tree we must\ncheck the depth\". And now die_if_using_banned_filter() needs to know\nthat, too. The policy is implemented twice.\n\nI do appreciate that the way you've written it means that if somebody\nforgets to update die_if_using_banned_filter() to match the logic in\nallows_filter_choice(), we'd at least still die, just with a less good\nerror message. But it seems better still not to require the two to match\nin the first place.\n\n> If we were\n> to combine 'allows_filter_choice', 'banned_filter', and\n> 'die_if_using_banned_filter' into one function that traversed the filter\n> tree and 'die()'d as soon as it got to a banned one, that function would\n> have to know how to:\n> \n> 1. Recurse through the tree when it hits a LOFC_COMBINE node.\n> \n> 2. At each node, translate the filter->choice into the appropriate key\n> name, look it up, and then figure out how to interpret its allowed\n> status (including falling back to the default if unspecified).\n> \n> 3. And, it would have to figure out how to format the message at each\n> step.\n> \n> (3) I think is made easier, since we know what message to format based\n> on whether or not we're in the 'opts->choice == LOFC_TREE_DEPTH' arm or\n> not. But, there are two more things that we now have to cram into that\n> same function.\n\nYou can still split those things into functions; see the patch I posted.\n\n> Maybe I'm being too strict an adherent to having simpler functions, but\n> I'm failing to see how to combine these in a way that's cleaner than\n> what's written here.\n\nTo me this is less about \"clean\" and more about \"don't ever duplicate\npolicy code\". I don't mind duplicating boilerplate, but introducing a\nplace where somebody touching function X must remember to also touch Y\n(and gets no compiler support to remind them) is a bad thing. I guess\nyou can call that \"clean\", but I'd take longer or more functions as a\ntradeoff to avoid that.\n\nMy suggested patch does introduce more side effects. I think that's OK\nbecause there really is only a single caller here. But if you wanted it\ncleaner, then I think having allows_filter_choice() fill out an error\nstrbuf would eliminate my concern without drastically altering the flow\nof your code.\n\n-Peff\n"},{"id":"402605","messageId":"20200731214311.GB1457058@coredump.intra.peff.net","threadId":"53910","inReplyTo":"20200731213604.GA1457058@coredump.intra.peff.net","subject":"Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-31T21:43:11Z","receivedAt":"2020-07-31T21:43:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 31, 2020 at 05:36:04PM -0400, Jeff King wrote:\n\n> My suggested patch does introduce more side effects. I think that's OK\n> because there really is only a single caller here. But if you wanted it\n> cleaner, then I think having allows_filter_choice() fill out an error\n> strbuf would eliminate my concern without drastically altering the flow\n> of your code.\n\nThat could be something like the patch below. banned_filter() could\nswitch to returning a simple boolean, but I didn't do that here.\n\nI did (as with my other patch) reorder the logic in allows_filter_choice(),\nas I think it's much easier to follow by resolving \"allowed\" to the\nfallback earlier. That lets you handle \"not allowed at all\" first and\nearly-return (or die) before dealing with type-specific checks.\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 131445b212..86786f0e13 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -993,59 +993,60 @@ 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\tstruct list_objects_filter_options *opts)\n+\t\t\t\tstruct list_objects_filter_options *opts,\n+\t\t\t\tstruct strbuf *err)\n {\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+\tint allowed;\n \tif (item)\n \t\tallowed = (intptr_t) item->util;\n+\telse\n+\t\tallowed = data->allow_filter_fallback;\n \n-\tif (allowed != 0 &&\n-\t opts->choice == LOFC_TREE_DEPTH &&\n-\t opts->tree_exclude_depth > data->tree_filter_max_depth)\n+\tif (!allowed) {\n+\t\tstrbuf_addf(err, \"filter '%s' not supported\", key);\n \t\treturn 0;\n+\t}\n+\n+\tif (opts->choice == LOFC_TREE_DEPTH &&\n+\t opts->tree_exclude_depth > data->tree_filter_max_depth) {\n+\t\tstrbuf_addf(err, \"tree filter maximum depth is %lu, but got %lu\",\n+\t\t\t data->tree_filter_max_depth, opts->tree_exclude_depth);\n+\t\treturn 0;\n+\t}\n \n-\tif (allowed > -1)\n-\t\treturn allowed;\n-\treturn data->allow_filter_fallback;\n+\treturn 1;\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+\tstruct list_objects_filter_options *opts,\n+\tstruct strbuf *err)\n {\n \tsize_t i;\n \n-\tif (!allows_filter_choice(data, opts))\n+\tif (!allows_filter_choice(data, opts, err))\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\tif (banned_filter(data, sub, err))\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 \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct list_objects_filter_options *banned =\n+\t\tbanned_filter(data, &data->filter_options, &buf);\n \tif (!banned)\n \t\treturn;\n \n-\tstrbuf_addf(&buf, \"git upload-pack: 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 \tdie(\"%s\", buf.buf);\n }\n"},{"id":"402709","messageId":"cover.1596476928.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1595468657.git.me@ttaylorr.com","subject":"[PATCH v4 0/3] upload-pack: custom allowed object filters","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-03T18:00:04Z","receivedAt":"2020-08-03T18:00:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nHere's what I anticipate to be the final reroll of my series to teach\nthe new 'uploadpackfilter' configuration section, which allows for more\nfine-grained control over which object filters upload-pack is willing to\nserve.\n\nTwo changes from last time:\n\n - I adopted Peff's suggestion in beginning in [1], but appropriately\n split it over the existing patch structure.\n\n - I dropped the old patch 3/4, since it really should have never been\n there in the first place, and just made the refactoring more noisy\n than necessary.\n\n (For the curious, this patch was written as a preparatory step\n within GitHub's fork in order to add the\n 'uploadpackfilter.tree.maxDepth' configuration. This was after the\n initial work when I hadn't yet considered adding such a thing. Now\n that we know the full arc, it makes sense to just pass the right\n parameters from the get-go).\n\nThanks again for all of the review!\n\n[1] :https://lore.kernel.org/git/20200731210114.GC1440890@coredump.intra.peff.net/\n\nTaylor Blau (3):\n list_objects_filter_options: introduce\n 'list_object_filter_config_name'\n upload-pack.c: allow banning certain object filter(s)\n upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'\n\n Documentation/config/uploadpack.txt | 18 +++++\n list-objects-filter-options.c | 23 ++++++\n list-objects-filter-options.h | 6 ++\n t/t5616-partial-clone.sh | 33 +++++++++\n upload-pack.c | 104 ++++++++++++++++++++++++++++\n 5 files changed, 184 insertions(+)\n\nRange-diff against v3:\n-: ---------- > 1: 21531927e4 Revert \"fmt-merge-msg: stop treating `master` specially\"\n-: ---------- > 2: 6e6029a82a fmt-merge-msg: allow merge destination to be omitted again\n-: ---------- > 3: 25429fed5c refs: move the logic to add \\t to reflog to the files backend\n-: ---------- > 4: 3db796c1c0 t6300: fix issues related to %(contents:size)\n-: ---------- > 5: 85b4e0a6dc Third batch\n1: b1b3dd7de9 = 6: f4c7771875 list_objects_filter_options: introduce 'list_object_filter_config_name'\n2: a0a0427757 ! 7: b34f4eaed9 upload-pack.c: allow banning certain object filter(s)\n @@ Commit message\n 'uploadpack.allowfilter', which controls whether or not the 'filter'\n capability is advertised.\n\n + Helped-by: Jeff King \n Signed-off-by: Taylor Blau \n\n ## Documentation/config/uploadpack.txt ##\n @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil\n +\ttest_config -C srv.bare uploadpackfilter.blob:none.allow false &&\n +\ttest_must_fail ok=sigpipe 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 ++\tgrep \"filter '\\''blob:none'\\'' not supported\" err\n +'\n +\n +test_expect_success 'upload-pack fails banned combine object filters' '\n @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil\n +\ttest_config -C srv.bare uploadpackfilter.blob:none.allow false &&\n +\ttest_must_fail ok=sigpipe 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 ++\tgrep \"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 uploadpackfilter.allow false &&\n +\ttest_must_fail ok=sigpipe 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 ++\tgrep \"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 @@ upload-pack.c: static int process_deepen_not(const char *line, struct string_lis\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 ++NORETURN __attribute__((format(printf,2,3)))\n ++static void send_err_and_die(struct upload_pack_data *data,\n ++\t\t\t const char *fmt, ...)\n +{\n -+\tconst char *key = list_object_filter_config_name(c);\n ++\tstruct strbuf buf = STRBUF_INIT;\n ++\tva_list ap;\n ++\n ++\tva_start(ap, fmt);\n ++\tstrbuf_vaddf(&buf, fmt, ap);\n ++\tva_end(ap);\n ++\n ++\tpacket_writer_error(&data->writer, \"%s\", buf.buf);\n ++\tdie(\"%s\", buf.buf);\n ++}\n ++\n ++static void check_one_filter(struct upload_pack_data *data,\n ++\t\t\t struct list_objects_filter_options *opts)\n ++{\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;\n ++\n +\tif (item)\n -+\t\treturn (intptr_t) item->util;\n -+\treturn data->allow_filter_fallback;\n ++\t\tallowed = (intptr_t)item->util;\n ++\telse\n ++\t\tallowed = data->allow_filter_fallback;\n ++\n ++\tif (!allowed)\n ++\t\tsend_err_and_die(data, \"filter '%s' not supported\", key);\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 ++static void check_filter_recurse(struct upload_pack_data *data,\n ++\t\t\t\t struct 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 -+\tstruct strbuf buf = STRBUF_INIT;\n -+\tif (!banned)\n ++\tcheck_one_filter(data, opts);\n ++\tif (opts->choice != LOFC_COMBINE)\n +\t\treturn;\n +\n -+\tstrbuf_addf(&buf, \"git upload-pack: filter '%s' not supported\",\n -+\t\t list_object_filter_config_name(banned->choice));\n ++\tfor (i = 0; i < opts->sub_nr; i++)\n ++\t\tcheck_filter_recurse(data, &opts->sub[i]);\n ++}\n +\n -+\tpacket_writer_error(&data->writer, \"%s\\n\", buf.buf);\n -+\tdie(\"%s\", buf.buf);\n ++static void die_if_using_banned_filter(struct upload_pack_data *data)\n ++{\n ++\tcheck_filter_recurse(data, &data->filter_options);\n +}\n +\n static void receive_needs(struct upload_pack_data *data,\n3: ad3f0cce56 < -: ---------- upload-pack.c: pass 'struct list_objects_filter_options *'\n4: c9d71809f4 ! 8: a0e7731a55 upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'\n @@ Commit message\n\n which allows '--filter=tree:0', but no other values.\n\n - Unfortunately, since the tree depth is an unsigned long, we can't use,\n - say, -1 as a sentinel value, and so we must also keep track of \"have we\n - set this\" as well as \"to what value\".\n -\n Signed-off-by: Taylor Blau \n\n ## Documentation/config/uploadpack.txt ##\n @@ Documentation/config/uploadpack.txt: uploadpackfilter..allow::\n\n ## t/t5616-partial-clone.sh ##\n @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object filters with fallback' '\n - \ttest_i18ngrep \"filter '\\''blob:none'\\'' not supported\" err\n + \tgrep \"filter '\\''blob:none'\\'' not supported\" err\n '\n\n +test_expect_success 'upload-pack limits tree depth filters' '\n @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object f\n +\ttest_config -C srv.bare uploadpackfilter.tree.maxDepth 0 &&\n +\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n +\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n -+\ttest_i18ngrep \"filter '\\''tree'\\'' not supported (maximum depth: 0, but got: 1)\" err\n ++\tgrep \"tree filter allows max depth 0, but got 1\" err\n +'\n +\n test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' '\n @@ upload-pack.c: static void upload_pack_data_init(struct upload_pack_data *data)\n \tpacket_writer_init(&data->writer, 1);\n\n \tdata->keepalive = 5;\n -@@ upload-pack.c: 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 +@@ upload-pack.c: static void check_one_filter(struct upload_pack_data *data,\n +\n + \tif (!allowed)\n + \t\tsend_err_and_die(data, \"filter '%s' not supported\", key);\n +\n -+\tif (allowed != 0 &&\n -+\t opts->choice == LOFC_TREE_DEPTH &&\n ++\tif (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 ++\t\tsend_err_and_die(data,\n ++\t\t\t\t \"tree filter allows max depth %lu, but got %lu\",\n ++\t\t\t\t data->tree_filter_max_depth,\n ++\t\t\t\t opts->tree_exclude_depth);\n }\n\n -@@ upload-pack.c: static void die_if_using_banned_filter(struct upload_pack_data *data)\n -\n - \tstrbuf_addf(&buf, \"git upload-pack: 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 - \tdie(\"%s\", buf.buf);\n + static void check_filter_recurse(struct upload_pack_data *data,\n @@ upload-pack.c: static int parse_object_filter_config(const char *var, const char *value,\n \tif (!strcmp(key, \"allow\"))\n \t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n--\n2.28.0.rc1.13.ge78abce653\n"},{"id":"402710","messageId":"b34f4eaed98817b38cffea7ff44c8111429790ee.1596476929.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1596476928.git.me@ttaylorr.com","subject":"[PATCH v4 2/3] upload-pack.c: allow banning certain object filter(s)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-03T18:00:10Z","receivedAt":"2020-08-03T18:00:14Z","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 - 'uploadpackfilter.allow'\n - 'uploadpackfilter..allow'\n\nwhere '' may be one of 'blobNone', 'blobLimit', 'tree', and so on.\n\nSetting the second configuration variable for any valid value of\n'' explicitly allows or disallows restricting that kind of object\nfilter.\n\nIf a client requests the object filter and the respective\nconfiguration value is not set, 'git upload-pack' will default to the\nvalue of 'uploadpackfilter.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\nHelped-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n Documentation/config/uploadpack.txt | 12 ++++\n t/t5616-partial-clone.sh | 24 ++++++++\n upload-pack.c | 86 +++++++++++++++++++++++++++++\n 3 files changed, 122 insertions(+)\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex ed1c835695..fffe8ac648 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -57,6 +57,18 @@ uploadpack.allowFilter::\n \tIf this option is set, `upload-pack` will support partial\n \tclone and partial fetch object filtering.\n \n+uploadpackfilter.allow::\n+\tProvides a default value for unspecified object filters (see: the\n+\tbelow configuration variable).\n+\tDefaults to `true`.\n+\n+uploadpackfilter..allow::\n+\tExplicitly allow or ban the object filter corresponding to\n+\t``, where `` 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 `uploadpackfilter.allow`.\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 37de0afb02..aa68782587 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -235,6 +235,30 @@ 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+\ttest_config -C srv.bare uploadpackfilter.blob:none.allow false &&\n+\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \\\n+\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n+\tgrep \"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 uploadpackfilter.allow false &&\n+\ttest_config -C srv.bare uploadpackfilter.combine.allow true &&\n+\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n+\ttest_config -C srv.bare uploadpackfilter.blob:none.allow false &&\n+\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n+\t\t--filter=blob:none \"file://$(pwd)/srv.bare\" pc3 2>err &&\n+\tgrep \"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 uploadpackfilter.allow false &&\n+\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \\\n+\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n+\tgrep \"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 8673741070..2098af8156 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@@ -984,6 +990,56 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,\n \treturn 0;\n }\n \n+NORETURN __attribute__((format(printf,2,3)))\n+static void send_err_and_die(struct upload_pack_data *data,\n+\t\t\t const char *fmt, ...)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tva_list ap;\n+\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(&buf, fmt, ap);\n+\tva_end(ap);\n+\n+\tpacket_writer_error(&data->writer, \"%s\", buf.buf);\n+\tdie(\"%s\", buf.buf);\n+}\n+\n+static void check_one_filter(struct upload_pack_data *data,\n+\t\t\t struct list_objects_filter_options *opts)\n+{\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;\n+\n+\tif (item)\n+\t\tallowed = (intptr_t)item->util;\n+\telse\n+\t\tallowed = data->allow_filter_fallback;\n+\n+\tif (!allowed)\n+\t\tsend_err_and_die(data, \"filter '%s' not supported\", key);\n+}\n+\n+static void check_filter_recurse(struct upload_pack_data *data,\n+\t\t\t\t struct list_objects_filter_options *opts)\n+{\n+\tsize_t i;\n+\n+\tcheck_one_filter(data, opts);\n+\tif (opts->choice != LOFC_COMBINE)\n+\t\treturn;\n+\n+\tfor (i = 0; i < opts->sub_nr; i++)\n+\t\tcheck_filter_recurse(data, &opts->sub[i]);\n+}\n+\n+static void die_if_using_banned_filter(struct upload_pack_data *data)\n+{\n+\tcheck_filter_recurse(data, &data->filter_options);\n+}\n+\n static void receive_needs(struct upload_pack_data *data,\n \t\t\t struct packet_reader *reader)\n {\n@@ -1014,6 +1070,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@@ -1171,6 +1228,32 @@ static int find_symref(const char *refname, const struct object_id *oid,\n \treturn 0;\n }\n \n+static int parse_object_filter_config(const char *var, const char *value,\n+\t\t\t\t struct upload_pack_data *data)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *sub, *key;\n+\tsize_t sub_len;\n+\n+\tif (parse_config_key(var, \"uploadpackfilter\", &sub, &sub_len, &key))\n+\t\treturn 0;\n+\n+\tif (!sub) {\n+\t\tif (!strcmp(key, \"allow\"))\n+\t\t\tdata->allow_filter_fallback = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n+\tstrbuf_add(&buf, sub, sub_len);\n+\n+\tif (!strcmp(key, \"allow\"))\n+\t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n+\t\t\t(void *)(intptr_t)git_config_bool(var, value);\n+\n+\tstrbuf_release(&buf);\n+\treturn 0;\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@@ -1210,6 +1293,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@@ -1390,6 +1475,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.28.0.rc1.13.ge78abce653\n\n"},{"id":"402711","messageId":"f4c777187559cf48b82132413b5a1e18dab90e93.1596476929.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1596476928.git.me@ttaylorr.com","subject":"[PATCH v4 1/3] list_objects_filter_options: introduce 'list_object_filter_config_name'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-03T18:00:14Z","receivedAt":"2020-08-03T18:00:20Z","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 \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.28.0.rc1.13.ge78abce653\n\n"},{"id":"402712","messageId":"a0e7731a55b6cda3e1dd549725692a2d4c9b0900.1596476929.git.me@ttaylorr.com","threadId":"53910","inReplyTo":"cover.1596476928.git.me@ttaylorr.com","subject":"[PATCH v4 3/3] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-03T18:00:17Z","receivedAt":"2020-08-03T18:00:25Z","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:' 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 uploadpackfilter.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 uploadpackfilter.tree.maxDepth \n\nwhere '' 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 uploadpackfilter.tree.allow true\n $ git config uploadpackfilter.tree.maxDepth 0\n\nwhich allows '--filter=tree:0', but no other values.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/uploadpack.txt | 6 ++++++\n t/t5616-partial-clone.sh | 9 +++++++++\n upload-pack.c | 18 ++++++++++++++++++\n 3 files changed, 33 insertions(+)\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex fffe8ac648..ee7b3ac94f 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -69,6 +69,12 @@ uploadpackfilter..allow::\n \tcombined filters, both `combine` and all of the nested filter\n \tkinds must be allowed. Defaults to `uploadpackfilter.allow`.\n \n+uploadpackfilter.tree.maxDepth::\n+\tOnly allow `--filter=tree=` when `n` is no more than the value of\n+\t`uploadpackfilter.tree.maxDepth`. If set, this also implies\n+\t`uploadpackfilter.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 aa68782587..96ab45feab 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -259,6 +259,15 @@ test_expect_success 'upload-pack fails banned object filters with fallback' '\n \tgrep \"filter '\\''blob:none'\\'' not supported\" err\n '\n \n+test_expect_success 'upload-pack limits tree depth filters' '\n+\ttest_config -C srv.bare uploadpackfilter.allow false &&\n+\ttest_config -C srv.bare uploadpackfilter.tree.allow true &&\n+\ttest_config -C srv.bare uploadpackfilter.tree.maxDepth 0 &&\n+\ttest_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \\\n+\t\t\"file://$(pwd)/srv.bare\" pc3 2>err &&\n+\tgrep \"tree filter allows max depth 0, but got 1\" 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 2098af8156..fbd2db04cd 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@@ -1020,6 +1022,13 @@ static void check_one_filter(struct upload_pack_data *data,\n \n \tif (!allowed)\n \t\tsend_err_and_die(data, \"filter '%s' not supported\", key);\n+\n+\tif (opts->choice == LOFC_TREE_DEPTH &&\n+\t opts->tree_exclude_depth > data->tree_filter_max_depth)\n+\t\tsend_err_and_die(data,\n+\t\t\t\t \"tree filter allows max depth %lu, but got %lu\",\n+\t\t\t\t data->tree_filter_max_depth,\n+\t\t\t\t opts->tree_exclude_depth);\n }\n \n static void check_filter_recurse(struct upload_pack_data *data,\n@@ -1249,6 +1258,15 @@ static int parse_object_filter_config(const char *var, const char *value,\n \tif (!strcmp(key, \"allow\"))\n \t\tstring_list_insert(&data->allowed_filters, buf.buf)->util =\n \t\t\t(void *)(intptr_t)git_config_bool(var, value);\n+\telse if (!strcmp(buf.buf, \"tree\") && !strcmp(key, \"maxdepth\")) {\n+\t\tif (!value) {\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn config_error_nonbool(var);\n+\t\t}\n+\t\tstring_list_insert(&data->allowed_filters, buf.buf)->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(&buf);\n \treturn 0;\n-- \n2.28.0.rc1.13.ge78abce653\n"},{"id":"402778","messageId":"20200804003722.GA2726931@coredump.intra.peff.net","threadId":"53910","inReplyTo":"cover.1596476928.git.me@ttaylorr.com","subject":"Re: [PATCH v4 0/3] upload-pack: custom allowed object filters","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-04T00:37:22Z","receivedAt":"2020-08-04T00:37:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 03, 2020 at 02:00:04PM -0400, Taylor Blau wrote:\n\n> Here's what I anticipate to be the final reroll of my series to teach\n> the new 'uploadpackfilter' configuration section, which allows for more\n> fine-grained control over which object filters upload-pack is willing to\n> serve.\n\nThanks, this version looks good to me.\n\n> Two changes from last time:\n> \n> - I adopted Peff's suggestion in beginning in [1], but appropriately\n> split it over the existing patch structure.\n\nThe adaptation looks good. The send_err_and_die() helper could possibly\nbe used for some of the packet_writer_error() callers, too. Somebody\ncould convert them on top if they want, but I'm not sure it's even worth\nit. IMHO the correct long-term direction is to convert those die() calls\ninto silent exits and let the ERR packet speak for itself. But we\nshouldn't do that until the client side is more robust against\npipe-death races.\n\n> - I dropped the old patch 3/4, since it really should have never been\n> there in the first place, and just made the refactoring more noisy\n> than necessary.\n\nYeah, I think folding it in to the first patch as you did makes the\nseries simpler to follow.\n\n-Peff\n"}]}