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

2013-02-11 Thread John Keeping
On Mon, Feb 11, 2013 at 08:42:21AM -0800, Junio C Hamano wrote:
 John Keeping j...@keeping.me.uk 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

2013-02-11 Thread Junio C Hamano
John Keeping j...@keeping.me.uk writes:

 On Mon, Feb 11, 2013 at 08:42:21AM -0800, Junio C Hamano wrote:
 John Keeping j...@keeping.me.uk 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

2013-02-10 Thread Junio C Hamano
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?

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


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

2013-02-10 Thread John Keeping
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


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

2013-02-10 Thread Junio C Hamano
John Keeping j...@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.

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