git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 0/4] upload-pack: custom allowed object filters

From
Taylor Blau <me@ttaylorr.com>
Date
Jul 27, 2020, 14:25 UTC
Message-ID
<20200727142511.GA62919@syl.lan>
In-Reply-To
<20200724195126.GB4013174@coredump.intra.peff.net>
On Fri, Jul 24, 2020 at 03:51:26PM -0400, Jeff King wrote:
Show 25 quoted lines
> On Fri, Jul 24, 2020 at 12:51:33PM -0400, Taylor Blau wrote:
>
> > Let me double check my understanding... I think that you are suggesting
> > the following three things:
> >
> >   - Write the same message as an err packet over the wire as we do when
> >     'die()'ing from inside of upload-pack.c
> >
> >   - Don't mark said message(s) for translation, matching what we do in
> >     the rest of upload-pack.c.
> >
> >   - Re-introduce the 'test_must_fail ok=sigpipe' and stop grepping
> >     stderr for the right message.
> >
> > Do I have that right?
>
> I'm not Gábor, but that is the sequence I think is best.
>
> Between the die() and the ERR, the ERR packets are way more useful in
> practice, since they actually go back to the client. I'd even suggest we
> do away with the die() messages entirely (since they're either redundant
> or go nowhere, depending on the protocol), but I think it would make
> sense to wait until the raciness issues are fixed (until then, they
> _might_ help in the redundant cases, which is what the test here is
> relying on).

All sounds good. I'll wait for an ACK from Gábor to make sure that this is the direction that they want, too.

In the meantime, this is what I have locally. I'll send a series which has this as its range-diff if Gábor approves of the direction.

1:  6a77af563e = 1:  6a77af563e list_objects_filter_options: introduce 'list_object_filter_config_name'
2:  6dbf58441d ! 2:  80d19481d8 upload-pack.c: allow banning certain object filter(s)
    @@ t/t5616-partial-clone.sh: test_expect_success 'implicitly construct combine: fil
     +test_expect_success 'upload-pack fails banned object filters' '
     +	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_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
    ++		"file://$(pwd)/srv.bare" pc3
     +'
     +
     +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.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_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \
    ++		--filter=blob:none "file://$(pwd)/srv.bare" pc3
     +'
     +
     +test_expect_success 'upload-pack fails banned object filters with fallback' '
     +	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
    ++	test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none \
    ++		"file://$(pwd)/srv.bare" pc3
     +'
     +
      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
     +{
     +	struct list_objects_filter_options *banned = banned_filter(data,
     +								   &data->filter_options);
    ++	struct strbuf buf = STRBUF_INIT;
     +	if (!banned)
     +		return;
     +
    -+	die(_("git upload-pack: filter '%s' not supported"),
    -+	    list_object_filter_config_name(banned->choice));
    ++	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,
3:  bacdea47d9 = 3:  790552c7c2 upload-pack.c: pass 'struct list_objects_filter_options *'
4:  79af94a41b ! 4:  eb9a81eb1f upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'
    @@ 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
    + 		"file://$(pwd)/srv.bare" pc3
      '
     +test_expect_success 'upload-pack limits tree depth filters' '
    @@ upload-pack.c: static int allows_filter_choice(struct upload_pack_data *data,
      }
     @@ upload-pack.c: 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;
    --	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));
    + 	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);
    -+	die("%s", buf.buf);
    - }
    - static void receive_needs(struct upload_pack_data *data,
    + 	packet_writer_error(&data->writer, "%s\n", buf.buf);
    + 	die("%s", buf.buf);
     @@ 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 =
> -Peff

Thanks, Taylor

Previous: Jeff KingNext: SZEDER Gábor
Message 9 of 32 in “upload-pack: custom allowed object filters”
  1. 0/4 upload-pack: custom allowed object filtersTaylor Blau, Jul 23, 2020
  2. 1/4 list_objects_filter_options: introduce 'list_object_filter_config_name'Taylor Blau, Jul 23, 2020
  3. 2/4 upload-pack.c: allow banning certain object filter(s)Taylor Blau, Jul 23, 2020
  4. 3/4 upload-pack.c: pass 'struct list_objects_filter_options *'Taylor Blau, Jul 23, 2020
  5. 4/4 upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'Taylor Blau, Jul 23, 2020
  6. SZEDER GáborJul 23, 2020
  7. Taylor BlauJul 24, 2020
  8. Jeff KingJul 24, 2020
  9. Taylor BlauJul 27, 2020
  10. SZEDER GáborJul 27, 2020
  11. Taylor BlauJul 27, 2020
  12. Jeff KingJul 27, 2020
  13. SZEDER GáborJul 27, 2020
  14. Taylor BlauJul 27, 2020
  15. 0/4 upload-pack: custom allowed object filtersTaylor Blau, Jul 31, 2020
  16. 1/4 list_objects_filter_options: introduce 'list_object_filter_config_name'Taylor Blau, Jul 31, 2020
  17. 2/4 upload-pack.c: allow banning certain object filter(s)Taylor Blau, Jul 31, 2020
  18. Jeff KingJul 31, 2020
  19. Taylor BlauJul 31, 2020
  20. 3/4 upload-pack.c: pass 'struct list_objects_filter_options *'Taylor Blau, Jul 31, 2020
  21. 4/4 upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'Taylor Blau, Jul 31, 2020
  22. Jeff KingJul 31, 2020
  23. Jeff KingJul 31, 2020
  24. Taylor BlauJul 31, 2020
  25. Taylor BlauJul 31, 2020
  26. Jeff KingJul 31, 2020
  27. Jeff KingJul 31, 2020
  28. 0/3 upload-pack: custom allowed object filtersTaylor Blau, Aug 3, 2020
  29. 2/3 upload-pack.c: allow banning certain object filter(s)Taylor Blau, Aug 3, 2020
  30. 1/3 list_objects_filter_options: introduce 'list_object_filter_config_name'Taylor Blau, Aug 3, 2020
  31. 3/3 upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth'Taylor Blau, Aug 3, 2020
  32. Jeff KingAug 4, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.