git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] t5516: fail to run in verbose mode
@ 2022-11-21 13:40 Jiang Xin
  2022-11-22  2:22 ` [Internet][PATCH] " kylezhao(赵柯宇)
  2022-11-22  4:19 ` [PATCH] " Junio C Hamano
  0 siblings, 2 replies; 8+ messages in thread
From: Jiang Xin @ 2022-11-21 13:40 UTC (permalink / raw)
  To: Taylor Blau, Git List, Kyle Zhao; +Cc: Jiang Xin, 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* RE: [Internet][PATCH] t5516: fail to run in verbose mode
  2022-11-21 13:40 [PATCH] t5516: fail to run in verbose mode Jiang Xin
@ 2022-11-22  2:22 ` kylezhao(赵柯宇)
  2022-11-22  4:19 ` [PATCH] " Junio C Hamano
  1 sibling, 0 replies; 8+ messages in thread
From: kylezhao(赵柯宇) @ 2022-11-22  2:22 UTC (permalink / raw)
  To: Jiang Xin, Taylor Blau, Git List; +Cc: Jiang Xin

> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t5516: fail to run in verbose mode
  2022-11-21 13:40 [PATCH] t5516: fail to run in verbose mode Jiang Xin
  2022-11-22  2:22 ` [Internet][PATCH] " kylezhao(赵柯宇)
@ 2022-11-22  4:19 ` Junio C Hamano
  2022-11-22 19:08   ` Jeff King
  1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2022-11-22  4:19 UTC (permalink / raw)
  To: Jiang Xin; +Cc: Taylor Blau, Git List, Kyle Zhao, Jiang Xin

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t5516: fail to run in verbose mode
  2022-11-22  4:19 ` [PATCH] " Junio C Hamano
@ 2022-11-22 19:08   ` Jeff King
  2022-11-23  1:17     ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2022-11-22 19:08 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jiang Xin, Taylor Blau, Git List, Kyle Zhao, Jiang Xin

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t5516: fail to run in verbose mode
  2022-11-22 19:08   ` Jeff King
@ 2022-11-23  1:17     ` Junio C Hamano
  2022-11-24  1:16       ` Jeff King
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2022-11-23  1:17 UTC (permalink / raw)
  To: Jeff King; +Cc: Jiang Xin, Taylor Blau, Git List, Kyle Zhao, Jiang Xin

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t5516: fail to run in verbose mode
  2022-11-23  1:17     ` Junio C Hamano
@ 2022-11-24  1:16       ` Jeff King
  2022-11-25  4:58         ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2022-11-24  1:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jiang Xin, Taylor Blau, Git List, Kyle Zhao, Jiang Xin

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t5516: fail to run in verbose mode
  2022-11-24  1:16       ` Jeff King
@ 2022-11-25  4:58         ` Junio C Hamano
  2022-11-28  5:13           ` Jeff King
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2022-11-25  4:58 UTC (permalink / raw)
  To: Jeff King; +Cc: Jiang Xin, Taylor Blau, Git List, Kyle Zhao, Jiang Xin

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] t5516: fail to run in verbose mode
  2022-11-25  4:58         ` Junio C Hamano
@ 2022-11-28  5:13           ` Jeff King
  0 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2022-11-28  5:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jiang Xin, Taylor Blau, Git List, Kyle Zhao, Jiang Xin

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-11-28  5:16 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-21 13:40 [PATCH] t5516: fail to run in verbose mode Jiang Xin
2022-11-22  2:22 ` [Internet][PATCH] " kylezhao(赵柯宇)
2022-11-22  4:19 ` [PATCH] " Junio C Hamano
2022-11-22 19:08   ` Jeff King
2022-11-23  1:17     ` Junio C Hamano
2022-11-24  1:16       ` Jeff King
2022-11-25  4:58         ` Junio C Hamano
2022-11-28  5:13           ` Jeff King

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).