reftable/record: don't `BUG()` in `reftable_record_cmp()`

The reftable library aborts with a bug in case `reftable_record_cmp()`
is invoked with two records of differing types. This would cause the
program to die without the caller being able to handle the error, which
is not something we want in the context of library code. And it ties us
to the Git codebase.

Refactor the code such that `reftable_record_cmp()` returns an error
code separate from the actual comparison result. This requires us to
also adapt some callers up the callchain in a similar fashion.

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

diff --git a/reftable/merged.c b/reftable/merged.c
index 4156eec07fc904ffac76db1433985dd0fc348507..563864068c1882779261597bf4528fe338e690c7 100644 (file)
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -66,8 +66,11 @@ static int merged_iter_seek(struct merged_iter *mi, struct reftable_record *want
        int err;
 
        mi->advance_index = -1;
-       while (!merged_iter_pqueue_is_empty(mi->pq))
-               merged_iter_pqueue_remove(&mi->pq);
+       while (!merged_iter_pqueue_is_empty(mi->pq)) {
+               err = merged_iter_pqueue_remove(&mi->pq, NULL);
+               if (err < 0)
+                       return err;
+       }
 
        for (size_t i = 0; i < mi->subiters_len; i++) {
                err = iterator_seek(&mi->subiters[i].iter, want);
@@ -120,7 +123,9 @@ static int merged_iter_next_entry(struct merged_iter *mi,
        if (empty)
                return 1;
 
-       entry = merged_iter_pqueue_remove(&mi->pq);
+       err = merged_iter_pqueue_remove(&mi->pq, &entry);
+       if (err < 0)
+               return err;
 
        /*
          One can also use reftable as datacenter-local storage, where the ref
@@ -134,11 +139,16 @@ static int merged_iter_next_entry(struct merged_iter *mi,
                struct pq_entry top = merged_iter_pqueue_top(mi->pq);
                int cmp;
 
-               cmp = reftable_record_cmp(top.rec, entry.rec);
+               err = reftable_record_cmp(top.rec, entry.rec, &cmp);
+               if (err < 0)
+                       return err;
                if (cmp > 0)
                        break;
 
-               merged_iter_pqueue_remove(&mi->pq);
+               err = merged_iter_pqueue_remove(&mi->pq, NULL);
+               if (err < 0)
+                       return err;
+
                err = merged_iter_advance_subiter(mi, top.index);
                if (err < 0)
                        return err;

diff --git a/reftable/pq.c b/reftable/pq.c
index 5591e875e1e845a595e2dbcc3689098abc2f518f..ef8035cfd9b6925caaebb5033074cb38e5a42d77 100644 (file)
--- a/reftable/pq.c
+++ b/reftable/pq.c
@@ -15,13 +15,18 @@ https://developers.google.com/open-source/licenses/bsd
 
 int pq_less(struct pq_entry *a, struct pq_entry *b)
 {
-       int cmp = reftable_record_cmp(a->rec, b->rec);
+       int cmp, err;
+
+       err = reftable_record_cmp(a->rec, b->rec, &cmp);
+       if (err < 0)
+               return err;
+
        if (cmp == 0)
                return a->index > b->index;
        return cmp < 0;
 }
 
-struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)
+int merged_iter_pqueue_remove(struct merged_iter_pqueue *pq, struct pq_entry *out)
 {
        size_t i = 0;
        struct pq_entry e = pq->heap[0];
@@ -32,17 +37,34 @@ struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)
                size_t min = i;
                size_t j = 2 * i + 1;
                size_t k = 2 * i + 2;
-               if (j < pq->len && pq_less(&pq->heap[j], &pq->heap[i]))
-                       min = j;
-               if (k < pq->len && pq_less(&pq->heap[k], &pq->heap[min]))
-                       min = k;
+               int cmp;
+
+               if (j < pq->len) {
+                       cmp = pq_less(&pq->heap[j], &pq->heap[i]);
+                       if (cmp < 0)
+                               return -1;
+                       else if (cmp)
+                               min = j;
+               }
+
+               if (k < pq->len) {
+                       cmp = pq_less(&pq->heap[k], &pq->heap[min]);
+                       if (cmp < 0)
+                               return -1;
+                       else if (cmp)
+                               min = k;
+               }
+
                if (min == i)
                        break;
                SWAP(pq->heap[i], pq->heap[min]);
                i = min;
        }
 
-       return e;
+       if (out)
+               *out = e;
+
+       return 0;
 }
 
 int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry *e)

diff --git a/reftable/pq.h b/reftable/pq.h
index 83c062eecac9f2ffe46d8fd7485775f2a84b3a54..ff39016445b3f71373c47373510c37587b9dace8 100644 (file)
--- a/reftable/pq.h
+++ b/reftable/pq.h
@@ -22,7 +22,7 @@ struct merged_iter_pqueue {
        size_t cap;
 };
 
-struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq);
+int merged_iter_pqueue_remove(struct merged_iter_pqueue *pq, struct pq_entry *out);
 int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry *e);
 void merged_iter_pqueue_release(struct merged_iter_pqueue *pq);
 int pq_less(struct pq_entry *a, struct pq_entry *b);

diff --git a/reftable/record.c b/reftable/record.c
index 1e18f8dffb1300a86a5854933d2c600b5a491c33..b39d99fcc75a66b55de225699b416ffa3381c4ec 100644 (file)
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -1195,12 +1195,14 @@ int reftable_record_is_deletion(struct reftable_record *rec)
                reftable_record_data(rec));
 }
 
-int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b)
+int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b,
+                       int *cmp)
 {
        if (a->type != b->type)
-               BUG("cannot compare reftable records of different type");
-       return reftable_record_vtable(a)->cmp(
-               reftable_record_data(a), reftable_record_data(b));
+               return -1;
+       *cmp = reftable_record_vtable(a)->cmp(reftable_record_data(a),
+                                             reftable_record_data(b));
+       return 0;
 }
 
 int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, uint32_t hash_size)

diff --git a/reftable/record.h b/reftable/record.h
index e1846c294ba0bae1ce65054449d93c15f68e74f5..867810a9328218e2f58188a6038245cfab8f329a 100644 (file)
--- a/reftable/record.h
+++ b/reftable/record.h
@@ -134,7 +134,7 @@ struct reftable_record {
 int reftable_record_init(struct reftable_record *rec, uint8_t typ);
 
 /* see struct record_vtable */
-int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b);
+int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b, int *cmp);
 int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, uint32_t hash_size);
 int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest);
 int reftable_record_copy_from(struct reftable_record *rec,

diff --git a/t/unit-tests/t-reftable-pq.c b/t/unit-tests/t-reftable-pq.c
index d8a4c283a11701111fa01853d4d4ec60cd8c958b..c128fe8616a6041b9d5a2b30df91f3f00c892308 100644 (file)
--- a/t/unit-tests/t-reftable-pq.c
+++ b/t/unit-tests/t-reftable-pq.c
@@ -21,7 +21,9 @@ static void merged_iter_pqueue_check(const struct merged_iter_pqueue *pq)
 
 static int pq_entry_equal(struct pq_entry *a, struct pq_entry *b)
 {
-       return !reftable_record_cmp(a->rec, b->rec) && (a->index == b->index);
+       int cmp;
+       check(!reftable_record_cmp(a->rec, b->rec, &cmp));
+       return !cmp && (a->index == b->index);
 }
 
 static void t_pq_record(void)
@@ -49,7 +51,9 @@ static void t_pq_record(void)
 
        while (!merged_iter_pqueue_is_empty(pq)) {
                struct pq_entry top = merged_iter_pqueue_top(pq);
-               struct pq_entry e = merged_iter_pqueue_remove(&pq);
+               struct pq_entry e;
+
+               check(!merged_iter_pqueue_remove(&pq, &e));
                merged_iter_pqueue_check(&pq);
 
                check(pq_entry_equal(&top, &e));
@@ -90,7 +94,9 @@ static void t_pq_index(void)
 
        for (i = N - 1; i > 0; i--) {
                struct pq_entry top = merged_iter_pqueue_top(pq);
-               struct pq_entry e = merged_iter_pqueue_remove(&pq);
+               struct pq_entry e;
+
+               check(!merged_iter_pqueue_remove(&pq, &e));
                merged_iter_pqueue_check(&pq);
 
                check(pq_entry_equal(&top, &e));
@@ -129,7 +135,9 @@ static void t_merged_iter_pqueue_top(void)
 
        for (i = N - 1; i > 0; i--) {
                struct pq_entry top = merged_iter_pqueue_top(pq);
-               struct pq_entry e = merged_iter_pqueue_remove(&pq);
+               struct pq_entry e;
+
+               check(!merged_iter_pqueue_remove(&pq, &e));
 
                merged_iter_pqueue_check(&pq);
                check(pq_entry_equal(&top, &e));

diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
index 6540bd20e37f1437b19cfc902cf65d1d0042b196..595496637364a11b1337da39a430ade0518f36e0 100644 (file)
--- a/t/unit-tests/t-reftable-record.c
+++ b/t/unit-tests/t-reftable-record.c
@@ -100,16 +100,20 @@ static void t_reftable_ref_record_comparison(void)
                        .u.ref.value.symref = (char *) "refs/heads/master",
                },
        };
+       int cmp;
 
        check(!reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
-       check(!reftable_record_cmp(&in[0], &in[1]));
+       check(!reftable_record_cmp(&in[0], &in[1], &cmp));
+       check(!cmp);
 
        check(!reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1));
-       check_int(reftable_record_cmp(&in[1], &in[2]), >, 0);
+       check(!reftable_record_cmp(&in[1], &in[2], &cmp));
+       check_int(cmp, >, 0);
 
        in[1].u.ref.value_type = in[0].u.ref.value_type;
        check(reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
-       check(!reftable_record_cmp(&in[0], &in[1]));
+       check(!reftable_record_cmp(&in[0], &in[1], &cmp));
+       check(!cmp);
 }
 
 static void t_reftable_ref_record_compare_name(void)
@@ -209,17 +213,20 @@ static void t_reftable_log_record_comparison(void)
                        .u.log.update_index = 22,
                },
        };
