t4153: stop redirecting input from /dev/zero

Commit Message

Jeff King July 17, 2024, 7 a.m. UTC

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 <[e-mail]>
Signed-off-by: Jeff King <[e-mail]>
---
 t/t4153-am-resume-override-opts.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Randall S. Becker July 17, 2024, 1:11 p.m. UTC | #1

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

Junio C Hamano July 17, 2024, 3:35 p.m. UTC | #2

Jeff King <[e-mail]> 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.

Patch

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
 '