| Message ID | cover.1596476928.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | upload-pack: custom allowed object filters | expand |
On Mon, Aug 03, 2020 at 02:00:04PM -0400, Taylor Blau wrote: > Here's what I anticipate to be the final reroll of my series to teach > the new 'uploadpackfilter' configuration section, which allows for more > fine-grained control over which object filters upload-pack is willing to > serve. Thanks, this version looks good to me. > Two changes from last time: > > - I adopted Peff's suggestion in beginning in [1], but appropriately > split it over the existing patch structure. The adaptation looks good. The send_err_and_die() helper could possibly be used for some of the packet_writer_error() callers, too. Somebody could convert them on top if they want, but I'm not sure it's even worth it. IMHO the correct long-term direction is to convert those die() calls into silent exits and let the ERR packet speak for itself. But we shouldn't do that until the client side is more robust against pipe-death races. > - I dropped the old patch 3/4, since it really should have never been > there in the first place, and just made the refactoring more noisy > than necessary. Yeah, I think folding it in to the first patch as you did makes the series simpler to follow. -Peff
Hi, Here's what I anticipate to be the final reroll of my series to teach the new 'uploadpackfilter' configuration section, which allows for more fine-grained control over which object filters upload-pack is willing to serve. Two changes from last time: - I adopted Peff's suggestion in beginning in [1], but appropriately split it over the existing patch structure. - I dropped the old patch 3/4, since it really should have never been there in the first place, and just made the refactoring more noisy than necessary. (For the curious, this patch was written as a preparatory step within GitHub's fork in order to add the 'uploadpackfilter.tree.maxDepth' configuration. This was after the initial work when I hadn't yet considered adding such a thing. Now that we know the full arc, it makes sense to just pass the right parameters from the get-go). Thanks again for all of the review! [1] :https://lore.kernel.org/git/20200731210114.GC1440890@coredump.intra.peff.net/ Taylor Blau (3): list_objects_filter_options: introduce 'list_object_filter_config_name' upload-pack.c: allow banning certain object filter(s) upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Documentation/config/uploadpack.txt | 18 +++++ list-objects-filter-options.c | 23 ++++++ list-objects-filter-options.h | 6 ++ t/t5616-partial-clone.sh | 33 +++++++++ upload-pack.c | 104 ++++++++++++++++++++++++++++ 5 files changed, 184 insertions(+) Range-diff against v3: -: ---------- > 1: 21531927e4 Revert "fmt-merge-msg: stop treating `master` specially" -: ---------- > 2: 6e6029a82a fmt-merge-msg: allow merge destination to be omitted again -: ---------- > 3: 25429fed5c refs: move the logic to add \t to reflog to the files backend -: ---------- > 4: 3db796c1c0 t6300: fix issues related to %(contents:size) -: ---------- > 5: 85b4e0a6dc Third batch 1: b1b3dd7de9 = 6: f4c7771875 list_objects_filter_options: introduce 'list_object_filter_config_name' 2: a0a0427757 ! 7: b34f4eaed9 upload-pack.c: allow banning certain object filter(s) @@ Commit message 'uploadpack.allowfilter', which controls whether or not the 'filter' capability is advertised. + Helped-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> ## Documentation/config/uploadpack.txt ## @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil + 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 ++ grep "filter '\''blob:none'\'' not supported" err +' + +test_expect_success 'upload-pack fails banned combine object filters' ' @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil + 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 ++ grep "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 ++ grep "filter '\''blob:none'\'' not supported" err +' + test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' ' @@ upload-pack.c: static int process_deepen_not(const char *line, struct string_lis return 0; } -+static int allows_filter_choice(struct upload_pack_data *data, -+ enum list_objects_filter_choice c) ++NORETURN __attribute__((format(printf,2,3))) ++static void send_err_and_die(struct upload_pack_data *data, ++ const char *fmt, ...) +{ -+ const char *key = list_object_filter_config_name(c); ++ struct strbuf buf = STRBUF_INIT; ++ va_list ap; ++ ++ 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; ++ + if (item) -+ return (intptr_t) item->util; -+ return data->allow_filter_fallback; ++ allowed = (intptr_t)item->util; ++ else ++ allowed = data->allow_filter_fallback; ++ ++ if (!allowed) ++ send_err_and_die(data, "filter '%s' not supported", key); +} + -+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->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) ++ check_one_filter(data, opts); ++ if (opts->choice != LOFC_COMBINE) + return; + -+ strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", -+ list_object_filter_config_name(banned->choice)); ++ for (i = 0; i < opts->sub_nr; i++) ++ check_filter_recurse(data, &opts->sub[i]); ++} + -+ packet_writer_error(&data->writer, "%s\n", buf.buf); -+ die("%s", buf.buf); ++static void die_if_using_banned_filter(struct upload_pack_data *data) ++{ ++ check_filter_recurse(data, &data->filter_options); +} + static void receive_needs(struct upload_pack_data *data, 3: ad3f0cce56 < -: ---------- upload-pack.c: pass 'struct list_objects_filter_options *' 4: c9d71809f4 ! 8: a0e7731a55 upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' @@ Commit message 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 ## @@ Documentation/config/uploadpack.txt: uploadpackfilter.<filter>.allow:: ## t/t5616-partial-clone.sh ## @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object filters with fallback' ' - test_i18ngrep "filter '\''blob:none'\'' not supported" err + grep "filter '\''blob:none'\'' not supported" err ' +test_expect_success 'upload-pack limits tree depth filters' ' @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object f + 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 ++ grep "tree filter allows max depth 0, but got 1" err +' + test_expect_success 'partial clone fetches blobs pointed to by refs even if normally filtered out' ' @@ upload-pack.c: static void upload_pack_data_init(struct upload_pack_data *data) packet_writer_init(&data->writer, 1); data->keepalive = 5; -@@ upload-pack.c: 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; +@@ upload-pack.c: static void check_one_filter(struct upload_pack_data *data, + + if (!allowed) + send_err_and_die(data, "filter '%s' not supported", key); + -+ if (allowed != 0 && -+ opts->choice == LOFC_TREE_DEPTH && ++ if (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; ++ send_err_and_die(data, ++ "tree filter allows max depth %lu, but got %lu", ++ data->tree_filter_max_depth, ++ opts->tree_exclude_depth); } -@@ upload-pack.c: 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); + static void check_filter_recurse(struct upload_pack_data *data, @@ upload-pack.c: 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 = -- 2.28.0.rc1.13.ge78abce653