Re: [PATCH 1/9] extension.partialclone: introduce partial clone extension

> On Thu,  2 Nov 2017 20:20:44 +0000
> Jeff Hostetler <git@jeffhostetler.com> wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Introduce the ability to have missing objects in a repo.  This
>> functionality is guarded by new repository extension options:
>>      `extensions.partialcloneremote` and
>>      `extensions.partialclonefilter`.
> 
> With this, it is unclear what happens if extensions.partialcloneremote
> is not set but extensions.partialclonefilter is set. For something as
> significant as a repository extension (which Git uses to determine if it
> will even attempt to interact with a repo), I think - I would prefer
> just extensions.partialclone (or extensions.partialcloneremote, though I
> prefer the former) which determines the remote (the important part,
> which controls the dynamic object fetching), and have another option
> "core.partialclonefilter" which is only useful if
> "extensions.partialclone" is set.

> 
> I also don't think extensions.partialclonefilter (or
> core.partialclonefilter, if we use my suggestion) needs to be introduced
> so early in the patch set when it will only be used once we start
> fetching/cloning.
> 
>> +void partial_clone_utils_register(
>> +	const struct list_objects_filter_options *filter_options,
>> +	const char *remote,
>> +	const char *cmd_name)
>> +{
> 
> This function is useful once we have fetch/clone, but probably not
> before that. Since the fetch/clone patches are several patches ahead,
> could this be moved there?

> 
>> @@ -420,6 +420,19 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
>>   			;
>>   		else if (!strcmp(ext, "preciousobjects"))
>>   			data->precious_objects = git_config_bool(var, value);
>> +
>> +		else if (!strcmp(ext, KEY_PARTIALCLONEREMOTE))
>> +			if (!value)
>> +				return config_error_nonbool(var);
>> +			else
>> +				data->partial_clone_remote = xstrdup(value);
>> +
>> +		else if (!strcmp(ext, KEY_PARTIALCLONEFILTER))
>> +			if (!value)
>> +				return config_error_nonbool(var);
>> +			else
>> +				data->partial_clone_filter = xstrdup(value);
>> +
>>   		else
>>   			string_list_append(&data->unknown_extensions, ext);
>>   	} else if (strcmp(var, "core.bare") == 0) {
> 
> With a complicated block, probably better to use braces in these
> clauses.
>