| Message ID | a0a04277578333daa0b38a8e06e1738b920f5ac3.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:31PM -0400, Taylor Blau wrote: > Git clients may ask the server for a partial set of objects, where the > set of objects being requested is refined by one or more object filters. > Server administrators can configure 'git upload-pack' to allow or ban > these filters by setting the 'uploadpack.allowFilter' variable to > 'true' or 'false', respectively. > > However, administrators using bitmaps may wish to allow certain kinds of > object filters, but ban others. Specifically, they may wish to allow > object filters that can be optimized by the use of bitmaps, while > rejecting other object filters which aren't and represent a perceived > performance degradation (as well as an increased load factor on the > server). > > Allow configuring 'git upload-pack' to support object filters on a > case-by-case basis by introducing two new configuration variables: > > - 'uploadpackfilter.allow' > - 'uploadpackfilter.<kind>.allow' > > where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on. Minor nit, but <kind> is "blob:none", "blob:limit", etc. The code and documentation gets this right; it's just the commit message. I'm pretty sure this is a casualty of updating the syntax as the series was developed. One trick I use is to avoid repeating explanations that are in the documentation from the patch already. I.e., explain "why" here, but it's OK to let "what" come from the patch itself. That's not only one less thing to remember to update, but it's less for reviewers to read through, too. </meta-patch-advice> > +test_expect_success 'upload-pack fails banned object filters' ' > + test_config -C srv.bare uploadpackfilter.blob:none.allow false && > + test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \ > + "file://$(pwd)/srv.bare" pc3 2>err && > + test_i18ngrep "filter '\''blob:none'\'' not supported" err > +' These messages aren't translated now, so we can just use grep, I think. Ditto in the other tests below. > +static void die_if_using_banned_filter(struct upload_pack_data *data) > +{ > + struct list_objects_filter_options *banned = banned_filter(data, > + &data->filter_options); > + struct strbuf buf = STRBUF_INIT; > + if (!banned) > + return; > + > + strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", > + list_object_filter_config_name(banned->choice)); > + > + packet_writer_error(&data->writer, "%s\n", buf.buf); > + die("%s", buf.buf); > +} Hmm, the strbuf was unexpected. I'd have just written it out twice. After all, the messages don't have to be the same. And perhaps we don't want them to be the same? A user receiving the ERR packet would see: remote error: git upload-pack: filter 'foo' not supported do we need the "git upload-pack" part there? Other errors say just "upload-pack". IMHO even that is unnecessarily verbose, and I wouldn't mind a separate patch to reduce it. But definitely going even longer doesn't seem like the right direction. :) I also wondered about the trailing newline. Other callers of packet_writer_error() don't seem to use it. I think in practice it doesn't matter much because readers will generally be using CHOMP_NEWLINE, but it probably makes sense to be consistent. > [...] Aside from this nits, this patch looks good to me. -Peff
On Fri, Jul 31, 2020 at 04:54:34PM -0400, Jeff King wrote: > On Fri, Jul 31, 2020 at 04:26:31PM -0400, Taylor Blau wrote: > > > Git clients may ask the server for a partial set of objects, where the > > set of objects being requested is refined by one or more object filters. > > Server administrators can configure 'git upload-pack' to allow or ban > > these filters by setting the 'uploadpack.allowFilter' variable to > > 'true' or 'false', respectively. > > > > However, administrators using bitmaps may wish to allow certain kinds of > > object filters, but ban others. Specifically, they may wish to allow > > object filters that can be optimized by the use of bitmaps, while > > rejecting other object filters which aren't and represent a perceived > > performance degradation (as well as an increased load factor on the > > server). > > > > Allow configuring 'git upload-pack' to support object filters on a > > case-by-case basis by introducing two new configuration variables: > > > > - 'uploadpackfilter.allow' > > - 'uploadpackfilter.<kind>.allow' > > > > where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on. > > Minor nit, but <kind> is "blob:none", "blob:limit", etc. The code and > documentation gets this right; it's just the commit message. > > I'm pretty sure this is a casualty of updating the syntax as the series > was developed. One trick I use is to avoid repeating explanations that > are in the documentation from the patch already. I.e., explain "why" > here, but it's OK to let "what" come from the patch itself. That's not > only one less thing to remember to update, but it's less for reviewers > to read through, too. > > </meta-patch-advice> Good advice, and you're right that <kind> is blob:none and similar, not 'blobNone'. In this case, I think the "why" is pretty boring, so I don't mind repeating myself a little bit in the commit message. > > > +test_expect_success 'upload-pack fails banned object filters' ' > > + test_config -C srv.bare uploadpackfilter.blob:none.allow false && > > + test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \ > > + "file://$(pwd)/srv.bare" pc3 2>err && > > + test_i18ngrep "filter '\''blob:none'\'' not supported" err > > +' > > These messages aren't translated now, so we can just use grep, I think. > > Ditto in the other tests below. Ack, yep. > > > +static void die_if_using_banned_filter(struct upload_pack_data *data) > > +{ > > + struct list_objects_filter_options *banned = banned_filter(data, > > + &data->filter_options); > > + struct strbuf buf = STRBUF_INIT; > > + if (!banned) > > + return; > > + > > + strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", > > + list_object_filter_config_name(banned->choice)); > > + > > + packet_writer_error(&data->writer, "%s\n", buf.buf); > > + die("%s", buf.buf); > > +} > > Hmm, the strbuf was unexpected. I'd have just written it out twice. > After all, the messages don't have to be the same. And perhaps we don't > want them to be the same? A user receiving the ERR packet would see: > > remote error: git upload-pack: filter 'foo' not supported > > do we need the "git upload-pack" part there? Other errors say just > "upload-pack". IMHO even that is unnecessarily verbose, and I wouldn't > mind a separate patch to reduce it. But definitely going even longer > doesn't seem like the right direction. :) Let's drop 'git upload-pack: ' entirely, and just start the message with 'filter ...'. It looks like you figured out why we use a strbuf here in your response to the fourth patch. > I also wondered about the trailing newline. Other callers of > packet_writer_error() don't seem to use it. I think in practice it > doesn't matter much because readers will generally be using > CHOMP_NEWLINE, but it probably makes sense to be consistent. Dropping the newline should be easy enough. > > > [...] > > Aside from this nits, this patch looks good to me. > > -Peff Thanks, Taylor
diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt index ed1c835695..fffe8ac648 100644 --- a/Documentation/config/uploadpack.txt +++ b/Documentation/config/uploadpack.txt @@ -57,6 +57,18 @@ uploadpack.allowFilter:: If this option is set, `upload-pack` will support partial clone and partial fetch object filtering. +uploadpackfilter.allow:: + Provides a default value for unspecified object filters (see: the + below configuration variable). + Defaults to `true`. + +uploadpackfilter.<filter>.allow:: + Explicitly allow or ban the object filter corresponding to + `<filter>`, where `<filter>` may be one of: `blob:none`, + `blob:limit`, `tree`, `sparse:oid`, or `combine`. If using + combined filters, both `combine` and all of the nested filter + kinds must be allowed. Defaults to `uploadpackfilter.allow`. + uploadpack.allowRefInWant:: If this option is set, `upload-pack` will support the `ref-in-want` feature of the protocol version 2 `fetch` command. This feature diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 37de0afb02..0d46b5a2f8 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -235,6 +235,30 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' ' test_cmp unique_types.expected unique_types.actual ' +test_expect_success 'upload-pack fails banned object filters' ' + test_config -C srv.bare uploadpackfilter.blob:none.allow false && + test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \ + "file://$(pwd)/srv.bare" pc3 2>err && + test_i18ngrep "filter '\''blob:none'\'' not supported" err +' + +test_expect_success 'upload-pack fails banned combine object filters' ' + test_config -C srv.bare uploadpackfilter.allow false && + test_config -C srv.bare uploadpackfilter.combine.allow true && + test_config -C srv.bare uploadpackfilter.tree.allow true && + test_config -C srv.bare uploadpackfilter.blob:none.allow false && + test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \ + --filter=blob:none "file://$(pwd)/srv.bare" pc3 2>err && + test_i18ngrep "filter '\''blob:none'\'' not supported" err +' + +test_expect_success 'upload-pack fails banned object filters with fallback' ' + test_config -C srv.bare uploadpackfilter.allow false && + test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \ + "file://$(pwd)/srv.bare" pc3 2>err && + test_i18ngrep "filter '\''blob:none'\'' not supported" err +' + test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' ' rm -rf src dst && git init src && diff --git a/upload-pack.c b/upload-pack.c index 8673741070..ed2098edd0 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -88,6 +88,7 @@ struct upload_pack_data { enum allow_uor allow_uor; struct list_objects_filter_options filter_options; + struct string_list allowed_filters; struct packet_writer writer; @@ -103,6 +104,7 @@ struct upload_pack_data { unsigned no_progress : 1; unsigned use_include_tag : 1; unsigned allow_filter : 1; + unsigned allow_filter_fallback : 1; unsigned done : 1; /* v2 only */ unsigned allow_ref_in_want : 1; /* v2 only */ @@ -120,6 +122,7 @@ static void upload_pack_data_init(struct upload_pack_data *data) struct string_list deepen_not = STRING_LIST_INIT_DUP; struct string_list uri_protocols = STRING_LIST_INIT_DUP; struct object_array extra_edge_obj = OBJECT_ARRAY_INIT; + struct string_list allowed_filters = STRING_LIST_INIT_DUP; memset(data, 0, sizeof(*data)); data->symref = symref; @@ -131,6 +134,8 @@ static void upload_pack_data_init(struct upload_pack_data *data) data->deepen_not = deepen_not; data->uri_protocols = uri_protocols; data->extra_edge_obj = extra_edge_obj; + data->allowed_filters = allowed_filters; + data->allow_filter_fallback = 1; packet_writer_init(&data->writer, 1); data->keepalive = 5; @@ -147,6 +152,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data) string_list_clear(&data->deepen_not, 0); object_array_clear(&data->extra_edge_obj); list_objects_filter_release(&data->filter_options); + string_list_clear(&data->allowed_filters, 1); free((char *)data->pack_objects_hook); } @@ -984,6 +990,50 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not, return 0; } +static int allows_filter_choice(struct upload_pack_data *data, + enum list_objects_filter_choice c) +{ + const char *key = list_object_filter_config_name(c); + struct string_list_item *item = string_list_lookup(&data->allowed_filters, + key); + if (item) + return (intptr_t) item->util; + return data->allow_filter_fallback; +} + +static struct list_objects_filter_options *banned_filter( + struct upload_pack_data *data, + struct list_objects_filter_options *opts) +{ + size_t i; + + if (!allows_filter_choice(data, opts->choice)) + return opts; + + if (opts->choice == LOFC_COMBINE) + for (i = 0; i < opts->sub_nr; i++) { + struct list_objects_filter_options *sub = &opts->sub[i]; + if (banned_filter(data, sub)) + return sub; + } + return NULL; +} + +static void die_if_using_banned_filter(struct upload_pack_data *data) +{ + struct list_objects_filter_options *banned = banned_filter(data, + &data->filter_options); + struct strbuf buf = STRBUF_INIT; + if (!banned) + return; + + strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", + list_object_filter_config_name(banned->choice)); + + packet_writer_error(&data->writer, "%s\n", buf.buf); + die("%s", buf.buf); +} + static void receive_needs(struct upload_pack_data *data, struct packet_reader *reader) { @@ -1014,6 +1064,7 @@ static void receive_needs(struct upload_pack_data *data, die("git upload-pack: filtering capability not negotiated"); list_objects_filter_die_if_populated(&data->filter_options); parse_list_objects_filter(&data->filter_options, arg); + die_if_using_banned_filter(data); continue; } @@ -1171,6 +1222,32 @@ static int find_symref(const char *refname, const struct object_id *oid, return 0; } +static int parse_object_filter_config(const char *var, const char *value, + struct upload_pack_data *data) +{ + struct strbuf buf = STRBUF_INIT; + const char *sub, *key; + size_t sub_len; + + if (parse_config_key(var, "uploadpackfilter", &sub, &sub_len, &key)) + return 0; + + if (!sub) { + if (!strcmp(key, "allow")) + data->allow_filter_fallback = git_config_bool(var, value); + return 0; + } + + strbuf_add(&buf, sub, sub_len); + + if (!strcmp(key, "allow")) + string_list_insert(&data->allowed_filters, buf.buf)->util = + (void *)(intptr_t)git_config_bool(var, value); + + strbuf_release(&buf); + return 0; +} + static int upload_pack_config(const char *var, const char *value, void *cb_data) { struct upload_pack_data *data = cb_data; @@ -1210,6 +1287,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data) return git_config_string(&data->pack_objects_hook, var, value); } + parse_object_filter_config(var, value, data); + return parse_hide_refs_config(var, value, "uploadpack"); } @@ -1390,6 +1469,7 @@ static void process_args(struct packet_reader *request, if (data->allow_filter && skip_prefix(arg, "filter ", &p)) { list_objects_filter_die_if_populated(&data->filter_options); parse_list_objects_filter(&data->filter_options, p); + die_if_using_banned_filter(data); continue; }
Git clients may ask the server for a partial set of objects, where the set of objects being requested is refined by one or more object filters. Server administrators can configure 'git upload-pack' to allow or ban these filters by setting the 'uploadpack.allowFilter' variable to 'true' or 'false', respectively. However, administrators using bitmaps may wish to allow certain kinds of object filters, but ban others. Specifically, they may wish to allow object filters that can be optimized by the use of bitmaps, while rejecting other object filters which aren't and represent a perceived performance degradation (as well as an increased load factor on the server). Allow configuring 'git upload-pack' to support object filters on a case-by-case basis by introducing two new configuration variables: - 'uploadpackfilter.allow' - 'uploadpackfilter.<kind>.allow' where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on. Setting the second configuration variable for any valid value of '<kind>' explicitly allows or disallows restricting that kind of object filter. If a client requests the object filter <kind> and the respective configuration value is not set, 'git upload-pack' will default to the value of 'uploadpackfilter.allow', which itself defaults to 'true' to maintain backwards compatibility. Note that this differs from 'uploadpack.allowfilter', which controls whether or not the 'filter' capability is advertised. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/config/uploadpack.txt | 12 +++++ t/t5616-partial-clone.sh | 24 +++++++++ upload-pack.c | 80 +++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+)