Re: What's cooking in git.git (Jul 2019, #03; Fri, 12)

[Matthew DeVore <matvore@comcast.net>]

On Fri, Jul 12, 2019 at 02:02:52PM -0700, Junio C Hamano wrote:
> * md/list-objects-filter-combo (2019-06-28) 10 commits
>  - list-objects-filter-options: make parser void
>  - list-objects-filter-options: clean up use of ALLOC_GROW
>  - list-objects-filter-options: allow mult. --filter
>  - strbuf: give URL-encoding API a char predicate fn
>  - list-objects-filter-options: make filter_spec a string_list
>  - list-objects-filter-options: move error check up
>  - list-objects-filter: implement composite filters
>  - list-objects-filter-options: always supply *errbuf
>  - list-objects-filter: put omits set in filter struct
>  - list-objects-filter: encapsulate filter components
> 
>  The list-objects-filter API (used to create a sparse/lazy clone)
>  learned to take a combined filter specification.
> 
>  There is a bit of interaction with cc/multi-promisor topic, whose
>  conflict resolution I have no confidence in X-<.  Extra sets of
>  eyes are appreciated.
> 

Sorry for the delay. I was on vacation and then catching up for a week after I
got back. I uploaded a merged commit here:

https://github.com/matvore/git/tree/filts

And the merged file itself (only this one had conflicts) is here:

https://github.com/matvore/git/blob/filts/list-objects-filter.c

I'll comment on the conflicts:

> diff --cc list-objects-filter-options.c
> index ba1425cb4a,28c571f922..0000000000
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@@ -1,25 -1,19 +1,30 @@@
>   #include "cache.h"
>   #include "commit.h"
>   #include "config.h"
>   #include "revision.h"
>   #include "argv-array.h"
>   #include "list-objects.h"
>   #include "list-objects-filter.h"
>   #include "list-objects-filter-options.h"
> ++<<<<<<< md/list-objects-filter-combo
>  +#include "trace.h"
>  +#include "url.h"
>  +
>  +static int parse_combine_filter(
>  +	struct list_objects_filter_options *filter_options,
>  +	const char *arg,
>  +	struct strbuf *errbuf);
> ++||||||| merged common ancestors
> ++=======
> + #include "promisor-remote.h"
> ++>>>>>>> cc/multi-promisor
>   

This portion is trivial to merge - just move the "promisor-remote.h" inclusion
to the correct spot.

