diff mbox series

[10/10] reftable: make reftable_record a tagged union

Message ID 8deccc3a1dff7e4f7d613fa63d2781fd1f11f841.1638899124.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Reftable coverity fixes | expand

Commit Message

Han-Wen Nienhuys Dec. 7, 2021, 5:45 p.m. UTC
From: Han-Wen Nienhuys <hanwen@google.com>

This reduces the amount of glue code, because we don't need a void pointer or
vtable within the structure.

The only snag is that reftable_index_record contain a strbuf, so it cannot be
zero-initialized. To address this, introduce reftable_record_for() to create a
fresh instance, given a record type.

Thanks to Peff for the suggestion.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 reftable/block.c       |   4 +-
 reftable/block_test.c  |  22 +++---
 reftable/generic.c     |  35 ++++----
 reftable/iter.c        |   4 +-
 reftable/merged.c      |  37 ++++-----
 reftable/pq.c          |   3 +-
 reftable/pq_test.c     |  31 ++++----
 reftable/reader.c      | 105 ++++++++++++------------
 reftable/record.c      | 176 ++++++++++++++++-------------------------
 reftable/record.h      |  45 +++++------
 reftable/record_test.c | 162 +++++++++++++++++++------------------
 reftable/writer.c      |  46 ++++++-----
 12 files changed, 319 insertions(+), 351 deletions(-)

Comments

Junio C Hamano Dec. 7, 2021, 9:56 p.m. UTC | #1
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +struct reftable_record {
> +	uint8_t type;
> +	union {
> +		struct reftable_ref_record ref;
> +		struct reftable_log_record log;
> +		struct reftable_obj_record obj;
> +		struct reftable_index_record idx;
> +	};
> +};

error: ISO C99 doesn't support unnamed structs/unions [-Werror=pedantic]
Jeff King Dec. 8, 2021, 2:15 a.m. UTC | #2
On Tue, Dec 07, 2021 at 01:56:07PM -0800, Junio C Hamano wrote:

> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > +struct reftable_record {
> > +	uint8_t type;
> > +	union {
> > +		struct reftable_ref_record ref;
> > +		struct reftable_log_record log;
> > +		struct reftable_obj_record obj;
> > +		struct reftable_index_record idx;
> > +	};
> > +};
> 
> error: ISO C99 doesn't support unnamed structs/unions [-Werror=pedantic]

Hmm. It's interesting that the regular DEVELOPER=1 doesn't catch this.
It's because we don't specify -std there, and newer gcc defaults to
gnu17 (unnamed unions appeared in c11, I think). I wonder if it would be
helpful to teach config.mak.dev to pass -std=c99.

Anyway, the usual fix is to stick a "u.foo" everywhere. Patch is below
for convenience. Two things to note:

  - I used ".u.foo = {...}" as a designated initializer. I think this is
    allowed by the standard, but I don't think it's something we've used
    before. If it's a problem, it can be solved by using an extra level
    of braces (which is OK, just uglier).

  - Han-Wen, if you don't just squash in the patch below, note that it
    also has a fixup for some misleading indentation in an initializer
    in test_reftable_log_record_roundtrip(). You might want to grab that
    separately.

---
 reftable/block_test.c  | 16 ++++----
 reftable/generic.c     |  8 ++--
 reftable/iter.c        |  4 +-
 reftable/merged.c      |  4 +-
 reftable/pq_test.c     |  8 ++--
 reftable/reader.c      | 26 ++++++------
 reftable/record.c      | 10 ++---
 reftable/record.h      |  2 +-
 reftable/record_test.c | 74 +++++++++++++++++------------------
 reftable/writer.c      | 12 +++---
 10 files changed, 82 insertions(+), 82 deletions(-)

diff --git a/reftable/block_test.c b/reftable/block_test.c
index 670329f222..fa2ee092ec 100644
--- a/reftable/block_test.c
+++ b/reftable/block_test.c
@@ -48,14 +48,14 @@ static void test_block_read_write(void)
 		snprintf(name, sizeof(name), "branch%02d", i);
 		memset(hash, i, sizeof(hash));
 
-		rec.ref.refname = name;
-		rec.ref.value_type = REFTABLE_REF_VAL1;
-		rec.ref.value.val1 = hash;
+		rec.u.ref.refname = name;
+		rec.u.ref.value_type = REFTABLE_REF_VAL1;
+		rec.u.ref.value.val1 = hash;
 
 		names[i] = xstrdup(name);
 		n = block_writer_add(&bw, &rec);
-		rec.ref.refname = NULL;
-		rec.ref.value_type = REFTABLE_REF_DELETION;
+		rec.u.ref.refname = NULL;
+		rec.u.ref.value_type = REFTABLE_REF_DELETION;
 		EXPECT(n == 0);
 	}
 
@@ -74,7 +74,7 @@ static void test_block_read_write(void)
 		if (r > 0) {
 			break;
 		}
-		EXPECT_STREQ(names[j], rec.ref.refname);
+		EXPECT_STREQ(names[j], rec.u.ref.refname);
 		j++;
 	}
 
@@ -92,15 +92,15 @@ static void test_block_read_write(void)
 		n = block_iter_next(&it, &rec);
 		EXPECT(n == 0);
 
-		EXPECT_STREQ(names[i], rec.ref.refname);
+		EXPECT_STREQ(names[i], rec.u.ref.refname);
 
 		want.len--;
 		n = block_reader_seek(&br, &it, &want);
 		EXPECT(n == 0);
 
 		n = block_iter_next(&it, &rec);
 		EXPECT(n == 0);
-		EXPECT_STREQ(names[10 * (i / 10)], rec.ref.refname);
+		EXPECT_STREQ(names[10 * (i / 10)], rec.u.ref.refname);
 
 		block_iter_close(&it);
 	}
