Re: [PATCH 07/13] reftable/merged: split up initialization and seeking of records

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

On Fri, May 10, 2024 at 02:18:16PM -0500, Justin Tobler wrote:
> On 24/05/08 01:04PM, Patrick Steinhardt wrote:
> > To initialize a `struct merged_iter`, we need to seek all subiterators
> > to the wanted record and then add their results to the priority queue
> > used to sort the records. This logic is split up across two functions,
> > `merged_table_seek_record()` and `merged_table_iter()`. The scope of
> 
> Did we mean `merged_iter_init` instead of `merged_table_iter()` here?

Indeed, good catch.

> > @@ -246,32 +230,33 @@ static int merged_table_seek_record(struct reftable_merged_table *mt,
> >  				    struct reftable_iterator *it,
> >  				    struct reftable_record *rec)
> >  {
> > -	struct merged_iter merged = {
> > -		.typ = reftable_record_type(rec),
> > -		.hash_id = mt->hash_id,
> > -		.suppress_deletions = mt->suppress_deletions,
> > -		.advance_index = -1,
> > -	};
> > -	struct merged_iter *p;
> > +	struct merged_iter merged, *p;
> >  	int err;
> >  
> > -	REFTABLE_CALLOC_ARRAY(merged.subiters, mt->stack_len);
> > +	merged_iter_init(&merged, mt);
> > +
> >  	for (size_t i = 0; i < mt->stack_len; i++) {
> > +		reftable_record_init(&merged.subiters[merged.stack_len].rec,
> > +				     reftable_record_type(rec));
> 
> I find it somewhat confusing how we increment the subiterator index
> here. I assume this is because when a record is not found we don't want
> to add it to the index. Might be nice to add a comment here explaining
> this.
> 
> Edit: Looks like you address this in the next commit so I guess we are
> good :)

Seconded. And yes, as you noticed, this is one of the reasons why I
change this in the next patch.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]