[PATCH/RFC] upload-pack: fix slow fetches from a shallow repository

[PATCH/RFC] upload-pack: fix slow fetches from a shallow repository

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

Fetching from a shallow repository with a long history can hang. In a
homebrew-core clone with 900k commits, fetching one new commit into
another clone took 7 minutes. The same fetch over protocol v0 takes
about a second.

Over protocol v2, the server goes through all of its history to work
out which of its shallow commits to tell the client about, and a
history with many merges makes it go over the same commits again and
again. Protocol v0 lists all shallow commits and lets the client sort
them out.

Do the same in v2 when the client does not ask for a depth. This also
makes `git clone --reject-shallow` refuse a shallow repository over v2
in every case, as it already does over v0. For `git fetch --unshallow`,
which still needs to go through the history, visit each commit only
once.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
    upload-pack: fix slow fetches from a shallow repository
    
    Fix git fetch hanging for minutes when the remote is a shallow
    repository with a long history.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2455%2FHaraldNordgren%2Fupload-pack-shallow-walk-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2455/HaraldNordgren/upload-pack-shallow-walk-v1
Pull-Request: https://github.com/git/git/pull/2455

 shallow.c     | 3 ++-
 upload-pack.c | 5 ++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/shallow.c b/shallow.c
index 8e244a5669..921db5e17a 100644
--- a/shallow.c
+++ b/shallow.c
@@ -209,7 +209,8 @@ static struct commit_list *get_shallows_or_depth(struct object_array *heads,
 				*depth_slot = xmalloc(sizeof(int));
 				**depth_slot = cur_depth;
 			} else {
-				if (cur_depth >= **depth_slot)
+				if (depth == INFINITE_DEPTH ||
+				    cur_depth >= **depth_slot)
 					continue;
 				**depth_slot = cur_depth;
 			}
diff --git a/upload-pack.c b/upload-pack.c
index 197bcfa279..844bf80575 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1718,9 +1718,8 @@ static void send_shallow_info(struct upload_pack_data *data)
 
 	packet_writer_write(&data->writer, "shallow-info\n");
 
-	if (!send_shallow_list(data) &&
-	    is_repository_shallow(the_repository))
-		deepen(data, INFINITE_DEPTH);
+	if (!send_shallow_list(data))
+		advertise_shallow_grafts(1);
 
 	packet_delim(1);
 }

base-commit: c51408889c3b372814bc20b9431c3b74d0d268eb
-- 
gitgitgadget

Re: [PATCH/RFC] upload-pack: fix slow fetches from a shallow repository

[Junio C Hamano]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/upload-pack.c b/upload-pack.c
> index 197bcfa279..844bf80575 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1718,9 +1718,8 @@ static void send_shallow_info(struct upload_pack_data *data)
>  
>  	packet_writer_write(&data->writer, "shallow-info\n");
>  
> -	if (!send_shallow_list(data) &&
> -	    is_repository_shallow(the_repository))
> -		deepen(data, INFINITE_DEPTH);
> +	if (!send_shallow_list(data))
> +		advertise_shallow_grafts(1);

