Re: [PATCH 07/14] refs/iterator: separate lifecycle from iteration

[Patrick Steinhardt <ps@pks.im>]

On Tue, Feb 18, 2025 at 09:13:55AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index fd001d800c6..ac3e84b2b18 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -426,6 +426,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> >  		strbuf_setlen(src, src_len);
> >  		die(_("failed to iterate over '%s'"), src->buf);
> >  	}
> > +
> > +	dir_iterator_free(iter);
> >  }
> >
> 
> A bit puzzled to see `dir_iterator_*` change here, I'm assuming it's
> linked to the 'files-backend' and perhaps similar to the changes
> mentioned about `ref_iterator_*` in the commit message. Would be nice to
> call out in the commit message too.

Yeah, that's the reason. I've added a note to the commit message.

> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > index a7b6f74b6e3..38a1956d1a8 100644
> > --- a/refs/packed-backend.c
> > +++ b/refs/packed-backend.c
> > @@ -1362,8 +1355,10 @@ static int write_with_updates(struct packed_ref_store *refs,
> >  	 */
> >  	iter = packed_ref_iterator_begin(&refs->base, "", NULL,
> >  					 DO_FOR_EACH_INCLUDE_BROKEN);
> > -	if ((ok = ref_iterator_advance(iter)) != ITER_OK)
> > +	if ((ok = ref_iterator_advance(iter)) != ITER_OK) {
> > +		ref_iterator_free(iter);
> 
> Nit: Since we don't return early here, wouldn't the `ref_iterator_free`
> at the end of the function be sufficient? I think the only early return
> when `iter == NULL` is towards the end of the function, where it might
> be better to add a `goto error`.

Yeah, the code here could definitely be improved with the new semantics.
But I was aiming to keep the required refactoring work at callsites to
the bare minimum, so I'd rather prefer to keep this unchanged.

Patrick