reftable/tree: handle allocation failures

The tree interfaces of the reftable library handle both insertion and
searching of tree nodes with a single function, where the behaviour is
altered between the two via an `insert` bit. This makes it quit awkward
to handle allocation failures because on inserting we'd have to check
for `NULL` pointers and return an error, whereas on searching entries we
don't have to handle it as an allocation error.

Split up concerns of this function into two separate functions, one for
inserting entries and one for searching entries. This makes it easy for
us to check for allocation errors as `tree_insert()` should never return
a `NULL` pointer now. Adapt callers accordingly.

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

diff --git a/reftable/tree.c b/reftable/tree.c
index 5ffb2e0d690cadfc54dbd731a406b782ac759e02..f4dbe720901e143795b31df54b09f56ebc5350d6 100644 (file)
--- a/reftable/tree.c
+++ b/reftable/tree.c
@@ -11,28 +11,44 @@ https://developers.google.com/open-source/licenses/bsd
 
 #include "basics.h"
 
-struct tree_node *tree_search(void *key, struct tree_node **rootp,
-                             int (*compare)(const void *, const void *),
-                             int insert)
+struct tree_node *tree_search(struct tree_node *tree,
+                             void *key,
+                             int (*compare)(const void *, const void *))
 {
        int res;
+       if (!tree)
+               return NULL;
+       res = compare(key, tree->key);
+       if (res < 0)
+               return tree_search(tree->left, key, compare);
+       else if (res > 0)
+               return tree_search(tree->right, key, compare);
+       return tree;
+}
+
+struct tree_node *tree_insert(struct tree_node **rootp,
+                             void *key,
+                             int (*compare)(const void *, const void *))
+{
+       int res;
+
        if (!*rootp) {
-               if (!insert) {
+               struct tree_node *n;
+
+               REFTABLE_CALLOC_ARRAY(n, 1);
+               if (!n)
                        return NULL;
-               } else {
-                       struct tree_node *n;
-                       REFTABLE_CALLOC_ARRAY(n, 1);
-                       n->key = key;
-                       *rootp = n;
-                       return *rootp;
-               }
+
+               n->key = key;
+               *rootp = n;
+               return *rootp;
        }
 
        res = compare(key, (*rootp)->key);
        if (res < 0)
-               return tree_search(key, &(*rootp)->left, compare, insert);
+               return tree_insert(&(*rootp)->left, key, compare);
        else if (res > 0)
-               return tree_search(key, &(*rootp)->right, compare, insert);
+               return tree_insert(&(*rootp)->right, key, compare);
        return *rootp;
 }
 

diff --git a/reftable/tree.h b/reftable/tree.h
index fbdd002e23afcf61054b77cb2053e4d298e941bb..9604453b6d541a2a7f9679e11ac1b0ed09a1e94f 100644 (file)
--- a/reftable/tree.h
+++ b/reftable/tree.h
@@ -15,12 +15,23 @@ struct tree_node {
        struct tree_node *left, *right;
 };
 
-/* looks for `key` in `rootp` using `compare` as comparison function. If insert
- * is set, insert the key if it's not found. Else, return NULL.
+/*
+ * Search the tree for the node matching the given key using `compare` as
+ * comparison function. Returns the node whose key matches or `NULL` in case
+ * the key does not exist in the tree.
+ */
+struct tree_node *tree_search(struct tree_node *tree,
+                             void *key,
+                             int (*compare)(const void *, const void *));
+
+/*
+ * Insert a node into the tree. Returns the newly inserted node if the key does
+ * not yet exist. Otherwise it returns the preexisting node. Returns `NULL`
+ * when allocating the new node fails.
  */
-struct tree_node *tree_search(void *key, struct tree_node **rootp,
-                             int (*compare)(const void *, const void *),
-                             int insert);
+struct tree_node *tree_insert(struct tree_node **rootp,
+                             void *key,
+                             int (*compare)(const void *, const void *));
 
 /* performs an infix walk of the tree. */
 void infix_walk(struct tree_node *t, void (*action)(void *arg, void *key),

diff --git a/reftable/writer.c b/reftable/writer.c
index 791e246337487d78b2b58dfe6a8d624447671ebc..e180c108403e0d2d511492a00f70b4ccaa684bb1 100644 (file)
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -208,8 +208,7 @@ static int writer_index_hash(struct reftable_writer *w, struct strbuf *hash)
        struct obj_index_tree_node *key;
        struct tree_node *node;
 
-       node = tree_search(&want, &w->obj_index_tree,
-                          &obj_index_tree_node_compare, 0);
+       node = tree_search(w->obj_index_tree, &want, &obj_index_tree_node_compare);
        if (!node) {
                struct obj_index_tree_node empty = OBJ_INDEX_TREE_NODE_INIT;
 
@@ -221,8 +220,8 @@ static int writer_index_hash(struct reftable_writer *w, struct strbuf *hash)
 
                strbuf_reset(&key->hash);
                strbuf_addbuf(&key->hash, hash);
-               tree_search((void *)key, &w->obj_index_tree,
-                           &obj_index_tree_node_compare, 1);
+               tree_insert(&w->obj_index_tree, key,
+                           &obj_index_tree_node_compare);
        } else {
                key = node->key;
        }

diff --git a/t/unit-tests/t-reftable-tree.c b/t/unit-tests/t-reftable-tree.c
index 700479d34b109683177b9d98dacb32ad63f9e945..79b175a45a7aefecd588ca222408cafafd2bba4b 100644 (file)
--- a/t/unit-tests/t-reftable-tree.c
+++ b/t/unit-tests/t-reftable-tree.c
@@ -37,16 +37,17 @@ static void t_tree_search(void)
         * values[1] and values[10] (inclusive) in the tree.
         */
        do {
-               nodes[i] = tree_search(&values[i], &root, &t_compare, 1);
+               nodes[i] = tree_insert(&root, &values[i], &t_compare);
+               check(nodes[i] != NULL);
                i = (i * 7) % 11;
        } while (i != 1);
 
        for (i = 1; i < ARRAY_SIZE(nodes); i++) {
                check_pointer_eq(&values[i], nodes[i]->key);
-               check_pointer_eq(nodes[i], tree_search(&values[i], &root, &t_compare, 0));
+               check_pointer_eq(nodes[i], tree_search(root, &values[i], &t_compare));
        }
 
-       check(!tree_search(values, &root, t_compare, 0));
+       check(!tree_search(root, values, t_compare));
        tree_free(root);
 }
 
@@ -62,7 +63,8 @@ static void t_infix_walk(void)
        size_t count = 0;
 
        do {
-               tree_search(&values[i], &root, t_compare, 1);
+               struct tree_node *node = tree_insert(&root, &values[i], t_compare);
+               check(node != NULL);
                i = (i * 7) % 11;
                count++;
        } while (i != 1);