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

From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, peff@peff.net, chriscool@tuxfamily.org
Subject: Re: [PATCH 2/4] upload-pack.c: allow banning certain object filter(s)
Date: Wed, 15 Jul 2020 12:00:43 +0200	[thread overview]
Message-ID: <20200715100043.GG11341@szeder.dev> (raw)
In-Reply-To: <f0982d24e74155f6c0e405e5e3ae8c3e579f798a.1593720075.git.me@ttaylorr.com>

On Thu, Jul 02, 2020 at 04:06:32PM -0400, Taylor Blau wrote:
> 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)