Re: [PATCH 02/11] revision: put object filter into struct rev_info

[Junio C Hamano <gitster@pobox.com>]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  static int try_bitmap_count(struct rev_info *revs,
> -			    struct list_objects_filter_options *filter,
>  			    int filter_provided_objects)

This makes quite a lot of sense as filter is now available as
revs->filter.

>  {
>  	uint32_t commit_count = 0,
> @@ -436,7 +434,8 @@ static int try_bitmap_count(struct rev_info *revs,
>  	 */
>  	max_count = revs->max_count;
>  
> -	bitmap_git = prepare_bitmap_walk(revs, filter, filter_provided_objects);
> +	bitmap_git = prepare_bitmap_walk(revs, revs->filter,
> +					 filter_provided_objects);

And we should be able to do the same to prepare_bitmap_walk().  It
is OK if such a change comes later and not as part of this commit.

Perhaps it is deliberate.  Unlike the helpers this step touches,
namely, try_bitmap_count(), try_bitmap_traversal(), and
try_bitmap_disk_usage(), prepare_bitmap_walk() is not a file-scope
static helper and updating it will need touching many more places.

> @@ -597,13 +595,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  		}
>  
>  		if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {

#leftoverbit.  We need to remember to clean this up, now "--filter"
is well established (I am assuming this literal-string pasting is
because we didn't know what the right and final word to be used as
the option name back when this code was originally written), when
the code around here is quiescent.

> -			parse_list_objects_filter(&filter_options, arg);
> -			if (filter_options.choice && !revs.blob_objects)
> +			if (!revs.filter)
> +				CALLOC_ARRAY(revs.filter, 1);
> +			parse_list_objects_filter(revs.filter, arg);
> +			if (revs.filter->choice && !revs.blob_objects)
>  				die(_("object filtering requires --objects"));
>  			continue;

OK.  The original "filter_options" was a structure and not a pointer
to a structure; now we have a pointer to a structure in revs as a
member so we need an on-demand allocation.  CALLOC_ARRAY() instead
of xcalloc(), when we know we are creating one element and not an
array of elements whose size happens to be one, is not wrong but it
does look strange.  Was there a reason why we avoid xcalloc()?

Makes me also wonder how big the filter_options structure is;
because we will not use unbounded many revs structure, it may have
been a simpler conversion to turn a static struct into an embedded
struct member in a struct (instead of a member of a struct that is a
pointer to a struct).  That way, we did not have to ...

>  		}
>  		if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) {
> -			list_objects_filter_set_no_filter(&filter_options);
> +			if (!revs.filter)
> +				CALLOC_ARRAY(revs.filter, 1);

... repeat the on-demand allocation.  If some code used to pass
&filter_options in a parameter to helper functions, and such calling
sites get rewritten to pass the value in the revs.filter pointer,
and if revs hasn't gone through this codepath, these helper functions
will start receiving NULL in their filter_options parameter, which
they may or may not be prepared to take.  This "we get rid of a
global struct and replace it with an on-demand allocated structure,
pointer to which is stored in the revs structure" rewrite somehow
makes me nervous.

> diff --git a/revision.h b/revision.h
> index 3c58c18c63a..1ddb73ab82e 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -81,6 +81,7 @@ struct rev_cmdline_info {
>  
>  struct oidset;
>  struct topo_walk_info;
> +struct list_objects_filter_options;

Is the forward-declaration the only reason why we needed to have a
pointer to a(n opaque) struct, not an embedded struct, as a member?

>  struct rev_info {
>  	/* Starting list */
> @@ -94,6 +95,9 @@ struct rev_info {
>  	/* The end-points specified by the end user */
>  	struct rev_cmdline_info cmdline;
>  
> +	/* Object filter options. NULL for no filtering. */
> +	struct list_objects_filter_options *filter;
> +
>  	/* excluding from --branches, --refs, etc. expansion */
>  	struct string_list *ref_excludes;