{"thread":{"id":"52256","subject":"[PATCH 0/2] More fetch_if_missing=0 removal","startedAt":"2019-11-13T00:34:26Z","lastAt":"2019-11-13T02:50:55Z","messageCount":5,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"386068","messageId":"cover.1573604516.git.jonathantanmy@google.com","threadId":"52256","inReplyTo":null,"subject":"[PATCH 0/2] More fetch_if_missing=0 removal","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-13T00:34:18Z","receivedAt":"2019-11-13T00:34:26Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is based on jt/fetch-remove-lazy-fetch-plugging.\n\nThis patch set both uses the work in commit 6462d5eb9a (\"fetch: remove\nfetch_if_missing=0\", 2019-11-08) and covers up some of the edge use\ncases that the aforementioned commit did not cover.\n\nJunio wrote in [1]:\n\n> What I am not certain about is if we are ready\n> to move to the right direction for the longer term, or we should still\n> be relying on the big-hammer global bit for expediency a bit longer.\n\nI'm not sure if this patch set is an argument for or against. I did\nthink 6462d5eb9a would be sufficient (but as these patches prove, it was\nnot), but at the same time, with the additional test coverage provided\nby these patches, the overall work might be worth cooking in next.\n(Whenever, at $DAY_JOB, we encounter an edge case with partial clone, we\nhave been submitting patches with test cases for \"git fetch\" or \"git\nclone\" but sometimes not both, so between 6462d5eb9a and this patch set,\nthese edge cases would be covered too.)\n\n[1] https://public-inbox.org/git/xmqqh83s8h3m.fsf@gitster-ct.c.googlers.com/\n\nJonathan Tan (2):\n clone: remove fetch_if_missing=0\n promisor-remote: remove fetch_if_missing=0\n\n builtin/clone.c | 3 ---\n connected.c | 3 ++-\n fetch-pack.c | 46 ++++++++++++++++++++++++++++++++--------------\n promisor-remote.c | 3 ---\n 4 files changed, 34 insertions(+), 21 deletions(-)\n\n-- \n2.24.0.rc1.363.gb1bccd3e3d-goog\n\n"},{"id":"386069","messageId":"84d8cee52e7c1dc030826c8caa0e107f80783b9c.1573604516.git.jonathantanmy@google.com","threadId":"52256","inReplyTo":"cover.1573604516.git.jonathantanmy@google.com","subject":"[PATCH 1/2] clone: remove fetch_if_missing=0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-13T00:34:19Z","receivedAt":"2019-11-13T00:34:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 6462d5eb9a (\"fetch: remove fetch_if_missing=0\", 2019-11-08)\nstrove to remove the need for fetch_if_missing=0 from the fetching\nmechanism, so it is plausible to attempt removing fetch_if_missing=0\nfrom clone as well. But doing so reveals a bug - when the server does\nnot send an object directly pointed to by a ref, this should be an\nerror, not a trigger for a lazy fetch. (This case in the fetching\nmechanism was covered by a test using \"git clone\", not \"git fetch\",\nwhich is why the aforementioned commit didn't uncover the bug.)\n\nThe bug can be fixed by suppressing lazy-fetching during the\nconnectivity check. Fix this bug, and remove fetch_if_missing from\nclone.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/clone.c | 3 ---\n connected.c | 3 ++-\n 2 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex c46ee29f0a..8bf12ce5e3 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -927,8 +927,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tstruct argv_array ref_prefixes = ARGV_ARRAY_INIT;\n \n-\tfetch_if_missing = 0;\n-\n \tpacket_trace_identity(\"clone\");\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n \t\t\t builtin_clone_usage, 0);\n@@ -1265,7 +1263,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tjunk_mode = JUNK_LEAVE_REPO;\n-\tfetch_if_missing = 1;\n \terr = checkout(submodule_progress);\n \n \tstrbuf_release(&reflog_msg);\ndiff --git a/connected.c b/connected.c\nindex 36c4e5dedb..c337f5f7f4 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -62,7 +62,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t * received the objects pointed to by each wanted ref.\n \t\t */\n \t\tdo {\n-\t\t\tif (!repo_has_object_file(the_repository, &oid))\n+\t\t\tif (!repo_has_object_file_with_flags(the_repository, &oid,\n+\t\t\t\t\t\t\t OBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\t\treturn 1;\n \t\t} while (!fn(cb_data, &oid));\n \t\treturn 0;\n-- \n2.24.0.rc1.363.gb1bccd3e3d-goog\n\n"},{"id":"386070","messageId":"82daff8665c0a9b5df1dacbc2cff16d98dcc8175.1573604516.git.jonathantanmy@google.com","threadId":"52256","inReplyTo":"cover.1573604516.git.jonathantanmy@google.com","subject":"[PATCH 2/2] promisor-remote: remove fetch_if_missing=0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-13T00:34:20Z","receivedAt":"2019-11-13T00:34:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 6462d5eb9a (\"fetch: remove fetch_if_missing=0\", 2019-11-08)\nstrove to remove the need for fetch_if_missing=0 from the fetching\nmechanism, so it is plausible to attempt removing fetch_if_missing=0\nfrom the lazy-fetching mechanism in promisor-remote as well.\n\nBut doing so reveals a bug - when the server does not send an object\npointed to by a tag object, an infinite loop occurs: Git attempts to\nfetch the missing object, which causes a deferencing of all refs (for\nnegotiation), which causes a lazy fetch of that missing object, and so\non. This bug is because of unnecessary use of the fetch negotiator\nduring lazy fetching - it is not used after initialization, but it is\nstill initialized (which causes the dereferencing of all refs).\n\nThus, when the negotiator is not used during fetching, refrain from\ninitializing it. Then, remove fetch_if_missing from promisor-remote.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 46 ++++++++++++++++++++++++++++++++--------------\n promisor-remote.c | 3 ---\n 2 files changed, 32 insertions(+), 17 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 37178e2d34..490b111822 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -896,8 +896,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tstruct object_id oid;\n \tconst char *agent_feature;\n \tint agent_len;\n-\tstruct fetch_negotiator negotiator;\n-\tfetch_negotiator_init(r, &negotiator);\n+\tstruct fetch_negotiator negotiator_alloc;\n+\tstruct fetch_negotiator *negotiator;\n+\n+\tif (args->no_dependents) {\n+\t\tnegotiator = NULL;\n+\t} else {\n+\t\tnegotiator = &negotiator_alloc;\n+\t\tfetch_negotiator_init(r, negotiator);\n+\t}\n \n \tsort_ref_list(&ref, ref_compare_name);\n \tQSORT(sought, nr_sought, cmp_ref_by_name);\n@@ -984,7 +991,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\tdie(_(\"Server does not support --deepen\"));\n \n \tif (!args->no_dependents) {\n-\t\tmark_complete_and_common_ref(&negotiator, args, &ref);\n+\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n \t\tfilter_refs(args, &ref, sought, nr_sought);\n \t\tif (everything_local(args, &ref)) {\n \t\t\tpacket_flush(fd[1]);\n@@ -993,7 +1000,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t} else {\n \t\tfilter_refs(args, &ref, sought, nr_sought);\n \t}\n-\tif (find_common(&negotiator, args, fd, &oid, ref) < 0)\n+\tif (find_common(negotiator, args, fd, &oid, ref) < 0)\n \t\tif (!args->keep_pack)\n \t\t\t/* When cloning, it is not unusual to have\n \t\t\t * no common commit.\n@@ -1013,7 +1020,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\tdie(_(\"git fetch-pack: fetch failed.\"));\n \n all_done:\n-\tnegotiator.release(&negotiator);\n+\tif (negotiator)\n+\t\tnegotiator->release(negotiator);\n \treturn ref;\n }\n \n@@ -1231,7 +1239,8 @@ static int process_acks(struct fetch_negotiator *negotiator,\n \t\t\t\tstruct commit *commit;\n \t\t\t\toidset_insert(common, &oid);\n \t\t\t\tcommit = lookup_commit(the_repository, &oid);\n-\t\t\t\tnegotiator->ack(negotiator, commit);\n+\t\t\t\tif (negotiator)\n+\t\t\t\t\tnegotiator->ack(negotiator, commit);\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n@@ -1383,8 +1392,16 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct packet_reader reader;\n \tint in_vain = 0, negotiation_started = 0;\n \tint haves_to_send = INITIAL_FLUSH;\n-\tstruct fetch_negotiator negotiator;\n-\tfetch_negotiator_init(r, &negotiator);\n+\tstruct fetch_negotiator negotiator_alloc;\n+\tstruct fetch_negotiator *negotiator;\n+\n+\tif (args->no_dependents) {\n+\t\tnegotiator = NULL;\n+\t} else {\n+\t\tnegotiator = &negotiator_alloc;\n+\t\tfetch_negotiator_init(r, negotiator);\n+\t}\n+\n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n@@ -1408,15 +1425,15 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \n \t\t\t/* Filter 'ref' by 'sought' and those that aren't local */\n \t\t\tif (!args->no_dependents) {\n-\t\t\t\tmark_complete_and_common_ref(&negotiator, args, &ref);\n+\t\t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n \t\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n \t\t\t\tif (everything_local(args, &ref))\n \t\t\t\t\tstate = FETCH_DONE;\n \t\t\t\telse\n \t\t\t\t\tstate = FETCH_SEND_REQUEST;\n \n-\t\t\t\tmark_tips(&negotiator, args->negotiation_tips);\n-\t\t\t\tfor_each_cached_alternate(&negotiator,\n+\t\t\t\tmark_tips(negotiator, args->negotiation_tips);\n+\t\t\t\tfor_each_cached_alternate(negotiator,\n \t\t\t\t\t\t\t insert_one_alternate_object);\n \t\t\t} else {\n \t\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n@@ -1430,7 +1447,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t\t\t \"negotiation_v2\",\n \t\t\t\t\t\t the_repository);\n \t\t\t}\n-\t\t\tif (send_fetch_request(&negotiator, fd[1], args, ref,\n+\t\t\tif (send_fetch_request(negotiator, fd[1], args, ref,\n \t\t\t\t\t &common,\n \t\t\t\t\t &haves_to_send, &in_vain,\n \t\t\t\t\t reader.use_sideband))\n@@ -1440,7 +1457,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\tbreak;\n \t\tcase FETCH_PROCESS_ACKS:\n \t\t\t/* Process ACKs/NAKs */\n-\t\t\tswitch (process_acks(&negotiator, &reader, &common)) {\n+\t\t\tswitch (process_acks(negotiator, &reader, &common)) {\n \t\t\tcase 2:\n \t\t\t\tstate = FETCH_GET_PACK;\n \t\t\t\tbreak;\n@@ -1475,7 +1492,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t}\n \t}\n \n-\tnegotiator.release(&negotiator);\n+\tif (negotiator)\n+\t\tnegotiator->release(negotiator);\n \toidset_clear(&common);\n \treturn ref;\n }\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 9bd5b79d59..9f338c945f 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -16,10 +16,8 @@ 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@@ -28,7 +26,6 @@ static int fetch_refs(const char *remote_name, struct ref *ref)\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-- \n2.24.0.rc1.363.gb1bccd3e3d-goog\n\n"},{"id":"386087","messageId":"xmqqy2wkbidw.fsf@gitster-ct.c.googlers.com","threadId":"52256","inReplyTo":"cover.1573604516.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/2] More fetch_if_missing=0 removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-13T02:49:15Z","receivedAt":"2019-11-13T02:49:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> (Whenever, at $DAY_JOB, we encounter an edge case with partial clone, we\n> have been submitting patches with test cases for \"git fetch\" or \"git\n> clone\" but sometimes not both, so between 6462d5eb9a and this patch set,\n> these edge cases would be covered too.)\n\nThanks, wonderful.\n"},{"id":"386088","messageId":"xmqqtv78bibc.fsf@gitster-ct.c.googlers.com","threadId":"52256","inReplyTo":"84d8cee52e7c1dc030826c8caa0e107f80783b9c.1573604516.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] clone: remove fetch_if_missing=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-13T02:50:47Z","receivedAt":"2019-11-13T02:50:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> diff --git a/connected.c b/connected.c\n> index 36c4e5dedb..c337f5f7f4 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -62,7 +62,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n> \t\t * received the objects pointed to by each wanted ref.\n> \t\t */\n> \t\tdo {\n> -\t\t\tif (!repo_has_object_file(the_repository, &oid))\n> +\t\t\tif (!repo_has_object_file_with_flags(the_repository, &oid,\n> +\t\t\t\t\t\t\t OBJECT_INFO_SKIP_FETCH_OBJECT))\n\nWow, good find. Really good find. I guess the exercise to see if\nwe can get rid of (or at least use less of) the big knob was a good\nidea.\n\nThanks, will queue.\n\n\n> \t\t\t\treturn 1;\n> \t\t} while (!fn(cb_data, &oid));\n> \t\treturn 0;\n"}]}