diff mbox series

[v6,12/15] reftable: implement record equality generically

Message ID 6385e449ba714e3ce41f10276fc60e9757519492.1642691534.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit c983374035bcba0f70d8908d735d17dfef4e0edf
Headers show
Series Reftable coverity fixes | expand

Commit Message

Han-Wen Nienhuys Jan. 20, 2022, 3:12 p.m. UTC
From: Han-Wen Nienhuys <hanwen@google.com>

This simplifies unittests a little, and provides further coverage for
reftable_record_copy().

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 reftable/record.c      | 57 +++++++++++++++++++++++++++++++++++++++++-
 reftable/record.h      |  5 +++-
 reftable/record_test.c | 23 +++--------------
 3 files changed, 63 insertions(+), 22 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Jan. 21, 2022, 11:52 a.m. UTC | #1
On Thu, Jan 20 2022, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> This simplifies unittests a little, and provides further coverage for
> reftable_record_copy().
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  reftable/record.c      | 57 +++++++++++++++++++++++++++++++++++++++++-
>  reftable/record.h      |  5 +++-
>  reftable/record_test.c | 23 +++--------------
>  3 files changed, 63 insertions(+), 22 deletions(-)
>
> diff --git a/reftable/record.c b/reftable/record.c
> index f7c77c51539..2a9e41a992e 100644
> --- a/reftable/record.c
> +++ b/reftable/record.c
> @@ -430,6 +430,15 @@ static int reftable_ref_record_is_deletion_void(const void *p)
>  		(const struct reftable_ref_record *)p);
>  }
>  
> +

stray extra newline being added here.

> +static int reftable_ref_record_equal_void(const void *a,
> +					  const void *b, int hash_size)
> +{
> +	struct reftable_ref_record *ra = (struct reftable_ref_record *) a;
> +	struct reftable_ref_record *rb = (struct reftable_ref_record *) b;
> +	return reftable_ref_record_equal(ra, rb, hash_size);
> +}
> +
>  static struct reftable_record_vtable reftable_ref_record_vtable = {
>  	.key = &reftable_ref_record_key,
>  	.type = BLOCK_TYPE_REF,
> @@ -439,6 +448,7 @@ static struct reftable_record_vtable reftable_ref_record_vtable = {
>  	.decode = &reftable_ref_record_decode,
>  	.release = &reftable_ref_record_release_void,
>  	.is_deletion = &reftable_ref_record_is_deletion_void,
> +	.equal = &reftable_ref_record_equal_void,
>  };
>  
>  static void reftable_obj_record_key(const void *r, struct strbuf *dest)
> @@ -572,6 +582,25 @@ static int not_a_deletion(const void *p)
>  	return 0;
>  }
>  
> +static int reftable_obj_record_equal_void(const void *a, const void *b, int hash_size)
> +{
> +	struct reftable_obj_record *ra = (struct reftable_obj_record *) a;
> +	struct reftable_obj_record *rb = (struct reftable_obj_record *) b;
> +
> +	if (ra->hash_prefix_len != rb->hash_prefix_len
> +	    || ra->offset_len != rb->offset_len)
> +		return 0;
> +
> +	if (ra->hash_prefix_len &&
> +	    memcmp(ra->hash_prefix, rb->hash_prefix, ra->hash_prefix_len))
> +		return 0;

Similar to the memcpy() paranoia isn't this memcmp() paranoia?
I.e. memcmp() returns 0 on a n=0, so we can lose the
"ra->hash_prefix_len &&" here, no?

> +	if (ra->offset_len &&
> +	    memcmp(ra->offsets, rb->offsets, ra->offset_len * sizeof(uint64_t)))
> +		return 0;

...and here, since 0 * sizeof() will just get us zero.
diff mbox series

Patch

diff --git a/reftable/record.c b/reftable/record.c
index f7c77c51539..2a9e41a992e 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -430,6 +430,15 @@  static int reftable_ref_record_is_deletion_void(const void *p)
 		(const struct reftable_ref_record *)p);
 }
 
