From: John Keeping <john@keeping.me.uk>
Date: Sun, 10 Feb 2013 21:02:29 GMT
Subject: Re: [PATCH] fixup! graph: output padding for merge subsequent parents
Message-ID: <20130210210229.GB2270@serenity.lan>
In-Reply-To: <7vliawt19c.fsf@alter.siamese.dyndns.org>

On Sun, Feb 10, 2013 at 11:30:39AM -0800, Junio C Hamano wrote:
> John Keeping <john@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