[PATCH v1 0/8] Introducing odb remote

[PATCH v1 0/8] Introducing odb remote

[Christian Couder]

This is a follow up from the patch series called "Promisor remotes and
external ODB support" that I sent earlier this year.

Following discussions of these patch series, where Junio said "a
minimum s/ext/remote/ would clarify what it is", I decided to rename
"external odb" to "odb remote". I am still open to another name, but I
think that "odb remote" works well with "odb helper" that was already
used in the series and is as good or perhaps better than "remote odb",
as a "remote odb" I think would be easier to confuse with a regular
"remote".

Another obvious difference with the previous series is that this
series is only about integrating with the promisor/narrow clone work
and showing that it makes it possible to use more than one promisor
remote. Everything that is not necessary for that integration has been
removed for now (though you can still find it in one of my branches on
GitHub if you want).

This makes this patch series much shorter than the previous ones and
already useful. So hopefully this will make it possible to start
reviewing and merging it.

There is one test in patch 8/8 that shows that more than one promisor
remote can now be used, but I feel that it could be interesting to add
other such tests, so I am open to ideas in this area.

High level overview of this patch series
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  - Patch 1/8:

This makes functions in fetch-object.c return an error code, which is
necessary to later tell that they failed and try another odb remote
when there is more than one. This could also just be seen as a fix to
these functions.

  - Patch 2/8:

This introduces the minimum infrastructure for odb remotes.

  - Patches 3/8 and 4/8:

These patches implement odb_remote_get_direct() and
odb_remote_get_many_direct() using the functions from fetch-object.c.
These new functions will be used in following patches to replace the
functions from fetch-object.c.

  - Patch 5/8:

This implement odb_remote_reinit() which will be needed to reparse the
odb remote configuration.

  - Patches 6/8 and 7/8:

These patches integrate the odb remote mechanism into the
promisor/narrow clone code. The "extensions.partialclone" config
option is replaced by "odb.<name>.promisorRemote" and
"core.partialclonefilter" is replaced by "odb.<name>.partialclonefilter".

  - Patch 8/8:

This adds a test case that shows that now more than one promisor
remote can be used.

Links
~~~~~

This patch series on GitHub:

https://github.com/chriscool/git/commits/odb-remote

Version 1 and 2 of the "Promisor remotes and external ODB support" series:

https://public-inbox.org/git/20180103163403.11303-1-chriscool@tuxfamily.org/
https://public-inbox.org/git/20180319133147.15413-1-chriscool@tuxfamily.org/

Version 1 and 2 of the "Promisor remotes and external ODB support" series on GitHub:

https://github.com/chriscool/git/commits/gl-small-promisor-external-odb12
https://github.com/chriscool/git/commits/gl-small-promisor-external-odb71


Christian Couder (8):
  fetch-object: make functions return an error code
  Add initial odb remote support
  odb-remote: implement odb_remote_get_direct()
  odb-remote: implement odb_remote_get_many_direct()
  odb-remote: add odb_remote_reinit()
  Use odb_remote_get_direct() and has_external_odb()
  Use odb.origin.partialclonefilter instead of core.partialclonefilter
  t0410: test fetching from many promisor remotes

 Makefile                      |   2 +
 builtin/cat-file.c            |   5 +-
 builtin/fetch.c               |  13 ++--
 builtin/gc.c                  |   3 +-
 builtin/repack.c              |   3 +-
 cache.h                       |   2 -
 connected.c                   |   3 +-
 environment.c                 |   1 -
 fetch-object.c                |  15 +++--
 fetch-object.h                |   6 +-
 list-objects-filter-options.c |  49 ++++++++------
 list-objects-filter-options.h |   3 +-
 odb-helper.c                  |  45 +++++++++++++
 odb-helper.h                  |  18 ++++++
 odb-remote.c                  | 118 ++++++++++++++++++++++++++++++++++
 odb-remote.h                  |  10 +++
 packfile.c                    |   3 +-
 setup.c                       |   7 +-
 sha1-file.c                   |   9 +--
 t/t0410-partial-clone.sh      |  54 +++++++++++-----
 t/t5500-fetch-pack.sh         |   4 +-
 t/t5601-clone.sh              |   2 +-
 t/t5616-partial-clone.sh      |   4 +-
 unpack-trees.c                |   6 +-
 24 files changed, 306 insertions(+), 79 deletions(-)
 create mode 100644 odb-helper.c
 create mode 100644 odb-helper.h
 create mode 100644 odb-remote.c
 create mode 100644 odb-remote.h

-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 1/8] fetch-object: make functions return an error code

[Christian Couder]

The callers of the fetch_object() and fetch_objects() might
be interested in knowing if these functions succeeded or not.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 fetch-object.c | 15 +++++++++------
 fetch-object.h |  6 +++---
 sha1-file.c    |  4 ++--
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/fetch-object.c b/fetch-object.c
index 853624f811..ccc4ea7f1a 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -5,11 +5,12 @@
 #include "transport.h"
 #include "fetch-object.h"
 
-static void fetch_refs(const char *remote_name, struct ref *ref)
+static int fetch_refs(const char *remote_name, struct ref *ref)
 {
 	struct remote *remote;
 	struct transport *transport;
 	int original_fetch_if_missing = fetch_if_missing;
+	int res;
 
 	fetch_if_missing = 0;
 	remote = remote_get(remote_name);
@@ -19,18 +20,20 @@ static void fetch_refs(const char *remote_name, struct ref *ref)
 
 	transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
 	transport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, "1");
-	transport_fetch_refs(transport, ref);
+	res = transport_fetch_refs(transport, ref);
 	fetch_if_missing = original_fetch_if_missing;
+
+	return res;
 }
 
-void fetch_object(const char *remote_name, const unsigned char *sha1)
+int fetch_object(const char *remote_name, const unsigned char *sha1)
 {
 	struct ref *ref = alloc_ref(sha1_to_hex(sha1));
 	hashcpy(ref->old_oid.hash, sha1);
-	fetch_refs(remote_name, ref);
+	return fetch_refs(remote_name, ref);
 }
 
-void fetch_objects(const char *remote_name, const struct oid_array *to_fetch)
+int fetch_objects(const char *remote_name, const struct oid_array *to_fetch)
 {
 	struct ref *ref = NULL;
 	int i;
@@ -41,5 +44,5 @@ void fetch_objects(const char *remote_name, const struct oid_array *to_fetch)
 		new_ref->next = ref;
 		ref = new_ref;
 	}
-	fetch_refs(remote_name, ref);
+	return fetch_refs(remote_name, ref);
 }
diff --git a/fetch-object.h b/fetch-object.h
index 4b269d07ed..12e1f9ee70 100644
--- a/fetch-object.h
+++ b/fetch-object.h
@@ -3,9 +3,9 @@
 
 #include "sha1-array.h"
 
