[v2,5/7] fetch-pack: do not lazy-fetch during ref iteration
diff mbox series

Message ID d42e98ff0760932cffb021eb680b76da74e4461f.1597184949.git.jonathantanmy@google.com
State New
Headers show
Series
  • Lazy fetch with subprocess
Related show

Commit Message

Jonathan Tan Aug. 11, 2020, 10:52 p.m. UTC
Teach "fetch-pack" not to lazy fetch whenever iterating over refs. This
is done by using the raw form of ref iteration and by dereferencing tags
ourselves.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c             | 79 ++++++++++++++++++++++------------------
 t/t5616-partial-clone.sh | 20 ++++++++++
 2 files changed, 64 insertions(+), 35 deletions(-)

Comments

Junio C Hamano Aug. 12, 2020, 6:25 p.m. UTC | #1
Jonathan Tan <jonathantanmy@google.com> writes:

> Teach "fetch-pack" not to lazy fetch whenever iterating over
> refs. 

Don't readers need the reason why is it a good thing to do explained
here?

> This
> is done by using the raw form of ref iteration and by dereferencing tags
> ourselves.

Hmph.  The way this patch implements deref_without_lazy_fetch()
makes it hard to reuse in other contexts, as it mixes what is done
by mark_complete(), which is very much "git fetch" specific.

Would "git fetch" be the only user potentially benefit from being
able to dereference a tag that already exists locally and ignore
missing ones?  I wonder if teaching deref_tag() a new flag would be
a better alternative to keep the separation of concerns at different
layers.

> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c             | 79 ++++++++++++++++++++++------------------
>  t/t5616-partial-clone.sh | 20 ++++++++++
>  2 files changed, 64 insertions(+), 35 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 80fb3bd899..707bbc31fd 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -108,24 +108,48 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
>  		cb(negotiator, cache.items[i]);
>  }
>  
> +static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
> +					       int mark_tags_complete)
> +{
> +	enum object_type type;
> +	struct object_info info = { .typep = &type };
> +
> +	while (1) {
> +		if (oid_object_info_extended(the_repository, oid, &info,
> +					     OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))
> +			return NULL;
> +		if (type == OBJ_TAG) {
> +			struct tag *tag = (struct tag *)
> +				parse_object(the_repository, oid);
> +
> +			if (!tag->tagged)
> +				return NULL;
> +			if (mark_tags_complete)
> +				tag->object.flags |= COMPLETE;
> +			oid = &tag->tagged->oid;
> +		} else {
> +			break;
> +		}
> +	}
> +	if (type == OBJ_COMMIT)
> +		return (struct commit *) parse_object(the_repository, oid);
> +	return NULL;
> +}
> +
>  static int rev_list_insert_ref(struct fetch_negotiator *negotiator,
> -			       const char *refname,
>  			       const struct object_id *oid)
>  {
> -	struct object *o = deref_tag(the_repository,
> -				     parse_object(the_repository, oid),
> -				     refname, 0);
> -
> -	if (o && o->type == OBJ_COMMIT)
> -		negotiator->add_tip(negotiator, (struct commit *)o);
> +	struct commit *c = deref_without_lazy_fetch(oid, 0);
>  
> +	if (c)
> +		negotiator->add_tip(negotiator, c);
>  	return 0;
>  }
>  
>  static int rev_list_insert_ref_oid(const char *refname, const struct object_id *oid,
>  				   int flag, void *cb_data)
>  {
> -	return rev_list_insert_ref(cb_data, refname, oid);
> +	return rev_list_insert_ref(cb_data, oid);
>  }
>  
>  enum ack_type {
> @@ -201,7 +225,7 @@ static void send_request(struct fetch_pack_args *args,
>  static void insert_one_alternate_object(struct fetch_negotiator *negotiator,
>  					struct object *obj)
>  {
> -	rev_list_insert_ref(negotiator, NULL, &obj->oid);
> +	rev_list_insert_ref(negotiator, &obj->oid);
>  }
>  
>  #define INITIAL_FLUSH 16
> @@ -230,13 +254,12 @@ static void mark_tips(struct fetch_negotiator *negotiator,
>  	int i;
>  
>  	if (!negotiation_tips) {
> -		for_each_ref(rev_list_insert_ref_oid, negotiator);
> +		for_each_rawref(rev_list_insert_ref_oid, negotiator);
>  		return;
>  	}
>  
>  	for (i = 0; i < negotiation_tips->nr; i++)
> -		rev_list_insert_ref(negotiator, NULL,
> -				    &negotiation_tips->oid[i]);
> +		rev_list_insert_ref(negotiator, &negotiation_tips->oid[i]);
>  	return;
>  }
>  
> @@ -503,21 +526,11 @@ static struct commit_list *complete;
>  
>  static int mark_complete(const struct object_id *oid)
>  {
> -	struct object *o = parse_object(the_repository, oid);
> -
> -	while (o && o->type == OBJ_TAG) {
> -		struct tag *t = (struct tag *) o;
> -		if (!t->tagged)
> -			break; /* broken repository */
> -		o->flags |= COMPLETE;
> -		o = parse_object(the_repository, &t->tagged->oid);
> -	}
> -	if (o && o->type == OBJ_COMMIT) {
> -		struct commit *commit = (struct commit *)o;
> -		if (!(commit->object.flags & COMPLETE)) {
> -			commit->object.flags |= COMPLETE;
> -			commit_list_insert(commit, &complete);
> -		}
> +	struct commit *commit = deref_without_lazy_fetch(oid, 1);
> +
> +	if (commit && !(commit->object.flags & COMPLETE)) {
> +		commit->object.flags |= COMPLETE;
> +		commit_list_insert(commit, &complete);
>  	}
>  	return 0;
>  }
> @@ -702,7 +715,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
>  	 */
>  	trace2_region_enter("fetch-pack", "mark_complete_local_refs", NULL);
>  	if (!args->deepen) {
> -		for_each_ref(mark_complete_oid, NULL);
> +		for_each_rawref(mark_complete_oid, NULL);
>  		for_each_cached_alternate(NULL, mark_alternate_complete);
>  		commit_list_sort_by_date(&complete);
>  		if (cutoff)
> @@ -716,16 +729,12 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
>  	 */
>  	trace2_region_enter("fetch-pack", "mark_common_remote_refs", NULL);
>  	for (ref = *refs; ref; ref = ref->next) {
> -		struct object *o = deref_tag(the_repository,
> -					     lookup_object(the_repository,
> -					     &ref->old_oid),
> -					     NULL, 0);
> +		struct commit *c = deref_without_lazy_fetch(&ref->old_oid, 0);
>  
> -		if (!o || o->type != OBJ_COMMIT || !(o->flags & COMPLETE))
> +		if (!c || !(c->object.flags & COMPLETE))
>  			continue;
>  
> -		negotiator->known_common(negotiator,
> -					 (struct commit *)o);
> +		negotiator->known_common(negotiator, c);
>  	}
>  	trace2_region_leave("fetch-pack", "mark_common_remote_refs", NULL);
>  
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 8a27452a51..e53492d595 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -384,6 +384,26 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '
>  	grep "want $(cat hash)" trace
>  '
>  
> +test_expect_success 'fetch does not lazy-fetch missing targets of its refs' '
> +	rm -rf server client trace &&
> +
> +	test_create_repo server &&
> +	test_config -C server uploadpack.allowfilter 1 &&
> +	test_config -C server uploadpack.allowanysha1inwant 1 &&
> +	test_commit -C server foo &&
> +
> +	git clone --filter=blob:none "file://$(pwd)/server" client &&
> +	# Make all refs point to nothing by deleting all objects.
> +	rm client/.git/objects/pack/* &&
> +
> +	test_commit -C server bar &&
> +	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
> +		--no-tags --recurse-submodules=no \
> +		origin refs/tags/bar &&
> +	FOO_HASH=$(git -C server rev-parse foo) &&
> +	! grep "want $FOO_HASH" trace
> +'
> +
>  # The following two tests must be in this order. It is important that
>  # the srv.bare repository did not have tags during clone, but has tags
>  # in the fetch.

Patch
diff mbox series

diff --git a/fetch-pack.c b/fetch-pack.c
index 80fb3bd899..707bbc31fd 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -108,24 +108,48 @@  static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
 		cb(negotiator, cache.items[i]);
 }
 
+static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
+					       int mark_tags_complete)
+{
+	enum object_type type;
+	struct object_info info = { .typep = &type };
+
+	while (1) {
+		if (oid_object_info_extended(the_repository, oid, &info,
+					     OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))
+			return NULL;
+		if (type == OBJ_TAG) {
+			struct tag *tag = (struct tag *)
+				parse_object(the_repository, oid);
+
+			if (!tag->tagged)
+				return NULL;
+			if (mark_tags_complete)
+				tag->object.flags |= COMPLETE;
+			oid = &tag->tagged->oid;
+		} else {
+			break;
+		}
+	}
+	if (type == OBJ_COMMIT)
+		return (struct commit *) parse_object(the_repository, oid);
+	return NULL;
+}
+
 static int rev_list_insert_ref(struct fetch_negotiator *negotiator,
-			       const char *refname,
 			       const struct object_id *oid)
 {
-	struct object *o = deref_tag(the_repository,
-				     parse_object(the_repository, oid),
-				     refname, 0);
-
-	if (o && o->type == OBJ_COMMIT)
-		negotiator->add_tip(negotiator, (struct commit *)o);
+	struct commit *c = deref_without_lazy_fetch(oid, 0);
 
+	if (c)
+		negotiator->add_tip(negotiator, c);
 	return 0;
 }
 
 static int rev_list_insert_ref_oid(const char *refname, const struct object_id *oid,
 				   int flag, void *cb_data)
 {
-	return rev_list_insert_ref(cb_data, refname, oid);
+	return rev_list_insert_ref(cb_data, oid);
 }
 
 enum ack_type {
@@ -201,7 +225,7 @@  static void send_request(struct fetch_pack_args *args,
 static void insert_one_alternate_object(struct fetch_negotiator *negotiator,
 					struct object *obj)
 {
-	rev_list_insert_ref(negotiator, NULL, &obj->oid);
+	rev_list_insert_ref(negotiator, &obj->oid);
 }
 
 #define INITIAL_FLUSH 16
@@ -230,13 +254,12 @@  static void mark_tips(struct fetch_negotiator *negotiator,
 	int i;
 
 	if (!negotiation_tips) {
-		for_each_ref(rev_list_insert_ref_oid, negotiator);
+		for_each_rawref(rev_list_insert_ref_oid, negotiator);
 		return;
 	}
 
 	for (i = 0; i < negotiation_tips->nr; i++)
-		rev_list_insert_ref(negotiator, NULL,
-				    &negotiation_tips->oid[i]);
+		rev_list_insert_ref(negotiator, &negotiation_tips->oid[i]);
 	return;
 }
 
@@ -503,21 +526,11 @@  static struct commit_list *complete;
 
 static int mark_complete(const struct object_id *oid)
 {
-	struct object *o = parse_object(the_repository, oid);
-
-	while (o && o->type == OBJ_TAG) {
-		struct tag *t = (struct tag *) o;
-		if (!t->tagged)
-			break; /* broken repository */
-		o->flags |= COMPLETE;
-		o = parse_object(the_repository, &t->tagged->oid);
-	}
-	if (o && o->type == OBJ_COMMIT) {
-		struct commit *commit = (struct commit *)o;
-		if (!(commit->object.flags & COMPLETE)) {
-			commit->object.flags |= COMPLETE;
-			commit_list_insert(commit, &complete);
-		}
+	struct commit *commit = deref_without_lazy_fetch(oid, 1);
+
+	if (commit && !(commit->object.flags & COMPLETE)) {
+		commit->object.flags |= COMPLETE;
+		commit_list_insert(commit, &complete);
 	}
 	return 0;
 }
@@ -702,7 +715,7 @@  static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
 	 */
 	trace2_region_enter("fetch-pack", "mark_complete_local_refs", NULL);
 	if (!args->deepen) {
-		for_each_ref(mark_complete_oid, NULL);
+		for_each_rawref(mark_complete_oid, NULL);
 		for_each_cached_alternate(NULL, mark_alternate_complete);
 		commit_list_sort_by_date(&complete);
 		if (cutoff)
@@ -716,16 +729,12 @@  static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
 	 */
 	trace2_region_enter("fetch-pack", "mark_common_remote_refs", NULL);
 	for (ref = *refs; ref; ref = ref->next) {
-		struct object *o = deref_tag(the_repository,
-					     lookup_object(the_repository,
-					     &ref->old_oid),
-					     NULL, 0);
+		struct commit *c = deref_without_lazy_fetch(&ref->old_oid, 0);
 
-		if (!o || o->type != OBJ_COMMIT || !(o->flags & COMPLETE))
+		if (!c || !(c->object.flags & COMPLETE))
 			continue;
 
-		negotiator->known_common(negotiator,
-					 (struct commit *)o);
+		negotiator->known_common(negotiator, c);
 	}
 	trace2_region_leave("fetch-pack", "mark_common_remote_refs", NULL);
 
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 8a27452a51..e53492d595 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -384,6 +384,26 @@  test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '
 	grep "want $(cat hash)" trace
 '
 
+test_expect_success 'fetch does not lazy-fetch missing targets of its refs' '
+	rm -rf server client trace &&
+
+	test_create_repo server &&
+	test_config -C server uploadpack.allowfilter 1 &&
+	test_config -C server uploadpack.allowanysha1inwant 1 &&
+	test_commit -C server foo &&
+
+	git clone --filter=blob:none "file://$(pwd)/server" client &&
+	# Make all refs point to nothing by deleting all objects.
+	rm client/.git/objects/pack/* &&
+
+	test_commit -C server bar &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
+		--no-tags --recurse-submodules=no \
+		origin refs/tags/bar &&
+	FOO_HASH=$(git -C server rev-parse foo) &&
+	! grep "want $FOO_HASH" trace
+'
+
 # The following two tests must be in this order. It is important that
 # the srv.bare repository did not have tags during clone, but has tags
 # in the fetch.