* Bug in "git log --graph -p -m" (version 1.7.7.6) @ 2013-02-05 17:00 Dale R. Worley 2013-02-05 17:40 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Dale R. Worley @ 2013-02-05 17:00 UTC (permalink / raw) To: git I have found a situation where "git log" produces (apparently) endless output. Presumably this is a bug. Following is a (Linux) script that reliably reproduces the error for me (on Fedora 16): ---------- set -ve # Print the git version. git --version # Create respository. rm -rf .git git init # Initial commit. ( echo 1 ; echo 2 ; echo 3 ) >file git add file git commit -m 'Commit P' git branch B HEAD # Next commit on master adds line "1a". ( echo 1 ; echo 1a ; echo 2 ; echo 3 ) >file git add file git commit -m 'Commit Q' git checkout B # Next commit on B adds line "2a". ( echo 1 ; echo 2 ; echo 2a ; echo 3 ) >file git add file git commit -m 'Commit R' # Merge the two commits, but add line "3a" to the commit as well. git checkout master git merge --no-commit B # Show what the merge produces. cat file # Add line "3a". ( echo 1 ; echo 1a ; echo 2 ; echo 2a ; echo 3 ; echo 3a ) >file git commit -m 'Commit S' # These log commands work. git log git log --graph git log --graph -p # This log command produces infinite output. git log --graph -p -m ---------- Dale ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Bug in "git log --graph -p -m" (version 1.7.7.6) 2013-02-05 17:00 Bug in "git log --graph -p -m" (version 1.7.7.6) Dale R. Worley @ 2013-02-05 17:40 ` Junio C Hamano 2013-02-05 21:09 ` Matthieu Moy 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2013-02-05 17:40 UTC (permalink / raw) To: Dale R. Worley; +Cc: git worley@alum.mit.edu (Dale R. Worley) writes: > I have found a situation where "git log" produces (apparently) > endless output. Presumably this is a bug. Following is a (Linux) > script that reliably reproduces the error for me (on Fedora 16): Wasn't this fixed at v1.8.1.1~13 or is this a different issue? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Bug in "git log --graph -p -m" (version 1.7.7.6) 2013-02-05 17:40 ` Junio C Hamano @ 2013-02-05 21:09 ` Matthieu Moy 2013-02-06 15:03 ` Dale R. Worley 0 siblings, 1 reply; 9+ messages in thread From: Matthieu Moy @ 2013-02-05 21:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dale R. Worley, git Junio C Hamano <gitster@pobox.com> writes: > worley@alum.mit.edu (Dale R. Worley) writes: > >> I have found a situation where "git log" produces (apparently) >> endless output. Presumably this is a bug. Following is a (Linux) >> script that reliably reproduces the error for me (on Fedora 16): > > Wasn't this fixed at v1.8.1.1~13 or is this a different issue? In any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get undless output. On the other hand, I get a slightly misformatted output: * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d) |\ Merge: 2c1e6a3 33e70e7 | | Author: Matthieu Moy <Matthieu.Moy@imag.fr> | | Date: Tue Feb 5 22:05:33 2013 +0100 | | | | Commit S | | | | diff --git a/file b/file | | index 6bb4d3e..afd2e75 100644 | | --- a/file | | +++ b/file | | @@ -1,4 +1,5 @@ | | 1 | | 1a | | 2 | | +2a | | 3 | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8) | | Merge: 2c1e6a3 33e70e7 | | Author: Matthieu Moy <Matthieu.Moy@imag.fr> | | Date: Tue Feb 5 22:05:33 2013 +0100 The second "commit" line (diff with second parent) doesn't have the "| |" prefix, I don't think this is intentional. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Bug in "git log --graph -p -m" (version 1.7.7.6) 2013-02-05 21:09 ` Matthieu Moy @ 2013-02-06 15:03 ` Dale R. Worley 2013-02-06 15:14 ` John Keeping 0 siblings, 1 reply; 9+ messages in thread From: Dale R. Worley @ 2013-02-06 15:03 UTC (permalink / raw) To: Matthieu Moy; +Cc: gitster, git > From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> > > In any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get > undless output. On the other hand, I get a slightly misformatted output: > > * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d) > |\ Merge: 2c1e6a3 33e70e7 > | | Author: Matthieu Moy <Matthieu.Moy@imag.fr> > | | Date: Tue Feb 5 22:05:33 2013 +0100 > | | > | | Commit S > | | > | | diff --git a/file b/file > | | index 6bb4d3e..afd2e75 100644 > | | --- a/file > | | +++ b/file > | | @@ -1,4 +1,5 @@ > | | 1 > | | 1a > | | 2 > | | +2a > | | 3 > | | > commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8) > | | Merge: 2c1e6a3 33e70e7 > | | Author: Matthieu Moy <Matthieu.Moy@imag.fr> > | | Date: Tue Feb 5 22:05:33 2013 +0100 > > The second "commit" line (diff with second parent) doesn't have the > "| |" prefix, I don't think this is intentional. The second "commit" line should start with "| * ": > | | > | * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8) > | | Merge: 2c1e6a3 33e70e7 > | | Author: Matthieu Moy <Matthieu.Moy@imag.fr> > | | Date: Tue Feb 5 22:05:33 2013 +0100 Dale ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Bug in "git log --graph -p -m" (version 1.7.7.6) 2013-02-06 15:03 ` Dale R. Worley @ 2013-02-06 15:14 ` John Keeping 2013-02-06 18:33 ` Matthieu Moy 0 siblings, 1 reply; 9+ messages in thread From: John Keeping @ 2013-02-06 15:14 UTC (permalink / raw) To: Dale R. Worley; +Cc: Matthieu Moy, gitster, git On Wed, Feb 06, 2013 at 10:03:00AM -0500, Dale R. Worley wrote: > > From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> > > > > In any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get > > undless output. On the other hand, I get a slightly misformatted output: > > > > * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d) > > |\ Merge: 2c1e6a3 33e70e7 > > | | Author: Matthieu Moy <Matthieu.Moy@imag.fr> > > | | Date: Tue Feb 5 22:05:33 2013 +0100 > > | | > > | | Commit S > > | | > > | | diff --git a/file b/file > > | | index 6bb4d3e..afd2e75 100644 > > | | --- a/file > > | | +++ b/file > > | | @@ -1,4 +1,5 @@ > > | | 1 > > | | 1a > > | | 2 > > | | +2a > > | | 3 > > | | > > commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8) > > | | Merge: 2c1e6a3 33e70e7 > > | | Author: Matthieu Moy <Matthieu.Moy@imag.fr> > > | | Date: Tue Feb 5 22:05:33 2013 +0100 > > > > The second "commit" line (diff with second parent) doesn't have the > > "| |" prefix, I don't think this is intentional. > > The second "commit" line should start with "| * ": No. That would indicate a commit on the branch that is the second parent of the first commit. But this is the same commit as the one above, just with a diff against its second parent instead of its first parent. I would argue that the line should start with "| | ", since it really is just a continuation of the same commit. | | | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8) | | Merge: 2c1e6a3 33e70e7 | | Author: Matthieu Moy <Matthieu.Moy@imag.fr> | | Date: Tue Feb 5 22:05:33 2013 +0100 John ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Bug in "git log --graph -p -m" (version 1.7.7.6) 2013-02-06 15:14 ` John Keeping @ 2013-02-06 18:33 ` Matthieu Moy 2013-02-06 19:57 ` [PATCH] graph: output padding for merge subsequent parents John Keeping 0 siblings, 1 reply; 9+ messages in thread From: Matthieu Moy @ 2013-02-06 18:33 UTC (permalink / raw) To: John Keeping; +Cc: Dale R. Worley, gitster, git John Keeping <john@keeping.me.uk> writes: > I would argue that the line should start with "| | ", since it really is > just a continuation of the same commit. > > | | > | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8) > | | Merge: 2c1e6a3 33e70e7 > | | Author: Matthieu Moy <Matthieu.Moy@imag.fr> > | | Date: Tue Feb 5 22:05:33 2013 +0100 Yes. I had a look at the code, I guess the call to graph_show_commit() in show_log() (in log-tree.c) should have called graph_show_padding() but didn't in this case. Then I got lost in graph.c :-(. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] graph: output padding for merge subsequent parents 2013-02-06 18:33 ` Matthieu Moy @ 2013-02-06 19:57 ` John Keeping 2013-02-08 17:52 ` Matthieu Moy 0 siblings, 1 reply; 9+ messages in thread From: John Keeping @ 2013-02-06 19:57 UTC (permalink / raw) To: Matthieu Moy; +Cc: Dale R. Worley, gitster, git On Wed, Feb 06, 2013 at 07:33:08PM +0100, Matthieu Moy wrote: > John Keeping <john@keeping.me.uk> writes: > > > I would argue that the line should start with "| | ", since it really is > > just a continuation of the same commit. > > > > | | > > | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8) > > | | Merge: 2c1e6a3 33e70e7 > > | | Author: Matthieu Moy <Matthieu.Moy@imag.fr> > > | | Date: Tue Feb 5 22:05:33 2013 +0100 > > Yes. > > I had a look at the code, I guess the call to graph_show_commit() in > show_log() (in log-tree.c) should have called graph_show_padding() but > didn't in this case. Then I got lost in graph.c :-(. I think this is the correct answer. But now I've found that "git log --graph -c -p" doesn't indent the diff - that seems to be a separate issue. -- >8 -- When showing merges in git-log, the same commit is shown once for each parent. Combined with "--graph" this results in graph_show_commit() being called once for each parent without graph_update() being called. Currently graph_show_commit() does not print anything on subsequent invocations for the same commit (this was changed by commit 656197a - "graph.c: infinite loop in git whatchanged --graph -m" from the previous behaviour of looping infinitely). Change this so that if the graph code believes it has already shown the commit it prints a single padding line. Signed-off-by: John Keeping <john@keeping.me.uk> --- graph.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/graph.c b/graph.c index 391a712..2a3fc5c 100644 --- a/graph.c +++ b/graph.c @@ -1227,6 +1227,16 @@ void graph_show_commit(struct git_graph *graph) if (!graph) return; + /* + * When showing a diff of a merge against each of its parents, we + * are called once for each parent without graph_update having been + * called. In this case, simply output a single padding line. + */ + if (graph_is_commit_finished(graph)) { + graph_show_padding(graph); + shown_commit_line = 1; + } + while (!shown_commit_line && !graph_is_commit_finished(graph)) { shown_commit_line = graph_next_line(graph, &msgbuf); fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout); -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] graph: output padding for merge subsequent parents 2013-02-06 19:57 ` [PATCH] graph: output padding for merge subsequent parents John Keeping @ 2013-02-08 17:52 ` Matthieu Moy 2013-02-08 19:40 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Matthieu Moy @ 2013-02-08 17:52 UTC (permalink / raw) To: John Keeping; +Cc: Dale R. Worley, gitster, git John Keeping <john@keeping.me.uk> writes: > diff --git a/graph.c b/graph.c > index 391a712..2a3fc5c 100644 > --- a/graph.c > +++ b/graph.c > @@ -1227,6 +1227,16 @@ void graph_show_commit(struct git_graph *graph) > if (!graph) > return; > > + /* > + * When showing a diff of a merge against each of its parents, we > + * are called once for each parent without graph_update having been > + * called. In this case, simply output a single padding line. > + */ > + if (graph_is_commit_finished(graph)) { > + graph_show_padding(graph); > + shown_commit_line = 1; > + } > + > while (!shown_commit_line && !graph_is_commit_finished(graph)) { This works, but if we know we're not going to enter the while loop, it seams even easier to do this: --- a/graph.c +++ b/graph.c @@ -1227,7 +1227,17 @@ void graph_show_commit(struct git_graph *graph) if (!graph) return; - while (!shown_commit_line && !graph_is_commit_finished(graph)) { + /* + * When showing a diff of a merge against each of its parents, we + * are called once for each parent without graph_update having been + * called. In this case, simply output a single padding line. + */ + if (graph_is_commit_finished(graph)) { + graph_show_padding(graph); + return; + } + + while (!shown_commit_line) { shown_commit_line = graph_next_line(graph, &msgbuf); fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout); if (!shown_commit_line) -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] graph: output padding for merge subsequent parents 2013-02-08 17:52 ` Matthieu Moy @ 2013-02-08 19:40 ` Junio C Hamano 0 siblings, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2013-02-08 19:40 UTC (permalink / raw) To: Matthieu Moy; +Cc: John Keeping, Dale R. Worley, git Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > John Keeping <john@keeping.me.uk> writes: > >> diff --git a/graph.c b/graph.c >> index 391a712..2a3fc5c 100644 >> --- a/graph.c >> +++ b/graph.c >> @@ -1227,6 +1227,16 @@ void graph_show_commit(struct git_graph *graph) >> if (!graph) >> return; >> >> + /* >> + * When showing a diff of a merge against each of its parents, we >> + * are called once for each parent without graph_update having been >> + * called. In this case, simply output a single padding line. >> + */ >> + if (graph_is_commit_finished(graph)) { >> + graph_show_padding(graph); >> + shown_commit_line = 1; >> + } >> + >> while (!shown_commit_line && !graph_is_commit_finished(graph)) { > > This works, but if we know we're not going to enter the while loop, it > seams even easier to do this: > > --- a/graph.c > +++ b/graph.c > @@ -1227,7 +1227,17 @@ void graph_show_commit(struct git_graph *graph) > if (!graph) > return; > > - while (!shown_commit_line && !graph_is_commit_finished(graph)) { > + /* > + * When showing a diff of a merge against each of its parents, we > + * are called once for each parent without graph_update having been > + * called. In this case, simply output a single padding line. > + */ > + if (graph_is_commit_finished(graph)) { > + graph_show_padding(graph); > + return; > + } > + > + while (!shown_commit_line) { > shown_commit_line = graph_next_line(graph, &msgbuf); > fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout); > if (!shown_commit_line) In this particular case, with the current state of this function, it is probably OK, but an early return like this tend to be a source of future bugs in the longer term, to make the codeflow skip whatever necessary clean-up that needs to be done after the loop exits. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-02-08 19:40 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-02-05 17:00 Bug in "git log --graph -p -m" (version 1.7.7.6) Dale R. Worley 2013-02-05 17:40 ` Junio C Hamano 2013-02-05 21:09 ` Matthieu Moy 2013-02-06 15:03 ` Dale R. Worley 2013-02-06 15:14 ` John Keeping 2013-02-06 18:33 ` Matthieu Moy 2013-02-06 19:57 ` [PATCH] graph: output padding for merge subsequent parents John Keeping 2013-02-08 17:52 ` Matthieu Moy 2013-02-08 19:40 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).