-extern void fetch_object(const char *remote_name, const unsigned char *sha1);
+extern int fetch_object(const char *remote_name, const unsigned char *sha1);
 
-extern void fetch_objects(const char *remote_name,
-			  const struct oid_array *to_fetch);
+extern int fetch_objects(const char *remote_name,
+			 const struct oid_array *to_fetch);
 
 #endif
diff --git a/sha1-file.c b/sha1-file.c
index 46072602ff..7d3c1ebd91 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -1290,8 +1290,8 @@ int oid_object_info_extended(const struct object_id *oid, struct object_info *oi
 		if (fetch_if_missing && repository_format_partial_clone &&
 		    !already_retried) {
 			/*
-			 * TODO Investigate haveing fetch_object() return
-			 * TODO error/success and stopping the music here.
+			 * TODO Investigate checking fetch_object() return
+			 * TODO value and stopping on error here.
 			 */
 			fetch_object(repository_format_partial_clone, real->hash);
 			already_retried = 1;
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 2/8] Add initial odb remote support

[Christian Couder]

The odb-remote.{c,h} files will contain the functions
that are called by the rest of Git mostly from
"sha1_file.c" to access the objects managed by the
odb remotes.

The odb-helper.{c,h} files will contain the functions to
actually implement communication with either the internal
functions or the external scripts or processes that will
manage and provide remote git objects.

For now only infrastructure to create helpers from the
config and to check if there are odb remotes and helpers
is implemented.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile     |  2 ++
 odb-helper.c | 16 ++++++++++++
 odb-helper.h | 13 ++++++++++
 odb-remote.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 odb-remote.h |  7 +++++
 5 files changed, 110 insertions(+)
 create mode 100644 odb-helper.c
 create mode 100644 odb-helper.h
 create mode 100644 odb-remote.c
 create mode 100644 odb-remote.h

diff --git a/Makefile b/Makefile
index ad880d1fc5..f64dea287b 100644
--- a/Makefile
+++ b/Makefile
@@ -896,6 +896,8 @@ LIB_OBJS += notes-cache.o
 LIB_OBJS += notes-merge.o
 LIB_OBJS += notes-utils.o
 LIB_OBJS += object.o
+LIB_OBJS += odb-helper.o
+LIB_OBJS += odb-remote.o
 LIB_OBJS += oidmap.o
 LIB_OBJS += oidset.o
 LIB_OBJS += packfile.o
diff --git a/odb-helper.c b/odb-helper.c
new file mode 100644
index 0000000000..b4d403ffa9
--- /dev/null
+++ b/odb-helper.c
@@ -0,0 +1,16 @@
+#include "cache.h"
+#include "object.h"
+#include "argv-array.h"
+#include "odb-helper.h"
+#include "run-command.h"
+#include "sha1-lookup.h"
+
+struct odb_helper *odb_helper_new(const char *name, int namelen)
+{
+	struct odb_helper *o;
+
+	o = xcalloc(1, sizeof(*o));
+	o->name = xmemdupz(name, namelen);
+
+	return o;
+}
diff --git a/odb-helper.h b/odb-helper.h
new file mode 100644
index 0000000000..61d2ad082b
--- /dev/null
+++ b/odb-helper.h
@@ -0,0 +1,13 @@
+#ifndef ODB_HELPER_H
+#define ODB_HELPER_H
+
+struct odb_helper {
+	const char *name;
+	const char *dealer;
+
+	struct odb_helper *next;
+};
+
+extern struct odb_helper *odb_helper_new(const char *name, int namelen);
+
+#endif /* ODB_HELPER_H */
diff --git a/odb-remote.c b/odb-remote.c
new file mode 100644
index 0000000000..e03b953ec6
--- /dev/null
+++ b/odb-remote.c
@@ -0,0 +1,72 @@
+#include "cache.h"
+#include "odb-remote.h"
+#include "odb-helper.h"
+#include "config.h"
+
+static struct odb_helper *helpers;
+static struct odb_helper **helpers_tail = &helpers;
+
+static struct odb_helper *find_or_create_helper(const char *name, int len)
+{
+	struct odb_helper *o;
+
+	for (o = helpers; o; o = o->next)
+		if (!strncmp(o->name, name, len) && !o->name[len])
+			return o;
+
+	o = odb_helper_new(name, len);
+	*helpers_tail = o;
+	helpers_tail = &o->next;
+
+	return o;
+}
+
+static int odb_remote_config(const char *var, const char *value, void *data)
+{
+	struct odb_helper *o;
+	const char *name;
+	int namelen;
+	const char *subkey;
+
+	if (parse_config_key(var, "odb", &name, &namelen, &subkey) < 0)
+		return 0;
+
+	o = find_or_create_helper(name, namelen);
+
+	if (!strcmp(subkey, "promisorremote"))
+		return git_config_string(&o->dealer, var, value);
+
+	return 0;
+}
+
+static void odb_remote_init(void)
+{
+	static int initialized;
+
+	if (initialized)
+		return;
+	initialized = 1;
+
+	git_config(odb_remote_config, NULL);
+}
+
+struct odb_helper *find_odb_helper(const char *dealer)
+{
+	struct odb_helper *o;
+
+	odb_remote_init();
+
+	if (!dealer)
+		return helpers;
+
+	for (o = helpers; o; o = o->next)
+		if (!strcmp(o->dealer, dealer))
+			return o;
+
+	return NULL;
+}
+
+int has_odb_remote(void)
+{
+	return !!find_odb_helper(NULL);
+}
diff --git a/odb-remote.h b/odb-remote.h
new file mode 100644
index 0000000000..68aa330244
--- /dev/null
+++ b/odb-remote.h
@@ -0,0 +1,7 @@
+#ifndef ODB_REMOTE_H
+#define ODB_REMOTE_H
+
+extern struct odb_helper *find_odb_helper(const char *dealer);
+extern int has_odb_remote(void);
+
+#endif /* ODB_REMOTE_H */
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 3/8] odb-remote: implement odb_remote_get_direct()

[Christian Couder]

This is implemented only in the promisor remote mode
for now by calling fetch_object().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 odb-helper.c | 14 ++++++++++++++
 odb-helper.h |  3 ++-
 odb-remote.c | 17 +++++++++++++++++
 odb-remote.h |  1 +
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/odb-helper.c b/odb-helper.c
index b4d403ffa9..2851fe2657 100644
--- a/odb-helper.c
+++ b/odb-helper.c
@@ -4,6 +4,7 @@
 #include "odb-helper.h"
 #include "run-command.h"
 #include "sha1-lookup.h"
