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

[Patrick Steinhardt <ps@pks.im>]

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

On Thu, Mar 07, 2024 at 01:38:56PM +0100, Toon claes wrote:
> > 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

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