{"thread":{"id":"50717","subject":"[PATCH v3 00/11] Many promisor remotes","startedAt":"2019-03-12T13:30:18Z","lastAt":"2019-04-01T17:53:36Z","messageCount":22,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":11},"messages":[{"id":"371248","messageId":"20190312132959.11764-1-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":null,"subject":"[PATCH v3 00/11] Many promisor remotes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:48Z","receivedAt":"2019-03-12T13:30:18Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This path series is a follow up from the \"remote odb\" patch series\nthat I sent last year, which were a follow up from previous\nseries. See the links section for more information.\n\nThe goal of this patch series is to make it possible to have and to\nfetch missing objects from multiple remotes instead of only one.\n\nFor now the fetch order is the order of the remotes in the config,\nexcept for the remote specified by extensions.partialclone config\noption which comes last in the fetch order.\n\nI selected the name \"Promisor remote\" over \"Partial clone remote\"\nbecause it is shorter and because it is not only about cloning.\n\nThe existing extensions.partialclone config option is respected, but\nit is not written in the config when a partial clone or fetch is\nmade. Instead remote..promisor is set to \"true\". This may create\na compatibility issue, but it makes it possible to start using many\npromisor remotes by just cloning and fetching from different remotes\nwith partial clone filters. The compatibility issue could be resolved\nin a future iteration by just setting extensions.partialclone instead\nof remote..promisor the first time a promisor remote is used.\n\nIn general I have tried to change as few things as possible.\n\nHigh level changes since the V2\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\n - remove the 2 first patches from V2 that have been sent and merged\n separately\n\n - improved commit message in patch 8/11 documentation patches\n \n - slightly improved cover letter\n\nAs I got no comment on V2, I am not sure what to improve at this\npoint.\n\nHigh level overview of this patch series\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\n - Patch 1/11:\n\nThis patch, makes functions in fetch-object.c return an error code,\nwhich is necessary to later tell that they failed and try another\npromisor remote when there is more than one. This could also just be\nseen as a fix to these functions.\n\n - Patch 2/11:\n\nThis introduces the minimum infrastructure for promisor remotes.\n\n - Patch 3/11, 4/11 and 5/11:\n\nThis add a few missing bits in the promisor remote infrastructure that\nwill be needed in the following patches.\n\n - Patch 6/11:\n\nThis replaces the previous interface that used only one promisor\nremote defined in extensions.partialclone with the new interface\ncreated by the previous patches.\n\n - Patch 7/11:\n\nThis replaces the way a partial clone filter was handled by a new way\nbased on the previous patches that support more than one partial clone\nfilter.\n\n - Patch 8/11:\n\nThis patch removes the limitation in builtin/fetch.c to have only one\npromisor remote.\n\n - Patch 9/11:\n\nThis adds test cases that shows that now more than one promisor remote\ncan be used and that remote..promisor is set to \"true\" when\nfetching from a new promisor remote.\n\n - Patch 10/11 and 11/11:\n\nThese are new documentation patches, to explain how things can work\nwith more than one promisor remote.\n\nLinks\n~~~~~\n\nThis patch series on GitHub:\n\nV3: https://github.com/chriscool/git/commits/many-promisor-remotes\nV2: https://github.com/chriscool/git/commits/many-promisor-remotes35\nV1: https://github.com/chriscool/git/commits/many-promisor-remotes17\n\nOn the mailing list:\n\nV2: https://public-inbox.org/git/20190122144212.15119-1-chriscool@tuxfamily.org/\nV1: https://public-inbox.org/git/20181211052746.16218-1-chriscool@tuxfamily.org/\n\nThis patch series is a follow up from the discussions related to\nthe remote odb V4 patch series:\n\nhttps://public-inbox.org/git/20180802061505.2983-1-chriscool@tuxfamily.org/\n\nEspecially in:\n\nhttps://public-inbox.org/git/CAP8UFD3nrhjANwNDqTwx5ZtnZNcnbAFqUN=u=LrvzuH4+3wQQA@mail.gmail.com/\n\nI said that I would like to work on things in the following order:\n\n 1) Teaching partial clone to attempt to fetch missing objects from\nmultiple remotes instead of only one using the order in the config.\n\n 2) Simplifying the protocol for fetching missing objects so that it\ncan be satisfied by a lighter weight object storage system than a full\nGit server.\n\n 3) Making it possible to explicitly define an order in which the\nremotes are accessed.\n\n 4) Making the criteria for what objects can be missing more\naggressive, so that I can \"git add\" a large file and work with it\nusing Git without even having a second copy of that object in my local\nobject store.\n\nAnd this patch series is about the 1).\n\nThe previous remote odb patch series on GitHub:\n\nV5: https://github.com/chriscool/git/commits/remote-odb\nV4: https://github.com/chriscool/git/commits/remote-odb5\nV3: https://github.com/chriscool/git/commits/remote-odb3\nV2: https://github.com/chriscool/git/commits/remote-odb2\nV1: https://github.com/chriscool/git/commits/remote-odb1\n\nDiscussions related to previous versions of the odb patch series:\n\nV4: https://public-inbox.org/git/20180802061505.2983-1-chriscool@tuxfamily.org/\nV3: https://public-inbox.org/git/20180713174959.16748-1-chriscool@tuxfamily.org/\nV2: https://public-inbox.org/git/20180630083542.20347-1-chriscool@tuxfamily.org/\nV1: https://public-inbox.org/git/20180623121846.19750-1-chriscool@tuxfamily.org/\n\n\nChristian Couder (11):\n fetch-object: make functions return an error code\n Add initial support for many promisor remotes\n promisor-remote: implement promisor_remote_get_direct()\n promisor-remote: add promisor_remote_reinit()\n promisor-remote: use repository_format_partial_clone\n Use promisor_remote_get_direct() and has_promisor_remote()\n promisor-remote: parse remote.*.partialclonefilter\n builtin/fetch: remove unique promisor remote limitation\n t0410: test fetching from many promisor remotes\n partial-clone: add multiple remotes in the doc\n remote: add promisor and partial clone config to the doc\n\n Documentation/config/remote.txt | 8 ++\n Documentation/technical/partial-clone.txt | 83 ++++++++----\n Makefile | 1 +\n builtin/cat-file.c | 5 +-\n builtin/fetch.c | 29 ++---\n builtin/gc.c | 3 +-\n builtin/repack.c | 3 +-\n cache-tree.c | 3 +-\n connected.c | 3 +-\n fetch-object.c | 13 +-\n fetch-object.h | 4 +-\n list-objects-filter-options.c | 51 ++++----\n list-objects-filter-options.h | 3 +-\n packfile.c | 3 +-\n promisor-remote.c | 148 ++++++++++++++++++++++\n promisor-remote.h | 22 ++++\n sha1-file.c | 14 +-\n t/t0410-partial-clone.sh | 49 ++++++-\n t/t5601-clone.sh | 3 +-\n t/t5616-partial-clone.sh | 4 +-\n unpack-trees.c | 6 +-\n 21 files changed, 362 insertions(+), 96 deletions(-)\n create mode 100644 promisor-remote.c\n create mode 100644 promisor-remote.h\n\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371249","messageId":"20190312132959.11764-2-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 01/11] fetch-object: make functions return an error code","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:49Z","receivedAt":"2019-03-12T13:30:18Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nThe callers of the fetch_object() and fetch_objects() might\nbe interested in knowing if these functions succeeded or not.\n\nSigned-off-by: Christian Couder \nSigned-off-by: Junio C Hamano \n---\n fetch-object.c | 13 ++++++++-----\n fetch-object.h | 4 ++--\n sha1-file.c | 4 ++--\n 3 files changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/fetch-object.c b/fetch-object.c\nindex 4266548800..eac4d448ef 100644\n--- a/fetch-object.c\n+++ b/fetch-object.c\n@@ -5,11 +5,12 @@\n #include \"transport.h\"\n #include \"fetch-object.h\"\n \n-static void fetch_refs(const char *remote_name, struct ref *ref)\n+static int fetch_refs(const char *remote_name, struct ref *ref)\n {\n \tstruct remote *remote;\n \tstruct transport *transport;\n \tint original_fetch_if_missing = fetch_if_missing;\n+\tint res;\n \n \tfetch_if_missing = 0;\n \tremote = remote_get(remote_name);\n@@ -19,12 +20,14 @@ static void fetch_refs(const char *remote_name, struct ref *ref)\n \n \ttransport_set_option(transport, TRANS_OPT_FROM_PROMISOR, \"1\");\n \ttransport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, \"1\");\n-\ttransport_fetch_refs(transport, ref);\n+\tres = transport_fetch_refs(transport, ref);\n \tfetch_if_missing = original_fetch_if_missing;\n+\n+\treturn res;\n }\n \n-void fetch_objects(const char *remote_name, const struct object_id *oids,\n-\t\t int oid_nr)\n+int fetch_objects(const char *remote_name, const struct object_id *oids,\n+\t\t int oid_nr)\n {\n \tstruct ref *ref = NULL;\n \tint i;\n@@ -36,5 +39,5 @@ void fetch_objects(const char *remote_name, const struct object_id *oids,\n \t\tnew_ref->next = ref;\n \t\tref = new_ref;\n \t}\n-\tfetch_refs(remote_name, ref);\n+\treturn fetch_refs(remote_name, ref);\n }\ndiff --git a/fetch-object.h b/fetch-object.h\nindex d6444caa5a..7bcc7cadb0 100644\n--- a/fetch-object.h\n+++ b/fetch-object.h\n@@ -3,7 +3,7 @@\n \n struct object_id;\n \n-void fetch_objects(const char *remote_name, const struct object_id *oids,\n-\t\t int oid_nr);\n+int fetch_objects(const char *remote_name, const struct object_id *oids,\n+\t\t int oid_nr);\n \n #endif\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 494606f771..01cc0590f4 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1372,8 +1372,8 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \t\tif (fetch_if_missing && repository_format_partial_clone &&\n \t\t !already_retried && r == the_repository) {\n \t\t\t/*\n-\t\t\t * TODO Investigate having fetch_object() return\n-\t\t\t * TODO error/success and stopping the music here.\n+\t\t\t * TODO Investigate checking fetch_object() return\n+\t\t\t * TODO value and stopping on error here.\n \t\t\t * TODO Pass a repository struct through fetch_object,\n \t\t\t * such that arbitrary repositories work.\n \t\t\t */\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371250","messageId":"20190312132959.11764-4-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 03/11] promisor-remote: implement promisor_remote_get_direct()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:51Z","receivedAt":"2019-03-12T13:30:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nThis is implemented for now by calling fetch_objects(). It fetches\nfrom all the promisor remotes.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 17 +++++++++++++++++\n promisor-remote.h | 1 +\n 2 files changed, 18 insertions(+)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex d2f574651e..f86f9d0b84 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"promisor-remote.h\"\n #include \"config.h\"\n+#include \"fetch-object.h\"\n \n static struct promisor_remote *promisors;\n static struct promisor_remote **promisors_tail = &promisors;\n@@ -98,3 +99,19 @@ int has_promisor_remote(void)\n {\n \treturn !!promisor_remote_find(NULL);\n }\n+\n+int promisor_remote_get_direct(const struct object_id *oids, int oid_nr)\n+{\n+\tstruct promisor_remote *o;\n+\n+\tpromisor_remote_init();\n+\n+\tfor (o = promisors; o; o = o->next) {\n+\t\tif (fetch_objects(o->remote_name, oids, oid_nr) < 0)\n+\t\t\tcontinue;\n+\t\treturn 0;\n+\t}\n+\n+\treturn -1;\n+}\n+\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex bfbf7c0f21..f9f5825417 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -13,5 +13,6 @@ struct promisor_remote {\n extern struct promisor_remote *promisor_remote_new(const char *remote_name);\n extern struct promisor_remote *promisor_remote_find(const char *remote_name);\n extern int has_promisor_remote(void);\n+extern int promisor_remote_get_direct(const struct object_id *oids, int oid_nr);\n \n #endif /* PROMISOR_REMOTE_H */\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371251","messageId":"20190312132959.11764-3-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 02/11] Add initial support for many promisor remotes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:50Z","receivedAt":"2019-03-12T13:30:22Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nThe promisor-remote.{c,h} files will contain functions to\nmanage many promisor remotes.\n\nWe expect that there will not be a lot of promisor remotes,\nso it is ok to use a simple linked list to manage them.\n\nHelped-by: Jeff King \nSigned-off-by: Christian Couder \n---\n Makefile | 1 +\n promisor-remote.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++\n promisor-remote.h | 17 ++++++++\n 3 files changed, 118 insertions(+)\n create mode 100644 promisor-remote.c\n create mode 100644 promisor-remote.h\n\ndiff --git a/Makefile b/Makefile\nindex 537493822b..4f24ccb3dc 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -972,6 +972,7 @@ LIB_OBJS += preload-index.o\n LIB_OBJS += pretty.o\n LIB_OBJS += prio-queue.o\n LIB_OBJS += progress.o\n+LIB_OBJS += promisor-remote.o\n LIB_OBJS += prompt.o\n LIB_OBJS += protocol.o\n LIB_OBJS += quote.o\ndiff --git a/promisor-remote.c b/promisor-remote.c\nnew file mode 100644\nindex 0000000000..d2f574651e\n--- /dev/null\n+++ b/promisor-remote.c\n@@ -0,0 +1,100 @@\n+#include \"cache.h\"\n+#include \"promisor-remote.h\"\n+#include \"config.h\"\n+\n+static struct promisor_remote *promisors;\n+static struct promisor_remote **promisors_tail = &promisors;\n+\n+struct promisor_remote *promisor_remote_new(const char *remote_name)\n+{\n+\tstruct promisor_remote *o;\n+\n+\to = xcalloc(1, sizeof(*o));\n+\to->remote_name = xstrdup(remote_name);\n+\n+\t*promisors_tail = o;\n+\tpromisors_tail = &o->next;\n+\n+\treturn o;\n+}\n+\n+static struct promisor_remote *promisor_remote_look_up(const char *remote_name,\n+\t\t\t\t\t\t struct promisor_remote **previous)\n+{\n+\tstruct promisor_remote *o, *p;\n+\n+\tfor (p = NULL, o = promisors; o; p = o, o = o->next)\n+\t\tif (o->remote_name && !strcmp(o->remote_name, remote_name)) {\n+\t\t\tif (previous)\n+\t\t\t\t*previous = p;\n+\t\t\treturn o;\n+\t\t}\n+\n+\treturn NULL;\n+}\n+\n+static void promisor_remote_move_to_tail(struct promisor_remote *o,\n+\t\t\t\t\t struct promisor_remote *previous)\n+{\n+\tif (previous)\n+\t\tprevious->next = o->next;\n+\telse\n+\t\tpromisors = o->next ? o->next : o;\n+\to->next = NULL;\n+\t*promisors_tail = o;\n+\tpromisors_tail = &o->next;\n+}\n+\n+static int promisor_remote_config(const char *var, const char *value, void *data)\n+{\n+\tstruct promisor_remote *o;\n+\tconst char *name;\n+\tint namelen;\n+\tconst char *subkey;\n+\n+\tif (parse_config_key(var, \"remote\", &name, &namelen, &subkey) < 0)\n+\t\treturn 0;\n+\n+\tif (!strcmp(subkey, \"promisor\")) {\n+\t\tchar *remote_name;\n+\n+\t\tif (!git_config_bool(var, value))\n+\t\t\treturn 0;\n+\n+\t\tremote_name = xmemdupz(name, namelen);\n+\n+\t\tif (!promisor_remote_look_up(remote_name, NULL))\n+\t\t\tpromisor_remote_new(remote_name);\n+\n+\t\tfree(remote_name);\n+\t\treturn 0;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static void promisor_remote_init(void)\n+{\n+\tstatic int initialized;\n+\n+\tif (initialized)\n+\t\treturn;\n+\tinitialized = 1;\n+\n+\tgit_config(promisor_remote_config, NULL);\n+}\n+\n+struct promisor_remote *promisor_remote_find(const char *remote_name)\n+{\n+\tpromisor_remote_init();\n+\n+\tif (!remote_name)\n+\t\treturn promisors;\n+\n+\treturn promisor_remote_look_up(remote_name, NULL);\n+}\n+\n+int has_promisor_remote(void)\n+{\n+\treturn !!promisor_remote_find(NULL);\n+}\ndiff --git a/promisor-remote.h b/promisor-remote.h\nnew file mode 100644\nindex 0000000000..bfbf7c0f21\n--- /dev/null\n+++ b/promisor-remote.h\n@@ -0,0 +1,17 @@\n+#ifndef PROMISOR_REMOTE_H\n+#define PROMISOR_REMOTE_H\n+\n+/*\n+ * Promisor remote linked list\n+ * Its information come from remote.XXX config entries.\n+ */\n+struct promisor_remote {\n+\tconst char *remote_name;\n+\tstruct promisor_remote *next;\n+};\n+\n+extern struct promisor_remote *promisor_remote_new(const char *remote_name);\n+extern struct promisor_remote *promisor_remote_find(const char *remote_name);\n+extern int has_promisor_remote(void);\n+\n+#endif /* PROMISOR_REMOTE_H */\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371252","messageId":"20190312132959.11764-5-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 04/11] promisor-remote: add promisor_remote_reinit()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:52Z","receivedAt":"2019-03-12T13:30:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nWe will need to reinitialize the promisor remote configuration\nas we will make some changes to it in a later commit.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 14 ++++++++++++--\n promisor-remote.h | 1 +\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex f86f9d0b84..ea74f6d8a8 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -74,17 +74,27 @@ static int promisor_remote_config(const char *var, const char *value, void *data\n \treturn 0;\n }\n \n-static void promisor_remote_init(void)\n+static void promisor_remote_do_init(int force)\n {\n \tstatic int initialized;\n \n-\tif (initialized)\n+\tif (!force && initialized)\n \t\treturn;\n \tinitialized = 1;\n \n \tgit_config(promisor_remote_config, NULL);\n }\n \n+static inline void promisor_remote_init(void)\n+{\n+\tpromisor_remote_do_init(0);\n+}\n+\n+void promisor_remote_reinit(void)\n+{\n+\tpromisor_remote_do_init(1);\n+}\n+\n struct promisor_remote *promisor_remote_find(const char *remote_name)\n {\n \tpromisor_remote_init();\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex f9f5825417..f96722bc66 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -10,6 +10,7 @@ struct promisor_remote {\n \tstruct promisor_remote *next;\n };\n \n+extern void promisor_remote_reinit(void);\n extern struct promisor_remote *promisor_remote_new(const char *remote_name);\n extern struct promisor_remote *promisor_remote_find(const char *remote_name);\n extern int has_promisor_remote(void);\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371253","messageId":"20190312132959.11764-6-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 05/11] promisor-remote: use repository_format_partial_clone","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:53Z","receivedAt":"2019-03-12T13:30:25Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"A remote specified using the extensions.partialClone config\noption should be considered a promisor remote too.\n\nThis remote should be at the end of the promisor remote list,\nso that it is used only if objects have not been found in other\nremotes.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex ea74f6d8a8..dcf6ef6521 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -83,6 +83,17 @@ static void promisor_remote_do_init(int force)\n \tinitialized = 1;\n \n \tgit_config(promisor_remote_config, NULL);\n+\n+\tif (repository_format_partial_clone) {\n+\t\tstruct promisor_remote *o, *previous;\n+\n+\t\to = promisor_remote_look_up(repository_format_partial_clone,\n+\t\t\t\t\t &previous);\n+\t\tif (o)\n+\t\t\tpromisor_remote_move_to_tail(o, previous);\n+\t\telse\n+\t\t\tpromisor_remote_new(repository_format_partial_clone);\n+\t}\n }\n \n static inline void promisor_remote_init(void)\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371254","messageId":"20190312132959.11764-8-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 07/11] promisor-remote: parse remote.*.partialclonefilter","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:55Z","receivedAt":"2019-03-12T13:30:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This makes it possible to specify a different partial clone\nfilter for each promisor remote.\n\nSigned-off-by: Christian Couder \n---\n builtin/fetch.c | 2 +-\n list-objects-filter-options.c | 27 +++++++++++++++------------\n list-objects-filter-options.h | 3 ++-\n promisor-remote.c | 10 ++++++++++\n promisor-remote.h | 5 ++++-\n t/t0410-partial-clone.sh | 2 +-\n t/t5601-clone.sh | 1 +\n t/t5616-partial-clone.sh | 2 +-\n 8 files changed, 35 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7edf70ae6c..9d436bd727 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1491,7 +1491,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n \t * the config.\n \t */\n \tif (!filter_options.choice)\n-\t\tpartial_clone_get_default_filter_spec(&filter_options);\n+\t\tpartial_clone_get_default_filter_spec(&filter_options, remote->name);\n \treturn;\n }\n \ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex f41a831fce..02f48b7c40 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -30,6 +30,9 @@ static int gently_parse_list_objects_filter(\n {\n \tconst char *v0;\n \n+\tif (!arg)\n+\t\treturn 0;\n+\n \tif (filter_options->choice) {\n \t\tif (errbuf) {\n \t\t\tstrbuf_addstr(\n@@ -146,6 +149,7 @@ void partial_clone_register(\n \tconst struct list_objects_filter_options *filter_options)\n {\n \tchar *cfg_name;\n+\tchar *filter_name;\n \n \t/* Check if it is already registered */\n \tif (!promisor_remote_find(remote)) {\n@@ -160,27 +164,26 @@ void partial_clone_register(\n \t/*\n \t * Record the initial filter-spec in the config as\n \t * the default for subsequent fetches from this remote.\n-\t *\n-\t * TODO: record it into remote..partialclonefilter\n \t */\n-\tcore_partial_clone_filter_default =\n-\t\txstrdup(filter_options->filter_spec);\n-\tgit_config_set(\"core.partialclonefilter\",\n-\t\t core_partial_clone_filter_default);\n+\tfilter_name = xstrfmt(\"remote.%s.partialclonefilter\", remote);\n+\tgit_config_set(filter_name, filter_options->filter_spec);\n+\tfree(filter_name);\n \n \t/* Make sure the config info are reset */\n \tpromisor_remote_reinit();\n }\n \n void partial_clone_get_default_filter_spec(\n-\tstruct list_objects_filter_options *filter_options)\n+\tstruct list_objects_filter_options *filter_options,\n+\tconst char *remote)\n {\n+\tstruct promisor_remote *promisor = promisor_remote_find(remote);\n+\n \t/*\n \t * Parse default value, but silently ignore it if it is invalid.\n \t */\n-\tif (!core_partial_clone_filter_default)\n-\t\treturn;\n-\tgently_parse_list_objects_filter(filter_options,\n-\t\t\t\t\t core_partial_clone_filter_default,\n-\t\t\t\t\t NULL);\n+\tif (promisor)\n+\t\tgently_parse_list_objects_filter(filter_options,\n+\t\t\t\t\t\t promisor->partial_clone_filter,\n+\t\t\t\t\t\t NULL);\n }\ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex e3adc78ebf..70d27f44ef 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -89,6 +89,7 @@ void partial_clone_register(\n \tconst char *remote,\n \tconst struct list_objects_filter_options *filter_options);\n void partial_clone_get_default_filter_spec(\n-\tstruct list_objects_filter_options *filter_options);\n+\tstruct list_objects_filter_options *filter_options,\n+\tconst char *remote);\n \n #endif /* LIST_OBJECTS_FILTER_OPTIONS_H */\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex dcf6ef6521..ed33dfa86c 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -70,6 +70,16 @@ static int promisor_remote_config(const char *var, const char *value, void *data\n \t\tfree(remote_name);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(subkey, \"partialclonefilter\")) {\n+\t\tchar *remote_name = xmemdupz(name, namelen);\n+\n+\t\to = promisor_remote_look_up(remote_name, NULL);\n+\t\tif (!o)\n+\t\t\to = promisor_remote_new(remote_name);\n+\n+\t\tfree(remote_name);\n+\t\treturn git_config_string(&o->partial_clone_filter, var, value);\n+\t}\n \n \treturn 0;\n }\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex f96722bc66..f0d609a3f5 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -3,10 +3,13 @@\n \n /*\n * Promisor remote linked list\n- * Its information come from remote.XXX config entries.\n+ *\n+ * Information in its fields come from remote.XXX config entries or\n+ * from extensions.partialclone or core.partialclonefilter.\n */\n struct promisor_remote {\n \tconst char *remote_name;\n+\tconst char *partial_clone_filter;\n \tstruct promisor_remote *next;\n };\n \ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex bce02788e6..9266037714 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -26,7 +26,7 @@ promise_and_delete () {\n test_expect_success 'extensions.partialclone without filter' '\n \ttest_create_repo server &&\n \tgit clone --filter=\"blob:none\" \"file://$(pwd)/server\" client &&\n-\tgit -C client config --unset core.partialclonefilter &&\n+\tgit -C client config --unset remote.origin.partialclonefilter &&\n \tgit -C client fetch origin\n '\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex dd658f8b32..2e82c70f33 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -654,6 +654,7 @@ partial_clone () {\n \n \t# Ensure that unneeded blobs are not inadvertently fetched.\n \ttest_config -C client remote.origin.promisor \"false\" &&\n+\tgit -C client config --unset remote.origin.partialclonefilter &&\n \ttest_must_fail git -C client cat-file -e \"$HASH1\" &&\n \n \t# But this blob was fetched, because clone performs an initial checkout\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex e8ca825ab7..ba0489b41a 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -43,7 +43,7 @@ test_expect_success 'do partial clone 1' '\n \ttest_cmp expect_1.oids observed.oids &&\n \ttest \"$(git -C pc1 config --local core.repositoryformatversion)\" = \"1\" &&\n \ttest \"$(git -C pc1 config --local remote.origin.promisor)\" = \"true\" &&\n-\ttest \"$(git -C pc1 config --local core.partialclonefilter)\" = \"blob:none\"\n+\ttest \"$(git -C pc1 config --local remote.origin.partialclonefilter)\" = \"blob:none\"\n '\n \n # checkout master to force dynamic object fetch of blobs at HEAD.\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371255","messageId":"20190312132959.11764-9-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 08/11] builtin/fetch: remove unique promisor remote limitation","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:56Z","receivedAt":"2019-03-12T13:30:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"As the infrastructure for more than one promisor remote\nhas been introduced in previous patches, we can remove\ncode that forbids the registration of more than one\npromisor remote.\n\nSigned-off-by: Christian Couder \n---\n builtin/fetch.c | 20 +++++---------------\n 1 file changed, 5 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9d436bd727..56c6827165 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1465,26 +1465,16 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n \t\treturn;\n \n \t/*\n-\t * If this is the FIRST partial-fetch request, we enable partial\n-\t * on this repo and remember the given filter-spec as the default\n-\t * for subsequent fetches to this remote.\n+\t * If this is a partial-fetch request, we enable partial on\n+\t * this repo if not already enabled and remember the given\n+\t * filter-spec as the default for subsequent fetches to this\n+\t * remote.\n \t */\n-\tif (!has_promisor_remote() && filter_options.choice) {\n+\tif (filter_options.choice) {\n \t\tpartial_clone_register(remote->name, &filter_options);\n \t\treturn;\n \t}\n \n-\t/*\n-\t * We are currently limited to only ONE promisor remote and only\n-\t * allow partial-fetches from the promisor remote.\n-\t */\n-\tif (!promisor_remote_find(remote->name)) {\n-\t\tif (filter_options.choice)\n-\t\t\tdie(_(\"--filter can only be used with the remote \"\n-\t\t\t \"configured in extensions.partialClone\"));\n-\t\treturn;\n-\t}\n-\n \t/*\n \t * Do a partial-fetch from the promisor remote using either the\n \t * explicitly given filter-spec or inherit the filter-spec from\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371256","messageId":"20190312132959.11764-7-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 06/11] Use promisor_remote_get_direct() and has_promisor_remote()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:54Z","receivedAt":"2019-03-12T13:30:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Instead of using the repository_format_partial_clone global\nand fetch_objects() directly, let's use has_promisor_remote()\nand promisor_remote_get_direct().\n\nThis way all the configured promisor remotes will be taken\ninto account, not only the one specified by\nextensions.partialClone.\n\nAlso when cloning or fetching using a partial clone filter,\nremote.origin.promisor will be set to \"true\" instead of\nsetting extensions.partialClone to \"origin\". This makes it\npossible to use many promisor remote just by fetching from\nthem.\n\nSigned-off-by: Christian Couder \n---\n builtin/cat-file.c | 5 +++--\n builtin/fetch.c | 11 ++++++-----\n builtin/gc.c | 3 ++-\n builtin/repack.c | 3 ++-\n cache-tree.c | 3 ++-\n connected.c | 3 ++-\n list-objects-filter-options.c | 28 +++++++++++++++-------------\n packfile.c | 3 ++-\n sha1-file.c | 14 ++++++++------\n t/t5601-clone.sh | 2 +-\n t/t5616-partial-clone.sh | 2 +-\n unpack-trees.c | 6 +++---\n 12 files changed, 47 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 0f092382e1..85ae10bf0b 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -15,6 +15,7 @@\n #include \"sha1-array.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n+#include \"promisor-remote.h\"\n \n struct batch_options {\n \tint enabled;\n@@ -523,8 +524,8 @@ static int batch_objects(struct batch_options *opt)\n \tif (opt->all_objects) {\n \t\tstruct object_cb_data cb;\n \n-\t\tif (repository_format_partial_clone)\n-\t\t\twarning(\"This repository has extensions.partialClone set. Some objects may not be loaded.\");\n+\t\tif (has_promisor_remote())\n+\t\t\twarning(\"This repository uses promisor remotes. Some objects may not be loaded.\");\n \n \t\tcb.opt = opt;\n \t\tcb.expand = &data;\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b620fd54b4..7edf70ae6c 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -23,6 +23,7 @@\n #include \"packfile.h\"\n #include \"list-objects-filter-options.h\"\n #include \"commit-reach.h\"\n+#include \"promisor-remote.h\"\n \n static const char * const builtin_fetch_usage[] = {\n \tN_(\"git fetch [] [ [...]]\"),\n@@ -1460,7 +1461,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n \t * If no prior partial clone/fetch and the current fetch DID NOT\n \t * request a partial-fetch, do a normal fetch.\n \t */\n-\tif (!repository_format_partial_clone && !filter_options.choice)\n+\tif (!has_promisor_remote() && !filter_options.choice)\n \t\treturn;\n \n \t/*\n@@ -1468,7 +1469,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n \t * on this repo and remember the given filter-spec as the default\n \t * for subsequent fetches to this remote.\n \t */\n-\tif (!repository_format_partial_clone && filter_options.choice) {\n+\tif (!has_promisor_remote() && filter_options.choice) {\n \t\tpartial_clone_register(remote->name, &filter_options);\n \t\treturn;\n \t}\n@@ -1477,7 +1478,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n \t * We are currently limited to only ONE promisor remote and only\n \t * allow partial-fetches from the promisor remote.\n \t */\n-\tif (strcmp(remote->name, repository_format_partial_clone)) {\n+\tif (!promisor_remote_find(remote->name)) {\n \t\tif (filter_options.choice)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t \"configured in extensions.partialClone\"));\n@@ -1609,7 +1610,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (depth || deepen_since || deepen_not.nr)\n \t\tdeepen = 1;\n \n-\tif (filter_options.choice && !repository_format_partial_clone)\n+\tif (filter_options.choice && !has_promisor_remote())\n \t\tdie(\"--filter can only be used when extensions.partialClone is set\");\n \n \tif (all) {\n@@ -1643,7 +1644,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (remote) {\n-\t\tif (filter_options.choice || repository_format_partial_clone)\n+\t\tif (filter_options.choice || has_promisor_remote())\n \t\t\tfetch_one_setup_partial(remote);\n \t\tresult = fetch_one(remote, argc, argv, prune_tags_ok);\n \t} else {\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 020f725acc..0bec41b25f 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -27,6 +27,7 @@\n #include \"pack-objects.h\"\n #include \"blob.h\"\n #include \"tree.h\"\n+#include \"promisor-remote.h\"\n \n #define FAILED_RUN \"failed to run %s\"\n \n@@ -640,7 +641,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\targv_array_push(&prune, prune_expire);\n \t\t\tif (quiet)\n \t\t\t\targv_array_push(&prune, \"--no-progress\");\n-\t\t\tif (repository_format_partial_clone)\n+\t\t\tif (has_promisor_remote())\n \t\t\t\targv_array_push(&prune,\n \t\t\t\t\t\t\"--exclude-promisor-objects\");\n \t\t\tif (run_command_v_opt(prune.argv, RUN_GIT_CMD))\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 67f8978043..3b935690c8 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -11,6 +11,7 @@\n #include \"midx.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n+#include \"promisor-remote.h\"\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n@@ -366,7 +367,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targv_array_push(&cmd.args, \"--all\");\n \targv_array_push(&cmd.args, \"--reflog\");\n \targv_array_push(&cmd.args, \"--indexed-objects\");\n-\tif (repository_format_partial_clone)\n+\tif (has_promisor_remote())\n \t\targv_array_push(&cmd.args, \"--exclude-promisor-objects\");\n \tif (write_bitmaps)\n \t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\ndiff --git a/cache-tree.c b/cache-tree.c\nindex b13bfaf71e..64c285a746 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -5,6 +5,7 @@\n #include \"cache-tree.h\"\n #include \"object-store.h\"\n #include \"replace-object.h\"\n+#include \"promisor-remote.h\"\n \n #ifndef DEBUG\n #define DEBUG 0\n@@ -357,7 +358,7 @@ static int update_one(struct cache_tree *it,\n \t\t}\n \n \t\tce_missing_ok = mode == S_IFGITLINK || missing_ok ||\n-\t\t\t(repository_format_partial_clone &&\n+\t\t\t(has_promisor_remote() &&\n \t\t\t ce_skip_worktree(ce));\n \t\tif (is_null_oid(oid) ||\n \t\t (!ce_missing_ok && !has_object_file(oid))) {\ndiff --git a/connected.c b/connected.c\nindex 1bba888eff..0eaaedee6a 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -4,6 +4,7 @@\n #include \"connected.h\"\n #include \"transport.h\"\n #include \"packfile.h\"\n+#include \"promisor-remote.h\"\n \n /*\n * If we feed all the commits we want to verify to this command\n@@ -56,7 +57,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \targv_array_push(&rev_list.args,\"rev-list\");\n \targv_array_push(&rev_list.args, \"--objects\");\n \targv_array_push(&rev_list.args, \"--stdin\");\n-\tif (repository_format_partial_clone)\n+\tif (has_promisor_remote())\n \t\targv_array_push(&rev_list.args, \"--exclude-promisor-objects\");\n \tif (!opt->is_deepening_fetch) {\n \t\targv_array_push(&rev_list.args, \"--not\");\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex c0036f7378..f41a831fce 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -6,6 +6,7 @@\n #include \"list-objects.h\"\n #include \"list-objects-filter.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"promisor-remote.h\"\n \n /*\n * Parse value of the argument to the \"filter\" keyword.\n@@ -144,30 +145,31 @@ void partial_clone_register(\n \tconst char *remote,\n \tconst struct list_objects_filter_options *filter_options)\n {\n-\t/*\n-\t * Record the name of the partial clone remote in the\n-\t * config and in the global variable -- the latter is\n-\t * used throughout to indicate that partial clone is\n-\t * enabled and to expect missing objects.\n-\t */\n-\tif (repository_format_partial_clone &&\n-\t *repository_format_partial_clone &&\n-\t strcmp(remote, repository_format_partial_clone))\n-\t\tdie(_(\"cannot change partial clone promisor remote\"));\n+\tchar *cfg_name;\n \n-\tgit_config_set(\"core.repositoryformatversion\", \"1\");\n-\tgit_config_set(\"extensions.partialclone\", remote);\n+\t/* Check if it is already registered */\n+\tif (!promisor_remote_find(remote)) {\n+\t\tgit_config_set(\"core.repositoryformatversion\", \"1\");\n \n-\trepository_format_partial_clone = xstrdup(remote);\n+\t\t/* Add promisor config for the remote */\n+\t\tcfg_name = xstrfmt(\"remote.%s.promisor\", remote);\n+\t\tgit_config_set(cfg_name, \"true\");\n+\t\tfree(cfg_name);\n+\t}\n \n \t/*\n \t * Record the initial filter-spec in the config as\n \t * the default for subsequent fetches from this remote.\n+\t *\n+\t * TODO: record it into remote..partialclonefilter\n \t */\n \tcore_partial_clone_filter_default =\n \t\txstrdup(filter_options->filter_spec);\n \tgit_config_set(\"core.partialclonefilter\",\n \t\t core_partial_clone_filter_default);\n+\n+\t/* Make sure the config info are reset */\n+\tpromisor_remote_reinit();\n }\n \n void partial_clone_get_default_filter_spec(\ndiff --git a/packfile.c b/packfile.c\nindex 16bcb75262..0f95cdc1c9 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -16,6 +16,7 @@\n #include \"tree.h\"\n #include \"object-store.h\"\n #include \"midx.h\"\n+#include \"promisor-remote.h\"\n \n char *odb_pack_name(struct strbuf *buf,\n \t\t const unsigned char *sha1,\n@@ -2119,7 +2120,7 @@ int is_promisor_object(const struct object_id *oid)\n \tstatic int promisor_objects_prepared;\n \n \tif (!promisor_objects_prepared) {\n-\t\tif (repository_format_partial_clone) {\n+\t\tif (has_promisor_remote()) {\n \t\t\tfor_each_packed_object(add_promisor_object,\n \t\t\t\t\t &promisor_objects,\n \t\t\t\t\t FOR_EACH_OBJECT_PROMISOR_ONLY);\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 01cc0590f4..715a2b882a 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -32,6 +32,7 @@\n #include \"packfile.h\"\n #include \"fetch-object.h\"\n #include \"object-store.h\"\n+#include \"promisor-remote.h\"\n \n /* The maximum size for an object header. */\n #define MAX_HEADER_LEN 32\n@@ -1369,15 +1370,16 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \t\t}\n \n \t\t/* Check if it is a missing object */\n-\t\tif (fetch_if_missing && repository_format_partial_clone &&\n+\t\tif (fetch_if_missing && has_promisor_remote() &&\n \t\t !already_retried && r == the_repository) {\n \t\t\t/*\n-\t\t\t * TODO Investigate checking fetch_object() return\n-\t\t\t * TODO value and stopping on error here.\n-\t\t\t * TODO Pass a repository struct through fetch_object,\n-\t\t\t * such that arbitrary repositories work.\n+\t\t\t * TODO Investigate checking promisor_remote_get_direct()\n+\t\t\t * TODO return value and stopping on error here.\n+\t\t\t * TODO Pass a repository struct through\n+\t\t\t * promisor_remote_get_direct(), such that arbitrary\n+\t\t\t * repositories work.\n \t\t\t */\n-\t\t\tfetch_objects(repository_format_partial_clone, real, 1);\n+\t\t\tpromisor_remote_get_direct(real, 1);\n \t\t\talready_retried = 1;\n \t\t\tcontinue;\n \t\t}\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex d6948cbdab..dd658f8b32 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -653,7 +653,7 @@ partial_clone () {\n \tgit -C client fsck &&\n \n \t# Ensure that unneeded blobs are not inadvertently fetched.\n-\ttest_config -C client extensions.partialclone \"not a remote\" &&\n+\ttest_config -C client remote.origin.promisor \"false\" &&\n \ttest_must_fail git -C client cat-file -e \"$HASH1\" &&\n \n \t# But this blob was fetched, because clone performs an initial checkout\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 9643acb161..e8ca825ab7 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -42,7 +42,7 @@ test_expect_success 'do partial clone 1' '\n \n \ttest_cmp expect_1.oids observed.oids &&\n \ttest \"$(git -C pc1 config --local core.repositoryformatversion)\" = \"1\" &&\n-\ttest \"$(git -C pc1 config --local extensions.partialclone)\" = \"origin\" &&\n+\ttest \"$(git -C pc1 config --local remote.origin.promisor)\" = \"true\" &&\n \ttest \"$(git -C pc1 config --local core.partialclonefilter)\" = \"blob:none\"\n '\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 22c41a3ba8..47353d85c3 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -17,6 +17,7 @@\n #include \"fsmonitor.h\"\n #include \"object-store.h\"\n #include \"fetch-object.h\"\n+#include \"promisor-remote.h\"\n \n /*\n * Error messages expected by scripts out of plumbing commands such as\n@@ -398,7 +399,7 @@ static int check_updates(struct unpack_trees_options *o)\n \t\tload_gitmodules_file(index, &state);\n \n \tenable_delayed_checkout(&state);\n-\tif (repository_format_partial_clone && o->update && !o->dry_run) {\n+\tif (has_promisor_remote() && o->update && !o->dry_run) {\n \t\t/*\n \t\t * Prefetch the objects that are to be checked out in the loop\n \t\t * below.\n@@ -415,8 +416,7 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\t}\n \t\t}\n \t\tif (to_fetch.nr)\n-\t\t\tfetch_objects(repository_format_partial_clone,\n-\t\t\t\t to_fetch.oid, to_fetch.nr);\n+\t\t\tpromisor_remote_get_direct(to_fetch.oid, to_fetch.nr);\n \t\tfetch_if_missing = fetch_if_missing_store;\n \t\toid_array_clear(&to_fetch);\n \t}\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371257","messageId":"20190312132959.11764-10-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 09/11] t0410: test fetching from many promisor remotes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:57Z","receivedAt":"2019-03-12T13:30:30Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nThis shows that it is now possible to fetch objects from more\nthan one promisor remote, and that fetching from a new\npromisor remote can configure it as one.\n\nSigned-off-by: Christian Couder \n---\n t/t0410-partial-clone.sh | 47 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 46 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 9266037714..146b0a1e03 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -182,8 +182,53 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n \tgrep \"git< fetch=.*ref-in-want\" trace\n '\n \n+test_expect_success 'fetching of missing objects from another promisor remote' '\n+\tgit clone \"file://$(pwd)/server\" server2 &&\n+\ttest_commit -C server2 bar &&\n+\tgit -C server2 repack -a -d --write-bitmap-index &&\n+\tHASH2=$(git -C server2 rev-parse bar) &&\n+\n+\tgit -C repo remote add server2 \"file://$(pwd)/server2\" &&\n+\tgit -C repo config remote.server2.promisor true &&\n+\tgit -C repo cat-file -p \"$HASH2\" &&\n+\n+\tgit -C repo fetch server2 &&\n+\trm -rf repo/.git/objects/* &&\n+\tgit -C repo cat-file -p \"$HASH2\" &&\n+\n+\t# Ensure that the .promisor file is written, and check that its\n+\t# associated packfile contains the object\n+\tls repo/.git/objects/pack/pack-*.promisor >promisorlist &&\n+\ttest_line_count = 1 promisorlist &&\n+\tIDX=$(cat promisorlist | sed \"s/promisor$/idx/\") &&\n+\tgit verify-pack --verbose \"$IDX\" | grep \"$HASH2\"\n+'\n+\n+test_expect_success 'fetching of missing objects configures a promisor remote' '\n+\tgit clone \"file://$(pwd)/server\" server3 &&\n+\ttest_commit -C server3 baz &&\n+\tgit -C server3 repack -a -d --write-bitmap-index &&\n+\tHASH3=$(git -C server3 rev-parse baz) &&\n+\tgit -C server3 config uploadpack.allowfilter 1 &&\n+\n+\trm repo/.git/objects/pack/pack-*.promisor &&\n+\n+\tgit -C repo remote add server3 \"file://$(pwd)/server3\" &&\n+\tgit -C repo fetch --filter=\"blob:none\" server3 $HASH3 &&\n+\n+\ttest \"$(git -C repo config remote.server3.promisor)\" = \"true\" &&\n+\n+\t# Ensure that the .promisor file is written, and check that its\n+\t# associated packfile contains the object\n+\tls repo/.git/objects/pack/pack-*.promisor >promisorlist &&\n+\ttest_line_count = 1 promisorlist &&\n+\tIDX=$(cat promisorlist | sed \"s/promisor$/idx/\") &&\n+\tgit verify-pack --verbose \"$IDX\" | grep \"$HASH3\"\n+'\n+\n test_expect_success 'fetching of missing blobs works' '\n-\trm -rf server repo &&\n+\trm -rf server server2 repo &&\n+\trm -rf server server3 repo &&\n \ttest_create_repo server &&\n \ttest_commit -C server foo &&\n \tgit -C server repack -a -d --write-bitmap-index &&\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371258","messageId":"20190312132959.11764-11-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 10/11] partial-clone: add multiple remotes in the doc","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:58Z","receivedAt":"2019-03-12T13:30:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"While at it, let's remove a reference to ODB effort as the ODB\neffort has been replaced by directly enhancing partial clone\nand promisor remote features.\n\nSigned-off-by: Christian Couder \n---\n Documentation/technical/partial-clone.txt | 83 ++++++++++++++++-------\n 1 file changed, 58 insertions(+), 25 deletions(-)\n\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex 896c7b3878..58adcc5ce1 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -100,18 +100,18 @@ or commits that reference missing trees.\n Handling Missing Objects\n ------------------------\n \n-- An object may be missing due to a partial clone or fetch, or missing due\n- to repository corruption. To differentiate these cases, the local\n- repository specially indicates such filtered packfiles obtained from the\n- promisor remote as \"promisor packfiles\".\n+- An object may be missing due to a partial clone or fetch, or missing\n+ due to repository corruption. To differentiate these cases, the\n+ local repository specially indicates such filtered packfiles\n+ obtained from promisor remotes as \"promisor packfiles\".\n +\n These promisor packfiles consist of a \".promisor\" file with\n arbitrary contents (like the \".keep\" files), in addition to\n their \".pack\" and \".idx\" files.\n \n - The local repository considers a \"promisor object\" to be an object that\n- it knows (to the best of its ability) that the promisor remote has promised\n- that it has, either because the local repository has that object in one of\n+ it knows (to the best of its ability) that promisor remotes have promised\n+ that they have, either because the local repository has that object in one of\n its promisor packfiles, or because another promisor object refers to it.\n +\n When Git encounters a missing object, Git can see if it is a promisor object\n@@ -123,7 +123,7 @@ expensive-to-modify list of missing objects.[a]\n - Since almost all Git code currently expects any referenced object to be\n present locally and because we do not want to force every command to do\n a dry-run first, a fallback mechanism is added to allow Git to attempt\n- to dynamically fetch missing objects from the promisor remote.\n+ to dynamically fetch missing objects from promisor remotes.\n +\n When the normal object lookup fails to find an object, Git invokes\n fetch-object to try to get the object from the server and then retry\n@@ -157,8 +157,7 @@ and prefetch those objects in bulk.\n +\n We are not happy with this global variable and would like to remove it,\n but that requires significant refactoring of the object code to pass an\n-additional flag. We hope that concurrent efforts to add an ODB API can\n-encompass this.\n+additional flag.\n \n \n Fetching Missing Objects\n@@ -182,21 +181,53 @@ has been updated to not use any object flags when the corresponding argument\n though they are not necessary.\n \n \n+Using many promisor remotes\n+---------------------------\n+\n+Many promisor remotes can be configured and used.\n+\n+This allows for example a user to have multiple geographically-close\n+cache servers for fetching missing blobs while continuing to do\n+filtered `git-fetch` commands from the central server.\n+\n+When fetching objects, promisor remotes are tried one after the other\n+until all the objects have been fetched.\n+\n+Remotes that are considered \"promisor\" remotes are those specified by\n+the following configuration variables:\n+\n+- `extensions.partialClone = `\n+\n+- `remote..promisor = true`\n+\n+- `remote..partialCloneFilter = ...`\n+\n+Only one promisor remote can be configured using the\n+`extensions.partialClone` config variable. This promisor remote will\n+be the last one tried when fetching objects.\n+\n+The other promisor remotes will be tried in the order they appear in\n+the config file.\n+\n Current Limitations\n -------------------\n \n-- The remote used for a partial clone (or the first partial fetch\n- following a regular clone) is marked as the \"promisor remote\".\n+- It is not possible to specify the order in which the promisor\n+ remotes are tried in other ways than the order in which they appear\n+ in the config file.\n +\n-We are currently limited to a single promisor remote and only that\n-remote may be used for subsequent partial fetches.\n+It is also not possible to specify an order to be used when fetching\n+from one remote and a different order when fetching from another\n+remote.\n+\n+- It is not possible to push only specific objects to a promisor\n+ remote.\n +\n-We accept this limitation because we believe initial users of this\n-feature will be using it on repositories with a strong single central\n-server.\n+It is not possible to push at the same time to multiple promisor\n+remote in a specific order.\n \n-- Dynamic object fetching will only ask the promisor remote for missing\n- objects. We assume that the promisor remote has a complete view of the\n+- Dynamic object fetching will only ask promisor remotes for missing\n+ objects. We assume that promisor remotes have a complete view of the\n repository and can satisfy all such requests.\n \n - Repack essentially treats promisor and non-promisor packfiles as 2\n@@ -218,15 +249,17 @@ server.\n Future Work\n -----------\n \n-- Allow more than one promisor remote and define a strategy for fetching\n- missing objects from specific promisor remotes or of iterating over the\n- set of promisor remotes until a missing object is found.\n+- Improve the way to specify the order in which promisor remotes are\n+ tried.\n +\n-A user might want to have multiple geographically-close cache servers\n-for fetching missing blobs while continuing to do filtered `git-fetch`\n-commands from the central server, for example.\n+For example this could allow to specify explicitly something like:\n+\"When fetching from this remote, I want to use these promisor remotes\n+in this order, though, when pushing or fetching to that remote, I want\n+to use those promisor remotes in that order.\"\n+\n+- Allow pushing to promisor remotes.\n +\n-Or the user might want to work in a triangular work flow with multiple\n+The user might want to work in a triangular work flow with multiple\n promisor remotes that each have an incomplete view of the repository.\n \n - Allow repack to work on promisor packfiles (while keeping them distinct\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371259","messageId":"20190312132959.11764-12-chriscool@tuxfamily.org","threadId":"50717","inReplyTo":"20190312132959.11764-1-chriscool@tuxfamily.org","subject":"[PATCH v3 11/11] remote: add promisor and partial clone config to the doc","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-12T13:29:59Z","receivedAt":"2019-03-12T13:30:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n Documentation/config/remote.txt | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\nindex 6c4cad83a2..a8e6437a90 100644\n--- a/Documentation/config/remote.txt\n+++ b/Documentation/config/remote.txt\n@@ -76,3 +76,11 @@ remote..pruneTags::\n +\n See also `remote..prune` and the PRUNING section of\n linkgit:git-fetch[1].\n+\n+remote..promisor::\n+\tWhen set to true, this remote will be used to fetch promisor\n+\tobjects.\n+\n+remote..partialclonefilter::\n+\tThe filter that will be applied when fetching from this\n+\tpromisor remote.\n-- \n2.21.0.166.gb5e4dbcfd3\n\n"},{"id":"371326","messageId":"xmqqtvg7e7pn.fsf@gitster-ct.c.googlers.com","threadId":"50717","inReplyTo":"20190312132959.11764-3-chriscool@tuxfamily.org","subject":"Re: [PATCH v3 02/11] Add initial support for many promisor remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-13T04:09:24Z","receivedAt":"2019-03-13T04:09:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> +struct promisor_remote *promisor_remote_new(const char *remote_name)\n> +{\n\nShouldn't this be static? The config callback that calls this\nfunction is inside this file.\n\n> +\tstruct promisor_remote *o;\n> +\n> +\to = xcalloc(1, sizeof(*o));\n> +\to->remote_name = xstrdup(remote_name);\n\nA comment on this later...\n\n> +static struct promisor_remote *promisor_remote_look_up(const char *remote_name,\n> +\t\t\t\t\t\t struct promisor_remote **previous)\n\nIn our codebase, this operation is far more often called \"lookup\",\none word, according to \"git grep -e look_up \\*.h\".\n\n> +{\n> +\tstruct promisor_remote *o, *p;\n> +\n> +\tfor (p = NULL, o = promisors; o; p = o, o = o->next)\n> +\t\tif (o->remote_name && !strcmp(o->remote_name, remote_name)) {\n> +\t\t\tif (previous)\n> +\t\t\t\t*previous = p;\n\nI think the \"previous\" thing is for the callers to learn what\npointer points at the found entry, allowing e.g. an element to be\ninserted just before the found element. If so, would it make more\nsense to use the more familiar pattern to use\n\n\t*previous = &promisors;\n\nhere? That would remove the need to switch on NULL-ness of previous\nin the caller.\n\n> diff --git a/promisor-remote.h b/promisor-remote.h\n> new file mode 100644\n> index 0000000000..bfbf7c0f21\n> --- /dev/null\n> +++ b/promisor-remote.h\n> @@ -0,0 +1,17 @@\n> +#ifndef PROMISOR_REMOTE_H\n> +#define PROMISOR_REMOTE_H\n> +\n> +/*\n> + * Promisor remote linked list\n> + * Its information come from remote.XXX config entries.\n> + */\n> +struct promisor_remote {\n> +\tconst char *remote_name;\n> +\tstruct promisor_remote *next;\n> +};\n\nWould it make the management of storage easier to make it\n\n\tstruct promisor_remote {\n\t\tstruct promisor_remote *next;\n\t\tconst char name[FLEX_ARRAY];\n\t};\n\nthat will allow allocation with\n\n\tstruct promisor_remote *r;\n\tFLEX_ALLOC_STR(r, name, remote_name);\n\nOr if the remote_name field must be a pointer, perhaps use\nFLEXPTR_ALLOC_STR().\n\nWhat is the rule for these promisor names? If these entries were on\nthe configuration file, then:\n\n\t[remote \"origin\"]\n\t\turl = ...\n\t\tpromisor = frotz\n\t\tpromisor = nitfol\n\n\t[remote \"mirror\"}\n\t\turl = ...\n\t\tpromisor = frotz\n\t\tpromisor = Frotz\n\t\tpromisor = nit fol\n\nwould the two \"frotz\" for the two remotes refer to the same thing,\nor are \"promisor\" values scoped to each remote?\n\nCan the name of promisor be any string? If they end up getting used\nas part of a path on the filesystem, we'd need to worry about case\nsensitivity and UTF-8 normalization issues as well.\n\nIn a large enough project where multi-promisor makes sense, what is\nthe expected number of promisors a repository would define? 10s?\n1000s? Would a linked list still make sense when deployed in the\nreal world, or would we be forced to move to something like hashmap\nlater?\n\nYou do not have to have the answers to all these questions, and even\nthe ones with concrete answers, you do not necessarily have to act\non them right now (e.g. you may anticipate the eventual need to move\nto hashmap, but prototyping with linked list is perfectly fine;\nbeing aware of the possibility alone would force us to be careful to\nmake sure that the implementation detail does not leak through too\nmuch and confined within _lookup(), _find(), etc. functions, and\nthat awareness is good enough at this point).\n\nThanks.\n"},{"id":"371327","messageId":"xmqqpnqve71d.fsf@gitster-ct.c.googlers.com","threadId":"50717","inReplyTo":"20190312132959.11764-4-chriscool@tuxfamily.org","subject":"Re: [PATCH v3 03/11] promisor-remote: implement promisor_remote_get_direct()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-13T04:23:58Z","receivedAt":"2019-03-13T04:24:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> @@ -98,3 +99,19 @@ int has_promisor_remote(void)\n> {\n> \treturn !!promisor_remote_find(NULL);\n> }\n> +\n> +int promisor_remote_get_direct(const struct object_id *oids, int oid_nr)\n> +{\n> +\tstruct promisor_remote *o;\n> +\n> +\tpromisor_remote_init();\n> +\n> +\tfor (o = promisors; o; o = o->next) {\n> +\t\tif (fetch_objects(o->remote_name, oids, oid_nr) < 0)\n> +\t\t\tcontinue;\n> +\t\treturn 0;\n\nSuppose the caller asks to fetch 3 objects, A, B and C, from two\npromisors. The first promisor can give you A and B but cannot give\nyou C. The second promisor only can give you C.\n\nDoes fetch_objects() return failure after attempting to fetch A, B\nand C from the first promisor? Then we go on to the second promisor\nbut do we ask all three? That would mean the second promisor will\nalso fail because it cannot give you A and B, and then the whole\nthing would fail. It would be nicer if the mechanism would allow us\nto fetch what is still missing from later promisor, perhaps.\n\nAs the original \"fetch\" protocol only allows you to fetch a pack\nwith everything you asked for in it, instead of feeding you a pack\nwith best effort, I think the answer to the above is \"it is very\nhard to improve over what we have here\", but people may have\ninteresting ideas ;-)\n\n> +\t}\n> +\n> +\treturn -1;\n> +}\n> +\n\nAdding trailing blank line at the end?\n\n> diff --git a/promisor-remote.h b/promisor-remote.h\n> index bfbf7c0f21..f9f5825417 100644\n> --- a/promisor-remote.h\n> +++ b/promisor-remote.h\n> @@ -13,5 +13,6 @@ struct promisor_remote {\n> extern struct promisor_remote *promisor_remote_new(const char *remote_name);\n> extern struct promisor_remote *promisor_remote_find(const char *remote_name);\n> extern int has_promisor_remote(void);\n> +extern int promisor_remote_get_direct(const struct object_id *oids, int oid_nr);\n> \n> #endif /* PROMISOR_REMOTE_H */\n"},{"id":"371328","messageId":"xmqqlg1je6uk.fsf@gitster-ct.c.googlers.com","threadId":"50717","inReplyTo":"20190312132959.11764-5-chriscool@tuxfamily.org","subject":"Re: [PATCH v3 04/11] promisor-remote: add promisor_remote_reinit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-13T04:28:03Z","receivedAt":"2019-03-13T04:28:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> From: Christian Couder \n>\n> We will need to reinitialize the promisor remote configuration\n> as we will make some changes to it in a later commit.\n>\n> Signed-off-by: Christian Couder \n> ---\n\nAt this point, turning \"initialized\" into a file-scope static, and \nbuilding reinit as\n\n\tvoid promisor_remote_reinit(void)\n\t{\n\t\tinitialized = 0;\n\t\t... clear existing \"promisor\" entries ...\n\t\tpromisor_remote_init();\n\t}\n\nmay make more sense.\n\n> -static void promisor_remote_init(void)\n> +static void promisor_remote_do_init(int force)\n> {\n> \tstatic int initialized;\n> \n> -\tif (initialized)\n> +\tif (!force && initialized)\n> \t\treturn;\n> \tinitialized = 1;\n> \n> \tgit_config(promisor_remote_config, NULL);\n\nThe promisors and promisors_tail would need to be reinitiazlied and\nexisting elements on the list purged. Otherwise, after removing an\nentry from the configuration file, the entry will still stay around.\n\n> }\n> \n> +static inline void promisor_remote_init(void)\n> +{\n> +\tpromisor_remote_do_init(0);\n> +}\n> +\n> +void promisor_remote_reinit(void)\n> +{\n> +\tpromisor_remote_do_init(1);\n> +}\n> +\n> struct promisor_remote *promisor_remote_find(const char *remote_name)\n> {\n> \tpromisor_remote_init();\n> diff --git a/promisor-remote.h b/promisor-remote.h\n> index f9f5825417..f96722bc66 100644\n> --- a/promisor-remote.h\n> +++ b/promisor-remote.h\n> @@ -10,6 +10,7 @@ struct promisor_remote {\n> \tstruct promisor_remote *next;\n> };\n> \n> +extern void promisor_remote_reinit(void);\n> extern struct promisor_remote *promisor_remote_new(const char *remote_name);\n> extern struct promisor_remote *promisor_remote_find(const char *remote_name);\n> extern int has_promisor_remote(void);\n"},{"id":"371329","messageId":"xmqqftrre6p1.fsf@gitster-ct.c.googlers.com","threadId":"50717","inReplyTo":"20190312132959.11764-6-chriscool@tuxfamily.org","subject":"Re: [PATCH v3 05/11] promisor-remote: use repository_format_partial_clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-13T04:31:22Z","receivedAt":"2019-03-13T04:31:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> A remote specified using the extensions.partialClone config\n> option should be considered a promisor remote too.\n>\n> This remote should be at the end of the promisor remote list,\n> so that it is used only if objects have not been found in other\n> remotes.\n\nThat's a declaration, not a rationale, and does not answer \"Why\nshould the origin be only used as the last resort?\".\n\n> Signed-off-by: Christian Couder \n> ---\n> promisor-remote.c | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n>\n> diff --git a/promisor-remote.c b/promisor-remote.c\n> index ea74f6d8a8..dcf6ef6521 100644\n> --- a/promisor-remote.c\n> +++ b/promisor-remote.c\n> @@ -83,6 +83,17 @@ static void promisor_remote_do_init(int force)\n> \tinitialized = 1;\n> \n> \tgit_config(promisor_remote_config, NULL);\n> +\n> +\tif (repository_format_partial_clone) {\n> +\t\tstruct promisor_remote *o, *previous;\n> +\n> +\t\to = promisor_remote_look_up(repository_format_partial_clone,\n> +\t\t\t\t\t &previous);\n> +\t\tif (o)\n> +\t\t\tpromisor_remote_move_to_tail(o, previous);\n> +\t\telse\n> +\t\t\tpromisor_remote_new(repository_format_partial_clone);\n> +\t}\n> }\n> \n> static inline void promisor_remote_init(void)\n"},{"id":"371330","messageId":"xmqqbm2fe6jr.fsf@gitster-ct.c.googlers.com","threadId":"50717","inReplyTo":"xmqqtvg7e7pn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 02/11] Add initial support for many promisor remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-13T04:34:32Z","receivedAt":"2019-03-13T04:34:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> What is the rule for these promisor names?\n\nDisregard this part (and only this part). The values are not names,\nbut just \"is this thing a promisor\" boolean.\n"},{"id":"372915","messageId":"CAP8UFD208vY=0tduwSipBHYTPJCrBtsME6GouZMiKrnXJ=0zAw@mail.gmail.com","threadId":"50717","inReplyTo":"xmqqtvg7e7pn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 02/11] Add initial support for many promisor remotes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-01T16:41:33Z","receivedAt":"2019-04-01T16:41:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Mar 13, 2019 at 5:09 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > +struct promisor_remote *promisor_remote_new(const char *remote_name)\n> > +{\n>\n> Shouldn't this be static? The config callback that calls this\n> function is inside this file.\n\nYeah, I made it static.\n\n> > + struct promisor_remote *o;\n> > +\n> > + o = xcalloc(1, sizeof(*o));\n> > + o->remote_name = xstrdup(remote_name);\n>\n> A comment on this later...\n>\n> > +static struct promisor_remote *promisor_remote_look_up(const char *remote_name,\n> > + struct promisor_remote **previous)\n>\n> In our codebase, this operation is far more often called \"lookup\",\n> one word, according to \"git grep -e look_up \\*.h\".\n\nOk, I changed it to \"lookup\".\n\n> > +{\n> > + struct promisor_remote *o, *p;\n> > +\n> > + for (p = NULL, o = promisors; o; p = o, o = o->next)\n> > + if (o->remote_name && !strcmp(o->remote_name, remote_name)) {\n> > + if (previous)\n> > + *previous = p;\n>\n> I think the \"previous\" thing is for the callers to learn what\n> pointer points at the found entry, allowing e.g. an element to be\n> inserted just before the found element.\n\nActually it's to make it easy to move the found element.\n\n> If so, would it make more\n> sense to use the more familiar pattern to use\n>\n> *previous = &promisors;\n>\n> here?\n\nIf I do that I get an \"error: assignment from incompatible pointer\ntype\" as \"*previous\" is of type \"struct promisor_remote *\" while\n\"&promisors\" is of type \"struct promisor_remote **\".\n\nMaybe you mean:\n\n *previous = promisors;\n\nbut I fail to see how that would correctly pass the previous element\nwhen the found one is not the first one.\n\n> That would remove the need to switch on NULL-ness of previous\n> in the caller.\n\nIn the only caller that passes a non NULL previous, we call\npromisor_remote_move_to_tail() which does:\n\n if (previous)\n previous->next = o->next;\n else\n promisors = o->next ? o->next : o;\n\nSo yeah we check the NULL-ness of previous, but if previous has been\nset to promisors, then previous->next = o->next will not set promisors\ncorrectly.\n\nI guess we are not here in the case were the familiar pattern you are\nthinking about can be applied. Or is there an example, maybe in the\nGit source code, that I could learn from?\n\nAnother possibility is to just use hashmap as you suggest below or\nlist.h. It might be a bit wasteful, but the code simplification might\nbe worth it.\n\n> > diff --git a/promisor-remote.h b/promisor-remote.h\n> > new file mode 100644\n> > index 0000000000..bfbf7c0f21\n> > --- /dev/null\n> > +++ b/promisor-remote.h\n> > @@ -0,0 +1,17 @@\n> > +#ifndef PROMISOR_REMOTE_H\n> > +#define PROMISOR_REMOTE_H\n> > +\n> > +/*\n> > + * Promisor remote linked list\n> > + * Its information come from remote.XXX config entries.\n> > + */\n> > +struct promisor_remote {\n> > + const char *remote_name;\n> > + struct promisor_remote *next;\n> > +};\n>\n> Would it make the management of storage easier to make it\n>\n> struct promisor_remote {\n> struct promisor_remote *next;\n> const char name[FLEX_ARRAY];\n> };\n>\n> that will allow allocation with\n>\n> struct promisor_remote *r;\n> FLEX_ALLOC_STR(r, name, remote_name);\n\nOk to use a flex array. If we ever use arrays or hashmaps of promisor\nremotes, we might have to go back to not using one.\n\n> Or if the remote_name field must be a pointer, perhaps use\n> FLEXPTR_ALLOC_STR().\n\n[...]\n\n> Can the name of promisor be any string? If they end up getting used\n> as part of a path on the filesystem, we'd need to worry about case\n> sensitivity and UTF-8 normalization issues as well.\n\nIt looks like for regular remotes we only check if they start with /.\nSo I don't think we need to do more than that for promisor remotes. I\nadded the check.\n\n> In a large enough project where multi-promisor makes sense, what is\n> the expected number of promisors a repository would define? 10s?\n> 1000s? Would a linked list still make sense when deployed in the\n> real world, or would we be forced to move to something like hashmap\n> later?\n\nI am ok to use hashmap to make it similar with regular remotes.\n\nFor now I don't expect large projects to use more than 10s promisors\nthough. They are defined in the config file and I don't think people\nwill be happy if they have to manage more than 10s promisors in their\nconfig file. If people really start to use more than that, they are\nlikely to ask us for a new mechanism to manage them (and to\nautomatically have them configured from servers). So maybe we can\nchange that if/when we have to work on such mechanism.\n\n\n\n\n> You do not have to have the answers to all these questions, and even\n> the ones with concrete answers, you do not necessarily have to act\n> on them right now (e.g. you may anticipate the eventual need to move\n> to hashmap, but prototyping with linked list is perfectly fine;\n> being aware of the possibility alone would force us to be careful to\n> make sure that the implementation detail does not leak through too\n> much and confined within _lookup(), _find(), etc. functions, and\n> that awareness is good enough at this point).\n>\n> Thanks.\n"},{"id":"372918","messageId":"CAP8UFD1SK2mFBYJLqY9VE_Z5tQ5Mor2guc-X3yTbzfUZbHM_2Q@mail.gmail.com","threadId":"50717","inReplyTo":"xmqqpnqve71d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 03/11] promisor-remote: implement promisor_remote_get_direct()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-01T16:41:44Z","receivedAt":"2019-04-01T16:41:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Mar 13, 2019 at 5:24 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n\n> > +int promisor_remote_get_direct(const struct object_id *oids, int oid_nr)\n> > +{\n> > + struct promisor_remote *o;\n> > +\n> > + promisor_remote_init();\n> > +\n> > + for (o = promisors; o; o = o->next) {\n> > + if (fetch_objects(o->remote_name, oids, oid_nr) < 0)\n> > + continue;\n> > + return 0;\n>\n> Suppose the caller asks to fetch 3 objects, A, B and C, from two\n> promisors. The first promisor can give you A and B but cannot give\n> you C. The second promisor only can give you C.\n>\n> Does fetch_objects() return failure after attempting to fetch A, B\n> and C from the first promisor?\n\nYes, I think it should still send the objects it has and then fail.\n\nMaybe it doesn't work like this right now (I haven't checked), or\nmaybe the failure should be different than a regular failure. In those\ncases I can maybe improve on that in a later iteration.\n\n> Then we go on to the second promisor\n> but do we ask all three? That would mean the second promisor will\n> also fail because it cannot give you A and B, and then the whole\n> thing would fail. It would be nicer if the mechanism would allow us\n> to fetch what is still missing from later promisor, perhaps.\n\nYeah, I implemented fetching only what is still missing in v4.\n\n> As the original \"fetch\" protocol only allows you to fetch a pack\n> with everything you asked for in it, instead of feeding you a pack\n> with best effort, I think the answer to the above is \"it is very\n> hard to improve over what we have here\", but people may have\n> interesting ideas ;-)\n\nI think we should make it best effort when acting as a promisor\nremote. I will take a look at that.\n\n> > + }\n> > +\n> > + return -1;\n> > +}\n> > +\n>\n> Adding trailing blank line at the end?\n\nRemoved.\n"},{"id":"372919","messageId":"CAP8UFD1n9=DH-AA+7u10Xk=7Rs3Kq+Bn4AdyxoG+cu2_pOQKvA@mail.gmail.com","threadId":"50717","inReplyTo":"xmqqlg1je6uk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 04/11] promisor-remote: add promisor_remote_reinit()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-01T16:41:59Z","receivedAt":"2019-04-01T16:42:16Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Mar 13, 2019 at 5:28 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > From: Christian Couder \n> >\n> > We will need to reinitialize the promisor remote configuration\n> > as we will make some changes to it in a later commit.\n> >\n> > Signed-off-by: Christian Couder \n> > ---\n>\n> At this point, turning \"initialized\" into a file-scope static, and\n> building reinit as\n>\n> void promisor_remote_reinit(void)\n> {\n> initialized = 0;\n> ... clear existing \"promisor\" entries ...\n> promisor_remote_init();\n> }\n>\n> may make more sense.\n\nOk, I implemented that.\n\n> > -static void promisor_remote_init(void)\n> > +static void promisor_remote_do_init(int force)\n> > {\n> > static int initialized;\n> >\n> > - if (initialized)\n> > + if (!force && initialized)\n> > return;\n> > initialized = 1;\n> >\n> > git_config(promisor_remote_config, NULL);\n>\n> The promisors and promisors_tail would need to be reinitiazlied and\n> existing elements on the list purged. Otherwise, after removing an\n> entry from the configuration file, the entry will still stay around.\n\nYeah, that's in the new version too.\n\n> > }\n"},{"id":"372920","messageId":"CAP8UFD1K=n1CriNSY75MyBG7ZDYUQRq3LcEMrxTJVcyhQgyJrg@mail.gmail.com","threadId":"50717","inReplyTo":"xmqqftrre6p1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 05/11] promisor-remote: use repository_format_partial_clone","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-01T16:42:19Z","receivedAt":"2019-04-01T16:42:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Mar 13, 2019 at 5:31 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > A remote specified using the extensions.partialClone config\n> > option should be considered a promisor remote too.\n> >\n> > This remote should be at the end of the promisor remote list,\n> > so that it is used only if objects have not been found in other\n> > remotes.\n>\n> That's a declaration, not a rationale, and does not answer \"Why\n> should the origin be only used as the last resort?\".\n\nI have added the following explanation:\n\n If we are using promisor remotes other than the origin, it is\n because these other promisor remotes are likely to be better\n for some reason (maybe they are closer or faster for some kind\n of objects) than the origin, so it's logical that we try to use\n them first.\n"},{"id":"372922","messageId":"xmqqbm1pvdri.fsf@gitster-ct.c.googlers.com","threadId":"50717","inReplyTo":"CAP8UFD1K=n1CriNSY75MyBG7ZDYUQRq3LcEMrxTJVcyhQgyJrg@mail.gmail.com","subject":"Re: [PATCH v3 05/11] promisor-remote: use repository_format_partial_clone","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-01T17:25:37Z","receivedAt":"2019-04-01T17:53:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Wed, Mar 13, 2019 at 5:31 AM Junio C Hamano wrote:\n>>\n>> Christian Couder writes:\n>>\n>> > A remote specified using the extensions.partialClone config\n>> > option should be considered a promisor remote too.\n>> >\n>> > This remote should be at the end of the promisor remote list,\n>> > so that it is used only if objects have not been found in other\n>> > remotes.\n>>\n>> That's a declaration, not a rationale, and does not answer \"Why\n>> should the origin be only used as the last resort?\".\n>\n> I have added the following explanation:\n>\n> If we are using promisor remotes other than the origin, it is\n> because these other promisor remotes are likely to be better\n> for some reason (maybe they are closer or faster for some kind\n> of objects) than the origin, so it's logical that we try to use\n> them first.\n\nAlternatively, perhaps you cloned from the origin repository which\nis a mirror that is not as reliable as the real thing but is faster\nto access when it _is_ up, and you added the true source that\n'origin' mirrors from as the fallback, as it should only be used\nwhen 'origin' is down.\n\nSo what you wrote is not a convincing explanation why we should\ntreat the origin as the fallback.\n\nWriting in the end-user documentation something like \"because the\n'origin' remote is treated as the last resort fallback repository,\nafter consulting other promisor remotes first and failing to obtain\nneeded objects, you may want to use repositories with better\navailability listed as 'other promisor remotes' and set the worst\nconnected remote as 'origin'\" may be a valid thing to do, though.\n\nI do not like to see the tool making such a policy decision for\nusers very much, but at least by honestly documenting, we explain\nwhat we unilaterally decide to do without having a strong\njustification (i.e. we could decide the other way and the decision\nwould be equally valid, and because there is no single best choice,\nwe arbitrarily picked one) to the users, and with that, they can\nadjust their use case to what the tool does to take advantage of the\ntool's design decision.\n"}]}