diff mbox series

[v2] upload-pack: do not lazy-fetch "have" objects

Message ID 20200716180950.2852753-1-jonathantanmy@google.com (mailing list archive)
State New, archived
Headers show
Series [v2] upload-pack: do not lazy-fetch "have" objects | expand

Commit Message

Jonathan Tan July 16, 2020, 6:09 p.m. UTC
When upload-pack receives a request containing "have" hashes, it (among
other things) checks if the served repository has the corresponding
objects. However, it does not do so with the
OBJECT_INFO_SKIP_FETCH_OBJECT flag, so if serving a partial clone, a
lazy fetch will be triggered first.

This was discovered at $DAYJOB when a user fetched from a partial clone
(into another partial clone - although this would also happen if the
repo to be fetched into is not a partial clone).

Therefore, whenever "have" hashes are checked for existence, pass the
OBJECT_INFO_SKIP_FETCH_OBJECT flag. Also add the OBJECT_INFO_QUICK flag
to improve performance, as it is typical that such objects do not exist
in the serving repo, and the consequences of a false negative are minor
(usually, a slightly larger pack sent).

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Differences from v1: OBJECT_INFO_QUICK is also used wherever
OBJECT_INFO_SKIP_FETCH_OBJECT is added.

Using OBJECT_INFO_QUICK makes sense to me, so here's an updated patch.
---
 t/t5616-partial-clone.sh | 38 ++++++++++++++++++++++++++++++++++++++
 upload-pack.c            |  6 ++++--
 2 files changed, 42 insertions(+), 2 deletions(-)

Comments

Jeff King July 20, 2020, 5:42 p.m. UTC | #1
On Thu, Jul 16, 2020 at 11:09:50AM -0700, Jonathan Tan wrote:

> Differences from v1: OBJECT_INFO_QUICK is also used wherever
> OBJECT_INFO_SKIP_FETCH_OBJECT is added.
> 
> Using OBJECT_INFO_QUICK makes sense to me, so here's an updated patch.

Yeah, I think this is reasonable, and the patch looks good to me.

-Peff
diff mbox series

Patch

diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 8a27452a51..37de0afb02 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -422,6 +422,44 @@  test_expect_success 'single-branch tag following respects partial clone' '
 	test_must_fail git -C single rev-parse --verify refs/tags/C
 '
 
+test_expect_success 'fetch from a partial clone, protocol v0' '
+	rm -rf server client trace &&
+
+	# Pretend that the server is a partial clone
+	git init server &&
+	git -C server remote add a_remote "file://$(pwd)/" &&
+	test_config -C server core.repositoryformatversion 1 &&
+	test_config -C server extensions.partialclone a_remote &&
+	test_config -C server protocol.version 0 &&
+	test_commit -C server foo &&
+
+	# Fetch from the server
+	git init client &&
+	test_config -C client protocol.version 0 &&
+	test_commit -C client bar &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "file://$(pwd)/server" &&
+	! grep "version 2" trace
+'
+
+test_expect_success 'fetch from a partial clone, protocol v2' '
+	rm -rf server client trace &&
+
+	# Pretend that the server is a partial clone
+	git init server &&
+	git -C server remote add a_remote "file://$(pwd)/" &&
+	test_config -C server core.repositoryformatversion 1 &&
+	test_config -C server extensions.partialclone a_remote &&
+	test_config -C server protocol.version 2 &&
+	test_commit -C server foo &&
+
+	# Fetch from the server
+	git init client &&
+	test_config -C client protocol.version 2 &&
+	test_commit -C client bar &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "file://$(pwd)/server" &&
+	grep "version 2" trace
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
diff --git a/upload-pack.c b/upload-pack.c
index 951a2b23aa..8673741070 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -482,7 +482,8 @@  static int got_oid(struct upload_pack_data *data,
 {
 	if (get_oid_hex(hex, oid))
 		die("git upload-pack: expected SHA1 object, got '%s'", hex);
-	if (!has_object_file(oid))
+	if (!has_object_file_with_flags(oid,
+					OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT))
 		return -1;
 	return do_got_oid(data, oid);
 }
@@ -1423,7 +1424,8 @@  static int process_haves(struct upload_pack_data *data, struct oid_array *common
 	for (i = 0; i < data->haves.nr; i++) {
 		const struct object_id *oid = &data->haves.oid[i];
 
-		if (!has_object_file(oid))
+		if (!has_object_file_with_flags(oid,
+						OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT))
 			continue;
 
 		oid_array_append(common, oid);