Re: [PATCH] t5500: count objects through stderr, not trace

From: Junio C Hamano <gitster@pobox.com>
Date: May 6, 2020, 22:28 UTC
Message-ID: <xmqqd07g66l1.fsf@gitster.c.googlers.com>
In-Reply-To: <20200506220741.71021-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 14 quoted lines

> In two tests introduced by 4fa3f00abb ("fetch-pack: in protocol v2,
> in_vain only after ACK", 2020-04-28) and 2f0a093dd6 ("fetch-pack: in
> protocol v2, reset in_vain upon ACK", 2020-04-28), the count of objects
> downloaded is checked by grepping for a specific message in the packet
> trace. However, this is flaky as that specific message may be delivered
> over 2 or more packet lines.
>
> Instead, grep over stderr, just like the "fetch creating new shallow
> root" test in the same file.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> Thanks, Dscho. The commits introducing the flakiness have made it to
> master, so this commit is on master.

Hmph. Do we coalesce the payload we receive in adjacent two packet lines into one before writing it out, or do we know in this codepath that the progress messages are the only thing we are writing out to the standard error stream and we do not need to worry about other stuff getting mixed in? If that is the case, perhaps "just like we already do in another test" is a lot weaker justification (it sounds as if we are saying "another test is doing this thing, so if it is broken, we are not making things worse") than what you actually have ("the only thing we are writing out to the standard error stream in the non-error case is these progress messages, and the 'Total 3 ' string, even if it is carried in two separate fragments, will be shown without getting mixed with anything else on the standard error stream").

Assuming that the way I (re)read your justification is correct, the patch looks good to me.

Thanks, both.

Show 43 quoted lines

>  t/t5500-fetch-pack.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 52dd1a688c..8c54e34ef1 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -386,7 +386,7 @@ test_expect_success 'clone shallow with packed refs' '
>  '
>  
>  test_expect_success 'in_vain not triggered before first ACK' '
> -	rm -rf myserver myclient trace &&
> +	rm -rf myserver myclient &&
>  	git init myserver &&
>  	test_commit -C myserver foo &&
>  	git clone "file://$(pwd)/myserver" myclient &&
> @@ -399,12 +399,12 @@ test_expect_success 'in_vain not triggered before first ACK' '
>  	# The new commit that the client wants to fetch.
>  	test_commit -C myserver bar &&
>  
> -	GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin &&
> -	test_i18ngrep "Total 3 " trace
> +	git -C myclient fetch --progress origin 2>log &&
> +	test_i18ngrep "remote: Total 3 " log
>  '
>  
>  test_expect_success 'in_vain resetted upon ACK' '
> -	rm -rf myserver myclient trace &&
> +	rm -rf myserver myclient &&
>  	git init myserver &&
>  
>  	# Linked list of commits on master. The first is common; the rest are
> @@ -429,8 +429,8 @@ test_expect_success 'in_vain resetted upon ACK' '
>  	# first. The 256th commit is common between the client and the server,
>  	# and should reset in_vain. This allows negotiation to continue until
>  	# the client reports that first_anotherbranch_commit is common.
> -	GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin master &&
> -	test_i18ngrep "Total 3 " trace
> +	git -C myclient fetch --progress origin master 2>log &&
> +	test_i18ngrep "Total 3 " log
>  '
>  
>  test_expect_success 'fetch in shallow repo unreachable shallow objects' '