From: Jeff Hostetler <git@jeffhostetler.com>
Date: Fri, 03 Nov 2017 13:57:18 GMT
Subject: Re: [PATCH 1/9] extension.partialclone: introduce partial clone extension
Message-ID: <1db01a45-85c4-6243-c43f-9f5e50e9a6a8@jeffhostetler.com>
In-Reply-To: <20171102152427.32544b3d6149e7a7bfe840c8@google.com>



On 11/2/2017 6:24 PM, Jonathan Tan wrote:
> 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.

Yes, that is a point I wanted to ask about.  I renamed the
extensions.partialclone that you created and then I moved your
remote.<name>.blob-max-bytes setting to be in extensions too.
Moving it to core.partialclonefilter is fine.

> 
> 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?

Sure.

> 
>> @@ -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.
> 

Good point.

Thanks,
Jeff