On Sun, Feb 10, 2013 at 02:38:46PM -0800, Junio C Hamano wrote: > John Keeping <john@xxxxxxxxxxxxx> 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. Perhaps it's best to leave the patch as it originally was to guarantee that we can't get stuck in graph_show_commit(), even when it's called at an unexpected time, but I see you've already squashed this change in. Would you prefer me to resend the original patch or send an update with this change and the above reasoning in the commit message? John -- 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