On Tue, Nov 22, 2022 at 01:19:39PM +0900, Junio C Hamano wrote:
> Jiang Xin <worldhello.net@xxxxxxxxx> writes:
>
> > From: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
> >
> > 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
