[PATCH] fetch-pack should not ask for a ref which is already there

[PATCH] fetch-pack should not ask for a ref which is already there

[Johannes Schindelin]

With this patch, instead of blindly asking for every remote ref, fetch-pack
first looks in the local repository if that ref is already there.

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

---

 fetch-pack.c |   30 ++++++++++++++++++++----------
 1 files changed, 20 insertions(+), 10 deletions(-)

6bf41421bed0d640677c5233d2be6813b2211979
diff --git a/fetch-pack.c b/fetch-pack.c
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -16,20 +16,27 @@ static int find_common(int fd[2], unsign
 	int count = 0, flushes = 0, retval;
 	FILE *revs;
 
-	revs = popen("git-rev-list $(git-rev-parse --all)", "r");
-	if (!revs)
-		die("unable to run 'git-rev-list'");
-
 	while (refs) {
 		unsigned char *remote = refs->old_sha1;
-		if (verbose)
-			fprintf(stderr,
-				"want %s (%s)\n", sha1_to_hex(remote),
-				refs->name);
-		packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
+		if(!has_sha1_file(remote)) {
+			if (verbose)
+				fprintf(stderr,
+					"want %s (%s)\n", sha1_to_hex(remote),
+					refs->name);
+			packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
+			count++;
+		}
 		refs = refs->next;
 	}
 	packet_flush(fd[1]);
+
+	if(count==0)
+		return 1;
+
+	revs = popen("git-rev-list $(git-rev-parse --all)", "r");
+	if (!revs)
+		die("unable to run 'git-rev-list'");
+
 	flushes = 1;
 	retval = -1;
 	while (fgets(line, sizeof(line), revs) != NULL) {
@@ -86,7 +93,10 @@ static int fetch_pack(int fd[2], int nr_
 		packet_flush(fd[1]);
 		die("no matching remote head");
 	}
-	if (find_common(fd, sha1, ref) < 0)
+	status = find_common(fd, sha1, ref);
+	if(status > 0)
+		return 0;
+	if(status < 0)
 		fprintf(stderr, "warning: no common commits\n");
 	pid = fork();
 	if (pid < 0)

Re: [PATCH] fetch-pack should not ask for a ref which is already there

[Junio C Hamano]

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

> With this patch, instead of blindly asking for every remote ref, fetch-pack
> first looks in the local repository if that ref is already there.

This patch breaks things.  git-fetch-pack is supposed to report
the resulting SHA1 and refs on its standard output.  If you are
up to date with this patch you would lose that.

Re: [PATCH] fetch-pack should not ask for a ref which is already there

[Johannes Schindelin]

Hi,

On Thu, 29 Sep 2005, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > With this patch, instead of blindly asking for every remote ref, fetch-pack
> > first looks in the local repository if that ref is already there.
> 
> This patch breaks things.  git-fetch-pack is supposed to report
> the resulting SHA1 and refs on its standard output.  If you are
> up to date with this patch you would lose that.

Yeah, sorry. I did not test that one. Maybe an automated test for 
git-fetch-pack would be useful after all...

How about the following? (I avoided an ugly goto, but had to duplicate 
code :-( )

---

[PATCH] fetch-pack should not ask for a ref which is already there

With this patch, instead of blindly asking for every remote ref, fetch-pack
first looks in the local repository if that ref is already there.

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

---

 fetch-pack.c |   36 ++++++++++++++++++++++++++----------
 1 files changed, 26 insertions(+), 10 deletions(-)

fbef982e4c5c4591b8d5517ef1694ab71e29c9e5
diff --git a/fetch-pack.c b/fetch-pack.c
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -16,20 +16,27 @@ static int find_common(int fd[2], unsign
 	int count = 0, flushes = 0, retval;
 	FILE *revs;
 
-	revs = popen("git-rev-list $(git-rev-parse --all)", "r");
-	if (!revs)
-		die("unable to run 'git-rev-list'");
-
 	while (refs) {
 		unsigned char *remote = refs->old_sha1;
-		if (verbose)
-			fprintf(stderr,
-				"want %s (%s)\n", sha1_to_hex(remote),
-				refs->name);
-		packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
+		if(!has_sha1_file(remote)) {
+			if (verbose)
+				fprintf(stderr,
+					"want %s (%s)\n", sha1_to_hex(remote),
+					refs->name);
+			packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
+			count++;
+		}
 		refs = refs->next;
 	}
 	packet_flush(fd[1]);
+
+	if(count==0)
+		return 1;
+
+	revs = popen("git-rev-list $(git-rev-parse --all)", "r");
+	if (!revs)
+		die("unable to run 'git-rev-list'");
+
 	flushes = 1;
 	retval = -1;
 	while (fgets(line, sizeof(line), revs) != NULL) {
@@ -86,7 +93,16 @@ static int fetch_pack(int fd[2], int nr_
 		packet_flush(fd[1]);
 		die("no matching remote head");
 	}
-	if (find_common(fd, sha1, ref) < 0)
+	status = find_common(fd, sha1, ref);
+	if(status > 0) {
+		while (ref) {
+			printf("%s %s\n",
+			       sha1_to_hex(ref->old_sha1), ref->name);
+			ref = ref->next;
+		}
+		return 0;
+	}
+	if(status < 0)
 		fprintf(stderr, "warning: no common commits\n");
 	pid = fork();
 	if (pid < 0)

Re: [PATCH] fetch-pack should not ask for a ref which is already there

[Junio C Hamano]

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

> Yeah, sorry. I did not test that one. Maybe an automated test for 
> git-fetch-pack would be useful after all...

Sounds sensible.  I think git-send-pack has one.

> How about the following? (I avoided an ugly goto, but had to duplicate 
> code :-( )

I suspect it would be cleaner to return (struct ref *) from
fetch_pack (ideally use NULL to signal failure but there is no
need -- it just dies) and print the refs from main() only after
seeing the last close and finish_connect succeed.

Re: [PATCH] fetch-pack should not ask for a ref which is already there

[Johannes Schindelin]

Hi,

On Thu, 29 Sep 2005, Junio C Hamano wrote:

> I suspect it would be cleaner to return (struct ref *) from
> fetch_pack (ideally use NULL to signal failure but there is no
> need -- it just dies) and print the refs from main() only after
> seeing the last close and finish_connect succeed.

Here's my next try:

[PATCH] fetch-pack should not ask for a ref which is already there
From: Johannes Schindelin <gene099@localhost>
Date: 1127950437 +0200

With this patch, instead of blindly asking for every remote ref, fetch-pack
first looks in the local repository if that ref is already there.

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

---

 fetch-pack.c |   56 ++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 36 insertions(+), 20 deletions(-)

210c4c6fe4a353d59e4ca2871d7f2a3c1b9b1238
diff --git a/fetch-pack.c b/fetch-pack.c
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -16,20 +16,27 @@ static int find_common(int fd[2], unsign
 	int count = 0, flushes = 0, retval;
 	FILE *revs;
 
-	revs = popen("git-rev-list $(git-rev-parse --all)", "r");
-	if (!revs)
-		die("unable to run 'git-rev-list'");
-
 	while (refs) {
 		unsigned char *remote = refs->old_sha1;
-		if (verbose)
-			fprintf(stderr,
-				"want %s (%s)\n", sha1_to_hex(remote),
-				refs->name);
-		packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
+		if(!has_sha1_file(remote)) {
+			if (verbose)
+				fprintf(stderr,
+					"want %s (%s)\n", sha1_to_hex(remote),
+					refs->name);
+			packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
+			count++;
+		}
 		refs = refs->next;
 	}
 	packet_flush(fd[1]);
+
+	if(count==0)
+		return 1;
+
+	revs = popen("git-rev-list $(git-rev-parse --all)", "r");
+	if (!revs)
+		die("unable to run 'git-rev-list'");
+
 	flushes = 1;
 	retval = -1;
 	while (fgets(line, sizeof(line), revs) != NULL) {
@@ -74,7 +81,7 @@ static int find_common(int fd[2], unsign
 	return retval;
 }
 
-static int fetch_pack(int fd[2], int nr_match, char **match)
+static struct ref *fetch_pack(int fd[2], int nr_match, char **match)
 {
 	struct ref *ref;
 	unsigned char sha1[20];
@@ -86,7 +93,10 @@ static int fetch_pack(int fd[2], int nr_
 		packet_flush(fd[1]);
 		die("no matching remote head");
 	}
-	if (find_common(fd, sha1, ref) < 0)
+	status = find_common(fd, sha1, ref);
+	if(status > 0)
+		return ref;
+	if(status < 0)
 		fprintf(stderr, "warning: no common commits\n");
 	pid = fork();
 	if (pid < 0)
@@ -109,12 +119,7 @@ static int fetch_pack(int fd[2], int nr_
 		int code = WEXITSTATUS(status);
 		if (code)
 			die("git-unpack-objects died with error code %d", code);
-		while (ref) {
-			printf("%s %s\n",
-			       sha1_to_hex(ref->old_sha1), ref->name);
-			ref = ref->next;
-		}
-		return 0;
+		return ref;
 	}
 	if (WIFSIGNALED(status)) {
 		int sig = WTERMSIG(status);
@@ -125,10 +130,11 @@ static int fetch_pack(int fd[2], int nr_
 
 int main(int argc, char **argv)
 {
-	int i, ret, nr_heads;
+	int i, nr_heads;
 	char *dest = NULL, **heads;
 	int fd[2];
 	pid_t pid;
+	struct ref *ref;
 
 	nr_heads = 0;
 	heads = NULL;
@@ -160,9 +166,19 @@ int main(int argc, char **argv)
 	pid = git_connect(fd, dest, exec);
 	if (pid < 0)
 		return 1;
-	ret = fetch_pack(fd, nr_heads, heads);
+	ref = fetch_pack(fd, nr_heads, heads);
+	if(!ref)
+		return 1;
+
+	while (ref) {
+		printf("%s %s\n",
+		       sha1_to_hex(ref->old_sha1), ref->name);
+		ref = ref->next;
+	}
+
 	close(fd[0]);
 	close(fd[1]);
 	finish_connect(pid);
-	return ret;
+
+	return 0;
 }

Re: [PATCH] fetch-pack should not ask for a ref which is already there

[Linus Torvalds]

On Thu, 29 Sep 2005, Johannes Schindelin wrote:
>
> With this patch, instead of blindly asking for every remote ref, fetch-pack
> first looks in the local repository if that ref is already there.

No. This is WRONG.

It may seem like a sane thing to do, but it is very very horribly broken.

The fact is, if an earlier fetch was interrupted, or if you've used things
like rsync, you may have disconnected objects in your object store. The 
fact that you have a particular commit object is _not_ a guarantee that 
you have everything that leads up to it.

The "fetch" semantics are simple: we only write out new refs _after_ we've 
fetched all the objects that point to them. That means that while we 
cannot trust a "oh, I already have this commit, let's skip it", we _can_ 
trust "oh, I already have these refs, let's skip them".

So please do _not_ add logic like this to git-fetch. I'd _much_ rather
fetch some objects twice than end up with a corrupt repository.

		Linus

Re: [PATCH] fetch-pack should not ask for a ref which is already there

[Johannes Schindelin]

Hi,

On Thu, 29 Sep 2005, Linus Torvalds wrote:

> On Thu, 29 Sep 2005, Johannes Schindelin wrote:
> >
> > With this patch, instead of blindly asking for every remote ref, fetch-pack
> > first looks in the local repository if that ref is already there.
> 
> No. This is WRONG.

Actually, this is not wrong. The patch is wrong. Here's why:

> The "fetch" semantics are simple: we only write out new refs _after_ we've 
> fetched all the objects that point to them. That means that while we 
> cannot trust a "oh, I already have this commit, let's skip it", we _can_ 
> trust "oh, I already have these refs, let's skip them".

What the commit message should suggest was: if a local ref is identical to 
the remote ref, do not "want" it. But of course, I got it all wrong. BTW 
this patch is not to annoy you, but to ease the burden on the server for 
git-daemon. (Many a developer begins the day by fetching the latest and 
greatest).

The correct way to go about it would be to check that

$(git-rev-list <remote_ref> $(git-rev-parse --all | sed "s/^/^/"))

is empty. However, I am lazy, and in most cases, it is sufficient to check 
if the remote ref is identical to a local ref. So here goes another try:

[PATCH] fetch-pack should not ask for a ref which is already there

With this patch, instead of blindly asking for every remote ref, fetch-pack
first looks in the local repository if that ref is already a local ref.

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

---

 fetch-pack.c |   72 ++++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 52 insertions(+), 20 deletions(-)

edfbae3927434270c35dfcad417e7d202e509e90
diff --git a/fetch-pack.c b/fetch-pack.c
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -9,6 +9,22 @@ static const char fetch_pack_usage[] =
 "git-fetch-pack [-q] [-v] [--exec=upload-pack] [host:]directory <refs>...";
 static const char *exec = "git-upload-pack";
 
+static int has_ref_flag;
+static const unsigned char *has_ref_sha1;
+
+static int has_ref_helper(const char *path, const unsigned char *sha1) {
+	if(!memcmp(sha1, has_ref_sha1, 20))
+		has_ref_flag = 1;
+	return has_ref_flag;
+}
+
+static int has_ref(const unsigned char *sha1) {
+	has_ref_sha1 = sha1;
+	has_ref_flag = 0;
+	for_each_ref(has_ref_helper);
+	return has_ref_flag;
+}
+
 static int find_common(int fd[2], unsigned char *result_sha1,
 		       struct ref *refs)
 {
@@ -16,20 +32,27 @@ static int find_common(int fd[2], unsign
 	int count = 0, flushes = 0, retval;
 	FILE *revs;
 
-	revs = popen("git-rev-list $(git-rev-parse --all)", "r");
-	if (!revs)
-		die("unable to run 'git-rev-list'");
-
 	while (refs) {
 		unsigned char *remote = refs->old_sha1;
-		if (verbose)
-			fprintf(stderr,
-				"want %s (%s)\n", sha1_to_hex(remote),
-				refs->name);
-		packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
+		if(!has_ref(remote)) {
+			if (verbose)
+				fprintf(stderr,
+					"want %s (%s)\n", sha1_to_hex(remote),
+					refs->name);
+			packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
+			count++;
+		}
 		refs = refs->next;
 	}
 	packet_flush(fd[1]);
+
+	if(count==0)
+		return 1;
+
+	revs = popen("git-rev-list $(git-rev-parse --all)", "r");
+	if (!revs)
+		die("unable to run 'git-rev-list'");
+
 	flushes = 1;
 	retval = -1;
 	while (fgets(line, sizeof(line), revs) != NULL) {
@@ -74,7 +97,7 @@ static int find_common(int fd[2], unsign
 	return retval;
 }
 
-static int fetch_pack(int fd[2], int nr_match, char **match)
+static struct ref *fetch_pack(int fd[2], int nr_match, char **match)
 {
 	struct ref *ref;
 	unsigned char sha1[20];
@@ -86,7 +109,10 @@ static int fetch_pack(int fd[2], int nr_
 		packet_flush(fd[1]);
 		die("no matching remote head");
 	}
-	if (find_common(fd, sha1, ref) < 0)
+	status = find_common(fd, sha1, ref);
+	if(status > 0)
+		return ref;
+	if(status < 0)
 		fprintf(stderr, "warning: no common commits\n");
 	pid = fork();
 	if (pid < 0)
@@ -109,12 +135,7 @@ static int fetch_pack(int fd[2], int nr_
 		int code = WEXITSTATUS(status);
 		if (code)
 			die("git-unpack-objects died with error code %d", code);
-		while (ref) {
-			printf("%s %s\n",
-			       sha1_to_hex(ref->old_sha1), ref->name);
-			ref = ref->next;
-		}
-		return 0;
+		return ref;
 	}
 	if (WIFSIGNALED(status)) {
 		int sig = WTERMSIG(status);
@@ -125,10 +146,11 @@ static int fetch_pack(int fd[2], int nr_
 
 int main(int argc, char **argv)
 {
-	int i, ret, nr_heads;
+	int i, nr_heads;
 	char *dest = NULL, **heads;
 	int fd[2];
 	pid_t pid;
+	struct ref *ref;
 
 	nr_heads = 0;
 	heads = NULL;
@@ -160,9 +182,19 @@ int main(int argc, char **argv)
 	pid = git_connect(fd, dest, exec);
 	if (pid < 0)
 		return 1;
-	ret = fetch_pack(fd, nr_heads, heads);
+	ref = fetch_pack(fd, nr_heads, heads);
+	if(!ref)
+		return 1;
+
+	while (ref) {
+		printf("%s %s\n",
+		       sha1_to_hex(ref->old_sha1), ref->name);
+		ref = ref->next;
+	}
+
 	close(fd[0]);
 	close(fd[1]);
 	finish_connect(pid);
-	return ret;
+
+	return 0;
 }

Re: [PATCH] fetch-pack should not ask for a ref which is already there

[Johannes Schindelin]

Hi,

On Thu, 29 Sep 2005, Johannes Schindelin wrote:

> Hi,
> 
> On Thu, 29 Sep 2005, Linus Torvalds wrote:
> 
> > On Thu, 29 Sep 2005, Johannes Schindelin wrote:
> > >
> > > With this patch, instead of blindly asking for every remote ref, 
> > > fetch-pack first looks in the local repository if that ref is 
> > > already there.
> > 
> > No. This is WRONG.

Following up on the "I'd rather download twice than have a corrupt 
repository":

Wouldn't it make much more sense to add a flag which repairs an incomplete 
fetch? After all, not every day you fsck up a fetch, right? So, be nice to 
the server, the network and all the rest, in most cases, and be not so 
nice if you know something went wrong and you want to fix it.

Ciao,
Dscho

Re: [PATCH] fetch-pack should not ask for a ref which is already there

[Linus Torvalds]

On Thu, 29 Sep 2005, Johannes Schindelin wrote:
>
> Wouldn't it make much more sense to add a flag which repairs an incomplete
> fetch?

No.

We've seen crap. We've _seen_ people use the old git-ssh-pull etc that 
would result in incomplete repositories, and having people use the 
"--recover" flag.

THAT KIND OF CRAP IS UNACCEPTABLE! I had to walk Andrew through a broken 
repository because he had used those unreliable fetch methods. I was 
ashamed of git at that point.

If a ^C results in a repository that needs to be fixed up, the "source 
control management" is BROKEN. It's not source control, it's a buggy mess.

I refuse to use such tools. End of story. We do this right, or we don't do 
it at all. And doing it right means that you only write refs after you've 
downloaded everything, and you only _ever_ depend on refs. You don't 
_ever_ say "if I have this object, I think I have everything".

The thing is, _reliability_ is #1. It's not performance. Performance is 
important, but performance doesn't matter AT ALL if it comes at the 
expense of reliability.

 		Linus

Re: [PATCH] fetch-pack should not ask for a ref which is already there

[Johannes Schindelin]

Hi,

On Thu, 29 Sep 2005, Linus Torvalds wrote:

> On Thu, 29 Sep 2005, Johannes Schindelin wrote:
> > 
> > Wouldn't it make much more sense to add a flag which repairs an 
> > incomplete fetch?
> 
> No.
> 
> We've seen crap. We've _seen_ people use the old git-ssh-pull etc that 
> would result in incomplete repositories, and having people use the 
> "--recover" flag.
> 
> THAT KIND OF CRAP IS UNACCEPTABLE! I had to walk Andrew through a broken 
> repository because he had used those unreliable fetch methods. I was 
> ashamed of git at that point.
> 
> If a ^C results in a repository that needs to be fixed up, the "source 
> control management" is BROKEN. It's not source control, it's a buggy 
> mess.

Okay, fair enough, I had that coming. But how about the opposite? A flag, 
that says "I want to take the fast path, and if I fsck up, it is my fault 
alone", just like "-f" to git-checkout?

Ciao,
Dscho