On Thu, Jan 20 2022, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
> [...]
> +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(©, rec, GIT_SHA1_RAWSZ);
> EXPECT(reftable_record_equal(rec, ©, GIT_SHA1_RAWSZ));
> +
> + puts("testing print coverage:\n");
> + reftable_record_print(©, GIT_SHA1_RAWSZ);
...i.e. here.
> +
> reftable_record_release(©);
> }
>
> @@ -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.
