Re: [PATCH 02/12] reftable/merged: make `merged_iter` structure private

From: Patrick Steinhardt <ps@pks.im>
To: git@vger.kernel.org
Subject: Re: [PATCH 02/12] reftable/merged: make `merged_iter` structure private
Date: Tue, 27 Feb 2024 17:49:55 +0100	[thread overview]
Message-ID: <Zd4Ss37Yl4d-oEzE@ncase> (raw)
In-Reply-To: <qprofop624nevbicid4rplfqlanfrujyxiilfqwchrppnmw7u4@3dvw5udjtrah>

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

On Tue, Feb 20, 2024 at 12:15:23PM -0600, Justin Tobler wrote:
> On 24/02/14 08:45AM, Patrick Steinhardt wrote:
> > The `merged_iter` structure is not used anywhere outside of "merged.c",
> > but is declared in its header. Move it into the code file so that it is
> > clear that its implementation details are never exposed to anything.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  reftable/merged.c | 9 +++++++++
> >  reftable/merged.h | 9 ---------
> >  2 files changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/reftable/merged.c b/reftable/merged.c
> > index 1aa6cd31b7..12ebd732e8 100644
> > --- a/reftable/merged.c
> > +++ b/reftable/merged.c
> > @@ -17,6 +17,15 @@ license that can be found in the LICENSE file or at
> >  #include "reftable-error.h"
> >  #include "system.h"
> >  
> 
> suggestion: I think it would be nice to document a little about the
> merge iterator here at a high-level. Maybe just to explain that this
> allows iteration over multiple tables as if it were a single table.

Agreed. I have planned to invest more time into documenting the reftable
library overall, but would rather want to push this out to another patch
series.

> > +struct merged_iter {
> > +	struct reftable_iterator *stack;
> > +	uint32_t hash_id;
> > +	size_t stack_len;
> > +	uint8_t typ;
> > +	int suppress_deletions;
> > +	struct merged_iter_pqueue pq;
> > +};
> > +
> >  static int merged_iter_init(struct merged_iter *mi)
> >  {
> >  	for (size_t i = 0; i < mi->stack_len; i++) {
> > diff --git a/reftable/merged.h b/reftable/merged.h
> > index 7d9f95d27e..288ad66656 100644
> > --- a/reftable/merged.h
> > +++ b/reftable/merged.h
> > @@ -24,15 +24,6 @@ struct reftable_merged_table {
> >  	uint64_t max;
> >  };
> >  
> 
> Since we are removing `merge_iter` from the header here, I think we can
> also remove the `#include "pg.h"`.

Good catch! We can replace it with "system.h", which makes sure to
include <git-compat-util.h>.

Patrick

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