+
+static int reftable_ref_record_equal_void(const void *a,
+					  const void *b, int hash_size)
+{
+	struct reftable_ref_record *ra = (struct reftable_ref_record *) a;
+	struct reftable_ref_record *rb = (struct reftable_ref_record *) b;
+	return reftable_ref_record_equal(ra, rb, hash_size);
+}
+
 static struct reftable_record_vtable reftable_ref_record_vtable = {
 	.key = &reftable_ref_record_key,
 	.type = BLOCK_TYPE_REF,
@@ -439,6 +448,7 @@  static struct reftable_record_vtable reftable_ref_record_vtable = {
 	.decode = &reftable_ref_record_decode,
 	.release = &reftable_ref_record_release_void,
 	.is_deletion = &reftable_ref_record_is_deletion_void,
+	.equal = &reftable_ref_record_equal_void,
 };
 
 static void reftable_obj_record_key(const void *r, struct strbuf *dest)
@@ -572,6 +582,25 @@  static int not_a_deletion(const void *p)
 	return 0;
 }
 
+static int reftable_obj_record_equal_void(const void *a, const void *b, int hash_size)
+{
+	struct reftable_obj_record *ra = (struct reftable_obj_record *) a;
+	struct reftable_obj_record *rb = (struct reftable_obj_record *) b;
+
+	if (ra->hash_prefix_len != rb->hash_prefix_len
+	    || ra->offset_len != rb->offset_len)
+		return 0;
+
+	if (ra->hash_prefix_len &&
+	    memcmp(ra->hash_prefix, rb->hash_prefix, ra->hash_prefix_len))
+		return 0;
+	if (ra->offset_len &&
+	    memcmp(ra->offsets, rb->offsets, ra->offset_len * sizeof(uint64_t)))
+		return 0;
+
+	return 1;
+}
+
 static struct reftable_record_vtable reftable_obj_record_vtable = {
 	.key = &reftable_obj_record_key,
 	.type = BLOCK_TYPE_OBJ,
@@ -580,7 +609,8 @@  static struct reftable_record_vtable reftable_obj_record_vtable = {
 	.encode = &reftable_obj_record_encode,
 	.decode = &reftable_obj_record_decode,
 	.release = &reftable_obj_record_release,
-	.is_deletion = not_a_deletion,
+	.is_deletion = &not_a_deletion,
+	.equal = &reftable_obj_record_equal_void,
 };
 
 void reftable_log_record_print(struct reftable_log_record *log,
@@ -881,6 +911,14 @@  static int zero_hash_eq(uint8_t *a, uint8_t *b, int sz)
 	return !memcmp(a, b, sz);
 }
 
+static int reftable_log_record_equal_void(const void *a,
+					  const void *b, int hash_size)
+{
+	return reftable_log_record_equal((struct reftable_log_record *) a,
+					 (struct reftable_log_record *) b,
+					 hash_size);
+}
+
 int reftable_log_record_equal(const struct reftable_log_record *a,
 			      const struct reftable_log_record *b, int hash_size)
 {
@@ -924,6 +962,7 @@  static struct reftable_record_vtable reftable_log_record_vtable = {
 	.decode = &reftable_log_record_decode,
 	.release = &reftable_log_record_release_void,
 	.is_deletion = &reftable_log_record_is_deletion_void,
+	.equal = &reftable_log_record_equal_void
 };
 
 struct reftable_record reftable_new_record(uint8_t typ)
@@ -1042,6 +1081,14 @@  static int reftable_index_record_decode(void *rec, struct strbuf key,
 	return start.len - in.len;
 }
 
+static int reftable_index_record_equal(const void *a, const void *b, int hash_size)
+{
+	struct reftable_index_record *ia = (struct reftable_index_record *) a;
+	struct reftable_index_record *ib = (struct reftable_index_record *) b;
+
+	return ia->offset == ib->offset && !strbuf_cmp(&ia->last_key, &ib->last_key);
+}
+
 static struct reftable_record_vtable reftable_index_record_vtable = {
 	.key = &reftable_index_record_key,
 	.type = BLOCK_TYPE_INDEX,
@@ -1051,6 +1098,7 @@  static struct reftable_record_vtable reftable_index_record_vtable = {
 	.decode = &reftable_index_record_decode,
 	.release = &reftable_index_record_release,
 	.is_deletion = &not_a_deletion,
+	.equal = &reftable_index_record_equal,
 };
 
 void reftable_record_key(struct reftable_record *rec, struct strbuf *dest)
@@ -1098,6 +1146,13 @@  int reftable_record_is_deletion(struct reftable_record *rec)
 	return rec->ops->is_deletion(rec->data);
 }
 
+int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size)
+{
+	if (a->ops != b->ops)
+		return 0;
+	return a->ops->equal(a->data, b->data, hash_size);
+}
+
 void reftable_record_from_ref(struct reftable_record *rec,
 			      struct reftable_ref_record *ref_rec)
 {
diff --git a/reftable/record.h b/reftable/record.h
index 498e8c50bf4..da75d7d1f11 100644
--- a/reftable/record.h
+++ b/reftable/record.h
@@ -58,6 +58,9 @@  struct reftable_record_vtable {
 
 	/* is this a tombstone? */
 	int (*is_deletion)(const void *rec);
+
+	/* Are two records equal? This assumes they have the same type. Returns 0 for non-equal. */
+	int (*equal)(const void *a, const void *b, int hash_size);
 };
 
 /* record is a generic wrapper for different types of records. */
@@ -98,7 +101,7 @@  struct reftable_obj_record {
 };
 
 /* see struct record_vtable */
-
+int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size);
 void reftable_record_key(struct reftable_record *rec, struct strbuf *dest);
 uint8_t reftable_record_type(struct reftable_record *rec);
 void reftable_record_copy_from(struct reftable_record *rec,
diff --git a/reftable/record_test.c b/reftable/record_test.c
index f4ad7cace41..92680848156 100644
--- a/reftable/record_test.c
+++ b/reftable/record_test.c
@@ -21,18 +21,7 @@  static void test_copy(struct reftable_record *rec)
 	reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
 	/* do it twice to catch memory leaks */
 	reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
-	switch (reftable_record_type(&copy)) {
-	case BLOCK_TYPE_REF:
-		EXPECT(reftable_ref_record_equal(reftable_record_as_ref(&copy),
-						 reftable_record_as_ref(rec),
-						 GIT_SHA1_RAWSZ));
-		break;
-	case BLOCK_TYPE_LOG:
-		EXPECT(reftable_log_record_equal(reftable_record_as_log(&copy),
-						 reftable_record_as_log(rec),
-						 GIT_SHA1_RAWSZ));
-		break;
-	}
+	EXPECT(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
 	reftable_record_destroy(&copy);
 }
 
@@ -346,13 +335,7 @@  static void test_reftable_obj_record_roundtrip(void)
 					   GIT_SHA1_RAWSZ);
 		EXPECT(n == m);
 
-		EXPECT(in.hash_prefix_len == out.hash_prefix_len);
-		EXPECT(in.offset_len == out.offset_len);
-
-		EXPECT(!memcmp(in.hash_prefix, out.hash_prefix,
-			       in.hash_prefix_len));
-		EXPECT(0 == memcmp(in.offsets, out.offsets,
-				   sizeof(uint64_t) * in.offset_len));
+		EXPECT(reftable_record_equal(&rec, &rec_out, GIT_SHA1_RAWSZ));
 		strbuf_release(&key);
 		reftable_record_release(&rec_out);
 	}
@@ -390,7 +373,7 @@  static void test_reftable_index_record_roundtrip(void)
 	m = reftable_record_decode(&out_rec, key, extra, dest, GIT_SHA1_RAWSZ);
 	EXPECT(m == n);
 
-	EXPECT(in.offset == out.offset);
+	EXPECT(reftable_record_equal(&rec, &out_rec, GIT_SHA1_RAWSZ));
 
 	reftable_record_release(&out_rec);
 	strbuf_release(&key);