Web lists-archives.com

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






On 11/2/2017 6:24 PM, Jonathan Tan wrote:
On Thu,  2 Nov 2017 20:20:44 +0000
Jeff Hostetler <git@xxxxxxxxxxxxxxxxx> wrote:

From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>

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