On Wed, Feb 19, 2025 at 02:17:07PM +0100, Patrick Steinhardt wrote:

quoted

On Wed, Feb 19, 2025 at 09:06:58PM +0800, shejialuo wrote:

quoted

On Wed, Feb 19, 2025 at 01:59:13PM +0100, Patrick Steinhardt wrote:

quoted

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