{"thread":{"id":"58831","subject":"[PATCH] t5516: fail to run in verbose mode","startedAt":"2022-11-21T13:41:03Z","lastAt":"2022-11-28T05:16:44Z","messageCount":8,"participants":["Jiang Xin","kylezhao (赵柯宇)","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"467688","messageId":"20221121134040.12260-1-worldhello.net@gmail.com","threadId":"58831","inReplyTo":null,"subject":"[PATCH] t5516: fail to run in verbose mode","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2022-11-21T13:40:40Z","receivedAt":"2022-11-21T13:41:03Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin \n\nThe test case \"push with config push.useBitmap\" of t5516 was introduced\nin commit 82f67ee13f (send-pack.c: add config push.useBitmaps,\n2022-06-17). It won't work in verbose mode, e.g.:\n\n $ sh t5516-fetch-push.sh --run='1,115' -v\n\nThis is because \"git-push\" will run in a tty in this case, and the\nsubcommand \"git pack-objects\" will contain an argument \"--progress\"\ninstead of \"-q\". Adding a specific option \"--quiet\" to \"git push\" will\nget a stable result for t5516.\n\nSigned-off-by: Jiang Xin \n---\n t/t5516-fetch-push.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 4f2bfaf005..98a27a2948 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1858,19 +1858,19 @@ test_expect_success 'push with config push.useBitmaps' '\n \tgit checkout main &&\n \ttest_unconfig push.useBitmaps &&\n \tGIT_TRACE2_EVENT=\"$PWD/default\" \\\n-\tgit push testrepo main:test &&\n+\tgit push --quiet testrepo main:test &&\n \ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n \t\t--thin --delta-base-offset -q The test case \"push with config push.useBitmap\" of t5516 was introduced in\n> commit 82f67ee13f (send-pack.c: add config push.useBitmaps, 2022-06-17). It\n> won't work in verbose mode, e.g.:\n> \n> $ sh t5516-fetch-push.sh --run='1,115' -v\n> \n> This is because \"git-push\" will run in a tty in this case, and the subcommand\n> \"git pack-objects\" will contain an argument \"--progress\"\n> instead of \"-q\". Adding a specific option \"--quiet\" to \"git push\" will get a stable\n> result for t5516.\n> \n\nThat's true. Thanks for noticing and fixing.\n\nI didn't consider this usage of test case, and it works after your patch.\n\nThanks,\nKyle\n"},{"id":"467746","messageId":"xmqqo7szsjs4.fsf@gitster.g","threadId":"58831","inReplyTo":"20221121134040.12260-1-worldhello.net@gmail.com","subject":"Re: [PATCH] t5516: fail to run in verbose mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-22T04:19:39Z","receivedAt":"2022-11-22T04:19:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> From: Jiang Xin \n>\n> The test case \"push with config push.useBitmap\" of t5516 was introduced\n> in commit 82f67ee13f (send-pack.c: add config push.useBitmaps,\n> 2022-06-17). It won't work in verbose mode, e.g.:\n>\n> $ sh t5516-fetch-push.sh --run='1,115' -v\n>\n> This is because \"git-push\" will run in a tty in this case, and the\n\nRight. \"-v\" involves redirecting the stdout/stderr of the commands\nbeing run in the test to stdout/stderr in the environment the tests\nare run, so\n\n $ sh t5516-fetch-push.sh --run='1,115' -v >log 2>&1\n\nwould have succeeded correctly. Forcing the behaviour with the\n\"--quiet\" option is certainly a good way to gain stability.\n"},{"id":"467801","messageId":"Y30eK/mNYeLNtOi2@coredump.intra.peff.net","threadId":"58831","inReplyTo":"xmqqo7szsjs4.fsf@gitster.g","subject":"Re: [PATCH] t5516: fail to run in verbose mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-22T19:08:27Z","receivedAt":"2022-11-22T19:08:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 22, 2022 at 01:19:39PM +0900, Junio C Hamano wrote:\n\n> Jiang Xin writes:\n> \n> > From: Jiang Xin \n> >\n> > The test case \"push with config push.useBitmap\" of t5516 was introduced\n> > in commit 82f67ee13f (send-pack.c: add config push.useBitmaps,\n> > 2022-06-17). It won't work in verbose mode, e.g.:\n> >\n> > $ sh t5516-fetch-push.sh --run='1,115' -v\n> >\n> > This is because \"git-push\" will run in a tty in this case, and the\n> \n> Right. \"-v\" involves redirecting the stdout/stderr of the commands\n> being run in the test to stdout/stderr in the environment the tests\n> are run, so\n> \n> $ sh t5516-fetch-push.sh --run='1,115' -v >log 2>&1\n> \n> would have succeeded correctly. Forcing the behaviour with the\n> \"--quiet\" option is certainly a good way to gain stability.\n\nI agree this is a good fix for now, but I wonder philosophically what it\nmeans. That is, I could see two arguments:\n\n 1. Our tests sometimes run with stderr connected to a tty and\n sometimes not. This means the test environment isn't consistent,\n and perhaps we should be piping all \"-v\" tests through \"cat\" or\n something so that the environment is stable.\n\n 2. Having \"-v\" run through a tty is yet another source of variation\n that may help us find bugs (though of course sometimes it finds\n false positives). I'm a little uncomfortable with this just because\n it's not providing us variation in any kind of planned or cohesive\n way. This failure lasted for months before somebody noticed it was\n broken under \"-v\".\n\n But if we take that approach, then tests are responsible for\n handling both cases. Passing \"--quiet\" here works, though it feels\n like making test_subcommand more flexible is the right fix. Looks\n like we had an \"inexact\" variant at one point, but it was dropped\n (because it was buggy) in 16dcec218b (test-lib-functions: remove\n test_subcommand_inexact, 2022-03-25).\n\nNone of which needs to hold up this patch, but I wonder if we'd want to\npursue the larger fix in (1).\n\n-Peff\n"},{"id":"467840","messageId":"xmqqwn7mo4eu.fsf@gitster.g","threadId":"58831","inReplyTo":"Y30eK/mNYeLNtOi2@coredump.intra.peff.net","subject":"Re: [PATCH] t5516: fail to run in verbose mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-23T01:17:29Z","receivedAt":"2022-11-23T01:19:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I agree this is a good fix for now, but I wonder philosophically what it\n> means. That is, I could see two arguments:\n>\n> 1. Our tests sometimes run with stderr connected to a tty and\n> sometimes not. This means the test environment isn't consistent,\n> and perhaps we should be piping all \"-v\" tests through \"cat\" or\n> something so that the environment is stable.\n\nYes, this is tempting (and I almost brought it up in my response),\nand a pipe to \"|cat\" may be hopefully closer than tester's tty to\nthe redirection to \"/dev/null\". I didn't know how much closer,\nthough, and the differences may still matter (we could teach \"git\ngrep\" or \"git diff --exit-code\" to notice that the output is sent to\n/dev/null and stop at the first hit, for example), but still \"|cat\"\nis closer than \">/dev/tty\".\n\n> None of which needs to hold up this patch, but I wonder if we'd want to\n> pursue the larger fix in (1).\n\nYeah, I agree.\n\nThanks.\n"},{"id":"467899","messageId":"Y37F1nxifC8o8qEQ@coredump.intra.peff.net","threadId":"58831","inReplyTo":"xmqqwn7mo4eu.fsf@gitster.g","subject":"Re: [PATCH] t5516: fail to run in verbose mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-24T01:16:06Z","receivedAt":"2022-11-24T01:16:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 23, 2022 at 10:17:29AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I agree this is a good fix for now, but I wonder philosophically what it\n> > means. That is, I could see two arguments:\n> >\n> > 1. Our tests sometimes run with stderr connected to a tty and\n> > sometimes not. This means the test environment isn't consistent,\n> > and perhaps we should be piping all \"-v\" tests through \"cat\" or\n> > something so that the environment is stable.\n> \n> Yes, this is tempting (and I almost brought it up in my response),\n> and a pipe to \"|cat\" may be hopefully closer than tester's tty to\n> the redirection to \"/dev/null\". I didn't know how much closer,\n> though, and the differences may still matter (we could teach \"git\n> grep\" or \"git diff --exit-code\" to notice that the output is sent to\n> /dev/null and stop at the first hit, for example), but still \"|cat\"\n> is closer than \">/dev/tty\".\n\nOne thing I'd worry about is buffering. One of the nice things about\n\"-v\" is that there is nothing between you and the running programs, so\nyou are much less likely to be fooled about the order of events in the\noutput. Or wondering why nothing is happening because real-time output\nseems to have stalled. But piping through \"cat\" may end up with weird\npauses while it fills up a 4k buffer. Using stdbuf could help, but\nthat's far from portable.\n\n-Peff\n"},{"id":"467968","messageId":"xmqqzgcfeik0.fsf@gitster.g","threadId":"58831","inReplyTo":"Y37F1nxifC8o8qEQ@coredump.intra.peff.net","subject":"Re: [PATCH] t5516: fail to run in verbose mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-25T04:58:55Z","receivedAt":"2022-11-25T04:59:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> One thing I'd worry about is buffering. One of the nice things about\n> \"-v\" is that there is nothing between you and the running programs, so\n> you are much less likely to be fooled about the order of events in the\n> output. Or wondering why nothing is happening because real-time output\n> seems to have stalled. But piping through \"cat\" may end up with weird\n> pauses while it fills up a 4k buffer. Using stdbuf could help, but\n> that's far from portable.\n\nWe could pipe to \"dd bs=1 conv=fsync\" (tongue-in-cheek---I think\nconv=fsync is a GNU thing).\n"},{"id":"468061","messageId":"Y4RDkGZE3cZFS25d@coredump.intra.peff.net","threadId":"58831","inReplyTo":"xmqqzgcfeik0.fsf@gitster.g","subject":"Re: [PATCH] t5516: fail to run in verbose mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-28T05:13:52Z","receivedAt":"2022-11-28T05:16:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 25, 2022 at 01:58:55PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > One thing I'd worry about is buffering. One of the nice things about\n> > \"-v\" is that there is nothing between you and the running programs, so\n> > you are much less likely to be fooled about the order of events in the\n> > output. Or wondering why nothing is happening because real-time output\n> > seems to have stalled. But piping through \"cat\" may end up with weird\n> > pauses while it fills up a 4k buffer. Using stdbuf could help, but\n> > that's far from portable.\n> \n> We could pipe to \"dd bs=1 conv=fsync\" (tongue-in-cheek---I think\n> conv=fsync is a GNU thing).\n\nWe don't need the fsync; we are just worried about in-process buffering,\nnot kernel-level flushing. So bs=1 is sufficient, in that it would\nuse syscalls to read/write single bytes. It would just be horribly\ninefficient.\n\nWe really just want immediate partial read()/write() but with sensible\nbuffer sizes. It would be easy to write a 5-line C program that did this\nif we really wanted to. I'm not entirely convinced it's worth worrying\ntoo much about, though.\n\n-Peff\n"}]}