patcht5516: fail to run in verbose mode

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(-)

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

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

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.

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

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

Jeff King <peff@peff.net> writes:

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".

Yeah, I agree.

Thanks.

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

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

Jeff King <peff@peff.net> writes:

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

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

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