DORMANTno replies

[PATCH] Do not use git-rev-list in git-fetch-pack

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:42:09
Subsystem: the rest · Maintainer: Linus Torvalds

The code used to call git-rev-list to enumerate the local revisions.
A disadvantage of that method was that git-fetch-pack would happily
enumerate ancestors of acknowledged common commits, which was just
taking unnecessary bandwidth.

Further, git-upload-pack makes use only of the first MAX_HAS == 16
revisions! So if git-fetch-pack has something to say, it better does
so concisely.

Therefore, do not use git-rev-list on the fetching side. Send the
revisions starting with the local heads, ignoring the acknowledged
revisions and their ancestors.

When git-fetch-pack is verbose, it now says which sha1 got acknowledged.

Signed-off-by: Johannes Schindelin <redacted>

---

	I am quite certain that MAX_HAS is too low. Since
	MAX_NEEDS == 256, I think this value would be better for
	MAX_HAS, too, especially since git-fetch-pack no longer
	sends so much crud with this patch.

	Also: If I read the code correctly, find_common() only sends
	"have" lines as long as it does not receive an "ACK" line,
	i.e. in effect git-fetch-pack sends only up to the first
	common revision (plus at most 31 others).

	That is all well if we are expecting only one upstream
	repository, but not in a truly distributed environment.

	Maybe it is time to revise that behaviour? For example,
	fetch-pack could stop when it is out of non-common revs,
	or when it got MAX_HAS "ACK" lines.

	Also, git-upload-pack could accept only up to a certain
	number of "have" lines.

 fetch-pack.c |  143 ++++++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 109 insertions(+), 34 deletions(-)

applies-to: 50f9bed9a78e1f51178ebf6be3cd9c20b2ffcb0b
2b5886e2198ea4b8403839b7188e8ad9e6f1713b
diff --git a/fetch-pack.c b/fetch-pack.c
index 8566ab1..cdde008 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -13,18 +13,110 @@ static const char fetch_pack_usage[] =
 static const char *exec = "git-upload-pack";
 
 #define COMPLETE	(1U << 0)
