On Mon, 28 Apr 2008, Linus Torvalds wrote:
> 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?
Looks like f3ec549481827b10609a43bf504517a0e8063a12 was trying to do
something clever when parse_commit returns non-zero, and accidentally did
the same thing with the case where it isn't needed.
>
> 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;
I think it's supposed to be:
if (commit->object.parsed ||
!parse_commit(commit)))
parents = commit->parents;
Just looking at what the other changes in the same commit do.
-Daniel
*This .sig left intentionally blank*
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
