[PATCH] t5516: fail to run in verbose mode

[PATCH] t5516: fail to run in verbose mode

[Jiang Xin]

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

The test case "push with config push.useBitmap" of t5516 was introduced
in commit 82f67ee13f (send-pack.c: add config push.useBitmaps,
2022-06-17). It won't work in verbose mode, e.g.:

    $ sh t5516-fetch-push.sh --run='1,115' -v

This is because "git-push" will run in a tty in this case, and the
subcommand "git pack-objects" will contain an argument "--progress"
instead of "-q". Adding a specific option "--quiet" to "git push" will
get a stable result for t5516.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 t/t5516-fetch-push.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4f2bfaf005..98a27a2948 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1858,19 +1858,19 @@ test_expect_success 'push with config push.useBitmaps' '
 	git checkout main &&
 	test_unconfig push.useBitmaps &&
 	GIT_TRACE2_EVENT="$PWD/default" \
-	git push testrepo main:test &&
+	git push --quiet testrepo main:test &&
 	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
 		--thin --delta-base-offset -q <default &&
 
 	test_config push.useBitmaps true &&
 	GIT_TRACE2_EVENT="$PWD/true" \
-	git push testrepo main:test2 &&
+	git push --quiet testrepo main:test2 &&
 	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
 		--thin --delta-base-offset -q <true &&
 
 	test_config push.useBitmaps false &&
 	GIT_TRACE2_EVENT="$PWD/false" \
-	git push testrepo main:test3 &&
+	git push --quiet testrepo main:test3 &&
 	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
 		--thin --delta-base-offset -q --no-use-bitmap-index <false
 '
-- 
2.38.1.420.g319605f8f0

RE: [Internet][PATCH] t5516: fail to run in verbose mode

[kylezhao(赵柯宇)]

> The test case "push with config push.useBitmap" of t5516 was introduced in
> commit 82f67ee13f (send-pack.c: add config push.useBitmaps, 2022-06-17). It
> won't work in verbose mode, e.g.:
> 
>     $ sh t5516-fetch-push.sh --run='1,115' -v
> 
> This is because "git-push" will run in a tty in this case, and the subcommand
> "git pack-objects" will contain an argument "--progress"
> instead of "-q". Adding a specific option "--quiet" to "git push" will get a stable
> result for t5516.
> 

That's true. Thanks for noticing and fixing.

I didn't consider this usage of test case, and it works after your patch.

Thanks,
Kyle

Re: [PATCH] t5516: fail to run in verbose mode

[Junio C Hamano]

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> The test case "push with config push.useBitmap" of t5516 was introduced
> in commit 82f67ee13f (send-pack.c: add config push.useBitmaps,
> 2022-06-17). It won't work in verbose mode, e.g.:
>
>     $ sh t5516-fetch-push.sh --run='1,115' -v
>
> This is because "git-push" will run in a tty in this case, and the

Right.  "-v" involves redirecting the stdout/stderr of the commands
being run in the test to stdout/stderr in the environment the tests
are run, so

    $ sh t5516-fetch-push.sh --run='1,115' -v >log 2>&1

would have succeeded correctly.  Forcing the behaviour with the
"--quiet" option is certainly a good way to gain stability.

Re: [PATCH] t5516: fail to run in verbose mode

[Jeff King]

On Tue, Nov 22, 2022 at 01:19:39PM +0900, Junio C Hamano wrote:

> Jiang Xin <worldhello.net@gmail.com> writes:
> 
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > The test case "push with config push.useBitmap" of t5516 was introduced
> > in commit 82f67ee13f (send-pack.c: add config push.useBitmaps,
> > 2022-06-17). It won't work in verbose mode, e.g.:
> >
> >     $ sh t5516-fetch-push.sh --run='1,115' -v
> >
> > This is because "git-push" will run in a tty in this case, and the
> 
> Right.  "-v" involves redirecting the stdout/stderr of the commands
> being run in the test to stdout/stderr in the environment the tests
> are run, so
> 
>     $ sh t5516-fetch-push.sh --run='1,115' -v >log 2>&1
> 
> would have succeeded correctly.  Forcing the behaviour with the
> "--quiet" option is certainly a good way to gain stability.

