Re: [PATCH v1 1/5] list-objects-filter: refactor into a context struct

From: Matthew DeVore <matvore@comcast.net>
Date: May 28, 2019, 18:48 UTC
Message-ID: <20190528184801.GA4556@comcast.net>
In-Reply-To: <20190524004938.GB46998@google.com>

On Thu, May 23, 2019 at 05:49:38PM -0700, Emily Shaffer wrote:

Show 7 quoted lines

> This commit message might read more easily on its own if you define
> "this bundle of data" at least once. Since there are things being moved
> from both list-objects-filter.c (filter_blobs_none_data) and
> list-objects-filter.h (list_objects_filter_result and filter_free_fn)
> into the new struct in list-objects-filter.h, it's not immediately clear
> to me from the diff what's going on.
>

I will take care to define the context struct in the commit message and the code comments.

I was originally intending to put this in the context struct: data common to all filter types used when executing a filter. But now I'm leaning toward just using the struct as a grab-bag for execution data for *all* filter types. This would be more similar to how the list_objects_filter_options struct works, and it would save having to define a special free_fn for each (actually, most) filter types. I'll play around with this idea and probably put it in the next roll-up (comments welcome, though).

Show 20 quoted lines

> > -static void *filter_blobs_none__init(
> > -	struct oidset *omitted,
> > +static void filter_blobs_none__init(
> >  	struct list_objects_filter_options *filter_options,
> > -	filter_object_fn *filter_fn,
> > -	filter_free_fn *filter_free_fn)
> > +	struct filter_context *ctx)
> >  {
> > -	struct filter_blobs_none_data *d = xcalloc(1, sizeof(*d));
> > -	d->omits = omitted;
> > -
> > -	*filter_fn = filter_blobs_none;
> > -	*filter_free_fn = free;
> > -	return d;
> > +	ctx->filter_fn = filter_blobs_none;
> 
> I think you want to set ctx->free_fn here too, right? It seems like
> you're not setting ctx->omitted anymore because you'd be reading that
> information in from ctx->omitted (so it's redundant).
>

Not necessary because the blobs:none filter type doesn't have filter-specific data for it. blobs:none is unique in that regard.