[23/27] t3600: mark git command as failing
diff mbox series

Message ID bc4c208f7814c3362fa5949af3cb8d2d2904f386.1573779465.git.liu.denton@gmail.com
State New
Headers show
Series
  • t: general test cleanup + `set -o pipefail`
Related show

Commit Message

Denton Liu Nov. 15, 2019, 1:01 a.m. UTC
In a future patch, we plan on running tests with `set -o pipefail`.
Since we intentionally induce SIGPIPE, before the return code
was being masked away. However, now `git rm` will cause an error code to
be returned because of the SIGPIPE.

Mark the failing command with `test_must_fail ok=sigpipe` so that
failures induced by SIGPIPE don't propogate.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3600-rm.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Nov. 15, 2019, 5:35 a.m. UTC | #1
Denton Liu <liu.denton@gmail.com> writes:

> In a future patch, we plan on running tests with `set -o pipefail`.
> Since we intentionally induce SIGPIPE, before the return code
> was being masked away. However, now `git rm` will cause an error code to
> be returned because of the SIGPIPE.
>
> Mark the failing command with `test_must_fail ok=sigpipe` so that
> failures induced by SIGPIPE don't propogate.

Hmph, would this pipeline _always_ fail?  I somehow thought that a
process that writes into a pipe that is not being read would fail
only if the downstream dies and closes before it write(2)s, and if
the output from this "git rm -n" is small enough (say to fit within
the pipe buffer) and downstream no-op is slow enough to die, the
upstream may probably not notice and happily and successfully exit.

    : with slow downstream
    $ ( ( printf "%s\n" a b c d e); echo "my exit $?" 1>&2 ) |
      ( sleep 2; echo "downstream exit $?" ); echo "overall exit $?"
    my exit 0
    downstream exit 0
    overall exit 0

    : with slow upstream
    $ ( (sleep 2; printf "%s\n" a b c d e); echo "my exit $?" 1>&2 ) |
      ( echo "downstream exit $?" ); echo "overall exit $?"
    downstream exit 0
    my exit 141
    overall exit 0

So, no, I do not think this is a good idea.  It would be racy.


> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t3600-rm.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 0ea858d652..d1b3703edb 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -252,7 +252,7 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
>  		i=$(( $i + 1 ))
>  	done | git update-index --index-info &&
>  	# git command is intentionally placed upstream of pipe to induce SIGPIPE
> -	git rm -n "some-file-*" | : &&
> +	test_must_fail ok=sigpipe git rm -n "some-file-*" | : &&
>  	test_path_is_missing .git/index.lock
>  '

Patch
diff mbox series

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 0ea858d652..d1b3703edb 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -252,7 +252,7 @@  test_expect_success 'choking "git rm" should not let it die with cruft' '
 		i=$(( $i + 1 ))
 	done | git update-index --index-info &&
 	# git command is intentionally placed upstream of pipe to induce SIGPIPE
-	git rm -n "some-file-*" | : &&
+	test_must_fail ok=sigpipe git rm -n "some-file-*" | : &&
 	test_path_is_missing .git/index.lock
 '