{"thread":{"id":"12365","subject":"[PATCH] Fix builtin checkout crashing when given an invalid path","startedAt":"2008-02-28T16:30:47Z","lastAt":"2008-02-28T20:52:48Z","messageCount":3,"participants":["Alex Riesen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"70278","messageId":"20080228163047.GA4069@steel.home","threadId":"12365","inReplyTo":null,"subject":"[PATCH] Fix builtin checkout crashing when given an invalid path","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-02-28T16:30:47Z","receivedAt":"2008-02-28T16:30:47Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Signed-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nNoticed in t2008, which actually passed, but silently removed\ncore-files (I saw segfaults in syslog) and did not properly check the\nexit code.  The change for the t2008 comes as seperate patch, but it\nshould be noted that \"! command\" is *not* how you check for a command\nto have failed. It could have crashed.\n\n builtin-checkout.c |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex 4a4bb8b..9579ff4 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -545,6 +545,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \n \tif (argc) {\n \t\tconst char **pathspec = get_pathspec(prefix, argv);\n+\n+\t\tif (!pathspec)\n+\t\t\tdie(\"invalid path specification\");\n+\n \t\t/* Checkout paths */\n \t\tif (opts.new_branch || opts.force || opts.merge) {\n \t\t\tif (argc == 1) {\n-- \n1.5.4.3.253.g9f1d5\n\n\n"},{"id":"70279","messageId":"20080228164953.GB4069@steel.home","threadId":"12365","inReplyTo":"20080228163047.GA4069@steel.home","subject":"[PATCH] Expect the exit code of builtin checkout to be in portable range","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-02-28T16:49:53Z","receivedAt":"2008-02-28T16:49:53Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"This allows crashes to be noticed at least in bash and dash, which put\nthe signal which terminated the command in its exit status.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nAlex Riesen, Thu, Feb 28, 2008 17:30:47 +0100:\n> Noticed in t2008, which actually passed, but silently removed\n> core-files (I saw segfaults in syslog) and did not properly check the\n> exit code.  The change for the t2008 comes as seperate patch, but it\n> should be noted that \"! command\" is *not* how you check for a command\n> to have failed. It could have crashed.\n\nSo we'd better check the exit status in failure tests.\nLike this, for instance\n\n t/t2008-checkout-subdir.sh |   28 ++++++++++++++++++++--------\n 1 files changed, 20 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh\nindex 4a723dc..1295a09 100755\n--- a/t/t2008-checkout-subdir.sh\n+++ b/t/t2008-checkout-subdir.sh\n@@ -67,16 +67,28 @@ test_expect_success 'checkout with simple prefix' '\n \n '\n \n-test_expect_success 'relative path outside tree should fail' \\\n-\t'! git checkout HEAD -- ../../Makefile'\n+test_expect_success 'relative path outside tree should fail' '\n+\tgit checkout HEAD -- ../../Makefile\n+\ttest $? -gt 0 -a $? -le 128\n+'\n \n-test_expect_success 'incorrect relative path to file should fail (1)' \\\n-\t'! git checkout HEAD -- ../file0'\n+test_expect_success 'incorrect relative path to file should fail (1)' '\n+\tgit checkout HEAD -- ../file0\n+\ttest $? -gt 0 -a $? -le 128\n+'\n \n-test_expect_success 'incorrect relative path should fail (2)' \\\n-\t'( cd dir1 && ! git checkout HEAD -- ./file0 )'\n+test_expect_success 'incorrect relative path should fail (2)' '\n+\t( cd dir1 && {\n+\t    git checkout HEAD -- ./file0\n+\t    test $? -gt 0 -a $? -le 128\n+\t} )\n+'\n \n-test_expect_success 'incorrect relative path should fail (3)' \\\n-\t'( cd dir1 && ! git checkout HEAD -- ../../file0 )'\n+test_expect_success 'incorrect relative path should fail (3)' '\n+\t( cd dir1 && {\n+\t    git checkout HEAD -- ../../file0\n+\t    test $? -gt 0 -a $? -le 128\n+\t} )\n+'\n \n test_done\n-- \n1.5.4.3.253.g9f1d5\n\n"},{"id":"70311","messageId":"7vk5kohkgv.fsf@gitster.siamese.dyndns.org","threadId":"12365","inReplyTo":"20080228164953.GB4069@steel.home","subject":"Re: [PATCH] Expect the exit code of builtin checkout to be in portable range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-28T20:52:48Z","receivedAt":"2008-02-28T20:52:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> This allows crashes to be noticed at least in bash and dash, which put\n> the signal which terminated the command in its exit status.\n>\n> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>\n> ---\n>\n> Alex Riesen, Thu, Feb 28, 2008 17:30:47 +0100:\n>> Noticed in t2008, which actually passed, but silently removed\n>> core-files (I saw segfaults in syslog) and did not properly check the\n>> exit code.  The change for the t2008 comes as seperate patch, but it\n>> should be noted that \"! command\" is *not* how you check for a command\n>> to have failed. It could have crashed.\n>\n> So we'd better check the exit status in failure tests.\n> Like this, for instance\n\nYeah, very well spotted, although I would have liked it if this\nwere caught before it hit 'master'.\n\nI think we want a helper shell function to fix potentially many\nother uses of \"! git-command\" construct, so how about doing it\nthis way?\n\n---\n t/t2008-checkout-subdir.sh |    8 ++++----\n t/test-lib.sh              |   17 +++++++++++++++++\n 2 files changed, 21 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t2008-checkout-subdir.sh b/t/t2008-checkout-subdir.sh\nindex 4a723dc..3e098ab 100755\n--- a/t/t2008-checkout-subdir.sh\n+++ b/t/t2008-checkout-subdir.sh\n@@ -68,15 +68,15 @@ test_expect_success 'checkout with simple prefix' '\n '\n \n test_expect_success 'relative path outside tree should fail' \\\n-\t'! git checkout HEAD -- ../../Makefile'\n+\t'test_must_fail git checkout HEAD -- ../../Makefile'\n \n test_expect_success 'incorrect relative path to file should fail (1)' \\\n-\t'! git checkout HEAD -- ../file0'\n+\t'test_must_fail git checkout HEAD -- ../file0'\n \n test_expect_success 'incorrect relative path should fail (2)' \\\n-\t'( cd dir1 && ! git checkout HEAD -- ./file0 )'\n+\t'( cd dir1 && test_must_fail git checkout HEAD -- ./file0 )'\n \n test_expect_success 'incorrect relative path should fail (3)' \\\n-\t'( cd dir1 && ! git checkout HEAD -- ../../file0 )'\n+\t'( cd dir1 && test_must_fail git checkout HEAD -- ../../file0 )'\n \n test_done\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 83889c4..90df619 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -270,6 +270,23 @@ test_expect_code () {\n \techo >&3 \"\"\n }\n \n+# This is not among top-level (test_expect_success | test_expect_failure)\n+# but is a prefix that can be used in the test script, like:\n+#\n+#\ttest_expect_success 'complain and die' '\n+#           do something &&\n+#           do something else &&\n+#\t    test_must_fail git checkout ../outerspace\n+#\t'\n+#\n+# Writing this as \"! git checkout ../outerspace\" is wrong, because\n+# the failure could be due to a segv.  We want a controlled failure.\n+\n+test_must_fail () {\n+\t\"$@\"\n+\ttest $? -gt 0 -a $? -le 128\n+}\n+\n # Most tests can use the created repository, but some may need to create more.\n # Usage: test_create_repo <directory>\n test_create_repo () {\n"}]}