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

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, May 21, 2019 at 05:21:50PM -0700, Matthew DeVore wrote:
> The next patch will create and manage filters in a new way, which means
> that this bundle of data will have to be managed at a new callsite. Make
> this bundle of data more manageable by putting it in a struct and
> making it part of the list-objects-filter module's API.

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.

[snip]
> -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).

>  }

[snip]
> -/*
> - * A filter for list-objects to omit large blobs.
> - * And to OPTIONALLY collect a list of the omitted OIDs.
> - */
> +/* A filter for list-objects to omit large blobs. */
>  struct filter_blobs_limit_data {
> -	struct oidset *omits;
>  	unsigned long max_bytes;

I suppose I don't have a good enough grasp of the architecture here to
follow why you want to move 'omits' but not 'max_bytes' into the new
struct. Maybe it will become clear as I look at the rest of your patches
:)

>  };

Most of this patch looks like a pretty straightforward conversion.
Thanks.

 - Emily