On Wed, Feb 19, 2025 at 02:20:15PM +0100, Patrick Steinhardt wrote:
> 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
Oh, my mistake. I omit that part during review... Sorry here.
