[Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop

[Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop

[rsbecker]

Here is the breakage. This looks like a non-portable dependency on /dev/zero
was introduced.

expecting success of 4153.6 '--reject overrides --no-reject':
        rm -fr .git/rebase-apply &&
        git reset --hard &&
        git checkout first &&
        rm -f file.rej &&

        test_must_fail git am --no-reject side1.eml &&
        test_path_is_dir .git/rebase-apply &&
        test_path_is_missing file.rej &&

        test_must_fail git am --retry --reject </dev/zero &&
        test_path_is_dir .git/rebase-apply &&
        test_path_is_file file.rej

+ rm -fr .git/rebase-apply
+ git reset --hard
HEAD is now at 4f3810f side2
+ git checkout first
Warning: you are leaving 2 commits behind, not connected to
any of your branches:

  4f3810f side2
  deca85c side1

If you want to keep them by creating a new branch, this may be a good time
to do so with:

 git branch <new-branch-name> 4f3810f

HEAD is now at 3aaf771 first
+ rm -f file.rej
+ test_must_fail git am --no-reject side1.eml
Applying: side1
error: patch failed: file:1
error: file: patch does not apply
Patch failed at 0001 side1
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am
--abort".
hint: Disable this message with "git config advice.mergeConflict false"
+ test_path_is_dir .git/rebase-apply
+ test_path_is_missing file.rej
+ test_must_fail git am --retry --reject
+ test_eval_inner_[6]: /dev/zero: cannot open
error: last command exited with $?=1
not ok 6 - --reject overrides --no-reject
#
#               rm -fr .git/rebase-apply &&
#               git reset --hard &&
#               git checkout first &&
#               rm -f file.rej &&
#
#               test_must_fail git am --no-reject side1.eml &&
#               test_path_is_dir .git/rebase-apply &&
#               test_path_is_missing file.rej &&
#
#               test_must_fail git am --retry --reject </dev/zero &&
#               test_path_is_dir .git/rebase-apply &&
#               test_path_is_file file.rej
#
1..6

--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.

Re: [Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> Here is the breakage. This looks like a non-portable dependency on /dev/zero
> was introduced.
>
> expecting success of 4153.6 '--reject overrides --no-reject':
>         rm -fr .git/rebase-apply &&
>         git reset --hard &&
>         git checkout first &&
>         rm -f file.rej &&
>
>         test_must_fail git am --no-reject side1.eml &&
>         test_path_is_dir .git/rebase-apply &&
>         test_path_is_missing file.rej &&
>
>         test_must_fail git am --retry --reject </dev/zero &&
>         test_path_is_dir .git/rebase-apply &&
>         test_path_is_file file.rej

I think you are blaming 53ce2e3f (am: add explicit "--retry" option,
2024-06-06), but /dev/zero was in use before that commit.

        diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh
        ...
        @@ -40,7 +44,7 @@ test_expect_success TTY '--3way overrides --no-3way' '

                # Applying side1 with am --3way will succeed due to the threeway-merge.
                # Applying side2 will fail as --3way does not apply to it.
        -	test_must_fail test_terminal git am --3way </dev/zero &&
        +	test_must_fail git am --retry --3way &&
                test_path_is_dir .git/rebase-apply &&
                test side1 = "$(cat file2)"
         '

In this hunk, use of /dev/zero goes away; if /dev/zero were a
problem, you would have been failing this test already.

        @@ -94,7 +98,7 @@ test_expect_success TTY '--reject overrides --no-reject' '
                test_path_is_dir .git/rebase-apply &&
                test_path_is_missing file.rej &&

        -	test_must_fail test_terminal git am --reject </dev/zero &&
        +	test_must_fail git am --retry --reject </dev/zero &&
                test_path_is_dir .git/rebase-apply &&
                test_path_is_file file.rej
         '

And this hunk uses /dev/zero the same way before and after.  The
older use of /dev/zero in these tests were from 852a1710 (am: let
command-line options override saved options, 2015-08-04), which was
part of v2.6.0 that was tagged in late Sep 2015.

In short this is nothing new in this release.

Preparing a garbage file that is sufficiently large (like 1kB) and
feeding from that file to the commands instead of from /dev/null may
be sufficient to reduce the dependence of /dev/zero but given that
this is ancient, there are many other uses of /dev/zero in the test
directory, it does not seem ultra-urgent to address this, at least
to me.  Certainly not during the pre-release freeze.

Thanks for a report.

Re: [Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop

[Jeff King]

On Sat, Jul 13, 2024 at 07:23:25PM -0700, Junio C Hamano wrote:

> And this hunk uses /dev/zero the same way before and after.  The
> older use of /dev/zero in these tests were from 852a1710 (am: let
> command-line options override saved options, 2015-08-04), which was
> part of v2.6.0 that was tagged in late Sep 2015.
> 
> In short this is nothing new in this release.

One thing my recent patch did was remove the TTY prereq from those
tests. So it's possible that they're now being run on his platform for
the first time.

> Preparing a garbage file that is sufficiently large (like 1kB) and
> feeding from that file to the commands instead of from /dev/null may
> be sufficient to reduce the dependence of /dev/zero but given that
> this is ancient, there are many other uses of /dev/zero in the test
> directory, it does not seem ultra-urgent to address this, at least
> to me.  Certainly not during the pre-release freeze.

The other mentions of /dev/zero that I see are all in tests marked as
EXPENSIVE, which likewise aren't run normally.

So I think it is plausibly a new issue for some platforms, though I
agree it is not super urgent.

With --retry I think we would not actually read stdin at all, so we
could just remove the mention of /dev/zero entirely. But if we wanted to
be sure it did not read and choke on any input provided, I think just:

diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh
index a4d0c03ca6..76783bdd67 100755
--- a/t/t4153-am-resume-override-opts.sh
+++ b/t/t4153-am-resume-override-opts.sh
@@ -45,6 +45,7 @@ test_expect_success '--3way overrides --no-3way' '
 
 	# Applying side1 with am --3way will succeed due to the threeway-merge.
 	# Applying side2 will fail as --3way does not apply to it.
+	echo garbage |
 	test_must_fail git am --retry --3way &&
 	test_path_is_dir .git/rebase-apply &&
 	test side1 = "$(cat file2)"
@@ -99,7 +100,8 @@ test_expect_success '--reject overrides --no-reject' '
 	test_path_is_dir .git/rebase-apply &&
 	test_path_is_missing file.rej &&
 
-	test_must_fail git am --retry --reject </dev/zero &&
+	echo garbage |
+	test_must_fail git am --retry --reject &&
 	test_path_is_dir .git/rebase-apply &&
 	test_path_is_file file.rej
 '

would be sufficient.

-Peff

RE: [Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop

[rsbecker]

On Saturday, July 13, 2024 10:23 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> Here is the breakage. This looks like a non-portable dependency on
>> /dev/zero was introduced.
>>
>> expecting success of 4153.6 '--reject overrides --no-reject':
>>         rm -fr .git/rebase-apply &&
>>         git reset --hard &&
>>         git checkout first &&
>>         rm -f file.rej &&
>>
>>         test_must_fail git am --no-reject side1.eml &&
>>         test_path_is_dir .git/rebase-apply &&
>>         test_path_is_missing file.rej &&
>>
>>         test_must_fail git am --retry --reject </dev/zero &&
>>         test_path_is_dir .git/rebase-apply &&
>>         test_path_is_file file.rej
>
>I think you are blaming 53ce2e3f (am: add explicit "--retry" option,
2024-06-06),
>but /dev/zero was in use before that commit.
>
>        diff --git a/t/t4153-am-resume-override-opts.sh
b/t/t4153-am-resume-
>override-opts.sh
>        ...
>        @@ -40,7 +44,7 @@ test_expect_success TTY '--3way overrides
--no-3way' '
>
>                # Applying side1 with am --3way will succeed due to the
threeway-merge.
>                # Applying side2 will fail as --3way does not apply to it.
>        -	test_must_fail test_terminal git am --3way </dev/zero &&
>        +	test_must_fail git am --retry --3way &&
>                test_path_is_dir .git/rebase-apply &&
>                test side1 = "$(cat file2)"
>         '
>
>In this hunk, use of /dev/zero goes away; if /dev/zero were a problem, you
would
>have been failing this test already.
>
>        @@ -94,7 +98,7 @@ test_expect_success TTY '--reject overrides
--no-reject' '
>                test_path_is_dir .git/rebase-apply &&
>                test_path_is_missing file.rej &&
>
>        -	test_must_fail test_terminal git am --reject </dev/zero &&
>        +	test_must_fail git am --retry --reject </dev/zero &&
>                test_path_is_dir .git/rebase-apply &&
>                test_path_is_file file.rej
>         '
>
>And this hunk uses /dev/zero the same way before and after.  The older use
of
>/dev/zero in these tests were from 852a1710 (am: let command-line options
>override saved options, 2015-08-04), which was part of v2.6.0 that was
tagged in
>late Sep 2015.
>
>In short this is nothing new in this release.
>
>Preparing a garbage file that is sufficiently large (like 1kB) and feeding
from that file
>to the commands instead of from /dev/null may be sufficient to reduce the
>dependence of /dev/zero but given that this is ancient, there are many
other uses
>of /dev/zero in the test directory, it does not seem ultra-urgent to
address this, at
>least to me.  Certainly not during the pre-release freeze.
>
>Thanks for a report.

I think the /dev/zero usage was being hidden. We definitely do not have
/dev/zero, but I think the prior hung failed anyway, so the test diff not
differentiate between the test fail and using /dev/zero. So it looks like
--retry. This worked at 2.44.2 and for years before that.

RE: [Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop

[rsbecker]

On Sunday, July 14, 2024 3:06 Am, Peff wrote:
>To: Junio C Hamano <gitster@pobox.com>
>Cc: rsbecker@nexbridge.com; git@vger.kernel.org
>Subject: Re: [Test Breakage 2.46.0-rc0] Test t4135.06 fails on NonStop
>
>On Sat, Jul 13, 2024 at 07:23:25PM -0700, Junio C Hamano wrote:
>
>> And this hunk uses /dev/zero the same way before and after.  The older
>> use of /dev/zero in these tests were from 852a1710 (am: let
>> command-line options override saved options, 2015-08-04), which was
>> part of v2.6.0 that was tagged in late Sep 2015.
>>
>> In short this is nothing new in this release.
>
>One thing my recent patch did was remove the TTY prereq from those tests. So it's
>possible that they're now being run on his platform for the first time.

Good catch. That was it. From the prior CI build:
*** t4153-am-resume-override-opts.sh ***
ok 1 - setup
ok 2 # skip --3way overrides --no-3way (missing TTY)
ok 3 - --no-quiet overrides --quiet
ok 4 - --signoff overrides --no-signoff
ok 5 # skip --reject overrides --no-reject (missing TTY)

>> Preparing a garbage file that is sufficiently large (like 1kB) and
>> feeding from that file to the commands instead of from /dev/null may
>> be sufficient to reduce the dependence of /dev/zero but given that
>> this is ancient, there are many other uses of /dev/zero in the test
>> directory, it does not seem ultra-urgent to address this, at least to
>> me.  Certainly not during the pre-release freeze.
>
>The other mentions of /dev/zero that I see are all in tests marked as EXPENSIVE,
>which likewise aren't run normally.
>
>So I think it is plausibly a new issue for some platforms, though I agree it is not super
>urgent.
>
>With --retry I think we would not actually read stdin at all, so we could just remove
>the mention of /dev/zero entirely. But if we wanted to be sure it did not read and
>choke on any input provided, I think just:
>
>diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-
>opts.sh
>index a4d0c03ca6..76783bdd67 100755
>--- a/t/t4153-am-resume-override-opts.sh
>+++ b/t/t4153-am-resume-override-opts.sh
>@@ -45,6 +45,7 @@ test_expect_success '--3way overrides --no-3way' '
>
> 	# Applying side1 with am --3way will succeed due to the threeway-merge.
> 	# Applying side2 will fail as --3way does not apply to it.
>+	echo garbage |
> 	test_must_fail git am --retry --3way &&
> 	test_path_is_dir .git/rebase-apply &&
> 	test side1 = "$(cat file2)"
>@@ -99,7 +100,8 @@ test_expect_success '--reject overrides --no-reject' '
> 	test_path_is_dir .git/rebase-apply &&
> 	test_path_is_missing file.rej &&
>
>-	test_must_fail git am --retry --reject </dev/zero &&
>+	echo garbage |
>+	test_must_fail git am --retry --reject &&
> 	test_path_is_dir .git/rebase-apply &&
> 	test_path_is_file file.rej
> '
>
>would be sufficient.

And it does work properly on the box. Thanks.
ok 1 - setup
ok 2 - --retry fails without in-progress operation
ok 3 - --3way overrides --no-3way
ok 4 - --no-quiet overrides --quiet
ok 5 - --signoff overrides --no-signoff
ok 6 - --reject overrides --no-reject
# passed all 6 test(s)
1..6

Thanks,
Randall

[PATCH] t4153: stop redirecting input from /dev/zero

[Jeff King]

On Sun, Jul 14, 2024 at 03:05:58AM -0400, Jeff King wrote:

> With --retry I think we would not actually read stdin at all, so we
> could just remove the mention of /dev/zero entirely. But if we wanted to
> be sure it did not read and choke on any input provided, I think just:
> 
> diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh
> index a4d0c03ca6..76783bdd67 100755
> --- a/t/t4153-am-resume-override-opts.sh
> +++ b/t/t4153-am-resume-override-opts.sh
> @@ -45,6 +45,7 @@ test_expect_success '--3way overrides --no-3way' '
>  
>  	# Applying side1 with am --3way will succeed due to the threeway-merge.
>  	# Applying side2 will fail as --3way does not apply to it.
> +	echo garbage |
>  	test_must_fail git am --retry --3way &&
>  	test_path_is_dir .git/rebase-apply &&
>  	test side1 = "$(cat file2)"
> @@ -99,7 +100,8 @@ test_expect_success '--reject overrides --no-reject' '
>  	test_path_is_dir .git/rebase-apply &&
>  	test_path_is_missing file.rej &&
>  
> -	test_must_fail git am --retry --reject </dev/zero &&
> +	echo garbage |
> +	test_must_fail git am --retry --reject &&
>  	test_path_is_dir .git/rebase-apply &&
>  	test_path_is_file file.rej
>  '

Looking at the history, the use of /dev/zero was not here to simulate
garbage on stdin. It really was just meant to be a descriptor which
never ran out of input, as a workaround for test_terminal. This is
described in the commit mentioned below.

So I think we should just do this, which can go on top of jk/am-retry. I
still do not think it is super-urgent, but given the simplicity of the
fix is probably worth doing for the upcoming v2.46, which exposes these
tests to more platforms.

-- >8 --
Subject: [PATCH] t4153: stop redirecting input from /dev/zero

Commit 852a171018 (am: let command-line options override saved options,
2015-08-04) redirected a few "git am" invocations from /dev/zero, even
though it did not expect "am" to read the input. This was necessary at
the time because those tests used test_terminal, and as described in
18d8c26930 (test_terminal: redirect child process' stdin to a pty,
2015-08-04):

  Note that due to the way the code is structured, the child's stdin
  pseudo-tty will be closed when we finish reading from our stdin. This
  means that in the common case, where our stdin is attached to /dev/null,
  the child's stdin pseudo-tty will be closed immediately. Some operations
  like isatty(), which git-am uses, require the file descriptor to be
  open, and hence if the success of the command depends on such functions,
  test_terminal's stdin should be redirected to a source with large amount
  of data to ensure that the child's stdin is not closed, e.g.

              test_terminal git am --3way </dev/zero

But we later dropped the use of test_terminal in 53ce2e3f0a (am: add
explicit "--retry" option, 2024-06-06). That commit dropped one of the
redirections from /dev/zero but not the other.

In theory the remaining one should not cause any problems, but it turns
out that at least one platform (NonStop) does not have /dev/zero at all.
We never noticed before because it also did not pass the TTY prereq,
meaning these tests were not run at all there until 53ce2e3f0a.

So let's drop the useless /dev/zero mention. There are others in the
test suite, but they are run only for tests marked with EXPENSIVE (so
not typically by default).

Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4153-am-resume-override-opts.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh
index a4d0c03ca6..dd6ad8f7a8 100755
--- a/t/t4153-am-resume-override-opts.sh
+++ b/t/t4153-am-resume-override-opts.sh
@@ -99,7 +99,7 @@ test_expect_success '--reject overrides --no-reject' '
 	test_path_is_dir .git/rebase-apply &&
 	test_path_is_missing file.rej &&
 
-	test_must_fail git am --retry --reject </dev/zero &&
+	test_must_fail git am --retry --reject &&
 	test_path_is_dir .git/rebase-apply &&
 	test_path_is_file file.rej
 '
-- 
2.46.0.rc0.511.gabae7d6275

RE: [PATCH] t4153: stop redirecting input from /dev/zero

[rsbecker]

On Wednesday, July 17, 2024 3:01 AM, Jeff King wrote:
>On Sun, Jul 14, 2024 at 03:05:58AM -0400, Jeff King wrote:
>
>> With --retry I think we would not actually read stdin at all, so we
>> could just remove the mention of /dev/zero entirely. But if we wanted
>> to be sure it did not read and choke on any input provided, I think just:
>>
>> diff --git a/t/t4153-am-resume-override-opts.sh
>> b/t/t4153-am-resume-override-opts.sh
>> index a4d0c03ca6..76783bdd67 100755
>> --- a/t/t4153-am-resume-override-opts.sh
>> +++ b/t/t4153-am-resume-override-opts.sh
>> @@ -45,6 +45,7 @@ test_expect_success '--3way overrides --no-3way' '
>>
>>  	# Applying side1 with am --3way will succeed due to the threeway-merge.
>>  	# Applying side2 will fail as --3way does not apply to it.
>> +	echo garbage |
>>  	test_must_fail git am --retry --3way &&
>>  	test_path_is_dir .git/rebase-apply &&
>>  	test side1 = "$(cat file2)"
>> @@ -99,7 +100,8 @@ test_expect_success '--reject overrides --no-reject' '
>>  	test_path_is_dir .git/rebase-apply &&
>>  	test_path_is_missing file.rej &&
>>
>> -	test_must_fail git am --retry --reject </dev/zero &&
>> +	echo garbage |
>> +	test_must_fail git am --retry --reject &&
>>  	test_path_is_dir .git/rebase-apply &&
>>  	test_path_is_file file.rej
>>  '
>
>Looking at the history, the use of /dev/zero was not here to simulate garbage on
>stdin. It really was just meant to be a descriptor which never ran out of input, as a
>workaround for test_terminal. This is described in the commit mentioned below.
>
>So I think we should just do this, which can go on top of jk/am-retry. I still do not
>think it is super-urgent, but given the simplicity of the fix is probably worth doing
>for the upcoming v2.46, which exposes these tests to more platforms.

Might not be urgent but without this we cannot pass this test on NonStop and I'm assuming other non-linux systems.
Thanks,
--Randall

Re: [PATCH] t4153: stop redirecting input from /dev/zero

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But we later dropped the use of test_terminal in 53ce2e3f0a (am: add
> explicit "--retry" option, 2024-06-06). That commit dropped one of the
> redirections from /dev/zero but not the other.

Ah, it makes sense to drop that other one with this patch, then.

Will queue.  Thanks.