{"thread":{"id":"50903","subject":"[PATCH v5 01/16] t0410: remove pipes after git commands","startedAt":"2019-04-09T16:11:46Z","lastAt":"2019-06-25T13:50:36Z","messageCount":28,"participants":["Christian Couder","Junio C Hamano","Derrick Stolee","Johannes Schindelin"],"isPatch":true,"patchVersion":5,"patchTotal":16},"messages":[{"id":"373502","messageId":"20190409161116.30256-2-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 01/16] t0410: remove pipes after git commands","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:01Z","receivedAt":"2019-04-09T16:11:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Let's not run a git command, especially one with \"verify\" in its\nname, upstream of a pipe, because the pipe will hide the git\ncommand's exit code.\n\nWhile at it, let's also avoid a useless `cat` command piping\ninto `sed`.\n\nHelped-by: SZEDER Gábor \nSigned-off-by: Christian Couder \n---\n t/t0410-partial-clone.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 5bd892f2f7..3559313bd0 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -166,8 +166,9 @@ test_expect_success 'fetching of missing objects' '\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 \"$HASH\"\n+\tIDX=$(sed \"s/promisor$/idx/\" promisorlist) &&\n+\tgit verify-pack --verbose \"$IDX\" >out &&\n+\tgrep \"$HASH\" out\n '\n \n test_expect_success 'fetching of missing objects works with ref-in-want enabled' '\n@@ -514,8 +515,9 @@ test_expect_success 'fetching of missing objects from an HTTP server' '\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 \"$HASH\"\n+\tIDX=$(sed \"s/promisor$/idx/\" promisorlist) &&\n+\tgit verify-pack --verbose \"$IDX\" >out &&\n+\tgrep \"$HASH\" out\n '\n \n test_done\n-- \n2.21.0.750.g68c8ebb2ac\n\n"},{"id":"373503","messageId":"20190409161116.30256-1-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":null,"subject":"[PATCH v5 00/16] Many promisor remotes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:00Z","receivedAt":"2019-04-09T16:11:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch series is based on:\n\n763fb763b8 (Merge branch 'jt/batch-fetch-blobs-in-diff' into jch, 2019-04-08)\n\nto avoid issues with jt/batch-fetch-blobs-in-diff.\n\nIntroduction\n~~~~~~~~~~~~\n\nThis 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\nThe code might not work with many promisor remotes that don't all have\nall the promised objects, as that would require the fetch protocol to\nsend packs with best effort, as described by Junio in:\n\nhttps://public-inbox.org/git/xmqqpnqve71d.fsf@gitster-ct.c.googlers.com/\n\nIn general I have tried to change as few things as possible in the\nfirst patches of the series, though the last patches try to hide the\nold features that only made sense for the general code to use when\nthere was only one promisor remote.\n\nHigh level view of new patches since the V4\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nThere are 5 new patches in this series compared to V4. They are:\n\n - Patch 1/16 (t0410: remove pipes after git commands) \n\nThis is a preparatory cleanup patch to improve t0410 as suggested by\nSzeder Gábor.\n\n - Patch 8/16 (diff: use promisor-remote.h instead of fetch-object.h)\n\nThis patch applies to diff.c the same changes that are made in patch\n7/11. They are in a separate patch because the changes need to be made\nonly if the jt/batch-fetch-blobs-in-diff series is merged. So\ndepending on what happens to jt/batch-fetch-blobs-in-diff, this patch\ncan either be squashed into 7/11, be squashed in to\njt/batch-fetch-blobs-in-diff, or be dropped.\n\n - Patch 14/16 (Remove fetch-object.{c,h} in favor of promisor-remote.{c,h})\n - Patch 15/16 (Move repository_format_partial_clone to promisor-remote.c)\n - Patch 16/16 (Move core_partial_clone_filter_default to promisor-remote.c)\n\nThese patches try to hide the old features (fetch_objects(),\nrepository_format_partial_clone and core_partial_clone_filter_default)\nthat only made sense for the general code to use when there was only\none promisor remote. This ensures that there will be compilation\nerrors rather than bugs or test failures if the old features are used\nin the old fashion way.\n\nHigh level view of other changes since the V4\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nThe title and purpose of the old patches didn't change, but the\nfollowing changes were made:\n\n - Patch 3/16 (Add initial support for many promisor remotes)\n\nRemove check for \"r->name\" as \"name\" became a flex array in struct\npromisor_remote as suggested by Szeder Gábor and Junio.\n\n - Patch 4/16 (promisor-remote: implement promisor_remote_get_direct())\n\nAdded call to free() \"missing\" array.\n\nAdded \"struct object_id;\" declaration in \"promisor-remote.h\".\n\n - Patch 6/16 (promisor-remote: use repository_format_partial_clone)\n\nImproved explanation about why the remote specified using the\nextensions.partialClone config option is tried last.\n\n - Patch 7/16 (Use promisor_remote_get_direct() and has_promisor_remote())\n\nRemove now useless includes of \"fetch-object.h\" as suggested by Junio.\n\n - Patch 11/16 (t0410: test fetching from many promisor remotes)\n\nImprove tests as suggested by Szeder Gábor.\n\n - Patch 12/16 (partial-clone: add multiple remotes in the doc)\n\nAdd explanation in the doc about why the remote specified using the\nextensions.partialClone config option is tried last as suggested by\nJunio.\n\nOther doc improvements.\n\nHigh level overview of old patches in this patch series\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\n - Patch 2/16 (fetch-object: make functions return an error code)\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 3/16 (Add initial support for many promisor remotes)\n\nThis introduces the minimum infrastructure for promisor remotes.\n\n - Patch 4/16 (promisor-remote: implement promisor_remote_get_direct()) \n - Patch 5/16 (promisor-remote: add promisor_remote_reinit())\n - Patch 6/16 (promisor-remote: use repository_format_partial_clone)\n\nThese patches add a few missing bits in the promisor remote\ninfrastructure that will be needed in the following patches.\n\n - Patch 7/16 (Use promisor_remote_get_direct() and has_promisor_remote())\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 9/16 (promisor-remote: parse remote.*.partialclonefilter)\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 10/16 (builtin/fetch: remove unique promisor remote limitation)\n\nThis patch removes the limitation in builtin/fetch.c to have only one\npromisor remote.\n\n - Patch 12/16 (t0410: test fetching from many promisor remotes)\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 13/16 (partial-clone: add multiple remotes in the doc)\n - Patch 14/16 (remote: add promisor and partial clone config to the doc)\n\nThese documentation patches explain how things can work with more than\none promisor remote.\n\nLinks\n~~~~~\n\nThis patch series on GitHub:\n\nV5: https://github.com/chriscool/git/commits/many-promisor-remotes\nV4: https://github.com/chriscool/git/commits/many-promisor-remotes58\nV3: https://github.com/chriscool/git/commits/many-promisor-remotes40\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\nV4: https://public-inbox.org/git/20190401164045.17328-1-chriscool@tuxfamily.org/\nV3: https://public-inbox.org/git/20190312132959.11764-1-chriscool@tuxfamily.org/\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 (16):\n t0410: remove pipes after git commands\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 diff: use promisor-remote.h instead of fetch-object.h\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 Remove fetch-object.{c,h} in favor of promisor-remote.{c,h}\n Move repository_format_partial_clone to promisor-remote.c\n Move core_partial_clone_filter_default to promisor-remote.c\n\n Documentation/config/remote.txt | 8 +\n Documentation/technical/partial-clone.txt | 117 +++++++---\n Makefile | 2 +-\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 cache.h | 2 -\n config.c | 5 -\n connected.c | 3 +-\n diff.c | 8 +-\n environment.c | 2 -\n fetch-object.c | 40 ----\n fetch-object.h | 9 -\n list-objects-filter-options.c | 51 +++--\n list-objects-filter-options.h | 3 +-\n packfile.c | 3 +-\n promisor-remote.c | 264 ++++++++++++++++++++++\n promisor-remote.h | 29 +++\n setup.c | 3 +-\n sha1-file.c | 15 +-\n t/t0410-partial-clone.sh | 61 ++++-\n t/t5601-clone.sh | 3 +-\n t/t5616-partial-clone.sh | 4 +-\n unpack-trees.c | 7 +-\n 26 files changed, 514 insertions(+), 168 deletions(-)\n delete mode 100644 fetch-object.c\n delete mode 100644 fetch-object.h\n create mode 100644 promisor-remote.c\n create mode 100644 promisor-remote.h\n\n-- \n2.21.0.750.g68c8ebb2ac\n\n"},{"id":"373504","messageId":"20190409161116.30256-3-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 02/16] fetch-object: make functions return an error code","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:02Z","receivedAt":"2019-04-09T16:11:49Z","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 ad02649124..83358737f3 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1373,8 +1373,8 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \t\t !already_retried && r == the_repository &&\n \t\t !(flags & OBJECT_INFO_FOR_PREFETCH)) {\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.750.g68c8ebb2ac\n\n"},{"id":"373505","messageId":"20190409161116.30256-4-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 03/16] Add initial support for many promisor remotes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:03Z","receivedAt":"2019-04-09T16:11:50Z","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 \nHelped-by: SZEDER Gábor \nSigned-off-by: Christian Couder \n---\n Makefile | 1 +\n promisor-remote.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++\n promisor-remote.h | 16 +++++++++\n 3 files changed, 109 insertions(+)\n create mode 100644 promisor-remote.c\n create mode 100644 promisor-remote.h\n\ndiff --git a/Makefile b/Makefile\nindex 34fa9da39a..3523ae0517 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -954,6 +954,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..c249b80e02\n--- /dev/null\n+++ b/promisor-remote.c\n@@ -0,0 +1,92 @@\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+static struct promisor_remote *promisor_remote_new(const char *remote_name)\n+{\n+\tstruct promisor_remote *r;\n+\n+\tif (*remote_name == '/') {\n+\t\twarning(_(\"promisor remote name cannot begin with '/': %s\"),\n+\t\t\tremote_name);\n+\t\treturn NULL;\n+\t}\n+\n+\tFLEX_ALLOC_STR(r, name, remote_name);\n+\n+\t*promisors_tail = r;\n+\tpromisors_tail = &r->next;\n+\n+\treturn r;\n+}\n+\n+static struct promisor_remote *promisor_remote_lookup(const char *remote_name,\n+\t\t\t\t\t\t struct promisor_remote **previous)\n+{\n+\tstruct promisor_remote *r, *p;\n+\n+\tfor (p = NULL, r = promisors; r; p = r, r = r->next)\n+\t\tif (!strcmp(r->name, remote_name)) {\n+\t\t\tif (previous)\n+\t\t\t\t*previous = p;\n+\t\t\treturn r;\n+\t\t}\n+\n+\treturn NULL;\n+}\n+\n+static int promisor_remote_config(const char *var, const char *value, void *data)\n+{\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_lookup(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_lookup(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..01dcdf4dc7\n--- /dev/null\n+++ b/promisor-remote.h\n@@ -0,0 +1,16 @@\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+\tstruct promisor_remote *next;\n+\tconst char name[FLEX_ARRAY];\n+};\n+\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.750.g68c8ebb2ac\n\n"},{"id":"373506","messageId":"20190409161116.30256-6-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 05/16] promisor-remote: add promisor_remote_reinit()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:05Z","receivedAt":"2019-04-09T16:11:54Z","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 | 22 ++++++++++++++++++++--\n promisor-remote.h | 1 +\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 289f1dd074..46271eb3e3 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -67,10 +67,10 @@ static int promisor_remote_config(const char *var, const char *value, void *data\n \treturn 0;\n }\n \n+static int initialized;\n+\n static void promisor_remote_init(void)\n {\n-\tstatic int initialized;\n-\n \tif (initialized)\n \t\treturn;\n \tinitialized = 1;\n@@ -78,6 +78,24 @@ static void promisor_remote_init(void)\n \tgit_config(promisor_remote_config, NULL);\n }\n \n+static void promisor_remote_clear(void)\n+{\n+\twhile (promisors) {\n+\t\tstruct promisor_remote *r = promisors;\n+\t\tpromisors = promisors->next;\n+\t\tfree(r);\n+\t}\n+\n+\tpromisors_tail = &promisors;\n+}\n+\n+void promisor_remote_reinit(void)\n+{\n+\tinitialized = 0;\n+\tpromisor_remote_clear();\n+\tpromisor_remote_init();\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 92650cfd4c..ff69963907 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -12,6 +12,7 @@ struct promisor_remote {\n \tconst char name[FLEX_ARRAY];\n };\n \n+extern void promisor_remote_reinit(void);\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-- \n2.21.0.750.g68c8ebb2ac\n\n"},{"id":"373507","messageId":"20190409161116.30256-7-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 06/16] promisor-remote: use repository_format_partial_clone","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:06Z","receivedAt":"2019-04-09T16:11:55Z","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\nFor simplicity and to make things predictable, this promisor\nremote should be either always the last one we try to get\nobjects from, or the first one. So it should always be either\nat the end of the promisor remote list, or at its start.\n\nWe decided to make it the last one we try, because it is\nlikely that someone using many promisor remotes is doing so\nbecause the other promisor remotes are better for some reason\n(maybe they are closer or faster for some kind of objects)\nthan the origin, and the origin is likely to be the remote\nspecified by extensions.partialClone.\n\nThis justification is not very strong, but one choice had to\nbe made, and anyway the long term plan should be to make the\norder somehow fully configurable.\n\nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 46271eb3e3..737689d044 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -40,6 +40,18 @@ static struct promisor_remote *promisor_remote_lookup(const char *remote_name,\n \treturn NULL;\n }\n \n+static void promisor_remote_move_to_tail(struct promisor_remote *r,\n+\t\t\t\t\t struct promisor_remote *previous)\n+{\n+\tif (previous)\n+\t\tprevious->next = r->next;\n+\telse\n+\t\tpromisors = r->next ? r->next : r;\n+\tr->next = NULL;\n+\t*promisors_tail = r;\n+\tpromisors_tail = &r->next;\n+}\n+\n static int promisor_remote_config(const char *var, const char *value, void *data)\n {\n \tconst char *name;\n@@ -76,6 +88,17 @@ static void promisor_remote_init(void)\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_lookup(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 void promisor_remote_clear(void)\n-- \n2.21.0.750.g68c8ebb2ac\n\n"},{"id":"373508","messageId":"20190409161116.30256-5-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:04Z","receivedAt":"2019-04-09T16:11:56Z","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\nHelped-by: Ramsay Jones \nSigned-off-by: Christian Couder \n---\n promisor-remote.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++\n promisor-remote.h | 3 +++\n 2 files changed, 69 insertions(+)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex c249b80e02..289f1dd074 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -1,6 +1,8 @@\n #include \"cache.h\"\n+#include \"object-store.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@@ -90,3 +92,67 @@ int has_promisor_remote(void)\n {\n \treturn !!promisor_remote_find(NULL);\n }\n+\n+static int remove_fetched_oids(struct object_id **oids, int oid_nr, int to_free)\n+{\n+\tint i, missing_nr = 0;\n+\tint *missing = xcalloc(oid_nr, sizeof(*missing));\n+\tstruct object_id *old_oids = *oids;\n+\tstruct object_id *new_oids;\n+\tint old_fetch_if_missing = fetch_if_missing;\n+\n+\tfetch_if_missing = 0;\n+\n+\tfor (i = 0; i < oid_nr; i++)\n+\t\tif (oid_object_info_extended(the_repository, &old_oids[i], NULL, 0)) {\n+\t\t\tmissing[i] = 1;\n+\t\t\tmissing_nr++;\n+\t\t}\n+\n+\tfetch_if_missing = old_fetch_if_missing;\n+\n+\tif (missing_nr) {\n+\t\tint j = 0;\n+\t\tnew_oids = xcalloc(missing_nr, sizeof(*new_oids));\n+\t\tfor (i = 0; i < oid_nr; i++)\n+\t\t\tif (missing[i])\n+\t\t\t\toidcpy(&new_oids[j++], &old_oids[i]);\n+\t\t*oids = new_oids;\n+\t\tif (to_free)\n+\t\t\tfree(old_oids);\n+\t}\n+\n+\tfree(missing);\n+\n+\treturn missing_nr;\n+}\n+\n+int promisor_remote_get_direct(const struct object_id *oids, int oid_nr)\n+{\n+\tstruct promisor_remote *r;\n+\tstruct object_id *missing_oids = (struct object_id *)oids;\n+\tint missing_nr = oid_nr;\n+\tint to_free = 0;\n+\tint res = -1;\n+\n+\tpromisor_remote_init();\n+\n+\tfor (r = promisors; r; r = r->next) {\n+\t\tif (fetch_objects(r->name, missing_oids, missing_nr) < 0) {\n+\t\t\tif (missing_nr == 1)\n+\t\t\t\tcontinue;\n+\t\t\tmissing_nr = remove_fetched_oids(&missing_oids, missing_nr, to_free);\n+\t\t\tif (missing_nr) {\n+\t\t\t\tto_free = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\t\tres = 0;\n+\t\tbreak;\n+\t}\n+\n+\tif (to_free)\n+\t\tfree(missing_oids);\n+\n+\treturn res;\n+}\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex 01dcdf4dc7..92650cfd4c 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -1,6 +1,8 @@\n #ifndef PROMISOR_REMOTE_H\n #define PROMISOR_REMOTE_H\n \n+struct object_id;\n+\n /*\n * Promisor remote linked list\n * Its information come from remote.XXX config entries.\n@@ -12,5 +14,6 @@ struct promisor_remote {\n \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.750.g68c8ebb2ac\n\n"},{"id":"373509","messageId":"20190409161116.30256-8-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 07/16] Use promisor_remote_get_direct() and has_promisor_remote()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:07Z","receivedAt":"2019-04-09T16:11:58Z","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 | 15 ++++++++-------\n t/t5601-clone.sh | 2 +-\n t/t5616-partial-clone.sh | 2 +-\n unpack-trees.c | 7 +++----\n 12 files changed, 47 insertions(+), 38 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 4ba63d5ac6..f74bd78144 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@@ -1611,7 +1612,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@@ -1645,7 +1646,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 8943bcc300..824a8832b5 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@@ -661,7 +662,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 3ea0583d02..57568ce9a8 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@@ -359,7 +360,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 498498b1e3..fa5a9bb136 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@@ -2147,7 +2148,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 83358737f3..e7e037b6ce 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -30,8 +30,8 @@\n #include \"mergesort.h\"\n #include \"quote.h\"\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,16 +1369,17 @@ 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 !(flags & OBJECT_INFO_FOR_PREFETCH)) {\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 23854cab26..15720847b0 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 9a8f9886b3..c9e5f14165 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 22985aa4a3..d86cd922e0 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -16,7 +16,7 @@\n #include \"submodule-config.h\"\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 +398,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@@ -417,8 +417,7 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\toid_array_append(&to_fetch, &ce->oid);\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\toid_array_clear(&to_fetch);\n \t}\n \tfor (i = 0; i < index->cache_nr; i++) {\n-- \n2.21.0.750.g68c8ebb2ac\n\n"},{"id":"373510","messageId":"20190409161116.30256-9-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 08/16] diff: use promisor-remote.h instead of fetch-object.h","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:08Z","receivedAt":"2019-04-09T16:12:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"The repository_format_partial_clone global and fetch_objects()\nshould not be used anymore when there can be more than one\npromisor remote. Instead let's use has_promisor_remote()\nand promisor_remote_get_direct() from \"promisor-remote.h\".\n\nThis way all the configured promisor remotes will be taken\ninto account, not only the one specified by\nextensions.partialClone.\n\nSigned-off-by: Christian Couder \n---\n diff.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 4d3cf83a27..02864171fd 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -25,7 +25,7 @@\n #include \"packfile.h\"\n #include \"parse-options.h\"\n #include \"help.h\"\n-#include \"fetch-object.h\"\n+#include \"promisor-remote.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -6490,8 +6490,7 @@ static void add_if_missing(struct repository *r,\n \n void diffcore_std(struct diff_options *options)\n {\n-\tif (options->repo == the_repository &&\n-\t repository_format_partial_clone) {\n+\tif (options->repo == the_repository && has_promisor_remote()) {\n \t\t/*\n \t\t * Prefetch the diff pairs that are about to be flushed.\n \t\t */\n@@ -6508,8 +6507,7 @@ void diffcore_std(struct diff_options *options)\n \t\t\t/*\n \t\t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n \t\t\t */\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\toid_array_clear(&to_fetch);\n \t}\n \n-- \n2.21.0.750.g68c8ebb2ac\n\n"},{"id":"373512","messageId":"20190409161116.30256-10-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 09/16] promisor-remote: parse remote.*.partialclonefilter","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:09Z","receivedAt":"2019-04-09T16:12:01Z","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 | 15 +++++++++++++++\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, 40 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f74bd78144..13d8133130 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 737689d044..707a8005c5 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -75,6 +75,21 @@ 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\tstruct promisor_remote *r;\n+\t\tchar *remote_name = xmemdupz(name, namelen);\n+\n+\t\tr = promisor_remote_lookup(remote_name, NULL);\n+\t\tif (!r)\n+\t\t\tr = promisor_remote_new(remote_name);\n+\n+\t\tfree(remote_name);\n+\n+\t\tif (!r)\n+\t\t\treturn 0;\n+\n+\t\treturn git_config_string(&r->partial_clone_filter, var, value);\n+\t}\n \n \treturn 0;\n }\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex ff69963907..562c7ad8a4 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -5,10 +5,13 @@ struct object_id;\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 \tstruct promisor_remote *next;\n+\tconst char *partial_clone_filter;\n \tconst char name[FLEX_ARRAY];\n };\n \ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 3559313bd0..3082eff2bf 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 15720847b0..8842b029bd 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 c9e5f14165..107b247336 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.750.g68c8ebb2ac\n\n"},{"id":"373513","messageId":"20190409161116.30256-11-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 10/16] builtin/fetch: remove unique promisor remote limitation","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:10Z","receivedAt":"2019-04-09T16:12:02Z","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 13d8133130..5657d054ec 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.750.g68c8ebb2ac\n\n"},{"id":"373511","messageId":"20190409161116.30256-13-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 12/16] partial-clone: add multiple remotes in the doc","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:12Z","receivedAt":"2019-04-09T16:12:04Z","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 | 117 ++++++++++++++++------\n 1 file changed, 84 insertions(+), 33 deletions(-)\n\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex 896c7b3878..210373e258 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -30,12 +30,20 @@ advance* during clone and fetch operations and thereby reduce download\n times and disk usage. Missing objects can later be \"demand fetched\"\n if/when needed.\n \n+A remote that can later provide the missing objects is called a\n+promisor remote, as it promises to send the objects when\n+requested. Initialy Git supported only one promisor remote, the origin\n+remote from which the user cloned and that was configured in the\n+\"extensions.partialClone\" config option. Later support for more than\n+one promisor remote has been implemented.\n+\n Use of partial clone requires that the user be online and the origin\n-remote be available for on-demand fetching of missing objects. This may\n-or may not be problematic for the user. For example, if the user can\n-stay within the pre-selected subset of the source tree, they may not\n-encounter any missing objects. Alternatively, the user could try to\n-pre-fetch various objects if they know that they are going offline.\n+remote or other promisor remotes be available for on-demand fetching\n+of missing objects. This may or may not be problematic for the user.\n+For example, if the user can stay within the pre-selected subset of\n+the source tree, they may not encounter any missing objects.\n+Alternatively, the user could try to pre-fetch various objects if they\n+know that they are going offline.\n \n \n Non-Goals\n@@ -100,18 +108,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,12 +131,12 @@ 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-the object lookup. This allows objects to be \"faulted in\" without\n-complicated prediction algorithms.\n+promisor_remote_get_direct() to try to get the object from a promisor\n+remote and then retry the object lookup. This allows objects to be\n+\"faulted in\" without complicated prediction algorithms.\n +\n For efficiency reasons, no check as to whether the missing object is\n actually a promisor object is performed.\n@@ -157,8 +165,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 +189,63 @@ 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+We decided to make it the last one we try, because it is likely that\n+someone using many promisor remotes is doing so because the other\n+promisor remotes are better for some reason (maybe they are closer or\n+faster for some kind of objects) than the origin, and the origin is\n+likely to be the remote specified by extensions.partialClone.\n+\n+This justification is not very strong, but one choice had to be made,\n+and anyway the long term plan should be to make the order somehow\n+fully configurable.\n+\n+For now though the other promisor remotes will be tried in the order\n+they appear in 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 +267,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.750.g68c8ebb2ac\n\n"},{"id":"373514","messageId":"20190409161116.30256-12-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 11/16] t0410: test fetching from many promisor remotes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:11Z","receivedAt":"2019-04-09T16:12:06Z","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\nHelped-by: SZEDER Gábor \nSigned-off-by: Christian Couder \n---\n t/t0410-partial-clone.sh | 49 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 3082eff2bf..2498e72a34 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -183,8 +183,55 @@ 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=$(sed \"s/promisor$/idx/\" promisorlist) &&\n+\tgit verify-pack --verbose \"$IDX\" >out &&\n+\tgrep \"$HASH2\" out\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_cmp_config -C repo true remote.server3.promisor &&\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=$(sed \"s/promisor$/idx/\" promisorlist) &&\n+\tgit verify-pack --verbose \"$IDX\" >out &&\n+\tgrep \"$HASH3\" out\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.750.g68c8ebb2ac\n\n"},{"id":"373515","messageId":"20190409161116.30256-14-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 13/16] remote: add promisor and partial clone config to the doc","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:13Z","receivedAt":"2019-04-09T16:12:07Z","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.750.g68c8ebb2ac\n\n"},{"id":"373516","messageId":"20190409161116.30256-15-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 14/16] Remove fetch-object.{c,h} in favor of promisor-remote.{c,h}","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:14Z","receivedAt":"2019-04-09T16:12:08Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"As fetch_objects() is now used only in promisor-remote.c\nand should't be used outside it, let's move it into\npromisor-remote.c, make it static there, and remove\nfetch-object.{c,h}.\n\nSigned-off-by: Christian Couder \n---\n Makefile | 1 -\n fetch-object.c | 43 -------------------------------------------\n fetch-object.h | 9 ---------\n promisor-remote.c | 40 +++++++++++++++++++++++++++++++++++++++-\n 4 files changed, 39 insertions(+), 54 deletions(-)\n delete mode 100644 fetch-object.c\n delete mode 100644 fetch-object.h\n\ndiff --git a/Makefile b/Makefile\nindex 3523ae0517..0996fb6646 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -890,7 +890,6 @@ LIB_OBJS += ewah/ewah_io.o\n LIB_OBJS += ewah/ewah_rlw.o\n LIB_OBJS += exec-cmd.o\n LIB_OBJS += fetch-negotiator.o\n-LIB_OBJS += fetch-object.o\n LIB_OBJS += fetch-pack.o\n LIB_OBJS += fsck.o\n LIB_OBJS += fsmonitor.o\ndiff --git a/fetch-object.c b/fetch-object.c\ndeleted file mode 100644\nindex eac4d448ef..0000000000\n--- a/fetch-object.c\n+++ /dev/null\n@@ -1,43 +0,0 @@\n-#include \"cache.h\"\n-#include \"packfile.h\"\n-#include \"pkt-line.h\"\n-#include \"strbuf.h\"\n-#include \"transport.h\"\n-#include \"fetch-object.h\"\n-\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-\tif (!remote->url[0])\n-\t\tdie(_(\"Remote with no URL\"));\n-\ttransport = transport_get(remote, remote->url[0]);\n-\n-\ttransport_set_option(transport, TRANS_OPT_FROM_PROMISOR, \"1\");\n-\ttransport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, \"1\");\n-\tres = transport_fetch_refs(transport, ref);\n-\tfetch_if_missing = original_fetch_if_missing;\n-\n-\treturn res;\n-}\n-\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-\n-\tfor (i = 0; i < oid_nr; i++) {\n-\t\tstruct ref *new_ref = alloc_ref(oid_to_hex(&oids[i]));\n-\t\toidcpy(&new_ref->old_oid, &oids[i]);\n-\t\tnew_ref->exact_oid = 1;\n-\t\tnew_ref->next = ref;\n-\t\tref = new_ref;\n-\t}\n-\treturn fetch_refs(remote_name, ref);\n-}\ndiff --git a/fetch-object.h b/fetch-object.h\ndeleted file mode 100644\nindex 7bcc7cadb0..0000000000\n--- a/fetch-object.h\n+++ /dev/null\n@@ -1,9 +0,0 @@\n-#ifndef FETCH_OBJECT_H\n-#define FETCH_OBJECT_H\n-\n-struct object_id;\n-\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/promisor-remote.c b/promisor-remote.c\nindex 707a8005c5..066489b637 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -2,7 +2,45 @@\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n #include \"config.h\"\n-#include \"fetch-object.h\"\n+#include \"transport.h\"\n+\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+\tif (!remote->url[0])\n+\t\tdie(_(\"Remote with no URL\"));\n+\ttransport = transport_get(remote, remote->url[0]);\n+\n+\ttransport_set_option(transport, TRANS_OPT_FROM_PROMISOR, \"1\");\n+\ttransport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, \"1\");\n+\tres = transport_fetch_refs(transport, ref);\n+\tfetch_if_missing = original_fetch_if_missing;\n+\n+\treturn res;\n+}\n+\n+static int fetch_objects(const char *remote_name,\n+\t\t\t const struct object_id *oids,\n+\t\t\t int oid_nr)\n+{\n+\tstruct ref *ref = NULL;\n+\tint i;\n+\n+\tfor (i = 0; i < oid_nr; i++) {\n+\t\tstruct ref *new_ref = alloc_ref(oid_to_hex(&oids[i]));\n+\t\toidcpy(&new_ref->old_oid, &oids[i]);\n+\t\tnew_ref->exact_oid = 1;\n+\t\tnew_ref->next = ref;\n+\t\tref = new_ref;\n+\t}\n+\treturn fetch_refs(remote_name, ref);\n+}\n \n static struct promisor_remote *promisors;\n static struct promisor_remote **promisors_tail = &promisors;\n-- \n2.21.0.750.g68c8ebb2ac\n\n"},{"id":"373517","messageId":"20190409161116.30256-16-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 15/16] Move repository_format_partial_clone to promisor-remote.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:15Z","receivedAt":"2019-04-09T16:12:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Now that we have has_promisor_remote() and can use many\npromisor remotes, let's hide repository_format_partial_clone\nas a static in promisor-remote.c to avoid it being use\nfor anything other than managing backward compatibility.\n\nSigned-off-by: Christian Couder \n---\n cache.h | 1 -\n environment.c | 1 -\n promisor-remote.c | 7 +++++++\n promisor-remote.h | 6 ++++++\n setup.c | 3 ++-\n 5 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e928fe9d3b..217e434d5d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -960,7 +960,6 @@ extern int grafts_replace_parents;\n #define GIT_REPO_VERSION 0\n #define GIT_REPO_VERSION_READ 1\n extern int repository_format_precious_objects;\n-extern char *repository_format_partial_clone;\n extern const char *core_partial_clone_filter_default;\n extern int repository_format_worktree_config;\n \ndiff --git a/environment.c b/environment.c\nindex 89af47cb85..8855d2fc11 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -31,7 +31,6 @@ int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int ref_paranoia = -1;\n int repository_format_precious_objects;\n-char *repository_format_partial_clone;\n const char *core_partial_clone_filter_default;\n int repository_format_worktree_config;\n const char *git_commit_encoding;\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 066489b637..371c78385f 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -4,6 +4,13 @@\n #include \"config.h\"\n #include \"transport.h\"\n \n+static char *repository_format_partial_clone;\n+\n+void set_repository_format_partial_clone(char *partial_clone)\n+{\n+\trepository_format_partial_clone = xstrdup_or_null(partial_clone);\n+}\n+\n static int fetch_refs(const char *remote_name, struct ref *ref)\n {\n \tstruct remote *remote;\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex 562c7ad8a4..c3c07c2a23 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -20,4 +20,10 @@ 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+/*\n+ * This should be used only once from setup.c to set the value we got\n+ * from the extensions.partialclone config option.\n+ */\n+extern void set_repository_format_partial_clone(char *partial_clone);\n+\n #endif /* PROMISOR_REMOTE_H */\ndiff --git a/setup.c b/setup.c\nindex d0c958c3b2..434628706b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -4,6 +4,7 @@\n #include \"dir.h\"\n #include \"string-list.h\"\n #include \"chdir-notify.h\"\n+#include \"promisor-remote.h\"\n \n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n@@ -477,7 +478,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t}\n \n \trepository_format_precious_objects = candidate->precious_objects;\n-\trepository_format_partial_clone = xstrdup_or_null(candidate->partial_clone);\n+\tset_repository_format_partial_clone(candidate->partial_clone);\n \trepository_format_worktree_config = candidate->worktree_config;\n \tstring_list_clear(&candidate->unknown_extensions, 0);\n \n-- \n2.21.0.750.g68c8ebb2ac\n\n"},{"id":"373518","messageId":"20190409161116.30256-17-chriscool@tuxfamily.org","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"[PATCH v5 16/16] Move core_partial_clone_filter_default to promisor-remote.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-09T16:11:16Z","receivedAt":"2019-04-09T16:12:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Now that we can have a different default partial clone filter for\neach promisor remote, let's hide core_partial_clone_filter_default\nas a static in promisor-remote.c to avoid it being use for\nanything other than managing backward compatibility.\n\nSigned-off-by: Christian Couder \n---\n cache.h | 1 -\n config.c | 5 -----\n environment.c | 1 -\n promisor-remote.c | 5 +++++\n 4 files changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 217e434d5d..7ed867b119 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -960,7 +960,6 @@ extern int grafts_replace_parents;\n #define GIT_REPO_VERSION 0\n #define GIT_REPO_VERSION_READ 1\n extern int repository_format_precious_objects;\n-extern const char *core_partial_clone_filter_default;\n extern int repository_format_worktree_config;\n \n /*\ndiff --git a/config.c b/config.c\nindex c2846df3f1..dd95b659b4 100644\n--- a/config.c\n+++ b/config.c\n@@ -1344,11 +1344,6 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(var, \"core.partialclonefilter\")) {\n-\t\treturn git_config_string(&core_partial_clone_filter_default,\n-\t\t\t\t\t var, value);\n-\t}\n-\n \tif (!strcmp(var, \"core.usereplacerefs\")) {\n \t\tread_replace_refs = git_config_bool(var, value);\n \t\treturn 0;\ndiff --git a/environment.c b/environment.c\nindex 8855d2fc11..efa072680a 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -31,7 +31,6 @@ int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int ref_paranoia = -1;\n int repository_format_precious_objects;\n-const char *core_partial_clone_filter_default;\n int repository_format_worktree_config;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 371c78385f..f77d0e3b50 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -5,6 +5,7 @@\n #include \"transport.h\"\n \n static char *repository_format_partial_clone;\n+static const char *core_partial_clone_filter_default;\n \n void set_repository_format_partial_clone(char *partial_clone)\n {\n@@ -103,6 +104,10 @@ static int promisor_remote_config(const char *var, const char *value, void *data\n \tint namelen;\n \tconst char *subkey;\n \n+\tif (!strcmp(var, \"core.partialclonefilter\"))\n+\t\treturn git_config_string(&core_partial_clone_filter_default,\n+\t\t\t\t\t var, value);\n+\n \tif (parse_config_key(var, \"remote\", &name, &namelen, &subkey) < 0)\n \t\treturn 0;\n \n-- \n2.21.0.750.g68c8ebb2ac\n\n"},{"id":"373851","messageId":"xmqqmukrr52m.fsf@gitster-ct.c.googlers.com","threadId":"50903","inReplyTo":"20190409161116.30256-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v5 00/16] Many promisor remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-15T09:27:45Z","receivedAt":"2019-04-15T09:27:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> This patch series is based on:\n>\n> 763fb763b8 (Merge branch 'jt/batch-fetch-blobs-in-diff' into jch, 2019-04-08)\n>\n> to avoid issues with jt/batch-fetch-blobs-in-diff.\n\nYuck. As an experienced contributor, you should know better than\nthat by now to do that. A merge into jch/pu are rebuilt at least\nonce and often three times a day, and in no way a good solid base\nto build on top.\n\nIf you really need to depend on another topic or two, please base\nyour work on a merge between 'master' (or some well known ancestor\nof it) and the tips of the topics instead.\n\nHaving said that, I thought that the semantic conflict has been\ncorrected and the machinery to rebuild 'pu' has been replaying the\ncorrect resolution ever since, so there was no need for such a\nrebase? Isn't it the case and do we still have the breakage due to\nsemantic conflict with JTan's topic in 'pu'?\n"},{"id":"373853","messageId":"xmqqimvfr25d.fsf@gitster-ct.c.googlers.com","threadId":"50903","inReplyTo":"xmqqmukrr52m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 00/16] Many promisor remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-15T10:30:54Z","receivedAt":"2019-04-15T10:30:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Christian Couder writes:\n>\n>> This patch series is based on:\n>>\n>> 763fb763b8 (Merge branch 'jt/batch-fetch-blobs-in-diff' into jch, 2019-04-08)\n>>\n>> to avoid issues with jt/batch-fetch-blobs-in-diff.\n> ...\n> If you really need to depend on another topic or two, please base\n> your work on a merge between 'master' (or some well known ancestor\n> of it) and the tips of the topics instead.\n\nWell, I've done this myself by first queuing these on 763fb763b8 and\nthen made a merge between jt/batch-fetch-blobs-in-diff and master\nand applied these pathes on top of the result. You should be able\nto see the resulting topic replacing the old one in 'pu' in todays\npushout.\n\nNow I can lose the semantic conflict resolution the rebuilding\nmachinery was keeping, which makes things a bit simpler ;-)\n\n"},{"id":"373854","messageId":"CAP8UFD3p+usPw=xwW7TWBZSZNGJKBgGRDNAFUocSkhO5--dvAA@mail.gmail.com","threadId":"50903","inReplyTo":"xmqqmukrr52m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 00/16] Many promisor remotes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-15T10:37:40Z","receivedAt":"2019-04-15T10:37:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Apr 15, 2019 at 11:27 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > This patch series is based on:\n> >\n> > 763fb763b8 (Merge branch 'jt/batch-fetch-blobs-in-diff' into jch, 2019-04-08)\n> >\n> > to avoid issues with jt/batch-fetch-blobs-in-diff.\n>\n> Yuck. As an experienced contributor, you should know better than\n> that by now to do that. A merge into jch/pu are rebuilt at least\n> once and often three times a day, and in no way a good solid base\n> to build on top.\n\nSorry if it creates problems.\n\n> If you really need to depend on another topic or two, please base\n> your work on a merge between 'master' (or some well known ancestor\n> of it) and the tips of the topics instead.\n\nOk I will do that then.\n\n> Having said that, I thought that the semantic conflict has been\n> corrected and the machinery to rebuild 'pu' has been replaying the\n> correct resolution ever since, so there was no need for such a\n> rebase? Isn't it the case and do we still have the breakage due to\n> semantic conflict with JTan's topic in 'pu'?\n\nThere is one patch in the series, Patch 8/16 (diff: use\npromisor-remote.h instead of fetch-object.h), that fix the breakage,\nso if the series is applied on top of jt/batch-fetch-blobs-in-diff, it\nwill apply correctly and if it is not applied on top of\njt/batch-fetch-blobs-in-diff then the patch can just be dropped and\neverything else will apply correctly. I thought that it might be\nbetter to make the fix explicit than to rely on the rebuild machinery.\n"},{"id":"373855","messageId":"CAP8UFD3nusWgF=MoaqF6-VdVZa0EJJg4E=Nxoc4Ra_SZ1nvDkw@mail.gmail.com","threadId":"50903","inReplyTo":"xmqqimvfr25d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 00/16] Many promisor remotes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-15T10:39:29Z","receivedAt":"2019-04-15T10:39:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Apr 15, 2019 at 12:30 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > Christian Couder writes:\n> >\n> >> This patch series is based on:\n> >>\n> >> 763fb763b8 (Merge branch 'jt/batch-fetch-blobs-in-diff' into jch, 2019-04-08)\n> >>\n> >> to avoid issues with jt/batch-fetch-blobs-in-diff.\n> > ...\n> > If you really need to depend on another topic or two, please base\n> > your work on a merge between 'master' (or some well known ancestor\n> > of it) and the tips of the topics instead.\n>\n> Well, I've done this myself by first queuing these on 763fb763b8 and\n> then made a merge between jt/batch-fetch-blobs-in-diff and master\n> and applied these pathes on top of the result. You should be able\n> to see the resulting topic replacing the old one in 'pu' in todays\n> pushout.\n\nThanks, I will take a look.\n\n> Now I can lose the semantic conflict resolution the rebuilding\n> machinery was keeping, which makes things a bit simpler ;-)\n\nYeah, nice!\n"},{"id":"376446","messageId":"b4d69d2b-dc0d-fffb-2909-c54060fe9cd1@gmail.com","threadId":"50903","inReplyTo":"20190409161116.30256-5-chriscool@tuxfamily.org","subject":"Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-30T17:21:51Z","receivedAt":"2019-05-30T17:21:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/9/2019 12:11 PM, Christian Couder wrote:\n> From: Christian Couder \n> \n> This is implemented for now by calling fetch_objects(). It fetches\n> from all the promisor remotes.\n\nHi Christian,\n\nSorry for jumping on the thread late, but I noticed some peculiarities\nwhen looking at the test coverage report.\n\n> +static int remove_fetched_oids(struct object_id **oids, int oid_nr, int to_free)\n\nThis method does not seem to be covered by the test suite at all.\nIs this scenario difficult to set up for a test?\n\n> +{\n> +\tint i, missing_nr = 0;\n> +\tint *missing = xcalloc(oid_nr, sizeof(*missing));\n> +\tstruct object_id *old_oids = *oids;\n> +\tstruct object_id *new_oids;\n> +\tint old_fetch_if_missing = fetch_if_missing;\n> +\n> +\tfetch_if_missing = 0;\n\nThis global 'fetch_if_missing' swap seems very fragile. I'm guessing you are using\nit to prevent a loop when calling oid_object_info_extended() below. Can you instead\npass a flag to the method that disables the fetch_if_missing behavior?\n\n> +\n> +\tfor (i = 0; i < oid_nr; i++)\n> +\t\tif (oid_object_info_extended(the_repository, &old_oids[i], NULL, 0)) {\n\nA use of \"the_repository\" this deep in new code is asking for a refactor later to remove it.\nPlease try to pass a \"struct repository *r\" through your methods so we minimize references\nto the_repository (and the amount of work required to remove them later).\n\n> +\t\t\tmissing[i] = 1;\n> +\t\t\tmissing_nr++;\n> +\t\t}\n> +\n> +\tfetch_if_missing = old_fetch_if_missing;\n> +\n> +\tif (missing_nr) {\n> +\t\tint j = 0;\n> +\t\tnew_oids = xcalloc(missing_nr, sizeof(*new_oids));\n> +\t\tfor (i = 0; i < oid_nr; i++)\n> +\t\t\tif (missing[i])\n> +\t\t\t\toidcpy(&new_oids[j++], &old_oids[i]);\n> +\t\t*oids = new_oids;\n> +\t\tif (to_free)\n> +\t\t\tfree(old_oids);\n> +\t}\n> +\n> +\tfree(missing);\n> +\n> +\treturn missing_nr;\n> +}\n> +\n> +int promisor_remote_get_direct(const struct object_id *oids, int oid_nr)\n> +{\n> +\tstruct promisor_remote *r;\n> +\tstruct object_id *missing_oids = (struct object_id *)oids;\n> +\tint missing_nr = oid_nr;\n\nNote that for this method, \"missing_nr\" actually means \"number of oids still in the list\".\n\n> +\tint to_free = 0;\n> +\tint res = -1;\n> +\n> +\tpromisor_remote_init();\n> +\n> +\tfor (r = promisors; r; r = r->next) {\n> +\t\tif (fetch_objects(r->name, missing_oids, missing_nr) < 0) {\n\nThis block hits if we have any missing objects. This is not currently hit by the test\nsuite.\n\n> +\t\t\tif (missing_nr == 1)\n> +\t\t\t\tcontinue;\n\nBut we skip the call below if there is exactly one object in the list, as it must be the one\nmissing object. So, to be interesting we need to try fetching multiple objects.\n\n> +\t\t\tmissing_nr = remove_fetched_oids(&missing_oids, missing_nr, to_free);\n\nHere is the one call, and after this assignment \"missing_nr\" does mean the number of missing objects.\nHowever, I do think this could be clarified by using remaining_nr and remaining_oids.\n\n> +\t\t\tif (missing_nr) {\n> +\t\t\t\tto_free = 1;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n\nNow this block took a bit to grok. You use to_free in the if(to_free) free(missing_oids); below.\nBut it also changes the behavior of remove_fetched_oids(). This means that the first time\nremove_fetched_oids() will preserve the list (because it is the input list) but all later\ncalls will free the newly-created intermediate list. This checks out.\n\nWhat is confusing to me: is there any reason that missing_nr would be zero in this situation?\nI guess if the fetch_objects() failed to find some objects, but we ended up having them locally\nin a new call to oid_object_info_extended(). That's a fringe case that is worth guarding against\nbut I wouldn't worry about testing.\n\n> +\t\t}\n> +\t\tres = 0;\n> +\t\tbreak;\n> +\t}\n> +\n> +\tif (to_free)\n> +\t\tfree(missing_oids);\n> +\n> +\treturn res;\n> +}\n\nWhile the test coverage report brought this patch to my attention, it does seem correct.\nI still think a test exposing this method would be good, especially one that requires\na fetch_objects() call to multiple remotes to really exercise the details of remove_fetched_oids().\n\nThanks,\n-Stolee\n\n"},{"id":"376459","messageId":"nycvar.QRO.7.76.6.1905302235000.44@tvgsbejvaqbjf.bet","threadId":"50903","inReplyTo":"b4d69d2b-dc0d-fffb-2909-c54060fe9cd1@gmail.com","subject":"Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-30T20:46:00Z","receivedAt":"2019-05-30T20:46:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 30 May 2019, Derrick Stolee wrote:\n\n> On 4/9/2019 12:11 PM, Christian Couder wrote:\n> > From: Christian Couder \n> >\n> > +{\n> > +\tint i, missing_nr = 0;\n> > +\tint *missing = xcalloc(oid_nr, sizeof(*missing));\n> > +\tstruct object_id *old_oids = *oids;\n> > +\tstruct object_id *new_oids;\n> > +\tint old_fetch_if_missing = fetch_if_missing;\n> > +\n> > +\tfetch_if_missing = 0;\n>\n> This global 'fetch_if_missing' swap seems very fragile. I'm guessing you\n> are using it to prevent a loop when calling oid_object_info_extended()\n> below. Can you instead pass a flag to the method that disables the\n> fetch_if_missing behavior?\n\nFWIW I mentioned the very same concern here:\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1903272300020.41@tvgsbejvaqbjf.bet/\n\nThe situation is *pretty* bad by now. I see `fetch_if_missing` mentioned\n25 times in `master`, and all but one are in .c files or in cache.h.\n\nThe flag is actually used only in `oid_object_info_extended()`, and that\nfunction accepts an `unsigned flags`, so one might think that it could be\nextended to accept also a `OBJECT_INFO_LOOKUP_FETCH_IF_MISSING`. But then,\nthere are many callers of that function, some of them also pretty low in\nthe food chain. For example, `oid_object_info()` (does not accept `flags`)\nor `read_object()` (does not accept flags either).\n\nSo it looks as if the idea to pass this flag down the call chain entailed\na pretty serious avalanche effect.\n\nAn alternative that strikes me as inelegant, still, but nevertheless\nbetter would be to move `fetch_if_missing` into `struct repository`.\n\nCiao,\nDscho\n"},{"id":"376460","messageId":"2329a4c7-bfb9-10ce-9d1c-8c754d6dee05@gmail.com","threadId":"50903","inReplyTo":"nycvar.QRO.7.76.6.1905302235000.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-30T20:54:47Z","receivedAt":"2019-05-30T20:54:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/30/2019 4:46 PM, Johannes Schindelin wrote:\n> Hi,\n> \n> On Thu, 30 May 2019, Derrick Stolee wrote:\n> \n>> On 4/9/2019 12:11 PM, Christian Couder wrote:\n>>> From: Christian Couder \n>>>\n>>> +{\n>>> +\tint i, missing_nr = 0;\n>>> +\tint *missing = xcalloc(oid_nr, sizeof(*missing));\n>>> +\tstruct object_id *old_oids = *oids;\n>>> +\tstruct object_id *new_oids;\n>>> +\tint old_fetch_if_missing = fetch_if_missing;\n>>> +\n>>> +\tfetch_if_missing = 0;\n>>\n>> This global 'fetch_if_missing' swap seems very fragile. I'm guessing you\n>> are using it to prevent a loop when calling oid_object_info_extended()\n>> below. Can you instead pass a flag to the method that disables the\n>> fetch_if_missing behavior?\n> \n> FWIW I mentioned the very same concern here:\n> https://public-inbox.org/git/nycvar.QRO.7.76.6.1903272300020.41@tvgsbejvaqbjf.bet/\n> \n> The situation is *pretty* bad by now. I see `fetch_if_missing` mentioned\n> 25 times in `master`, and all but one are in .c files or in cache.h.\n> \n> The flag is actually used only in `oid_object_info_extended()`, and that\n> function accepts an `unsigned flags`, so one might think that it could be\n> extended to accept also a `OBJECT_INFO_LOOKUP_FETCH_IF_MISSING`. But then,\n> there are many callers of that function, some of them also pretty low in\n> the food chain. For example, `oid_object_info()` (does not accept `flags`)\n> or `read_object()` (does not accept flags either).\n>\n> So it looks as if the idea to pass this flag down the call chain entailed\n> a pretty serious avalanche effect.\n\nIt could be approached in small bits.\n\nFirst, add an OBJECT_INFO_NEVER_FETCH_IF_MISSING flag that overrides fetch_if_missing,\nand then use the flag in small places like this one. Then, build up to the other\nmethods as appropriate.\n\n> An alternative that strikes me as inelegant, still, but nevertheless\n> better would be to move `fetch_if_missing` into `struct repository`.\n\nThis is literally the _least_ we should do to reduce our dependence on\nglobals. Maybe this happens first, then the flag idea could be done bits\nat a time.\n\nThanks,\n-Stolee\n\n"},{"id":"376466","messageId":"CAP8UFD0zvqC2s3_RsWfKwoWQL=2iV6PbaL8+hwUXrWBVe+6sgA@mail.gmail.com","threadId":"50903","inReplyTo":"b4d69d2b-dc0d-fffb-2909-c54060fe9cd1@gmail.com","subject":"Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-05-31T05:10:26Z","receivedAt":"2019-05-31T05:10:41Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Stolee,\n\nOn Thu, May 30, 2019 at 7:21 PM Derrick Stolee wrote:\n>\n> On 4/9/2019 12:11 PM, Christian Couder wrote:\n> > From: Christian Couder \n> >\n> > This is implemented for now by calling fetch_objects(). It fetches\n> > from all the promisor remotes.\n>\n> Sorry for jumping on the thread late, but I noticed some peculiarities\n> when looking at the test coverage report.\n\nYou are welcome. It needs review according to Junio, so it's\ndefinitely a good thing that you take a look at it.\n\n> > +static int remove_fetched_oids(struct object_id **oids, int oid_nr, int to_free)\n>\n> This method does not seem to be covered by the test suite at all.\n> Is this scenario difficult to set up for a test?\n\nI think so. If I remember correctly, I added this following a review\nby Junio because it could be possible that a promisor/partial clone\nremote only sends parts of the promisor objects it is asked. In this\ncase the objects that have been fetched should be removed from the\nlist of objects we try to fetch from the next promisor/partial clone\nremote.\n\nThe issue is that now if a promisor/partial clone remote can send only\nparts of the promisor objects it is asked, it should fail, as far as I\nunderstand, which means that we will not actually get the objects it\nshould send. That's why I think it's not easy, or perhaps even not\npossible, to test this.\n\n> > +{\n> > + int i, missing_nr = 0;\n> > + int *missing = xcalloc(oid_nr, sizeof(*missing));\n> > + struct object_id *old_oids = *oids;\n> > + struct object_id *new_oids;\n> > + int old_fetch_if_missing = fetch_if_missing;\n> > +\n> > + fetch_if_missing = 0;\n>\n> This global 'fetch_if_missing' swap seems very fragile. I'm guessing you are using\n> it to prevent a loop when calling oid_object_info_extended() below. Can you instead\n> pass a flag to the method that disables the fetch_if_missing behavior?\n\nIf such a flag existed when I wrote the function I would certainly\nhave used it, as I also dislike this kind of messing with a global\n(and globals in general).\n\nI will see if I can do something about it according to what you\nsuggest later in this thread.\n\n> > + for (i = 0; i < oid_nr; i++)\n> > + if (oid_object_info_extended(the_repository, &old_oids[i], NULL, 0)) {\n>\n> A use of \"the_repository\" this deep in new code is asking for a refactor later to remove it.\n> Please try to pass a \"struct repository *r\" through your methods so we minimize references\n> to the_repository (and the amount of work required to remove them later).\n\nOk, I will take a look at that.\n\n> > +int promisor_remote_get_direct(const struct object_id *oids, int oid_nr)\n> > +{\n> > + struct promisor_remote *r;\n> > + struct object_id *missing_oids = (struct object_id *)oids;\n> > + int missing_nr = oid_nr;\n>\n> Note that for this method, \"missing_nr\" actually means \"number of oids still in the list\".\n>\n> > + int to_free = 0;\n> > + int res = -1;\n> > +\n> > + promisor_remote_init();\n> > +\n> > + for (r = promisors; r; r = r->next) {\n> > + if (fetch_objects(r->name, missing_oids, missing_nr) < 0) {\n>\n> This block hits if we have any missing objects. This is not currently hit by the test\n> suite.\n>\n> > + if (missing_nr == 1)\n> > + continue;\n>\n> But we skip the call below if there is exactly one object in the list, as it must be the one\n> missing object. So, to be interesting we need to try fetching multiple objects.\n>\n> > + missing_nr = remove_fetched_oids(&missing_oids, missing_nr, to_free);\n>\n> Here is the one call, and after this assignment \"missing_nr\" does mean the number of missing objects.\n> However, I do think this could be clarified by using remaining_nr and remaining_oids.\n\nOk, I will take a look at using \"remaining_nr\" and \"remaining_oids\".\n\n> > + if (missing_nr) {\n> > + to_free = 1;\n> > + continue;\n> > + }\n>\n> Now this block took a bit to grok. You use to_free in the if(to_free) free(missing_oids); below.\n> But it also changes the behavior of remove_fetched_oids(). This means that the first time\n> remove_fetched_oids() will preserve the list (because it is the input list) but all later\n> calls will free the newly-created intermediate list. This checks out.\n>\n> What is confusing to me: is there any reason that missing_nr would be zero in this situation?\n\nI don't think so but I will check again, and maybe add a comment.\n\n> I guess if the fetch_objects() failed to find some objects, but we ended up having them locally\n> in a new call to oid_object_info_extended(). That's a fringe case that is worth guarding against\n> but I wouldn't worry about testing.\n>\n> > + }\n> > + res = 0;\n> > + break;\n> > + }\n> > +\n> > + if (to_free)\n> > + free(missing_oids);\n> > +\n> > + return res;\n> > +}\n>\n> While the test coverage report brought this patch to my attention, it does seem correct.\n> I still think a test exposing this method would be good, especially one that requires\n> a fetch_objects() call to multiple remotes to really exercise the details of remove_fetched_oids().\n\nYeah, I would like to actually test it. I will take another look at\nwhat can be done to test this. Perhaps I will look at what can be done\nto still get some objects when fetching from a promisor/partial clone\nremote even when it doesn't have all of the objects we request.\n\nThanks for the review,\nChristian.\n"},{"id":"376470","messageId":"nycvar.QRO.7.76.6.1905311332560.44@tvgsbejvaqbjf.bet","threadId":"50903","inReplyTo":"2329a4c7-bfb9-10ce-9d1c-8c754d6dee05@gmail.com","subject":"Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-31T11:35:13Z","receivedAt":"2019-05-31T11:36:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Thu, 30 May 2019, Derrick Stolee wrote:\n\n> On 5/30/2019 4:46 PM, Johannes Schindelin wrote:\n> >\n> > On Thu, 30 May 2019, Derrick Stolee wrote:\n> >\n> >> On 4/9/2019 12:11 PM, Christian Couder wrote:\n> >>> From: Christian Couder \n> >>>\n> >>> +{\n> >>> +\tint i, missing_nr = 0;\n> >>> +\tint *missing = xcalloc(oid_nr, sizeof(*missing));\n> >>> +\tstruct object_id *old_oids = *oids;\n> >>> +\tstruct object_id *new_oids;\n> >>> +\tint old_fetch_if_missing = fetch_if_missing;\n> >>> +\n> >>> +\tfetch_if_missing = 0;\n> >>\n> >> This global 'fetch_if_missing' swap seems very fragile. I'm guessing you\n> >> are using it to prevent a loop when calling oid_object_info_extended()\n> >> below. Can you instead pass a flag to the method that disables the\n> >> fetch_if_missing behavior?\n> >\n> > FWIW I mentioned the very same concern here:\n> > https://public-inbox.org/git/nycvar.QRO.7.76.6.1903272300020.41@tvgsbejvaqbjf.bet/\n> >\n> > The situation is *pretty* bad by now. I see `fetch_if_missing` mentioned\n> > 25 times in `master`, and all but one are in .c files or in cache.h.\n> >\n> > The flag is actually used only in `oid_object_info_extended()`, and that\n> > function accepts an `unsigned flags`, so one might think that it could be\n> > extended to accept also a `OBJECT_INFO_LOOKUP_FETCH_IF_MISSING`. But then,\n> > there are many callers of that function, some of them also pretty low in\n> > the food chain. For example, `oid_object_info()` (does not accept `flags`)\n> > or `read_object()` (does not accept flags either).\n> >\n> > So it looks as if the idea to pass this flag down the call chain entailed\n> > a pretty serious avalanche effect.\n>\n> It could be approached in small bits.\n>\n> First, add an OBJECT_INFO_NEVER_FETCH_IF_MISSING flag that overrides\n> fetch_if_missing, and then use the flag in small places like this one.\n> Then, build up to the other methods as appropriate.\n\nThat is a good idea. I fear that it will still take a Herculean effort to\nget there, as some of the call paths strike me as rather deep...\n\n> > An alternative that strikes me as inelegant, still, but nevertheless\n> > better would be to move `fetch_if_missing` into `struct repository`.\n>\n> This is literally the _least_ we should do to reduce our dependence on\n> globals. Maybe this happens first, then the flag idea could be done bits\n> at a time.\n\nOkay, then, I added https://github.com/gitgitgadget/git/issues/251 so we\nwon't forget.\n\nBTW I am rather happy about the way the GitGitGadget issues turn out: I\nadded a couple of left-over bits, and could already close two tickets\nafter other developers pointed out that they had already been addressed,\nsomething an unsuspecting GSoC student, for example, could not otherwise\nhave found out very easily (or for that matter, I myself...).\n\nCiao,\nDscho\n"},{"id":"376483","messageId":"xmqqlfymwqic.fsf@gitster-ct.c.googlers.com","threadId":"50903","inReplyTo":"2329a4c7-bfb9-10ce-9d1c-8c754d6dee05@gmail.com","subject":"Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-31T16:14:03Z","receivedAt":"2019-05-31T16:14:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>>> This global 'fetch_if_missing' swap seems very fragile. I'm guessing you\n>>> are using it to prevent a loop when calling oid_object_info_extended()\n>>> below. Can you instead pass a flag to the method that disables the\n>>> fetch_if_missing behavior?\n>> ...\n>> The flag is actually used only in `oid_object_info_extended()`, and that\n>> function accepts an `unsigned flags`, so one might think that it could be\n>> extended to accept also a `OBJECT_INFO_LOOKUP_FETCH_IF_MISSING`. But then,\n>> there are many callers of that function, some of them also pretty low in\n>> the food chain. For example, `oid_object_info()` (does not accept `flags`)\n>> or `read_object()` (does not accept flags either).\n>>\n>> So it looks as if the idea to pass this flag down the call chain entailed\n>> a pretty serious avalanche effect.\n>\n> It could be approached in small bits.\n>\n> First, add an OBJECT_INFO_NEVER_FETCH_IF_MISSING flag that overrides fetch_if_missing,\n> and then use the flag in small places like this one. Then, build up to the other\n> methods as appropriate.\n>\n>> An alternative that strikes me as inelegant, still, but nevertheless\n>> better would be to move `fetch_if_missing` into `struct repository`.\n>\n> This is literally the _least_ we should do to reduce our dependence on\n> globals. Maybe this happens first, then the flag idea could be done bits\n> at a time.\n\nThe bit is not an attribute of a repository instance, and I agree it\nis an ugly hack to take advantage of an unrelated fact that a repo\nis getting passed throughout the codechain. It is better than\nnothing if we stop there and will not do anything more to the topic,\nbut in the longer term, it is not that better than a global, I am\nafraid. We may not be doing the save-flip-and-restore-the-bit dance\non the global anymore, but instead would be doing the same for the\nfield in the repository object, no?\n\nIn any case, thanks for taking a look at the topic; what it wants to\nachieve is worthwhile, but its execution does look like it needs\nquite a lot more polishing, which is helped by review comments like\nthese.\n\n\n"},{"id":"377998","messageId":"CAP8UFD2-BUVsaVcO-pRNY+qc8WBZC=NUOQHnNYbBVKc=q_aOSg@mail.gmail.com","threadId":"50903","inReplyTo":"CAP8UFD0zvqC2s3_RsWfKwoWQL=2iV6PbaL8+hwUXrWBVe+6sgA@mail.gmail.com","subject":"Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-25T13:50:21Z","receivedAt":"2019-06-25T13:50:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, May 31, 2019 at 7:10 AM Christian Couder\n wrote:\n\n> On Thu, May 30, 2019 at 7:21 PM Derrick Stolee wrote:\n> >\n> > On 4/9/2019 12:11 PM, Christian Couder wrote:\n\n> > > +{\n> > > + int i, missing_nr = 0;\n> > > + int *missing = xcalloc(oid_nr, sizeof(*missing));\n> > > + struct object_id *old_oids = *oids;\n> > > + struct object_id *new_oids;\n> > > + int old_fetch_if_missing = fetch_if_missing;\n> > > +\n> > > + fetch_if_missing = 0;\n> >\n> > This global 'fetch_if_missing' swap seems very fragile. I'm guessing you are using\n> > it to prevent a loop when calling oid_object_info_extended() below. Can you instead\n> > pass a flag to the method that disables the fetch_if_missing behavior?\n>\n> If such a flag existed when I wrote the function I would certainly\n> have used it, as I also dislike this kind of messing with a global\n> (and globals in general).\n>\n> I will see if I can do something about it according to what you\n> suggest later in this thread.\n\nIn the V6 patch series I just sent, the new\nOBJECT_INFO_SKIP_FETCH_OBJECT flag that you introduced is used.\n\n> > > + for (i = 0; i < oid_nr; i++)\n> > > + if (oid_object_info_extended(the_repository, &old_oids[i], NULL, 0)) {\n> >\n> > A use of \"the_repository\" this deep in new code is asking for a refactor later to remove it.\n> > Please try to pass a \"struct repository *r\" through your methods so we minimize references\n> > to the_repository (and the amount of work required to remove them later).\n>\n> Ok, I will take a look at that.\n\nA \"struct repository *r\" is passed in V6. I forgot to mention that in\nthe cover letter.\n\n> > > + missing_nr = remove_fetched_oids(&missing_oids, missing_nr, to_free);\n> >\n> > Here is the one call, and after this assignment \"missing_nr\" does mean the number of missing objects.\n> > However, I do think this could be clarified by using remaining_nr and remaining_oids.\n>\n> Ok, I will take a look at using \"remaining_nr\" and \"remaining_oids\".\n\nDone in V6 too.\n\n> > > + if (missing_nr) {\n> > > + to_free = 1;\n> > > + continue;\n> > > + }\n> >\n> > Now this block took a bit to grok. You use to_free in the if(to_free) free(missing_oids); below.\n> > But it also changes the behavior of remove_fetched_oids(). This means that the first time\n> > remove_fetched_oids() will preserve the list (because it is the input list) but all later\n> > calls will free the newly-created intermediate list. This checks out.\n> >\n> > What is confusing to me: is there any reason that missing_nr would be zero in this situation?\n>\n> I don't think so but I will check again, and maybe add a comment.\n\nActually missing_nr, or now remaining_nr, would be 0 if all the\npromised objects have been fetched.\n\n> > > + }\n> > > + res = 0;\n> > > + break;\n> > > + }\n> > > +\n> > > + if (to_free)\n> > > + free(missing_oids);\n> > > +\n> > > + return res;\n> > > +}\n> >\n> > While the test coverage report brought this patch to my attention, it does seem correct.\n> > I still think a test exposing this method would be good, especially one that requires\n> > a fetch_objects() call to multiple remotes to really exercise the details of remove_fetched_oids().\n>\n> Yeah, I would like to actually test it. I will take another look at\n> what can be done to test this. Perhaps I will look at what can be done\n> to still get some objects when fetching from a promisor/partial clone\n> remote even when it doesn't have all of the objects we request.\n\nI haven't improved test coverage or looked at how we could better\nhandle a partial fetch. I plan to look at that soon.\n\nThanks,\nChristian.\n"}]}