Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jul 15, 2020, 10:00 UTC
Message-ID: <20200715100043.GG11341@szeder.dev>
In-Reply-To: <f0982d24e74155f6c0e405e5e3ae8c3e579f798a.1593720075.git.me@ttaylorr.com>

On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:

Show 33 quoted lines

> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 8a27452a51..5dcd0b5656 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -235,6 +235,32 @@ test_expect_success 'implicitly construct combine: filter with repeated flags' '
>  	test_cmp unique_types.expected unique_types.actual
>  '
>  
> +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_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_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_must_fail git clone --no-checkout --filter=blob:none \
> +		"file://$(pwd)/srv.bare" pc3 2>err &&
> +	test_i18ngrep "filter '\''blob:none'\'' not supported" err
> +'

These three tests are very flaky: 'git upload-pack' can error out while clone is still sending packets (usually the 'done' line), resulting in SIGPIPE and frequent CI failures. Running this test script with '-r 1,2,17-19 --stress' tends to fail in a couple of seconds.

Using 'test_must_fail ok=sigpipe', as you did in the test in the last patch, avoids the test failure caused by SIGPIPE, of course, but, unfortunately, all three tests remain flaky, because the expected error message sometimes doesn't make it to 'git clone's stderr, e.g.:

  expecting success of 5616.19 'upload-pack fails banned object filters with fallback': 
          test_config -C srv.bare uploadpack.filter.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
  
  + test_config -C srv.bare uploadpack.filter.allow false
  + pwd
  + test_must_fail ok=sigpipe git clone --no-checkout --filter=blob:none file:///home/szeder/src/git/t/trash directory.t5616-partial-clone.stress-2/srv.bare pc3
  + test_i18ngrep filter 'blob:none' not supported err
  error: 'grep filter 'blob:none' not supported err' didn't find a match in:
  Cloning into 'pc3'...
  fatal: git upload-pack: banned object filter requested
  error: last command exited with $?=1
  not ok 19 - upload-pack fails banned object filters with fallback

Once upon a time I had a PoC patch to deal with 'git upload-pack' aborting while 'git fetch' is still send_request()-ing, by catching the write error to the closed connection and trying read any pending ERR packets; Christian cleaned it up and submitted it with a proper commit message in

  https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/

but it haven't been picked up yet. Disappointingly, that patch doesn't solve these issues... I haven't looked what's going on (perhaps 'git clone' does something differently than 'git fetch'? no idea)