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

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