Re: [PATCH v4 01/10] list-objects-filter: make API easier to use

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 21, 2019, 22:58 UTC
Message-ID: <20190621225838.226321-1-jonathantanmy@google.com>
In-Reply-To: <70568c42ae6d59dacbb36ffb8e4a8828b6595158.1560558910.git.matvore@google.com>

Show 7 quoted lines

> Make the list-objects-filter.h API more opaque and easier to use. This
> prepares for combined filter support, where filters will be created and
> used in a new context.
> 
> Helped-by: Jeff Hostetler <git@jeffhostetler.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Matthew DeVore <matvore@google.com>

So what happens is that filter_fn, filter_free_fn, and filter_data are encapsulated into one opaque object, and users will now use filter_fn and filter_free_fn through other functions that we expose, allowing us to add some conveniences that currently have to be repeated at each call site.

I would prefer the following commit message:

  list-objects-filter: encapsulate filter components

  Encapsulate filter_fn, filter_free_fn, and filter_data into its own
  opaque struct.

  Due to opaqueness, filter_fn and filter_free_fn can no longer be
  accessed directly by users. Currently, all usages of filter_fn are
  guarded by a necessary check:

    (obj->flags & NOT_USER_GIVEN) && filter_fn

  Take the opportunity to include this check into the new function
  list_objects_filter__filter_object(), so that we no longer need to
  write this check at every caller of the filter function.

  Also, the init functions in list-objects-filter.c no longer need to
  confusingly return the filter constituents in various places
  (filter_fn and filter_free_fn as out parameters, and filter_data as
  the function's return value); they can just initialize the "struct
  filter" passed in.

Show 16 quoted lines

> +enum list_objects_filter_result list_objects_filter__filter_object(
> +	struct repository *r,
> +	enum list_objects_filter_situation filter_situation,
> +	struct object *obj,
> +	const char *pathname,
> +	const char *filename,
> +	struct filter *filter)
> +{
> +	if (filter && (obj->flags & NOT_USER_GIVEN))
> +		return filter->filter_object_fn(r, filter_situation, obj,
> +						pathname, filename,
> +						filter->filter_data);
> +	/*
> +	 * No filter is active or user gave object explicitly. Choose default
> +	 * behavior based on filter situation.
> +	 */

This part is when we do not need to apply the filter (or none exists). I think the comment will be better if stated more explicitly:

  No filter is active or user gave object explicitly. In this case,
  always show the object (except when LOFS_END_TREE, since this tree had
  already been shown when LOFS_BEGIN_TREE).

> +	if (filter_situation == LOFS_END_TREE)
> +		return 0;
> +	return LOFR_MARK_SEEN | LOFR_DO_SHOW;
> +}