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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 21, 2022, 12:33 UTC
Message-ID: <220121.86bl05jm8o.gmgdl@evledraar.gmail.com>
In-Reply-To: <82f140cab5c30b59de534d773a5d00343e0868f9.1642691534.git.gitgitgadget@gmail.com>

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...

Show 12 quoted lines

> +	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....

Show 23 quoted lines

> +	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.

Show 10 quoted lines

> +
>  	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...

Show 18 quoted lines

> +	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.