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>
Patrick Steinhardt committed
Oct 7, 2024 at 06:38 UTC
9f119599a69eb11f0712cab3bdbc2000eb91abd7
4 files changed
+79
-35
cache-tree.c
+68
-29
@@ -2,6 +2,7 @@
2
3
#include "git-compat-util.h"
4
#include "environment.h"
5
+#include "gettext.h"
6
#include "hex.h"
7
#include "lockfile.h"
8
#include "tree.h"
@@ -864,15 +865,15 @@ int cache_tree_matches_traversal(struct cache_tree *root,
865
return 0;
866
}
867
867
-static void verify_one_sparse(struct index_state *istate,
868
- struct strbuf *path,
869
- int pos)
868
+static int verify_one_sparse(struct index_state *istate,
869
+ struct strbuf *path,
870
+ int pos)
871
{
872
struct cache_entry *ce = istate->cache[pos];
872
-
873
if (!S_ISSPARSEDIR(ce->ce_mode))
874
- BUG("directory '%s' is present in index, but not sparse",
875
- path->buf);
874
+ return error(_("directory '%s' is present in index, but not sparse"),
875
+ path->buf);
876
+ return 0;
877
}
878
879
/*
@@ -881,6 +882,7 @@ static void verify_one_sparse(struct index_state *istate,
882
* 1 - Restart verification - a call to ensure_full_index() freed the cache
883
* tree that is being verified and verification needs to be restarted from
884
* the new toplevel cache tree.
885
+ * -1 - Verification failed.
886
*/
887
static int verify_one(struct repository *r,
888
struct index_state *istate,
@@ -890,18 +892,23 @@ static int verify_one(struct repository *r,
892
int i, pos, len = path->len;
893
struct strbuf tree_buf = STRBUF_INIT;
894
struct object_id new_oid;
895
+ int ret;
896
897
for (i = 0; i < it->subtree_nr; i++) {
898
strbuf_addf(path, "%s/", it->down[i]->name);
896
- if (verify_one(r, istate, it->down[i]->cache_tree, path))
897
- return 1;
899
+ ret = verify_one(r, istate, it->down[i]->cache_tree, path);
900
+ if (ret)
901
+ goto out;
902
+
903
strbuf_setlen(path, len);
904
}
905
906
if (it->entry_count < 0 ||
907
/* no verification on tests (t7003) that replace trees */
903
- lookup_replace_object(r, &it->oid) != &it->oid)
904
- return 0;
908
+ lookup_replace_object(r, &it->oid) != &it->oid) {
909
+ ret = 0;
910
+ goto out;
911
+ }
912
913
if (path->len) {
914
/*
@@ -911,12 +918,14 @@ static int verify_one(struct repository *r,
918
*/
919
int is_sparse = istate->sparse_index;
920
pos = index_name_pos(istate, path->buf, path->len);
914
- if (is_sparse && !istate->sparse_index)
915
- return 1;
921
+ if (is_sparse && !istate->sparse_index) {
922
+ ret = 1;
923
+ goto out;
924
+ }
925
926
if (pos >= 0) {
918
- verify_one_sparse(istate, path, pos);
919
- return 0;
927
+ ret = verify_one_sparse(istate, path, pos);
928
+ goto out;
929
}
930
931
pos = -pos - 1;
@@ -934,16 +943,23 @@ static int verify_one(struct repository *r,
943
unsigned mode;
944
int entlen;
945
937
- if (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE))
938
- BUG("%s with flags 0x%x should not be in cache-tree",
939
- ce->name, ce->ce_flags);
946
+ if (ce->ce_flags & (CE_STAGEMASK | CE_INTENT_TO_ADD | CE_REMOVE)) {
947
+ ret = error(_("%s with flags 0x%x should not be in cache-tree"),
948
+ ce->name, ce->ce_flags);
949
+ goto out;
950
+ }
951
+
952
name = ce->name + path->len;
953
slash = strchr(name, '/');
954
if (slash) {
955
entlen = slash - name;
956
+
957
sub = find_subtree(it, ce->name + path->len, entlen, 0);
945
- if (!sub || sub->cache_tree->entry_count < 0)
946
- BUG("bad subtree '%.*s'", entlen, name);
958
+ if (!sub || sub->cache_tree->entry_count < 0) {
959
+ ret = error(_("bad subtree '%.*s'"), entlen, name);
960
+ goto out;
961
+ }
962
+
963
oid = &sub->cache_tree->oid;
964
mode = S_IFDIR;
965
i += sub->cache_tree->entry_count;
@@ -956,27 +972,50 @@ static int verify_one(struct repository *r,
972
strbuf_addf(&tree_buf, "%o %.*s%c", mode, entlen, name, '\0');
973
strbuf_add(&tree_buf, oid->hash, r->hash_algo->rawsz);
974
}
975
+
976
hash_object_file(r->hash_algo, tree_buf.buf, tree_buf.len, OBJ_TREE,
977
&new_oid);
961
- if (!oideq(&new_oid, &it->oid))
962
- BUG("cache-tree for path %.*s does not match. "
963
- "Expected %s got %s", len, path->buf,
964
- oid_to_hex(&new_oid), oid_to_hex(&it->oid));
978
+
979
+ if (!oideq(&new_oid, &it->oid)) {
980
+ ret = error(_("cache-tree for path %.*s does not match. "
981
+ "Expected %s got %s"), len, path->buf,
982
+ oid_to_hex(&new_oid), oid_to_hex(&it->oid));
983
+ goto out;
984
+ }
985
+
986
+ ret = 0;
987
+out:
988
strbuf_setlen(path, len);
989
strbuf_release(&tree_buf);
967
- return 0;
990
+ return ret;
991
}
992
970
-void cache_tree_verify(struct repository *r, struct index_state *istate)
993
+int cache_tree_verify(struct repository *r, struct index_state *istate)
994
{
995
struct strbuf path = STRBUF_INIT;
996
+ int ret;
997
974
- if (!istate->cache_tree)
975
- return;
976
- if (verify_one(r, istate, istate->cache_tree, &path)) {
998
+ if (!istate->cache_tree) {
999
+ ret = 0;
1000
+ goto out;
1001
+ }
1002
+
1003
+ ret = verify_one(r, istate, istate->cache_tree, &path);
1004
+ if (ret < 0)
1005
+ goto out;
1006
+ if (ret > 0) {
1007
strbuf_reset(&path);
978
- if (verify_one(r, istate, istate->cache_tree, &path))
1008
+
1009
+ ret = verify_one(r, istate, istate->cache_tree, &path);
1010
+ if (ret < 0)
1011
+ goto out;
1012
+ if (ret > 0)
1013
BUG("ensure_full_index() called twice while verifying cache tree");
1014
}
1015
+
1016
+ ret = 0;
1017
+
1018
+out:
1019
strbuf_release(&path);
1020
+ return ret;
1021
}
cache-tree.h
+1
-1
@@ -33,7 +33,7 @@ struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);
33
34
int cache_tree_fully_valid(struct cache_tree *);
35
int cache_tree_update(struct index_state *, int);
36
-void cache_tree_verify(struct repository *, struct index_state *);
36
+int cache_tree_verify(struct repository *, struct index_state *);
37
38
/* bitmasks to write_index_as_tree flags */
39
#define WRITE_TREE_MISSING_OK 1
read-cache.c
+3
-2
@@ -3331,8 +3331,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
3331
int new_shared_index, ret, test_split_index_env;
3332
struct split_index *si = istate->split_index;
3333
3334
- if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0))
3335
- cache_tree_verify(the_repository, istate);
3334
+ if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0) &&
3335
+ cache_tree_verify(the_repository, istate) < 0)
3336
+ return -1;
3337
3338
if ((flags & SKIP_IF_UNCHANGED) && !istate->cache_changed) {
3339
if (flags & COMMIT_LOCK)
unpack-trees.c
+7
-3
@@ -2070,9 +2070,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
2070
if (o->dst_index) {
2071
move_index_extensions(&o->internal.result, o->src_index);
2072
if (!ret) {
2073
- if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0))
2074
- cache_tree_verify(the_repository,
2075
- &o->internal.result);
2073
+ if (git_env_bool("GIT_TEST_CHECK_CACHE_TREE", 0) &&
2074
+ cache_tree_verify(the_repository,
2075
+ &o->internal.result) < 0) {
2076
+ ret = -1;
2077
+ goto done;
2078
+ }
2079
+
2080
if (!o->skip_cache_tree_update &&
2081
!cache_tree_fully_valid(o->internal.result.cache_tree))
2082
cache_tree_update(&o->internal.result,