| Message ID | c9d71809f49dc1b600b75769e0b6aae17382bf4e.1596227003.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | upload-pack: custom allowed object filters | expand |
On Fri, Jul 31, 2020 at 04:26:39PM -0400, Taylor Blau wrote: > +test_expect_success 'upload-pack limits tree depth filters' ' > + test_config -C srv.bare uploadpackfilter.allow false && > + test_config -C srv.bare uploadpackfilter.tree.allow true && > + test_config -C srv.bare uploadpackfilter.tree.maxDepth 0 && > + test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \ > + "file://$(pwd)/srv.bare" pc3 2>err && > + test_i18ngrep "filter '\''tree'\'' not supported (maximum depth: 0, but got: 1)" err > +' Same i18ngrep comment as in the earlier patch (i.e., we can use grep here). > @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data) > > strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", > list_object_filter_config_name(banned->choice)); > + if (banned->choice == LOFC_TREE_DEPTH && > + data->tree_filter_max_depth != ULONG_MAX) > + strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"), > + data->tree_filter_max_depth, > + banned->tree_exclude_depth); Hmm. So I see now why you wanted to go with the strbuf in the earlier patch. This does still feel awkward, though. You check "is it allowed" in an earlier function, we get "nope, it's not allowed", and now we have to reimplement the check here. That seems like a maintenance burden. I think a more natural flow would be either: - the "is it allowed" functions calls immediately into the function that sends the error and dies (this might need a conditional if there's a caller who doesn't want to die; I didn't check) or - on failure it populates an error buffer itself, which the caller can then pass along as it sees fit -Peff
On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote: > Hmm. So I see now why you wanted to go with the strbuf in the earlier > patch. This does still feel awkward, though. You check "is it allowed" > in an earlier function, we get "nope, it's not allowed", and now we have > to reimplement the check here. That seems like a maintenance burden. > > I think a more natural flow would be either: > > - the "is it allowed" functions calls immediately into the function > that sends the error and dies (this might need a conditional if > there's a caller who doesn't want to die; I didn't check) > > or > > - on failure it populates an error buffer itself, which the caller can > then pass along as it sees fit The first one is easy to do, because there's no other caller. Worth it? -- >8 -- diff --git a/upload-pack.c b/upload-pack.c index 131445b212..574a447d5c 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -992,62 +992,63 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not, return 0; } -static int allows_filter_choice(struct upload_pack_data *data, - struct list_objects_filter_options *opts) +/* probably this helper could be used in lots more places */ +NORETURN __attribute__((format(printf,2,3))) +static void send_err_and_die(struct upload_pack_data *data, + const char *fmt, ...) +{ + struct strbuf buf = STRBUF_INIT; + va_list ap; + + /* yuck, buf not necessary if we had va_list versions of our helpers */ + va_start(ap, fmt); + strbuf_vaddf(&buf, fmt, ap); + va_end(ap); + + packet_writer_error(&data->writer, "%s", buf.buf); + die("%s", buf.buf); +} + +static void check_one_filter(struct upload_pack_data *data, + struct list_objects_filter_options *opts) { const char *key = list_object_filter_config_name(opts->choice); struct string_list_item *item = string_list_lookup(&data->allowed_filters, key); - int allowed = -1; + int allowed; if (item) allowed = (intptr_t) item->util; + else + allowed = data->allow_filter_fallback; - if (allowed != 0 && - opts->choice == LOFC_TREE_DEPTH && - opts->tree_exclude_depth > data->tree_filter_max_depth) - return 0; + if (!allowed) + send_err_and_die(data, "filter '%s' not supported", + list_object_filter_config_name(opts->choice)); - if (allowed > -1) - return allowed; - return data->allow_filter_fallback; + if (opts->choice == LOFC_TREE_DEPTH && + opts->tree_exclude_depth > data->tree_filter_max_depth) + send_err_and_die(data, + "tree filter allows max depth %lu, but got %lu", + data->tree_filter_max_depth, + opts->tree_exclude_depth); } -static struct list_objects_filter_options *banned_filter( - struct upload_pack_data *data, - struct list_objects_filter_options *opts) +static void check_filter_recurse(struct upload_pack_data *data, + struct list_objects_filter_options *opts) { size_t i; - if (!allows_filter_choice(data, opts)) - return opts; + check_one_filter(data, opts); if (opts->choice == LOFC_COMBINE) for (i = 0; i < opts->sub_nr; i++) { - struct list_objects_filter_options *sub = &opts->sub[i]; - if (banned_filter(data, sub)) - return sub; + check_filter_recurse(data, &opts->sub[i]); } - return NULL; } static void die_if_using_banned_filter(struct upload_pack_data *data) { - struct list_objects_filter_options *banned = banned_filter(data, - &data->filter_options); - struct strbuf buf = STRBUF_INIT; - if (!banned) - return; - - strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", - list_object_filter_config_name(banned->choice)); - if (banned->choice == LOFC_TREE_DEPTH && - data->tree_filter_max_depth != ULONG_MAX) - strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"), - data->tree_filter_max_depth, - banned->tree_exclude_depth); - - packet_writer_error(&data->writer, "%s\n", buf.buf); - die("%s", buf.buf); + check_filter_recurse(data, &data->filter_options); } static void receive_needs(struct upload_pack_data *data,
On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote: > On Fri, Jul 31, 2020 at 04:26:39PM -0400, Taylor Blau wrote: > > > +test_expect_success 'upload-pack limits tree depth filters' ' > > + test_config -C srv.bare uploadpackfilter.allow false && > > + test_config -C srv.bare uploadpackfilter.tree.allow true && > > + test_config -C srv.bare uploadpackfilter.tree.maxDepth 0 && > > + test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \ > > + "file://$(pwd)/srv.bare" pc3 2>err && > > + test_i18ngrep "filter '\''tree'\'' not supported (maximum depth: 0, but got: 1)" err > > +' > > Same i18ngrep comment as in the earlier patch (i.e., we can use grep > here). > > > @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data) > > > > strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", > > list_object_filter_config_name(banned->choice)); > > + if (banned->choice == LOFC_TREE_DEPTH && > > + data->tree_filter_max_depth != ULONG_MAX) > > + strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"), > > + data->tree_filter_max_depth, > > + banned->tree_exclude_depth); > > Hmm. So I see now why you wanted to go with the strbuf in the earlier > patch. This does still feel awkward, though. You check "is it allowed" > in an earlier function, we get "nope, it's not allowed", and now we have > to reimplement the check here. That seems like a maintenance burden. I'm not sure that I follow. Is the earlier function that you're referring to 'banned_filter'? If so, the only use of that function is within 'die_if_using_banned_filter'. 'banned_filter' exists only insofar as to answer the question "return me the first banned filter, if one exists, or NULL otherwise". Then, dying here is as simple as (1) lookup the banned filter, and (2) check if it's NULL or not. If you're referring to 'allows_filter_choice', I guess I see what you're getting it, but to be honest I'm not sure if I'm buying it. If we were to combine 'allows_filter_choice', 'banned_filter', and 'die_if_using_banned_filter' into one function that traversed the filter tree and 'die()'d as soon as it got to a banned one, that function would have to know how to: 1. Recurse through the tree when it hits a LOFC_COMBINE node. 2. At each node, translate the filter->choice into the appropriate key name, look it up, and then figure out how to interpret its allowed status (including falling back to the default if unspecified). 3. And, it would have to figure out how to format the message at each step. (3) I think is made easier, since we know what message to format based on whether or not we're in the 'opts->choice == LOFC_TREE_DEPTH' arm or not. But, there are two more things that we now have to cram into that same function. Maybe I'm being too strict an adherent to having simpler functions, but I'm failing to see how to combine these in a way that's cleaner than what's written here. > I think a more natural flow would be either: > > - the "is it allowed" functions calls immediately into the function > that sends the error and dies (this might need a conditional if > there's a caller who doesn't want to die; I didn't check) > > or > > - on failure it populates an error buffer itself, which the caller can > then pass along as it sees fit I guess; I'm not a huge fan of the side-effecting nature, but maybe it's cleaner. I dunno. I appreciate your review, but I feel like we're in a bikeshed. > -Peff Thanks, Taylor
On Fri, Jul 31, 2020 at 05:22:43PM -0400, Jeff King wrote: > On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote: > > > Hmm. So I see now why you wanted to go with the strbuf in the earlier > > patch. This does still feel awkward, though. You check "is it allowed" > > in an earlier function, we get "nope, it's not allowed", and now we have > > to reimplement the check here. That seems like a maintenance burden. > > > > I think a more natural flow would be either: > > > > - the "is it allowed" functions calls immediately into the function > > that sends the error and dies (this might need a conditional if > > there's a caller who doesn't want to die; I didn't check) > > > > or > > > > - on failure it populates an error buffer itself, which the caller can > > then pass along as it sees fit > > The first one is easy to do, because there's no other caller. Worth it? > > -- >8 -- > diff --git a/upload-pack.c b/upload-pack.c > index 131445b212..574a447d5c 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -992,62 +992,63 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not, > return 0; > } > > -static int allows_filter_choice(struct upload_pack_data *data, > - struct list_objects_filter_options *opts) > +/* probably this helper could be used in lots more places */ > +NORETURN __attribute__((format(printf,2,3))) > +static void send_err_and_die(struct upload_pack_data *data, > + const char *fmt, ...) > +{ > + struct strbuf buf = STRBUF_INIT; > + va_list ap; > + > + /* yuck, buf not necessary if we had va_list versions of our helpers */ > + va_start(ap, fmt); > + strbuf_vaddf(&buf, fmt, ap); > + va_end(ap); > + > + packet_writer_error(&data->writer, "%s", buf.buf); > + die("%s", buf.buf); > +} > + > +static void check_one_filter(struct upload_pack_data *data, > + struct list_objects_filter_options *opts) > { > const char *key = list_object_filter_config_name(opts->choice); > struct string_list_item *item = string_list_lookup(&data->allowed_filters, > key); > - int allowed = -1; > + int allowed; > if (item) > allowed = (intptr_t) item->util; > + else > + allowed = data->allow_filter_fallback; > > - if (allowed != 0 && > - opts->choice == LOFC_TREE_DEPTH && > - opts->tree_exclude_depth > data->tree_filter_max_depth) > - return 0; > + if (!allowed) > + send_err_and_die(data, "filter '%s' not supported", > + list_object_filter_config_name(opts->choice)); > > - if (allowed > -1) > - return allowed; > - return data->allow_filter_fallback; > + if (opts->choice == LOFC_TREE_DEPTH && > + opts->tree_exclude_depth > data->tree_filter_max_depth) > + send_err_and_die(data, > + "tree filter allows max depth %lu, but got %lu", > + data->tree_filter_max_depth, > + opts->tree_exclude_depth); > } > > -static struct list_objects_filter_options *banned_filter( > - struct upload_pack_data *data, > - struct list_objects_filter_options *opts) > +static void check_filter_recurse(struct upload_pack_data *data, > + struct list_objects_filter_options *opts) > { > size_t i; > > - if (!allows_filter_choice(data, opts)) > - return opts; > + check_one_filter(data, opts); > > if (opts->choice == LOFC_COMBINE) > for (i = 0; i < opts->sub_nr; i++) { > - struct list_objects_filter_options *sub = &opts->sub[i]; > - if (banned_filter(data, sub)) > - return sub; > + check_filter_recurse(data, &opts->sub[i]); > } > - return NULL; > } > > static void die_if_using_banned_filter(struct upload_pack_data *data) > { > - struct list_objects_filter_options *banned = banned_filter(data, > - &data->filter_options); > - struct strbuf buf = STRBUF_INIT; > - if (!banned) > - return; > - > - strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", > - list_object_filter_config_name(banned->choice)); > - if (banned->choice == LOFC_TREE_DEPTH && > - data->tree_filter_max_depth != ULONG_MAX) > - strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"), > - data->tree_filter_max_depth, > - banned->tree_exclude_depth); > - > - packet_writer_error(&data->writer, "%s\n", buf.buf); > - die("%s", buf.buf); > + check_filter_recurse(data, &data->filter_options); > } > > static void receive_needs(struct upload_pack_data *data, I think that we crossed emails, but I'm still not convinced that this is any cleaner than what I wrote. Yes, it's a maintenance problem if we add more filter-specific logic like what we have in the LOFC_TREE_DEPTH case, but I feel like we're bending over backwards in the meantime to accommodate a problem that we don't have. Thanks, Taylor
On Fri, Jul 31, 2020 at 05:29:05PM -0400, Taylor Blau wrote: > > > @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data) > > > > > > strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", > > > list_object_filter_config_name(banned->choice)); > > > + if (banned->choice == LOFC_TREE_DEPTH && > > > + data->tree_filter_max_depth != ULONG_MAX) > > > + strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"), > > > + data->tree_filter_max_depth, > > > + banned->tree_exclude_depth); > > > > Hmm. So I see now why you wanted to go with the strbuf in the earlier > > patch. This does still feel awkward, though. You check "is it allowed" > > in an earlier function, we get "nope, it's not allowed", and now we have > > to reimplement the check here. That seems like a maintenance burden. > > I'm not sure that I follow. Is the earlier function that you're > referring to 'banned_filter'? If so, the only use of that function is > within 'die_if_using_banned_filter'. 'banned_filter' exists only insofar > as to answer the question "return me the first banned filter, if one > exists, or NULL otherwise". > > Then, dying here is as simple as (1) lookup the banned filter, and (2) > check if it's NULL or not. > > If you're referring to 'allows_filter_choice', I guess I see what you're > getting it, but to be honest I'm not sure if I'm buying it. Yeah, it's allows_filter_choice() that knows "if it's a tree we must check the depth". And now die_if_using_banned_filter() needs to know that, too. The policy is implemented twice. I do appreciate that the way you've written it means that if somebody forgets to update die_if_using_banned_filter() to match the logic in allows_filter_choice(), we'd at least still die, just with a less good error message. But it seems better still not to require the two to match in the first place. > If we were > to combine 'allows_filter_choice', 'banned_filter', and > 'die_if_using_banned_filter' into one function that traversed the filter > tree and 'die()'d as soon as it got to a banned one, that function would > have to know how to: > > 1. Recurse through the tree when it hits a LOFC_COMBINE node. > > 2. At each node, translate the filter->choice into the appropriate key > name, look it up, and then figure out how to interpret its allowed > status (including falling back to the default if unspecified). > > 3. And, it would have to figure out how to format the message at each > step. > > (3) I think is made easier, since we know what message to format based > on whether or not we're in the 'opts->choice == LOFC_TREE_DEPTH' arm or > not. But, there are two more things that we now have to cram into that > same function. You can still split those things into functions; see the patch I posted. > Maybe I'm being too strict an adherent to having simpler functions, but > I'm failing to see how to combine these in a way that's cleaner than > what's written here. To me this is less about "clean" and more about "don't ever duplicate policy code". I don't mind duplicating boilerplate, but introducing a place where somebody touching function X must remember to also touch Y (and gets no compiler support to remind them) is a bad thing. I guess you can call that "clean", but I'd take longer or more functions as a tradeoff to avoid that. My suggested patch does introduce more side effects. I think that's OK because there really is only a single caller here. But if you wanted it cleaner, then I think having allows_filter_choice() fill out an error strbuf would eliminate my concern without drastically altering the flow of your code. -Peff
On Fri, Jul 31, 2020 at 05:36:04PM -0400, Jeff King wrote: > My suggested patch does introduce more side effects. I think that's OK > because there really is only a single caller here. But if you wanted it > cleaner, then I think having allows_filter_choice() fill out an error > strbuf would eliminate my concern without drastically altering the flow > of your code. That could be something like the patch below. banned_filter() could switch to returning a simple boolean, but I didn't do that here. I did (as with my other patch) reorder the logic in allows_filter_choice(), as I think it's much easier to follow by resolving "allowed" to the fallback earlier. That lets you handle "not allowed at all" first and early-return (or die) before dealing with type-specific checks. diff --git a/upload-pack.c b/upload-pack.c index 131445b212..86786f0e13 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -993,59 +993,60 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not, } static int allows_filter_choice(struct upload_pack_data *data, - struct list_objects_filter_options *opts) + struct list_objects_filter_options *opts, + struct strbuf *err) { const char *key = list_object_filter_config_name(opts->choice); struct string_list_item *item = string_list_lookup(&data->allowed_filters, key); - int allowed = -1; + int allowed; if (item) allowed = (intptr_t) item->util; + else + allowed = data->allow_filter_fallback; - if (allowed != 0 && - opts->choice == LOFC_TREE_DEPTH && - opts->tree_exclude_depth > data->tree_filter_max_depth) + if (!allowed) { + strbuf_addf(err, "filter '%s' not supported", key); return 0; + } + + if (opts->choice == LOFC_TREE_DEPTH && + opts->tree_exclude_depth > data->tree_filter_max_depth) { + strbuf_addf(err, "tree filter maximum depth is %lu, but got %lu", + data->tree_filter_max_depth, opts->tree_exclude_depth); + return 0; + } - if (allowed > -1) - return allowed; - return data->allow_filter_fallback; + return 1; } static struct list_objects_filter_options *banned_filter( struct upload_pack_data *data, - struct list_objects_filter_options *opts) + struct list_objects_filter_options *opts, + struct strbuf *err) { size_t i; - if (!allows_filter_choice(data, opts)) + if (!allows_filter_choice(data, opts, err)) return opts; if (opts->choice == LOFC_COMBINE) for (i = 0; i < opts->sub_nr; i++) { struct list_objects_filter_options *sub = &opts->sub[i]; - if (banned_filter(data, sub)) + if (banned_filter(data, sub, err)) return sub; } return NULL; } static void die_if_using_banned_filter(struct upload_pack_data *data) { - struct list_objects_filter_options *banned = banned_filter(data, - &data->filter_options); struct strbuf buf = STRBUF_INIT; + struct list_objects_filter_options *banned = + banned_filter(data, &data->filter_options, &buf); if (!banned) return; - strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", - list_object_filter_config_name(banned->choice)); - if (banned->choice == LOFC_TREE_DEPTH && - data->tree_filter_max_depth != ULONG_MAX) - strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"), - data->tree_filter_max_depth, - banned->tree_exclude_depth); - packet_writer_error(&data->writer, "%s\n", buf.buf); die("%s", buf.buf); }
diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt index fffe8ac648..ee7b3ac94f 100644 --- a/Documentation/config/uploadpack.txt +++ b/Documentation/config/uploadpack.txt @@ -69,6 +69,12 @@ uploadpackfilter.<filter>.allow:: combined filters, both `combine` and all of the nested filter kinds must be allowed. Defaults to `uploadpackfilter.allow`. +uploadpackfilter.tree.maxDepth:: + Only allow `--filter=tree=<n>` when `n` is no more than the value of + `uploadpackfilter.tree.maxDepth`. If set, this also implies + `uploadpackfilter.tree.allow=true`, unless this configuration + variable had already been set. Has no effect if unset. + uploadpack.allowRefInWant:: If this option is set, `upload-pack` will support the `ref-in-want` feature of the protocol version 2 `fetch` command. This feature diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 0d46b5a2f8..35cb6a34a3 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -259,6 +259,15 @@ test_expect_success 'upload-pack fails banned object filters with fallback' ' test_i18ngrep "filter '\''blob:none'\'' not supported" err ' +test_expect_success 'upload-pack limits tree depth filters' ' + test_config -C srv.bare uploadpackfilter.allow false && + test_config -C srv.bare uploadpackfilter.tree.allow true && + test_config -C srv.bare uploadpackfilter.tree.maxDepth 0 && + test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \ + "file://$(pwd)/srv.bare" pc3 2>err && + test_i18ngrep "filter '\''tree'\'' not supported (maximum depth: 0, but got: 1)" err +' + test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' ' rm -rf src dst && git init src && diff --git a/upload-pack.c b/upload-pack.c index 5fa22da31f..131445b212 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -105,6 +105,7 @@ struct upload_pack_data { unsigned use_include_tag : 1; unsigned allow_filter : 1; unsigned allow_filter_fallback : 1; + unsigned long tree_filter_max_depth; unsigned done : 1; /* v2 only */ unsigned allow_ref_in_want : 1; /* v2 only */ @@ -136,6 +137,7 @@ static void upload_pack_data_init(struct upload_pack_data *data) data->extra_edge_obj = extra_edge_obj; data->allowed_filters = allowed_filters; data->allow_filter_fallback = 1; + data->tree_filter_max_depth = ULONG_MAX; packet_writer_init(&data->writer, 1); data->keepalive = 5; @@ -996,8 +998,17 @@ static int allows_filter_choice(struct upload_pack_data *data, const char *key = list_object_filter_config_name(opts->choice); struct string_list_item *item = string_list_lookup(&data->allowed_filters, key); + int allowed = -1; if (item) - return (intptr_t) item->util; + allowed = (intptr_t) item->util; + + if (allowed != 0 && + opts->choice == LOFC_TREE_DEPTH && + opts->tree_exclude_depth > data->tree_filter_max_depth) + return 0; + + if (allowed > -1) + return allowed; return data->allow_filter_fallback; } @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data) strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", list_object_filter_config_name(banned->choice)); + if (banned->choice == LOFC_TREE_DEPTH && + data->tree_filter_max_depth != ULONG_MAX) + strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"), + data->tree_filter_max_depth, + banned->tree_exclude_depth); packet_writer_error(&data->writer, "%s\n", buf.buf); die("%s", buf.buf); @@ -1243,6 +1259,15 @@ static int parse_object_filter_config(const char *var, const char *value, if (!strcmp(key, "allow")) string_list_insert(&data->allowed_filters, buf.buf)->util = (void *)(intptr_t)git_config_bool(var, value); + else if (!strcmp(buf.buf, "tree") && !strcmp(key, "maxdepth")) { + if (!value) { + strbuf_release(&buf); + return config_error_nonbool(var); + } + string_list_insert(&data->allowed_filters, buf.buf)->util = + (void *)(intptr_t)1; + data->tree_filter_max_depth = git_config_ulong(var, value); + } strbuf_release(&buf); return 0;
In b79cf959b2 (upload-pack.c: allow banning certain object filter(s), 2020-02-26), we introduced functionality to disallow certain object filters from being chosen from within 'git upload-pack'. Traditionally, administrators use this functionality to disallow filters that are known to perform slowly, for e.g., those that do not have bitmap-level filtering. In the past, the '--filter=tree:<n>' was one such filter that does not have bitmap-level filtering support, and so was likely to be banned by administrators. However, in the previous couple of commits, we introduced bitmap-level filtering for the case when 'n' is equal to '0', i.e., as if we had a '--filter=tree:none' choice. While it would be sufficient to simply write $ git config uploadpackfilter.tree.allow true (since it would allow all values of 'n'), we would like to be able to allow this filter for certain values of 'n', i.e., those no greater than some pre-specified maximum. In order to do this, introduce a new configuration key, as follows: $ git config uploadpackfilter.tree.maxDepth <m> where '<m>' specifies the maximum allowed value of 'n' in the filter 'tree:n'. Administrators who wish to allow for only the value '0' can write: $ git config uploadpackfilter.tree.allow true $ git config uploadpackfilter.tree.maxDepth 0 which allows '--filter=tree:0', but no other values. Unfortunately, since the tree depth is an unsigned long, we can't use, say, -1 as a sentinel value, and so we must also keep track of "have we set this" as well as "to what value". Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/config/uploadpack.txt | 6 ++++++ t/t5616-partial-clone.sh | 9 +++++++++ upload-pack.c | 27 ++++++++++++++++++++++++++- 3 files changed, 41 insertions(+), 1 deletion(-)