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

From: shejialuo <[email-masked]>
Date: Wed, 19 Feb 2025 21:06:58 +0800

On Wed, Feb 19, 2025 at 01:59:13PM +0100, Patrick Steinhardt wrote:
> On Wed, Feb 19, 2025 at 08:41:03PM +0800, shejialuo wrote:
> > But there is one thing I want to argue with. I don't think we need to
> > rename "abort" callback to "release" and also "ref_iterator_abort" to
> > "ref_iterator_free" for the following reasons:
> > 
> > 1. We never call "release" expect in the "ref_iterator_free" function.
> > For other exposed functions "ref_iterator_advance", "ref_iterator_peel"
> > and the original "ref_iterator_abort". We will just call the registered
> > callback "advance", "peel" or "abort" via virtual table. I somehow think
> > we should follow this pattern. But I don't know actually.
> > 2. When I read the patch yesterday, I really wonder what is the
> > difference between "release" and "free". Why do we only change the
> > "ref_iterator_abort" to "ref_iterator_free" but for the callback, we
> > rename "abort" to "release". I know that you want to distinguish to
> > emphasis that we won't free the iterator but only release its resource
> > for ref iterator. But could abort also mean this?
> 
> The difference between "release" and "free" is explicitly documented in
> our CodingGuidelines. Quoting the relevant parts:
> 
>     - `S_release()` releases a structure's contents without freeing the
>       structure.
> 
>     - `S_free()` releases a structure's contents and frees the
>       structure.
> 
> So following these coding guidelines, we have to call the underlying
> implementations that are specific to the iterators `release()` because
> they don't free the iterator itself. And because the generic part _does_
> free the iterator itself in addition to releasing its state, it has to
> be called `free()`.
> 

Make sense.

> 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.

Thanks,
Jialuo

> Patrick