The advertise_shallow_grafts() function is written for protocol
v0 and uses packet_write_fmt() to write raw data directly to the
named file descriptor (in this case, fd #1).

However, protocol v2 negotiation uses the packet_writer API.  When
sideband-all is in use, the response must be multiplexed over the
sideband channel.  Because the updated code fails to use the
packet_writer API, 'shallow' lines are written without the '\001'
band designator.  The other side then interprets 's' (the first
byte of 'shallow', which is 115 in decimal) as the sideband channel
and dies with a protocol violation.

 t/t5702-protocol-v2.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git c/t/t5702-protocol-v2.sh w/t/t5702-protocol-v2.sh
index 7e69ba7f84..df3ec8c1c9 100755
--- c/t/t5702-protocol-v2.sh
+++ w/t/t5702-protocol-v2.sh
@@ -15,6 +15,18 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 start_git_daemon --export-all --enable=receive-pack
 daemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent
 
+test_expect_success 'fetch from shallow server over v2' '
+	test_when_finished "rm -fr server shallow-server client" &&
+	git init server &&
+	test_commit -C server one &&
+	test_commit -C server two &&
+	git clone --depth=1 "file://$(pwd)/server" shallow-server &&
+	test_commit -C shallow-server three &&
+
+	git -C shallow-server config uploadpack.allowsidebandall true &&
+	git -c protocol.version=2 clone "file://$(pwd)/shallow-server" client
+'
+
 test_expect_success 'create repo to be served by git-daemon' '
 	git init "$daemon_parent" &&
 	test_commit -C "$daemon_parent" one

Re: [PATCH/RFC] upload-pack: fix slow fetches from a shallow repository

[Harald Nordgren]

> The advertise_shallow_grafts() function is written for protocol
> v0 and uses packet_write_fmt() to write raw data directly to the
> named file descriptor (in this case, fd #1).
>
> However, protocol v2 negotiation uses the packet_writer API.  When
> sideband-all is in use, the response must be multiplexed over the
> sideband channel.  Because the updated code fails to use the
> packet_writer API, 'shallow' lines are written without the '\001'
> band designator.  The other side then interprets 's' (the first
> byte of 'shallow', which is 115 in decimal) as the sideband channel
> and dies with a protocol violation.

Thanks for pointing this out! I'll take a look!


Harald

[PATCH v2] upload-pack: fix slow fetches from a shallow repository

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

Fetching from a shallow repository with a long history can hang. In a
homebrew-core clone with 900k commits, fetching one new commit into
another clone took 7 minutes. The same fetch over protocol v0 takes
about a second.

Over protocol v2, the server goes through all of its history to work
out which of its shallow commits to tell the client about, and a
history with many merges makes it go over the same commits again and
again. Protocol v0 lists all shallow commits and lets the client sort
them out.

Do the same in v2 when the client does not ask for a depth. This also
makes `git clone --reject-shallow` refuse a shallow repository over v2
in every case, as it already does over v0. For `git fetch --unshallow`,
which still needs to go through the history, visit each commit only
once.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
    upload-pack: fix slow fetches from a shallow repository
    
    Fix git fetch hanging for minutes when the remote is a shallow
    repository with a long history.
    
    Changes in v2:
    
     * Fix fetches with sideband-all by writing the shallow lines through
       the packet writer. Add a test that clones from a shallow repository
       with uploadpack.allowSidebandAll set.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2455%2FHaraldNordgren%2Fupload-pack-shallow-walk-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2455/HaraldNordgren/upload-pack-shallow-walk-v2
Pull-Request: https://github.com/git/git/pull/2455

Range-diff vs v1:

 1:  4f4192120c ! 1:  8b0cb4c0e0 upload-pack: fix slow fetches from a shallow repository
     @@ shallow.c: static struct commit_list *get_shallows_or_depth(struct object_array
       				**depth_slot = cur_depth;
       			}
      
     + ## t/t5537-fetch-shallow.sh ##
     +@@ t/t5537-fetch-shallow.sh: test_expect_success 'clone from shallow clone' '
     + 	)
     + '
     + 
     ++test_expect_success 'clone from shallow clone with sideband-all' '
     ++	test_config -C shallow uploadpack.allowSidebandAll true &&
     ++	test_when_finished "rm -rf shallow-sideband-all" &&
     ++	git clone --no-local shallow shallow-sideband-all &&
     ++	(
     ++		cd shallow-sideband-all &&
     ++		git log --format=%s >actual &&
     ++		test_write_lines 4 3 >expect &&
     ++		test_cmp expect actual
     ++	)
     ++'
     ++
     + test_expect_success 'fetch from shallow clone' '
     + 	(
     + 	cd shallow &&
     +
       ## upload-pack.c ##
     -@@ upload-pack.c: static void send_shallow_info(struct upload_pack_data *data)
     +@@ upload-pack.c: static void send_wanted_ref_info(struct upload_pack_data *data)
     + 	packet_writer_delim(&data->writer);
     + }
       
     - 	packet_writer_write(&data->writer, "shallow-info\n");
     ++static int send_shallow_graft(const struct commit_graft *graft, void *cb_data)
     ++{
     ++	struct packet_writer *writer = cb_data;
     ++
     ++	if (graft->nr_parent == -1)
     ++		packet_writer_write(writer, "shallow %s\n",
     ++				    oid_to_hex(&graft->oid));
     ++	return 0;
     ++}
     ++
     + static void send_shallow_info(struct upload_pack_data *data)
     + {
     + 	/* No shallow info needs to be sent */
     +@@ upload-pack.c: static void send_shallow_info(struct upload_pack_data *data)
       
     --	if (!send_shallow_list(data) &&
     --	    is_repository_shallow(the_repository))
     + 	if (!send_shallow_list(data) &&
     + 	    is_repository_shallow(the_repository))
      -		deepen(data, INFINITE_DEPTH);
     -+	if (!send_shallow_list(data))
     -+		advertise_shallow_grafts(1);
     ++		for_each_commit_graft(send_shallow_graft, &data->writer);
       
       	packet_delim(1);
       }


 shallow.c                |  3 ++-
 t/t5537-fetch-shallow.sh | 12 ++++++++++++
 upload-pack.c            | 12 +++++++++++-
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/shallow.c b/shallow.c
index 8e244a5669..921db5e17a 100644
--- a/shallow.c
+++ b/shallow.c
@@ -209,7 +209,8 @@ static struct commit_list *get_shallows_or_depth(struct object_array *heads,
 				*depth_slot = xmalloc(sizeof(int));
 				**depth_slot = cur_depth;
 			} else {
-				if (cur_depth >= **depth_slot)
+				if (depth == INFINITE_DEPTH ||
+				    cur_depth >= **depth_slot)
 					continue;
 				**depth_slot = cur_depth;
 			}
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index f323ceebd2..140d5c2dba 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -43,6 +43,18 @@ test_expect_success 'clone from shallow clone' '
 	)
 '
 
