[PATCH 0/2] More fetch_if_missing=0 removal

[PATCH 0/2] More fetch_if_missing=0 removal

[Jonathan Tan]

This is based on jt/fetch-remove-lazy-fetch-plugging.

This patch set both uses the work in commit 6462d5eb9a ("fetch: remove
fetch_if_missing=0", 2019-11-08) and covers up some of the edge use
cases that the aforementioned commit did not cover.

Junio wrote in [1]:

> What I am not certain about is if we are ready
> to move to the right direction for the longer term, or we should still
> be relying on the big-hammer global bit for expediency a bit longer.

I'm not sure if this patch set is an argument for or against. I did
think 6462d5eb9a would be sufficient (but as these patches prove, it was
not), but at the same time, with the additional test coverage provided
by these patches, the overall work might be worth cooking in next.
(Whenever, at $DAY_JOB, we encounter an edge case with partial clone, we
have been submitting patches with test cases for "git fetch" or "git
clone" but sometimes not both, so between 6462d5eb9a and this patch set,
these edge cases would be covered too.)

[1] https://public-inbox.org/git/xmqqh83s8h3m.fsf@gitster-ct.c.googlers.com/

Jonathan Tan (2):
  clone: remove fetch_if_missing=0
  promisor-remote: remove fetch_if_missing=0

 builtin/clone.c   |  3 ---
 connected.c       |  3 ++-
 fetch-pack.c      | 46 ++++++++++++++++++++++++++++++++--------------
 promisor-remote.c |  3 ---
 4 files changed, 34 insertions(+), 21 deletions(-)

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH 1/2] clone: remove fetch_if_missing=0

[Jonathan Tan]

Commit 6462d5eb9a ("fetch: remove fetch_if_missing=0", 2019-11-08)
strove to remove the need for fetch_if_missing=0 from the fetching
mechanism, so it is plausible to attempt removing fetch_if_missing=0
from clone as well. But doing so reveals a bug - when the server does
not send an object directly pointed to by a ref, this should be an
error, not a trigger for a lazy fetch. (This case in the fetching
mechanism was covered by a test using "git clone", not "git fetch",
which is why the aforementioned commit didn't uncover the bug.)

The bug can be fixed by suppressing lazy-fetching during the
connectivity check. Fix this bug, and remove fetch_if_missing from
clone.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/clone.c | 3 ---
 connected.c     | 3 ++-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index c46ee29f0a..8bf12ce5e3 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -927,8 +927,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
 
-	fetch_if_missing = 0;
-
 	packet_trace_identity("clone");
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
 			     builtin_clone_usage, 0);
@@ -1265,7 +1263,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 
 	junk_mode = JUNK_LEAVE_REPO;
-	fetch_if_missing = 1;
 	err = checkout(submodule_progress);
 
 	strbuf_release(&reflog_msg);
diff --git a/connected.c b/connected.c
index 36c4e5dedb..c337f5f7f4 100644
--- a/connected.c
+++ b/connected.c
@@ -62,7 +62,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		 * received the objects pointed to by each wanted ref.
 		 */
 		do {
-			if (!repo_has_object_file(the_repository, &oid))
+			if (!repo_has_object_file_with_flags(the_repository, &oid,
+							     OBJECT_INFO_SKIP_FETCH_OBJECT))
 				return 1;
 		} while (!fn(cb_data, &oid));
 		return 0;
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH 1/2] clone: remove fetch_if_missing=0

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> diff --git a/connected.c b/connected.c
> index 36c4e5dedb..c337f5f7f4 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -62,7 +62,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>  		 * received the objects pointed to by each wanted ref.
>  		 */
>  		do {
> -			if (!repo_has_object_file(the_repository, &oid))
> +			if (!repo_has_object_file_with_flags(the_repository, &oid,
> +							     OBJECT_INFO_SKIP_FETCH_OBJECT))

Wow, good find.  Really good find.  I guess the exercise to see if
we can get rid of (or at least use less of) the big knob was a good
idea.

Thanks, will queue.


>  				return 1;
>  		} while (!fn(cb_data, &oid));
>  		return 0;

[PATCH 2/2] promisor-remote: remove fetch_if_missing=0

[Jonathan Tan]

Commit 6462d5eb9a ("fetch: remove fetch_if_missing=0", 2019-11-08)
strove to remove the need for fetch_if_missing=0 from the fetching
mechanism, so it is plausible to attempt removing fetch_if_missing=0
from the lazy-fetching mechanism in promisor-remote as well.

But doing so reveals a bug - when the server does not send an object
pointed to by a tag object, an infinite loop occurs: Git attempts to
fetch the missing object, which causes a deferencing of all refs (for
negotiation), which causes a lazy fetch of that missing object, and so
on. This bug is because of unnecessary use of the fetch negotiator
during lazy fetching - it is not used after initialization, but it is
still initialized (which causes the dereferencing of all refs).

Thus, when the negotiator is not used during fetching, refrain from
initializing it. Then, remove fetch_if_missing from promisor-remote.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c      | 46 ++++++++++++++++++++++++++++++++--------------
 promisor-remote.c |  3 ---
 2 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 37178e2d34..490b111822 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -896,8 +896,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	struct object_id oid;
 	const char *agent_feature;
 	int agent_len;
