From: Junio C Hamano Date: Mon, 28 Apr 2008 23:20:08 GMT Subject: Re: Hmm.. Try harder to find the common commits in git protocol? Message-ID: <7vabjdbm8n.fsf@gitster.siamese.dyndns.org> In-Reply-To: Linus Torvalds writes: > 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,... > This patch should fix that problem, but I wonder why it got rewritten that > way in the first place? Sorry, my fault while reviewing the series. The intent of the update was too focused on not touching possibly bad data (i.e. when parse_commit() does not return zero to indicate success) and failed to make sure that it acts identically on good data --- very bad.