cache-tree: refactor verification to return error codes

The function `cache_tree_verify()` will `BUG()` whenever it finds that
the cache-tree extension of the index is corrupt. The function is thus
inherently untestable because the resulting call to `abort()` will be
detected by our testing framework and labelled an error. And rightfully
so: it shouldn't ever be possible to hit bugs, as they should indicate a
programming error rather than corruption of on-disk state.

Refactor the function to instead return error codes. This also ensures
that the function can be used e.g. by git-fsck(1) without the whole
process dying. Furthermore, this refactoring plugs some memory leaks
when returning early by creating a common exit path.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/cache-tree.c b/cache-tree.c
index 50610c3f3cb8e7d3c7b84a513286590201d6f99a..4228b6fad486eb6c8f822876128c35999e158f0b 100644 (file)
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -2,6 +2,7 @@
 
 #include "git-compat-util.h"
 #include "environment.h"
+#include "gettext.h"
 #include "hex.h"
 #include "lockfile.h"
 #include "tree.h"
@@ -864,15 +865,15 @@ int cache_tree_matches_traversal(struct cache_tree *root,
        return 0;
 }
 
-static void verify_one_sparse(struct index_state *istate,
-                             struct strbuf *path,
-                             int pos)
+static int verify_one_sparse(struct index_state *istate,
+                            struct strbuf *path,
+                            int pos)
 {
        struct cache_entry *ce = istate->cache[pos];
-
        if (!S_ISSPARSEDIR(ce->ce_mode))
-               BUG("directory '%s' is present in index, but not sparse",
-                   path->buf);
+               return error(_("directory '%s' is present in index, but not sparse"),
+                            path->buf);
+       return 0;
 }
 
 /*
@@ -881,6 +882,7 @@ static void verify_one_sparse(struct index_state *istate,
  *  1 - Restart verification - a call to ensure_full_index() freed the cache
  *      tree that is being verified and verification needs to be restarted from
  *      the new toplevel cache tree.
+ *  -1 - Verification failed.
  */
 static int verify_one(struct repository *r,
                      struct index_state *istate,
@@ -890,18 +892,23 @@ static int verify_one(struct repository *r,
        int i, pos, len = path->len;
        struct strbuf tree_buf = STRBUF_INIT;
        struct object_id new_oid;
+       int ret;
 
        for (i = 0; i < it->subtree_nr; i++) {
                strbuf_addf(path, "%s/", it->down[i]->name);
-               if (verify_one(r, istate, it->down[i]->cache_tree, path))
-                       return 1;
+               ret = verify_one(r, istate, it->down[i]->cache_tree, path);
+               if (ret)
+                       goto out;
+
                strbuf_setlen(path, len);
        }
 
        if (it->entry_count < 0 ||
            /* no verification on tests (t7003) that replace trees */
-           lookup_replace_object(r, &it->oid) != &it->oid)
-               return 0;
+           lookup_replace_object(r, &it->oid) != &it->oid) {
+               ret = 0;
+               goto out;
+       }
 
        if (path->len) {
                /*
@@ -911,12 +918,14 @@ static int verify_one(struct repository *r,
                 */
                int is_sparse = istate->sparse_index;
                pos = index_name_pos(istate, path->buf, path->len);
-               if (is_sparse && !istate->sparse_index)
-                       return 1;
+               if (is_sparse && !istate->sparse_index) {
+                       ret = 1;
+                       goto out;
+               }
 
                if (pos >= 0) {
-                       verify_one_sparse(istate, path, pos);
-                       return 0;
+                       ret = verify_one_sparse(istate, path, pos);
+                       goto out;
                }
 
                pos = -pos - 1;
@@ -934,16 +943,23 @@ static int verify_one(struct repository *r,
                unsigned mode;
                int entlen;
 
-               if (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE))
-                       BUG("%s with flags 0x%x should not be in cache-tree",
-                           ce->name, ce->ce_flags);
+               if (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE)) {
+                       ret = error(_("%s with flags 0x%x should not be in cache-tree"),
+                                   ce->name, ce->ce_flags);
+                       goto out;
+               }
+
                name = ce->name + path->len;
                slash = strchr(name, '/');
                if (slash) {
                        entlen = slash - name;
+
                        sub = find_subtree(it, ce->name + path->len, entlen, 0);
-                       if (!sub || sub->cache_tree->entry_count < 0)
-                               BUG("bad subtree '%.*s'", entlen, name);
+                       if (!sub || sub->cache_tree->entry_count < 0) {
+                               ret = error(_("bad subtree '%.*s'"), entlen, name);
+                               goto out;
+                       }
+
                        oid = &sub->cache_tree->oid;
                        mode = S_IFDIR;
                        i += sub->cache_tree->entry_count;
@@ -956,27 +972,50 @@ static int verify_one(struct repository *r,
                strbuf_addf(&tree_buf, "%o %.*s%c", mode, entlen, name, '\0');
                strbuf_add(&tree_buf, oid->hash, r->hash_algo->rawsz);
        }
+
        hash_object_file(r->hash_algo, tree_buf.buf, tree_buf.len, OBJ_TREE,
                         &new_oid);
-       if (!oideq(&new_oid, &it->oid))
-               BUG("cache-tree for path %.*s does not match. "
-                   "Expected %s got %s", len, path->buf,
-                   oid_to_hex(&new_oid), oid_to_hex(&it->oid));
+
+       if (!oideq(&new_oid, &it->oid)) {
+               ret = error(_("cache-tree for path %.*s does not match. "
+                             "Expected %s got %s"), len, path->buf,
+                           oid_to_hex(&new_oid), oid_to_hex(&it->oid));
+               goto out;
+       }
+
+       ret = 0;
+out:
        strbuf_setlen(path, len);
        strbuf_release(&tree_buf);
-       return 0;
+       return ret;
 }
 
-void cache_tree_verify(struct repository *r, struct index_state *istate)
+int cache_tree_verify(struct repository *r, struct index_state *istate)
 {
        struct strbuf path = STRBUF_INIT;
+       int ret;
 
-       if (!istate->cache_tree)
-               return;
-       if (verify_one(r, istate, istate->cache_tree, &path)) {
+       if (!istate->cache_tree) {
+               ret = 0;
+               goto out;
+       }
+
+       ret = verify_one(r, istate, istate->cache_tree, &path);
+       if (ret < 0)
+               goto out;
+       if (ret > 0) {
                strbuf_reset(&path);
-               if (verify_one(r, istate, istate->cache_tree, &path))
+
+               ret = verify_one(r, istate, istate->cache_tree, &path);
+               if (ret < 0)
+                       goto out;
+               if (ret > 0)
                        BUG("ensure_full_index() called twice while verifying cache tree");
        }
+
+       ret = 0;
+
+out:
        strbuf_release(&path);
+       return ret;
 }

diff --git a/cache-tree.h b/cache-tree.h
index faae88be63c2ce012c700af3a88e02e165982a4e..b82c4963e7c805ffd03adbb96407f5e39a0a8d4c 100644 (file)
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -33,7 +33,7 @@ struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);
 
 int cache_tree_fully_valid(struct cache_tree *);
 int cache_tree_update(struct index_state *, int);
-void cache_tree_verify(struct repository *, struct index_state *);
+int cache_tree_verify(struct repository *, struct index_state *);
 
 /* bitmasks to write_index_as_tree flags */
 #define WRITE_TREE_MISSING_OK 1

diff --git a/read-cache.c b/read-cache.c
index 4e67dc182e7467708f3d6195b642523b0f9a5d01..d72a3266b6f14db0e27d84ffbf70deca48110615 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -3331,8 +3331,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
        int new_shared_index, ret, test_split_index_env;
        struct split_index *si = istate->split_index;
 
-       if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0))
-               cache_tree_verify(the_repository, istate);
+       if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0) &&
+           cache_tree_verify(the_repository, istate) < 0)
+               return -1;
 
        if ((flags & SKIP_IF_UNCHANGED) && !istate->cache_changed) {
                if (flags & COMMIT_LOCK)

diff --git a/unpack-trees.c b/unpack-trees.c
index 9a55cb62040c9c243a228e6681320fbe1f4b62ee..21cc197d4714d3686797fb76f01914b8b53fd8c4 100644 (file)
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2070,9 +2070,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
        if (o->dst_index) {
                move_index_extensions(&o->internal.result, o->src_index);
                if (!ret) {
-                       if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0))
-                               cache_tree_verify(the_repository,
-                                                 &o->internal.result);
+                       if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0) &&
+                           cache_tree_verify(the_repository,
+                                             &o->internal.result) < 0) {
+                               ret = -1;
+                               goto done;
+                       }
+
                        if (!o->skip_cache_tree_update &&
                            !cache_tree_fully_valid(o->internal.result.cache_tree))
                                cache_tree_update(&o->internal.result,