# [PATCH 0/2 v2] ls-tree: exit with non-zero status on error 7 messages from 2011-07-24 to 2011-07-24. Participants: Jon Seymour, Junio C Hamano, Jens Lehmann. Thread: https://gitlist.dev/t/27902 ## Jon Seymour, 2011-07-24 01:07 Subject: [PATCH 0/2 v2] ls-tree: exit with non-zero status on error Message-ID: <1311469670-15392-1-git-send-email-jon.seymour@gmail.com> URL: https://gitlist.dev/e/1311469670-15392-1-git-send-email-jon.seymour%40gmail.com ``` While working with a damaged repository, I noticed that git ls-tree was reporting an error even though it set a zero exit code. This patch uses the return code from read_tree_recursive instead. | v2: Amended test per Jens Lehmann's suggestion. Jon Seymour (2): Add a test to check that git ls-tree sets non-zero exit code on error. Ensure git ls-tree exits with a non-zero exit code if read_tree_recursive fails. builtin/ls-tree.c | 6 +++--- t/t3103-ls-tree-missing-tree.sh | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) create mode 100755 t/t3103-ls-tree-missing-tree.sh -- 1.7.6.347.g6a5a9c ``` ## Jon Seymour, 2011-07-24 01:07 Subject: [PATCH 1/2] Add a test to check that git ls-tree sets non-zero exit code on error. Message-ID: <1311469670-15392-2-git-send-email-jon.seymour@gmail.com> URL: https://gitlist.dev/e/1311469670-15392-2-git-send-email-jon.seymour%40gmail.com In-Reply-To: <1311469670-15392-1-git-send-email-jon.seymour@gmail.com> ``` Expected to fail at this commit, fixed by subsequent commit. Improved-by: Jens Lehmann Signed-off-by: Jon Seymour --- t/t3103-ls-tree-missing-tree.sh | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-) create mode 100755 t/t3103-ls-tree-missing-tree.sh diff --git a/t/t3103-ls-tree-missing-tree.sh b/t/t3103-ls-tree-missing-tree.sh new file mode 100755 index 0000000..cd17fa7 --- /dev/null +++ b/t/t3103-ls-tree-missing-tree.sh @@ -0,0 +1,19 @@ +#!/bin/sh + +test_description='ls-tree exits with non-zero status if it also reports an error' + +. ./test-lib.sh + +test_expect_success 'setup' ' + mkdir a && + touch a/one && + git add a/one && + git commit -m test +' + +test_expect_failure 'ls-tree fails with non-zero exit code on broken tree' ' + rm -f .git/objects/5f/cffbd6e4c5c5b8d81f5e9314b20e338e3ffff5 && + test_must_fail git ls-tree -r HEAD +' + +test_done -- 1.7.6.347.g96e0b ``` ## Jon Seymour, 2011-07-24 01:07 Subject: [PATCH 2/2] Ensure git ls-tree exits with a non-zero exit code if read_tree_recursive fails. Message-ID: <1311469670-15392-3-git-send-email-jon.seymour@gmail.com> URL: https://gitlist.dev/e/1311469670-15392-3-git-send-email-jon.seymour%40gmail.com In-Reply-To: <1311469670-15392-1-git-send-email-jon.seymour@gmail.com> ``` In the case of a corrupt repository, git ls-tree may report an error but presently it exits with a code of 0. This change uses the return code of read_tree_recursive instead. Improved-by: Jens Lehmann Signed-off-by: Jon Seymour --- builtin/ls-tree.c | 6 +++--- t/t3103-ls-tree-missing-tree.sh | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index f08c5b0..6d6c992 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -120,7 +120,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) { unsigned char sha1[20]; struct tree *tree; - int i, full_tree = 0; + int i, full_tree = 0, err; const struct option ls_tree_options[] = { OPT_BIT('d', NULL, &ls_options, "only show trees", LS_TREE_ONLY), @@ -173,7 +173,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) tree = parse_tree_indirect(sha1); if (!tree) die("not a tree object"); - read_tree_recursive(tree, "", 0, 0, &pathspec, show_tree, NULL); + err = read_tree_recursive(tree, "", 0, 0, &pathspec, show_tree, NULL); - return 0; + return err; } diff --git a/t/t3103-ls-tree-missing-tree.sh b/t/t3103-ls-tree-missing-tree.sh index cd17fa7..365ac07 100755 --- a/t/t3103-ls-tree-missing-tree.sh +++ b/t/t3103-ls-tree-missing-tree.sh @@ -11,7 +11,7 @@ test_expect_success 'setup' ' git commit -m test ' -test_expect_failure 'ls-tree fails with non-zero exit code on broken tree' ' +test_expect_success 'ls-tree fails with non-zero exit code on broken tree' ' rm -f .git/objects/5f/cffbd6e4c5c5b8d81f5e9314b20e338e3ffff5 && test_must_fail git ls-tree -r HEAD ' -- 1.7.6.347.g96e0b ``` ## Junio C Hamano, 2011-07-24 07:45 Subject: Re: [PATCH 1/2] Add a test to check that git ls-tree sets non-zero exit code on error. Message-ID: <7vipqsw29v.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vipqsw29v.fsf%40alter.siamese.dyndns.org In-Reply-To: <1311469670-15392-2-git-send-email-jon.seymour@gmail.com> ``` Jon Seymour writes: > t/t3103-ls-tree-missing-tree.sh | 19 +++++++++++++++++++ > 1 files changed, 19 insertions(+), 0 deletions(-) > create mode 100755 t/t3103-ls-tree-missing-tree.sh I'd rather not waste a new test number for something trivial like this. Don't we already have a script that tests ls-tree? ``` ## Junio C Hamano, 2011-07-24 07:45 Subject: Re: [PATCH 2/2] Ensure git ls-tree exits with a non-zero exit code if read_tree_recursive fails. Message-ID: <7vei1gw28w.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vei1gw28w.fsf%40alter.siamese.dyndns.org In-Reply-To: <1311469670-15392-3-git-send-email-jon.seymour@gmail.com> ``` Jon Seymour writes: > In the case of a corrupt repository, git ls-tree may report an error but > presently it exits with a code of 0. > > This change uses the return code of read_tree_recursive instead. > > Improved-by: Jens Lehmann > Signed-off-by: Jon Seymour > --- > builtin/ls-tree.c | 6 +++--- > t/t3103-ls-tree-missing-tree.sh | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index f08c5b0..6d6c992 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -120,7 +120,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > { > unsigned char sha1[20]; > struct tree *tree; > - int i, full_tree = 0; > + int i, full_tree = 0, err; > const struct option ls_tree_options[] = { > OPT_BIT('d', NULL, &ls_options, "only show trees", > LS_TREE_ONLY), > @@ -173,7 +173,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > tree = parse_tree_indirect(sha1); > if (!tree) > die("not a tree object"); > - read_tree_recursive(tree, "", 0, 0, &pathspec, show_tree, NULL); > + err = read_tree_recursive(tree, "", 0, 0, &pathspec, show_tree, NULL); > > - return 0; > + return err; > } Makes sense. Thanks. ``` ## Jon Seymour, 2011-07-24 09:12 Subject: Re: [PATCH 1/2] Add a test to check that git ls-tree sets non-zero exit code on error. Message-ID: URL: https://gitlist.dev/e/CAH3AnrrH3YcvzxPo%3DgdXavUh-yMsXcZ6UND4j%3DuvvC7Cbnmqng%40mail.gmail.com In-Reply-To: <7vipqsw29v.fsf@alter.siamese.dyndns.org> ``` On Sun, Jul 24, 2011 at 5:45 PM, Junio C Hamano wrote: > Jon Seymour writes: > >>  t/t3103-ls-tree-missing-tree.sh |   19 +++++++++++++++++++ >>  1 files changed, 19 insertions(+), 0 deletions(-) >>  create mode 100755 t/t3103-ls-tree-missing-tree.sh > > I'd rather not waste a new test number for something trivial like this. > Don't we already have a script that tests ls-tree? > Happy to take suggestions about which other test I should modify to include this one and how that should be renamed (if at all). The other tests t3100 -> t3102 are all currently quite coherent and focused. Perhaps a rename &/or merge of existing tests is required? FYI: t31xx has quite a lot of head room (e.g. there are still 96 names available in that space) jon. ``` ## Jens Lehmann, 2011-07-24 13:42 Subject: Re: [PATCH 2/2] Ensure git ls-tree exits with a non-zero exit code if read_tree_recursive fails. Message-ID: <4E2C212E.8000404@web.de> URL: https://gitlist.dev/e/4E2C212E.8000404%40web.de In-Reply-To: <1311469670-15392-3-git-send-email-jon.seymour@gmail.com> ``` Am 24.07.2011 03:07, schrieb Jon Seymour: > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index f08c5b0..6d6c992 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -120,7 +120,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > { > unsigned char sha1[20]; > struct tree *tree; > - int i, full_tree = 0; > + int i, full_tree = 0, err; > const struct option ls_tree_options[] = { > OPT_BIT('d', NULL, &ls_options, "only show trees", > LS_TREE_ONLY), > @@ -173,7 +173,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > tree = parse_tree_indirect(sha1); > if (!tree) > die("not a tree object"); > - read_tree_recursive(tree, "", 0, 0, &pathspec, show_tree, NULL); > + err = read_tree_recursive(tree, "", 0, 0, &pathspec, show_tree, NULL); > > - return 0; > + return err; > } Nit: Is it really necessary to introduce a new variable "err" for that? Looks like a "return read_tree_recursive(...)" would suffice here ... ```