Re: [PATCH 10/10] reftable: make reftable_record a tagged union
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 8, 2021, 16:47 UTC
- Message-ID
- <xmqqv8zz2ge1.fsf@gitster.g>
- In-Reply-To
- <072ff09c-9174-e769-7ebb-4bb248199337@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
Show 30 quoted lines
> 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...