Re: [PATCH 09/12] reftable/record: reuse refname when decoding

[Patrick Steinhardt <ps@pks.im>]

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

On Wed, Feb 28, 2024 at 11:06:52AM +1100, James Liu wrote:
> On Wed Feb 14, 2024 at 6:46 PM AEDT, Patrick Steinhardt wrote:
> > This refactoring is safe to do because all functions that assigning to
> > the refname will first call `release_reftable_record()`, which will zero
> > out the complete record after releasing memory.
> 
> s/release_reftable_record/reftable_ref_record_release
> 
> > Furthermore, with this change we now perform a mostly constant number of
> > allocations when iterating.
> 
> That's awesome!
> 
> > +	SWAP(refname, r->refname);
> > +	SWAP(refname_cap, r->refname_cap);
> >  	reftable_ref_record_release(r);
> > +	SWAP(refname, r->refname);
> > +	SWAP(refname_cap, r->refname_cap);
> 
> What do you think about reversing the order of the `SWAP` arguments in
> the last two invocations? If my understanding is correct that we're
> preserving the `refname` and `refname_cap` fields so we can set them back
> into a freshly initialised `r`, reversing the args might make that intent
> a bit clearer.

Yeah, fair enough.

> Also, since we're unconditionally `memcpy`ing the key into `r->refname`
> below, can we avoid the `SWAP(refname, r->refname)` call altogether?

No, otherwise `reftable_ref_record_release()` would have already
released the underlying pointer of `r->refname` and the call to
`REFTABLE_ALLOC_GROW()` would always have to reallocate it.

Patrick

> > -	assert(hash_size > 0);
> > -
> > -	r->refname = reftable_malloc(key.len + 1);
> > +	REFTABLE_ALLOC_GROW(r->refname, key.len + 1, r->refname_cap);
> >  	memcpy(r->refname, key.buf, key.len);
> >  	r->refname[key.len] = 0;
> 

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