On Sun, Feb 10, 2013 at 11:30:39AM -0800, Junio C Hamano wrote:
> John Keeping <j...@keeping.me.uk> writes:
>
> > Can you squash this into the first commit before you do?
> >
> > Matthieu is correct that the graph_is_commit_finished() check isn't
> > needed in the loop now that we've pulled it out to be checked first -
> > the value returned can't change during the loop. I've left the early
> > return out.
> >
> > graph.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/graph.c b/graph.c
> > index 2a3fc5c..56f970f 100644
> > --- a/graph.c
> > +++ b/graph.c
> > @@ -1237,7 +1237,7 @@ void graph_show_commit(struct git_graph *graph)
> > shown_commit_line = 1;
> > }
> >
> > - while (!shown_commit_line && !graph_is_commit_finished(graph)) {
> > + while (!shown_commit_line) {
> > shown_commit_line = graph_next_line(graph, &msgbuf);
> > fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
> > if (!shown_commit_line)
>
> 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.
John
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
