>From 056d6de78fcf11a0f404f25faf8cecc9f9ee9c1c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <address@hidden>
Date: Wed, 5 Mar 2014 18:41:16 +0000
Subject: [PATCH] tests: avoid the :> construct which can hide errors

On most shells `:>file || framework_failure_` will not evaluate
the framework_failure_ even if there was an error writing the file.
shells which do evaluate the failure are ksh 93u+ and bash 4.2,
while shells wich don't include bash 4.3, solaris, freebsd, dash.

Furthermore this construct is problematic on Solaris 10 sh,
which will try to optimize away a `:' command in a loop
after the first iteration, even if it is redirected.

* tests/cp/link-deref.sh: Remove the leading colon on redirections.
* tests/cp/reflink-perm.sh: Likewise.
* tests/id/zero.sh: Likewise.
* tests/install/install-C.sh: Likewise.
* tests/misc/env.sh: Likewise.
* tests/misc/md5sum-bsd.sh: Likewise.
* tests/misc/runcon-no-reorder.sh: Likewise.
* tests/mv/partition-perm.sh: Likewise.
* tests/rm/r-root.sh: Likewise.
* tests/split/l-chunk.sh: Likewise.
* tests/split/line-bytes.sh: Likewise.
* tests/tail-2/inotify-rotate.sh: Likewise.
* tests/tail-2/retry.sh: Likewise.
* tests/tail-2/symlink.sh: Likewise.
* tests/tail-2/wait.sh: Likewise.
* tests/touch/read-only.sh: Likewise.
+ cfg.mk (sc_prohibit_colon_redirection): A new syntax check
to avoid further instances of this creeping in.
---
 cfg.mk                          |    7 +++++++
 tests/cp/link-deref.sh          |    2 +-
 tests/cp/reflink-perm.sh        |    4 ++--
 tests/id/zero.sh                |    2 +-
 tests/install/install-C.sh      |    2 +-
 tests/misc/env.sh               |    2 +-
 tests/misc/md5sum-bsd.sh        |    2 +-
 tests/misc/runcon-no-reorder.sh |    2 +-
 tests/mv/partition-perm.sh      |    6 +++---
 tests/rm/r-root.sh              |    6 +++---
 tests/split/l-chunk.sh          |    8 ++++----
 tests/split/line-bytes.sh       |    4 ++--
 tests/tail-2/inotify-rotate.sh  |    2 +-
 tests/tail-2/retry.sh           |    2 +-
 tests/tail-2/symlink.sh         |    4 ++--
 tests/tail-2/wait.sh            |    2 +-
 tests/touch/read-only.sh        |    2 +-
 17 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 7f74afa..cc6f4b8 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -123,6 +123,13 @@ sc_tests_executable:
 	  | sed -e "s/^/$(ME): Please make test executable: /" | grep . \
 	    && exit 1; :
 
+# Avoid :>file which doesn't propagate errors
+sc_prohibit_colon_redirection:
+	@cd $(srcdir)/tests && GIT_PAGER= git grep -n ': *>.*||' \
+	  && { echo '$(ME): '"The leading colon in :> will hide errors" 1>&2; \
+	       exit 1; }  \
+	  || :
+
 # Create a list of regular expressions matching the names
 # of files included from system.h.  Exclude a couple.
 .re-list:
diff --git a/tests/cp/link-deref.sh b/tests/cp/link-deref.sh
index 73c86a8..1b66658 100755
--- a/tests/cp/link-deref.sh
+++ b/tests/cp/link-deref.sh
@@ -31,7 +31,7 @@ if grep '^#define HAVE_LINKAT 1' "$CONFIG_HEADER" > /dev/null \
 fi
 
 mkdir dir              || framework_failure_
-: > file               || framework_failure_
+> file                 || framework_failure_
 ln -s dir     dirlink  || framework_failure_
 ln -s file    filelink || framework_failure_
 ln -s nowhere danglink || framework_failure_
diff --git a/tests/cp/reflink-perm.sh b/tests/cp/reflink-perm.sh
index 0650065..52f83fc 100755
--- a/tests/cp/reflink-perm.sh
+++ b/tests/cp/reflink-perm.sh
@@ -20,8 +20,8 @@
 print_ver_ cp
 
 
-: > time_check
-: > file
+> time_check
+> file
 ts='2009-08-28 19:00'
 touch -d "$ts" file || framework_failure_
 test time_check -nt file || skip_ "The system clock is wrong"
diff --git a/tests/id/zero.sh b/tests/id/zero.sh
index 939b162..c826ff4 100755
--- a/tests/id/zero.sh
+++ b/tests/id/zero.sh
@@ -42,7 +42,7 @@ printf '%s\n' $users '' >> users || framework_failure_
 
 # Exercise "id -z" with various options.
 printf '\n' > exp || framework_failure_
-:> out || framework_failure_
+> out || framework_failure_
 
 while read u ; do
   for o in g gr G Gr u ur ; do
diff --git a/tests/install/install-C.sh b/tests/install/install-C.sh
index 6ce4977..9ee1685 100755
--- a/tests/install/install-C.sh
+++ b/tests/install/install-C.sh
@@ -54,7 +54,7 @@ echo test > a || framework_failure_
 echo "'a' -> 'b'" > out_installed_first || framework_failure_
 echo "removed 'b'
 'a' -> 'b'" > out_installed_second || framework_failure_
-: > out_empty || framework_failure_
+> out_empty || framework_failure_
 
 # destination file does not exist
 ginstall -Cv -m$mode1 a b > out || fail=1
diff --git a/tests/misc/env.sh b/tests/misc/env.sh
index 0987f7f..877a5e3 100755
--- a/tests/misc/env.sh
+++ b/tests/misc/env.sh
@@ -62,7 +62,7 @@ fi
 
 ENV_TEST1=a
 export ENV_TEST1
-: >out || framework_failure_
+>out || framework_failure_
 env ENV_TEST2= > all || fail=1
 grep '^ENV_TEST' all | LC_ALL=C sort >> out || framework_failure_
 env -u ENV_TEST1 ENV_TEST3=c > all || fail=1
diff --git a/tests/misc/md5sum-bsd.sh b/tests/misc/md5sum-bsd.sh
index 77f0a59..f73e062 100755
--- a/tests/misc/md5sum-bsd.sh
+++ b/tests/misc/md5sum-bsd.sh
@@ -70,7 +70,7 @@ nl='
 tab='	'
 rm check.md5
 for i in 'a\b' 'a\' "a${nl}b" "a${tab}b"; do
-  :> "$i"
+  > "$i"
   md5sum --tag "$i" >> check.md5
 done
 md5sum --strict -c check.md5 || fail=1
diff --git a/tests/misc/runcon-no-reorder.sh b/tests/misc/runcon-no-reorder.sh
index 7338ad5..ca8ad5b 100755
--- a/tests/misc/runcon-no-reorder.sh
+++ b/tests/misc/runcon-no-reorder.sh
@@ -27,7 +27,7 @@ echo "$diag" > exp || framework_failure_
 # On such a system it fails with the above diagnostic, which is fine.
 # Before the no-reorder change, it would have failed with a diagnostic
 # about -j being an invalid option.
-runcon $(id -Z) true -j 2> out && : > exp
+runcon $(id -Z) true -j 2> out && > exp
 
 # When run on a system with no /selinux/context (i.e., in a chroot),
 # it chcon fails with this: "runcon: invalid context: \
diff --git a/tests/mv/partition-perm.sh b/tests/mv/partition-perm.sh
index 2a25e52..713988f 100755
--- a/tests/mv/partition-perm.sh
+++ b/tests/mv/partition-perm.sh
@@ -21,11 +21,11 @@ print_ver_ mv
 cleanup_() { rm -rf "$other_partition_tmpdir"; }
 . "$abs_srcdir/tests/other-fs-tmpdir"
 
-: > file
-chmod a=rwx file
+> file || framework_failure_
+chmod a=rwx file || framework_failure_
 
 umask 077
-mv file "$other_partition_tmpdir"
+mv file "$other_partition_tmpdir" || framework_failure_
 
 test -f file && fail=1
 test -f "$other_partition_tmpdir/file" || fail=1
diff --git a/tests/rm/r-root.sh b/tests/rm/r-root.sh
index 04a88eb..e17b85b 100755
--- a/tests/rm/r-root.sh
+++ b/tests/rm/r-root.sh
@@ -104,7 +104,7 @@ test -d dir && framework_failure_
 # rm(1) must succeed as before, but this time both the evidence file "x"
 # and the test file / directory must still exist afterward.
 mkdir dir || framework_failure_
-: > file  || framework_failure_
+> file    || framework_failure_
 
 skip=
 for file in dir file ; do
@@ -168,8 +168,8 @@ done
 # Exercise "rm -r file1 / file2".
 # Expect a non-Zero exit status representing failure to remove "/",
 # yet 'file1' and 'file2' should be removed.
-: > file1 || framework_failure_
-: > file2 || framework_failure_
+> file1 || framework_failure_
+> file2 || framework_failure_
 
 # Now that we know that 'rm' won't call the unlinkat() system function for "/",
 # we could probably execute it without the LD_PRELOAD'ed safety net.
diff --git a/tests/split/l-chunk.sh b/tests/split/l-chunk.sh
index 066fb01..792c556 100755
--- a/tests/split/l-chunk.sh
+++ b/tests/split/l-chunk.sh
@@ -71,7 +71,7 @@ DEBUGGING=
 test "$DEBUGGING" && test "$VERBOSE" && set +x
 for ELIDE_EMPTY in '' '-e'; do
   for IO_BLKSIZE in 1 2 5 10 80 100; do
-    : > out
+    > out
     test "$DEBUGGING" && printf "\n---io-blk-size=$IO_BLKSIZE $ELIDE_EMPTY\n"
     for N in 6 8 12 15 22; do
       rm -f x*
@@ -119,15 +119,15 @@ test "$DEBUGGING" && test "$VERBOSE" && set -x
 
 
 # Check extraction of particular chunks
-: > out
+> out
 printf '1\n12345\n' > exp
 split -n l/13/15 in > out
 compare exp out || fail=1
-: > out
+> out
 printf '' > exp
 split -n l/14/15 in > out
 compare exp out || fail=1
-: > out
+> out
 printf '1\n12345\n1\n' > exp
 split -n l/15/15 in > out
 compare exp out || fail=1
diff --git a/tests/split/line-bytes.sh b/tests/split/line-bytes.sh
index 9dc06a6..5f6f505 100755
--- a/tests/split/line-bytes.sh
+++ b/tests/split/line-bytes.sh
@@ -63,8 +63,8 @@ cat <<\EOF > no_eol_splits_exp
 EOF
 
 for b in $(seq 10); do
-  : > splits
-  : > no_eol_splits
+  > splits
+  > no_eol_splits
   for s in $(seq 11); do
     rm x??
     split ---io=$b -C$s in || fail=1
diff --git a/tests/tail-2/inotify-rotate.sh b/tests/tail-2/inotify-rotate.sh
index a95f4ec..1c942cc 100755
--- a/tests/tail-2/inotify-rotate.sh
+++ b/tests/tail-2/inotify-rotate.sh
@@ -47,7 +47,7 @@ for i in $(seq 50); do
     # Normally less than a second is required here, but with heavy load
     # and a lot of disk activity, even 20 seconds is insufficient, which
     # leads to this timeout killing tail before the "ok" is written below.
-    :>k && :>x || framework_failure_ failed to initialize files
+    >k && >x || framework_failure_ failed to initialize files
     timeout 40 tail -F k > out 2>&1 &
     pid=$!
     sleep .1
diff --git a/tests/tail-2/retry.sh b/tests/tail-2/retry.sh
index 5b74b74..dbe66a4 100755
--- a/tests/tail-2/retry.sh
+++ b/tests/tail-2/retry.sh
@@ -45,7 +45,7 @@ grep -F 'tail: warning: --retry ignored' out || fail=1
 # === Test:
 # Ensure that "tail --retry --follow=name" waits for the file to appear.
 # Clear 'out' so that we can check its contents without races
-:>out                           || framework_failure_
+>out                            || framework_failure_
 timeout 10 tail -s.1 --follow=name --retry missing >out 2>&1 & pid=$!
 retry_delay_ wait4lines_ .1 6 1 || fail=1  # Wait for "cannot open" error.
 echo "X" > missing              || fail=1
diff --git a/tests/tail-2/symlink.sh b/tests/tail-2/symlink.sh
index 362cb63..b21f9e1 100755
--- a/tests/tail-2/symlink.sh
+++ b/tests/tail-2/symlink.sh
@@ -32,7 +32,7 @@ wait4lines_ ()
 # Ensure changing targets of cli specified symlinks are handled.
 # Prior to v8.22, inotify would fail to recognize changes in the targets.
 # Clear 'out' so that we can check its contents without races.
-:>out                           || framework_failure_
+>out                            || framework_failure_
 ln -nsf target symlink          || framework_failure_
 timeout 10 tail -s.1 -F symlink >out 2>&1 & pid=$!
 retry_delay_ wait4lines_ .1 6 1 || fail=1  # Wait for "cannot open..."
@@ -50,7 +50,7 @@ rm -f target out           || framework_failure_
 # Ensure we correctly handle the source symlink itself changing.
 # I.E. that we don't operate solely on the targets.
 # Clear 'out' so that we can check its contents without races.
-:>out                           || framework_failure_
+>out                            || framework_failure_
 echo "X1" > target1             || framework_failure_
 ln -nsf target1 symlink         || framework_failure_
 timeout 10 tail -s.1 -F symlink >out 2>&1 & pid=$!
diff --git a/tests/tail-2/wait.sh b/tests/tail-2/wait.sh
index 42a93bf..13898ac 100755
--- a/tests/tail-2/wait.sh
+++ b/tests/tail-2/wait.sh
@@ -52,7 +52,7 @@ for inotify in ---disable-inotify ''; do
   grep -Ev 'inotify (resources exhausted|cannot be used)' tail.err > x
   mv x tail.err
   test -s tail.err && fail=1
-  :>tail.err
+  >tail.err
 
   tail_F()
   {
diff --git a/tests/touch/read-only.sh b/tests/touch/read-only.sh
index 95cd4f7..d239738 100755
--- a/tests/touch/read-only.sh
+++ b/tests/touch/read-only.sh
@@ -20,7 +20,7 @@
 print_ver_ touch
 skip_if_root_
 
-: > read-only || framework_failure_
+> read-only || framework_failure_
 chmod 444 read-only || framework_failure_
 
 
-- 
1.7.7.6