Re: [PATCH] graph: avoid infinite loop in graph_show_commit()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 24, 2012, 23:36 UTC
Message-ID: <7vtxunvvr4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CACsJy8ApYKOU8v_-HkUC5uOb8gsheugKaXKMjbm0_-ygW_4jiQ@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Show 15 quoted lines

> On Sun, Sep 23, 2012 at 6:55 PM, Michal Kiedrowicz
> <michal.kiedrowicz@gmail.com> wrote:
>> Nguyễn Thái Ngọc Duy <pclouds <at> gmail.com> writes:
>>
>>>
>>> The loop can be triggered with "git diff-tree --graph commit" where
>>> the commit is a non-merge. It goes like this
>>
>>
>> Isn't this the same issue as in
>> http://article.gmane.org/gmane.comp.version-control.git/123979
>> ? (with slightly different fix)
>
> I don't know. I'm not familiar enough with graph.c to tell. Maybe Adam
> can have a look?

Has either of you tried the patch with the problematic case the other patch tries to solve? Michal's old patch does smell like it is going in the better direction in that it stops looping when we know we would only be showing the padding, which is a sign that we are done with showing the commit.

But I didn't look at it too closely. I'd prefer to see the assert(0) turned into die("BUG: internal error") at the end of graph_next_line() to catch these cases. Also I am not sure if assignment of the return value from graph_next_line() to shown_comit_line in the loop is correct (shouldn't it be OR'ing it in, so that "we have shown the information on this commit" is not lost when the function adds things after showing the commit???)