Re: [PATCH] fixup! graph: output padding for merge subsequent parents

[Junio C Hamano <gitster@pobox.com>]

John Keeping <john@keeping.me.uk> writes:

> On Sun, Feb 10, 2013 at 11:30:39AM -0800, Junio C Hamano wrote:
> ...
>> Is it correct to say that this essentially re-does 656197ad3805
>> (graph.c: infinite loop in git whatchanged --graph -m, 2009-07-25)
>> in a slightly different way, in that Michał's original fix also
>> protected against the case where graph->state is flipped to
>> GRAPH_PADDING by graph_next_line() that returns false, but with your
>> fixup, the code knows it never happens (i.e. when graph_next_line()
>> returns false, graph->state is always in the GRAPH_PADDING state),
>> and the only thing we need to be careful about is when graph->state
>> is already in the PADDING state upon entry to this function?
>
> Yes, although I wonder if we can end up in POST_MERGE or COLLAPSING
> state here as well.  The check in the loop guards against that because
> those will eventually end up as PADDING.
>
> As far as I can see, this is okay because we have called
> graph_show_remainder() at the end of outputting a commit, even when we
> end up outputting the same (merge) commit more than once.  But someone
> more familiar with the graph code might want to comment here.

More importantly, that kind of thought process needs to be
documented in the log message; that will help people to diagnose the
cause of the problem if they later find that this patch made an
incorrect assumption while simplifying the code.