[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 <redacted>

---

 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)

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

Hi,

On Thu, 29 Sep 2005, Linus Torvalds wrote:

On Thu, 29 Sep 2005, Johannes Schindelin wrote:

quoted

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 <redacted>

---

 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;
 }

Hi,

On Thu, 29 Sep 2005, Johannes Schindelin wrote:

Hi,

On Thu, 29 Sep 2005, Linus Torvalds wrote:

quoted

On Thu, 29 Sep 2005, Johannes Schindelin wrote:

quoted

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

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

Hi,

On Thu, 29 Sep 2005, Linus Torvalds wrote:

On Thu, 29 Sep 2005, Johannes Schindelin wrote:

quoted

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