{"thread":{"id":"49330","subject":"[PATCH 0/2] Bugfix for partial clones and ref-in-want servers","startedAt":"2018-09-12T15:47:45Z","lastAt":"2018-09-13T20:55:56Z","messageCount":6,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"357972","messageId":"cover.1536767071.git.jonathantanmy@google.com","threadId":"49330","inReplyTo":null,"subject":"[PATCH 0/2] Bugfix for partial clones and ref-in-want servers","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-12T15:47:36Z","receivedAt":"2018-09-12T15:47:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A coworker discovered a bug when having a partial clone against a server\nthat supports ref-in-want - the lazy fetches no longer work, because the\nclient attempts to fetch with \"want-ref <hash>\", which is not permitted\nby the protocol.\n\nThe first patch deduplicates some code, so that the bugfix need only be\napplied once, and the second patch contains the actual bugfix.\n\nJonathan Tan (2):\n  fetch-object: provide only one fetching function\n  fetch-object: set exact_oid when fetching\n\n fetch-object.c           | 17 ++++++-----------\n fetch-object.h           |  8 ++------\n sha1-file.c              |  2 +-\n t/t0410-partial-clone.sh | 12 ++++++++++++\n unpack-trees.c           |  2 +-\n 5 files changed, 22 insertions(+), 19 deletions(-)\n\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"357973","messageId":"72399daf65b9c0a7ed12589584f5108a9ff6fc26.1536767071.git.jonathantanmy@google.com","threadId":"49330","inReplyTo":"cover.1536767071.git.jonathantanmy@google.com","subject":"[PATCH 1/2] fetch-object: provide only one fetching function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-12T15:47:37Z","receivedAt":"2018-09-12T15:47:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"fetch-object.h currently provides two functions (fetch_object() and\nfetch_objects()) that could be replaced by a single, more flexible\nfunction. Replace those two functions with the more flexible function.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-object.c | 16 +++++-----------\n fetch-object.h |  8 ++------\n sha1-file.c    |  2 +-\n unpack-trees.c |  2 +-\n 4 files changed, 9 insertions(+), 19 deletions(-)\n\ndiff --git a/fetch-object.c b/fetch-object.c\nindex 853624f81..1af1bf857 100644\n--- a/fetch-object.c\n+++ b/fetch-object.c\n@@ -23,21 +23,15 @@ static void fetch_refs(const char *remote_name, struct ref *ref)\n \tfetch_if_missing = original_fetch_if_missing;\n }\n \n-void fetch_object(const char *remote_name, const unsigned char *sha1)\n-{\n-\tstruct ref *ref = alloc_ref(sha1_to_hex(sha1));\n-\thashcpy(ref->old_oid.hash, sha1);\n-\tfetch_refs(remote_name, ref);\n-}\n-\n-void fetch_objects(const char *remote_name, const struct oid_array *to_fetch)\n+void 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 < to_fetch->nr; i++) {\n-\t\tstruct ref *new_ref = alloc_ref(oid_to_hex(&to_fetch->oid[i]));\n-\t\toidcpy(&new_ref->old_oid, &to_fetch->oid[i]);\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->next = ref;\n \t\tref = new_ref;\n \t}\ndiff --git a/fetch-object.h b/fetch-object.h\nindex 4b269d07e..d2f996d4e 100644\n--- a/fetch-object.h\n+++ b/fetch-object.h\n@@ -1,11 +1,7 @@\n #ifndef FETCH_OBJECT_H\n #define FETCH_OBJECT_H\n \n-#include \"sha1-array.h\"\n-\n-extern void fetch_object(const char *remote_name, const unsigned char *sha1);\n-\n-extern void fetch_objects(const char *remote_name,\n-\t\t\t  const struct oid_array *to_fetch);\n+void 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 97b742384..2edf4564f 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1317,7 +1317,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\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-\t\t\tfetch_object(repository_format_partial_clone, real->hash);\n+\t\t\tfetch_objects(repository_format_partial_clone, real, 1);\n \t\t\talready_retried = 1;\n \t\t\tcontinue;\n \t\t}\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f25089b87..82a83dbc6 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -392,7 +392,7 @@ static int check_updates(struct unpack_trees_options *o)\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);\n+\t\t\t\t      to_fetch.oid, to_fetch.nr);\n \t\tfetch_if_missing = fetch_if_missing_store;\n \t\toid_array_clear(&to_fetch);\n \t}\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"357974","messageId":"ac5abaa46d41afe46a944264277e75ce4365d6fd.1536767071.git.jonathantanmy@google.com","threadId":"49330","inReplyTo":"cover.1536767071.git.jonathantanmy@google.com","subject":"[PATCH 2/2] fetch-object: set exact_oid when fetching","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-12T15:47:38Z","receivedAt":"2018-09-12T15:47:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"fetch_objects() currently does not set exact_oid in struct ref when\ninvoking transport_fetch_refs(). If the server supports ref-in-want,\nfetch_pack() uses this field to determine whether a wanted ref should be\nrequested as a \"want-ref\" line or a \"want\" line; without the setting of\nexact_oid, the wrong line will be sent.\n\nSet exact_oid, so that the correct line is sent.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-object.c           |  1 +\n t/t0410-partial-clone.sh | 12 ++++++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/fetch-object.c b/fetch-object.c\nindex 1af1bf857..426654880 100644\n--- a/fetch-object.c\n+++ b/fetch-object.c\n@@ -32,6 +32,7 @@ void fetch_objects(const char *remote_name, const struct object_id *oids,\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}\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 128130066..0ab02c337 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -170,6 +170,18 @@ test_expect_success 'fetching of missing objects' '\n \tgit verify-pack --verbose \"$IDX\" | grep \"$HASH\"\n '\n \n+test_expect_success 'fetching of missing objects works with ref-in-want enabled' '\n+\t# ref-in-want requires protocol version 2\n+\tgit -C server config protocol.version 2 &&\n+\tgit -C server config uploadpack.allowrefinwant 1 &&\n+\tgit -C repo config protocol.version 2 &&\n+\n+\trm -rf repo/.git/objects/* &&\n+\trm -f trace &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C repo cat-file -p \"$HASH\" &&\n+\tgrep \"git< fetch=.*ref-in-want\" trace\n+'\n+\n test_expect_success 'rev-list stops traversal at missing and promised commit' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-- \n2.19.0.397.gdd90340f6a-goog\n\n"},{"id":"358013","messageId":"xmqqefdyjsgh.fsf@gitster-ct.c.googlers.com","threadId":"49330","inReplyTo":"72399daf65b9c0a7ed12589584f5108a9ff6fc26.1536767071.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] fetch-object: provide only one fetching function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T21:50:38Z","receivedAt":"2018-09-12T21:50:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> fetch-object.h currently provides two functions (fetch_object() and\n> fetch_objects()) that could be replaced by a single, more flexible\n> function. Replace those two functions with the more flexible function.\n\nThe latter half of the first sentence and the second sentence are\npretty-much repeating the same thing.  \n\nI think you wanted to justify two changes:\n\n (1) There are two helpers.  fetch_object() fetches a single object\n     from a given remote; fetch_objects() fetches a set of objects\n     from a given remote.  It is not like the former is implemented\n     as a specialization of the latter (i.e. passing length=1\n     array), and it is not like the former is optimized specially\n     for a single object fetch that cannot be made into such a thin\n     wrapper.  It is not like the latter is implemented as a\n     repeated call to the former in a loop, either.  There is no\n     justification to keep two independent implementations, and\n     using a single helper that can be used by all the callers of\n     these two would make sense.\n\n (2) The variant that fetches multiple objects takes oid_array.  To\n     be used by all the existing callers of these two helpers, it is\n     better to use a different calling convention, namely, a array[]\n     and its length passed as separate parameters.\n     \nInstead of explaining why the new convention is better to justify\n(2), the above three lines handwave by saying \"more flexible\"\ntwice.  We should do better.\n\n\tfetch-object: unify fetch_object[s] functions\n\n\tThere are fetch_object() and fetch_objects() helpers in\n\tfetch-object.h; as the latter takes \"struct oid_array\",\n\tthe former cannot be made into a thin wrapper around the\n\tlatter without an extra allocation and set-up cost.\n\n\tUpdate fetch_objects() to take an array of \"struct\n\tobject_id\" and number of elements in it as separate\n\tparameters, remove fetch_object(), and adjust all existing\n\tcallers of these functions to use the new fetch_objects().\n\nperhaps?\n\n> diff --git a/sha1-file.c b/sha1-file.c\n> index 97b742384..2edf4564f 100644\n> --- a/sha1-file.c\n> +++ b/sha1-file.c\n> @@ -1317,7 +1317,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\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> -\t\t\tfetch_object(repository_format_partial_clone, real->hash);\n> +\t\t\tfetch_objects(repository_format_partial_clone, real, 1);\n>  \t\t\talready_retried = 1;\n>  \t\t\tcontinue;\n>  \t\t}\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index f25089b87..82a83dbc6 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -392,7 +392,7 @@ static int check_updates(struct unpack_trees_options *o)\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);\n> +\t\t\t\t      to_fetch.oid, to_fetch.nr);\n>  \t\tfetch_if_missing = fetch_if_missing_store;\n>  \t\toid_array_clear(&to_fetch);\n>  \t}\n\nChanges to these two callers do explain why <ptr, nr> is an\ninterface that is easier to use.  Those who already have an\noid_array can pass its fields as separate parameters without too\nmuch trouble, and those who only have an oid (or an array of oid and\nknows how many are in the array) do not have to wrap them into an\noid_array.\n\nThanks.\n"},{"id":"358080","messageId":"20180913180932.93341-1-jonathantanmy@google.com","threadId":"49330","inReplyTo":"xmqqefdyjsgh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] fetch-object: provide only one fetching function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-13T18:09:32Z","receivedAt":"2018-09-13T18:09:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Instead of explaining why the new convention is better to justify\n> (2), the above three lines handwave by saying \"more flexible\"\n> twice.  We should do better.\n> \n> \tfetch-object: unify fetch_object[s] functions\n> \n> \tThere are fetch_object() and fetch_objects() helpers in\n> \tfetch-object.h; as the latter takes \"struct oid_array\",\n> \tthe former cannot be made into a thin wrapper around the\n> \tlatter without an extra allocation and set-up cost.\n> \n> \tUpdate fetch_objects() to take an array of \"struct\n> \tobject_id\" and number of elements in it as separate\n> \tparameters, remove fetch_object(), and adjust all existing\n> \tcallers of these functions to use the new fetch_objects().\n> \n> perhaps?\n\nThanks - your explanation is much clearer than mine. Let me know if you\nwant a reroll (or if you can update the commit message yourself, that's\nfine too).\n"},{"id":"358090","messageId":"xmqqa7olf76w.fsf@gitster-ct.c.googlers.com","threadId":"49330","inReplyTo":"20180913180932.93341-1-jonathantanmy@google.com","subject":"Re: [PATCH 1/2] fetch-object: provide only one fetching function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-13T20:55:51Z","receivedAt":"2018-09-13T20:55:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> Instead of explaining why the new convention is better to justify\n>> (2), the above three lines handwave by saying \"more flexible\"\n>> twice.  We should do better.\n>> \n>> \tfetch-object: unify fetch_object[s] functions\n>> \n>> \tThere are fetch_object() and fetch_objects() helpers in\n>> \tfetch-object.h; as the latter takes \"struct oid_array\",\n>> \tthe former cannot be made into a thin wrapper around the\n>> \tlatter without an extra allocation and set-up cost.\n>> \n>> \tUpdate fetch_objects() to take an array of \"struct\n>> \tobject_id\" and number of elements in it as separate\n>> \tparameters, remove fetch_object(), and adjust all existing\n>> \tcallers of these functions to use the new fetch_objects().\n>> \n>> perhaps?\n>\n> Thanks - your explanation is much clearer than mine. Let me know if you\n> want a reroll (or if you can update the commit message yourself, that's\n> fine too).\n\nIf there is no other change needed for either of the patches, I do\nnot mind rewording the 1/2 myself to save a round-trip.\n\nThanks.\n"}]}