[PATCH] Fix builtin checkout crashing when given an invalid path

[PATCH] Fix builtin checkout crashing when given an invalid path

[Alex Riesen]

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Noticed in t2008, which actually passed, but silently removed
core-files (I saw segfaults in syslog) and did not properly check the
exit code.  The change for the t2008 comes as seperate patch, but it
should be noted that "! command" is *not* how you check for a command
to have failed. It could have crashed.

 builtin-checkout.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 4a4bb8b..9579ff4 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -545,6 +545,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 
 	if (argc) {
 		const char **pathspec = get_pathspec(prefix, argv);
+
+		if (!pathspec)
+			die("invalid path specification");
+
 		/* Checkout paths */
 		if (opts.new_branch || opts.force || opts.merge) {
 			if (argc == 1) {
-- 
1.5.4.3.253.g9f1d5

[PATCH] Expect the exit code of builtin checkout to be in portable range

[Alex Riesen]

This allows crashes to be noticed at least in bash and dash, which put
the signal which terminated the command in its exit status.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Alex Riesen, Thu, Feb 28, 2008 17:30:47 +0100:
> Noticed in t2008, which actually passed, but silently removed
> core-files (I saw segfaults in syslog) and did not properly check the
> exit code.  The change for the t2008 comes as seperate patch, but it
> should be noted that "! command" is *not* how you check for a command
> to have failed. It could have crashed.

So we'd better check the exit status in failure tests.
Like this, for instance

 t/t2008-checkout-subdir.sh |   28 ++++++++++++++++++++--------
 1 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh
index 4a723dc..1295a09 100755
--- a/t/t2008-checkout-subdir.sh
+++ b/t/t2008-checkout-subdir.sh
@@ -67,16 +67,28 @@ test_expect_success 'checkout with simple prefix' '
 
 '
 
-test_expect_success 'relative path outside tree should fail' \
-	'! git checkout HEAD -- ../../Makefile'
+test_expect_success 'relative path outside tree should fail' '
+	git checkout HEAD -- ../../Makefile
+	test $? -gt 0 -a $? -le 128
+'
 
-test_expect_success 'incorrect relative path to file should fail (1)' \
-	'! git checkout HEAD -- ../file0'
+test_expect_success 'incorrect relative path to file should fail (1)' '
+	git checkout HEAD -- ../file0
+	test $? -gt 0 -a $? -le 128
+'
 
-test_expect_success 'incorrect relative path should fail (2)' \
-	'( cd dir1 && ! git checkout HEAD -- ./file0 )'
+test_expect_success 'incorrect relative path should fail (2)' '
+	( cd dir1 && {
+	    git checkout HEAD -- ./file0
+	    test $? -gt 0 -a $? -le 128
+	} )
+'
 
-test_expect_success 'incorrect relative path should fail (3)' \
-	'( cd dir1 && ! git checkout HEAD -- ../../file0 )'
+test_expect_success 'incorrect relative path should fail (3)' '
+	( cd dir1 && {
+	    git checkout HEAD -- ../../file0
+	    test $? -gt 0 -a $? -le 128
+	} )
+'
 
 test_done
-- 
1.5.4.3.253.g9f1d5

Re: [PATCH] Expect the exit code of builtin checkout to be in portable range

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> This allows crashes to be noticed at least in bash and dash, which put
> the signal which terminated the command in its exit status.
>
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
>
> Alex Riesen, Thu, Feb 28, 2008 17:30:47 +0100:
>> Noticed in t2008, which actually passed, but silently removed
>> core-files (I saw segfaults in syslog) and did not properly check the
>> exit code.  The change for the t2008 comes as seperate patch, but it
>> should be noted that "! command" is *not* how you check for a command
>> to have failed. It could have crashed.
>
> So we'd better check the exit status in failure tests.
> Like this, for instance

Yeah, very well spotted, although I would have liked it if this
were caught before it hit 'master'.

I think we want a helper shell function to fix potentially many
other uses of "! git-command" construct, so how about doing it
this way?

---
 t/t2008-checkout-subdir.sh |    8 ++++----
 t/test-lib.sh              |   17 +++++++++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh
index 4a723dc..3e098ab 100755
--- a/t/t2008-checkout-subdir.sh
+++ b/t/t2008-checkout-subdir.sh
@@ -68,15 +68,15 @@ test_expect_success 'checkout with simple prefix' '
 '
 
 test_expect_success 'relative path outside tree should fail' \
-	'! git checkout HEAD -- ../../Makefile'
+	'test_must_fail git checkout HEAD -- ../../Makefile'
 
 test_expect_success 'incorrect relative path to file should fail (1)' \
-	'! git checkout HEAD -- ../file0'
+	'test_must_fail git checkout HEAD -- ../file0'
 
 test_expect_success 'incorrect relative path should fail (2)' \
-	'( cd dir1 && ! git checkout HEAD -- ./file0 )'
+	'( cd dir1 && test_must_fail git checkout HEAD -- ./file0 )'
 
 test_expect_success 'incorrect relative path should fail (3)' \
-	'( cd dir1 && ! git checkout HEAD -- ../../file0 )'
+	'( cd dir1 && test_must_fail git checkout HEAD -- ../../file0 )'
 
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 83889c4..90df619 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -270,6 +270,23 @@ test_expect_code () {
 	echo >&3 ""
 }
 
+# This is not among top-level (test_expect_success | test_expect_failure)
+# but is a prefix that can be used in the test script, like:
+#
+#	test_expect_success 'complain and die' '
+#           do something &&
+#           do something else &&
+#	    test_must_fail git checkout ../outerspace
+#	'
+#
+# Writing this as "! git checkout ../outerspace" is wrong, because
+# the failure could be due to a segv.  We want a controlled failure.
+
+test_must_fail () {
+	"$@"
+	test $? -gt 0 -a $? -le 128
+}
+
 # Most tests can use the created repository, but some may need to create more.
 # Usage: test_create_repo <directory>
 test_create_repo () {