git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v4 04/10] list-objects-filter: implement composite filters

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Jun 22, 2019, 00:26 UTC
Message-ID
<20190622002626.245441-1-jonathantanmy@google.com>
In-Reply-To
<47a2680875e6f68fbf1f2e5a5a2630d263cdf426.1560558910.git.matvore@google.com>
Show 5 quoted lines
> 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.

First of all, patches 2 and 3 are straightforward and LGTM. On to patch 4...

[snip]
Show 14 quoted lines
> 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 scheme is being introduced 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.

As of this commit, we don't support such arguments passed to rev-list in this way, so I would write these paragraphs as:

  A combined filter supports any number of subfilters, and is written in
  the following form:
    combine:<filter 1>+<filter 2>+<filter 3>
  Certain non-alphanumeric characters in each filter must be
  URL-encoded.
  For now, combined filters must be specified in this form. In a
  subsequent commit, rev-list will support multiple --filter arguments
  which will have the same effect as specifying one filter argument
  starting with "combine:".
Show 26 quoted lines
> 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       | 106 ++++++++++++++++++-
>  list-objects-filter-options.h       |  17 ++-
>  list-objects-filter.c               | 159 ++++++++++++++++++++++++++++
>  t/t6112-rev-list-filters-objects.sh | 151 +++++++++++++++++++++++++-
>  url.c                               |   6 ++
>  url.h                               |   8 ++
>  6 files changed, 441 insertions(+), 6 deletions(-)
> 
> @@ -28,22 +34,20 @@ static int gently_parse_list_objects_filter(
>  	struct strbuf *errbuf)
>  {
>  	const char *v0;
>  
>  	if (filter_options->choice) {
>  		strbuf_addstr(
>  			errbuf, _("multiple filter-specs cannot be combined"));
>  		return 1;
>  	}
>  
> -	filter_options->filter_spec = strdup(arg);
> -

This line has been removed from gently_parse_list_objects_filter() because this function gains another caller that does not need it. To compensate, this line has been added to both its existing callers.

Show 32 quoted lines
> @@ -31,27 +32,37 @@ struct list_objects_filter_options {
>  	 * the filtering algorithm to use.
>  	 */
>  	enum list_objects_filter_choice choice;
>  
>  	/*
>  	 * Choice is LOFC_DISABLED because "--no-filter" was requested.
>  	 */
>  	unsigned int no_filter : 1;
>  
>  	/*
> -	 * Parsed values (fields) from within the filter-spec.  These are
> -	 * choice-specific; not all values will be defined for any given
> -	 * choice.
> +	 * BEGIN choice-specific parsed values from within the filter-spec. Only
> +	 * some values will be defined for any given choice.
>  	 */
> +
>  	struct object_id *sparse_oid_value;
>  	unsigned long blob_limit_value;
>  	unsigned long tree_exclude_depth;
> +
> +	/* LOFC_COMBINE values */
> +
> +	/* This array contains all the subfilters which this filter combines. */
> +	size_t sub_nr, sub_alloc;
> +	struct list_objects_filter_options *sub;
> +
> +	/*
> +	 * END choice-specific parsed values.
> +	 */
>  };

I still think it's cleaner to just have a "left subfilter" and "right subfilter", but I don't feel strongly about it. In any case, this is an internal detail and can always be changed in the future.

Show 6 quoted lines
> +	/*
> +	 * Optional. If this function is supplied and the filter needs to
> +	 * collect omits, then this function is called once before free_fn is
> +	 * called.
> +	 */
> +	void (*finalize_omits_fn)(struct oidset *omits, void *filter_data);

This is needed because a combined filter's omits actually lie in the subfilters. Resolving it this way means that callers must call list_objects_filter__free() before using the omits set. Can you add documentation to __init() (which is the first function to take in the omits set) and __free() describing this?

(As stated in the test below, we cannot just share one omits set amongst all the subfilters - see filter_trees_update_omits and the call site that relies on its return value.)

Here comes the tricky part...
Show 13 quoted lines
> +static int should_delegate(enum list_objects_filter_situation filter_situation,
> +			   struct object *obj,
> +			   struct subfilter *sub)
> +{
> +	if (!sub->is_skipping_tree)
> +		return 1;
> +	if (filter_situation == LOFS_END_TREE &&
> +		oideq(&obj->oid, &sub->skip_tree)) {
> +		sub->is_skipping_tree = 0;
> +		return 1;
> +	}
> +	return 0;
> +}
Optional: I think this should be called "test_and_set_skip_tree" or
something like that, made to return the inverse of its current return
value, and documented:
  Returns the value of sub->is_skipping_tree at the moment of
  invocation. If iteration is at the LOFS_END_TREE of the tree currently
  being skipped, first clears sub->is_skipping_tree before returning.
Show 36 quoted lines
> +static enum list_objects_filter_result process_subfilter(
> +	struct repository *r,
> +	enum list_objects_filter_situation filter_situation,
> +	struct object *obj,
> +	const char *pathname,
> +	const char *filename,
> +	struct subfilter *sub)
> +{
> +	enum list_objects_filter_result result;
> +
> +	/*
> +	 * Check should_delegate before oidset_contains so that
> +	 * is_skipping_tree gets unset even when the object is marked as seen.
> +	 * As of this writing, no filter uses LOFR_MARK_SEEN on trees that also
> +	 * uses LOFR_SKIP_TREE, so the ordering is only theoretically
> +	 * important. Be cautious if you change the order of the below checks
> +	 * and more filters have been added!
> +	 */
> +	if (!should_delegate(filter_situation, obj, sub))
> +		return LOFR_ZERO;
> +	if (oidset_contains(&sub->seen, &obj->oid))
> +		return LOFR_ZERO;
> +
> +	result = list_objects_filter__filter_object(
> +		r, filter_situation, obj, pathname, filename, sub->filter);
> +
> +	if (result & LOFR_MARK_SEEN)
> +		oidset_insert(&sub->seen, &obj->oid);
> +
> +	if (result & LOFR_SKIP_TREE) {
> +		sub->is_skipping_tree = 1;
> +		sub->skip_tree = obj->oid;
> +	}
> +
> +	return result;
> +}
Looks good.
Show 28 quoted lines
> +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;
> +}

