[PATCH 0/2] fix quotes in maint-reflog-beyond-horizon and detached-stash

[PATCH 0/2] fix quotes in maint-reflog-beyond-horizon and detached-stash

[Jon Seymour]

In two recent series, I introduced tests that use test_must_fail incorrectly.

In effect, I was calling:

   test_must_fail "foo bar"

but this was failing because "foo bar" is not comamnd and not because
the command "foo" fails with arguments "bar".

These two patches fix these problems.

If necessary, I can re-roll the original series. Otherwise, these patches
can be squashed with the test-oriented commits currently in next.

Jon Seymour (2):
  maint-reflog-beyond-horizon: fix broken test_must_fail calls
  detached-stash: fix broken test_must_fail calls

 t/t1503-rev-parse-verify.sh |    4 ++--
 t/t3903-stash.sh            |   36 ++++++++++++++++++------------------
 2 files changed, 20 insertions(+), 20 deletions(-)

-- 
1.7.2.80.g22196.dirty

[PATCH 1/2] maint-reflog-beyond-horizon: fix broken test_must_fail calls

[Jon Seymour]

Some tests in maint-reflog-beyond-horizon are calling test_must_fail
in such a way that the arguments to test_must_fail do, indeed, fail
but not in the manner expected by the test.

This patch removes the unnecessary and unhelpful double quotes.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t1503-rev-parse-verify.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index 61092f7..100f857 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -111,8 +111,8 @@ test_expect_success 'master@{n} for various n' '
 	git rev-parse --verify master@{0} &&
 	git rev-parse --verify master@{1} &&
 	git rev-parse --verify master@{$Nm1} &&
-	test_must_fail "git rev-parse --verify master@{$N}" &&
-	test_must_fail "git rev-parse --verify master@{$Np1}"
+	test_must_fail git rev-parse --verify master@{$N} &&
+	test_must_fail git rev-parse --verify master@{$Np1}
 '
 
 test_done
-- 
1.7.2.80.g22196.dirty

[PATCH 2/2] detached-stash: fix broken test_must_fail calls

[Jon Seymour]

Some tests in detached-stash are calling test_must_fail
in such a way that the arguments to test_must_fail do, indeed, fail
but not in the manner expected by the test.

This patch removes the unnecessary and unhelpful double quotes.

Signed-off-by: Jon Seymour <jon.seymour@gmail.com>
---
 t/t3903-stash.sh |   36 ++++++++++++++++++------------------
 1 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ea9f979..d99f27a 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -435,7 +435,7 @@ test_expect_success 'stash drop - fail early if specified stash is not a stash r
 	git stash &&
 	echo bar > file &&
 	git stash &&
-	test_must_fail "git stash drop $(git rev-parse stash@{0})" &&
+	test_must_fail git stash drop $(git rev-parse stash@{0}) &&
 	git stash pop &&
 	test bar = "$(cat file)" &&
 	git reset --hard HEAD
@@ -449,7 +449,7 @@ test_expect_success 'stash pop - fail early if specified stash is not a stash re
 	git stash &&
 	echo bar > file &&
 	git stash &&
-	test_must_fail "git stash pop $(git rev-parse stash@{0})" &&
+	test_must_fail git stash pop $(git rev-parse stash@{0}) &&
 	git stash pop &&
 	test bar = "$(cat file)" &&
 	git reset --hard HEAD
@@ -462,31 +462,31 @@ test_expect_success 'ref with non-existant reflog' '
 	git add file2 &&
 	git stash &&
 	! "git rev-parse --quiet --verify does-not-exist" &&
