On Tue, Feb 18, 2025 at 09:13:55AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@xxxxxx> 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
