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

From: Emily Shaffer <emilyshaffer@google.com>
Date: May 24, 2019, 00:49 UTC
Message-ID: <20190524004938.GB46998@google.com>
In-Reply-To: <341bc55d4a3f5438b1523525cf683f96d75e8c3e.1558484115.git.matvore@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]

Show 15 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).

>  }

[snip]

Show 8 quoted lines

> -/*
> - * 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