+test_expect_success 'clone from shallow clone with sideband-all' '
+	test_config -C shallow uploadpack.allowSidebandAll true &&
+	test_when_finished "rm -rf shallow-sideband-all" &&
+	git clone --no-local shallow shallow-sideband-all &&
+	(
+		cd shallow-sideband-all &&
+		git log --format=%s >actual &&
+		test_write_lines 4 3 >expect &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'fetch from shallow clone' '
 	(
 	cd shallow &&
diff --git a/upload-pack.c b/upload-pack.c
index 197bcfa279..e6dc19526c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1709,6 +1709,16 @@ static void send_wanted_ref_info(struct upload_pack_data *data)
 	packet_writer_delim(&data->writer);
 }
 
+static int send_shallow_graft(const struct commit_graft *graft, void *cb_data)
+{
+	struct packet_writer *writer = cb_data;
+
+	if (graft->nr_parent == -1)
+		packet_writer_write(writer, "shallow %s\n",
+				    oid_to_hex(&graft->oid));
+	return 0;
+}
+
 static void send_shallow_info(struct upload_pack_data *data)
 {
 	/* No shallow info needs to be sent */
@@ -1720,7 +1730,7 @@ static void send_shallow_info(struct upload_pack_data *data)
 
 	if (!send_shallow_list(data) &&
 	    is_repository_shallow(the_repository))
-		deepen(data, INFINITE_DEPTH);
+		for_each_commit_graft(send_shallow_graft, &data->writer);
 
 	packet_delim(1);
 }

base-commit: c51408889c3b372814bc20b9431c3b74d0d268eb
-- 
gitgitgadget