[PATCH v4 0/4] Add Travis CI support

[PATCH v4 0/4] Add Travis CI support

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

diff to v3:
* Removed git-p4 OS X test fixes and submitted them in a separate roll as
  request by Junio [1].
* Fix brew formula typo.
  (thanks Eric)
* Move "make configure && ./configure" from "install" to "before_script" phase.
  (thanks Sebastian)
* Run a build configuration with all kinds of "NO_*" flags. I don't understand
  all of them. Please let me know if some of them don't make sense. I also
  noticed that "NO_ICONV" will cause various tests to fail [2]. Is this a bug
  that should be fixed as part of this roll? If this is no bug, would it make
  sense to remove this flag?
  (thanks Junio, Matthieu)
* Run make with multiple jobs. I found that -j2 is the fastest for building
  git on Travis. -j6 is the fastest to run the unit tests. I limited that to
  -j3 because I had the impression that I got more test failures with higher
  utilization.
  (thanks Jean-Noel, Matthieu)
* Run Linux builds on new Travis CI container infrastructure .
  (thanks Matthieu)
* Print Perforce/Git-LFS binary versions
* Run tests with test extension flags enabled. Unfortunately "GIT_TEST_LONG"
  caused "t0021" to fail on Linux [3]. I wasn't able to reproduce this on my
  local machine, yet. Ideas anyone? Furthermore "GETTEXT_POISON" causes a bunch
  of failed tests on Linux and OS X [4]. Is this intentional or a bug?
* Print verbose output of failed tests in the "after_failure" phase. Please
  note the little triangle on the left in the TravisCI log to expand the failed
  test output.
* Update LFS to 1.0.2
* Accept SIGPIPE output in some tests (similar to [5] by Junio)
* Make git-p4 tests more robust with a general timeout and retry logic for
  cleanup commands (I now the "rm" and "mkdir" retry looks stupid but I was
  able to reproduce this error on TravisCI consistently - does anyone have
  a possible explanation why this could be necessary? Open file handles?)
* Disable email notifications (at least initially). I found and fixed a few
  issues that caused flaky tests. However, I expect there are more and we can
  only find them over time. If you add this patch then TravisCI can run
  silently in the background. If people want to look at the TravisCI result
  then they can. All other people are not bothered by emails. If we find in
  six months from now that TravisCI has too many false failures then we could
  remove the .travis.yml, again, and move on.

I had to apply this patch on master to get David Turner's fix for flaky
t7063 [6]/[7].

You can find the TravisCI run for this roll here:
https://travis-ci.org/larsxschneider/git/builds/89598194

Cheers,
Lars

[1] http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279362
[2] https://travis-ci.org/larsxschneider/git/builds/89555114
[3] https://travis-ci.org/larsxschneider/git/builds/89554983
[4] https://travis-ci.org/larsxschneider/git/builds/89554899
[5] http://thread.gmane.org/gmane.comp.version-control.git/280120/focus=280543
[6] http://thread.gmane.org/gmane.comp.version-control.git/279647
[7] http://thread.gmane.org/gmane.comp.version-control.git/279889

Lars Schneider (4):
  add function test_must_fail_or_sigpipe and use it to fix flaky tests
  git-p4: add p4d timeout in tests
  git-p4: retry kill/cleanup operations in tests with timeout
  Add Travis CI support

 .travis.yml                     | 131 ++++++++++++++++++++++++++++++++++++++++
 t/lib-git-p4.sh                 |  51 +++++++++++++---
 t/t5504-fetch-receive-strict.sh |   3 +-
 t/t5516-fetch-push.sh           |   8 +--
 t/test-lib-functions.sh         |  23 +++++++
 5 files changed, 201 insertions(+), 15 deletions(-)
 create mode 100644 .travis.yml

--
2.5.1

[PATCH v4 1/4] add function test_must_fail_or_sigpipe and use it to fix flaky tests

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" is
flaky in the following case:
1. remote upload-pack finds out "not our ref"
2. remote sends a response and closes the pipe
3. fetch-pack still tries to write commands to the remote upload-pack
4. write call in wrapper.c dies with SIGPIPE

t5504 "9 - push with transfer.fsckobjects" is flaky, too, and returns
SIGPIPE once in a while. I had to remove the final "To dst..." output
check because there is no output if the process dies with SIGPIPE.

This patch accepts the SIGPIPE exit as legitimate test exit.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/t5504-fetch-receive-strict.sh |  3 +--
 t/t5516-fetch-push.sh           |  8 ++++----
 t/test-lib-functions.sh         | 23 +++++++++++++++++++++++
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 44f3d5f..129efa8 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -111,8 +111,7 @@ test_expect_success 'push with transfer.fsckobjects' '
 		cd dst &&
 		git config transfer.fsckobjects true
 	) &&
-	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
-	test_cmp exp act
+	test_must_fail_or_sigpipe git push --porcelain dst master:refs/heads/test >act
 '

 cat >bogus-commit <<\EOF
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index ec22c98..22a941b 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1162,15 +1162,15 @@ do
 		mk_empty shallow &&
 		(
 			cd shallow &&
-			test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
-			test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
+			test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_3 &&
+			test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_1 &&
 			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
 			git fetch ../testrepo/.git $SHA1_1 &&
 			git cat-file commit $SHA1_1 &&
-			test_must_fail git cat-file commit $SHA1_2 &&
+			test_must_fail_or_sigpipe git cat-file commit $SHA1_2 &&
 			git fetch ../testrepo/.git $SHA1_2 &&
 			git cat-file commit $SHA1_2 &&
-			test_must_fail git fetch ../testrepo/.git $SHA1_3
+			test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_3
 		)
 	'
 done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 73e37a1..19a598e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -600,6 +600,29 @@ test_must_fail () {
 	return 0
 }

