On Mon, Feb 24, 2025 at 10:49:14PM +0800, shejialuo wrote:
> On Wed, Feb 19, 2025 at 02:23:40PM +0100, Patrick Steinhardt wrote:
> > diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> > index 6457e02c1ea..b54547d71ee 100644
> > --- a/refs/ref-cache.c
> > +++ b/refs/ref-cache.c
> > @@ -362,9 +362,7 @@ struct cache_ref_iterator {
> > struct ref_iterator base;
> >
> > /*
> > - * The number of levels currently on the stack. This is always
> > - * at least 1, because when it becomes zero the iteration is
> > - * ended and this struct is freed.
> > + * The number of levels currently on the stack.
> > */
>
> So, this value could be zero? We want to use this to optimize because
> that we don't return the empty ref iterator any more.
Now it can, yes. Before it couldn't as we returned an empty iterator in
that case, but we cannot do anymore because you cannot re-seek an empty
iterator.
> > @@ -444,6 +448,40 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
> > }
> > }
> >
> > +static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
> > + const char *prefix)
> > +{
> > + struct cache_ref_iterator *iter =
> > + (struct cache_ref_iterator *)ref_iterator;
> > + struct ref_dir *dir;
> > +
> > + dir = get_ref_dir(iter->cache->root);
> > + if (prefix && *prefix)
> > + dir = find_containing_dir(dir, prefix);
> > +
> > + if (dir) {
> > + struct cache_ref_iterator_level *level;
> > +
> > + if (iter->prime_dir)
> > + prime_ref_dir(dir, prefix);
> > + iter->levels_nr = 1;
> > + level = &iter->levels[0];
> > + level->index = -1;
> > + level->dir = dir;
> > +
> > + if (prefix && *prefix) {
> > + iter->prefix = xstrdup(prefix);
>
> Should we free the original `iter->prefix` before we assign the new
> `prefix`? I have seen this pattern in previous patch. If the caller
> calls this function multiple times, there would be memory leak.
Oh, good catch, yes.
> > + level->prefix_state = PREFIX_WITHIN_DIR;
> > + } else {
> > + level->prefix_state = PREFIX_CONTAINS_DIR;
> > + }
> > + } else {
> > + iter->levels_nr = 0;
> > + }
>
> When we cannot find the dir, we set the `iter->levels_nr = 0`. Could we
> first check
>
> if (!dir) {
> iter->levels_nr = 0;
> return 0;
> }
>
> And thus we could avoid indentation. However, it seems that we always
> return 0. So, maybe we should not change.
Good idea.
Patrick