>   /*
>    * Parse value of the argument to the "filter" keyword.
>    * On the command line this looks like:
>    *       --filter=<arg>
>    * and in the pack protocol as:
>    *       "filter" SP <arg>
>    *
>    * The filter keyword will be used by many commands.
>    * See Documentation/rev-list-options.txt for allowed values for <arg>.
> @@@ -29,22 -23,33 +34,49 @@@
>    * expand_list_objects_filter_spec() first).  We also "intern" the arg for the
>    * convenience of the current command.
>    */
>   static int gently_parse_list_objects_filter(
>   	struct list_objects_filter_options *filter_options,
>   	const char *arg,
>   	struct strbuf *errbuf)
>   {
>   	const char *v0;
>   
> ++<<<<<<< md/list-objects-filter-combo
>  +	if (filter_options->choice)
>  +		BUG("filter_options already populated");
> ++||||||| merged common ancestors
> ++	if (filter_options->choice) {
> ++		if (errbuf) {
> ++			strbuf_addstr(
> ++				errbuf,
> ++				_("multiple filter-specs cannot be combined"));
> ++		}
> ++		return 1;
> ++	}
> ++
> ++	filter_options->filter_spec = strdup(arg);
> ++=======
> + 	if (!arg)
> + 		return 0;
> + 
> + 	if (filter_options->choice) {
> + 		if (errbuf) {
> + 			strbuf_addstr(
> + 				errbuf,
> + 				_("multiple filter-specs cannot be combined"));
> + 		}
> + 		return 1;
> + 	}
> + 
> + 	filter_options->filter_spec = strdup(arg);
> ++>>>>>>> cc/multi-promisor
  
The cc/multi-promisor branch allowed gently_parse_list_objects_filter to accept
a NULL filter spec, in which case it does nothing. So the merged branch keeps
this behavior.

md/list-objects-filter-combo changed the contract of this function such that an
attempt to combine filter specs will terminate with BUG rather than return an
error. All the callers already check filter_options.choice, so this is still
fine (it particular, I double-checked partial_clone_get_default_filter_spec and
its call site at builtin/fetch.c:1524)

>   	/*
>   	 * Record the initial filter-spec in the config as
>   	 * the default for subsequent fetches from this remote.
>   	 */
> ++<<<<<<< md/list-objects-filter-combo
>  +	core_partial_clone_filter_default =
>  +		xstrdup(expand_list_objects_filter_spec(filter_options));
>  +	git_config_set("core.partialclonefilter",
>  +		       core_partial_clone_filter_default);
> ++||||||| merged common ancestors
> ++	core_partial_clone_filter_default =
> ++		xstrdup(filter_options->filter_spec);
> ++	git_config_set("core.partialclonefilter",
> ++		       core_partial_clone_filter_default);
> ++=======
> + 	filter_name = xstrfmt("remote.%s.partialclonefilter", remote);
> + 	git_config_set(filter_name, filter_options->filter_spec);
> + 	free(filter_name);
> + 
> + 	/* Make sure the config info are reset */
> + 	promisor_remote_reinit();
> ++>>>>>>> cc/multi-promisor
>   }
>   
>   void partial_clone_get_default_filter_spec(

md/list-objects-filter-combo used the expand_list_objects_filter_spec function
to expand the filter spec string rather than get it directly. So the merged
result simply applies that alteration to cc/multi-promisor.

I checked whether callers to this function (partial_clone_register) would ever
give a null filter_options (or a non-null with a NULL filter_spec) and both
calls are guarded by "if (filter_options.choice)" so filter_options.filter_spec
should also be set.

> - 	struct list_objects_filter_options *filter_options)
> + 	struct list_objects_filter_options *filter_options,
> + 	const char *remote)
>   {
> ++<<<<<<< md/list-objects-filter-combo
>  +	struct strbuf errbuf = STRBUF_INIT;
>  +
> ++||||||| merged common ancestors
> ++=======
> + 	struct promisor_remote *promisor = promisor_remote_find(remote);
> + 
> ++>>>>>>> cc/multi-promisor

This part of the conflict is trivial to merge - just use both the variable
declarations.

  	/*
  	 * Parse default value, but silently ignore it if it is invalid.
  	 */
> ++<<<<<<< md/list-objects-filter-combo
>  +	if (!core_partial_clone_filter_default)
>  +		return;
>  +
>  +	string_list_append(&filter_options->filter_spec,
>  +			   core_partial_clone_filter_default);
>  +	gently_parse_list_objects_filter(filter_options,
>  +					 core_partial_clone_filter_default,
>  +					 &errbuf);
>  +	strbuf_release(&errbuf);
> ++||||||| merged common ancestors
> ++	if (!core_partial_clone_filter_default)
> ++		return;
> ++	gently_parse_list_objects_filter(filter_options,
> ++					 core_partial_clone_filter_default,
> ++					 NULL);
> ++=======
> + 	if (promisor)
> + 		gently_parse_list_objects_filter(filter_options,
> + 						 promisor->partial_clone_filter,
> + 						 NULL);
> ++>>>>>>> cc/multi-promisor
  }

This is a confusing conflict because cc/multi-promisor (understandably) turned
the if-guard inside-out, removing "return", since it's only guarding against
one line anyway. When I merged it, I swtiched it back (if (!promisor) return)
since creating and freeing the strbuf requires multiple lines.

I would be fine with rebasing my or Christian's change on top of the other, or
doing it as a merge.