# Bug in "git log --graph -p -m" (version 1.7.7.6) 9 messages from 2013-02-05 to 2013-02-08. Participants: Dale R. Worley, Junio C Hamano, Matthieu Moy, John Keeping. Thread: https://gitlist.dev/t/32835 ## Dale R. Worley, 2013-02-05 17:00 Subject: Bug in "git log --graph -p -m" (version 1.7.7.6) Message-ID: <201302051700.r15H0GXx031004@freeze.ariadne.com> URL: https://gitlist.dev/e/201302051700.r15H0GXx031004%40freeze.ariadne.com ``` 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 ``` ## Junio C Hamano, 2013-02-05 17:40 Subject: Re: Bug in "git log --graph -p -m" (version 1.7.7.6) Message-ID: <7vtxpqslpm.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtxpqslpm.fsf%40alter.siamese.dyndns.org In-Reply-To: <201302051700.r15H0GXx031004@freeze.ariadne.com> ``` 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? ``` ## Matthieu Moy, 2013-02-05 21:09 Subject: Re: Bug in "git log --graph -p -m" (version 1.7.7.6) Message-ID: URL: https://gitlist.dev/e/vpqmwvia2n7.fsf%40grenoble-inp.fr In-Reply-To: <7vtxpqslpm.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano 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 | | 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 | | 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/ ``` ## Dale R. Worley, 2013-02-06 15:03 Subject: Re: Bug in "git log --graph -p -m" (version 1.7.7.6) Message-ID: <201302061503.r16F30UA016375@freeze.ariadne.com> URL: https://gitlist.dev/e/201302061503.r16F30UA016375%40freeze.ariadne.com In-Reply-To: ``` > From: Matthieu Moy > > 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 > | | 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 > | | 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 > | | Date: Tue Feb 5 22:05:33 2013 +0100 Dale ``` ## John Keeping, 2013-02-06 15:14 Subject: Re: Bug in "git log --graph -p -m" (version 1.7.7.6) Message-ID: <20130206151447.GZ1342@serenity.lan> URL: https://gitlist.dev/e/20130206151447.GZ1342%40serenity.lan In-Reply-To: <201302061503.r16F30UA016375@freeze.ariadne.com> ``` On Wed, Feb 06, 2013 at 10:03:00AM -0500, Dale R. Worley wrote: > > From: Matthieu Moy > > > > 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 > > | | 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 > > | | 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 | | Date: Tue Feb 5 22:05:33 2013 +0100 John ``` ## Matthieu Moy, 2013-02-06 18:33 Subject: Re: Bug in "git log --graph -p -m" (version 1.7.7.6) Message-ID: URL: https://gitlist.dev/e/vpqbobxwavv.fsf%40grenoble-inp.fr In-Reply-To: <20130206151447.GZ1342@serenity.lan> ``` John Keeping 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 > | | 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/ ``` ## John Keeping, 2013-02-06 19:57 Subject: [PATCH] graph: output padding for merge subsequent parents Message-ID: <20130206195702.GA1342@serenity.lan> URL: https://gitlist.dev/e/20130206195702.GA1342%40serenity.lan In-Reply-To: ``` On Wed, Feb 06, 2013 at 07:33:08PM +0100, Matthieu Moy wrote: > John Keeping 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 > > | | 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 --- 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 ``` ## Matthieu Moy, 2013-02-08 17:52 Subject: Re: [PATCH] graph: output padding for merge subsequent parents Message-ID: URL: https://gitlist.dev/e/vpqtxpmae1p.fsf%40grenoble-inp.fr In-Reply-To: <20130206195702.GA1342@serenity.lan> ``` John Keeping 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/ ``` ## Junio C Hamano, 2013-02-08 19:40 Subject: Re: [PATCH] graph: output padding for merge subsequent parents Message-ID: <7vd2wa1tno.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd2wa1tno.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Matthieu Moy writes: > John Keeping 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. ```