-	test_must_fail "git stash drop does-not-exist" &&
-	test_must_fail "git stash drop does-not-exist@{0}" &&
-	test_must_fail "git stash pop does-not-exist" &&
-	test_must_fail "git stash pop does-not-exist@{0}" &&
-	test_must_fail "git stash apply does-not-exist" &&
-	test_must_fail "git stash apply does-not-exist@{0}" &&
-	test_must_fail "git stash show does-not-exist" &&
-	test_must_fail "git stash show does-not-exist@{0}" &&
-	test_must_fail "git stash branch tmp does-not-exist" &&
-	test_must_fail "git stash branch tmp does-not-exist@{0}" &&
+	test_must_fail git stash drop does-not-exist &&
+	test_must_fail git stash drop does-not-exist@{0} &&
+	test_must_fail git stash pop does-not-exist &&
+	test_must_fail git stash pop does-not-exist@{0} &&
+	test_must_fail git stash apply does-not-exist &&
+	test_must_fail git stash apply does-not-exist@{0} &&
+	test_must_fail git stash show does-not-exist &&
+	test_must_fail git stash show does-not-exist@{0} &&
+	test_must_fail git stash branch tmp does-not-exist &&
+	test_must_fail git stash branch tmp does-not-exist@{0} &&
 	git stash drop
 '
 
 test_expect_success 'invalid ref of the form stash@{n}, n >= N' '
 	git stash clear &&
-	test_must_fail "git stash drop stash@{0}" &&
+	test_must_fail git stash drop stash@{0} &&
 	echo bar5 > file &&
 	echo bar6 > file2 &&
 	git add file2 &&
 	git stash &&
-	test_must_fail "git drop stash@{1}" &&
-	test_must_fail "git pop stash@{1}" &&
-	test_must_fail "git apply stash@{1}" &&
-	test_must_fail "git show stash@{1}" &&
-	test_must_fail "git branch tmp stash@{1}" &&
+	test_must_fail git drop stash@{1} &&
+	test_must_fail git pop stash@{1} &&
+	test_must_fail git apply stash@{1} &&
+	test_must_fail git show stash@{1} &&
+	test_must_fail git branch tmp stash@{1} &&
 	git stash drop
 '
 
-- 
1.7.2.80.g22196.dirty

Re: [PATCH 0/2] fix quotes in maint-reflog-beyond-horizon and detached-stash

[Jeff King]

On Wed, Sep 01, 2010 at 12:49:18AM +1000, Jon Seymour wrote:

> In two recent series, I introduced tests that use test_must_fail incorrectly.
> 
> In effect, I was calling:
> 
>    test_must_fail "foo bar"
> 
> but this was failing because "foo bar" is not comamnd and not because
> the command "foo" fails with arguments "bar".
> 
> These two patches fix these problems.

I grepped around and I think you got all of the cases. I think we can
catch this in the test harness itself, though, with the following
series:

  [1/2]: tests: make test_must_fail more verbose
  [2/2]: tests: make test_must_fail fail on missing commands

With this series, I get (on pu):

  $ ./t1503-rev-parse-verify.sh -v -i
  [...]
  eval: 9: git rev-parse --verify master@{4}: not found
  test_must_fail: command not found: git rev-parse --verify master@{4}
  not ok - 7 master@{n} for various n

-Peff

[PATCH 1/2] tests: make test_must_fail more verbose

[Jeff King]

