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

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