[PATCH v4 0/3] upload-pack: custom allowed object filters

[Taylor Blau <me@ttaylorr.com>]

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