[PATCH 4/4] git-fetch-pack: Implement client part of the multi_ack extension

[PATCH 4/4] git-fetch-pack: Implement client part of the multi_ack extension

[Johannes Schindelin]

This patch concludes the series, which makes 
git-fetch-pack/git-upload-pack negotiate a potentially better set of 
common revs. It should make a difference when fetching from a repository 
with a few branches.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

 connect.c    |    5 ++++-
 fetch-pack.c |   50 +++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 41 insertions(+), 14 deletions(-)

applies-to: 6b4b7d9acf60aa99d961b599f37d0c824be79e27
9adb6b3971e7daa79221d7dbe05b66327b266b86
diff --git a/connect.c b/connect.c
index b171c5d..57e25a3 100644
--- a/connect.c
+++ b/connect.c
@@ -59,8 +59,11 @@ int get_ack(int fd, unsigned char *resul
 	if (!strcmp(line, "NAK"))
 		return 0;
 	if (!strncmp(line, "ACK ", 3)) {
-		if (!get_sha1_hex(line+4, result_sha1))
+		if (!get_sha1_hex(line+4, result_sha1)) {
+			if (strstr(line+45, "continue"))
+				return 2;
 			return 1;
+		}
 	}
 	die("git-fetch_pack: expected ACK/NAK, got '%s'", line);
 }
diff --git a/fetch-pack.c b/fetch-pack.c
index 3a903c4..57602b9 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -125,7 +125,7 @@ static int find_common(int fd[2], unsign
 		       struct ref *refs)
 {
 	int fetching;
-	int count = 0, flushes = 0, retval;
+	int count = 0, flushes = 0, multi_ack = 0, retval;
 	const unsigned char *sha1;
 
 	for_each_ref(rev_list_append_sha1);
@@ -156,20 +156,22 @@ static int find_common(int fd[2], unsign
 			continue;
 		}
 
-		packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
+		packet_write(fd[1], "want %s multi_ack\n", sha1_to_hex(remote));
 		fetching++;
 	}
 	packet_flush(fd[1]);
 	if (!fetching)
 		return 1;
 
-	flushes = 1;
+	flushes = 0;
 	retval = -1;
 	while ((sha1 = get_rev())) {
 		packet_write(fd[1], "have %s\n", sha1_to_hex(sha1));
 		if (verbose)
 			fprintf(stderr, "have %s\n", sha1_to_hex(sha1));
 		if (!(31 & ++count)) {
+			int ack;
+
 			packet_flush(fd[1]);
 			flushes++;
 
@@ -179,26 +181,48 @@ static int find_common(int fd[2], unsign
 			 */
 			if (count == 32)
 				continue;
-			if (get_ack(fd[0], result_sha1)) {
-				flushes = 0;
-				retval = 0;
-				if (verbose)
-					fprintf(stderr, "got ack\n");
-				break;
-			}
+
+			do {
+				ack = get_ack(fd[0], result_sha1);
+				if (verbose && ack)
+					fprintf(stderr, "got ack %d %s\n", ack,
+							sha1_to_hex(result_sha1));
+				if (ack == 1) {
+					if (!multi_ack)
+						flushes = 0;
+					retval = 0;
+					goto done;
+				} else if (ack == 2) {
+					multi_ack = 1;
+					mark_common((struct commit *)
+							lookup_object(result_sha1));
+					retval = 0;
+				}
+			} while(ack);
 			flushes--;
 		}
 	}
+done:
+	if (multi_ack) {
+		packet_flush(fd[1]);
+		flushes++;
+	}
 	packet_write(fd[1], "done\n");
 	if (verbose)
 		fprintf(stderr, "done\n");
+	if (retval != 0)
+		flushes++;
 	while (flushes) {
-		flushes--;
 		if (get_ack(fd[0], result_sha1)) {
 			if (verbose)
-				fprintf(stderr, "got ack\n");
-			return 0;
+				fprintf(stderr, "got ack %s\n",
+					sha1_to_hex(result_sha1));
+			if (!multi_ack)
+				return 0;
+			retval = 0;
+			continue;
 		}
+		flushes--;
 	}
 	return retval;
 }
