Subject: Re: [PATCH 10/10] reftable: make reftable_record a tagged union

From: Junio C Hamano <[email-masked]>
Date: Wed, 08 Dec 2021 08:47:02 -0800

Derrick Stolee <[email-masked]> writes:

> On 12/7/2021 12:45 PM, Han-Wen Nienhuys via GitGitGadget wrote:
>> From: Han-Wen Nienhuys <[email-masked]>
>> 
>> 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 <[email-masked]>
>> ---
>>  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...