{"thread":{"id":"41047","subject":"[PATCH v1 0/2] git-p4: kill watchdog and suppress irrelevant output","startedAt":"2015-12-20T16:44:27Z","lastAt":"2015-12-24T11:09:35Z","messageCount":8,"participants":["larsxschneider@gmail.com","Junio C Hamano","Lars Schneider","Luke Diamand"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"274782","messageId":"1450629869-49522-1-git-send-email-larsxschneider@gmail.com","threadId":"41047","inReplyTo":null,"subject":"[PATCH v1 0/2] git-p4: kill watchdog and suppress irrelevant output","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-12-20T16:44:27Z","receivedAt":"2015-12-20T16:44:27Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nHi,\n\nthese patches extend \"git-p4: add trap to kill p4d on test exit\" (dfe90e8)\nand therefore should be applied on master.\n\nThanks,\nLars\n\nLars Schneider (2):\n  git-p4: kill p4d watchdog on cleanup\n  git-p4: suppress non test relevant output\n\n t/lib-git-p4.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\n--\n2.5.1\n"},{"id":"274783","messageId":"1450629869-49522-2-git-send-email-larsxschneider@gmail.com","threadId":"41047","inReplyTo":"1450629869-49522-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v1 1/2] git-p4: kill p4d watchdog on cleanup","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-12-20T16:44:28Z","receivedAt":"2015-12-20T16:44:28Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nIf failing tests are executed with the \"immediate\" flag then \"kill_p4d\"\nis not called and consequently the watchdog process, which is supposed\nto detect a hanging p4d, is not killed. Kill the watchdog always in the\n\"on exit\" cleanup trap.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n t/lib-git-p4.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex f9ae1d7..30bf7ae 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -76,6 +76,7 @@ pidfile=\"$TRASH_DIRECTORY/p4d.pid\"\n\n # Sometimes \"prove\" seems to hang on exit because p4d is still running\n cleanup() {\n+\tkill -9 $watchdog_pid 2>/dev/null\n \tif test -f \"$pidfile\"\n \tthen\n \t\tkill -9 $(cat \"$pidfile\") 2>/dev/null && exit 255\n--\n2.5.1\n"},{"id":"274784","messageId":"1450629869-49522-3-git-send-email-larsxschneider@gmail.com","threadId":"41047","inReplyTo":"1450629869-49522-1-git-send-email-larsxschneider@gmail.com","subject":"[PATCH v1 2/2] git-p4: suppress non test relevant output","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-12-20T16:44:29Z","receivedAt":"2015-12-20T16:44:29Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nIf tests are executed in verbose mode then the retry logic clutters the\ntest output. Suppress that clutter.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n t/lib-git-p4.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\nindex 30bf7ae..03f29c1 100644\n--- a/t/lib-git-p4.sh\n+++ b/t/lib-git-p4.sh\n@@ -174,7 +174,7 @@ retry_until_fail() {\n \tuntil ! \"$@\" 2>/dev/null || test $(time_in_seconds) -gt $timeout\n \tdo\n \t\tsleep 1\n-\tdone\n+\tdone >/dev/null 2>&1\n }\n\n kill_p4d() {\n--\n2.5.1\n"},{"id":"274824","messageId":"xmqqh9jbfsra.fsf@gitster.mtv.corp.google.com","threadId":"41047","inReplyTo":"1450629869-49522-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v1 0/2] git-p4: kill watchdog and suppress irrelevant output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T20:31:37Z","receivedAt":"2015-12-21T20:31:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> From: Lars Schneider <larsxschneider@gmail.com>\n>\n> Hi,\n>\n> these patches extend \"git-p4: add trap to kill p4d on test exit\" (dfe90e8)\n> and therefore should be applied on master.\n\nWait, wait.  Please be a bit more careful when you use such a\nphrasing.  Did somebody review these and said that these are\ntrivially correct improvements?\n\nThis depends on what you exactly meant \"extend\" and \"improve\" (for\nthe other one); if the \"improvement\" were to make something that\nused to be unusable to usable, then the more sensible way forward\nduring the -rc stabilization period might be to revert the earlier\nmerges to 'master' that brought in undercooked topic.\n\nThanks.\n\n>\n> Thanks,\n> Lars\n>\n> Lars Schneider (2):\n>   git-p4: kill p4d watchdog on cleanup\n>   git-p4: suppress non test relevant output\n>\n>  t/lib-git-p4.sh | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> --\n> 2.5.1\n"},{"id":"274825","messageId":"xmqqd1tzfsfm.fsf@gitster.mtv.corp.google.com","threadId":"41047","inReplyTo":"1450629869-49522-3-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v1 2/2] git-p4: suppress non test relevant output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-21T20:38:37Z","receivedAt":"2015-12-21T20:38:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> From: Lars Schneider <larsxschneider@gmail.com>\n>\n> If tests are executed in verbose mode then the retry logic clutters the\n> test output. Suppress that clutter.\n>\n> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n> ---\n>  t/lib-git-p4.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\n> index 30bf7ae..03f29c1 100644\n> --- a/t/lib-git-p4.sh\n> +++ b/t/lib-git-p4.sh\n> @@ -174,7 +174,7 @@ retry_until_fail() {\n>  \tuntil ! \"$@\" 2>/dev/null || test $(time_in_seconds) -gt $timeout\n>  \tdo\n>  \t\tsleep 1\n> -\tdone\n> +\tdone >/dev/null 2>&1\n\nEh, what does this squelch?  The sleep in the body of the loop is\nsilent, \"test A -gt B\" on the loop condition would be silent too, so\nyou are squelching the invocation of \"$@\" whose standard error\nstream is already sent to 2>/dev/null?\n\nIf so, why not do it there instead?  You seem to run only \"kill\" to\nsend some signal to a process using this helper function, and it\nwould be silent on its standard output stream (even though it may\nsay \"no such process\" etc. on its standard error), so it is not\nclear to me what you are doing with this change here...\n"},{"id":"274845","messageId":"0185CA76-DDEE-4E7F-8EFF-65E80720E0AF@gmail.com","threadId":"41047","inReplyTo":"xmqqd1tzfsfm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/2] git-p4: suppress non test relevant output","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-12-22T08:47:35Z","receivedAt":"2015-12-22T08:47:35Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 21 Dec 2015, at 21:38, Junio C Hamano <gitster@pobox.com> wrote:\n\n> larsxschneider@gmail.com writes:\n> \n>> From: Lars Schneider <larsxschneider@gmail.com>\n>> \n>> If tests are executed in verbose mode then the retry logic clutters the\n>> test output. Suppress that clutter.\n>> \n>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>\n>> ---\n>> t/lib-git-p4.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>> \n>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh\n>> index 30bf7ae..03f29c1 100644\n>> --- a/t/lib-git-p4.sh\n>> +++ b/t/lib-git-p4.sh\n>> @@ -174,7 +174,7 @@ retry_until_fail() {\n>> \tuntil ! \"$@\" 2>/dev/null || test $(time_in_seconds) -gt $timeout\n>> \tdo\n>> \t\tsleep 1\n>> -\tdone\n>> +\tdone >/dev/null 2>&1\n> \n> Eh, what does this squelch?  The sleep in the body of the loop is\n> silent, \"test A -gt B\" on the loop condition would be silent too, so\n> you are squelching the invocation of \"$@\" whose standard error\n> stream is already sent to 2>/dev/null?\n> \n> If so, why not do it there instead?  You seem to run only \"kill\" to\n> send some signal to a process using this helper function, and it\n> would be silent on its standard output stream (even though it may\n> say \"no such process\" etc. on its standard error), so it is not\n> clear to me what you are doing with this change here...\n\nIf I run git-p4 tests in verbose mode (e.g. \"./t9823-git-p4-mock-lfs.sh -v\") without this patch then the last lines of the output look like this:\n\n>>> Output Start >>>\nexpecting success:\n\tkill_p4d\n\n./lib-git-p4.sh: line 172: 26289 Killed: 9               while true; do\n    if test $(time_in_seconds) -gt $timeout; then\n        kill -9 $pid; exit 1;\n    fi; sleep 1;\ndone\nok 8 - kill p4d\n\n# passed all 8 test(s)\n1..8 \n<<< Output end <<<\n\nHowever, I want them to look like this:\n\n>>> Output Start >>>\nexpecting success:\n\tkill_p4d\n\nok 8 - kill p4d\n\n# passed all 8 test(s)\n1..8\n<<< Output end <<<\n\nThis is achieved with the patch. I am no shell expert ... is there a nicer way to achieve the same?\n\nThanks,\nLars\n"},{"id":"274846","messageId":"F3AA0745-A9C1-40D4-AF63-EDBFB7A5DB19@gmail.com","threadId":"41047","inReplyTo":"xmqqh9jbfsra.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 0/2] git-p4: kill watchdog and suppress irrelevant output","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-12-22T09:12:15Z","receivedAt":"2015-12-22T09:12:15Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 21 Dec 2015, at 21:31, Junio C Hamano <gitster@pobox.com> wrote:\n\n> larsxschneider@gmail.com writes:\n> \n>> From: Lars Schneider <larsxschneider@gmail.com>\n>> \n>> Hi,\n>> \n>> these patches extend \"git-p4: add trap to kill p4d on test exit\" (dfe90e8)\n>> and therefore should be applied on master.\n> \n> Wait, wait.  Please be a bit more careful when you use such a\n> phrasing.  Did somebody review these and said that these are\n> trivially correct improvements?\n> \n> This depends on what you exactly meant \"extend\" and \"improve\" (for\n> the other one); if the \"improvement\" were to make something that\n> used to be unusable to usable, then the more sensible way forward\n> during the -rc stabilization period might be to revert the earlier\n> merges to 'master' that brought in undercooked topic.\n\nOh. Thanks or making me aware of the inappropriate phrases! \nNobody reviewed these patches, yet! \n\nMy intention was to let the reviewer know that this patch series is based on \n\"master\" and not on \"maint\". Sorry for the confusion!\n\nThe patch series \"git-p4: kill watchdog and suppress irrelevant output\" fixes\nsomething annoying that I did not spot earlier. In other words it changes\n\"usable but a bit annoying (dfe90e8)\" to \"usable\".\n\nThe patch series \"git-p4: ignore P4 changelists that only touch files\" changes \n\"usable (4ae048e)\" to \"maybe even better\" depending on the reviewers\nopinion.\n\nThanks,\nLars\n"},{"id":"274988","messageId":"567BD26F.3020006@diamand.org","threadId":"41047","inReplyTo":"0185CA76-DDEE-4E7F-8EFF-65E80720E0AF@gmail.com","subject":"Re: [PATCH v1 2/2] git-p4: suppress non test relevant output","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2015-12-24T11:09:35Z","receivedAt":"2015-12-24T11:09:35Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 22/12/15 08:47, Lars Schneider wrote:\n>\n> On 21 Dec 2015, at 21:38, Junio C Hamano <gitster@pobox.com> wrote:\n>\n\n>>\n>> If so, why not do it there instead?  You seem to run only \"kill\" to\n>> send some signal to a process using this helper function, and it\n>> would be silent on its standard output stream (even though it may\n>> say \"no such process\" etc. on its standard error), so it is not\n>> clear to me what you are doing with this change here...\n>\n> If I run git-p4 tests in verbose mode (e.g. \"./t9823-git-p4-mock-lfs.sh -v\") without this patch then the last lines of the output look like this:\n>\n>>>> Output Start >>>\n> expecting success:\n> \tkill_p4d\n>\n> ./lib-git-p4.sh: line 172: 26289 Killed: 9               while true; do\n>      if test $(time_in_seconds) -gt $timeout; then\n>          kill -9 $pid; exit 1;\n>      fi; sleep 1;\n> done\n> ok 8 - kill p4d\n>\n> # passed all 8 test(s)\n> 1..8\n> <<< Output end <<<\n>\n> However, I want them to look like this:\n>\n>>>> Output Start >>>\n> expecting success:\n> \tkill_p4d\n>\n> ok 8 - kill p4d\n>\n> # passed all 8 test(s)\n> 1..8\n> <<< Output end <<<\n>\n> This is achieved with the patch. I am no shell expert ... is there a nicer way to achieve the same?\n\nI get your desired output with the unmodified code from origin/next:\n\n$ ./t9823-git-p4-mock-lfs.sh -v\nexpecting success:\n\tkill_p4d\n\nok 8 - kill p4d\n\n# passed all 8 test(s)\n1..8\n\nBut that's because my shell is symlinked to /bin/dash. I suspect you are \nusing bash - when I run this with bash I get your command output. \nPossibly a bash bug?\n\nAs Junio says, it seems a bit weird that we have to redirect the stderr \nof that entire expression.\n\nLuke\n"}]}