git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: git@vger.kernel.org
Cc: peff@peff.net, chriscool@tuxfamily.org, gitster@pobox.com,
	szeder.dev@gmail.com
Subject: [PATCH v2 0/4] upload-pack: custom allowed object filters
Date: Wed, 22 Jul 2020 21:48:54 -0400	[thread overview]
Message-ID: <cover.1595468657.git.me@ttaylorr.com> (raw)

Here's a much-delayed v2 of my series to teach upload-pack to limit the
kinds of object filters that it is willing to server in a request.

Much is the same since last time, with two notable exceptions:

  - We use the 'uploadpackfilter' top-level configuration key instead of
    pretending that 'uploadpack.filter' is top-level, which greatly
    simplifies the call to 'parse_config_key()'.

  - Instead of writing an err packet, 'git upload-pack' simply 'die()'s,
    which propagates the error through 'git clone' always, and resolves
    a flaky set of tests that used to result in a SIGPIPE.

Thanks for your review in the meantime, and for encouraging me to send
this reroll now instead of later. Maybe I'll start unloading the rest of
my backlog tomorrow ;-).

Taylor Blau (4):
  list_objects_filter_options: introduce
    'list_object_filter_config_name'
  upload-pack.c: allow banning certain object filter(s)
  upload-pack.c: pass 'struct list_objects_filter_options *'
  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            |  32 +++++++++
 upload-pack.c                       | 103 ++++++++++++++++++++++++++++
 5 files changed, 182 insertions(+)