+#include "fetch-object.h"
 
 struct odb_helper *odb_helper_new(const char *name, int namelen)
 {
@@ -14,3 +15,16 @@ struct odb_helper *odb_helper_new(const char *name, int namelen)
 
 	return o;
 }
+
+int odb_helper_get_direct(struct odb_helper *o,
+			  const unsigned char *sha1)
+{
+	int res;
+	uint64_t start = getnanotime();
+
+	res = fetch_object(o->dealer, sha1);
+
+	trace_performance_since(start, "odb_helper_get_direct");
+
+	return res;
+}
diff --git a/odb-helper.h b/odb-helper.h
index 61d2ad082b..d21b625a7e 100644
--- a/odb-helper.h
+++ b/odb-helper.h
@@ -9,5 +9,6 @@ struct odb_helper {
 };
 
 extern struct odb_helper *odb_helper_new(const char *name, int namelen);
-
+extern int odb_helper_get_direct(struct odb_helper *o,
+				 const unsigned char *sha1);
 #endif /* ODB_HELPER_H */
diff --git a/odb-remote.c b/odb-remote.c
index e03b953ec6..dbab40c0f8 100644
--- a/odb-remote.c
+++ b/odb-remote.c
@@ -70,3 +70,20 @@ int has_odb_remote(void)
 {
 	return !!find_odb_helper(NULL);
 }
+
+int odb_remote_get_direct(const unsigned char *sha1)
+{
+	struct odb_helper *o;
+
+	trace_printf("trace: odb_remote_get_direct: %s", sha1_to_hex(sha1));
+
+	odb_remote_init();
+
+	for (o = helpers; o; o = o->next) {
+		if (odb_helper_get_direct(o, sha1) < 0)
+			continue;
+		return 0;
+	}
+
+	return -1;
+}
diff --git a/odb-remote.h b/odb-remote.h
index 68aa330244..27a480fcfe 100644
--- a/odb-remote.h
+++ b/odb-remote.h
@@ -3,5 +3,6 @@
 
 extern struct odb_helper *find_odb_helper(const char *dealer);
 extern int has_odb_remote(void);
+extern int odb_remote_get_direct(const unsigned char *sha1);
 
 #endif /* ODB_REMOTE_H */
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 4/8] odb-remote: implement odb_remote_get_many_direct()

[Christian Couder]

This function will be used to get many objects from a promisor
remote.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 odb-helper.c | 15 +++++++++++++++
 odb-helper.h |  3 +++
 odb-remote.c | 17 +++++++++++++++++
 odb-remote.h |  1 +
 4 files changed, 36 insertions(+)

diff --git a/odb-helper.c b/odb-helper.c
index 2851fe2657..5fe37e6fe5 100644
--- a/odb-helper.c
+++ b/odb-helper.c
@@ -28,3 +28,18 @@ int odb_helper_get_direct(struct odb_helper *o,
 
 	return res;
 }
+
+int odb_helper_get_many_direct(struct odb_helper *o,
+			       const struct oid_array *to_get)
+{
+	int res;
+	uint64_t start;
+
+	start = getnanotime();
+
+	res = fetch_objects(o->dealer, to_get);
+
+	trace_performance_since(start, "odb_helper_get_many_direct");
+
+	return res;
+}
diff --git a/odb-helper.h b/odb-helper.h
index d21b625a7e..d63210d516 100644
--- a/odb-helper.h
+++ b/odb-helper.h
@@ -11,4 +11,7 @@ struct odb_helper {
 extern struct odb_helper *odb_helper_new(const char *name, int namelen);
 extern int odb_helper_get_direct(struct odb_helper *o,
 				 const unsigned char *sha1);
+extern int odb_helper_get_many_direct(struct odb_helper *o,
+				      const struct oid_array *to_get);
+
 #endif /* ODB_HELPER_H */
diff --git a/odb-remote.c b/odb-remote.c
index dbab40c0f8..9a1561430c 100644
--- a/odb-remote.c
+++ b/odb-remote.c
@@ -87,3 +87,20 @@ int odb_remote_get_direct(const unsigned char *sha1)
 
 	return -1;
 }
+
+int odb_remote_get_many_direct(const struct oid_array *to_get)
+{
+	struct odb_helper *o;
+
+	trace_printf("trace: odb_remote_get_many_direct: nr: %d", to_get->nr);
+
+	odb_remote_init();
+
+	for (o = helpers; o; o = o->next) {
+		if (odb_helper_get_many_direct(o, to_get) < 0)
+			continue;
+		return 0;
+	}
+
+	return -1;
+}
diff --git a/odb-remote.h b/odb-remote.h
index 27a480fcfe..e6cedde722 100644
--- a/odb-remote.h
+++ b/odb-remote.h
@@ -4,5 +4,6 @@
 extern struct odb_helper *find_odb_helper(const char *dealer);
 extern int has_odb_remote(void);
 extern int odb_remote_get_direct(const unsigned char *sha1);
+extern int odb_remote_get_many_direct(const struct oid_array *to_get);
 
 #endif /* ODB_REMOTE_H */
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 5/8] odb-remote: add odb_remote_reinit()

[Christian Couder]

We will need to reinitialize the odb remote configuration
as we will make some changes to it in a later commit when
we will detect that a remote is also an odb remote.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 odb-remote.c | 14 ++++++++++++--
 odb-remote.h |  1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/odb-remote.c b/odb-remote.c
index 9a1561430c..0a734ff379 100644
--- a/odb-remote.c
+++ b/odb-remote.c
@@ -39,17 +39,27 @@ static int odb_remote_config(const char *var, const char *value, void *data)
 	return 0;
 }
 
-static void odb_remote_init(void)
+static void odb_remote_do_init(int force)
 {
 	static int initialized;
 
-	if (initialized)
+	if (!force && initialized)
 		return;
 	initialized = 1;
 
 	git_config(odb_remote_config, NULL);
 }
 
+static inline void odb_remote_init(void)
+{
+	odb_remote_do_init(0);
+}
+
+inline void odb_remote_reinit(void)
+{
+	odb_remote_do_init(1);
+}
+
 struct odb_helper *find_odb_helper(const char *dealer)
 {
 	struct odb_helper *o;
diff --git a/odb-remote.h b/odb-remote.h
index e6cedde722..d862216a8f 100644
--- a/odb-remote.h
+++ b/odb-remote.h
@@ -1,6 +1,7 @@
 #ifndef ODB_REMOTE_H
 #define ODB_REMOTE_H
 
+extern void odb_remote_reinit(void);
 extern struct odb_helper *find_odb_helper(const char *dealer);
 extern int has_odb_remote(void);
 extern int odb_remote_get_direct(const unsigned char *sha1);
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 6/8] Use odb_remote_get_direct() and has_external_odb()

[Christian Couder]

