Re: Hmm.. Try harder to find the common commits in git protocol?

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Apr 28, 2008, 20:15 UTC
Message-ID: <alpine.LFD.1.10.0804281310320.3119@woody.linux-foundation.org>
In-Reply-To: <alpine.LFD.1.10.0804280936080.3119@woody.linux-foundation.org>

Hmm. Trying to debug this, I find the behavior hard to reproduce. But I *also* find that there seems to be something wrong in builtin-fetch-pack.

Look at commit f3ec549481827b10609a43bf504517a0e8063a12 ("fetch-pack: check parse_commit/object results"), and tell me that the "parents" handling isn't totally broken. In particular, get_rev() does:

	struct commit_list *parents = NULL;
	..
	commit = rev_list->item;
	if (!(commit->object.parsed))
		if (!parse_commit(commit))
			parents = commit->parents;
	..

which means that "parents" will never even get set if the commit was already parsed!

And whether it got parsed or not depends on how we got there etc, so this may explain the occasionally odd behaviour I saw.

Basically, I don't think that code can be right, and with a cut-down repo, I get "no commits in common" due to this, because the whole "get_rev()" thing doesn't work.

This patch should fix that problem, but I wonder why it got rewritten that way in the first place?

			Linus

---
 builtin-fetch-pack.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 65350ca..c97a427 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -117,15 +117,15 @@ static const unsigned char* get_rev(void)
 
 	while (commit == NULL) {
 		unsigned int mark;
-		struct commit_list *parents = NULL;
+		struct commit_list *parents;
 
 		if (rev_list == NULL || non_common_revs == 0)
 			return NULL;
 
 		commit = rev_list->item;
-		if (!(commit->object.parsed))
-			if (!parse_commit(commit))
-				parents = commit->parents;
+		if (!commit->object.parsed)
+			parse_commit(commit);
+		parents = commit->parents;
 
 		commit->object.flags |= POPPED;
 		if (!(commit->object.flags & COMMON))