-	struct fetch_negotiator negotiator;
-	fetch_negotiator_init(r, &negotiator);
+	struct fetch_negotiator negotiator_alloc;
+	struct fetch_negotiator *negotiator;
+
+	if (args->no_dependents) {
+		negotiator = NULL;
+	} else {
+		negotiator = &negotiator_alloc;
+		fetch_negotiator_init(r, negotiator);
+	}
 
 	sort_ref_list(&ref, ref_compare_name);
 	QSORT(sought, nr_sought, cmp_ref_by_name);
@@ -984,7 +991,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		die(_("Server does not support --deepen"));
 
 	if (!args->no_dependents) {
-		mark_complete_and_common_ref(&negotiator, args, &ref);
+		mark_complete_and_common_ref(negotiator, args, &ref);
 		filter_refs(args, &ref, sought, nr_sought);
 		if (everything_local(args, &ref)) {
 			packet_flush(fd[1]);
@@ -993,7 +1000,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	} else {
 		filter_refs(args, &ref, sought, nr_sought);
 	}
-	if (find_common(&negotiator, args, fd, &oid, ref) < 0)
+	if (find_common(negotiator, args, fd, &oid, ref) < 0)
 		if (!args->keep_pack)
 			/* When cloning, it is not unusual to have
 			 * no common commit.
@@ -1013,7 +1020,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 		die(_("git fetch-pack: fetch failed."));
 
  all_done:
-	negotiator.release(&negotiator);
+	if (negotiator)
+		negotiator->release(negotiator);
 	return ref;
 }
 
@@ -1231,7 +1239,8 @@ static int process_acks(struct fetch_negotiator *negotiator,
 				struct commit *commit;
 				oidset_insert(common, &oid);
 				commit = lookup_commit(the_repository, &oid);
-				negotiator->ack(negotiator, commit);
+				if (negotiator)
+					negotiator->ack(negotiator, commit);
 			}
 			continue;
 		}
@@ -1383,8 +1392,16 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	struct packet_reader reader;
 	int in_vain = 0, negotiation_started = 0;
 	int haves_to_send = INITIAL_FLUSH;
-	struct fetch_negotiator negotiator;
-	fetch_negotiator_init(r, &negotiator);
+	struct fetch_negotiator negotiator_alloc;
+	struct fetch_negotiator *negotiator;
+
+	if (args->no_dependents) {
+		negotiator = NULL;
+	} else {
+		negotiator = &negotiator_alloc;
+		fetch_negotiator_init(r, negotiator);
+	}
+
 	packet_reader_init(&reader, fd[0], NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_DIE_ON_ERR_PACKET);
@@ -1408,15 +1425,15 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 
 			/* Filter 'ref' by 'sought' and those that aren't local */
 			if (!args->no_dependents) {
-				mark_complete_and_common_ref(&negotiator, args, &ref);
+				mark_complete_and_common_ref(negotiator, args, &ref);
 				filter_refs(args, &ref, sought, nr_sought);
 				if (everything_local(args, &ref))
 					state = FETCH_DONE;
 				else
 					state = FETCH_SEND_REQUEST;
 
-				mark_tips(&negotiator, args->negotiation_tips);
-				for_each_cached_alternate(&negotiator,
+				mark_tips(negotiator, args->negotiation_tips);
+				for_each_cached_alternate(negotiator,
 							  insert_one_alternate_object);
 			} else {
 				filter_refs(args, &ref, sought, nr_sought);
@@ -1430,7 +1447,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 						    "negotiation_v2",
 						    the_repository);
 			}
-			if (send_fetch_request(&negotiator, fd[1], args, ref,
+			if (send_fetch_request(negotiator, fd[1], args, ref,
 					       &common,
 					       &haves_to_send, &in_vain,
 					       reader.use_sideband))
@@ -1440,7 +1457,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 			break;
 		case FETCH_PROCESS_ACKS:
 			/* Process ACKs/NAKs */
-			switch (process_acks(&negotiator, &reader, &common)) {
+			switch (process_acks(negotiator, &reader, &common)) {
 			case 2:
 				state = FETCH_GET_PACK;
 				break;
@@ -1475,7 +1492,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		}
 	}
 
-	negotiator.release(&negotiator);
+	if (negotiator)
+		negotiator->release(negotiator);
 	oidset_clear(&common);
 	return ref;
 }
diff --git a/promisor-remote.c b/promisor-remote.c
index 9bd5b79d59..9f338c945f 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -16,10 +16,8 @@ static int fetch_refs(const char *remote_name, struct ref *ref)
 {
 	struct remote *remote;
 	struct transport *transport;
-	int original_fetch_if_missing = fetch_if_missing;
 	int res;
 
-	fetch_if_missing = 0;
 	remote = remote_get(remote_name);
 	if (!remote->url[0])
 		die(_("Remote with no URL"));
@@ -28,7 +26,6 @@ static int fetch_refs(const char *remote_name, struct ref *ref)
 	transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
 	transport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, "1");
 	res = transport_fetch_refs(transport, ref);
-	fetch_if_missing = original_fetch_if_missing;
 
 	return res;
 }
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH 0/2] More fetch_if_missing=0 removal

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> (Whenever, at $DAY_JOB, we encounter an edge case with partial clone, we
> have been submitting patches with test cases for "git fetch" or "git
> clone" but sometimes not both, so between 6462d5eb9a and this patch set,
> these edge cases would be covered too.)

Thanks, wonderful.