Re: [PATCH 4/4] ref-filter: use separate cache for contains_tag_algo

From: Jeff King <peff@peff.net>
Date: Mar 12, 2017, 11:12 UTC
Message-ID: <20170312111217.swapjaazeg3wbuyt@sigill.intra.peff.net>
In-Reply-To: <CACBZZX5A46-ua9y_HxqrqycQviR0uGnStnaPwH9jukzPVak6ug@mail.gmail.com>

On Sat, Mar 11, 2017 at 09:01:25PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 17 quoted lines

> On Thu, Mar 9, 2017 at 2:29 PM, Jeff King <peff@peff.net> wrote:
> > [...]
> > @@ -1874,6 +1886,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> >                 broken = 1;
> >         filter->kind = type & FILTER_REFS_KIND_MASK;
> >
> > +       init_contains_cache(&ref_cbdata.contains_cache);
> > +
> >         /*  Simple per-ref filtering */
> > [...]
> >
> > +       clear_contains_cache(&ref_cbdata.contains_cache);
> >
> >         /*  Filters that need revision walking */
> >         if (filter->merge_commit)
> 
> Shouldn't both of those be guarded by a "if (filter->with_commit)" test?

I thought about that while writing it, but decided that it was not worth complicating the initialization and cleanup with conditionals. The initialization is on par with what we would normally do with a static struct initializer, and the cleanup is a noop if the cache wasn't touched.

So my thinking was that it didn't matter either way, and dropping the conditional was one less thing for readers of the code to have to worry about.

That said, I'm not opposed to doing it the other way.

> That init/clear codepath is rather light, but it seems to me that we
> can avoid it entirely if filter->with_commit isn't defined. I've
> tested this locally and it still passes all tests.

Yes, it should be correct either way.

-Peff