Re: [PATCH] fixup! graph: output padding for merge subsequent parents
John Keeping writes: > On Mon, Feb 11, 2013 at 08:42:21AM -0800, Junio C Hamano wrote: >> John Keeping writes: >> >> > 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? >> >> Yes, please. Let's have the original (I think I have it in my >> reflog so no need to resend it) and this update on top as a separate >> patch with an updated log message. > > I was suggesting dropping the change to remove the > graph_is_commit_finished() check in the loop. I'm not sure it buys us > much and there are still situations that could result in the state > changing to PADDING during the loop if the graph API is used in an > unexpected way. OK, so the fixup! was not done with enough thought. I am fine with dropping it. Thanks. -- 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
Re: [PATCH] fixup! graph: output padding for merge subsequent parents
On Mon, Feb 11, 2013 at 08:42:21AM -0800, Junio C Hamano wrote: > John Keeping writes: > > > 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? > > Yes, please. Let's have the original (I think I have it in my > reflog so no need to resend it) and this update on top as a separate > patch with an updated log message. I was suggesting dropping the change to remove the graph_is_commit_finished() check in the loop. I'm not sure it buys us much and there are still situations that could result in the state changing to PADDING during the loop if the graph API is used in an unexpected way. Are others convinced that this change is always safe? 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
Re: [PATCH] fixup! graph: output padding for merge subsequent parents
John Keeping writes: > 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? Yes, please. Let's have the original (I think I have it in my reflog so no need to resend it) and this update on top as a separate patch with an updated log message. -- 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
Re: [PATCH] fixup! graph: output padding for merge subsequent parents
On Sun, Feb 10, 2013 at 02:38:46PM -0800, Junio C Hamano wrote: > John Keeping 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fixup! graph: output padding for merge subsequent parents
John Keeping 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. -- 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
Re: [PATCH] fixup! graph: output padding for merge subsequent parents
On Sun, Feb 10, 2013 at 11:30:39AM -0800, Junio C Hamano wrote:
> John Keeping 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
Re: [PATCH] fixup! graph: output padding for merge subsequent parents
John Keeping 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?
Sorry for an overlong single sentence question ;-)
Thanks.
--
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