+# Similar to test_must_fail, but tolerates sigpipe signal, too.
+
+test_must_fail_or_sigpipe () {
+	"$@"
+	exit_code=$?
+	if test $exit_code = 0; then
+		echo >&2 "test_must_fail: command succeeded: $*"
+		return 1
+	elif test $exit_code -ne 141 && \
+		 test $exit_code -gt 129 && \
+		 test $exit_code -le 192; then
+		echo >&2 "test_must_fail: died by signal: $*"
+		return 1
+	elif test $exit_code = 127; then
+		echo >&2 "test_must_fail: command not found: $*"
+		return 1
+	elif test $exit_code = 126; then
+		echo >&2 "test_must_fail: valgrind error: $*"
+		return 1
+	fi
+	return 0
+}
+
 # Similar to test_must_fail, but tolerates success, too.  This is
 # meant to be used in contexts like:
 #
--
2.5.1

[PATCH v4 2/4] git-p4: add p4d timeout in tests

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

In rare cases p4d seems to hang. This watchdog will kill the p4d
process after 300s in any case. That means each individual git p4 test
needs to finish before 300s or it will fail.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/lib-git-p4.sh | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 7548225..b1660f6 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -6,6 +6,10 @@
 # a subdirectory called "$git"
 TEST_NO_CREATE_REPO=NoThanks
 
+# Sometimes p4d seems to hang. Terminate the p4d process automatically after
+# the defined timeout in seconds.
+P4D_TIMEOUT=300
+
 . ./test-lib.sh
 
 if ! test_have_prereq PYTHON
