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