Because test_must_fail fails when a command succeeds, the
command frequently does not produce any output (since, after
all, it thought it was succeeding). So let's have
test_must_fail itself report that a problem occurred.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3a3d4c4..285bfd8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -591,7 +591,15 @@ test_path_is_missing () {
 
 test_must_fail () {
 	"$@"
-	test $? -gt 0 -a $? -le 129 -o $? -gt 192
+	exit_code=$?
+	if test $exit_code = 0; then
+		echo >&2 "test_must_fail: command succeeded: $*"
+		return 1
+	elif test $exit_code -gt 129 -a $exit_code -le 192; then
+		echo >&2 "test_must_fail: died by signal: $*"
+		return 1
+	fi
+	return 0
 }
 
 # Similar to test_must_fail, but tolerates success, too.  This is
-- 
1.7.2.2.119.gf9c33

[PATCH 2/2] tests: make test_must_fail fail on missing commands

[Jeff King]

The point of it is to run a command that produces failure. A
missing command is more likely an error in the test script
(e.g., using 'test_must_fail "command with arguments"', or
relying on a missing command).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 285bfd8..dbb13af 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -598,6 +598,9 @@ test_must_fail () {
 	elif test $exit_code -gt 129 -a $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
 	fi
 	return 0
 }
-- 
1.7.2.2.119.gf9c33

Re: [PATCH 2/2] tests: make test_must_fail fail on missing commands

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The point of it is to run a command that produces failure. A
> missing command is more likely an error in the test script
> (e.g., using 'test_must_fail "command with arguments"', or
> relying on a missing command).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/test-lib.sh |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 285bfd8..dbb13af 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -598,6 +598,9 @@ test_must_fail () {
>  	elif test $exit_code -gt 129 -a $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
>  	fi

Hmm.  One worry is that if we ever exit(127) ourselves this would be
confused, but hopefully we are not that clueless.

Re: [PATCH 0/2] fix quotes in maint-reflog-beyond-horizon and detached-stash

[Junio C Hamano]

Jon Seymour <jon.seymour@gmail.com> writes:

> In two recent series, I introduced tests that use test_must_fail incorrectly.
>
> In effect, I was calling:
>
>    test_must_fail "foo bar"
>
> but this was failing because "foo bar" is not comamnd and not because
> the command "foo" fails with arguments "bar".

Blush; sorry for not noticing them earlier, and thanks for the fix.

Re: [PATCH 1/2] tests: make test_must_fail more verbose

[Jonathan Nieder]

Jeff King wrote:

> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -591,7 +591,15 @@ test_path_is_missing () {
>  
>  test_must_fail () {
>  	"$@"
> -	test $? -gt 0 -a $? -le 129 -o $? -gt 192
> +	exit_code=$?
> +	if test $exit_code = 0; then
> +		echo >&2 "test_must_fail: command succeeded: $*"
> +		return 1
> +	elif test $exit_code -gt 129 -a $exit_code -le 192; then
> +		echo >&2 "test_must_fail: died by signal: $*"
> +		return 1
> +	fi
> +	return 0
>  }

Can the exit status (e.g. from a shell function) be negative?

Though your patch does not affect this, a command interrupted by a
signal will receive exit status > 192 in shells like ksh93.  Posix
says:

	As explained in other sections, certain exit status values
	have been reserved for special uses and should be used by
	applications only for those purposes:

	126
		A file to be executed was found, but it was not an
		executable utility.
	127
		A utility to be executed was not found.
	>128
		A command was interrupted by a signal.

Unfortunately that does not agree with git usage.

	129
		Incorrect command-line usage, or help requested
		(rather than SIGHUP).
	255
		Failed to create pipe for child process, fork
		failed, execvp failed, wait failed, invalid
		pathspec for add --patch, "git archive" or
		"git daemon" failure (rather than signal 127).

Here's a test_might_fail patch for consistency.

-- 8< --
Subject: tests: make test_might_fail more verbose

Let test_might_fail say something about its failures for consistency
with test_must_fail.

Cc: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib.sh |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 285bfd8..506787c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -615,7 +615,12 @@ test_must_fail () {
 
 test_might_fail () {
 	"$@"
-	test $? -ge 0 -a $? -le 129 -o $? -gt 192
+	exit_code=$?
+	if test $exit_code -gt 129 -a $exit_code -le 192; then
+		echo >&2 "test_might_fail: died by signal: $*"
+		return 1
+	fi
+	return 0
 }
 
 # test_cmp is a helper function to compare actual and expected output.
-- 
1.7.2.2

Re: [PATCH 2/2] tests: make test_must_fail fail on missing commands

[Jeff King]

On Tue, Aug 31, 2010 at 09:58:09AM -0700, Junio C Hamano wrote:

> > +	elif test $exit_code = 127; then
> > +		echo >&2 "test_must_fail: command not found: $*"
> > +		return 1
> >  	fi
> 
> Hmm.  One worry is that if we ever exit(127) ourselves this would be
> confused, but hopefully we are not that clueless.

Yeah, I considered that but dismissed it. I don't think we are that
clueless, and even if we were, then I think test_must_fail is doing a
good thing by pointing it out to us.

-Peff

Re: [PATCH 2/2] tests: make test_must_fail fail on missing commands

[Jonathan Nieder]

Jeff King wrote:

> The point of it is to run a command that produces failure. A
> missing command is more likely an error in the test script

Makes sense.  Here's the corresponding change for test_might_fail.

-- 8< --
Subject: tests: make test_might_fail fail on missing commands

Detect and report hard-to-notice spelling mistakes like

 test_might_fail "git config --unset whatever"

(the extra quotes prevent the shell from running git as intended;
instead, the shell looks for a "git config --unset whatever" file).

Cc: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 16ceb53..7da490d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -622,6 +622,9 @@ test_might_fail () {
 	if test $exit_code -gt 129 -a $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
 }
-- 
1.7.2.2

Re: [PATCH 1/2] tests: make test_must_fail more verbose

[Jeff King]

On Tue, Aug 31, 2010 at 12:10:55PM -0500, Jonathan Nieder wrote:

> > -	test $? -gt 0 -a $? -le 129 -o $? -gt 192
> [...]
> Can the exit status (e.g. from a shell function) be negative?

I don't believe so. Both bash and dash complain about "return -1" (and
POSIX says the number must be "an unsigned decimal integer"). More
importantly, though, is how shells interpret an exit status from a
command that has high bits set. Both bash and dash truncate it to 8-bits
and treat it as unsigned. I would think any reasonable shell would do
that (no, I don't have a Solaris 8 box handy to test on. :) ).

> Though your patch does not affect this, a command interrupted by a
> signal will receive exit status > 192 in shells like ksh93.

Yeah, that is from 5a26973 (t/test-lib.sh: exit with small negagive int
is ok with test_must_fail, 2008-07-12). I'm not sure I agree with the
intent of that commit. We should perhaps just eliminate all the places
where we use negative exit codes.

> Posix says:
> 	As explained in other sections, certain exit status values
> 	have been reserved for special uses and should be used by
> 	applications only for those purposes:
> 
> 	126
> 		A file to be executed was found, but it was not an
> 		executable utility.
> 	127
> 		A utility to be executed was not found.
> 	>128
> 		A command was interrupted by a signal.

Yup, that's what I have always assumed to be the case.

> Unfortunately that does not agree with git usage.
> 
> 	129
> 		Incorrect command-line usage, or help requested
> 		(rather than SIGHUP).

Yeah, we might consider changing that. We indeed confuse it with SIGHUP:

  $ git log -h 2>/dev/null; echo $?
  129
  $ git log -p >/dev/null; echo $? [now killall -HUP git elsewhere]
  129

We also use 128 for our generic die. I don't know if this is ever
problematic. It has the high bit set like a signal, but you don't
actually check the bits in the shell. And the actual representation at
the C level is not the same.

So part of me is in favor of changing these to something more "normal".
I am slightly worried, though, about breaking some caller. It is a
user-visible interface, though I suspect 99% of callers only ever cared
whether it was 0 or not.

> Here's a test_might_fail patch for consistency.

I didn't even know about that one. :)

They should definitely be kept in sync. I wonder if it is worth
implemening must_fail in terms of might_fail.

>  test_might_fail () {
>  	"$@"
> -	test $? -ge 0 -a $? -le 129 -o $? -gt 192
> +	exit_code=$?
> +	if test $exit_code -gt 129 -a $exit_code -le 192; then
> +		echo >&2 "test_might_fail: died by signal: $*"
> +		return 1
> +	fi
> +	return 0

I wonder if 2/2 would need the "missing command" bit, too. Hrm.

-Peff

Re: [PATCH 2/2] tests: make test_must_fail fail on missing commands

[Jeff King]

On Tue, Aug 31, 2010 at 12:26:57PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > The point of it is to run a command that produces failure. A
> > missing command is more likely an error in the test script
> 
> Makes sense.  Here's the corresponding change for test_might_fail.

I think this is probably worth doing. Unless somebody is doing something
silly like:

  test_might_fail command_that_might_exist

But that seems a pretty contrived scenario (I am imagining something
like "call sync now, but if we don't have it, don't fail". But in the
test scripts that seems unlikely).

-Peff

Re: [PATCH 2/2] tests: make test_must_fail fail on missing commands

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 31, 2010 at 18:08, Jeff King <peff@peff.net> wrote:
> On Tue, Aug 31, 2010 at 12:26:57PM -0500, Jonathan Nieder wrote:
>
>> Jeff King wrote:
>>
>> > The point of it is to run a command that produces failure. A
>> > missing command is more likely an error in the test script
>>
>> Makes sense.  Here's the corresponding change for test_might_fail.
>
> I think this is probably worth doing. Unless somebody is doing something
> silly like:
>
>  test_might_fail command_that_might_exist
>
> But that seems a pretty contrived scenario (I am imagining something
> like "call sync now, but if we don't have it, don't fail". But in the
> test scripts that seems unlikely).

Yes, test_must_fail is only for git commands, not external stuff like
sync, although some naughty people have gone and used it for grep/test
in a few places instead of using "!" it seems.

Re: [PATCH 1/2] tests: make test_must_fail more verbose

[Jonathan Nieder]

Jeff King wrote:
> On Tue, Aug 31, 2010 at 12:10:55PM -0500, Jonathan Nieder wrote:

>> Can the exit status (e.g. from a shell function) be negative?
>
> I don't believe so. Both bash and dash complain about "return -1" (and
> POSIX says the number must be "an unsigned decimal integer").

Thanks for checking; missed that.  It is clearer in POSIX that the
exit status from an external command will be truncated to an 8-bit
unsigned integer, which as you say is the most important thing.

Re: [PATCH 1/2] tests: make test_must_fail more verbose

[Jon Seymour]

On Wed, Sep 1, 2010 at 1:56 AM, Jeff King <peff@peff.net> wrote:
> Because test_must_fail fails when a command succeeds, the
> command frequently does not produce any output (since, after
> all, it thought it was succeeding). So let's have
> test_must_fail itself report that a problem occurred.
>
> Signed-off-by: Jeff King <peff@peff.net>

Jeff,

Nice fix - thank you!

It is nice to see that my initial sloppiness inspired a thoughtful
remediation. I'll try not to rely of gitters being always so helpful
:-)

jon.

> ---
>  t/test-lib.sh |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 3a3d4c4..285bfd8 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -591,7 +591,15 @@ test_path_is_missing () {
>
>  test_must_fail () {
>        "$@"
> -       test $? -gt 0 -a $? -le 129 -o $? -gt 192
> +       exit_code=$?
> +       if test $exit_code = 0; then
> +               echo >&2 "test_must_fail: command succeeded: $*"
> +               return 1
> +       elif test $exit_code -gt 129 -a $exit_code -le 192; then
> +               echo >&2 "test_must_fail: died by signal: $*"
> +               return 1
> +       fi
> +       return 0
>  }
>
>  # Similar to test_must_fail, but tolerates success, too.  This is
> --
> 1.7.2.2.119.gf9c33
>
>

Re: [PATCH 1/2] tests: make test_must_fail more verbose

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 1, 2010 at 03:37, Jon Seymour <jon.seymour@gmail.com> wrote:
> On Wed, Sep 1, 2010 at 1:56 AM, Jeff King <peff@peff.net> wrote:
>> Because test_must_fail fails when a command succeeds, the
>> command frequently does not produce any output (since, after
>> all, it thought it was succeeding). So let's have
>> test_must_fail itself report that a problem occurred.
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>
> Jeff,
>
> Nice fix - thank you!
>
> It is nice to see that my initial sloppiness inspired a thoughtful
> remediation. I'll try not to rely of gitters being always so helpful
> :-)

FWIW I plan to make this sort of thing better for everything when I
pick up my "WIP: Report intra-test progress with TAP subtests" series
again. I.e. use >&5 instead of >&2 and turn this sort of thing into
subtests.

But if you're interested in picking up someone else's slack it could
use some help :) I don't know when I'll get around to fixing it up:
http://github.com/avar/git/commit/e2ac35a8e49ceec98ca512bf106ce04c93c84b5c