And also looks good. Might be confusing for tree skipping to be communicated through is_skipping_tree instead of the return value, but is_skipping_tree needs to be set anyway for other reasons, so that's convenient.

Previous: Johannes SchindelinNext: Matthew DeVore
Message 48 of 74 in “Filter combination”
  1. 0/9 Filter combinationMatthew DeVore, Jun 1, 2019
  2. 1/9 list-objects-filter: make API easier to useMatthew DeVore, Jun 1, 2019
  3. 2/9 list-objects-filter: put omits set in filter structMatthew DeVore, Jun 1, 2019
  4. 3/9 list-objects-filter-options: always supply *errbufMatthew DeVore, Jun 1, 2019
  5. 4/9 list-objects-filter: implement composite filtersMatthew DeVore, Jun 1, 2019
  6. Jeff HostetlerJun 3, 2019
  7. Matthew DeVoreJun 6, 2019
  8. Jeff HostetlerJun 7, 2019
  9. 5/9 list-objects-filter-options: move error check upMatthew DeVore, Jun 1, 2019
  10. 6/9 list-objects-filter-options: make filter_spec a strbufMatthew DeVore, Jun 1, 2019
  11. Junio C HamanoJun 10, 2019
  12. Matthew DeVoreJun 11, 2019
  13. Junio C HamanoJun 11, 2019
  14. Matthew DeVoreJun 11, 2019
  15. Matthew DeVoreJun 11, 2019
  16. Junio C HamanoJun 11, 2019
  17. Matthew DeVoreJun 12, 2019
  18. Matthew DeVoreJun 12, 2019
  19. 7/9 list-objects-filter-options: allow mult. --filterMatthew DeVore, Jun 1, 2019
  20. 8/9 list-objects-filter-options: clean up use of ALLOC_GROWMatthew DeVore, Jun 1, 2019
  21. Jacob KellerJun 3, 2019
  22. Matthew DeVoreJun 3, 2019
  23. Jacob KellerJun 4, 2019
  24. 9/9 list-objects-filter-options: make parser voidMatthew DeVore, Jun 1, 2019
  25. Jeff HostetlerJun 3, 2019
  26. 00/10 Filter combinationMatthew DeVore, Jun 13, 2019
  27. 01/10 list-objects-filter: make API easier to useMatthew DeVore, Jun 13, 2019
  28. 02/10 list-objects-filter: put omits set in filter structMatthew DeVore, Jun 13, 2019
  29. 03/10 list-objects-filter-options: always supply *errbufMatthew DeVore, Jun 13, 2019
  30. 04/10 list-objects-filter: implement composite filtersMatthew DeVore, Jun 13, 2019
  31. 05/10 list-objects-filter-options: move error check upMatthew DeVore, Jun 13, 2019
  32. 06/10 list-objects-filter-options: make filter_spec a string_listMatthew DeVore, Jun 13, 2019
  33. 07/10 strbuf: give URL-encoding API a char predicate fnMatthew DeVore, Jun 13, 2019
  34. 08/10 list-objects-filter-options: allow mult. --filterMatthew DeVore, Jun 13, 2019
  35. 09/10 list-objects-filter-options: clean up use of ALLOC_GROWMatthew DeVore, Jun 13, 2019
  36. 10/10 list-objects-filter-options: make parser voidMatthew DeVore, Jun 13, 2019
  37. Junio C HamanoJun 14, 2019
  38. 00/10 Filter combinationMatthew DeVore, Jun 15, 2019
  39. 01/10 list-objects-filter: make API easier to useMatthew DeVore, Jun 15, 2019
  40. Jonathan TanJun 21, 2019
  41. Matthew DeVoreJun 27, 2019
  42. 02/10 list-objects-filter: put omits set in filter structMatthew DeVore, Jun 15, 2019
  43. 03/10 list-objects-filter-options: always supply *errbufMatthew DeVore, Jun 15, 2019
  44. 04/10 list-objects-filter: implement composite filtersMatthew DeVore, Jun 15, 2019
  45. Johannes SchindelinJun 18, 2019
  46. Matthew DeVoreJun 18, 2019
  47. Johannes SchindelinJun 21, 2019
  48. Jonathan TanJun 22, 2019
  49. Matthew DeVoreJun 27, 2019
  50. 05/10 list-objects-filter-options: move error check upMatthew DeVore, Jun 15, 2019
  51. 06/10 list-objects-filter-options: make filter_spec a string_listMatthew DeVore, Jun 15, 2019
  52. Jonathan TanJun 22, 2019
  53. Matthew DeVoreJun 27, 2019
  54. 07/10 strbuf: give URL-encoding API a char predicate fnMatthew DeVore, Jun 15, 2019
  55. 08/10 list-objects-filter-options: allow mult. --filterMatthew DeVore, Jun 15, 2019
  56. 09/10 list-objects-filter-options: clean up use of ALLOC_GROWMatthew DeVore, Jun 15, 2019
  57. 10/10 list-objects-filter-options: make parser voidMatthew DeVore, Jun 15, 2019
  58. Jonathan TanJun 22, 2019
  59. Matthew DeVoreJun 27, 2019
  60. Matthew DeVoreJun 27, 2019
  61. Junio C HamanoJun 18, 2019
  62. 00/10 Filter combinationMatthew DeVore, Jun 27, 2019
  63. 01/10 list-objects-filter: encapsulate filter componentsMatthew DeVore, Jun 27, 2019
  64. 03/10 list-objects-filter-options: always supply *errbufMatthew DeVore, Jun 27, 2019
  65. 02/10 list-objects-filter: put omits set in filter structMatthew DeVore, Jun 27, 2019
  66. 04/10 list-objects-filter: implement composite filtersMatthew DeVore, Jun 27, 2019
  67. 05/10 list-objects-filter-options: move error check upMatthew DeVore, Jun 27, 2019
  68. 06/10 list-objects-filter-options: make filter_spec a string_listMatthew DeVore, Jun 27, 2019
  69. 07/10 strbuf: give URL-encoding API a char predicate fnMatthew DeVore, Jun 27, 2019
  70. 08/10 list-objects-filter-options: allow mult. --filterMatthew DeVore, Jun 27, 2019
  71. 09/10 list-objects-filter-options: clean up use of ALLOC_GROWMatthew DeVore, Jun 27, 2019
  72. 10/10 list-objects-filter-options: make parser voidMatthew DeVore, Jun 27, 2019
  73. Junio C HamanoJun 28, 2019
  74. Jonathan TanJun 28, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.