From: Jeff Hostetler <git@jeffhostetler.com>
Date: Thu, 16 Nov 2017 17:53:27 GMT
Subject: Re: [PATCH 04/14] fetch: add object filtering for partial fetch
Message-ID: <7eb6b165-77e5-78b9-e3a9-151f6a53a39a@jeffhostetler.com>
In-Reply-To: <20171103133845.b270485dd04c6c6c1b47d42a@google.com>



On 11/3/2017 4:38 PM, Jonathan Tan wrote:
>> @@ -1242,6 +1249,20 @@ static int fetch_multiple(struct string_list *list)
>>   	int i, result = 0;
>>   	struct argv_array argv = ARGV_ARRAY_INIT;
>>   
>> +	if (filter_options.choice) {
>> +		/*
>> +		 * We currently only support partial-fetches to the remote
>> +		 * used for the partial-clone because we only support 1
>> +		 * promisor remote, so we DO NOT allow explicit command
>> +		 * line filter arguments.
>> +		 *
>> +		 * Note that the loop below will spawn background fetches
>> +		 * for each remote and one of them MAY INHERIT the proper
>> +		 * partial-fetch settings, so everything is consistent.
>> +		 */
>> +		die(_("partial-fetch is not supported on multiple remotes"));
>> +	}
>> +
>>   	if (!append && !dry_run) {
>>   		int errcode = truncate_fetch_head();
>>   		if (errcode)
> 
> My intention in doing the "fetch: refactor calculation of remote list"
> patch is so that the interaction between the provided list of remotes
> and the specification of the filter can be handled using the following
> diff:
> 
>      -	if (remote)
>      +	if (remote) {
>      +		if (filter_options.choice &&
>      +		    strcmp(remote->name, repository_format_partial_clone_remote))
>      +			die(_("--blob-max-bytes can only be used with the remote configured in core.partialClone"));
>       		result = fetch_one(remote, argc, argv);
>      -	else
>      +	} else {
>      +		if (filter_options.choice)
>      +			die(_("--blob-max-bytes can only be used with the remote configured in core.partialClone"));
>       		result = fetch_multiple(&list);
>      +	}
> 
> (Ignore the "blob-max-bytes" in the error message - that needs to be
> updated.)
> 
> The GitHub link I provided above has this diff, and it seems to work.
> 

I put the filter_options.choice tests inside the fetch_{one,multiple}
routines because the former needs to be able to register partial clone
with the config and/or inherit the default filter-spec for the
promisor remote and that took more code that what can neatly fit inline
here.  This will be more apparent in my next patch series.

Jeff