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

[Jonathan Tan <jonathantanmy@google.com>]

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.