---
0.99.8.GIT

[PATCH] fetch/upload: Fix corner case with few revs

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2382 bytes --]

When git-fetch-pack did not have enough revs to send, it did not realize 
that the server actually speaks multi_ack. The server would now continue 
sending ack´s, but the client would try to unpack objects. Oops.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

	I have a sizable collection of brown paper bags by now.

 fetch-pack.c  |   13 +++++++++----
 upload-pack.c |   15 +++++++++++----
 2 files changed, 20 insertions(+), 8 deletions(-)

applies-to: f4786932e8753bdd07e44829a97a47749b329ee8
9a0ea94256236f1d038b16eb834fdfa5987f308c
diff --git a/fetch-pack.c b/fetch-pack.c
index 7015dc5..b02a24a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -224,12 +224,17 @@ done:
 	if (retval != 0)
 		flushes++;
 	while (flushes) {
-		if (get_ack(fd[0], result_sha1)) {
+		int ack = get_ack(fd[0], result_sha1);
+		if (ack) {
 			if (verbose)
-				fprintf(stderr, "got ack %s\n",
+				fprintf(stderr, "got ack (%d) %s\n", ack,
 					sha1_to_hex(result_sha1));
-			if (!multi_ack)
-				return 0;
+			if (!multi_ack) {
+				if (ack == 2)
+					multi_ack = 1;
+				else
+					return 0;
+			}
 			retval = 0;
 			continue;
 		}
diff --git a/upload-pack.c b/upload-pack.c
index 25a343e..1dbde5f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -116,7 +116,7 @@ static int get_common_commits(void)
 {
 	static char line[1000];
 	unsigned char sha1[20];
-	int len;
+	int len, last_sent_was_nak = 0;
 
 	track_object_refs = 0;
 	save_commit_buffer = 0;
@@ -126,23 +126,30 @@ static int get_common_commits(void)
 		reset_timeout();
 
 		if (!len) {
-			if (multi_ack || nr_has == 0)
+			if (multi_ack || nr_has == 0) {
 				packet_write(1, "NAK\n");
+				last_sent_was_nak = 1;
+			}
 			continue;
 		}
 		len = strip(line, len);
 		if (!strncmp(line, "have ", 5)) {
 			if (got_sha1(line+5, sha1) &&
-					(multi_ack || nr_has == 1))
+					(multi_ack || nr_has == 1)) {
 				packet_write(1, "ACK %s%s\n",
 					sha1_to_hex(sha1),
 					multi_ack && nr_has < MAX_HAS ?
 					" continue" : "");
+				last_sent_was_nak = 0;
+			}
 			continue;
 		}
 		if (!strcmp(line, "done")) {
-			if (nr_has > 0)
+			if (nr_has > 0) {
+				if (multi_ack && !last_sent_was_nak)
+					packet_write(1, "NAK\n");
 				return 0;
+			}
 			packet_write(1, "NAK\n");
 			return -1;
 		}
---
0.99.8.GIT

Re: [PATCH] fetch/upload: Fix corner case with few revs

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> When git-fetch-pack did not have enough revs to send, it did not realize 
> that the server actually speaks multi_ack. The server would now continue 
> sending ack', but the client would try to unpack objects. Oops.

I've already pushed your initial set out to "master", but I
suspect we may be better of if I recall them and let it simmer a
bit longer in the proposed updates branch, and defer them post
0.99.9.  What do you think?

Re: [PATCH 4/4] git-fetch-pack: Implement client part of the multi_ack extension

[Alex Riesen]

Johannes Schindelin, Sun, Oct 23, 2005 03:40:13 +0200:
> This patch concludes the series, which makes 
> git-fetch-pack/git-upload-pack negotiate a potentially better set of 
> common revs. It should make a difference when fetching from a repository 
> with a few branches.

This broke git-pull for me (the local one):

    /d/e/f.git$ git-pull
    fatal: bad pack file
    fatal: git-unpack-objects died with error code 128
    Fetch failure: /a/b/c/.git

> applies-to: 6b4b7d9acf60aa99d961b599f37d0c824be79e27
> 9adb6b3971e7daa79221d7dbe05b66327b266b86
...
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 3a903c4..57602b9 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c

Reverting just fetch-pack.c part of the patch helps.

Re: [PATCH] fetch/upload: Fix corner case with few revs

[Johannes Schindelin]

Hi,

On Tue, 25 Oct 2005, Junio C Hamano wrote:

> I've already pushed your initial set out to "master", but I
> suspect we may be better of if I recall them and let it simmer a
> bit longer in the proposed updates branch, and defer them post
> 0.99.9.  What do you think?

Yes, please. Sorry for the problems.

Ciao,
Dscho

Re: [PATCH 4/4] git-fetch-pack: Implement client part of the multi_ack extension

[Johannes Schindelin]

Hi,

On Tue, 25 Oct 2005, Alex Riesen wrote:

> Johannes Schindelin, Sun, Oct 23, 2005 03:40:13 +0200:
> > This patch concludes the series, which makes 
> > git-fetch-pack/git-upload-pack negotiate a potentially better set of 
> > common revs. It should make a difference when fetching from a repository 
> > with a few branches.
> 
> This broke git-pull for me (the local one):
> 
>     /d/e/f.git$ git-pull
>     fatal: bad pack file
>     fatal: git-unpack-objects died with error code 128
>     Fetch failure: /a/b/c/.git
> 
> > applies-to: 6b4b7d9acf60aa99d961b599f37d0c824be79e27
> > 9adb6b3971e7daa79221d7dbe05b66327b266b86
> ...
> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index 3a903c4..57602b9 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> 
> Reverting just fetch-pack.c part of the patch helps.

Could you please try the patch I sent with the subject "[PATCH] 
fetch/upload: Fix corner case with few revs"? Your output looks exactly 
like what I fixed with that patch.

Ciao,
Dscho

Re: [PATCH 4/4] git-fetch-pack: Implement client part of the multi_ack extension

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Johannes Schindelin, Sun, Oct 23, 2005 03:40:13 +0200:
>> This patch concludes the series, which makes 
>> git-fetch-pack/git-upload-pack negotiate a potentially better set of 
>> common revs. It should make a difference when fetching from a repository 
>> with a few branches.
>
> This broke git-pull for me (the local one):

Is this the same problem I fixed with this commit, which sits at
the tip on the "master" branch?

commit 7efc8e43508b415e2540dbcb79521bde16c51e0c
tree 6234aa4f7095054a137e030030f914dc6633f809
parent 40a10462498bdd23d4e49f02867b8be50eb78704
author Junio C Hamano <junkio@cox.net> 1130061738 -0700
committer Junio C Hamano <junkio@cox.net> 1130192018 -0700

    upload-pack: fix thinko in common-commit finder code.

    The code to check if we have the object the other side has was bogus
    (my fault).

    Signed-off-by: Junio C Hamano <junkio@cox.net>

Re: [PATCH 4/4] git-fetch-pack: Implement client part of the multi_ack extension

[Johannes Schindelin]

Hi,

On Tue, 25 Oct 2005, Junio C Hamano wrote:

> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > Johannes Schindelin, Sun, Oct 23, 2005 03:40:13 +0200:
> >> This patch concludes the series, which makes 
> >> git-fetch-pack/git-upload-pack negotiate a potentially better set of 
> >> common revs. It should make a difference when fetching from a repository 
> >> with a few branches.
> >
> > This broke git-pull for me (the local one):
> 
> Is this the same problem I fixed with this commit, which sits at
> the tip on the "master" branch?
> 
> commit 7efc8e43508b415e2540dbcb79521bde16c51e0c
> tree 6234aa4f7095054a137e030030f914dc6633f809
> parent 40a10462498bdd23d4e49f02867b8be50eb78704
> author Junio C Hamano <junkio@cox.net> 1130061738 -0700
> committer Junio C Hamano <junkio@cox.net> 1130192018 -0700
> 
>     upload-pack: fix thinko in common-commit finder code.
> 
>     The code to check if we have the object the other side has was bogus
>     (my fault).
> 
>     Signed-off-by: Junio C Hamano <junkio@cox.net>

It looks to me like it is not. This looks more like upload-pack speaks 
multi_ack, but fetch-pack does not, since it gets the "got ack .. 
continue" too late.

I'll send out a patch implementing a first version of my "nasty" 
fetch-pack tests in a few minutes. This test showed me my error.

Ciao,
Dscho

Re: [PATCH 4/4] git-fetch-pack: Implement client part of the multi_ack extension

[Alex Riesen]

On 10/25/05, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > This patch concludes the series, which makes
> > > git-fetch-pack/git-upload-pack negotiate a potentially better set of
> > > common revs. It should make a difference when fetching from a repository
> > > with a few branches.
> >
> > This broke git-pull for me (the local one):
> >
> >     /d/e/f.git$ git-pull
> >     fatal: bad pack file
> >     fatal: git-unpack-objects died with error code 128
> >     Fetch failure: /a/b/c/.git
> >
> > > applies-to: 6b4b7d9acf60aa99d961b599f37d0c824be79e27
> > > 9adb6b3971e7daa79221d7dbe05b66327b266b86
> > ...
> > > diff --git a/fetch-pack.c b/fetch-pack.c
> > > index 3a903c4..57602b9 100644
> > > --- a/fetch-pack.c
> > > +++ b/fetch-pack.c
> >
> > Reverting just fetch-pack.c part of the patch helps.
>
> Could you please try the patch I sent with the subject "[PATCH]
> fetch/upload: Fix corner case with few revs"? Your output looks exactly
> like what I fixed with that patch.
>

I couldn't at the moment. Do you still need a test?

Re: [PATCH 4/4] git-fetch-pack: Implement client part of the multi_ack extension

[Johannes Schindelin]

Hi,

On Wed, 26 Oct 2005, Alex Riesen wrote:

> > Could you please try the patch I sent with the subject "[PATCH]
> > fetch/upload: Fix corner case with few revs"? Your output looks exactly
> > like what I fixed with that patch.
> >
> 
> I couldn't at the moment. Do you still need a test?

If you have time and can test it, yes, please.

Ciao,
Dscho

Re: [PATCH 4/4] git-fetch-pack: Implement client part of the multi_ack extension

[Alex Riesen]

On 10/26/05, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > Could you please try the patch I sent with the subject "[PATCH]
> > > fetch/upload: Fix corner case with few revs"? Your output looks exactly
> > > like what I fixed with that patch.
> >
> > I couldn't at the moment. Do you still need a test?
>
> If you have time and can test it, yes, please.
>

Will try. Which patch is it?

Re: [PATCH 4/4] git-fetch-pack: Implement client part of the multi_ack extension

[Alex Riesen]

Johannes Schindelin, Wed, Oct 26, 2005 10:41:47 +0200:
> > > Could you please try the patch I sent with the subject "[PATCH]
> > > fetch/upload: Fix corner case with few revs"? Your output looks exactly
> > > like what I fixed with that patch.
> > I couldn't at the moment. Do you still need a test?
> If you have time and can test it, yes, please.

Johannes Schindelin, Tue, Oct 25, 2005 17:34:07 +0200:
> When git-fetch-pack did not have enough revs to send, it did not realize 
> that the server actually speaks multi_ack. The server would now continue 
> sending ack´s, but the client would try to unpack objects. Oops.

This patch fixed it.