@@ -81,6 +85,19 @@ start_p4d() {
 	# will be caught with the "kill -0" check below.
 	i=${P4D_START_PATIENCE:-300}
 	pid=$(cat "$pidfile")
+
+	timeout=$(($(date +%s) + $P4D_TIMEOUT))
+    while true
+	do
+		if test $(date +%s) -gt $timeout
+		then
+			kill -9 $pid
+			exit 1
+		fi
+		sleep 1
+	done &
+	watchdog_pid=$!
+
 	ready=
 	while test $i -gt 0
 	do
@@ -131,7 +148,8 @@ kill_p4d() {
 	done &&
 	# complain if it would not die
 	test_must_fail kill $pid >/dev/null 2>&1 &&
-	rm -rf "$db" "$cli" "$pidfile"
+	rm -rf "$db" "$cli" "$pidfile" &&
+	retry_until_fail kill -9 $watchdog_pid
 }
 
 cleanup_git() {
-- 
2.5.1

[PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

In rare cases kill/cleanup operations in tests fail. Retry these
operations with a timeout to make the test less flaky.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 t/lib-git-p4.sh | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index b1660f6..9043b51 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -10,6 +10,10 @@ TEST_NO_CREATE_REPO=NoThanks
 # the defined timeout in seconds.
 P4D_TIMEOUT=300
 
+# Some operations require multiple attempts to be successful. Define
+# here the maximal retry timeout in seconds.
+RETRY_TIMEOUT=60
+
 . ./test-lib.sh
 
 if ! test_have_prereq PYTHON
@@ -138,14 +142,24 @@ p4_add_user() {
 	EOF
 }
 
+retry_until_success() {
+    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
+    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
+    do :
+    done
+}
+
+retry_until_fail() {
+    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
+    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
+    do :
+    done
+}
+
 kill_p4d() {
 	pid=$(cat "$pidfile")
-	# it had better exist for the first kill
-	kill $pid &&
-	for i in 1 2 3 4 5 ; do
-		kill $pid >/dev/null 2>&1 || break
-		sleep 1
-	done &&
+	retry_until_fail kill $pid
+	retry_until_fail kill -9 $pid
 	# complain if it would not die
 	test_must_fail kill $pid >/dev/null 2>&1 &&
 	rm -rf "$db" "$cli" "$pidfile" &&
@@ -153,8 +167,9 @@ kill_p4d() {
 }
 
 cleanup_git() {
-	rm -rf "$git" &&
-	mkdir "$git"
+	retry_until_success rm -r "$git"
+	test_must_fail test -d "$git" &&
+	retry_until_success mkdir "$git"
 }
 
 marshal_dump() {
-- 
2.5.1

[PATCH v4 4/4] Add Travis CI support

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

The tests are currently executed on "Ubuntu 12.04 LTS Server Edition
64 bit" and on "OS X Mavericks" using gcc and clang.

Perforce and Git-LFS are installed and therefore available for the
respective tests.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 .travis.yml | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 131 insertions(+)
 create mode 100644 .travis.yml

diff --git a/.travis.yml b/.travis.yml
new file mode 100644
index 0000000..61c70fa
--- /dev/null
+++ b/.travis.yml
@@ -0,0 +1,131 @@
+language: c
+
+os:
+  - linux
+  - osx
+
+compiler:
+  - clang
+  - gcc
+
+addons:
+  apt:
+    packages:
+    - language-pack-is
+
+env:
+  global:
+    - P4_VERSION="15.1"
+    - GIT_LFS_VERSION="1.0.2"
+    - DEFAULT_TEST_TARGET=prove
+    - GIT_PROVE_OPTS="--timer --jobs 3"
+    - GIT_TEST_OPTS="--verbose --tee"
+    - GETTEXT_ISO_LOCALE=YesPlease
+    - GETTEXT_LOCALE=YesPlease
+    # - GETTEXT_POISON=YesPlease
+    - GIT_TEST_CHAIN_LINT=YesPlease
+    - GIT_TEST_CLONE_2GB=YesPlease
+    # - GIT_TEST_LONG=YesPlease
+  matrix:
+    -
+      # NO_ICONV=YesPlease
+    - >
+      NO_CURL=YesPlease
+      NO_D_INO_IN_DIRENT=YesPlease
+      NO_DEFLATE_BOUND=YesPlease
+      NO_EXPAT=YesPlease
+      NO_GECOS_IN_PWENT=YesPlease
+      NO_GETTEXT=YesPlease
+      NO_HMAC_CTX_CLEANUP=YesPlease
+      NO_HSTRERROR=YesPlease
+      NO_INET_NTOP=YesPlease
+      NO_INET_PTON=YesPlease
+      NO_INITGROUPS=YesPlease
+      NO_INTTYPES_H=YesPlease
+      NO_IPV6=YesPlease
+      NO_IPV6=YesPlease
+      NO_LIBGEN_H=YesPlease
+      NO_MEMMEM=YesPlease
+      NO_MKDTEMP=YesPlease
+      NO_MKSTEMPS=YesPlease
+      NO_MMAP=YesPlease
+      NO_NSEC=YesPlease
+      NO_OPENSSL=YesPlease
+      NO_PERL=YesPlease
+      NO_PTHREADS=YesPlease
+      NO_REGEX=YesPlease
+      NO_SETENV=YesPlease
+      NO_SETITIMER=YesPlease
+      NO_SOCKADDR_STORAGE=YesPlease
+      NO_STRCASESTR=YesPlease
+      NO_STRLCPY=YesPlease
+      NO_STRTOUMAX=YesPlease
+      NO_STRUCT_ITIMERVAL=YesPlease
+      NO_SYMLINK_HEAD=YesPlease
+      NO_SYS_POLL_H=YesPlease
+      NO_SYS_SELECT_H=YesPlease
+      NO_UINTMAX_T=YesPlease
+      NO_UNSETENV=YesPlease
+
+before_install:
+  - >
+    case "${TRAVIS_OS_NAME:-linux}" in
+    linux)
+      mkdir --parents custom/p4
+      pushd custom/p4
+        wget --quiet http://filehost.perforce.com/perforce/r$P4_VERSION/bin.linux26x86_64/p4d
+        wget --quiet http://filehost.perforce.com/perforce/r$P4_VERSION/bin.linux26x86_64/p4
+        chmod u+x p4d
+        chmod u+x p4
+        export PATH="$(pwd):$PATH"
+      popd
+      mkdir --parents custom/git-lfs
+      pushd custom/git-lfs
+        wget --quiet https://github.com/github/git-lfs/releases/download/v$GIT_LFS_VERSION/git-lfs-linux-amd64-$GIT_LFS_VERSION.tar.gz
+        tar --extract --gunzip --file "git-lfs-linux-amd64-$GIT_LFS_VERSION.tar.gz"
+        cp git-lfs-$GIT_LFS_VERSION/git-lfs .
+        export PATH="$(pwd):$PATH"
+      popd
+      ;;
+    osx)
+      brew_force_set_latest_binary_hash () {
+        FORMULA=$1
+        SHA=$(brew fetch --force $FORMULA 2>&1 | grep ^SHA256: | cut -d ' ' -f 2)
+        sed -E -i.bak "s/sha256 \"[0-9a-f]{64}\"/sha256 \"$SHA\"/g" \
+          /usr/local/Library/Taps/homebrew/homebrew-binary/$FORMULA.rb
+      }
+      brew update --quiet
+      brew tap homebrew/binary --quiet
+      brew_force_set_latest_binary_hash perforce
+      brew_force_set_latest_binary_hash perforce-server
+      brew install git-lfs perforce-server perforce
+      ;;
+    esac;
+    echo "$(tput setaf 6)Perfoce Server Version$(tput sgr0)";
+    p4d -V | grep Rev.;
+    echo "$(tput setaf 6)Perfoce Client Version$(tput sgr0)";
+    p4 -V | grep Rev.;
+    echo "$(tput setaf 6)Git-LFS Version$(tput sgr0)";
+    git-lfs version;
+
+before_script: make configure && ./configure && make --jobs=2
+
+script: make --quiet test
+
+after_failure:
+  - >
+    : '<-- Click here to see detailed test output!                                                        ';
+    for TEST_EXIT in t/test-results/*.exit;
+    do
+      if [ "$(cat "$TEST_EXIT")" != "0" ];
+      then
+        TEST_OUT="${TEST_EXIT%exit}out";
+        echo "------------------------------------------------------------------------";
+        echo "$(tput setaf 1)${TEST_OUT}...$(tput sgr0)";
+        echo "------------------------------------------------------------------------";
+        cat "${TEST_OUT}";
+      fi;
+    done;
+
+notifications:
+  email: false
-- 
2.5.1

Re: [PATCH v4 2/4] git-p4: add p4d timeout in tests

[Eric Sunshine]

On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschneider@gmail.com> wrote:
> In rare cases p4d seems to hang. This watchdog will kill the p4d
> process after 300s in any case. That means each individual git p4 test
> needs to finish before 300s or it will fail.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> @@ -81,6 +85,19 @@ start_p4d() {
>         # will be caught with the "kill -0" check below.
>         i=${P4D_START_PATIENCE:-300}
>         pid=$(cat "$pidfile")
> +
> +       timeout=$(($(date +%s) + $P4D_TIMEOUT))
> +    while true

The 'while' line is incorrectly indented with spaces rather than tabs.

> +       do
> +               if test $(date +%s) -gt $timeout

I don't know how portable you intend this to be, but 'date +%s' is not
universally supported (it's missing on Solaris, for instance, and
perhaps AIX). For 6a9d16a (filter-branch: add passed/remaining seconds
on progress, 2015-09-07), we ultimately settled upon detecting +%s
support dynamically:

    if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
        # it's supported
    fi

Perhaps you'd want to detect this via a lazy prerequisite and skip
this if not supported?

> +               then
> +                       kill -9 $pid
> +                       exit 1
> +               fi
> +               sleep 1
> +       done &
> +       watchdog_pid=$!
> +
>         ready=
>         while test $i -gt 0
>         do
> @@ -131,7 +148,8 @@ kill_p4d() {
>         done &&
>         # complain if it would not die
>         test_must_fail kill $pid >/dev/null 2>&1 &&
> -       rm -rf "$db" "$cli" "$pidfile"
> +       rm -rf "$db" "$cli" "$pidfile" &&
> +       retry_until_fail kill -9 $watchdog_pid
>  }
>
>  cleanup_git() {
> --
> 2.5.1

Re: [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout

[Eric Sunshine]

On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschneider@gmail.com> wrote:
> In rare cases kill/cleanup operations in tests fail. Retry these
> operations with a timeout to make the test less flaky.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> +retry_until_success() {
> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
> +    do :
> +    done
> +}
> +
> +retry_until_fail() {
> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
> +    do :
> +    done
> +}

I'm confused by this. Patch 2/4 was already calling
retry_until_fail(), but it's introduction seems to be here in patch
3/4. Am I missing something obvious?

>  kill_p4d() {
>         pid=$(cat "$pidfile")
> -       # it had better exist for the first kill
> -       kill $pid &&
> -       for i in 1 2 3 4 5 ; do
> -               kill $pid >/dev/null 2>&1 || break
> -               sleep 1
> -       done &&
> +       retry_until_fail kill $pid
> +       retry_until_fail kill -9 $pid
>         # complain if it would not die
>         test_must_fail kill $pid >/dev/null 2>&1 &&
>         rm -rf "$db" "$cli" "$pidfile" &&
> @@ -153,8 +167,9 @@ kill_p4d() {
>  }
>
>  cleanup_git() {
> -       rm -rf "$git" &&
> -       mkdir "$git"
> +       retry_until_success rm -r "$git"
> +       test_must_fail test -d "$git" &&
> +       retry_until_success mkdir "$git"
>  }
>
>  marshal_dump() {
> --
> 2.5.1

Re: [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout

[Lars Schneider]

> On 06 Nov 2015, at 10:28, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschneider@gmail.com> wrote:
>> In rare cases kill/cleanup operations in tests fail. Retry these
>> operations with a timeout to make the test less flaky.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> +retry_until_success() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
>> +    done
>> +}
>> +
>> +retry_until_fail() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
>> +    done
>> +}
> 
> I'm confused by this. Patch 2/4 was already calling
> retry_until_fail(), but it's introduction seems to be here in patch
> 3/4. Am I missing something obvious?

No, my fault. I reordered the commits and forgot about this. I will fix the order in the next roll.

Thanks,
Lars

Re: [PATCH v4 4/4] Add Travis CI support

[Eric Sunshine]

On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschneider@gmail.com> wrote:
> The tests are currently executed on "Ubuntu 12.04 LTS Server Edition
> 64 bit" and on "OS X Mavericks" using gcc and clang.
>
> Perforce and Git-LFS are installed and therefore available for the
> respective tests.
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> diff --git a/.travis.yml b/.travis.yml
> @@ -0,0 +1,131 @@
> +  matrix:
> +    -
> +      # NO_ICONV=YesPlease
> +    - >
> +      NO_CURL=YesPlease
> +      NO_D_INO_IN_DIRENT=YesPlease
> +      NO_DEFLATE_BOUND=YesPlease
> +      NO_EXPAT=YesPlease
> +      NO_GECOS_IN_PWENT=YesPlease
> +      NO_GETTEXT=YesPlease
> +      NO_HMAC_CTX_CLEANUP=YesPlease
> +      NO_HSTRERROR=YesPlease
> +      NO_INET_NTOP=YesPlease
> +      NO_INET_PTON=YesPlease
> +      NO_INITGROUPS=YesPlease
> +      NO_INTTYPES_H=YesPlease
> +      NO_IPV6=YesPlease
> +      NO_IPV6=YesPlease
> +      NO_LIBGEN_H=YesPlease
> +      NO_MEMMEM=YesPlease
> +      NO_MKDTEMP=YesPlease
> +      NO_MKSTEMPS=YesPlease
> +      NO_MMAP=YesPlease
> +      NO_NSEC=YesPlease
> +      NO_OPENSSL=YesPlease
> +      NO_PERL=YesPlease
> +      NO_PTHREADS=YesPlease
> +      NO_REGEX=YesPlease
> +      NO_SETENV=YesPlease
> +      NO_SETITIMER=YesPlease
> +      NO_SOCKADDR_STORAGE=YesPlease
> +      NO_STRCASESTR=YesPlease
> +      NO_STRLCPY=YesPlease
> +      NO_STRTOUMAX=YesPlease
> +      NO_STRUCT_ITIMERVAL=YesPlease
> +      NO_SYMLINK_HEAD=YesPlease
> +      NO_SYS_POLL_H=YesPlease
> +      NO_SYS_SELECT_H=YesPlease
> +      NO_UINTMAX_T=YesPlease
> +      NO_UNSETENV=YesPlease

Hmm, aren't you losing test coverage by disabling some of these? Is
that desirable for continuous integration testing? Am I missing
something?

Re: [PATCH v4 4/4] Add Travis CI support

[Lars Schneider]

> On 06 Nov 2015, at 10:56, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschneider@gmail.com> wrote:
>> The tests are currently executed on "Ubuntu 12.04 LTS Server Edition
>> 64 bit" and on "OS X Mavericks" using gcc and clang.
>> 
>> Perforce and Git-LFS are installed and therefore available for the
>> respective tests.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> diff --git a/.travis.yml b/.travis.yml
>> @@ -0,0 +1,131 @@
>> +  matrix:
>> +    -
>> +      # NO_ICONV=YesPlease
>> +    - >
>> +      NO_CURL=YesPlease
>> +      NO_D_INO_IN_DIRENT=YesPlease
>> +      NO_DEFLATE_BOUND=YesPlease
>> +      NO_EXPAT=YesPlease
>> +      NO_GECOS_IN_PWENT=YesPlease
>> +      NO_GETTEXT=YesPlease
>> +      NO_HMAC_CTX_CLEANUP=YesPlease
>> +      NO_HSTRERROR=YesPlease
>> +      NO_INET_NTOP=YesPlease
>> +      NO_INET_PTON=YesPlease
>> +      NO_INITGROUPS=YesPlease
>> +      NO_INTTYPES_H=YesPlease
>> +      NO_IPV6=YesPlease
>> +      NO_IPV6=YesPlease
>> +      NO_LIBGEN_H=YesPlease
>> +      NO_MEMMEM=YesPlease
>> +      NO_MKDTEMP=YesPlease
>> +      NO_MKSTEMPS=YesPlease
>> +      NO_MMAP=YesPlease
>> +      NO_NSEC=YesPlease
>> +      NO_OPENSSL=YesPlease
>> +      NO_PERL=YesPlease
>> +      NO_PTHREADS=YesPlease
>> +      NO_REGEX=YesPlease
>> +      NO_SETENV=YesPlease
>> +      NO_SETITIMER=YesPlease
>> +      NO_SOCKADDR_STORAGE=YesPlease
>> +      NO_STRCASESTR=YesPlease
>> +      NO_STRLCPY=YesPlease
>> +      NO_STRTOUMAX=YesPlease
>> +      NO_STRUCT_ITIMERVAL=YesPlease
>> +      NO_SYMLINK_HEAD=YesPlease
>> +      NO_SYS_POLL_H=YesPlease
>> +      NO_SYS_SELECT_H=YesPlease
>> +      NO_UINTMAX_T=YesPlease
>> +      NO_UNSETENV=YesPlease
> 
> Hmm, aren't you losing test coverage by disabling some of these? Is
> that desirable for continuous integration testing? Am I missing
> something?
Per platform/compiler (Linux&Mac/clang&gcc) we run two configurations. One normal configuration (see the lonely "-" right under "matrix:") and one configuration with all sorts of things are disabled ("NO...").

You can see all 8 configurations ([linux, mac] * [clang, gcc] * [normal, NO_...]) here:
https://travis-ci.org/larsxschneider/git/builds/89598194

Cheers,
Lars 

Re: [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout

[Lars Schneider]

> On 06 Nov 2015, at 10:28, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschneider@gmail.com> wrote:
>> In rare cases kill/cleanup operations in tests fail. Retry these
>> operations with a timeout to make the test less flaky.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> +retry_until_success() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +    until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
>> +    done
>> +}
>> +
>> +retry_until_fail() {
>> +    timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +    until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +    do :
>> +    done
>> +}
> 
> I'm confused by this. Patch 2/4 was already calling
> retry_until_fail(), but it's introduction seems to be here in patch
> 3/4. Am I missing something obvious?
No, my fault. I reordered the commits and forgot about this. I will fix the order in the next roll.

Thanks,
Lars

Re: [PATCH v4 4/4] Add Travis CI support

[Sebastian Schuberth]

On Fri, Nov 6, 2015 at 2:11 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

> Per platform/compiler (Linux&Mac/clang&gcc) we run two configurations. One
> normal configuration (see the lonely "-" right under "matrix:") and one
> configuration with all sorts of things are disabled ("NO...").
>
> You can see all 8 configurations ([linux, mac] * [clang, gcc] * [normal,
> NO_...]) here:
> https://travis-ci.org/larsxschneider/git/builds/89598194

Aren't these 8 configurations a bit too much? I see the total running
time is about 2 hours. For my taste, this is way to much to give
developers feedback about the status of their PR. It should be
something < 30 minutes.

IMO, the purpose of the Travis CI configuration mainly is to 1) save
developers work by not requiring to build manually, 2) build on other
platforms than the developer has access to. I doubt that the average
developer manually builds anything close to these 8 configurations
before we had this job, so I wouldn't make it a requirement for Travis
to do much more than a developer could / would to manually.

On the other hand, I see the point in letting a CI system test more
configurations than manually feasible. But that should be done as part
of a different job then. E.g. we could have a "fast" PR validation
job, and a "slow" nightly build job.

-- 
Sebastian Schuberth

Re: [PATCH v4 2/4] git-p4: add p4d timeout in tests

[Lars Schneider]

> On 06 Nov 2015, at 10:23, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschneider@gmail.com> wrote:
>> In rare cases p4d seems to hang. This watchdog will kill the p4d
>> process after 300s in any case. That means each individual git p4 test
>> needs to finish before 300s or it will fail.
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> @@ -81,6 +85,19 @@ start_p4d() {
>>        # will be caught with the "kill -0" check below.
>>        i=${P4D_START_PATIENCE:-300}
>>        pid=$(cat "$pidfile")
>> +
>> +       timeout=$(($(date +%s) + $P4D_TIMEOUT))
>> +    while true
> 
> The 'while' line is incorrectly indented with spaces rather than tabs.'

Oh, you're right. One more thing that I will add to my pre mailing list submit check script :-)

> 
>> +       do
>> +               if test $(date +%s) -gt $timeout
> 
> I don't know how portable you intend this to be, but 'date +%s' is not
> universally supported (it's missing on Solaris, for instance, and
> perhaps AIX). For 6a9d16a (filter-branch: add passed/remaining seconds
> on progress, 2015-09-07), we ultimately settled upon detecting +%s
> support dynamically:
> 
>    if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
>        # it's supported
>    fi
> 
> Perhaps you'd want to detect this via a lazy prerequisite and skip
> this if not supported?

AFAIK Perforce does not run on Solaris and AIX anyways (see supported platforms [1]). Therefore these tests should not be executed on these platforms. A lazy prerequisite sounds like a good idea to make this explicit!

Thanks,
Lars

[1] https://www.perforce.com/perforce/doc.current/user/relnotes.txt

Re: [PATCH v4 4/4] Add Travis CI support

[Lars Schneider]

> On 06 Nov 2015, at 14:20, Sebastian Schuberth <sschuberth@gmail.com> wrote:
> 
> On Fri, Nov 6, 2015 at 2:11 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
> 
>> Per platform/compiler (Linux&Mac/clang&gcc) we run two configurations. One
>> normal configuration (see the lonely "-" right under "matrix:") and one
>> configuration with all sorts of things are disabled ("NO...").
>> 
>> You can see all 8 configurations ([linux, mac] * [clang, gcc] * [normal,
>> NO_...]) here:
>> https://travis-ci.org/larsxschneider/git/builds/89598194
> 
> Aren't these 8 configurations a bit too much? I see the total running
> time is about 2 hours. For my taste, this is way to much to give
> developers feedback about the status of their PR. It should be
> something < 30 minutes.
> 
> IMO, the purpose of the Travis CI configuration mainly is to 1) save
> developers work by not requiring to build manually, 2) build on other
> platforms than the developer has access to. I doubt that the average
> developer manually builds anything close to these 8 configurations
> before we had this job, so I wouldn't make it a requirement for Travis
> to do much more than a developer could / would to manually.
> 
> On the other hand, I see the point in letting a CI system test more
> configurations than manually feasible. But that should be done as part
> of a different job then. E.g. we could have a "fast" PR validation
> job, and a "slow" nightly build job.
> 

Well, I partly agree. Right now the running time is ~20 min (that means less than your 30min target!). After ~10min you even have all Linux results, Mac takes a bit longer. Travis shows you 2h because that is the time that would be required if all builds where run one after another (we run builds in parallel).

That being said, I see your point of to avoiding to burn Travis CI resources meaningless. If I am not mistaken then you can configure Travis in a way that it runs different configurations for different branches. E.g. I would like to run all 8 configurations on maint, master, next and maybe pu. All other branches on peoples own forks should be fine with the default Linux build (~10min).

What do you think?

Thanks,
Lars

Re: [PATCH v4 4/4] Add Travis CI support

[Sebastian Schuberth]

On Fri, Nov 6, 2015 at 2:28 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

> Well, I partly agree. Right now the running time is ~20 min (that means less than your 30min target!). After ~10min you even have all Linux results, Mac takes a bit longer. Travis shows you 2h because that is the time that would be required if all builds where run one after another (we run builds in parallel).

Are you sure about than? I mean, what sense does it make to show how
long it *would* have taken *if* the builds were running serially? I
can see that the longest of the jobs took 21 minutes, which is ok. But
that does not mean that all jobs completed in within 21 minutes. It
could be that not all jobs started at (about) the same time due to a
lack of resources, and that the last job did not compete before the 2
hours were over because it only started to run 1 hours and 40 minutes
befor ethe first job was started.

> That being said, I see your point of to avoiding to burn Travis CI resources meaningless. If I am not mistaken then you can configure Travis in a way that it runs different configurations for different branches. E.g. I would like to run all 8 configurations on maint, master, next and maybe pu. All other branches on peoples own forks should be fine with the default Linux build (~10min).
>
> What do you think?

I think running different configuration per branch makes sense, yes.

-- 
Sebastian Schuberth

Re: [PATCH v4 4/4] Add Travis CI support

[Lars Schneider]

> On 06 Nov 2015, at 14:36, Sebastian Schuberth <sschuberth@gmail.com> wrote:
> 
> On Fri, Nov 6, 2015 at 2:28 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
> 
>> Well, I partly agree. Right now the running time is ~20 min (that means less than your 30min target!). After ~10min you even have all Linux results, Mac takes a bit longer. Travis shows you 2h because that is the time that would be required if all builds where run one after another (we run builds in parallel).
> 
> Are you sure about than? I mean, what sense does it make to show how
> long it *would* have taken *if* the builds were running serially? I
> can see that the longest of the jobs took 21 minutes, which is ok. But
> that does not mean that all jobs completed in within 21 minutes. It
> could be that not all jobs started at (about) the same time due to a
> lack of resources, and that the last job did not compete before the 2
> hours were over because it only started to run 1 hours and 40 minutes
> befor ethe first job was started.
I am fairly certain about this. 

See, here is the first configuration and the first test case of a job:
https://travis-ci.org/larsxschneider/git/jobs/89598195
[08:21:06] t0002-gitfile.sh 

Here is the last configuration and the last test case of the same job:
[08:51:22] t9903-bash-prompt.sh

~30 min for all 8 configurations. You can enable Travis CI for you GitHub account and try it easily yourself :-)

> 
>> That being said, I see your point of to avoiding to burn Travis CI resources meaningless. If I am not mistaken then you can configure Travis in a way that it runs different configurations for different branches. E.g. I would like to run all 8 configurations on maint, master, next and maybe pu. All other branches on peoples own forks should be fine with the default Linux build (~10min).
>> 
>> What do you think?
> 
> I think running different configuration per branch makes sense, yes.
If the list decides to accept this patch. Do you think that would be a necessary requirement for the first iteration?

Thanks,
Lars

Re: [PATCH v4 4/4] Add Travis CI support

[Sebastian Schuberth]

On Fri, Nov 6, 2015 at 2:55 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

>> I think running different configuration per branch makes sense, yes.
>
> If the list decides to accept this patch. Do you think that would be a necessary requirement for the first iteration?

No. I think this could be addressed later as an improvements. To me
it's more important to finally get *some* sane Travis CI configuration
in.

-- 
Sebastian Schuberth

Re: [PATCH v4 4/4] Add Travis CI support

[Lars Schneider]

> On 06 Nov 2015, at 14:57, Sebastian Schuberth <sschuberth@gmail.com> wrote:
> 
> On Fri, Nov 6, 2015 at 2:55 PM, Lars Schneider <larsxschneider@gmail.com> wrote:
> 
>>> I think running different configuration per branch makes sense, yes.
>> 
>> If the list decides to accept this patch. Do you think that would be a necessary requirement for the first iteration?
> 
> No. I think this could be addressed later as an improvements. To me
> it's more important to finally get *some* sane Travis CI configuration
> in.
True. However, as I stated in my v4 cover letter the Travis CI integration is not yet perfect. I am constantly running builds to find flaky tests. Eg. here is one of them in git-p4 area that I will tackle next:
https://s3.amazonaws.com/archive.travis-ci.org/jobs/89603763/log.txt

I also see a weird "prove Tests our of sequence" error one in a while:
https://s3.amazonaws.com/archive.travis-ci.org/jobs/89603770/log.txt

Does anyone have an idea what could cause this?

Thanks,
Lars

Re: [PATCH v4 1/4] add function test_must_fail_or_sigpipe and use it to fix flaky tests

[Junio C Hamano]

larsxschneider@gmail.com writes:

> From: Lars Schneider <larsxschneider@gmail.com>
>
> t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" is
> flaky in the following case:
> 1. remote upload-pack finds out "not our ref"
> 2. remote sends a response and closes the pipe
> 3. fetch-pack still tries to write commands to the remote upload-pack
> 4. write call in wrapper.c dies with SIGPIPE
>
> t5504 "9 - push with transfer.fsckobjects" is flaky, too, and returns
> SIGPIPE once in a while. I had to remove the final "To dst..." output
> check because there is no output if the process dies with SIGPIPE.

Thanks for a clear write-up.

> This patch accepts the SIGPIPE exit as legitimate test exit.

It is not this patch that accepts such a failure as legit ;-).

"Accept such a death-with-sigpipe also as OK when we are expecting a
failure", perhaps?



> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index ec22c98..22a941b 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1162,15 +1162,15 @@ do
>  		mk_empty shallow &&
>  		(
>  			cd shallow &&
> -			test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
> -			test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
> +			test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_3 &&
> +			test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_1 &&
>  			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&

These I understand.

>  			git fetch ../testrepo/.git $SHA1_1 &&
>  			git cat-file commit $SHA1_1 &&
> -			test_must_fail git cat-file commit $SHA1_2 &&
> +			test_must_fail_or_sigpipe git cat-file commit $SHA1_2 &&

This I don't.  Under what condition is it sane for this "cat-file
commit" to fail with sigpipe?

>  			git fetch ../testrepo/.git $SHA1_2 &&
>  			git cat-file commit $SHA1_2 &&
> -			test_must_fail git fetch ../testrepo/.git $SHA1_3
> +			test_must_fail_or_sigpipe git fetch ../testrepo/.git $SHA1_3

And this I do.

>  		)
>  	'
>  done
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 73e37a1..19a598e 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -600,6 +600,29 @@ test_must_fail () {
>  	return 0
>  }
>
> +# Similar to test_must_fail, but tolerates sigpipe signal, too.
> +
> +test_must_fail_or_sigpipe () {
> +	"$@"
> +	exit_code=$?
> +	if test $exit_code = 0; then
> +		echo >&2 "test_must_fail: command succeeded: $*"
> +		return 1
> +	elif test $exit_code -ne 141 && \
> +		 test $exit_code -gt 129 && \
> +		 test $exit_code -le 192; then
> +		echo >&2 "test_must_fail: died by signal: $*"
> +		return 1
> +	elif test $exit_code = 127; then
> +		echo >&2 "test_must_fail: command not found: $*"
> +		return 1
> +	elif test $exit_code = 126; then
> +		echo >&2 "test_must_fail: valgrind error: $*"
> +		return 1
> +	fi
> +	return 0
> +}
> +

When you are coming up with this patch, you must have checked the
existing code around this area.  Did you notice that 126 is handled
differently between must_fail and might_fail?  Can you explain and
justify the difference?

One explanation I can think of is that the contributor who did
eeb69131 (tests: notice valgrind error in test_must_fail,
2013-03-31) did not notice that we do essentially the same thing in
might_fail and forgot to add it.  And it is not the fault of that
contributor, but the fault of the duplicated and poorly organized
code back then.

Adding the third variant in the way this patch does is making things
worse by inviting more mistakes.

How about doing something like the attached to consolidate the
existing two into one, and then build this third one on top?

 t/test-lib-functions.sh | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e8d3c0f..54497d6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -560,22 +560,7 @@ test_line_count () {
 # the failure could be due to a segv.  We want a controlled failure.
 
 test_must_fail () {
-	"$@"
-	exit_code=$?
-	if test $exit_code = 0; then
-		echo >&2 "test_must_fail: command succeeded: $*"
-		return 1
-	elif test $exit_code -gt 129 && test $exit_code -le 192; then
-		echo >&2 "test_must_fail: died by signal: $*"
-		return 1
-	elif test $exit_code = 127; then
-		echo >&2 "test_must_fail: command not found: $*"
-		return 1
-	elif test $exit_code = 126; then
-		echo >&2 "test_must_fail: valgrind error: $*"
-		return 1
-	fi
-	return 0
+	test_failure_helper test_must_fail "$@"
 }
 
 # Similar to test_must_fail, but tolerates success, too.  This is
@@ -590,13 +575,29 @@ test_must_fail () {
 # because we want to notice if it fails due to segv.
 
 test_might_fail () {
+	test_failure_helper test_might_fail "$@"
+}
+
+test_failure_helper () {
+	test_expect=$1
+	shift
 	"$@"
 	exit_code=$?
-	if test $exit_code -gt 129 && test $exit_code -le 192; then
-		echo >&2 "test_might_fail: died by signal: $*"
+	if test $test_expect != test_might_fail && test $exit_code = 0
+	then
+		echo >&2 "$test_expect: command succeeded: $*"
+		return 1
+	elif test $exit_code -gt 129 && test $exit_code -le 192
+	then
+		echo >&2 "$test_expect: died by signal: $*"
+		return 1
+	elif test $exit_code = 127
+	then
+		echo >&2 "$test_expect: command not found: $*"
 		return 1
-	elif test $exit_code = 127; then
-		echo >&2 "test_might_fail: command not found: $*"
+	elif test $exit_code = 126
+	then
+		echo >&2 "$test_expect: valgrind error: $*"
 		return 1
 	fi
 	return 0

Re: [PATCH v4 1/4] add function test_must_fail_or_sigpipe and use it to fix flaky tests

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Adding the third variant in the way this patch does is making things
> worse by inviting more mistakes.
>
> How about doing something like the attached to consolidate the
> existing two into one, and then build this third one on top?

Actually, I think this other variant I came up with is cleaner and
is more easily extensible.  It already has the support for sigpipe,
e.g. you can do

	test_must_fail ok=sigpipe git fetch ...

Of course, you can do

	test_must_fail_or_sigpipe () {
		test_must_fail ok=sigpipe "$@"
	}

if it is easier to read.  I do not have a very strong opinion either
way.

 t/test-lib-functions.sh | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e8d3c0f..b732f87 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -560,11 +560,26 @@ test_line_count () {
 # the failure could be due to a segv.  We want a controlled failure.
 
 test_must_fail () {
+	case "$1" in
+	ok=*)
+		_test_ok=${1#ok=}
+		shift
+		;;
+	*)
+		_test_ok=
+		;;
+	esac
 	"$@"
 	exit_code=$?
-	if test $exit_code = 0; then
+	if ! case ",$_test_ok," in *,success,*) false;; esac &&
+	   test $exit_code = 0
+	then
 		echo >&2 "test_must_fail: command succeeded: $*"
 		return 1
+	elif ! case ",$_test_ok," in *,sigpipe,*) false;; esac &&
+	   test $exit_code = 141
+	then
+		return 0
 	elif test $exit_code -gt 129 && test $exit_code -le 192; then
 		echo >&2 "test_must_fail: died by signal: $*"
 		return 1
@@ -590,16 +605,7 @@ test_must_fail () {
 # because we want to notice if it fails due to segv.
 
 test_might_fail () {
-	"$@"
-	exit_code=$?
-	if test $exit_code -gt 129 && test $exit_code -le 192; then
-		echo >&2 "test_might_fail: died by signal: $*"
-		return 1
-	elif test $exit_code = 127; then
-		echo >&2 "test_might_fail: command not found: $*"
-		return 1
-	fi
-	return 0
+	test_must_fail ok=success "$@"
 }
 
 # Similar to test_must_fail and test_might_fail, but check that a