Re: [PATCH 4/4] reftable/stack: register compacted tables as tempfiles

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 7, 2024, 12:58 UTC
Message-ID: <Zem5_Z171kzCkRK5@tanuki>
In-Reply-To: <87sf12fc7z.fsf@to1.studio>

On Thu, Mar 07, 2024 at 01:38:56PM +0100, Toon claes wrote:

Show 25 quoted lines

> > diff --git a/reftable/stack.c b/reftable/stack.c
> > index 977336b7d5..40129da16c 100644
> > --- a/reftable/stack.c
> > +++ b/reftable/stack.c
> > @@ -827,51 +827,57 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
> >  
> >  static int stack_compact_locked(struct reftable_stack *st,
> >  				size_t first, size_t last,
> > -				struct strbuf *temp_tab,
> > -				struct reftable_log_expiry_config *config)
> > +				struct reftable_log_expiry_config *config,
> > +				struct tempfile **temp_table_out)
> >  {
> >  	struct strbuf next_name = STRBUF_INIT;
> > -	int tab_fd = -1;
> > +	struct strbuf table_path = STRBUF_INIT;
> >  	struct reftable_writer *wr = NULL;
> > +	struct tempfile *temp_table;
> > +	int temp_table_fd;
> 
> Just one small nit, if you don't mind? In PATCH 2/4 you use
> `struct tempfile *tab_file` and `int tab_fd`. I would like to see
> consistency and use similar names. Personally I don't like table being
> shortened to "tab", and I think you feel the same as you've renamed the
> parameter from this function.

Yeah, I'm always a proponent of descriptive, unabbreviated names and would thus rather want to use "temp_table" or something like that. But in 2/4 I decided to stick with "tab_file" because there is a bunch of already existing variables in that function which are called similar -- "temp_tab_file_name", "tab_file_name" and "tab_fd". But renaming all of them to ensure function-level consistency would create a lot of churn.

I'll thus instead rename variables over here to their abbreviated versions.

Patrick