diff mbox series

[v6,15/15] reftable: add print functions to the record types

Message ID 82f140cab5c30b59de534d773a5d00343e0868f9.1642691534.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 01033de49f26b75afd1e868d56c332c60b141faa
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 isn't used per se, but it is useful for debugging, especially
Windows CI failures.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 reftable/record.c      | 85 +++++++++++++++++++++++++++++++++++-------
 reftable/record.h      |  4 ++
 reftable/record_test.c | 21 ++++++++++-
 3 files changed, 95 insertions(+), 15 deletions(-)

Comments

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

> From: Han-Wen Nienhuys <hanwen@google.com>
> [...]
> +void reftable_ref_record_print(const struct reftable_ref_record *ref,
> +			       uint32_t hash_id) {

...here...

> +	reftable_ref_record_print_sz(ref, hash_size(hash_id));
> +}
> +
>  static void reftable_ref_record_release_void(void *rec)
>  {
>  	reftable_ref_record_release(rec);
> @@ -443,6 +448,12 @@ static int reftable_ref_record_equal_void(const void *a,
>  	return reftable_ref_record_equal(ra, rb, hash_size);
>  }
>  
> +static void reftable_ref_record_print_void(const void *rec,
> +					   int hash_size)

...and here you avoid a line longer than 79 characters....

> +{
> +	reftable_ref_record_print_sz((struct reftable_ref_record *) rec, hash_size);

...but not here...

> +		strbuf_addf(&offset_str, "%" PRIu64 " ", obj->offsets[i]);

"%"PRIu64 not "%" PRIu64, per the usual coding style....

> -		printf("log{%s(%" PRIu64 ") delete", log->refname,
> +		printf("log{%s(%" PRIu64 ") delete\n", log->refname,

...although I see we have some existing code here in reftable/ using the other style..

> +static void reftable_index_record_print(const void *rec, int hash_size)
> +{
> +	const struct reftable_index_record *idx = rec;
> +	/* TODO: escape null chars? */

That's quite scary as a TODO comment, can we think about whether we're
doing the wrong thing here? I.e. the printf() will stop on a \0, or
maybe I'm misunderstanding this...

>  	/* 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);
> +
> +	/* Print on stdout, for debugging. */

Inaccurate comment, this isn't for ad-hoc debugging, we're using this in
the reftable tests....

> +	void (*print)(const void *rec, int hash_size);
>  };
>  
>  /* returns true for recognized block types. Block start with the block type. */
> @@ -112,6 +115,7 @@ struct reftable_record {
>  
>  /* see struct record_vtable */
>  int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size);
> +void reftable_record_print(struct reftable_record *rec, 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 c6fdd1925a9..f91ea5e8830 100644
> --- a/reftable/record_test.c
> +++ b/reftable/record_test.c
> @@ -25,6 +25,10 @@ static void test_copy(struct reftable_record *rec)
>  	/* do it twice to catch memory leaks */
>  	reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
>  	EXPECT(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
> +
> +	puts("testing print coverage:\n");
> +	reftable_record_print(&copy, GIT_SHA1_RAWSZ);

...i.e. here.

> +
>  	reftable_record_release(&copy);
>  }
>  
> @@ -176,7 +180,8 @@ static void test_reftable_log_record_equal(void)
>  static void test_reftable_log_record_roundtrip(void)
>  {
>  	int i;
> -	struct reftable_log_record in[2] = {
> +

...more stray whitespace...

> +	struct reftable_log_record in[] = {
>  		{
>  			.refname = xstrdup("refs/heads/master"),
>  			.update_index = 42,
> @@ -197,10 +202,24 @@ static void test_reftable_log_record_roundtrip(void)
>  			.refname = xstrdup("refs/heads/master"),
>  			.update_index = 22,
>  			.value_type = REFTABLE_LOG_DELETION,
> +		},
> +		{
> +			.refname = xstrdup("branch"),
> +			.update_index = 33,
> +			.value_type = REFTABLE_LOG_UPDATE,
> +			.value = {
> +				.update = {
> +					.old_hash = reftable_malloc(GIT_SHA1_RAWSZ),
> +					.new_hash = reftable_malloc(GIT_SHA1_RAWSZ),
> +					/* rest of fields left empty. */

But in test_reftable_log_record_roundtrip() we have another existing
user who leaves out fields, but doesn't have such a comment.

I think it's better to just leave it out here, clearly it's intentional,
but if it's going to be added to anything surely the other user defining
5/7 fields is better than this user of 2/7 fields (i.e. the 5/7 is more
likely to be an unintentional omission), this one is clearly
intentional.
Han-Wen Nienhuys Jan. 24, 2022, 3:50 p.m. UTC | #2 
On Fri, Jan 21, 2022 at 1:40 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > +static void reftable_index_record_print(const void *rec, int hash_size)
> > +{
> > +     const struct reftable_index_record *idx = rec;
> > +     /* TODO: escape null chars? */
>
> That's quite scary as a TODO comment, can we think about whether we're
> doing the wrong thing here? I.e. the printf() will stop on a \0, or
> maybe I'm misunderstanding this...

It's a print function, so we'd print a truncated last_key. This can
happen if we were printing an index record that points to a log
record. I'd address this if and when I have to debug into indexing
going wrong (which hasn't happened so far.)


> >       /* 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);
> > +
> > +     /* Print on stdout, for debugging. */
>
> Inaccurate comment, this isn't for ad-hoc debugging, we're using this in
> the reftable tests....

The tests say

> > +     puts("testing print coverage:\n");
> > +     reftable_record_print(&copy, GIT_SHA1_RAWSZ);

ie. I'm calling the print functions to make sure they don't crash or
leak memory. They're not used as part of functionality of either the
reftable code or its tests.

> > +                                     .old_hash = reftable_malloc(GIT_SHA1_RAWSZ),
> > +                                     .new_hash = reftable_malloc(GIT_SHA1_RAWSZ),
> > +                                     /* rest of fields left empty. */
>
> But in test_reftable_log_record_roundtrip() we have another existing
> user who leaves out fields, but doesn't have such a comment.

rephrased comment.
diff mbox series

Patch

diff --git a/reftable/record.c b/reftable/record.c
index a8cee628942..fbaa1fbef56 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -255,8 +255,8 @@  static void hex_format(char *dest, uint8_t *src, int hash_size)
 	}
 }
 
-void reftable_ref_record_print(const struct reftable_ref_record *ref,
-			       uint32_t hash_id)
+static void reftable_ref_record_print_sz(const struct reftable_ref_record *ref,
+					 int hash_size)
 {
 	char hex[GIT_MAX_HEXSZ + 1] = { 0 }; /* BUG */
 	printf("ref{%s(%" PRIu64 ") ", ref->refname, ref->update_index);
@@ -265,14 +265,14 @@  void reftable_ref_record_print(const struct reftable_ref_record *ref,
 		printf("=> %s", ref->value.symref);
 		break;
 	case REFTABLE_REF_VAL2:
-		hex_format(hex, ref->value.val2.value, hash_size(hash_id));
+		hex_format(hex, ref->value.val2.value, hash_size);
 		printf("val 2 %s", hex);
 		hex_format(hex, ref->value.val2.target_value,
-			   hash_size(hash_id));
+			   hash_size);
 		printf("(T %s)", hex);
 		break;
 	case REFTABLE_REF_VAL1:
-		hex_format(hex, ref->value.val1, hash_size(hash_id));
+		hex_format(hex, ref->value.val1, hash_size);
 		printf("val 1 %s", hex);
 		break;
 	case REFTABLE_REF_DELETION:
@@ -282,6 +282,11 @@  void reftable_ref_record_print(const struct reftable_ref_record *ref,
 	printf("}\n");
 }
 
+void reftable_ref_record_print(const struct reftable_ref_record *ref,
+			       uint32_t hash_id) {
+	reftable_ref_record_print_sz(ref, hash_size(hash_id));
+}
+
 static void reftable_ref_record_release_void(void *rec)
 {
 	reftable_ref_record_release(rec);
@@ -443,6 +448,12 @@  static int reftable_ref_record_equal_void(const void *a,
 	return reftable_ref_record_equal(ra, rb, hash_size);
 }
 
+static void reftable_ref_record_print_void(const void *rec,
+					   int hash_size)
+{
+	reftable_ref_record_print_sz((struct reftable_ref_record *) rec, hash_size);
+}
+
 static struct reftable_record_vtable reftable_ref_record_vtable = {
 	.key = &reftable_ref_record_key,
 	.type = BLOCK_TYPE_REF,
@@ -453,6 +464,7 @@  static struct reftable_record_vtable reftable_ref_record_vtable = {
 	.release = &reftable_ref_record_release_void,
 	.is_deletion = &reftable_ref_record_is_deletion_void,
 	.equal = &reftable_ref_record_equal_void,
+	.print = &reftable_ref_record_print_void,
 };
 
 static void reftable_obj_record_key(const void *r, struct strbuf *dest)
@@ -471,6 +483,21 @@  static void reftable_obj_record_release(void *rec)
 	memset(obj, 0, sizeof(struct reftable_obj_record));
 }
 
+static void reftable_obj_record_print(const void *rec, int hash_size)
+{
+	const struct reftable_obj_record *obj = rec;
+	char hex[GIT_MAX_HEXSZ + 1] = { 0 };
+	struct strbuf offset_str = STRBUF_INIT;
+	int i;
+
+	for (i = 0; i < obj->offset_len; i++)
+		strbuf_addf(&offset_str, "%" PRIu64 " ", obj->offsets[i]);
+	hex_format(hex, obj->hash_prefix, obj->hash_prefix_len);
+	printf("prefix %s (len %d), offsets [%s]\n",
+	       hex, obj->hash_prefix_len, offset_str.buf);
+	strbuf_release(&offset_str);
+}
+
 static void reftable_obj_record_copy_from(void *rec, const void *src_rec,
 					  int hash_size)
 {
@@ -617,31 +644,41 @@  static struct reftable_record_vtable reftable_obj_record_vtable = {
 	.release = &reftable_obj_record_release,
 	.is_deletion = &not_a_deletion,
 	.equal = &reftable_obj_record_equal_void,
+	.print = &reftable_obj_record_print,
 };
 
-void reftable_log_record_print(struct reftable_log_record *log,
-			       uint32_t hash_id)
+static void reftable_log_record_print_sz(struct reftable_log_record *log,
+					 int hash_size)
 {
 	char hex[GIT_MAX_HEXSZ + 1] = { 0 };
 
 	switch (log->value_type) {
 	case REFTABLE_LOG_DELETION:
-		printf("log{%s(%" PRIu64 ") delete", log->refname,
+		printf("log{%s(%" PRIu64 ") delete\n", log->refname,
 		       log->update_index);
 		break;
 	case REFTABLE_LOG_UPDATE:
 		printf("log{%s(%" PRIu64 ") %s <%s> %" PRIu64 " %04d\n",
-		       log->refname, log->update_index, log->value.update.name,
-		       log->value.update.email, log->value.update.time,
+		       log->refname, log->update_index,
+		       log->value.update.name ? log->value.update.name : "",
+		       log->value.update.email ? log->value.update.email : "",
+		       log->value.update.time,
 		       log->value.update.tz_offset);
-		hex_format(hex, log->value.update.old_hash, hash_size(hash_id));
+		hex_format(hex, log->value.update.old_hash, hash_size);
 		printf("%s => ", hex);
-		hex_format(hex, log->value.update.new_hash, hash_size(hash_id));
-		printf("%s\n\n%s\n}\n", hex, log->value.update.message);
+		hex_format(hex, log->value.update.new_hash, hash_size);
+		printf("%s\n\n%s\n}\n", hex,
+		       log->value.update.message ? log->value.update.message : "");
 		break;
 	}
 }
 
+void reftable_log_record_print(struct reftable_log_record *log,
+				      uint32_t hash_id)
+{
+	reftable_log_record_print_sz(log, hash_size(hash_id));
+}
+
 static void reftable_log_record_key(const void *r, struct strbuf *dest)
 {
 	const struct reftable_log_record *rec =
@@ -959,6 +996,11 @@  static int reftable_log_record_is_deletion_void(const void *p)
 		(const struct reftable_log_record *)p);
 }
 
+static void reftable_log_record_print_void(const void *rec, int hash_size)
+{
+	reftable_log_record_print_sz((struct reftable_log_record*)rec, hash_size);
+}
+
 static struct reftable_record_vtable reftable_log_record_vtable = {
 	.key = &reftable_log_record_key,
 	.type = BLOCK_TYPE_LOG,
@@ -968,7 +1010,8 @@  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
+	.equal = &reftable_log_record_equal_void,
+	.print = &reftable_log_record_print_void,
 };
 
 static void reftable_index_record_key(const void *r, struct strbuf *dest)
@@ -1043,6 +1086,13 @@  static int reftable_index_record_equal(const void *a, const void *b, int hash_si
 	return ia->offset == ib->offset && !strbuf_cmp(&ia->last_key, &ib->last_key);
 }
 
+static void reftable_index_record_print(const void *rec, int hash_size)
+{
+	const struct reftable_index_record *idx = rec;
+	/* TODO: escape null chars? */
+	printf("\"%s\" %" PRIu64 "\n", idx->last_key.buf, idx->offset);
+}
+
 static struct reftable_record_vtable reftable_index_record_vtable = {
 	.key = &reftable_index_record_key,
 	.type = BLOCK_TYPE_INDEX,
@@ -1053,6 +1103,7 @@  static struct reftable_record_vtable reftable_index_record_vtable = {
 	.release = &reftable_index_record_release,
 	.is_deletion = &not_a_deletion,
 	.equal = &reftable_index_record_equal,
+	.print = &reftable_index_record_print,
 };
 
 void reftable_record_key(struct reftable_record *rec, struct strbuf *dest)
@@ -1255,3 +1306,9 @@  struct reftable_record reftable_new_record(uint8_t typ)
 	}
 	return clean;
 }
+
+void reftable_record_print(struct reftable_record *rec, int hash_size)
+{
+	printf("'%c': ", rec->type);
+	reftable_record_vtable(rec)->print(reftable_record_data(rec), hash_size);
+}
diff --git a/reftable/record.h b/reftable/record.h
index 010a322e901..fd80cd451d5 100644
--- a/reftable/record.h
+++ b/reftable/record.h
@@ -61,6 +61,9 @@  struct reftable_record_vtable {
 
 	/* 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);
+
+	/* Print on stdout, for debugging. */
+	void (*print)(const void *rec, int hash_size);
 };
 
 /* returns true for recognized block types. Block start with the block type. */
@@ -112,6 +115,7 @@  struct reftable_record {
 
 /* see struct record_vtable */
 int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size);
+void reftable_record_print(struct reftable_record *rec, 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 c6fdd1925a9..f91ea5e8830 100644
--- a/reftable/record_test.c
+++ b/reftable/record_test.c
@@ -25,6 +25,10 @@  static void test_copy(struct reftable_record *rec)
 	/* do it twice to catch memory leaks */
 	reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
 	EXPECT(reftable_record_equal(rec, &copy, GIT_SHA1_RAWSZ));
+
+	puts("testing print coverage:\n");
+	reftable_record_print(&copy, GIT_SHA1_RAWSZ);
+
 	reftable_record_release(&copy);
 }
 
@@ -176,7 +180,8 @@  static void test_reftable_log_record_equal(void)
 static void test_reftable_log_record_roundtrip(void)
 {
 	int i;
-	struct reftable_log_record in[2] = {
+
+	struct reftable_log_record in[] = {
 		{
 			.refname = xstrdup("refs/heads/master"),
 			.update_index = 42,
@@ -197,10 +202,24 @@  static void test_reftable_log_record_roundtrip(void)
 			.refname = xstrdup("refs/heads/master"),
 			.update_index = 22,
 			.value_type = REFTABLE_LOG_DELETION,
+		},
+		{
+			.refname = xstrdup("branch"),
+			.update_index = 33,
+			.value_type = REFTABLE_LOG_UPDATE,
+			.value = {
+				.update = {
+					.old_hash = reftable_malloc(GIT_SHA1_RAWSZ),
+					.new_hash = reftable_malloc(GIT_SHA1_RAWSZ),
+					/* rest of fields left empty. */
+				},
+			},
 		}
 	};
 	set_test_hash(in[0].value.update.new_hash, 1);
 	set_test_hash(in[0].value.update.old_hash, 2);
+	set_test_hash(in[2].value.update.new_hash, 3);
+	set_test_hash(in[2].value.update.old_hash, 4);
 	for (i = 0; i < ARRAY_SIZE(in); i++) {
 		struct reftable_record rec = { .type = BLOCK_TYPE_LOG };
 		struct strbuf key = STRBUF_INIT;