diff --git a/reftable/generic.c b/reftable/generic.c
index 7f00aa9cdb..4446b8ed36 100644
--- a/reftable/generic.c
+++ b/reftable/generic.c
@@ -17,7 +17,7 @@ int reftable_table_seek_ref(struct reftable_table *tab,
 			    struct reftable_iterator *it, const char *name)
 {
 	struct reftable_record rec = { .type = BLOCK_TYPE_REF,
-				       .ref = {
+				       .u.ref = {
 					       .refname = (char *)name,
 				       } };
 	return tab->ops->seek_record(tab->table_arg, it, &rec);
@@ -27,7 +27,7 @@ int reftable_table_seek_log(struct reftable_table *tab,
 			    struct reftable_iterator *it, const char *name)
 {
 	struct reftable_record rec = { .type = BLOCK_TYPE_LOG,
-				       .log = {
+				       .u.log = {
 					       .refname = (char *)name,
 					       .update_index = ~((uint64_t)0),
 				       } };
@@ -130,7 +130,7 @@ int reftable_iterator_next_ref(struct reftable_iterator *it,
 {
 	struct reftable_record rec = { .type = BLOCK_TYPE_REF };
 	int err = iterator_next(it, &rec);
-	*ref = rec.ref;
+	*ref = rec.u.ref;
 	return err;
 }
 
@@ -139,7 +139,7 @@ int reftable_iterator_next_log(struct reftable_iterator *it,
 {
 	struct reftable_record rec = { .type = BLOCK_TYPE_LOG };
 	int err = iterator_next(it, &rec);
-	*log = rec.log;
+	*log = rec.u.log;
 	return err;
 }
 
diff --git a/reftable/iter.c b/reftable/iter.c
index fedf5d8ab0..a8d174c040 100644
--- a/reftable/iter.c
+++ b/reftable/iter.c
@@ -32,7 +32,7 @@ static int filtering_ref_iterator_next(void *iter_arg,
 				       struct reftable_record *rec)
 {
 	struct filtering_ref_iterator *fri = iter_arg;
-	struct reftable_ref_record *ref = &rec->ref;
+	struct reftable_ref_record *ref = &rec->u.ref;
 	int err = 0;
 	while (1) {
 		err = reftable_iterator_next_ref(&fri->it, ref);
@@ -127,7 +127,7 @@ static int indexed_table_ref_iter_next_block(struct indexed_table_ref_iter *it)
 static int indexed_table_ref_iter_next(void *p, struct reftable_record *rec)
 {
 	struct indexed_table_ref_iter *it = p;
-	struct reftable_ref_record *ref = &rec->ref;
+	struct reftable_ref_record *ref = &rec->u.ref;
 
 	while (1) {
 		int err = block_iter_next(&it->cur, rec);
diff --git a/reftable/merged.c b/reftable/merged.c
index e69955d7c2..e9b0edec97 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -291,7 +291,7 @@ int reftable_merged_table_seek_ref(struct reftable_merged_table *mt,
 {
 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_REF,
-		.ref = {
+		.u.ref = {
 			.refname = (char *)name,
 		},
 	};
@@ -304,7 +304,7 @@ int reftable_merged_table_seek_log_at(struct reftable_merged_table *mt,
 {
 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_LOG,
-		.log = {
+		.u.log = {
 			.refname = (char *)name,
 			.update_index = update_index,
 		}
diff --git a/reftable/pq_test.c b/reftable/pq_test.c
index 89d069d674..9f04d84fed 100644
--- a/reftable/pq_test.c
+++ b/reftable/pq_test.c
@@ -45,7 +45,7 @@ static void test_pq(void)
 		struct pq_entry e = {
 			.rec = {
 				.type = BLOCK_TYPE_REF,
-				.ref = {
+				.u.ref = {
 					.refname = names[i],
 				}
 			}
@@ -62,11 +62,11 @@ static void test_pq(void)
 
 		EXPECT(rec->type == BLOCK_TYPE_REF);
 		if (last) {
-			EXPECT(strcmp(last, rec->ref.refname) < 0);
+			EXPECT(strcmp(last, rec->u.ref.refname) < 0);
 		}
 		// this is names[i], so don't dealloc.
-		last = rec->ref.refname;
-		rec->ref.refname = NULL;
+		last = rec->u.ref.refname;
+		rec->u.ref.refname = NULL;
 		reftable_record_release(rec);
 	}
 	for (i = 0; i < N; i++) {
diff --git a/reftable/reader.c b/reftable/reader.c
index 0159f13112..ef322791fd 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -239,7 +239,7 @@ static int table_iter_next_in_block(struct table_iter *ti,
 {
 	int res = block_iter_next(&ti->bi, rec);
 	if (res == 0 && rec->type == BLOCK_TYPE_REF) {
-		rec->ref.update_index += ti->r->min_update_index;
+		rec->u.ref.update_index += ti->r->min_update_index;
 	}
 
 	return res;
@@ -485,17 +485,17 @@ static int reader_seek_indexed(struct reftable_reader *r,
 {
 	struct reftable_record want_index = {
 		.type = BLOCK_TYPE_INDEX,
-		.idx = { .last_key = STRBUF_INIT }
+		.u.idx = { .last_key = STRBUF_INIT }
 	};
 	struct reftable_record index_result = {
 		.type = BLOCK_TYPE_INDEX,
-		.idx = { .last_key = STRBUF_INIT },
+		.u.idx = { .last_key = STRBUF_INIT },
 	};
 	struct table_iter index_iter = TABLE_ITER_INIT;
 	struct table_iter next = TABLE_ITER_INIT;
 	int err = 0;
 
-	reftable_record_key(rec, &want_index.idx.last_key);
+	reftable_record_key(rec, &want_index.u.idx.last_key);
 	err = reader_start(r, &index_iter, rec->type, 1);
 	if (err < 0)
 		goto done;
@@ -507,12 +507,12 @@ static int reader_seek_indexed(struct reftable_reader *r,
 		if (err != 0)
 			goto done;
 
-		err = reader_table_iter_at(r, &next, index_result.idx.offset,
+		err = reader_table_iter_at(r, &next, index_result.u.idx.offset,
 					   0);
 		if (err != 0)
 			goto done;
 
-		err = block_iter_seek(&next.bi, &want_index.idx.last_key);
+		err = block_iter_seek(&next.bi, &want_index.u.idx.last_key);
 		if (err < 0)
 			goto done;
 
@@ -591,7 +591,7 @@ int reftable_reader_seek_ref(struct reftable_reader *r,
 {
 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_REF,
-		.ref = {
+		.u.ref = {
 			.refname = (char *)name,
 		},
 	};
@@ -604,7 +604,7 @@ int reftable_reader_seek_log_at(struct reftable_reader *r,
 {
 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_LOG,
-		.log = {
+		.u.log = {
 			.refname = (char *)name,
 			.update_index = update_index,
 		}
@@ -652,15 +652,15 @@ static int reftable_reader_refs_for_indexed(struct reftable_reader *r,
 {
 	struct reftable_record want = {
 		.type = BLOCK_TYPE_OBJ,
-		.obj = {
+		.u.obj = {
 			.hash_prefix = oid,
 			.hash_prefix_len = r->object_id_len,
 		},
 	};
 	struct reftable_iterator oit = { NULL };
 	struct reftable_record got = {
 		.type = BLOCK_TYPE_OBJ,
-		.obj = { 0 },
+		.u.obj = { 0 },
 	};
 	int err = 0;
 	struct indexed_table_ref_iter *itr = NULL;
@@ -675,7 +675,7 @@ static int reftable_reader_refs_for_indexed(struct reftable_reader *r,
 	if (err < 0)
 		goto done;
 
-	if (err > 0 || memcmp(want.obj.hash_prefix, got.obj.hash_prefix,
+	if (err > 0 || memcmp(want.u.obj.hash_prefix, got.u.obj.hash_prefix,
 			      r->object_id_len)) {
 		/* didn't find it; return empty iterator */
 		iterator_set_empty(it);
@@ -684,10 +684,10 @@ static int reftable_reader_refs_for_indexed(struct reftable_reader *r,
 	}
 
 	err = new_indexed_table_ref_iter(&itr, r, oid, hash_size(r->hash_id),
-					 got.obj.offsets, got.obj.offset_len);
+					 got.u.obj.offsets, got.u.obj.offset_len);
 	if (err < 0)
 		goto done;
-	got.obj.offsets = NULL;
+	got.u.obj.offsets = NULL;
 	iterator_from_indexed_table_ref_iter(it, itr);
 
 done:
diff --git a/reftable/record.c b/reftable/record.c
index b665feb709..68fdde9d7c 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -1123,13 +1123,13 @@ static void *reftable_record_data(struct reftable_record *rec)
 {
 	switch (rec->type) {
 	case BLOCK_TYPE_REF:
-		return &rec->ref;
+		return &rec->u.ref;
 	case BLOCK_TYPE_LOG:
-		return &rec->log;
+		return &rec->u.log;
 	case BLOCK_TYPE_INDEX:
-		return &rec->idx;
+		return &rec->u.idx;
 	case BLOCK_TYPE_OBJ:
-		return &rec->obj;
+		return &rec->u.obj;
 	}
 	abort();
 }
@@ -1154,7 +1154,7 @@ struct reftable_record reftable_record_for(uint8_t typ)
 {
 	struct reftable_record clean_idx = {
 		.type = BLOCK_TYPE_INDEX,
-		.idx = {
+		.u.idx = {
 			.last_key = STRBUF_INIT,
 		},
 	};
diff --git a/reftable/record.h b/reftable/record.h
index a702c67e85..1fe0c14a19 100644
--- a/reftable/record.h
+++ b/reftable/record.h
@@ -101,7 +101,7 @@ struct reftable_record {
 		struct reftable_log_record log;
 		struct reftable_obj_record obj;
 		struct reftable_index_record idx;
-	};
+	} u;
 };
 
 /* see struct record_vtable */
diff --git a/reftable/record_test.c b/reftable/record_test.c
index 23abfb037b..f234a0382a 100644
--- a/reftable/record_test.c
+++ b/reftable/record_test.c
@@ -23,11 +23,11 @@ static void test_copy(struct reftable_record *rec)
 	reftable_record_copy_from(&copy, rec, GIT_SHA1_RAWSZ);
 	switch (copy.type) {
 	case BLOCK_TYPE_REF:
-		EXPECT(reftable_ref_record_equal(&copy.ref, &rec->ref,
+		EXPECT(reftable_ref_record_equal(&copy.u.ref, &rec->u.ref,
 						 GIT_SHA1_RAWSZ));
 		break;
 	case BLOCK_TYPE_LOG:
-		EXPECT(reftable_log_record_equal(&copy.log, &rec->log,
+		EXPECT(reftable_log_record_equal(&copy.u.log, &rec->u.log,
 						 GIT_SHA1_RAWSZ));
 		break;
 	}
@@ -118,27 +118,27 @@ static void test_reftable_ref_record_roundtrip(void)
 		};
 		int n, m;
 
-		in.ref.value_type = i;
+		in.u.ref.value_type = i;
 		switch (i) {
 		case REFTABLE_REF_DELETION:
 			break;
 		case REFTABLE_REF_VAL1:
-			in.ref.value.val1 = reftable_malloc(GIT_SHA1_RAWSZ);
-			set_hash(in.ref.value.val1, 1);
+			in.u.ref.value.val1 = reftable_malloc(GIT_SHA1_RAWSZ);
+			set_hash(in.u.ref.value.val1, 1);
 			break;
 		case REFTABLE_REF_VAL2:
-			in.ref.value.val2.value =
+			in.u.ref.value.val2.value =
 				reftable_malloc(GIT_SHA1_RAWSZ);
-			set_hash(in.ref.value.val2.value, 1);
-			in.ref.value.val2.target_value =
+			set_hash(in.u.ref.value.val2.value, 1);
+			in.u.ref.value.val2.target_value =
 				reftable_malloc(GIT_SHA1_RAWSZ);
-			set_hash(in.ref.value.val2.target_value, 2);
+			set_hash(in.u.ref.value.val2.target_value, 2);
 			break;
 		case REFTABLE_REF_SYMREF:
-			in.ref.value.symref = xstrdup("target");
+			in.u.ref.value.symref = xstrdup("target");
 			break;
 		}
-		in.ref.refname = xstrdup("refs/heads/master");
+		in.u.ref.refname = xstrdup("refs/heads/master");
 
 		test_copy(&in);
 
@@ -152,7 +152,7 @@ static void test_reftable_ref_record_roundtrip(void)
 		m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ);
 		EXPECT(n == m);
 
-		EXPECT(reftable_ref_record_equal(&in.ref, &out.ref,
+		EXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref,
 						 GIT_SHA1_RAWSZ));
 		reftable_record_release(&in);
 
@@ -222,23 +222,23 @@ static void test_reftable_log_record_roundtrip(void)
 		/* populate out, to check for leaks. */
 		struct reftable_record out = {
 			.type = BLOCK_TYPE_LOG,
-			.log = {
+			.u.log = {
 				.refname = xstrdup("old name"),
-			.value_type = REFTABLE_LOG_UPDATE,
-			.value = {
-				.update = {
-					.new_hash = reftable_calloc(GIT_SHA1_RAWSZ),
-					.old_hash = reftable_calloc(GIT_SHA1_RAWSZ),
-					.name = xstrdup("old name"),
-					.email = xstrdup("old@email"),
-					.message = xstrdup("old message"),
+				.value_type = REFTABLE_LOG_UPDATE,
+				.value = {
+					.update = {
+						.new_hash = reftable_calloc(GIT_SHA1_RAWSZ),
+						.old_hash = reftable_calloc(GIT_SHA1_RAWSZ),
+						.name = xstrdup("old name"),
+						.email = xstrdup("old@email"),
+						.message = xstrdup("old message"),
+					},
 				},
 			},
-			},
 		};
 		int n, m, valtype;
 
-		rec.log = in[i];
+		rec.u.log = in[i];
 
 		test_copy(&rec);
 
@@ -251,7 +251,7 @@ static void test_reftable_log_record_roundtrip(void)
 					   GIT_SHA1_RAWSZ);
 		EXPECT(n == m);
 
-		EXPECT(reftable_log_record_equal(&in[i], &out.log,
+		EXPECT(reftable_log_record_equal(&in[i], &out.u.log,
 						 GIT_SHA1_RAWSZ));
 		reftable_log_record_release(&in[i]);
 		strbuf_release(&key);
@@ -330,7 +330,7 @@ static void test_reftable_obj_record_roundtrip(void)
 		};
 		struct reftable_record in = {
 			.type = BLOCK_TYPE_OBJ,
-			.obj = recs[i],
+			.u.obj = recs[i],
 		};
 		struct strbuf key = STRBUF_INIT;
 		struct reftable_record out = {
@@ -348,13 +348,13 @@ static void test_reftable_obj_record_roundtrip(void)
 					   GIT_SHA1_RAWSZ);
 		EXPECT(n == m);
 
-		EXPECT(in.obj.hash_prefix_len == out.obj.hash_prefix_len);
-		EXPECT(in.obj.offset_len == out.obj.offset_len);
+		EXPECT(in.u.obj.hash_prefix_len == out.u.obj.hash_prefix_len);
+		EXPECT(in.u.obj.offset_len == out.u.obj.offset_len);
 
-		EXPECT(!memcmp(in.obj.hash_prefix, out.obj.hash_prefix,
-			       in.obj.hash_prefix_len));
-		EXPECT(0 == memcmp(in.obj.offsets, out.obj.offsets,
-				   sizeof(uint64_t) * in.obj.offset_len));
+		EXPECT(!memcmp(in.u.obj.hash_prefix, out.u.obj.hash_prefix,
+			       in.u.obj.hash_prefix_len));
+		EXPECT(0 == memcmp(in.u.obj.offsets, out.u.obj.offsets,
+				   sizeof(uint64_t) * in.u.obj.offset_len));
 		strbuf_release(&key);
 		reftable_record_release(&out);
 	}
@@ -364,7 +364,7 @@ static void test_reftable_index_record_roundtrip(void)
 {
 	struct reftable_record in = {
 		.type = BLOCK_TYPE_INDEX,
-		.idx = {
+		.u.idx = {
 			.offset = 42,
 			.last_key = STRBUF_INIT,
 		},
@@ -377,28 +377,28 @@ static void test_reftable_index_record_roundtrip(void)
 	struct strbuf key = STRBUF_INIT;
 	struct reftable_record out = {
 		.type = BLOCK_TYPE_INDEX,
-		.idx = { .last_key = STRBUF_INIT },
+		.u.idx = { .last_key = STRBUF_INIT },
 	};
 	int n, m;
 	uint8_t extra;
 
-	strbuf_addstr(&in.idx.last_key, "refs/heads/master");
+	strbuf_addstr(&in.u.idx.last_key, "refs/heads/master");
 	reftable_record_key(&in, &key);
 	test_copy(&in);
 
-	EXPECT(0 == strbuf_cmp(&key, &in.idx.last_key));
+	EXPECT(0 == strbuf_cmp(&key, &in.u.idx.last_key));
 	n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
 	EXPECT(n > 0);
 
 	extra = reftable_record_val_type(&in);
 	m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ);
 	EXPECT(m == n);
 
-	EXPECT(in.idx.offset == out.idx.offset);
+	EXPECT(in.u.idx.offset == out.u.idx.offset);
 
 	reftable_record_release(&out);
 	strbuf_release(&key);
-	strbuf_release(&in.idx.last_key);
+	strbuf_release(&in.u.idx.last_key);
 }
 
 int record_test_main(int argc, const char *argv[])
diff --git a/reftable/writer.c b/reftable/writer.c
index df256f3a8b..f9544c5f97 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -253,7 +253,7 @@ int reftable_writer_add_ref(struct reftable_writer *w,
 {
 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_REF,
-		.ref = *ref,
+		.u.ref = *ref,
 	};
 	int err = 0;
 
@@ -263,7 +263,7 @@ int reftable_writer_add_ref(struct reftable_writer *w,
 	    ref->update_index > w->max_update_index)
 		return REFTABLE_API_ERROR;
 
-	rec.ref.update_index -= w->min_update_index;
+	rec.u.ref.update_index -= w->min_update_index;
 
 	err = writer_add_record(w, &rec);
 	if (err < 0)
@@ -304,7 +304,7 @@ static int reftable_writer_add_log_verbatim(struct reftable_writer *w,
 {
 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_LOG,
-		.log = *log,
+		.u.log = *log,
 	};
 	if (w->block_writer &&
 	    block_writer_type(w->block_writer) == BLOCK_TYPE_REF) {
@@ -397,7 +397,7 @@ static int writer_finish_section(struct reftable_writer *w)
 		for (i = 0; i < idx_len; i++) {
 			struct reftable_record rec = {
 				.type = BLOCK_TYPE_INDEX,
-				.idx = idx[i],
+				.u.idx = idx[i],
 			};
 			if (block_writer_add(w->block_writer, &rec) == 0) {
 				continue;
@@ -468,7 +468,7 @@ static void write_object_record(void *void_arg, void *key)
 	struct obj_index_tree_node *entry = key;
 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_OBJ,
-		.obj = {
+		.u.obj = {
 			.hash_prefix = (uint8_t *)entry->hash.buf,
 			.hash_prefix_len = arg->w->stats.object_id_len,
 			.offsets = entry->offsets,
@@ -491,7 +491,7 @@ static void write_object_record(void *void_arg, void *key)
 	if (arg->err == 0)
 		goto done;
 
-	rec.obj.offset_len = 0;
+	rec.u.obj.offset_len = 0;
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 
 	/* Should be able to write into a fresh block. */
Junio C Hamano Dec. 8, 2021, 4:13 a.m. UTC | #3
Jeff King <peff@peff.net> writes:

>> error: ISO C99 doesn't support unnamed structs/unions [-Werror=pedantic]
>
> Hmm. It's interesting that the regular DEVELOPER=1 doesn't catch this.
> It's because we don't specify -std there, and newer gcc defaults to
> gnu17 (unnamed unions appeared in c11, I think). I wonder if it would be
> helpful to teach config.mak.dev to pass -std=c99.

FWIW, I use -std=gnu99 as our Makefile suggests.
Han-Wen Nienhuys Dec. 8, 2021, 10:30 a.m. UTC | #4
On Wed, Dec 8, 2021 at 5:13 AM Junio C Hamano <gitster@pobox.com> wrote:
> >> error: ISO C99 doesn't support unnamed structs/unions [-Werror=pedantic]
> >
> > Hmm. It's interesting that the regular DEVELOPER=1 doesn't catch this.
> > It's because we don't specify -std there, and newer gcc defaults to
> > gnu17 (unnamed unions appeared in c11, I think). I wonder if it would be
> > helpful to teach config.mak.dev to pass -std=c99.
>
> FWIW, I use -std=gnu99 as our Makefile suggests.

I understand that the default build should be lenient rather than
strict for portability reasons. However, it would be good if the CI
was strict with this.
Derrick Stolee Dec. 8, 2021, 2:35 p.m. UTC | #5
On 12/7/2021 12:45 PM, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> This reduces the amount of glue code, because we don't need a void pointer or
> vtable within the structure.
> 
> The only snag is that reftable_index_record contain a strbuf, so it cannot be
> zero-initialized. To address this, introduce reftable_record_for() to create a
> fresh instance, given a record type.
> 
> Thanks to Peff for the suggestion.
> 
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  reftable/block.c       |   4 +-
>  reftable/block_test.c  |  22 +++---
>  reftable/generic.c     |  35 ++++----
>  reftable/iter.c        |   4 +-
>  reftable/merged.c      |  37 ++++-----
>  reftable/pq.c          |   3 +-
>  reftable/pq_test.c     |  31 ++++----
>  reftable/reader.c      | 105 ++++++++++++------------
>  reftable/record.c      | 176 ++++++++++++++++-------------------------
>  reftable/record.h      |  45 +++++------
>  reftable/record_test.c | 162 +++++++++++++++++++------------------
>  reftable/writer.c      |  46 ++++++-----

This is a HUGE diff, especially compared to the previous changes
in this series. I recommend splitting this out into its own series
and finding a way to break it down into smaller changes.

Thanks,
-Stolee
Han-Wen Nienhuys Dec. 8, 2021, 2:48 p.m. UTC | #6
On Wed, Dec 8, 2021 at 3:35 PM Derrick Stolee <stolee@gmail.com> wrote:
> This is a HUGE diff, especially compared to the previous changes
> in this series. I recommend splitting this out into its own series
> and finding a way to break it down into smaller changes.

Would you have a suggestion how? The reftable_record type is used
across the reftable library, so if we change its definition, that
impacts most callsites.
Junio C Hamano Dec. 8, 2021, 4:35 p.m. UTC | #7
Han-Wen Nienhuys <hanwen@google.com> writes:

> On Wed, Dec 8, 2021 at 5:13 AM Junio C Hamano <gitster@pobox.com> wrote:
>> >> error: ISO C99 doesn't support unnamed structs/unions [-Werror=pedantic]
>> >
>> > Hmm. It's interesting that the regular DEVELOPER=1 doesn't catch this.
>> > It's because we don't specify -std there, and newer gcc defaults to
>> > gnu17 (unnamed unions appeared in c11, I think). I wonder if it would be
>> > helpful to teach config.mak.dev to pass -std=c99.
>>
>> FWIW, I use -std=gnu99 as our Makefile suggests.
>
> I understand that the default build should be lenient rather than
> strict for portability reasons. However, it would be good if the CI
> was strict with this.

Yeah, I agree with the above, and was a bit surprised that I found
the issue via my local build after it came from GGG.
Junio C Hamano Dec. 8, 2021, 4:47 p.m. UTC | #8
Derrick Stolee <stolee@gmail.com> writes:

> On 12/7/2021 12:45 PM, Han-Wen Nienhuys via GitGitGadget wrote:
>> From: Han-Wen Nienhuys <hanwen@google.com>
>> 
>> This reduces the amount of glue code, because we don't need a void pointer or
>> vtable within the structure.
>> 
>> The only snag is that reftable_index_record contain a strbuf, so it cannot be
>> zero-initialized. To address this, introduce reftable_record_for() to create a
>> fresh instance, given a record type.
>> 
>> Thanks to Peff for the suggestion.
>> 
>> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
>> ---
>>  reftable/block.c       |   4 +-
>>  reftable/block_test.c  |  22 +++---
>>  reftable/generic.c     |  35 ++++----
>>  reftable/iter.c        |   4 +-
>>  reftable/merged.c      |  37 ++++-----
>>  reftable/pq.c          |   3 +-
>>  reftable/pq_test.c     |  31 ++++----
>>  reftable/reader.c      | 105 ++++++++++++------------
>>  reftable/record.c      | 176 ++++++++++++++++-------------------------
>>  reftable/record.h      |  45 +++++------
>>  reftable/record_test.c | 162 +++++++++++++++++++------------------
>>  reftable/writer.c      |  46 ++++++-----
>
> This is a HUGE diff, especially compared to the previous changes
> in this series. I recommend splitting this out into its own series
> and finding a way to break it down into smaller changes.

As the reftable_record structure is used everywhere (and that is why
this step has to touch everywhere), I suspect that a reviewable fix
in small chunks would be achievable only if we redo the topic that
introduces this hierarchy and fix the type at the source, as if the
reftable_record structure was a struct with union in it from the
beginning, I am afraid.

Perhaps reftable_record_for() can be implemented without changing
the shape of the underlying reftable_record structure in an earlier
step, then all the users of reftable_record instances can be
migrated to call it, and then finally the shape of the structure and
the implementation of reftable_record_for() can be updated?  

If that is doable, then the "migrate each users" part can be split
purely by size.  But (1) I do not know if the first step is even
doable, and (2) I am not sure if it is worth going a somewhat
roundabout route to get to the same destination in this case.

So...
Han-Wen Nienhuys Dec. 8, 2021, 5:51 p.m. UTC | #9
On Wed, Dec 8, 2021 at 5:47 PM Junio C Hamano <gitster@pobox.com> wrote:
> Perhaps reftable_record_for() can be implemented without changing
> the shape of the underlying reftable_record structure in an earlier
> step, then all the users of reftable_record instances can be
> migrated to call it, and then finally the shape of the structure and
> the implementation of reftable_record_for() can be updated?

We are going from

 reftable_ref_record ref = { 0 };
 reftable_record rec =  { 0 };
 record_from_ref_record(&rec, &ref);

to

 reftable_record rec =  { .type = BLOCK_TYPE_REF };

which is why this change is so nice.  reftable_record_for() is used in
just a few places, so it wouldn't make the change much smaller.
Derrick Stolee Dec. 8, 2021, 6:17 p.m. UTC | #10
On 12/8/2021 9:48 AM, Han-Wen Nienhuys wrote:
> On Wed, Dec 8, 2021 at 3:35 PM Derrick Stolee <stolee@gmail.com> wrote:
>> This is a HUGE diff, especially compared to the previous changes
>> in this series. I recommend splitting this out into its own series
>> and finding a way to break it down into smaller changes.
> 
> Would you have a suggestion how? The reftable_record type is used
> across the reftable library, so if we change its definition, that
> impacts most callsites.
 
Looking at the diff, I'm seeing a lot of things that don't seem
necessary to changing the struct, like renaming "reftable_new_record"
to "reftable_record_for". Yes, the implementation changes, but that
implementation is related to the struct changing and can be done in
a more isolated way.

Some things can be split out easier, such as the addition of
"uint8_t type" into the struct and replacing all reftable_record_type()
calls with <var>.type.

The big one is definitely the addition of a union, but it could be
done in a way that only changes the implementation of methods with
names such as reftable_record_from_obj() then we can replace those with
"<var>.obj" later.

Some of these changes are definitely going to have a lot of lines,
but when it's the same kind of change it is easy to verify. I'm
getting lost in this diff because it's doing too many things at once.

Thanks,
-Stolee
Han-Wen Nienhuys Dec. 23, 2021, 5:11 p.m. UTC | #11
On Wed, Dec 8, 2021 at 7:17 PM Derrick Stolee <stolee@gmail.com> wrote:
> Some of these changes are definitely going to have a lot of lines,
> but when it's the same kind of change it is easy to verify. I'm
> getting lost in this diff because it's doing too many things at once.

I took some of your advice in the last version of the patch. PTAL.
diff mbox series

Patch

diff --git a/reftable/block.c b/reftable/block.c
index d7347bb3152..ad713caa57b 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -382,7 +382,7 @@  int block_reader_seek(struct block_reader *br, struct block_iter *it,
 		.key = *want,
 		.r = br,
 	};
-	struct reftable_record rec = reftable_new_record(block_reader_type(br));
+	struct reftable_record rec = reftable_record_for(block_reader_type(br));
 	struct strbuf key = STRBUF_INIT;
 	int err = 0;
 	struct block_iter next = {
@@ -424,7 +424,7 @@  int block_reader_seek(struct block_reader *br, struct block_iter *it,
 done:
 	strbuf_release(&key);
 	strbuf_release(&next.last_key);
-	reftable_record_destroy(&rec);
+	reftable_record_release(&rec);
 
 	return err;
 }
diff --git a/reftable/block_test.c b/reftable/block_test.c
index 4b3ea262dcb..670329f2224 100644
--- a/reftable/block_test.c
+++ b/reftable/block_test.c
@@ -26,8 +26,9 @@  static void test_block_read_write(void)
 	struct block_writer bw = {
 		.last_key = STRBUF_INIT,
 	};
-	struct reftable_ref_record ref = { NULL };
-	struct reftable_record rec = { NULL };
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_REF,
+	};
 	int i = 0;
 	int n;
 	struct block_reader br = { 0 };
@@ -40,7 +41,6 @@  static void test_block_read_write(void)
 	block.source = malloc_block_source();
 	block_writer_init(&bw, BLOCK_TYPE_REF, block.data, block_size,
 			  header_off, hash_size(GIT_SHA1_FORMAT_ID));
-	reftable_record_from_ref(&rec, &ref);
 
 	for (i = 0; i < N; i++) {
 		char name[100];
@@ -48,14 +48,14 @@  static void test_block_read_write(void)
 		snprintf(name, sizeof(name), "branch%02d", i);
 		memset(hash, i, sizeof(hash));
 
-		ref.refname = name;
-		ref.value_type = REFTABLE_REF_VAL1;
-		ref.value.val1 = hash;
+		rec.ref.refname = name;
+		rec.ref.value_type = REFTABLE_REF_VAL1;
+		rec.ref.value.val1 = hash;
 
 		names[i] = xstrdup(name);
 		n = block_writer_add(&bw, &rec);
-		ref.refname = NULL;
-		ref.value_type = REFTABLE_REF_DELETION;
+		rec.ref.refname = NULL;
+		rec.ref.value_type = REFTABLE_REF_DELETION;
 		EXPECT(n == 0);
 	}
 
@@ -74,7 +74,7 @@  static void test_block_read_write(void)
 		if (r > 0) {
 			break;
 		}
-		EXPECT_STREQ(names[j], ref.refname);
+		EXPECT_STREQ(names[j], rec.ref.refname);
 		j++;
 	}
 
@@ -92,7 +92,7 @@  static void test_block_read_write(void)
 		n = block_iter_next(&it, &rec);
 		EXPECT(n == 0);
 
-		EXPECT_STREQ(names[i], ref.refname);
+		EXPECT_STREQ(names[i], rec.ref.refname);
 
 		want.len--;
 		n = block_reader_seek(&br, &it, &want);
@@ -100,7 +100,7 @@  static void test_block_read_write(void)
 
 		n = block_iter_next(&it, &rec);
 		EXPECT(n == 0);
-		EXPECT_STREQ(names[10 * (i / 10)], ref.refname);
+		EXPECT_STREQ(names[10 * (i / 10)], rec.ref.refname);
 
 		block_iter_close(&it);
 	}
diff --git a/reftable/generic.c b/reftable/generic.c
index 7a8a738d860..7f00aa9cdbe 100644
--- a/reftable/generic.c
+++ b/reftable/generic.c
@@ -7,6 +7,7 @@  https://developers.google.com/open-source/licenses/bsd
 */
 
 #include "basics.h"
+#include "constants.h"
 #include "record.h"
 #include "generic.h"
 #include "reftable-iterator.h"
@@ -15,23 +16,21 @@  https://developers.google.com/open-source/licenses/bsd
 int reftable_table_seek_ref(struct reftable_table *tab,
 			    struct reftable_iterator *it, const char *name)
 {
-	struct reftable_ref_record ref = {
-		.refname = (char *)name,
-	};
-	struct reftable_record rec = { NULL };
-	reftable_record_from_ref(&rec, &ref);
+	struct reftable_record rec = { .type = BLOCK_TYPE_REF,
+				       .ref = {
+					       .refname = (char *)name,
+				       } };
 	return tab->ops->seek_record(tab->table_arg, it, &rec);
 }
 
 int reftable_table_seek_log(struct reftable_table *tab,
 			    struct reftable_iterator *it, const char *name)
 {
-	struct reftable_log_record log = {
-		.refname = (char *)name,
-		.update_index = ~((uint64_t)0),
-	};
-	struct reftable_record rec = { NULL };
-	reftable_record_from_log(&rec, &log);
+	struct reftable_record rec = { .type = BLOCK_TYPE_LOG,
+				       .log = {
+					       .refname = (char *)name,
+					       .update_index = ~((uint64_t)0),
+				       } };
 	return tab->ops->seek_record(tab->table_arg, it, &rec);
 }
 
@@ -129,17 +128,19 @@  void reftable_iterator_destroy(struct reftable_iterator *it)
 int reftable_iterator_next_ref(struct reftable_iterator *it,
 			       struct reftable_ref_record *ref)
 {
-	struct reftable_record rec = { NULL };
-	reftable_record_from_ref(&rec, ref);
-	return iterator_next(it, &rec);
+	struct reftable_record rec = { .type = BLOCK_TYPE_REF };
+	int err = iterator_next(it, &rec);
+	*ref = rec.ref;
+	return err;
 }
 
 int reftable_iterator_next_log(struct reftable_iterator *it,
 			       struct reftable_log_record *log)
 {
-	struct reftable_record rec = { NULL };
-	reftable_record_from_log(&rec, log);
-	return iterator_next(it, &rec);
+	struct reftable_record rec = { .type = BLOCK_TYPE_LOG };
+	int err = iterator_next(it, &rec);
+	*log = rec.log;
+	return err;
 }
 
 int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)
diff --git a/reftable/iter.c b/reftable/iter.c
index 93d04f735b8..fedf5d8ab0b 100644
--- a/reftable/iter.c
+++ b/reftable/iter.c
@@ -32,7 +32,7 @@  static int filtering_ref_iterator_next(void *iter_arg,
 				       struct reftable_record *rec)
 {
 	struct filtering_ref_iterator *fri = iter_arg;
-	struct reftable_ref_record *ref = rec->data;
+	struct reftable_ref_record *ref = &rec->ref;
 	int err = 0;
 	while (1) {
 		err = reftable_iterator_next_ref(&fri->it, ref);
@@ -127,7 +127,7 @@  static int indexed_table_ref_iter_next_block(struct indexed_table_ref_iter *it)
 static int indexed_table_ref_iter_next(void *p, struct reftable_record *rec)
 {
 	struct indexed_table_ref_iter *it = p;
-	struct reftable_ref_record *ref = rec->data;
+	struct reftable_ref_record *ref = &rec->ref;
 
 	while (1) {
 		int err = block_iter_next(&it->cur, rec);
diff --git a/reftable/merged.c b/reftable/merged.c
index e5b53da6db3..e69955d7c2d 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -22,7 +22,7 @@  static int merged_iter_init(struct merged_iter *mi)
 {
 	int i = 0;
 	for (i = 0; i < mi->stack_len; i++) {
-		struct reftable_record rec = reftable_new_record(mi->typ);
+		struct reftable_record rec = reftable_record_for(mi->typ);
 		int err = iterator_next(&mi->stack[i], &rec);
 		if (err < 0) {
 			return err;
@@ -30,7 +30,7 @@  static int merged_iter_init(struct merged_iter *mi)
 
 		if (err > 0) {
 			reftable_iterator_destroy(&mi->stack[i]);
-			reftable_record_destroy(&rec);
+			reftable_record_release(&rec);
 		} else {
 			struct pq_entry e = {
 				.rec = rec,
@@ -57,18 +57,17 @@  static void merged_iter_close(void *p)
 static int merged_iter_advance_nonnull_subiter(struct merged_iter *mi,
 					       size_t idx)
 {
-	struct reftable_record rec = reftable_new_record(mi->typ);
 	struct pq_entry e = {
-		.rec = rec,
+		.rec = reftable_record_for(mi->typ),
 		.index = idx,
 	};
-	int err = iterator_next(&mi->stack[idx], &rec);
+	int err = iterator_next(&mi->stack[idx], &e.rec);
 	if (err < 0)
 		return err;
 
 	if (err > 0) {
 		reftable_iterator_destroy(&mi->stack[idx]);
-		reftable_record_destroy(&rec);
+		reftable_record_release(&e.rec);
 		return 0;
 	}
 
@@ -126,11 +125,11 @@  static int merged_iter_next_entry(struct merged_iter *mi,
 		if (err < 0) {
 			return err;
 		}
-		reftable_record_destroy(&top.rec);
+		reftable_record_release(&top.rec);
 	}
 
 	reftable_record_copy_from(rec, &entry.rec, hash_size(mi->hash_id));
-	reftable_record_destroy(&entry.rec);
+	reftable_record_release(&entry.rec);
 	strbuf_release(&entry_key);
 	return 0;
 }
@@ -246,7 +245,7 @@  static int merged_table_seek_record(struct reftable_merged_table *mt,
 		sizeof(struct reftable_iterator) * mt->stack_len);
 	struct merged_iter merged = {
 		.stack = iters,
-		.typ = reftable_record_type(rec),
+		.typ = rec->type,
 		.hash_id = mt->hash_id,
 		.suppress_deletions = mt->suppress_deletions,
 	};
@@ -290,11 +289,12 @@  int reftable_merged_table_seek_ref(struct reftable_merged_table *mt,
 				   struct reftable_iterator *it,
 				   const char *name)
 {
-	struct reftable_ref_record ref = {
-		.refname = (char *)name,
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_REF,
+		.ref = {
+			.refname = (char *)name,
+		},
 	};
-	struct reftable_record rec = { NULL };
-	reftable_record_from_ref(&rec, &ref);
 	return merged_table_seek_record(mt, it, &rec);
 }
 
@@ -302,12 +302,13 @@  int reftable_merged_table_seek_log_at(struct reftable_merged_table *mt,
 				      struct reftable_iterator *it,
 				      const char *name, uint64_t update_index)
 {
-	struct reftable_log_record log = {
-		.refname = (char *)name,
-		.update_index = update_index,
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_LOG,
+		.log = {
+			.refname = (char *)name,
+			.update_index = update_index,
+		}
 	};
-	struct reftable_record rec = { NULL };
-	reftable_record_from_log(&rec, &log);
 	return merged_table_seek_record(mt, it, &rec);
 }
 
diff --git a/reftable/pq.c b/reftable/pq.c
index efc474017a2..96ca6dd37b3 100644
--- a/reftable/pq.c
+++ b/reftable/pq.c
@@ -74,6 +74,7 @@  struct pq_entry merged_iter_pqueue_remove(struct merged_iter_pqueue *pq)
 void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, struct pq_entry e)
 {
 	int i = 0;
+
 	if (pq->len == pq->cap) {
 		pq->cap = 2 * pq->cap + 1;
 		pq->heap = reftable_realloc(pq->heap,
@@ -98,7 +99,7 @@  void merged_iter_pqueue_release(struct merged_iter_pqueue *pq)
 {
 	int i = 0;
 	for (i = 0; i < pq->len; i++) {
-		reftable_record_destroy(&pq->heap[i].rec);
+		reftable_record_release(&pq->heap[i].rec);
 	}
 	FREE_AND_NULL(pq->heap);
 	pq->len = pq->cap = 0;
diff --git a/reftable/pq_test.c b/reftable/pq_test.c
index c9bb05e37b7..89d069d674d 100644
--- a/reftable/pq_test.c
+++ b/reftable/pq_test.c
@@ -31,7 +31,7 @@  static void test_pq(void)
 	int N = ARRAY_SIZE(names) - 1;
 
 	struct merged_iter_pqueue pq = { NULL };
-	const char *last = NULL;
+	char *last = NULL;
 
 	int i = 0;
 	for (i = 0; i < N; i++) {
@@ -42,12 +42,14 @@  static void test_pq(void)
 
 	i = 1;
 	do {
-		struct reftable_record rec =
-			reftable_new_record(BLOCK_TYPE_REF);
-		struct pq_entry e = { 0 };
-
-		reftable_record_as_ref(&rec)->refname = names[i];
-		e.rec = rec;
+		struct pq_entry e = {
+			.rec = {
+				.type = BLOCK_TYPE_REF,
+				.ref = {
+					.refname = names[i],
+				}
+			}
+		};
 		merged_iter_pqueue_add(&pq, e);
 		merged_iter_pqueue_check(pq);
 		i = (i * 7) % N;
@@ -55,19 +57,18 @@  static void test_pq(void)
 
 	while (!merged_iter_pqueue_is_empty(pq)) {
 		struct pq_entry e = merged_iter_pqueue_remove(&pq);
-		struct reftable_ref_record *ref =
-			reftable_record_as_ref(&e.rec);
-
+		struct reftable_record *rec = &e.rec;
 		merged_iter_pqueue_check(pq);
 
+		EXPECT(rec->type == BLOCK_TYPE_REF);
 		if (last) {
-			EXPECT(strcmp(last, ref->refname) < 0);
+			EXPECT(strcmp(last, rec->ref.refname) < 0);
 		}
-		last = ref->refname;
-		ref->refname = NULL;
-		reftable_free(ref);
+		// this is names[i], so don't dealloc.
+		last = rec->ref.refname;
+		rec->ref.refname = NULL;
+		reftable_record_release(rec);
 	}
-
 	for (i = 0; i < N; i++) {
 		reftable_free(names[i]);
 	}
diff --git a/reftable/reader.c b/reftable/reader.c
index 8d308d858f8..0159f131122 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -238,9 +238,8 @@  static int table_iter_next_in_block(struct table_iter *ti,
 				    struct reftable_record *rec)
 {
 	int res = block_iter_next(&ti->bi, rec);
-	if (res == 0 && reftable_record_type(rec) == BLOCK_TYPE_REF) {
-		((struct reftable_ref_record *)rec->data)->update_index +=
-			ti->r->min_update_index;
+	if (res == 0 && rec->type == BLOCK_TYPE_REF) {
+		rec->ref.update_index += ti->r->min_update_index;
 	}
 
 	return res;
@@ -345,7 +344,7 @@  static int table_iter_next_block(struct table_iter *dest,
 
 static int table_iter_next(struct table_iter *ti, struct reftable_record *rec)
 {
-	if (reftable_record_type(rec) != ti->typ)
+	if (rec->type != ti->typ)
 		return REFTABLE_API_ERROR;
 
 	while (1) {
@@ -437,8 +436,7 @@  static int reader_start(struct reftable_reader *r, struct table_iter *ti,
 static int reader_seek_linear(struct reftable_reader *r, struct table_iter *ti,
 			      struct reftable_record *want)
 {
-	struct reftable_record rec =
-		reftable_new_record(reftable_record_type(want));
+	struct reftable_record rec = reftable_record_for(want->type);
 	struct strbuf want_key = STRBUF_INIT;
 	struct strbuf got_key = STRBUF_INIT;
 	struct table_iter next = TABLE_ITER_INIT;
@@ -475,7 +473,7 @@  static int reader_seek_linear(struct reftable_reader *r, struct table_iter *ti,
 
 done:
 	block_iter_close(&next.bi);
-	reftable_record_destroy(&rec);
+	reftable_record_release(&rec);
 	strbuf_release(&want_key);
 	strbuf_release(&got_key);
 	return err;
@@ -485,38 +483,40 @@  static int reader_seek_indexed(struct reftable_reader *r,
 			       struct reftable_iterator *it,
 			       struct reftable_record *rec)
 {
-	struct reftable_index_record want_index = { .last_key = STRBUF_INIT };
-	struct reftable_record want_index_rec = { NULL };
-	struct reftable_index_record index_result = { .last_key = STRBUF_INIT };
-	struct reftable_record index_result_rec = { NULL };
+	struct reftable_record want_index = {
+		.type = BLOCK_TYPE_INDEX,
+		.idx = { .last_key = STRBUF_INIT }
+	};
+	struct reftable_record index_result = {
+		.type = BLOCK_TYPE_INDEX,
+		.idx = { .last_key = STRBUF_INIT },
+	};
 	struct table_iter index_iter = TABLE_ITER_INIT;
 	struct table_iter next = TABLE_ITER_INIT;
 	int err = 0;
 
-	reftable_record_key(rec, &want_index.last_key);
-	reftable_record_from_index(&want_index_rec, &want_index);
-	reftable_record_from_index(&index_result_rec, &index_result);
-
-	err = reader_start(r, &index_iter, reftable_record_type(rec), 1);
+	reftable_record_key(rec, &want_index.idx.last_key);
+	err = reader_start(r, &index_iter, rec->type, 1);
 	if (err < 0)
 		goto done;
 
-	err = reader_seek_linear(r, &index_iter, &want_index_rec);
+	err = reader_seek_linear(r, &index_iter, &want_index);
 	while (1) {
-		err = table_iter_next(&index_iter, &index_result_rec);
+		err = table_iter_next(&index_iter, &index_result);
 		table_iter_block_done(&index_iter);
 		if (err != 0)
 			goto done;
 
-		err = reader_table_iter_at(r, &next, index_result.offset, 0);
+		err = reader_table_iter_at(r, &next, index_result.idx.offset,
+					   0);
 		if (err != 0)
 			goto done;
 
-		err = block_iter_seek(&next.bi, &want_index.last_key);
+		err = block_iter_seek(&next.bi, &want_index.idx.last_key);
 		if (err < 0)
 			goto done;
 
-		if (next.typ == reftable_record_type(rec)) {
+		if (next.typ == rec->type) {
 			err = 0;
 			break;
 		}
@@ -540,8 +540,8 @@  static int reader_seek_indexed(struct reftable_reader *r,
 done:
 	block_iter_close(&next.bi);
 	table_iter_close(&index_iter);
-	reftable_record_release(&want_index_rec);
-	reftable_record_release(&index_result_rec);
+	reftable_record_release(&want_index);
+	reftable_record_release(&index_result);
 	return err;
 }
 
@@ -549,15 +549,14 @@  static int reader_seek_internal(struct reftable_reader *r,
 				struct reftable_iterator *it,
 				struct reftable_record *rec)
 {
-	struct reftable_reader_offsets *offs =
-		reader_offsets_for(r, reftable_record_type(rec));
+	struct reftable_reader_offsets *offs = reader_offsets_for(r, rec->type);
 	uint64_t idx = offs->index_offset;
 	struct table_iter ti = TABLE_ITER_INIT;
 	int err = 0;
 	if (idx > 0)
 		return reader_seek_indexed(r, it, rec);
 
-	err = reader_start(r, &ti, reftable_record_type(rec), 0);
+	err = reader_start(r, &ti, rec->type, 0);
 	if (err < 0)
 		return err;
 	err = reader_seek_linear(r, &ti, rec);
@@ -576,7 +575,7 @@  static int reader_seek_internal(struct reftable_reader *r,
 static int reader_seek(struct reftable_reader *r, struct reftable_iterator *it,
 		       struct reftable_record *rec)
 {
-	uint8_t typ = reftable_record_type(rec);
+	uint8_t typ = rec->type;
 
 	struct reftable_reader_offsets *offs = reader_offsets_for(r, typ);
 	if (!offs->is_present) {
@@ -590,11 +589,12 @@  static int reader_seek(struct reftable_reader *r, struct reftable_iterator *it,
 int reftable_reader_seek_ref(struct reftable_reader *r,
 			     struct reftable_iterator *it, const char *name)
 {
-	struct reftable_ref_record ref = {
-		.refname = (char *)name,
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_REF,
+		.ref = {
+			.refname = (char *)name,
+		},
 	};
-	struct reftable_record rec = { NULL };
-	reftable_record_from_ref(&rec, &ref);
 	return reader_seek(r, it, &rec);
 }
 
@@ -602,12 +602,13 @@  int reftable_reader_seek_log_at(struct reftable_reader *r,
 				struct reftable_iterator *it, const char *name,
 				uint64_t update_index)
 {
-	struct reftable_log_record log = {
-		.refname = (char *)name,
-		.update_index = update_index,
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_LOG,
+		.log = {
+			.refname = (char *)name,
+			.update_index = update_index,
+		}
 	};
-	struct reftable_record rec = { NULL };
-	reftable_record_from_log(&rec, &log);
 	return reader_seek(r, it, &rec);
 }
 
@@ -649,31 +650,33 @@  static int reftable_reader_refs_for_indexed(struct reftable_reader *r,
 					    struct reftable_iterator *it,
 					    uint8_t *oid)
 {
-	struct reftable_obj_record want = {
-		.hash_prefix = oid,
-		.hash_prefix_len = r->object_id_len,
+	struct reftable_record want = {
+		.type = BLOCK_TYPE_OBJ,
+		.obj = {
+			.hash_prefix = oid,
+			.hash_prefix_len = r->object_id_len,
+		},
 	};
-	struct reftable_record want_rec = { NULL };
 	struct reftable_iterator oit = { NULL };
-	struct reftable_obj_record got = { NULL };
-	struct reftable_record got_rec = { NULL };
+	struct reftable_record got = {
+		.type = BLOCK_TYPE_OBJ,
+		.obj = { 0 },
+	};
 	int err = 0;
 	struct indexed_table_ref_iter *itr = NULL;
 
 	/* Look through the reverse index. */
-	reftable_record_from_obj(&want_rec, &want);
-	err = reader_seek(r, &oit, &want_rec);
+	err = reader_seek(r, &oit, &want);
 	if (err != 0)
 		goto done;
 
 	/* read out the reftable_obj_record */
-	reftable_record_from_obj(&got_rec, &got);
-	err = iterator_next(&oit, &got_rec);
+	err = iterator_next(&oit, &got);
 	if (err < 0)
 		goto done;
 
-	if (err > 0 ||
-	    memcmp(want.hash_prefix, got.hash_prefix, r->object_id_len)) {
+	if (err > 0 || memcmp(want.obj.hash_prefix, got.obj.hash_prefix,
+			      r->object_id_len)) {
 		/* didn't find it; return empty iterator */
 		iterator_set_empty(it);
 		err = 0;
@@ -681,15 +684,15 @@  static int reftable_reader_refs_for_indexed(struct reftable_reader *r,
 	}
 
 	err = new_indexed_table_ref_iter(&itr, r, oid, hash_size(r->hash_id),
-					 got.offsets, got.offset_len);
+					 got.obj.offsets, got.obj.offset_len);
 	if (err < 0)
 		goto done;
-	got.offsets = NULL;
+	got.obj.offsets = NULL;
 	iterator_from_indexed_table_ref_iter(it, itr);
 
 done:
 	reftable_iterator_destroy(&oit);
-	reftable_record_release(&got_rec);
+	reftable_record_release(&got);
 	return err;
 }
 
diff --git a/reftable/record.c b/reftable/record.c
index 8536bd03aa9..b665feb709f 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -15,6 +15,10 @@  https://developers.google.com/open-source/licenses/bsd
 #include "reftable-error.h"
 #include "basics.h"
 
+static struct reftable_record_vtable *
+reftable_record_vtable(struct reftable_record *rec);
+static void *reftable_record_data(struct reftable_record *rec);
+
 int get_var_int(uint64_t *dest, struct string_view *in)
 {
 	int ptr = 0;
@@ -926,58 +930,6 @@  static struct reftable_record_vtable reftable_log_record_vtable = {
 	.is_deletion = &reftable_log_record_is_deletion_void,
 };
 
-struct reftable_record reftable_new_record(uint8_t typ)
-{
-	struct reftable_record rec = { NULL };
-	switch (typ) {
-	case BLOCK_TYPE_REF: {
-		struct reftable_ref_record *r =
-			reftable_calloc(sizeof(struct reftable_ref_record));
-		reftable_record_from_ref(&rec, r);
-		return rec;
-	}
-
-	case BLOCK_TYPE_OBJ: {
-		struct reftable_obj_record *r =
-			reftable_calloc(sizeof(struct reftable_obj_record));
-		reftable_record_from_obj(&rec, r);
-		return rec;
-	}
-	case BLOCK_TYPE_LOG: {
-		struct reftable_log_record *r =
-			reftable_calloc(sizeof(struct reftable_log_record));
-		reftable_record_from_log(&rec, r);
-		return rec;
-	}
-	case BLOCK_TYPE_INDEX: {
-		struct reftable_index_record empty = { .last_key =
-							       STRBUF_INIT };
-		struct reftable_index_record *r =
-			reftable_calloc(sizeof(struct reftable_index_record));
-		*r = empty;
-		reftable_record_from_index(&rec, r);
-		return rec;
-	}
-	}
-	abort();
-	return rec;
-}
-
-/* clear out the record, yielding the reftable_record data that was
- * encapsulated. */
-static void *reftable_record_yield(struct reftable_record *rec)
-{
-	void *p = rec->data;
-	rec->data = NULL;
-	return p;
-}
-
-void reftable_record_destroy(struct reftable_record *rec)
-{
-	reftable_record_release(rec);
-	reftable_free(reftable_record_yield(rec));
-}
-
 static void reftable_index_record_key(const void *r, struct strbuf *dest)
 {
 	const struct reftable_index_record *rec = r;
@@ -1055,91 +1007,47 @@  static struct reftable_record_vtable reftable_index_record_vtable = {
 
 void reftable_record_key(struct reftable_record *rec, struct strbuf *dest)
 {
-	rec->ops->key(rec->data, dest);
-}
-
-uint8_t reftable_record_type(struct reftable_record *rec)
-{
-	return rec->ops->type;
+	reftable_record_vtable(rec)->key(reftable_record_data(rec), dest);
 }
 
 int reftable_record_encode(struct reftable_record *rec, struct string_view dest,
 			   int hash_size)
 {
-	return rec->ops->encode(rec->data, dest, hash_size);
+	return reftable_record_vtable(rec)->encode(reftable_record_data(rec),
+						   dest, hash_size);
 }
 
 void reftable_record_copy_from(struct reftable_record *rec,
 			       struct reftable_record *src, int hash_size)
 {
-	assert(src->ops->type == rec->ops->type);
+	assert(src->type == rec->type);
 
-	rec->ops->copy_from(rec->data, src->data, hash_size);
+	reftable_record_vtable(rec)->copy_from(reftable_record_data(rec),
+					       reftable_record_data(src),
+					       hash_size);
 }
 
 uint8_t reftable_record_val_type(struct reftable_record *rec)
 {
-	return rec->ops->val_type(rec->data);
+	return reftable_record_vtable(rec)->val_type(reftable_record_data(rec));
 }
 
 int reftable_record_decode(struct reftable_record *rec, struct strbuf key,
 			   uint8_t extra, struct string_view src, int hash_size)
 {
-	return rec->ops->decode(rec->data, key, extra, src, hash_size);
+	return reftable_record_vtable(rec)->decode(reftable_record_data(rec),
+						   key, extra, src, hash_size);
 }
 
 void reftable_record_release(struct reftable_record *rec)
 {
-	rec->ops->release(rec->data);
+	reftable_record_vtable(rec)->release(reftable_record_data(rec));
 }
 
 int reftable_record_is_deletion(struct reftable_record *rec)
 {
-	return rec->ops->is_deletion(rec->data);
-}
-
-void reftable_record_from_ref(struct reftable_record *rec,
-			      struct reftable_ref_record *ref_rec)
-{
-	assert(!rec->ops);
-	rec->data = ref_rec;
-	rec->ops = &reftable_ref_record_vtable;
-}
-
-void reftable_record_from_obj(struct reftable_record *rec,
-			      struct reftable_obj_record *obj_rec)
-{
-	assert(!rec->ops);
-	rec->data = obj_rec;
-	rec->ops = &reftable_obj_record_vtable;
-}
-
-void reftable_record_from_index(struct reftable_record *rec,
-				struct reftable_index_record *index_rec)
-{
-	assert(!rec->ops);
-	rec->data = index_rec;
-	rec->ops = &reftable_index_record_vtable;
-}
-
-void reftable_record_from_log(struct reftable_record *rec,
-			      struct reftable_log_record *log_rec)
-{
-	assert(!rec->ops);
-	rec->data = log_rec;
-	rec->ops = &reftable_log_record_vtable;
-}
-
-struct reftable_ref_record *reftable_record_as_ref(struct reftable_record *rec)
-{
-	assert(reftable_record_type(rec) == BLOCK_TYPE_REF);
-	return rec->data;
-}
-
-struct reftable_log_record *reftable_record_as_log(struct reftable_record *rec)
-{
-	assert(reftable_record_type(rec) == BLOCK_TYPE_LOG);
-	return rec->data;
+	return reftable_record_vtable(rec)->is_deletion(
+		reftable_record_data(rec));
 }
 
 static int hash_equal(uint8_t *a, uint8_t *b, int hash_size)
@@ -1210,3 +1118,53 @@  void string_view_consume(struct string_view *s, int n)
 	s->buf += n;
 	s->len -= n;
 }
+
+static void *reftable_record_data(struct reftable_record *rec)
+{
+	switch (rec->type) {
+	case BLOCK_TYPE_REF:
+		return &rec->ref;
+	case BLOCK_TYPE_LOG:
+		return &rec->log;
+	case BLOCK_TYPE_INDEX:
+		return &rec->idx;
+	case BLOCK_TYPE_OBJ:
+		return &rec->obj;
+	}
+	abort();
+}
+
+static struct reftable_record_vtable *
+reftable_record_vtable(struct reftable_record *rec)
+{
+	switch (rec->type) {
+	case BLOCK_TYPE_REF:
+		return &reftable_ref_record_vtable;
+	case BLOCK_TYPE_LOG:
+		return &reftable_log_record_vtable;
+	case BLOCK_TYPE_INDEX:
+		return &reftable_index_record_vtable;
+	case BLOCK_TYPE_OBJ:
+		return &reftable_obj_record_vtable;
+	}
+	abort();
+}
+
+struct reftable_record reftable_record_for(uint8_t typ)
+{
+	struct reftable_record clean_idx = {
+		.type = BLOCK_TYPE_INDEX,
+		.idx = {
+			.last_key = STRBUF_INIT,
+		},
+	};
+	struct reftable_record clean = {
+		.type = typ,
+	};
+
+	if (typ == BLOCK_TYPE_INDEX) {
+		return clean_idx;
+	}
+
+	return clean;
+}
diff --git a/reftable/record.h b/reftable/record.h
index 498e8c50bf4..a702c67e857 100644
--- a/reftable/record.h
+++ b/reftable/record.h
@@ -60,18 +60,9 @@  struct reftable_record_vtable {
 	int (*is_deletion)(const void *rec);
 };
 
-/* record is a generic wrapper for different types of records. */
-struct reftable_record {
-	void *data;
-	struct reftable_record_vtable *ops;
-};
-
 /* returns true for recognized block types. Block start with the block type. */
 int reftable_is_block_type(uint8_t typ);
 
-/* creates a malloced record of the given type. Dispose with record_destroy */
-struct reftable_record reftable_new_record(uint8_t typ);
-
 /* Encode `key` into `dest`. Sets `is_restart` to indicate a restart. Returns
  * number of bytes written. */
 int reftable_encode_key(int *is_restart, struct string_view dest,
@@ -97,8 +88,26 @@  struct reftable_obj_record {
 	int offset_len;
 };
 
+/* record is a generic wrapper for different types of records. It is normally
+ * created on the stack, or embedded within another struct. If the type is
+ * known, a fresh instance can be initialized explicitly. Otherwise, use
+ * reftable_record_for() to initialize generically (as the index_record is not
+ * valid as 0-initialized structure)
+ */
+struct reftable_record {
+	uint8_t type;
+	union {
+		struct reftable_ref_record ref;
+		struct reftable_log_record log;
+		struct reftable_obj_record obj;
+		struct reftable_index_record idx;
+	};
+};
+
 /* see struct record_vtable */
 
+/* return an initialized record for the given type */
+struct reftable_record reftable_record_for(uint8_t typ);
 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,
@@ -111,25 +120,9 @@  int reftable_record_decode(struct reftable_record *rec, struct strbuf key,
 			   int hash_size);
 int reftable_record_is_deletion(struct reftable_record *rec);
 
-/* zeroes out the embedded record */
+/* frees and zeroes out the embedded record */
 void reftable_record_release(struct reftable_record *rec);
 
-/* clear and deallocate embedded record, and zero `rec`. */
-void reftable_record_destroy(struct reftable_record *rec);
-
-/* initialize generic records from concrete records. The generic record should
- * be zeroed out. */
-void reftable_record_from_obj(struct reftable_record *rec,
-			      struct reftable_obj_record *objrec);
-void reftable_record_from_index(struct reftable_record *rec,
-				struct reftable_index_record *idxrec);
-void reftable_record_from_ref(struct reftable_record *rec,
-			      struct reftable_ref_record *refrec);
-void reftable_record_from_log(struct reftable_record *rec,
-			      struct reftable_log_record *logrec);
-struct reftable_ref_record *reftable_record_as_ref(struct reftable_record *ref);
-struct reftable_log_record *reftable_record_as_log(struct reftable_record *ref);
-
 /* for qsort. */
 int reftable_ref_record_compare_name(const void *a, const void *b);
 
diff --git a/reftable/record_test.c b/reftable/record_test.c
index f4ad7cace41..23abfb037b3 100644
--- a/reftable/record_test.c
+++ b/reftable/record_test.c
@@ -16,24 +16,22 @@ 
 
 static void test_copy(struct reftable_record *rec)
 {
-	struct reftable_record copy =
-		reftable_new_record(reftable_record_type(rec));
+	struct reftable_record copy = reftable_record_for(rec->type);
+
 	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)) {
+	switch (copy.type) {
 	case BLOCK_TYPE_REF:
-		EXPECT(reftable_ref_record_equal(reftable_record_as_ref(&copy),
-						 reftable_record_as_ref(rec),
+		EXPECT(reftable_ref_record_equal(&copy.ref, &rec->ref,
 						 GIT_SHA1_RAWSZ));
 		break;
 	case BLOCK_TYPE_LOG:
-		EXPECT(reftable_log_record_equal(reftable_record_as_log(&copy),
-						 reftable_record_as_log(rec),
+		EXPECT(reftable_log_record_equal(&copy.log, &rec->log,
 						 GIT_SHA1_RAWSZ));
 		break;
 	}
-	reftable_record_destroy(&copy);
+	reftable_record_release(&copy);
 }
 
 static void test_varint_roundtrip(void)
@@ -106,61 +104,60 @@  static void test_reftable_ref_record_roundtrip(void)
 	int i = 0;
 
 	for (i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {
-		struct reftable_ref_record in = { NULL };
-		struct reftable_ref_record out = { NULL };
-		struct reftable_record rec_out = { NULL };
+		struct reftable_record in = {
+			.type = BLOCK_TYPE_REF,
+		};
+		struct reftable_record out = {
+			.type = BLOCK_TYPE_REF
+		};
 		struct strbuf key = STRBUF_INIT;
-		struct reftable_record rec = { NULL };
 		uint8_t buffer[1024] = { 0 };
 		struct string_view dest = {
 			.buf = buffer,
 			.len = sizeof(buffer),
 		};
-
 		int n, m;
 
-		in.value_type = i;
+		in.ref.value_type = i;
 		switch (i) {
 		case REFTABLE_REF_DELETION:
 			break;
 		case REFTABLE_REF_VAL1:
-			in.value.val1 = reftable_malloc(GIT_SHA1_RAWSZ);
-			set_hash(in.value.val1, 1);
+			in.ref.value.val1 = reftable_malloc(GIT_SHA1_RAWSZ);
+			set_hash(in.ref.value.val1, 1);
 			break;
 		case REFTABLE_REF_VAL2:
-			in.value.val2.value = reftable_malloc(GIT_SHA1_RAWSZ);
-			set_hash(in.value.val2.value, 1);
-			in.value.val2.target_value =
+			in.ref.value.val2.value =
+				reftable_malloc(GIT_SHA1_RAWSZ);
+			set_hash(in.ref.value.val2.value, 1);
+			in.ref.value.val2.target_value =
 				reftable_malloc(GIT_SHA1_RAWSZ);
-			set_hash(in.value.val2.target_value, 2);
+			set_hash(in.ref.value.val2.target_value, 2);
 			break;
 		case REFTABLE_REF_SYMREF:
-			in.value.symref = xstrdup("target");
+			in.ref.value.symref = xstrdup("target");
 			break;
 		}
-		in.refname = xstrdup("refs/heads/master");
+		in.ref.refname = xstrdup("refs/heads/master");
 
-		reftable_record_from_ref(&rec, &in);
-		test_copy(&rec);
+		test_copy(&in);
 
-		EXPECT(reftable_record_val_type(&rec) == i);
+		EXPECT(reftable_record_val_type(&in) == i);
 
-		reftable_record_key(&rec, &key);
-		n = reftable_record_encode(&rec, dest, GIT_SHA1_RAWSZ);
+		reftable_record_key(&in, &key);
+		n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
 		EXPECT(n > 0);
 
 		/* decode into a non-zero reftable_record to test for leaks. */
-
-		reftable_record_from_ref(&rec_out, &out);
-		m = reftable_record_decode(&rec_out, key, i, dest,
-					   GIT_SHA1_RAWSZ);
+		m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ);
 		EXPECT(n == m);
 
-		EXPECT(reftable_ref_record_equal(&in, &out, GIT_SHA1_RAWSZ));
-		reftable_record_release(&rec_out);
+		EXPECT(reftable_ref_record_equal(&in.ref, &out.ref,
+						 GIT_SHA1_RAWSZ));
+		reftable_record_release(&in);
 
 		strbuf_release(&key);
-		reftable_ref_record_release(&in);
+		reftable_record_release(&out);
 	}
 }
 
@@ -213,7 +210,9 @@  static void test_reftable_log_record_roundtrip(void)
 	set_test_hash(in[0].value.update.new_hash, 1);
 	set_test_hash(in[0].value.update.old_hash, 2);
 	for (i = 0; i < ARRAY_SIZE(in); i++) {
-		struct reftable_record rec = { NULL };
+		struct reftable_record rec = {
+			.type = BLOCK_TYPE_LOG
+		};
 		struct strbuf key = STRBUF_INIT;
 		uint8_t buffer[1024] = { 0 };
 		struct string_view dest = {
@@ -221,8 +220,10 @@  static void test_reftable_log_record_roundtrip(void)
 			.len = sizeof(buffer),
 		};
 		/* populate out, to check for leaks. */
-		struct reftable_log_record out = {
-			.refname = xstrdup("old name"),
+		struct reftable_record out = {
+			.type = BLOCK_TYPE_LOG,
+			.log = {
+				.refname = xstrdup("old name"),
 			.value_type = REFTABLE_LOG_UPDATE,
 			.value = {
 				.update = {
@@ -233,11 +234,11 @@  static void test_reftable_log_record_roundtrip(void)
 					.message = xstrdup("old message"),
 				},
 			},
+			},
 		};
-		struct reftable_record rec_out = { NULL };
 		int n, m, valtype;
 
-		reftable_record_from_log(&rec, &in[i]);
+		rec.log = in[i];
 
 		test_copy(&rec);
 
@@ -245,16 +246,16 @@  static void test_reftable_log_record_roundtrip(void)
 
 		n = reftable_record_encode(&rec, dest, GIT_SHA1_RAWSZ);
 		EXPECT(n >= 0);
-		reftable_record_from_log(&rec_out, &out);
 		valtype = reftable_record_val_type(&rec);
-		m = reftable_record_decode(&rec_out, key, valtype, dest,
+		m = reftable_record_decode(&out, key, valtype, dest,
 					   GIT_SHA1_RAWSZ);
 		EXPECT(n == m);
 
-		EXPECT(reftable_log_record_equal(&in[i], &out, GIT_SHA1_RAWSZ));
+		EXPECT(reftable_log_record_equal(&in[i], &out.log,
+						 GIT_SHA1_RAWSZ));
 		reftable_log_record_release(&in[i]);
 		strbuf_release(&key);
-		reftable_record_release(&rec_out);
+		reftable_record_release(&out);
 	}
 }
 
@@ -322,47 +323,51 @@  static void test_reftable_obj_record_roundtrip(void)
 					       } };
 	int i = 0;
 	for (i = 0; i < ARRAY_SIZE(recs); i++) {
-		struct reftable_obj_record in = recs[i];
 		uint8_t buffer[1024] = { 0 };
 		struct string_view dest = {
 			.buf = buffer,
 			.len = sizeof(buffer),
 		};
-		struct reftable_record rec = { NULL };
+		struct reftable_record in = {
+			.type = BLOCK_TYPE_OBJ,
+			.obj = recs[i],
+		};
 		struct strbuf key = STRBUF_INIT;
-		struct reftable_obj_record out = { NULL };
-		struct reftable_record rec_out = { NULL };
+		struct reftable_record out = {
+			.type = BLOCK_TYPE_OBJ
+		};
 		int n, m;
 		uint8_t extra;
 
-		reftable_record_from_obj(&rec, &in);
-		test_copy(&rec);
-		reftable_record_key(&rec, &key);
-		n = reftable_record_encode(&rec, dest, GIT_SHA1_RAWSZ);
+		test_copy(&in);
+		reftable_record_key(&in, &key);
+		n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
 		EXPECT(n > 0);
-		extra = reftable_record_val_type(&rec);
-		reftable_record_from_obj(&rec_out, &out);
-		m = reftable_record_decode(&rec_out, key, extra, dest,
+		extra = reftable_record_val_type(&in);
+		m = reftable_record_decode(&out, key, extra, dest,
 					   GIT_SHA1_RAWSZ);
 		EXPECT(n == m);
 
-		EXPECT(in.hash_prefix_len == out.hash_prefix_len);
-		EXPECT(in.offset_len == out.offset_len);
+		EXPECT(in.obj.hash_prefix_len == out.obj.hash_prefix_len);
+		EXPECT(in.obj.offset_len == out.obj.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(!memcmp(in.obj.hash_prefix, out.obj.hash_prefix,
+			       in.obj.hash_prefix_len));
+		EXPECT(0 == memcmp(in.obj.offsets, out.obj.offsets,
+				   sizeof(uint64_t) * in.obj.offset_len));
 		strbuf_release(&key);
-		reftable_record_release(&rec_out);
+		reftable_record_release(&out);
 	}
 }
 
 static void test_reftable_index_record_roundtrip(void)
 {
-	struct reftable_index_record in = {
-		.offset = 42,
-		.last_key = STRBUF_INIT,
+	struct reftable_record in = {
+		.type = BLOCK_TYPE_INDEX,
+		.idx = {
+			.offset = 42,
+			.last_key = STRBUF_INIT,
+		},
 	};
 	uint8_t buffer[1024] = { 0 };
 	struct string_view dest = {
@@ -370,31 +375,30 @@  static void test_reftable_index_record_roundtrip(void)
 		.len = sizeof(buffer),
 	};
 	struct strbuf key = STRBUF_INIT;
-	struct reftable_record rec = { NULL };
-	struct reftable_index_record out = { .last_key = STRBUF_INIT };
-	struct reftable_record out_rec = { NULL };
+	struct reftable_record out = {
+		.type = BLOCK_TYPE_INDEX,
+		.idx = { .last_key = STRBUF_INIT },
+	};
 	int n, m;
 	uint8_t extra;
 
-	strbuf_addstr(&in.last_key, "refs/heads/master");
-	reftable_record_from_index(&rec, &in);
-	reftable_record_key(&rec, &key);
-	test_copy(&rec);
+	strbuf_addstr(&in.idx.last_key, "refs/heads/master");
+	reftable_record_key(&in, &key);
+	test_copy(&in);
 
-	EXPECT(0 == strbuf_cmp(&key, &in.last_key));
-	n = reftable_record_encode(&rec, dest, GIT_SHA1_RAWSZ);
+	EXPECT(0 == strbuf_cmp(&key, &in.idx.last_key));
+	n = reftable_record_encode(&in, dest, GIT_SHA1_RAWSZ);
 	EXPECT(n > 0);
 
-	extra = reftable_record_val_type(&rec);
-	reftable_record_from_index(&out_rec, &out);
-	m = reftable_record_decode(&out_rec, key, extra, dest, GIT_SHA1_RAWSZ);
+	extra = reftable_record_val_type(&in);
+	m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ);
 	EXPECT(m == n);
 
-	EXPECT(in.offset == out.offset);
+	EXPECT(in.idx.offset == out.idx.offset);
 
-	reftable_record_release(&out_rec);
+	reftable_record_release(&out);
 	strbuf_release(&key);
-	strbuf_release(&in.last_key);
+	strbuf_release(&in.idx.last_key);
 }
 
 int record_test_main(int argc, const char *argv[])
diff --git a/reftable/writer.c b/reftable/writer.c
index 3ca721e9f64..df256f3a8b5 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -221,10 +221,10 @@  static int writer_add_record(struct reftable_writer *w,
 	strbuf_reset(&w->last_key);
 	strbuf_addbuf(&w->last_key, &key);
 	if (w->block_writer == NULL) {
-		writer_reinit_block_writer(w, reftable_record_type(rec));
+		writer_reinit_block_writer(w, rec->type);
 	}
 
-	assert(block_writer_type(w->block_writer) == reftable_record_type(rec));
+	assert(block_writer_type(w->block_writer) == rec->type);
 
 	if (block_writer_add(w->block_writer, rec) == 0) {
 		err = 0;
@@ -236,7 +236,7 @@  static int writer_add_record(struct reftable_writer *w,
 		goto done;
 	}
 
-	writer_reinit_block_writer(w, reftable_record_type(rec));
+	writer_reinit_block_writer(w, rec->type);
 	err = block_writer_add(w->block_writer, rec);
 	if (err < 0) {
 		goto done;
@@ -251,8 +251,10 @@  done:
 int reftable_writer_add_ref(struct reftable_writer *w,
 			    struct reftable_ref_record *ref)
 {
-	struct reftable_record rec = { NULL };
-	struct reftable_ref_record copy = *ref;
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_REF,
+		.ref = *ref,
+	};
 	int err = 0;
 
 	if (ref->refname == NULL)
@@ -261,8 +263,7 @@  int reftable_writer_add_ref(struct reftable_writer *w,
 	    ref->update_index > w->max_update_index)
 		return REFTABLE_API_ERROR;
 
-	reftable_record_from_ref(&rec, &copy);
-	copy.update_index -= w->min_update_index;
+	rec.ref.update_index -= w->min_update_index;
 
 	err = writer_add_record(w, &rec);
 	if (err < 0)
@@ -301,7 +302,10 @@  int reftable_writer_add_refs(struct reftable_writer *w,
 static int reftable_writer_add_log_verbatim(struct reftable_writer *w,
 					    struct reftable_log_record *log)
 {
-	struct reftable_record rec = { NULL };
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_LOG,
+		.log = *log,
+	};
 	if (w->block_writer &&
 	    block_writer_type(w->block_writer) == BLOCK_TYPE_REF) {
 		int err = writer_finish_public_section(w);
@@ -311,8 +315,6 @@  static int reftable_writer_add_log_verbatim(struct reftable_writer *w,
 
 	w->next -= w->pending_padding;
 	w->pending_padding = 0;
-
-	reftable_record_from_log(&rec, log);
 	return writer_add_record(w, &rec);
 }
 
@@ -393,8 +395,10 @@  static int writer_finish_section(struct reftable_writer *w)
 		w->index_len = 0;
 		w->index_cap = 0;
 		for (i = 0; i < idx_len; i++) {
-			struct reftable_record rec = { NULL };
-			reftable_record_from_index(&rec, idx + i);
+			struct reftable_record rec = {
+				.type = BLOCK_TYPE_INDEX,
+				.idx = idx[i],
+			};
 			if (block_writer_add(w->block_writer, &rec) == 0) {
 				continue;
 			}
@@ -462,17 +466,18 @@  static void write_object_record(void *void_arg, void *key)
 {
 	struct write_record_arg *arg = void_arg;
 	struct obj_index_tree_node *entry = key;
-	struct reftable_obj_record obj_rec = {
-		.hash_prefix = (uint8_t *)entry->hash.buf,
-		.hash_prefix_len = arg->w->stats.object_id_len,
-		.offsets = entry->offsets,
-		.offset_len = entry->offset_len,
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_OBJ,
+		.obj = {
+			.hash_prefix = (uint8_t *)entry->hash.buf,
+			.hash_prefix_len = arg->w->stats.object_id_len,
+			.offsets = entry->offsets,
+			.offset_len = entry->offset_len,
+		}
 	};
-	struct reftable_record rec = { NULL };
 	if (arg->err < 0)
 		goto done;
 
-	reftable_record_from_obj(&rec, &obj_rec);
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 	if (arg->err == 0)
 		goto done;
@@ -485,7 +490,8 @@  static void write_object_record(void *void_arg, void *key)
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 	if (arg->err == 0)
 		goto done;
-	obj_rec.offset_len = 0;
+
+	rec.obj.offset_len = 0;
 	arg->err = block_writer_add(arg->w->block_writer, &rec);
 
 	/* Should be able to write into a fresh block. */