Range-diff against v1:
  2:  f0982d24e7 ! 134:  9fee52cb6d upload-pack.c: allow banning certain object filter(s)
    @@ Commit message
         Allow configuring 'git upload-pack' to support object filters on a
         case-by-case basis by introducing two new configuration variables:

    -      - 'uploadpack.filter.allow'
    -      - 'uploadpack.filter.<kind>.allow'
    +      - 'uploadpackfilter.allow'
    +      - 'uploadpackfilter.<kind>.allow'

         where '<kind>' may be one of 'blobNone', 'blobLimit', 'tree', and so on.

    @@ Commit message

         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 'uploadpack.filter.allow', which itself defaults to 'true' to
    +    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.
    @@ Documentation/config/uploadpack.txt: uploadpack.allowFilter::
      	If this option is set, `upload-pack` will support partial
      	clone and partial fetch object filtering.

    -+uploadpack.filter.allow::
    ++uploadpackfilter.allow::
     +	Provides a default value for unspecified object filters (see: the
     +	below configuration variable).
     +	Defaults to `true`.
     +
    -+uploadpack.filter.<filter>.allow::
    ++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 `uploadpack.filter.allow`.
    -++
    -+Note that the dot between 'filter' and '<filter>' is both non-standard
    -+and intentional. This is done to avoid a parsing ambiguity when
    -+specifying this configuration as an argument to Git's top-level `-c`.
    ++	kinds must be allowed. Defaults to `uploadpackfilter.allow`.
     +
      uploadpack.allowRefInWant::
      	If this option is set, `upload-pack` will support the `ref-in-want`
    @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil
      '

     +test_expect_success 'upload-pack fails banned object filters' '
    -+	# Test case-insensitivity by intentional use of "blob:None" rather than
    -+	# "blob:none".
    -+	test_config -C srv.bare uploadpack.filter.blob:None.allow false &&
    ++	test_config -C srv.bare uploadpackfilter.blob:none.allow false &&
     +	test_must_fail 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 uploadpack.filter.allow false &&
    -+	test_config -C srv.bare uploadpack.filter.combine.allow true &&
    -+	test_config -C srv.bare uploadpack.filter.tree.allow true &&
    -+	test_config -C srv.bare uploadpack.filter.blob:none.allow false &&
    ++	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 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 uploadpack.filter.allow false &&
    ++	test_config -C srv.bare uploadpackfilter.allow false &&
     +	test_must_fail git clone --no-checkout --filter=blob:none \
     +		"file://$(pwd)/srv.bare" pc3 2>err &&
     +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
    @@ upload-pack.c: static int process_deepen_not(const char *line, struct string_lis
     +	if (!banned)
     +		return;
     +
    -+	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
    -+			    list_object_filter_config_name(banned->choice));
    -+	die(_("git upload-pack: banned object filter requested"));
    ++	die(_("git upload-pack: filter '%s' not supported"),
    ++	    list_object_filter_config_name(banned->choice));
     +}
     +
      static void receive_needs(struct upload_pack_data *data,
    @@ upload-pack.c: static int find_symref(const char *refname, const struct object_i
      	return 0;
      }

    -+static void parse_object_filter_config(const char *var, const char *value,
    ++static int parse_object_filter_config(const char *var, const char *value,
     +				       struct upload_pack_data *data)
     +{
    -+	struct strbuf spec = STRBUF_INIT;
    ++	struct strbuf buf = STRBUF_INIT;
     +	const char *sub, *key;
     +	size_t sub_len;
     +
    -+	if (parse_config_key(var, "uploadpack", &sub, &sub_len, &key))
    -+		return;
    -+	if (!sub || !skip_prefix(sub, "filter.", &sub))
    -+		return;
    ++	if (parse_config_key(var, "uploadpackfilter", &sub, &sub_len, &key))
    ++		return 0;
     +
    -+	if (sub != key)
    -+		strbuf_add(&spec, sub, key - sub - 1);
    -+	strbuf_tolower(&spec);
    -+
    -+	if (!strcmp(key, "allow")) {
    -+		if (spec.len)
    -+			string_list_insert(&data->allowed_filters, spec.buf)->util =
    -+				(void *)(intptr_t)git_config_bool(var, value);
    -+		else
    ++	if (!sub) {
    ++		if (!strcmp(key, "allow"))
     +			data->allow_filter_fallback = git_config_bool(var, value);
    ++		return 0;
     +	}
     +
    -+	strbuf_release(&spec);
    ++	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)
  3:  3434bd5979 = 135:  550e4e13f1 upload-pack.c: pass 'struct list_objects_filter_options *'
  4:  9fa765a71d ! 136:  bb008f7427 upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'
    @@ Metadata
     Author: Taylor Blau <me@ttaylorr.com>

      ## Commit message ##
    -    upload-pack.c: introduce 'uploadpack.filter.tree.maxDepth'
    +    upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'

         In b79cf959b2 (upload-pack.c: allow banning certain object filter(s),
         2020-02-26), we introduced functionality to disallow certain object
    @@ Commit message

         While it would be sufficient to simply write

    -      $ git config uploadpack.filter.tree.allow true
    +      $ 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
    @@ Commit message

         In order to do this, introduce a new configuration key, as follows:

    -      $ git config uploadpack.filter.tree.maxDepth <m>
    +      $ 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 uploadpack.filter.tree.allow true
    -      $ git config uploadpack.filter.tree.maxDepth 0
    +      $ git config uploadpackfilter.tree.allow true
    +      $ git config uploadpackfilter.tree.maxDepth 0

         which allows '--filter=tree:0', but no other values.

    @@ Commit message
         Signed-off-by: Taylor Blau <me@ttaylorr.com>

      ## Documentation/config/uploadpack.txt ##
    -@@ Documentation/config/uploadpack.txt: Note that the dot between 'filter' and '<filter>' is both non-standard
    - and intentional. This is done to avoid a parsing ambiguity when
    - specifying this configuration as an argument to Git's top-level `-c`.
    +@@ Documentation/config/uploadpack.txt: uploadpackfilter.<filter>.allow::
    + 	combined filters, both `combine` and all of the nested filter
    + 	kinds must be allowed. Defaults to `uploadpackfilter.allow`.

    -+uploadpack.filter.tree.maxDepth::
    ++uploadpackfilter.tree.maxDepth::
     +	Only allow `--filter=tree=<n>` when `n` is no more than the value of
    -+	`uploadpack.filter.tree.maxDepth`. If set, this also implies
    -+	`uploadpack.filter.tree.allow=true`, unless this configuration
    ++	`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::
    @@ t/t5616-partial-clone.sh: test_expect_success 'upload-pack fails banned object f
      '

     +test_expect_success 'upload-pack limits tree depth filters' '
    -+	test_config -C srv.bare uploadpack.filter.allow false &&
    -+	test_config -C srv.bare uploadpack.filter.tree.allow true &&
    -+	test_config -C srv.bare uploadpack.filter.tree.maxDepth 0 &&
    ++	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
     +'
    @@ upload-pack.c: static void die_if_using_banned_filter(struct upload_pack_data *d
      	if (!banned)
      		return;

    --	packet_writer_error(&data->writer, _("filter '%s' not supported\n"),
    --			    list_object_filter_config_name(banned->choice));
    +-	die(_("git upload-pack: filter '%s' not supported"),
    +-	    list_object_filter_config_name(banned->choice));
     +	strbuf_addf(&buf, _("filter '%s' not supported"),
     +		    list_object_filter_config_name(banned->choice));
     +	if (banned->choice == LOFC_TREE_DEPTH &&
    @@ upload-pack.c: static void die_if_using_banned_filter(struct upload_pack_data *d
     +		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);
    -+
    -+	strbuf_release(&buf);
    -+
    - 	die(_("git upload-pack: banned object filter requested"));
    ++	die("%s", buf.buf);
      }

    -@@ upload-pack.c: static void parse_object_filter_config(const char *var, const char *value,
    - 				(void *)(intptr_t)git_config_bool(var, value);
    - 		else
    - 			data->allow_filter_fallback = git_config_bool(var, value);
    -+	} else if (!strcmp(spec.buf, "tree") && !strcmp(key, "maxdepth")) {
    -+		string_list_insert(&data->allowed_filters, "tree")->util
    -+			= (void *) (intptr_t) 1;
    + static void receive_needs(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 =
    + 			(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(&spec);
    + 	strbuf_release(&buf);
    + 	return 0;
--
2.28.0.rc1.13.ge78abce653

             reply	other threads:[~2020-07-23  2:04 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-23  1:48 Taylor Blau [this message]
2020-07-23  1:48 ` [PATCH v2 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau
2020-07-23  1:49 ` [PATCH v2 2/4] upload-pack.c: allow banning certain object filter(s) Taylor Blau
2020-07-23  1:49 ` [PATCH v2 3/4] upload-pack.c: pass 'struct list_objects_filter_options *' Taylor Blau
2020-07-23  1:49 ` [PATCH v2 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau
2020-07-23 20:43 ` [PATCH v2 0/4] upload-pack: custom allowed object filters SZEDER Gábor
2020-07-24 16:51   ` Taylor Blau
2020-07-24 19:51     ` Jeff King
2020-07-27 14:25       ` Taylor Blau
2020-07-27 19:34     ` SZEDER Gábor
2020-07-27 19:36       ` Taylor Blau
2020-07-27 19:42         ` Jeff King
2020-07-27 19:59         ` SZEDER Gábor
2020-07-27 20:03           ` Taylor Blau
2020-07-31 20:26 ` [PATCH v3 " Taylor Blau
2020-07-31 20:26   ` [PATCH v3 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau
2020-07-31 20:26   ` [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s) Taylor Blau
2020-07-31 20:54     ` Jeff King
2020-07-31 21:20       ` Taylor Blau
2020-07-31 20:26   ` [PATCH v3 3/4] upload-pack.c: pass 'struct list_objects_filter_options *' Taylor Blau
2020-07-31 20:26   ` [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau
2020-07-31 21:01     ` Jeff King
2020-07-31 21:22       ` Jeff King
2020-07-31 21:30         ` Taylor Blau
2020-07-31 21:29       ` Taylor Blau
2020-07-31 21:36         ` Jeff King
2020-07-31 21:43           ` Jeff King
2020-08-03 18:00 ` [PATCH v4 0/3] upload-pack: custom allowed object filters Taylor Blau
2020-08-03 18:00   ` [PATCH v4 2/3] upload-pack.c: allow banning certain object filter(s) Taylor Blau
2020-08-03 18:00   ` [PATCH v4 1/3] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau
2020-08-03 18:00   ` [PATCH v4 3/3] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau
2020-08-04  0:37   ` [PATCH v4 0/3] upload-pack: custom allowed object filters Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1595468657.git.me@ttaylorr.com \
    --to=me@ttaylorr.com \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [PATCH v2 0/4] upload-pack: custom allowed object filters' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).