Instead of using the repository_format_partial_clone global
and fetch_object() directly, let's use has_odb_remote() and
odb_remote_get_direct().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/cat-file.c            |  5 +++--
 builtin/fetch.c               | 11 ++++++-----
 builtin/gc.c                  |  3 ++-
 builtin/repack.c              |  3 ++-
 cache.h                       |  2 --
 connected.c                   |  3 ++-
 environment.c                 |  1 -
 list-objects-filter-options.c | 27 +++++++++++++++------------
 packfile.c                    |  3 ++-
 setup.c                       |  7 +------
 sha1-file.c                   |  9 +++++----
 t/t0410-partial-clone.sh      | 30 +++++++++++++++---------------
 t/t5500-fetch-pack.sh         |  4 ++--
 t/t5601-clone.sh              |  2 +-
 t/t5616-partial-clone.sh      |  2 +-
 unpack-trees.c                |  6 +++---
 16 files changed, 60 insertions(+), 58 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2c46d257cd..25665b206a 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -13,6 +13,7 @@
 #include "tree-walk.h"
 #include "sha1-array.h"
 #include "packfile.h"
+#include "odb-remote.h"
 
 struct batch_options {
 	int enabled;
@@ -477,8 +478,8 @@ static int batch_objects(struct batch_options *opt)
 
 		for_each_loose_object(batch_loose_object, &sa, 0);
 		for_each_packed_object(batch_packed_object, &sa, 0);
-		if (repository_format_partial_clone)
-			warning("This repository has extensions.partialClone set. Some objects may not be loaded.");
+		if (has_odb_remote())
+			warning("This repository uses an odb. Some objects may not be loaded.");
 
 		cb.opt = opt;
 		cb.expand = &data;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7ee83ac0f8..82a3e655ba 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -20,6 +20,7 @@
 #include "utf8.h"
 #include "packfile.h"
 #include "list-objects-filter-options.h"
+#include "odb-remote.h"
 
 static const char * const builtin_fetch_usage[] = {
 	N_("git fetch [<options>] [<repository> [<refspec>...]]"),
@@ -1320,7 +1321,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)
 	 * If no prior partial clone/fetch and the current fetch DID NOT
 	 * request a partial-fetch, do a normal fetch.
 	 */
-	if (!repository_format_partial_clone && !filter_options.choice)
+	if (!has_odb_remote() && !filter_options.choice)
 		return;
 
 	/*
@@ -1328,7 +1329,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)
 	 * on this repo and remember the given filter-spec as the default
 	 * for subsequent fetches to this remote.
 	 */
-	if (!repository_format_partial_clone && filter_options.choice) {
+	if (!has_odb_remote() && filter_options.choice) {
 		partial_clone_register(remote->name, &filter_options);
 		return;
 	}
@@ -1337,7 +1338,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)
 	 * We are currently limited to only ONE promisor remote and only
 	 * allow partial-fetches from the promisor remote.
 	 */
-	if (strcmp(remote->name, repository_format_partial_clone)) {
+	if (!find_odb_helper(remote->name)) {
 		if (filter_options.choice)
 			die(_("--filter can only be used with the remote configured in core.partialClone"));
 		return;
@@ -1473,7 +1474,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	if (depth || deepen_since || deepen_not.nr)
 		deepen = 1;
 
-	if (filter_options.choice && !repository_format_partial_clone)
+	if (filter_options.choice && !has_odb_remote())
 		die("--filter can only be used when extensions.partialClone is set");
 
 	if (all) {
@@ -1507,7 +1508,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	}
 
 	if (remote) {
-		if (filter_options.choice || repository_format_partial_clone)
+		if (filter_options.choice || has_odb_remote())
 			fetch_one_setup_partial(remote);
 		result = fetch_one(remote, argc, argv, prune_tags_ok);
 	} else {
diff --git a/builtin/gc.c b/builtin/gc.c
index d604940bb6..418cf2f0b0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -22,6 +22,7 @@
 #include "commit.h"
 #include "packfile.h"
 #include "object-store.h"
+#include "odb-remote.h"
 
 #define FAILED_RUN "failed to run %s"
 
@@ -466,7 +467,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			argv_array_push(&prune, prune_expire);
 			if (quiet)
 				argv_array_push(&prune, "--no-progress");
-			if (repository_format_partial_clone)
+			if (has_odb_remote())
 				argv_array_push(&prune,
 						"--exclude-promisor-objects");
 			if (run_command_v_opt(prune.argv, RUN_GIT_CMD))
diff --git a/builtin/repack.c b/builtin/repack.c
index 7bdb40142f..8d2126087b 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -8,6 +8,7 @@
 #include "strbuf.h"
 #include "string-list.h"
 #include "argv-array.h"
+#include "odb-remote.h"
 
 static int delta_base_offset = 1;
 static int pack_kept_objects = -1;
@@ -234,7 +235,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	argv_array_push(&cmd.args, "--all");
 	argv_array_push(&cmd.args, "--reflog");
 	argv_array_push(&cmd.args, "--indexed-objects");
-	if (repository_format_partial_clone)
+	if (has_odb_remote())
 		argv_array_push(&cmd.args, "--exclude-promisor-objects");
 	if (window)
 		argv_array_pushf(&cmd.args, "--window=%s", window);
diff --git a/cache.h b/cache.h
index 0c1fb9fbcc..17263f9013 100644
--- a/cache.h
+++ b/cache.h
@@ -897,13 +897,11 @@ extern int grafts_replace_parents;
 #define GIT_REPO_VERSION 0
 #define GIT_REPO_VERSION_READ 1
 extern int repository_format_precious_objects;
-extern char *repository_format_partial_clone;
 extern const char *core_partial_clone_filter_default;
 
 struct repository_format {
 	int version;
 	int precious_objects;
-	char *partial_clone; /* value of extensions.partialclone */
 	int is_bare;
 	int hash_algo;
 	char *work_tree;
diff --git a/connected.c b/connected.c
index 91feb78815..5d45f833d4 100644
--- a/connected.c
+++ b/connected.c
@@ -4,6 +4,7 @@
 #include "connected.h"
 #include "transport.h"
 #include "packfile.h"
+#include "odb-remote.h"
 
 /*
  * If we feed all the commits we want to verify to this command
@@ -56,7 +57,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	argv_array_push(&rev_list.args,"rev-list");
 	argv_array_push(&rev_list.args, "--objects");
 	argv_array_push(&rev_list.args, "--stdin");
-	if (repository_format_partial_clone)
+	if (has_odb_remote())
 		argv_array_push(&rev_list.args, "--exclude-promisor-objects");
 	argv_array_push(&rev_list.args, "--not");
 	argv_array_push(&rev_list.args, "--all");
diff --git a/environment.c b/environment.c
index 2a6de2330b..f8048f4a5d 100644
--- a/environment.c
+++ b/environment.c
@@ -30,7 +30,6 @@ int warn_ambiguous_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
 int ref_paranoia = -1;
 int repository_format_precious_objects;
-char *repository_format_partial_clone;
 const char *core_partial_clone_filter_default;
 const char *git_commit_encoding;
 const char *git_log_output_encoding;
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 6a3cc985c4..f8a4642d17 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -6,6 +6,7 @@
 #include "list-objects.h"
 #include "list-objects-filter.h"
 #include "list-objects-filter-options.h"
+#include "odb-remote.h"
 
 /*
  * Parse value of the argument to the "filter" keyword.
@@ -114,30 +115,32 @@ void partial_clone_register(
 	const char *remote,
 	const struct list_objects_filter_options *filter_options)
 {
-	/*
-	 * Record the name of the partial clone remote in the
-	 * config and in the global variable -- the latter is
-	 * used throughout to indicate that partial clone is
-	 * enabled and to expect missing objects.
-	 */
-	if (repository_format_partial_clone &&
-	    *repository_format_partial_clone &&
-	    strcmp(remote, repository_format_partial_clone))
-		die(_("cannot change partial clone promisor remote"));
+	char *cfg_name;
+
+	/* Check if it is already registered */
+	if (find_odb_helper(remote))
+		return;
 
 	git_config_set("core.repositoryformatversion", "1");
-	git_config_set("extensions.partialclone", remote);
 
-	repository_format_partial_clone = xstrdup(remote);
+	/* Add odb config for the remote */
+	cfg_name = xstrfmt("odb.%s.promisorRemote", remote);
+	git_config_set(cfg_name, remote);
+	free(cfg_name);
 
 	/*
 	 * Record the initial filter-spec in the config as
 	 * the default for subsequent fetches from this remote.
+	 *
+	 * TODO: move core.partialclonefilter into odb.<name>
 	 */
 	core_partial_clone_filter_default =
 		xstrdup(filter_options->filter_spec);
 	git_config_set("core.partialclonefilter",
 		       core_partial_clone_filter_default);
+
+	/* Make sure the config info are reset */
+	odb_remote_reinit();
 }
 
 void partial_clone_get_default_filter_spec(
diff --git a/packfile.c b/packfile.c
index 6c3ddc3c31..bf0d1e7a20 100644
--- a/packfile.c
+++ b/packfile.c
@@ -15,6 +15,7 @@
 #include "tree-walk.h"
 #include "tree.h"
 #include "object-store.h"
+#include "odb-remote.h"
 
 char *odb_pack_name(struct strbuf *buf,
 		    const unsigned char *sha1,
@@ -1960,7 +1961,7 @@ int is_promisor_object(const struct object_id *oid)
 	static int promisor_objects_prepared;
 
 	if (!promisor_objects_prepared) {
-		if (repository_format_partial_clone) {
+		if (has_odb_remote()) {
 			for_each_packed_object(add_promisor_object,
 					       &promisor_objects,
 					       FOR_EACH_OBJECT_PROMISOR_ONLY);
diff --git a/setup.c b/setup.c
index 3e03d442b6..be7f45cf4b 100644
--- a/setup.c
+++ b/setup.c
@@ -419,11 +419,7 @@ 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, "partialclone")) {
-			if (!value)
-				return config_error_nonbool(var);
-			data->partial_clone = xstrdup(value);
-		} else
+		else
 			string_list_append(&data->unknown_extensions, ext);
 	} else if (strcmp(var, "core.bare") == 0) {
 		data->is_bare = git_config_bool(var, value);
@@ -465,7 +461,6 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 	}
 
 	repository_format_precious_objects = candidate->precious_objects;
-	repository_format_partial_clone = candidate->partial_clone;
 	string_list_clear(&candidate->unknown_extensions, 0);
 	if (!has_common) {
 		if (candidate->is_bare != -1) {
diff --git a/sha1-file.c b/sha1-file.c
index 7d3c1ebd91..27eb3a99ac 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -32,6 +32,7 @@
 #include "packfile.h"
 #include "fetch-object.h"
 #include "object-store.h"
+#include "odb-remote.h"
 
 /* The maximum size for an object header. */
 #define MAX_HEADER_LEN 32
@@ -1287,13 +1288,13 @@ int oid_object_info_extended(const struct object_id *oid, struct object_info *oi
 		}
 
 		/* Check if it is a missing object */
-		if (fetch_if_missing && repository_format_partial_clone &&
+		if (fetch_if_missing && has_odb_remote() &&
 		    !already_retried) {
 			/*
-			 * TODO Investigate checking fetch_object() return
-			 * TODO value and stopping on error here.
+			 * TODO Investigate checking odb_remote_get_direct()
+			 * TODO return value and stopping on error here.
 			 */
-			fetch_object(repository_format_partial_clone, real->hash);
+			odb_remote_get_direct(real->hash);
 			already_retried = 1;
 			continue;
 		}
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index cc18b75c03..6af4712da8 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -43,7 +43,7 @@ test_expect_success 'missing reflog object, but promised by a commit, passes fsc
 
 	# But with the extension, it succeeds
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo fsck
 '
 
@@ -66,7 +66,7 @@ test_expect_success 'missing reflog object, but promised by a tag, passes fsck'
 	printf "$T\n" | pack_as_from_promisor &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo fsck
 '
 
@@ -84,7 +84,7 @@ test_expect_success 'missing reflog object alone fails fsck, even with extension
 	delete_object repo "$A" &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	test_must_fail git -C repo fsck
 '
 
@@ -100,7 +100,7 @@ test_expect_success 'missing ref object, but promised, passes fsck' '
 	promise_and_delete "$A" &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo fsck
 '
 
@@ -123,7 +123,7 @@ test_expect_success 'missing object, but promised, passes fsck' '
 	promise_and_delete "$AT" &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo fsck
 '
 
@@ -136,7 +136,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' '
 	promise_and_delete "$A" &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo fsck "$A"
 '
 
@@ -151,7 +151,7 @@ test_expect_success 'fetching of missing objects' '
 	rm -rf repo/.git/objects/* &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "origin" &&
+	git -C repo config odb.magic.promisorRemote "origin" &&
 	git -C repo cat-file -p "$HASH" &&
 
 	# Ensure that the .promisor file is written, and check that its
@@ -172,7 +172,7 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '
 	promise_and_delete "$FOO" &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo rev-list --exclude-promisor-objects --objects bar >out &&
 	grep $(git -C repo rev-parse bar) out &&
 	! grep $FOO out
@@ -197,7 +197,7 @@ test_expect_success 'rev-list stops traversal at missing and promised tree' '
 	promise_and_delete "$TREE2" &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo rev-list --exclude-promisor-objects --objects HEAD >out &&
 	grep $(git -C repo rev-parse foo) out &&
 	! grep $TREE out &&
@@ -216,7 +216,7 @@ test_expect_success 'rev-list stops traversal at missing and promised blob' '
 	promise_and_delete "$BLOB" &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo rev-list --exclude-promisor-objects --objects HEAD >out &&
 	grep $(git -C repo rev-parse HEAD) out &&
 	! grep $BLOB out
@@ -235,7 +235,7 @@ test_expect_success 'rev-list stops traversal at promisor commit, tree, and blob
 	printf "%s\n%s\n%s\n" $COMMIT $TREE $BLOB | pack_as_from_promisor &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo rev-list --exclude-promisor-objects --objects HEAD >out &&
 	! grep $COMMIT out &&
 	! grep $TREE out &&
@@ -259,7 +259,7 @@ test_expect_success 'rev-list accepts missing and promised objects on command li
 	promise_and_delete $BLOB &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo rev-list --exclude-promisor-objects --objects "$COMMIT" "$TREE" "$BLOB"
 '
 
@@ -272,7 +272,7 @@ test_expect_success 'gc does not repack promisor objects' '
 	HASH=$(printf "$TREE_HASH\n" | pack_as_from_promisor) &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo gc &&
 
 	# Ensure that the promisor packfile still exists, and remove it
@@ -296,7 +296,7 @@ test_expect_success 'gc stops traversal when a missing but promised object is re
 	HASH=$(promise_and_delete $TREE_HASH) &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "arbitrary string" &&
+	git -C repo config odb.magic.promisorRemote "arbitrary string" &&
 	git -C repo gc &&
 
 	# Ensure that the promisor packfile still exists, and remove it
@@ -327,7 +327,7 @@ test_expect_success 'fetching of missing objects from an HTTP server' '
 	rm -rf repo/.git/objects/* &&
 
 	git -C repo config core.repositoryformatversion 1 &&
-	git -C repo config extensions.partialclone "origin" &&
+	git -C repo config odb.magic.promisorRemote "origin" &&
 	git -C repo cat-file -p "$HASH" &&
 
 	# Ensure that the .promisor file is written, and check that its
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 0680dec808..3bb20b0566 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -792,14 +792,14 @@ fetch_filter_blob_limit_zero () {
 	test_config -C "$SERVER" uploadpack.allowfilter 1 &&
 
 	git clone "$URL" client &&
-	test_config -C client extensions.partialclone origin &&
+	test_config -C client odb.magic.promisorRemote origin &&
 
 	test_commit -C "$SERVER" two &&
 
 	git -C client fetch --filter=blob:limit=0 origin HEAD:somewhere &&
 
 	# Ensure that commit is fetched, but blob is not
-	test_config -C client extensions.partialclone "arbitrary string" &&
+	test_config -C client odb.magic.promisorRemote "arbitrary string" &&
 	git -C client cat-file -e $(git -C "$SERVER" rev-parse two) &&
 	test_must_fail git -C client cat-file -e $(git hash-object "$SERVER/two.t")
 }
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 0b62037744..022c65a7ae 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -647,7 +647,7 @@ partial_clone () {
 	git -C client fsck &&
 
 	# Ensure that unneeded blobs are not inadvertently fetched.
-	test_config -C client extensions.partialclone "not a remote" &&
+	test_config -C client odb.origin.promisorRemote "not a remote" &&
 	test_must_fail git -C client cat-file -e "$HASH1" &&
 
 	# But this blob was fetched, because clone performs an initial checkout
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index cee5565367..5ddd1e011c 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -40,7 +40,7 @@ test_expect_success 'do partial clone 1' '
 		| sort >observed.oids &&
 	test_cmp expect_1.oids observed.oids &&
 	test "$(git -C pc1 config --local core.repositoryformatversion)" = "1" &&
-	test "$(git -C pc1 config --local extensions.partialclone)" = "origin" &&
+	test "$(git -C pc1 config --local odb.origin.promisorRemote)" = "origin" &&
 	test "$(git -C pc1 config --local core.partialclonefilter)" = "blob:none"
 '
 
diff --git a/unpack-trees.c b/unpack-trees.c
index dec37ad1e7..fde63993a9 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -16,6 +16,7 @@
 #include "submodule-config.h"
 #include "fsmonitor.h"
 #include "fetch-object.h"
+#include "odb-remote.h"
 
 /*
  * Error messages expected by scripts out of plumbing commands such as
@@ -371,7 +372,7 @@ static int check_updates(struct unpack_trees_options *o)
 		load_gitmodules_file(index, &state);
 
 	enable_delayed_checkout(&state);
-	if (repository_format_partial_clone && o->update && !o->dry_run) {
+	if (has_odb_remote() && o->update && !o->dry_run) {
 		/*
 		 * Prefetch the objects that are to be checked out in the loop
 		 * below.
@@ -388,8 +389,7 @@ static int check_updates(struct unpack_trees_options *o)
 			}
 		}
 		if (to_fetch.nr)
-			fetch_objects(repository_format_partial_clone,
-				      &to_fetch);
+			odb_remote_get_many_direct(&to_fetch);
 		fetch_if_missing = fetch_if_missing_store;
 		oid_array_clear(&to_fetch);
 	}
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 7/8] Use odb.origin.partialclonefilter instead of core.partialclonefilter

[Christian Couder]

Let's make the partial clone filter specific to one odb
instead of general to all the odbs.

This makes it possible to have different partial clone
filters for different odbs.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/fetch.c               |  2 +-
 list-objects-filter-options.c | 26 ++++++++++++++++----------
 list-objects-filter-options.h |  3 ++-
 odb-helper.h                  |  1 +
 odb-remote.c                  |  2 ++
 t/t5616-partial-clone.sh      |  2 +-
 6 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 82a3e655ba..288d58c0c5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1350,7 +1350,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)
 	 * the config.
 	 */
 	if (!filter_options.choice)
-		partial_clone_get_default_filter_spec(&filter_options);
+		partial_clone_get_default_filter_spec(&filter_options, remote->name);
 	return;
 }
 
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index f8a4642d17..59378dfb0a 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -7,6 +7,7 @@
 #include "list-objects-filter.h"
 #include "list-objects-filter-options.h"
 #include "odb-remote.h"
+#include "odb-helper.h"
 
 /*
  * Parse value of the argument to the "filter" keyword.
@@ -29,6 +30,9 @@ static int gently_parse_list_objects_filter(
 {
 	const char *v0;
 
+	if (!arg)
+		return 0;
+
 	if (filter_options->choice) {
 		if (errbuf) {
 			strbuf_init(errbuf, 0);
@@ -116,6 +120,7 @@ void partial_clone_register(
 	const struct list_objects_filter_options *filter_options)
 {
 	char *cfg_name;
+	char *filter_name;
 
 	/* Check if it is already registered */
 	if (find_odb_helper(remote))
@@ -131,25 +136,26 @@ void partial_clone_register(
 	/*
 	 * Record the initial filter-spec in the config as
 	 * the default for subsequent fetches from this remote.
-	 *
-	 * TODO: move core.partialclonefilter into odb.<name>
 	 */
-	core_partial_clone_filter_default =
-		xstrdup(filter_options->filter_spec);
-	git_config_set("core.partialclonefilter",
-		       core_partial_clone_filter_default);
+	filter_name = xstrfmt("odb.%s.partialclonefilter", remote);
+	git_config_set(filter_name, filter_options->filter_spec);
+	free(filter_name);
 
 	/* Make sure the config info are reset */
 	odb_remote_reinit();
 }
 
 void partial_clone_get_default_filter_spec(
-	struct list_objects_filter_options *filter_options)
+	struct list_objects_filter_options *filter_options,
+	const char *remote)
 {
+	struct odb_helper *helper = find_odb_helper(remote);
+
 	/*
 	 * Parse default value, but silently ignore it if it is invalid.
 	 */
-	gently_parse_list_objects_filter(filter_options,
-					 core_partial_clone_filter_default,
-					 NULL);
+	if (helper)
+		gently_parse_list_objects_filter(filter_options,
+						 helper->partial_clone_filter,
+						 NULL);
 }
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index 0000a61f82..12ceef3230 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -74,6 +74,7 @@ void partial_clone_register(
 	const char *remote,
 	const struct list_objects_filter_options *filter_options);
 void partial_clone_get_default_filter_spec(
-	struct list_objects_filter_options *filter_options);
+	struct list_objects_filter_options *filter_options,
+	const char *remote);
 
 #endif /* LIST_OBJECTS_FILTER_OPTIONS_H */
diff --git a/odb-helper.h b/odb-helper.h
index d63210d516..02bc3dbe13 100644
--- a/odb-helper.h
+++ b/odb-helper.h
@@ -4,6 +4,7 @@
 struct odb_helper {
 	const char *name;
 	const char *dealer;
+	const char *partial_clone_filter;
 
 	struct odb_helper *next;
 };
diff --git a/odb-remote.c b/odb-remote.c
index 0a734ff379..2c7c35c763 100644
--- a/odb-remote.c
+++ b/odb-remote.c
@@ -35,6 +35,8 @@ static int odb_remote_config(const char *var, const char *value, void *data)
 
 	if (!strcmp(subkey, "promisorremote"))
 		return git_config_string(&o->dealer, var, value);
+	if (!strcmp(subkey, "partialclonefilter"))
+		return git_config_string(&o->partial_clone_filter, var, value);
 
 	return 0;
 }
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 5ddd1e011c..4231121181 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -41,7 +41,7 @@ test_expect_success 'do partial clone 1' '
 	test_cmp expect_1.oids observed.oids &&
 	test "$(git -C pc1 config --local core.repositoryformatversion)" = "1" &&
 	test "$(git -C pc1 config --local odb.origin.promisorRemote)" = "origin" &&
-	test "$(git -C pc1 config --local core.partialclonefilter)" = "blob:none"
+	test "$(git -C pc1 config --local odb.origin.partialclonefilter)" = "blob:none"
 '
 
 # checkout master to force dynamic object fetch of blobs at HEAD.
-- 
2.17.0.590.gbd05bfcafd

[PATCH v1 8/8] t0410: test fetching from many promisor remotes

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t0410-partial-clone.sh | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 6af4712da8..4a7a662512 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -162,8 +162,30 @@ test_expect_success 'fetching of missing objects' '
 	git verify-pack --verbose "$IDX" | grep "$HASH"
 '
 
+test_expect_success 'fetching of missing objects from another odb remote' '
+	git clone "file://$(pwd)/server" server2 &&
+	test_commit -C server2 bar &&
+	git -C server2 repack -a -d --write-bitmap-index &&
+	HASH2=$(git -C server2 rev-parse bar) &&
+
+	git -C repo remote add server2 "file://$(pwd)/server2" &&
+	git -C repo config odb.magic2.promisorRemote server2 &&
+	git -C repo cat-file -p "$HASH2" &&
+
+	git -C repo fetch server2 &&
+	rm -rf repo/.git/objects/* &&
+	git -C repo cat-file -p "$HASH2" &&
+
+	# Ensure that the .promisor file is written, and check that its
+	# associated packfile contains the object
+	ls repo/.git/objects/pack/pack-*.promisor >promisorlist &&
+	test_line_count = 1 promisorlist &&
+	IDX=$(cat promisorlist | sed "s/promisor$/idx/") &&
+	git verify-pack --verbose "$IDX" | grep "$HASH2"
+'
+
 test_expect_success 'rev-list stops traversal at missing and promised commit' '
-	rm -rf repo &&
+	rm -rf repo server server2 &&
 	test_create_repo repo &&
 	test_commit -C repo foo &&
 	test_commit -C repo bar &&
-- 
2.17.0.590.gbd05bfcafd

Re: [PATCH v1 0/8] Introducing odb remote

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> This is a follow up from the patch series called "Promisor remotes and
> external ODB support" that I sent earlier this year.
>
> Following discussions of these patch series, where Junio said "a
> minimum s/ext/remote/ would clarify what it is", I decided to rename
> "external odb" to "odb remote".

In "external odb", "odb" is a noun, decorated by adjective
"external".  I would expect the same would apply to "remote" and
"odb".

> I am still open to another name, but I
> think that "odb remote" works well with "odb helper" that was already
> used in the series and is as good or perhaps better than "remote odb",
> as a "remote odb" I think would be easier to confuse with a regular
> "remote".

As long as our use of "remote" as a noun is understood as omitting
(an obvious) "repository" from a noun phrase "remote repository", I
do not think there is any confusion.


"odb remote" sounds like the phrase is about a noun "remote" (we do
use "remote" as a noun when talking about fetching and pushing, as a
short hand for "remote repository") that uses "odb" as an adjective
somehow (e.g. "this is one of those remotes defined but not for the
usual fetching and pushing, but for accessing its odb", or something
ilke that), which somehow does not sit well on my tongue nor in ears.
Perhaps that is because I am not well versed in languages whose natural
word order in a noun phrase is noun followed by adjective, but I dunno.

> Another obvious difference with the previous series is that this
> series is only about integrating with the promisor/narrow clone work
> and showing that it makes it possible to use more than one promisor
> remote.

Sounds like a sensible "incremental" approach.

Will read the patches and commet on them in another sitting.
Thanks.

Re: [PATCH v1 1/8] fetch-object: make functions return an error code

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> The callers of the fetch_object() and fetch_objects() might
> be interested in knowing if these functions succeeded or not.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  fetch-object.c | 15 +++++++++------
>  fetch-object.h |  6 +++---
>  sha1-file.c    |  4 ++--
>  3 files changed, 14 insertions(+), 11 deletions(-)

"might", but nobody pays attention to the return value, as the
local availablity of the object at the end is the only thing that
matters, so at this step, it is not all that impactful a change.

Let's see how it plays out.

>  			/*
> -			 * TODO Investigate haveing fetch_object() return
> -			 * TODO error/success and stopping the music here.
> +			 * TODO Investigate checking fetch_object() return
> +			 * TODO value and stopping on error here.
>  			 */
>  			fetch_object(repository_format_partial_clone, real->hash);
>  			already_retried = 1;

Re: [PATCH v1 2/8] Add initial odb remote support

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> diff --git a/odb-helper.h b/odb-helper.h
> new file mode 100644
> index 0000000000..61d2ad082b
> --- /dev/null
> +++ b/odb-helper.h
> @@ -0,0 +1,13 @@
> +#ifndef ODB_HELPER_H
> +#define ODB_HELPER_H
> +

Here is a good space to write a comment on what this structure and
its fields are about.  Who are the dealers of helpers anyway?

> +struct odb_helper {
> +	const char *name;
> +	const char *dealer;
> +
> +	struct odb_helper *next;
> +};
> +
> +extern struct odb_helper *odb_helper_new(const char *name, int namelen);
> +
> +#endif /* ODB_HELPER_H */
> diff --git a/odb-remote.c b/odb-remote.c
> new file mode 100644
> index 0000000000..e03b953ec6
> --- /dev/null
> +++ b/odb-remote.c
> @@ -0,0 +1,72 @@
> +#include "cache.h"
> +#include "odb-remote.h"
> +#include "odb-helper.h"
> +#include "config.h"
> +
> +static struct odb_helper *helpers;
> +static struct odb_helper **helpers_tail = &helpers;
> +
> +static struct odb_helper *find_or_create_helper(const char *name, int len)
> +{
> +	struct odb_helper *o;
> +
> +	for (o = helpers; o; o = o->next)
> +		if (!strncmp(o->name, name, len) && !o->name[len])
> +			return o;
> +
> +	o = odb_helper_new(name, len);
> +	*helpers_tail = o;
> +	helpers_tail = &o->next;
> +
> +	return o;
> +}

This is a tangent, but I wonder if we can do better than
hand-rolling these singly-linked list of custom structure types
every time we add new code.  I am just guessing (because it is not
described in the log message) that the expectation is to have only
just a handful of helpers so looking for a helper by name is OK at
O(n), as long as we very infrequently look up a helper by name.

> +static int odb_remote_config(const char *var, const char *value, void *data)
> +{
> +	struct odb_helper *o;
> +	const char *name;
> +	int namelen;
> +	const char *subkey;
> +
> +	if (parse_config_key(var, "odb", &name, &namelen, &subkey) < 0)
> +		return 0;
> +
> +	o = find_or_create_helper(name, namelen);
> +
> +	if (!strcmp(subkey, "promisorremote"))
> +		return git_config_string(&o->dealer, var, value);

If the field is meant to record the name of the promisor remote,
then it is better to name it as such, no?

> +struct odb_helper *find_odb_helper(const char *dealer)

Ditto.

> +{
> +	struct odb_helper *o;
> +
> +	odb_remote_init();
> +
> +	if (!dealer)
> +		return helpers;
> +
> +	for (o = helpers; o; o = o->next)
> +		if (!strcmp(o->dealer, dealer))
> +			return o;

The code to create a new helper tried to avoid creating a helper
with duplicated name, but nobody tries to create more than one
helpers that point at the same promisor remote.  Yet here we try to
grab the first one that happens to point at the given promisor.

That somehow smells wrong.

Re: [PATCH v1 2/8] Add initial odb remote support

[Ævar Arnfjörð Bjarmason]

On Sun, May 13 2018, Christian Couder wrote:

> "sha1_file.c"[...]

sha1-file.c these days :)

Re: [PATCH v1 2/8] Add initial odb remote support

[Christian Couder]

On Tue, May 15, 2018 at 3:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:

>> --- /dev/null
>> +++ b/odb-helper.h
>> @@ -0,0 +1,13 @@
>> +#ifndef ODB_HELPER_H
>> +#define ODB_HELPER_H
>
> Here is a good space to write a comment on what this structure and
> its fields are about.  Who are the dealers of helpers anyway?

Ok I added a few comments and renamed "dealer" to "remote".

>> +static struct odb_helper *helpers;
>> +static struct odb_helper **helpers_tail = &helpers;
>> +
>> +static struct odb_helper *find_or_create_helper(const char *name, int len)
>> +{
>> +     struct odb_helper *o;
>> +
>> +     for (o = helpers; o; o = o->next)
>> +             if (!strncmp(o->name, name, len) && !o->name[len])
>> +                     return o;
>> +
>> +     o = odb_helper_new(name, len);
>> +     *helpers_tail = o;
>> +     helpers_tail = &o->next;
>> +
>> +     return o;
>> +}
>
> This is a tangent, but I wonder if we can do better than
> hand-rolling these singly-linked list of custom structure types
> every time we add new code.  I am just guessing (because it is not
> described in the log message) that the expectation is to have only
> just a handful of helpers so looking for a helper by name is OK at
> O(n), as long as we very infrequently look up a helper by name.

Yeah, I think there will be very few helpers. I added a comment in the
version I will send really soon now.

>> +     if (!strcmp(subkey, "promisorremote"))
>> +             return git_config_string(&o->dealer, var, value);
>
> If the field is meant to record the name of the promisor remote,
> then it is better to name it as such, no?

Yeah, it is renamed "remote" now.

>> +     for (o = helpers; o; o = o->next)
>> +             if (!strcmp(o->dealer, dealer))
>> +                     return o;
>
> The code to create a new helper tried to avoid creating a helper
> with duplicated name, but nobody tries to create more than one
> helpers that point at the same promisor remote.  Yet here we try to
> grab the first one that happens to point at the given promisor.
>
> That somehow smells wrong.

For each promisor remote I think it makes no sense to have more than
one remote odb pointing to it. So I am not sure what to do here.

Re: [PATCH v1 2/8] Add initial odb remote support

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> For each promisor remote I think it makes no sense to have more than
> one remote odb pointing to it. So I am not sure what to do here.

If it makes no sense, then detecting and erroring out would be a
sensible thing to do, no?