commit-reach: die on contains walk errors

Without generation numbers, repo_is_descendant_of() can return -1 when
it cannot read commit ancestry. commit_contains() exposes that result
through a Boolean interface, so ref-filter treats it as true. This can
include a ref for --contains or exclude it for --no-contains without
failing the command.

Die when repo_is_descendant_of() reports an error. The memoized walk
already dies when it cannot parse a commit, so callers of the
non-memoized path no longer turn a failed walk into a match.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/commit-reach.c b/commit-reach.c
index 349ca68a3761b0e16b28a05d5eefd88b060ea6d5..4acf1c02d57ee298b060adfb3b0fac9496ed42c7 100644 (file)
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -854,10 +854,16 @@ static enum contains_result contains_tag_algo(struct commit *candidate,
 int commit_contains(struct ref_filter *filter, struct commit *commit,
                    struct commit_list *list, struct contains_cache *cache)
 {
+       int result;
+
        if (filter->with_commit_tag_algo ||
            generation_numbers_enabled(the_repository))
                return contains_tag_algo(commit, list, cache) == CONTAINS_YES;
-       return repo_is_descendant_of(the_repository, commit, list);
+
+       result = repo_is_descendant_of(the_repository, commit, list);
+       if (result < 0)
+               die(_("failed to check reachability"));
+       return result;
 }
 
 int can_all_from_reach_with_flag(struct object_array *from,

diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index e06feb06e91956bd61df6db6161eb1eae4911b9e..72b27c8be37d448468450620aad4c69a05e4708a 100755 (executable)
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh
@@ -52,6 +52,28 @@ test_expect_success 'Missing objects are reported correctly' '
        test_must_be_empty brief-err
 '
 
+test_expect_success 'missing ancestors are reported by contains filters' '
+       test_when_finished "git update-ref -d refs/heads/missing-parent" &&
+       {
+               echo "tree $(git rev-parse HEAD^{tree})" &&
+               echo "parent $MISSING" &&
+               git cat-file commit HEAD |
+                       sed -n -e "/^author /p" -e "/^committer /p" &&
+               echo &&
+               echo "missing parent"
+       } >commit &&
+       broken=$(git hash-object -t commit -w commit) &&
+       git update-ref refs/heads/missing-parent "$broken" &&
+       for option in --contains --no-contains
+       do
+               test_must_fail git for-each-ref "$option=HEAD" \
+                       refs/heads/missing-parent >out 2>err &&
+               test_must_be_empty out &&
+               test_grep "parse commit $MISSING" err ||
+               return 1
+       done
+'
+
 test_expect_success 'ahead-behind requires an argument' '
        test_must_fail git for-each-ref \
                --format="%(ahead-behind)" 2>err &&