+       int cmp;
 
        check(!reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
        check(!reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1));
-       check_int(reftable_record_cmp(&in[1], &in[2]), >, 0);
+       check(!reftable_record_cmp(&in[1], &in[2], &cmp));
+       check_int(cmp, >, 0);
        /* comparison should be reversed for equal keys, because
         * comparison is now performed on the basis of update indices */
-       check_int(reftable_record_cmp(&in[0], &in[1]), <, 0);
+       check(!reftable_record_cmp(&in[0], &in[1], &cmp));
+       check_int(cmp, <, 0);
 
        in[1].u.log.update_index = in[0].u.log.update_index;
        check(reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
-       check(!reftable_record_cmp(&in[0], &in[1]));
+       check(!reftable_record_cmp(&in[0], &in[1], &cmp));
 }
 
 static void t_reftable_log_record_compare_key(void)
@@ -396,16 +403,20 @@ static void t_reftable_obj_record_comparison(void)
                        .u.obj.hash_prefix_len = 5,
                },
        };
+       int cmp;
 
        check(!reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
-       check(!reftable_record_cmp(&in[0], &in[1]));
+       check(!reftable_record_cmp(&in[0], &in[1], &cmp));
+       check(!cmp);
 
        check(!reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1));
-       check_int(reftable_record_cmp(&in[1], &in[2]), >, 0);
+       check(!reftable_record_cmp(&in[1], &in[2], &cmp));
+       check_int(cmp, >, 0);
 
        in[1].u.obj.offset_len = in[0].u.obj.offset_len;
        check(reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
-       check(!reftable_record_cmp(&in[0], &in[1]));
+       check(!reftable_record_cmp(&in[0], &in[1], &cmp));
+       check(!cmp);
 }
 
 static void t_reftable_obj_record_roundtrip(void)
