From: Jeff Hostetler <git@jeffhostetler.com>
Date: Mon, 03 Jun 2019 21:51:28 GMT
Subject: Re: [PATCH v2 4/9] list-objects-filter: implement composite filters
Message-ID: <0005347e-ceed-ac9e-ad0d-b7b11bc55d38@jeffhostetler.com>
In-Reply-To: <20190601003603.90794-5-matvore@google.com>



On 5/31/2019 8:35 PM, Matthew DeVore wrote:
> Allow combining filters such that only objects accepted by all filters
> are shown. The motivation for this is to allow getting directory
> listings without also fetching blobs. This can be done by combining
> blob:none with tree:<depth>. There are massive repositories that have
> larger-than-expected trees - even if you include only a single commit.
> 
> The current usage requires passing the filter to rev-list in the
> following form:
> 
> 	--filter=<FILTER1> --filter=<FILTER2> ...
> 
> Such usage is currently an error, so giving it a meaning is backwards-
> compatible.
> 
> The URL-encoding method is being implemented before the repeated flag
> logic, and the user-facing documentation for URL-encoding is being
> withheld until the repeated flag feature is implemented. The
> URL-encoding is in general not meant to be used directly by the user,
> and it is better to describe the URL-encoding feature in terms of the
> repeated flag.
> 
> Helped-by: Emily Shaffer <emilyshaffer@google.com>
> Helped-by: Jeff Hostetler <git@jeffhostetler.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Matthew DeVore <matvore@google.com>
> ---
>   list-objects-filter-options.c       | 135 ++++++++++++++++++++++-
>   list-objects-filter-options.h       |  17 ++-
>   list-objects-filter.c               | 159 +++++++++++++++++++++++++++
>   t/t6112-rev-list-filters-objects.sh | 163 +++++++++++++++++++++++++++-
>   4 files changed, 468 insertions(+), 6 deletions(-)
> 

[...]

> +static enum list_objects_filter_result filter_combine(
> +	struct repository *r,
> +	enum list_objects_filter_situation filter_situation,
> +	struct object *obj,
> +	const char *pathname,
> +	const char *filename,
> +	struct oidset *omits,
> +	void *filter_data)
> +{
> +	struct combine_filter_data *d = filter_data;
> +	enum list_objects_filter_result combined_result =
> +		LOFR_DO_SHOW | LOFR_MARK_SEEN | LOFR_SKIP_TREE;
> +	size_t sub;
> +
> +	for (sub = 0; sub < d->nr; sub++) {
> +		enum list_objects_filter_result sub_result = process_subfilter(
> +			r, filter_situation, obj, pathname, filename,
> +			&d->sub[sub]);
> +		if (!(sub_result & LOFR_DO_SHOW))
> +			combined_result &= ~LOFR_DO_SHOW;
> +		if (!(sub_result & LOFR_MARK_SEEN))
> +			combined_result &= ~LOFR_MARK_SEEN;
> +		if (!d->sub[sub].is_skipping_tree)
> +			combined_result &= ~LOFR_SKIP_TREE;
> +	}
> +
> +	return combined_result;
> +}

This may be too subtle a point for this phase, so feel free to ignore
this.

Since we are assuming 'compose' is an AND operation, there may be an
opportunity to short-cut some of this loop for blobs.  That is, if the
object is a blob and any filter rejects it, it is omitted, so we don't
need to keep looping for that object.  (Tree objects cannot be short-cut
this way because a tree may appear at different depths or in different
sparse "cones" and may have to be reconsidered.)

So you could add an "affects blobs only" bit to the per-filter data
and try this out.  For example a "compose:blob:none+sparse:foo" should
perform better than "compose:sparse:foo+blob:none" but give the same
results.

Again, this might be premature, so feel free to disregard.
Jeff