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@xxxxxxxxxxxxxxxxxxxxxxx/
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@xxxxxxxx>
Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
## 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@xxxxxxxxxxxx>
## 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
