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

From: Patrick Steinhardt <[email-masked]>
Date: Wed, 19 Feb 2025 14:20:15 +0100

On Wed, Feb 19, 2025 at 02:17:07PM +0100, Patrick Steinhardt wrote:
> On Wed, Feb 19, 2025 at 09:06:58PM +0800, shejialuo wrote:
> > On Wed, Feb 19, 2025 at 01:59:13PM +0100, Patrick Steinhardt wrote:
> > > Regarding the question why to even rename `ref_iterator_abort()` itself:
> > > this is done to avoid confusion going forward. Previously it really only
> > > had to be called when you actually wanted to abort an ongoing iteration
> > > over its yielded references. This is not the case anymore, and now you
> > > have to call it unconditionally after you're done with the iterator. So
> > > while the naming previously made sense, now it doesn't anymore.
> > > 
> > 
> > Good point, I didn't realise this part. Thanks for the detailed
> > explanation. I will continue to review the later patches. However, I
> > won't touch the oid part, because I am not familiar with this. By the
> > way, I think we miss out one thing in this patch:
> > 
> > We forget to free the dir iterator defined in the
> > "files-backend.c::files_fsck_refs_dir". I have just remembered that I
> > use dir iterator when checking the ref consistency.
> 
> Hm, good point. Why doesn't CI complain about this leak...? I'll
> investigate, thanks for the hint!

Wait, no, I had been looking at the wrong branch. We do free the
iterator:

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 11a620ea11a..859f1c11941 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3837,6 +3820,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
                ret = error(_("failed to iterate over '%s'"), sb.buf);

 out:
+       dir_iterator_free(iter);
        strbuf_release(&sb);
        strbuf_release(&refname);
        return ret;

Patrick