+#define COMMON		(1U << 1)
+#define COMMON_REF	(1U << 2)
+#define TO_SCAN		(1U << 3)
+
+struct commit_list *rev_list = NULL;
+
+/*
+   Unfortunately, we want not only commits, but sometimes also tags.
+*/
+
+static struct commit *fake_commit(struct object *object) {
+	struct commit *ret;
+
+	if (object->type == commit_type)
+		return (struct commit *)object;
+
+	ret = xmalloc(sizeof(struct commit));
+
+	memset(ret, 0, sizeof(struct commit));
+	ret->object = *object;
+
+	/* insert_by_date() shall put it at the end */
+	ret->date = 0;
+
+	return ret;
+}
+
+static int rev_list_insert(const char *path, const unsigned char *sha1)
+{
+	struct commit *commit  = fake_commit(parse_object(sha1));
+
+	/*
+	   Local heads are much more likely to be common, so send them first.
+	*/
+	if (commit->object.type == commit_type)
+		commit_list_insert(commit, &rev_list);
+	else {
+		insert_by_date(commit, &rev_list);
+
+		if (commit->object.type == tag_type) {
+			/* This commit has been faked. Reget. */
+			struct object *o = lookup_object(commit->object.sha1);
+			o = deref_tag(o);
+			if (o->type == commit_type)
+				insert_by_date((struct commit *)o, &rev_list);
+		}
+	}
+
+	return 0;
+}
+
+/*
+  Get the next rev to send, ignoring the common.
+*/
+
+static const unsigned char* get_rev()
+{
+	struct commit *commit = NULL;
+
+	while (commit == NULL) {
+		unsigned int mark;
+
+		if (rev_list == NULL)
+			return NULL;
+
+		commit = rev_list->item;
+
+		if (commit->object.flags&COMMON) {
+			/* do not send "have", and ignore ancestors */
+			commit = NULL;
+			mark = COMMON | TO_SCAN;
+		} else if (commit->object.flags&COMMON_REF)
+			/* send "have", and ignore ancestors */
+			mark = COMMON | TO_SCAN;
+		else
+			/* send "have", also for ancestors, until one is ack'ed */
+			mark = TO_SCAN;
+
+		pop_most_recent_commit(&rev_list, mark);
+	}
+
+	return commit->object.sha1;
+}
+
+static void mark_common(const unsigned char* sha1)
+{
+	struct object *o = lookup_object(sha1);
+
+	if (o != NULL && !(o->flags&COMMON)) {
+		o->flags |= COMMON;
+		if (o->type == commit_type)
+			insert_by_date((struct commit *)o, &rev_list);
+	}
+}
 
 static int find_common(int fd[2], unsigned char *result_sha1,
 		       struct ref *refs)
 {
 	int fetching;
-	static char line[1000];
-	static char rev_command[1024];
-	int count = 0, flushes = 0, retval, rev_command_len;
-	FILE *revs;
+	int count = 0, flushes = 0, retval;
+	const unsigned char *sha1;
+
+	for_each_ref(rev_list_insert);
 
-	strcpy(rev_command, "git-rev-list $(git-rev-parse --all)");
-	rev_command_len = strlen(rev_command);
 	fetching = 0;
 	for ( ; refs ; refs = refs->next) {
 		unsigned char *remote = refs->old_sha1;
@@ -42,25 +134,11 @@ static int find_common(int fd[2], unsign
 		 */
 		if (((o = lookup_object(remote)) != NULL) &&
 		    (o->flags & COMPLETE)) {
-			struct commit_list *p;
-			struct commit *commit =
-				(struct commit *) (o = deref_tag(o));
-			if (!o)
-				goto repair;
-			if (o->type != commit_type)
-				continue;
-			p = commit->parents;
-			while (p &&
-			       rev_command_len + 44 < sizeof(rev_command)) {
-				snprintf(rev_command + rev_command_len, 44,
-					 " ^%s",
-					 sha1_to_hex(p->item->object.sha1));
-				rev_command_len += 43;
-				p = p->next;
-			}
+			o->flags |= COMMON_REF | TO_SCAN;
+			commit_list_insert(fake_commit(o), &rev_list);
 			continue;
 		}
-	repair:
+
 		packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
 		fetching++;
 	}
@@ -68,21 +146,16 @@ static int find_common(int fd[2], unsign
 	if (!fetching)
 		return 1;
 
-	revs = popen(rev_command, "r");
-	if (!revs)
-		die("unable to run 'git-rev-list'");
-
 	flushes = 1;
 	retval = -1;
-	while (fgets(line, sizeof(line), revs) != NULL) {
-		unsigned char sha1[20];
-		if (get_sha1_hex(line, sha1))
-			die("git-fetch-pack: expected object name, got crud");
+	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)) {
 			packet_flush(fd[1]);
+			if (verbose)
+				fprintf(stderr, "flush\n");
 			flushes++;
 
 			/*
@@ -92,16 +165,17 @@ static int find_common(int fd[2], unsign
 			if (count == 32)
 				continue;
 			if (get_ack(fd[0], result_sha1)) {
+				mark_common(result_sha1);
 				flushes = 0;
 				retval = 0;
 				if (verbose)
-					fprintf(stderr, "got ack\n");
+					fprintf(stderr, "got ack %s\n",
+						sha1_to_hex(result_sha1));
 				break;
 			}
 			flushes--;
 		}
 	}
-	pclose(revs);
 	packet_write(fd[1], "done\n");
 	if (verbose)
 		fprintf(stderr, "done\n");
@@ -109,7 +183,8 @@ static int find_common(int fd[2], unsign
 		flushes--;
 		if (get_ack(fd[0], result_sha1)) {
 			if (verbose)
-				fprintf(stderr, "got ack\n");
+				fprintf(stderr, "got ack %s\n",
+					sha1_to_hex(result_sha1));
 			return 0;
 		}
 	}
---
0.99.8.GIT
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help