I agree this is a good fix for now, but I wonder philosophically what it
means. That is, I could see two arguments:

  1. Our tests sometimes run with stderr connected to a tty and
     sometimes not. This means the test environment isn't consistent,
     and perhaps we should be piping all "-v" tests through "cat" or
     something so that the environment is stable.

  2. Having "-v" run through a tty is yet another source of variation
     that may help us find bugs (though of course sometimes it finds
     false positives). I'm a little uncomfortable with this just because
     it's not providing us variation in any kind of planned or cohesive
     way. This failure lasted for months before somebody noticed it was
     broken under "-v".

     But if we take that approach, then tests are responsible for
     handling both cases. Passing "--quiet" here works, though it feels
     like making test_subcommand more flexible is the right fix. Looks
     like we had an "inexact" variant at one point, but it was dropped
     (because it was buggy) in 16dcec218b (test-lib-functions: remove
     test_subcommand_inexact, 2022-03-25).

None of which needs to hold up this patch, but I wonder if we'd want to
pursue the larger fix in (1).

-Peff

Re: [PATCH] t5516: fail to run in verbose mode

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I agree this is a good fix for now, but I wonder philosophically what it
> means. That is, I could see two arguments:
>
>   1. Our tests sometimes run with stderr connected to a tty and
>      sometimes not. This means the test environment isn't consistent,
>      and perhaps we should be piping all "-v" tests through "cat" or
>      something so that the environment is stable.

Yes, this is tempting (and I almost brought it up in my response),
and a pipe to "|cat" may be hopefully closer than tester's tty to
the redirection to "/dev/null".  I didn't know how much closer,
though, and the differences may still matter (we could teach "git
grep" or "git diff --exit-code" to notice that the output is sent to
/dev/null and stop at the first hit, for example), but still "|cat"
is closer than ">/dev/tty".

> None of which needs to hold up this patch, but I wonder if we'd want to
> pursue the larger fix in (1).

Yeah, I agree.

Thanks.

Re: [PATCH] t5516: fail to run in verbose mode

[Jeff King]

On Wed, Nov 23, 2022 at 10:17:29AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I agree this is a good fix for now, but I wonder philosophically what it
> > means. That is, I could see two arguments:
> >
> >   1. Our tests sometimes run with stderr connected to a tty and
> >      sometimes not. This means the test environment isn't consistent,
> >      and perhaps we should be piping all "-v" tests through "cat" or
> >      something so that the environment is stable.
> 
> Yes, this is tempting (and I almost brought it up in my response),
> and a pipe to "|cat" may be hopefully closer than tester's tty to
> the redirection to "/dev/null".  I didn't know how much closer,
> though, and the differences may still matter (we could teach "git
> grep" or "git diff --exit-code" to notice that the output is sent to
> /dev/null and stop at the first hit, for example), but still "|cat"
> is closer than ">/dev/tty".

One thing I'd worry about is buffering. One of the nice things about
"-v" is that there is nothing between you and the running programs, so
you are much less likely to be fooled about the order of events in the
output. Or wondering why nothing is happening because real-time output
seems to have stalled. But piping through "cat" may end up with weird
pauses while it fills up a 4k buffer. Using stdbuf could help, but
that's far from portable.

-Peff

Re: [PATCH] t5516: fail to run in verbose mode

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> One thing I'd worry about is buffering. One of the nice things about
> "-v" is that there is nothing between you and the running programs, so
> you are much less likely to be fooled about the order of events in the
> output. Or wondering why nothing is happening because real-time output
> seems to have stalled. But piping through "cat" may end up with weird
> pauses while it fills up a 4k buffer. Using stdbuf could help, but
> that's far from portable.

We could pipe to "dd bs=1 conv=fsync" (tongue-in-cheek---I think
conv=fsync is a GNU thing).

Re: [PATCH] t5516: fail to run in verbose mode

[Jeff King]

On Fri, Nov 25, 2022 at 01:58:55PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > One thing I'd worry about is buffering. One of the nice things about
> > "-v" is that there is nothing between you and the running programs, so
> > you are much less likely to be fooled about the order of events in the
> > output. Or wondering why nothing is happening because real-time output
> > seems to have stalled. But piping through "cat" may end up with weird
> > pauses while it fills up a 4k buffer. Using stdbuf could help, but
> > that's far from portable.
> 
> We could pipe to "dd bs=1 conv=fsync" (tongue-in-cheek---I think
> conv=fsync is a GNU thing).

We don't need the fsync; we are just worried about in-process buffering,
not kernel-level flushing. So bs=1 is sufficient, in that it would
use syscalls to read/write single bytes. It would just be horribly
inefficient.

We really just want immediate partial read()/write() but with sensible
buffer sizes. It would be easy to write a 5-line C program that did this
if we really wanted to. I'm not entirely convinced it's worth worrying
too much about, though.

-Peff