@@ -486,19 +497,24 @@ static void t_reftable_index_record_comparison(void)
                        .u.idx.last_key = REFTABLE_BUF_INIT,
                },
        };
+       int cmp;
+
        check(!reftable_buf_addstr(&in[0].u.idx.last_key, "refs/heads/master"));
        check(!reftable_buf_addstr(&in[1].u.idx.last_key, "refs/heads/master"));
        check(!reftable_buf_addstr(&in[2].u.idx.last_key, "refs/heads/branch"));
 
        check(!reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
-       check(!reftable_record_cmp(&in[0], &in[1]));
+       check(!reftable_record_cmp(&in[0], &in[1], &cmp));
+       check(!cmp);
 
        check(!reftable_record_equal(&in[1], &in[2], REFTABLE_HASH_SIZE_SHA1));
-       check_int(reftable_record_cmp(&in[1], &in[2]), >, 0);
+       check(!reftable_record_cmp(&in[1], &in[2], &cmp));
+       check_int(cmp, >, 0);
 
        in[1].u.idx.offset = in[0].u.idx.offset;
        check(reftable_record_equal(&in[0], &in[1], REFTABLE_HASH_SIZE_SHA1));
-       check(!reftable_record_cmp(&in[0], &in[1]));
+       check(!reftable_record_cmp(&in[0], &in[1], &cmp));
+       check(!cmp);
 
        for (size_t i = 0; i < ARRAY_SIZE(in); i++)
                reftable_record_release(&in[i]);