{"thread":{"id":"61777","subject":"[Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop","startedAt":"2024-07-13T16:41:38Z","lastAt":"2024-07-17T15:36:06Z","messageCount":8,"participants":["rsbecker@nexbridge.com","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"498636","messageId":"024201dad543$877221e0$965665a0$@nexbridge.com","threadId":"61777","inReplyTo":null,"subject":"[Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-07-13T16:41:31Z","receivedAt":"2024-07-13T16:41:38Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"Here is the breakage. This looks like a non-portable dependency on /dev/zero\nwas introduced.\n\nexpecting success of 4153.6 '--reject overrides --no-reject':\n rm -fr .git/rebase-apply &&\n git reset --hard &&\n git checkout first &&\n rm -f file.rej &&\n\n test_must_fail git am --no-reject side1.eml &&\n test_path_is_dir .git/rebase-apply &&\n test_path_is_missing file.rej &&\n\n test_must_fail git am --retry --reject 4f3810f\n\nHEAD is now at 3aaf771 first\n+ rm -f file.rej\n+ test_must_fail git am --no-reject side1.eml\nApplying: side1\nerror: patch failed: file:1\nerror: file: patch does not apply\nPatch failed at 0001 side1\nhint: Use 'git am --show-current-patch=diff' to see the failed patch\nhint: When you have resolved this problem, run \"git am --continue\".\nhint: If you prefer to skip this patch, run \"git am --skip\" instead.\nhint: To restore the original branch and stop patching, run \"git am\n--abort\".\nhint: Disable this message with \"git config advice.mergeConflict false\"\n+ test_path_is_dir .git/rebase-apply\n+ test_path_is_missing file.rej\n+ test_must_fail git am --retry --reject\n+ test_eval_inner_[6]: /dev/zero: cannot open\nerror: last command exited with $?=1\nnot ok 6 - --reject overrides --no-reject\n#\n# rm -fr .git/rebase-apply &&\n# git reset --hard &&\n# git checkout first &&\n# rm -f file.rej &&\n#\n# test_must_fail git am --no-reject side1.eml &&\n# test_path_is_dir .git/rebase-apply &&\n# test_path_is_missing file.rej &&\n#\n# test_must_fail git am --retry --reject writes:\n\n> Here is the breakage. This looks like a non-portable dependency on /dev/zero\n> was introduced.\n>\n> expecting success of 4153.6 '--reject overrides --no-reject':\n> rm -fr .git/rebase-apply &&\n> git reset --hard &&\n> git checkout first &&\n> rm -f file.rej &&\n>\n> test_must_fail git am --no-reject side1.eml &&\n> test_path_is_dir .git/rebase-apply &&\n> test_path_is_missing file.rej &&\n>\n> test_must_fail git am --retry --reject test_path_is_dir .git/rebase-apply &&\n> test_path_is_file file.rej\n\nI think you are blaming 53ce2e3f (am: add explicit \"--retry\" option,\n2024-06-06), but /dev/zero was in use before that commit.\n\n diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh\n ...\n @@ -40,7 +44,7 @@ test_expect_success TTY '--3way overrides --no-3way' '\n\n # Applying side1 with am --3way will succeed due to the threeway-merge.\n # Applying side2 will fail as --3way does not apply to it.\n -\ttest_must_fail test_terminal git am --3way And this hunk uses /dev/zero the same way before and after. The\n> older use of /dev/zero in these tests were from 852a1710 (am: let\n> command-line options override saved options, 2015-08-04), which was\n> part of v2.6.0 that was tagged in late Sep 2015.\n> \n> In short this is nothing new in this release.\n\nOne thing my recent patch did was remove the TTY prereq from those\ntests. So it's possible that they're now being run on his platform for\nthe first time.\n\n> Preparing a garbage file that is sufficiently large (like 1kB) and\n> feeding from that file to the commands instead of from /dev/null may\n> be sufficient to reduce the dependence of /dev/zero but given that\n> this is ancient, there are many other uses of /dev/zero in the test\n> directory, it does not seem ultra-urgent to address this, at least\n> to me. Certainly not during the pre-release freeze.\n\nThe other mentions of /dev/zero that I see are all in tests marked as\nEXPENSIVE, which likewise aren't run normally.\n\nSo I think it is plausibly a new issue for some platforms, though I\nagree it is not super urgent.\n\nWith --retry I think we would not actually read stdin at all, so we\ncould just remove the mention of /dev/zero entirely. But if we wanted to\nbe sure it did not read and choke on any input provided, I think just:\n\ndiff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh\nindex a4d0c03ca6..76783bdd67 100755\n--- a/t/t4153-am-resume-override-opts.sh\n+++ b/t/t4153-am-resume-override-opts.sh\n@@ -45,6 +45,7 @@ test_expect_success '--3way overrides --no-3way' '\n \n \t# Applying side1 with am --3way will succeed due to the threeway-merge.\n \t# Applying side2 will fail as --3way does not apply to it.\n+\techo garbage |\n \ttest_must_fail git am --retry --3way &&\n \ttest_path_is_dir .git/rebase-apply &&\n \ttest side1 = \"$(cat file2)\"\n@@ -99,7 +100,8 @@ test_expect_success '--reject overrides --no-reject' '\n \ttest_path_is_dir .git/rebase-apply &&\n \ttest_path_is_missing file.rej &&\n \n-\ttest_must_fail git am --retry --reject writes:\n>\n>> Here is the breakage. This looks like a non-portable dependency on\n>> /dev/zero was introduced.\n>>\n>> expecting success of 4153.6 '--reject overrides --no-reject':\n>> rm -fr .git/rebase-apply &&\n>> git reset --hard &&\n>> git checkout first &&\n>> rm -f file.rej &&\n>>\n>> test_must_fail git am --no-reject side1.eml &&\n>> test_path_is_dir .git/rebase-apply &&\n>> test_path_is_missing file.rej &&\n>>\n>> test_must_fail git am --retry --reject > test_path_is_dir .git/rebase-apply &&\n>> test_path_is_file file.rej\n>\n>I think you are blaming 53ce2e3f (am: add explicit \"--retry\" option,\n2024-06-06),\n>but /dev/zero was in use before that commit.\n>\n> diff --git a/t/t4153-am-resume-override-opts.sh\nb/t/t4153-am-resume-\n>override-opts.sh\n> ...\n> @@ -40,7 +44,7 @@ test_expect_success TTY '--3way overrides\n--no-3way' '\n>\n> # Applying side1 with am --3way will succeed due to the\nthreeway-merge.\n> # Applying side2 will fail as --3way does not apply to it.\n> -\ttest_must_fail test_terminal git am --3way +\ttest_must_fail git am --retry --3way &&\n> test_path_is_dir .git/rebase-apply &&\n> test side1 = \"$(cat file2)\"\n> '\n>\n>In this hunk, use of /dev/zero goes away; if /dev/zero were a problem, you\nwould\n>have been failing this test already.\n>\n> @@ -94,7 +98,7 @@ test_expect_success TTY '--reject overrides\n--no-reject' '\n> test_path_is_dir .git/rebase-apply &&\n> test_path_is_missing file.rej &&\n>\n> -\ttest_must_fail test_terminal git am --reject +\ttest_must_fail git am --retry --reject test_path_is_dir .git/rebase-apply &&\n> test_path_is_file file.rej\n> '\n>\n>And this hunk uses /dev/zero the same way before and after. The older use\nof\n>/dev/zero in these tests were from 852a1710 (am: let command-line options\n>override saved options, 2015-08-04), which was part of v2.6.0 that was\ntagged in\n>late Sep 2015.\n>\n>In short this is nothing new in this release.\n>\n>Preparing a garbage file that is sufficiently large (like 1kB) and feeding\nfrom that file\n>to the commands instead of from /dev/null may be sufficient to reduce the\n>dependence of /dev/zero but given that this is ancient, there are many\nother uses\n>of /dev/zero in the test directory, it does not seem ultra-urgent to\naddress this, at\n>least to me. Certainly not during the pre-release freeze.\n>\n>Thanks for a report.\n\nI think the /dev/zero usage was being hidden. We definitely do not have\n/dev/zero, but I think the prior hung failed anyway, so the test diff not\ndifferentiate between the test fail and using /dev/zero. So it looks like\n--retry. This worked at 2.44.2 and for years before that.\n\n"},{"id":"498676","messageId":"002001dad5f2$df7b69a0$9e723ce0$@nexbridge.com","threadId":"61777","inReplyTo":"20240714070558.GA575927@coredump.intra.peff.net","subject":"RE: [Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-07-14T13:36:41Z","receivedAt":"2024-07-14T13:36:50Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Sunday, July 14, 2024 3:06 Am, Peff wrote:\n>To: Junio C Hamano \n>Cc: rsbecker@nexbridge.com; git@vger.kernel.org\n>Subject: Re: [Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop\n>\n>On Sat, Jul 13, 2024 at 07:23:25PM -0700, Junio C Hamano wrote:\n>\n>> And this hunk uses /dev/zero the same way before and after. The older\n>> use of /dev/zero in these tests were from 852a1710 (am: let\n>> command-line options override saved options, 2015-08-04), which was\n>> part of v2.6.0 that was tagged in late Sep 2015.\n>>\n>> In short this is nothing new in this release.\n>\n>One thing my recent patch did was remove the TTY prereq from those tests. So it's\n>possible that they're now being run on his platform for the first time.\n\nGood catch. That was it. From the prior CI build:\n*** t4153-am-resume-override-opts.sh ***\nok 1 - setup\nok 2 # skip --3way overrides --no-3way (missing TTY)\nok 3 - --no-quiet overrides --quiet\nok 4 - --signoff overrides --no-signoff\nok 5 # skip --reject overrides --no-reject (missing TTY)\n\n>> Preparing a garbage file that is sufficiently large (like 1kB) and\n>> feeding from that file to the commands instead of from /dev/null may\n>> be sufficient to reduce the dependence of /dev/zero but given that\n>> this is ancient, there are many other uses of /dev/zero in the test\n>> directory, it does not seem ultra-urgent to address this, at least to\n>> me. Certainly not during the pre-release freeze.\n>\n>The other mentions of /dev/zero that I see are all in tests marked as EXPENSIVE,\n>which likewise aren't run normally.\n>\n>So I think it is plausibly a new issue for some platforms, though I agree it is not super\n>urgent.\n>\n>With --retry I think we would not actually read stdin at all, so we could just remove\n>the mention of /dev/zero entirely. But if we wanted to be sure it did not read and\n>choke on any input provided, I think just:\n>\n>diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-\n>opts.sh\n>index a4d0c03ca6..76783bdd67 100755\n>--- a/t/t4153-am-resume-override-opts.sh\n>+++ b/t/t4153-am-resume-override-opts.sh\n>@@ -45,6 +45,7 @@ test_expect_success '--3way overrides --no-3way' '\n>\n> \t# Applying side1 with am --3way will succeed due to the threeway-merge.\n> \t# Applying side2 will fail as --3way does not apply to it.\n>+\techo garbage |\n> \ttest_must_fail git am --retry --3way &&\n> \ttest_path_is_dir .git/rebase-apply &&\n> \ttest side1 = \"$(cat file2)\"\n>@@ -99,7 +100,8 @@ test_expect_success '--reject overrides --no-reject' '\n> \ttest_path_is_dir .git/rebase-apply &&\n> \ttest_path_is_missing file.rej &&\n>\n>-\ttest_must_fail git am --retry --reject +\techo garbage |\n>+\ttest_must_fail git am --retry --reject &&\n> \ttest_path_is_dir .git/rebase-apply &&\n> \ttest_path_is_file file.rej\n> '\n>\n>would be sufficient.\n\nAnd it does work properly on the box. Thanks.\nok 1 - setup\nok 2 - --retry fails without in-progress operation\nok 3 - --3way overrides --no-3way\nok 4 - --no-quiet overrides --quiet\nok 5 - --signoff overrides --no-signoff\nok 6 - --reject overrides --no-reject\n# passed all 6 test(s)\n1..6\n\nThanks,\nRandall\n\n"},{"id":"498817","messageId":"20240717070050.GG547635@coredump.intra.peff.net","threadId":"61777","inReplyTo":"20240714070558.GA575927@coredump.intra.peff.net","subject":"[PATCH] t4153: stop redirecting input from /dev/zero","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-17T07:00:50Z","receivedAt":"2024-07-17T07:00:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 14, 2024 at 03:05:58AM -0400, Jeff King wrote:\n\n> With --retry I think we would not actually read stdin at all, so we\n> could just remove the mention of /dev/zero entirely. But if we wanted to\n> be sure it did not read and choke on any input provided, I think just:\n> \n> diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh\n> index a4d0c03ca6..76783bdd67 100755\n> --- a/t/t4153-am-resume-override-opts.sh\n> +++ b/t/t4153-am-resume-override-opts.sh\n> @@ -45,6 +45,7 @@ test_expect_success '--3way overrides --no-3way' '\n> \n> \t# Applying side1 with am --3way will succeed due to the threeway-merge.\n> \t# Applying side2 will fail as --3way does not apply to it.\n> +\techo garbage |\n> \ttest_must_fail git am --retry --3way &&\n> \ttest_path_is_dir .git/rebase-apply &&\n> \ttest side1 = \"$(cat file2)\"\n> @@ -99,7 +100,8 @@ test_expect_success '--reject overrides --no-reject' '\n> \ttest_path_is_dir .git/rebase-apply &&\n> \ttest_path_is_missing file.rej &&\n> \n> -\ttest_must_fail git am --retry --reject +\techo garbage |\n> +\ttest_must_fail git am --retry --reject &&\n> \ttest_path_is_dir .git/rebase-apply &&\n> \ttest_path_is_file file.rej\n> '\n\nLooking at the history, the use of /dev/zero was not here to simulate\ngarbage on stdin. It really was just meant to be a descriptor which\nnever ran out of input, as a workaround for test_terminal. This is\ndescribed in the commit mentioned below.\n\nSo I think we should just do this, which can go on top of jk/am-retry. I\nstill do not think it is super-urgent, but given the simplicity of the\nfix is probably worth doing for the upcoming v2.46, which exposes these\ntests to more platforms.\n\n-- >8 --\nSubject: [PATCH] t4153: stop redirecting input from /dev/zero\n\nCommit 852a171018 (am: let command-line options override saved options,\n2015-08-04) redirected a few \"git am\" invocations from /dev/zero, even\nthough it did not expect \"am\" to read the input. This was necessary at\nthe time because those tests used test_terminal, and as described in\n18d8c26930 (test_terminal: redirect child process' stdin to a pty,\n2015-08-04):\n\n Note that due to the way the code is structured, the child's stdin\n pseudo-tty will be closed when we finish reading from our stdin. This\n means that in the common case, where our stdin is attached to /dev/null,\n the child's stdin pseudo-tty will be closed immediately. Some operations\n like isatty(), which git-am uses, require the file descriptor to be\n open, and hence if the success of the command depends on such functions,\n test_terminal's stdin should be redirected to a source with large amount\n of data to ensure that the child's stdin is not closed, e.g.\n\n test_terminal git am --3way \nSigned-off-by: Jeff King \n---\n t/t4153-am-resume-override-opts.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh\nindex a4d0c03ca6..dd6ad8f7a8 100755\n--- a/t/t4153-am-resume-override-opts.sh\n+++ b/t/t4153-am-resume-override-opts.sh\n@@ -99,7 +99,7 @@ test_expect_success '--reject overrides --no-reject' '\n \ttest_path_is_dir .git/rebase-apply &&\n \ttest_path_is_missing file.rej &&\n \n-\ttest_must_fail git am --retry --reject On Sun, Jul 14, 2024 at 03:05:58AM -0400, Jeff King wrote:\n>\n>> With --retry I think we would not actually read stdin at all, so we\n>> could just remove the mention of /dev/zero entirely. But if we wanted\n>> to be sure it did not read and choke on any input provided, I think just:\n>>\n>> diff --git a/t/t4153-am-resume-override-opts.sh\n>> b/t/t4153-am-resume-override-opts.sh\n>> index a4d0c03ca6..76783bdd67 100755\n>> --- a/t/t4153-am-resume-override-opts.sh\n>> +++ b/t/t4153-am-resume-override-opts.sh\n>> @@ -45,6 +45,7 @@ test_expect_success '--3way overrides --no-3way' '\n>>\n>> \t# Applying side1 with am --3way will succeed due to the threeway-merge.\n>> \t# Applying side2 will fail as --3way does not apply to it.\n>> +\techo garbage |\n>> \ttest_must_fail git am --retry --3way &&\n>> \ttest_path_is_dir .git/rebase-apply &&\n>> \ttest side1 = \"$(cat file2)\"\n>> @@ -99,7 +100,8 @@ test_expect_success '--reject overrides --no-reject' '\n>> \ttest_path_is_dir .git/rebase-apply &&\n>> \ttest_path_is_missing file.rej &&\n>>\n>> -\ttest_must_fail git am --retry --reject > +\techo garbage |\n>> +\ttest_must_fail git am --retry --reject &&\n>> \ttest_path_is_dir .git/rebase-apply &&\n>> \ttest_path_is_file file.rej\n>> '\n>\n>Looking at the history, the use of /dev/zero was not here to simulate garbage on\n>stdin. It really was just meant to be a descriptor which never ran out of input, as a\n>workaround for test_terminal. This is described in the commit mentioned below.\n>\n>So I think we should just do this, which can go on top of jk/am-retry. I still do not\n>think it is super-urgent, but given the simplicity of the fix is probably worth doing\n>for the upcoming v2.46, which exposes these tests to more platforms.\n\nMight not be urgent but without this we cannot pass this test on NonStop and I'm assuming other non-linux systems.\nThanks,\n--Randall\n\n"},{"id":"498834","messageId":"xmqqjzhk58v6.fsf@gitster.g","threadId":"61777","inReplyTo":"20240717070050.GG547635@coredump.intra.peff.net","subject":"Re: [PATCH] t4153: stop redirecting input from /dev/zero","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-17T15:35:57Z","receivedAt":"2024-07-17T15:36:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But we later dropped the use of test_terminal in 53ce2e3f0a (am: add\n> explicit \"--retry\" option, 2024-06-06). That commit dropped one of the\n> redirections from /dev/zero but not the other.\n\nAh, it makes sense to drop that other one with this patch, then.\n\nWill queue. Thanks.\n"}]}