{"thread":{"id":"32835","subject":"Bug in \"git log --graph -p -m\" (version 1.7.7.6)","startedAt":"2013-02-05T17:00:16Z","lastAt":"2013-02-08T19:40:11Z","messageCount":9,"participants":["Dale R. Worley","Junio C Hamano","Matthieu Moy","John Keeping"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"208745","messageId":"201302051700.r15H0GXx031004@freeze.ariadne.com","threadId":"32835","inReplyTo":null,"subject":"Bug in \"git log --graph -p -m\" (version 1.7.7.6)","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-02-05T17:00:16Z","receivedAt":"2013-02-05T17:00:16Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"I have found a situation where \"git log\" produces (apparently)\nendless output. Presumably this is a bug. Following is a (Linux)\nscript that reliably reproduces the error for me (on Fedora 16):\n\n----------\nset -ve\n\n# Print the git version.\ngit --version\n\n# Create respository.\nrm -rf .git\ngit init\n\n# Initial commit.\n( echo 1 ; echo 2 ; echo 3 ) >file\ngit add file\ngit commit -m 'Commit P'\ngit branch B HEAD\n\n# Next commit on master adds line \"1a\".\n( echo 1 ; echo 1a ; echo 2 ; echo 3 ) >file\ngit add file\ngit commit -m 'Commit Q'\n\ngit checkout B\n\n# Next commit on B adds line \"2a\".\n( echo 1 ; echo 2 ; echo 2a ; echo 3 ) >file\ngit add file\ngit commit -m 'Commit R'\n\n# Merge the two commits, but add line \"3a\" to the commit as well.\ngit checkout master\ngit merge --no-commit B\n# Show what the merge produces.\ncat file\n# Add line \"3a\".\n( echo 1 ; echo 1a ; echo 2 ; echo 2a ; echo 3 ; echo 3a ) >file\ngit commit -m 'Commit S'\n\n# These log commands work.\ngit log\ngit log --graph\ngit log --graph -p\n\n# This log command produces infinite output.\ngit log --graph -p -m\n----------\n\nDale\n"},{"id":"208750","messageId":"7vtxpqslpm.fsf@alter.siamese.dyndns.org","threadId":"32835","inReplyTo":"201302051700.r15H0GXx031004@freeze.ariadne.com","subject":"Re: Bug in \"git log --graph -p -m\" (version 1.7.7.6)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-05T17:40:37Z","receivedAt":"2013-02-05T17:40:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"worley@alum.mit.edu (Dale R. Worley) writes:\n\n> I have found a situation where \"git log\" produces (apparently)\n> endless output. Presumably this is a bug. Following is a (Linux)\n> script that reliably reproduces the error for me (on Fedora 16):\n\nWasn't this fixed at v1.8.1.1~13 or is this a different issue?\n"},{"id":"208766","messageId":"vpqmwvia2n7.fsf@grenoble-inp.fr","threadId":"32835","inReplyTo":"7vtxpqslpm.fsf@alter.siamese.dyndns.org","subject":"Re: Bug in \"git log --graph -p -m\" (version 1.7.7.6)","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-02-05T21:09:48Z","receivedAt":"2013-02-05T21:09:48Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> worley@alum.mit.edu (Dale R. Worley) writes:\n>\n>> I have found a situation where \"git log\" produces (apparently)\n>> endless output. Presumably this is a bug. Following is a (Linux)\n>> script that reliably reproduces the error for me (on Fedora 16):\n>\n> Wasn't this fixed at v1.8.1.1~13 or is this a different issue?\n\nIn any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get\nundless output. On the other hand, I get a slightly misformatted output:\n\n* commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d)\n|\\ Merge: 2c1e6a3 33e70e7\n| | Author: Matthieu Moy \n| | Date: Tue Feb 5 22:05:33 2013 +0100\n| | \n| | Commit S\n| | \n| | diff --git a/file b/file\n| | index 6bb4d3e..afd2e75 100644\n| | --- a/file\n| | +++ b/file\n| | @@ -1,4 +1,5 @@\n| | 1\n| | 1a\n| | 2\n| | +2a\n| | 3\n| | \ncommit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)\n| | Merge: 2c1e6a3 33e70e7\n| | Author: Matthieu Moy \n| | Date: Tue Feb 5 22:05:33 2013 +0100\n\nThe second \"commit\" line (diff with second parent) doesn't have the\n\"| |\" prefix, I don't think this is intentional.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"208802","messageId":"201302061503.r16F30UA016375@freeze.ariadne.com","threadId":"32835","inReplyTo":"vpqmwvia2n7.fsf@grenoble-inp.fr","subject":"Re: Bug in \"git log --graph -p -m\" (version 1.7.7.6)","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-02-06T15:03:00Z","receivedAt":"2013-02-06T15:03:00Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"> From: Matthieu Moy \n> \n> In any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get\n> undless output. On the other hand, I get a slightly misformatted output:\n> \n> * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d)\n> |\\ Merge: 2c1e6a3 33e70e7\n> | | Author: Matthieu Moy \n> | | Date: Tue Feb 5 22:05:33 2013 +0100\n> | | \n> | | Commit S\n> | | \n> | | diff --git a/file b/file\n> | | index 6bb4d3e..afd2e75 100644\n> | | --- a/file\n> | | +++ b/file\n> | | @@ -1,4 +1,5 @@\n> | | 1\n> | | 1a\n> | | 2\n> | | +2a\n> | | 3\n> | | \n> commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)\n> | | Merge: 2c1e6a3 33e70e7\n> | | Author: Matthieu Moy \n> | | Date: Tue Feb 5 22:05:33 2013 +0100\n> \n> The second \"commit\" line (diff with second parent) doesn't have the\n> \"| |\" prefix, I don't think this is intentional.\n\nThe second \"commit\" line should start with \"| * \":\n\n> | | \n> | * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)\n> | | Merge: 2c1e6a3 33e70e7\n> | | Author: Matthieu Moy \n> | | Date: Tue Feb 5 22:05:33 2013 +0100\n\nDale\n"},{"id":"208811","messageId":"20130206151447.GZ1342@serenity.lan","threadId":"32835","inReplyTo":"201302061503.r16F30UA016375@freeze.ariadne.com","subject":"Re: Bug in \"git log --graph -p -m\" (version 1.7.7.6)","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-06T15:14:47Z","receivedAt":"2013-02-06T15:14:47Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Feb 06, 2013 at 10:03:00AM -0500, Dale R. Worley wrote:\n> > From: Matthieu Moy \n> > \n> > In any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get\n> > undless output. On the other hand, I get a slightly misformatted output:\n> > \n> > * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d)\n> > |\\ Merge: 2c1e6a3 33e70e7\n> > | | Author: Matthieu Moy \n> > | | Date: Tue Feb 5 22:05:33 2013 +0100\n> > | | \n> > | | Commit S\n> > | | \n> > | | diff --git a/file b/file\n> > | | index 6bb4d3e..afd2e75 100644\n> > | | --- a/file\n> > | | +++ b/file\n> > | | @@ -1,4 +1,5 @@\n> > | | 1\n> > | | 1a\n> > | | 2\n> > | | +2a\n> > | | 3\n> > | | \n> > commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)\n> > | | Merge: 2c1e6a3 33e70e7\n> > | | Author: Matthieu Moy \n> > | | Date: Tue Feb 5 22:05:33 2013 +0100\n> > \n> > The second \"commit\" line (diff with second parent) doesn't have the\n> > \"| |\" prefix, I don't think this is intentional.\n> \n> The second \"commit\" line should start with \"| * \":\n\nNo. That would indicate a commit on the branch that is the second\nparent of the first commit. But this is the same commit as the one\nabove, just with a diff against its second parent instead of its first\nparent.\n\nI would argue that the line should start with \"| | \", since it really is\njust a continuation of the same commit.\n\n| | \n| | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)\n| | Merge: 2c1e6a3 33e70e7\n| | Author: Matthieu Moy \n| | Date: Tue Feb 5 22:05:33 2013 +0100\n\n\nJohn\n"},{"id":"208841","messageId":"vpqbobxwavv.fsf@grenoble-inp.fr","threadId":"32835","inReplyTo":"20130206151447.GZ1342@serenity.lan","subject":"Re: Bug in \"git log --graph -p -m\" (version 1.7.7.6)","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-02-06T18:33:08Z","receivedAt":"2013-02-06T18:33:08Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"John Keeping writes:\n\n> I would argue that the line should start with \"| | \", since it really is\n> just a continuation of the same commit.\n>\n> | | \n> | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)\n> | | Merge: 2c1e6a3 33e70e7\n> | | Author: Matthieu Moy \n> | | Date: Tue Feb 5 22:05:33 2013 +0100\n\nYes.\n\nI had a look at the code, I guess the call to graph_show_commit() in\nshow_log() (in log-tree.c) should have called graph_show_padding() but\ndidn't in this case. Then I got lost in graph.c :-(.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"208852","messageId":"20130206195702.GA1342@serenity.lan","threadId":"32835","inReplyTo":"vpqbobxwavv.fsf@grenoble-inp.fr","subject":"[PATCH] graph: output padding for merge subsequent parents","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-06T19:57:02Z","receivedAt":"2013-02-06T19:57:02Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Feb 06, 2013 at 07:33:08PM +0100, Matthieu Moy wrote:\n> John Keeping writes:\n> \n> > I would argue that the line should start with \"| | \", since it really is\n> > just a continuation of the same commit.\n> >\n> > | | \n> > | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8)\n> > | | Merge: 2c1e6a3 33e70e7\n> > | | Author: Matthieu Moy \n> > | | Date: Tue Feb 5 22:05:33 2013 +0100\n> \n> Yes.\n> \n> I had a look at the code, I guess the call to graph_show_commit() in\n> show_log() (in log-tree.c) should have called graph_show_padding() but\n> didn't in this case. Then I got lost in graph.c :-(.\n\nI think this is the correct answer. But now I've found that \"git log\n--graph -c -p\" doesn't indent the diff - that seems to be a separate\nissue.\n\n-- >8 --\n\nWhen showing merges in git-log, the same commit is shown once for each\nparent. Combined with \"--graph\" this results in graph_show_commit()\nbeing called once for each parent without graph_update() being called.\n\nCurrently graph_show_commit() does not print anything on subsequent\ninvocations for the same commit (this was changed by commit 656197a -\n\"graph.c: infinite loop in git whatchanged --graph -m\" from the previous\nbehaviour of looping infinitely).\n\nChange this so that if the graph code believes it has already shown the\ncommit it prints a single padding line.\n\nSigned-off-by: John Keeping \n---\n graph.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/graph.c b/graph.c\nindex 391a712..2a3fc5c 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -1227,6 +1227,16 @@ void graph_show_commit(struct git_graph *graph)\n \tif (!graph)\n \t\treturn;\n \n+\t/*\n+\t * When showing a diff of a merge against each of its parents, we\n+\t * are called once for each parent without graph_update having been\n+\t * called. In this case, simply output a single padding line.\n+\t */\n+\tif (graph_is_commit_finished(graph)) {\n+\t\tgraph_show_padding(graph);\n+\t\tshown_commit_line = 1;\n+\t}\n+\n \twhile (!shown_commit_line && !graph_is_commit_finished(graph)) {\n \t\tshown_commit_line = graph_next_line(graph, &msgbuf);\n \t\tfwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);\n-- \n1.8.1.2\n"},{"id":"209011","messageId":"vpqtxpmae1p.fsf@grenoble-inp.fr","threadId":"32835","inReplyTo":"20130206195702.GA1342@serenity.lan","subject":"Re: [PATCH] graph: output padding for merge subsequent parents","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-02-08T17:52:34Z","receivedAt":"2013-02-08T17:52:34Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"John Keeping writes:\n\n> diff --git a/graph.c b/graph.c\n> index 391a712..2a3fc5c 100644\n> --- a/graph.c\n> +++ b/graph.c\n> @@ -1227,6 +1227,16 @@ void graph_show_commit(struct git_graph *graph)\n> \tif (!graph)\n> \t\treturn;\n> \n> +\t/*\n> +\t * When showing a diff of a merge against each of its parents, we\n> +\t * are called once for each parent without graph_update having been\n> +\t * called. In this case, simply output a single padding line.\n> +\t */\n> +\tif (graph_is_commit_finished(graph)) {\n> +\t\tgraph_show_padding(graph);\n> +\t\tshown_commit_line = 1;\n> +\t}\n> +\n> \twhile (!shown_commit_line && !graph_is_commit_finished(graph)) {\n\nThis works, but if we know we're not going to enter the while loop, it\nseams even easier to do this:\n\n--- a/graph.c\n+++ b/graph.c\n@@ -1227,7 +1227,17 @@ void graph_show_commit(struct git_graph *graph)\n if (!graph)\n return;\n \n- while (!shown_commit_line && !graph_is_commit_finished(graph)) {\n+ /*\n+ * When showing a diff of a merge against each of its parents, we\n+ * are called once for each parent without graph_update having been\n+ * called. In this case, simply output a single padding line.\n+ */\n+ if (graph_is_commit_finished(graph)) {\n+ graph_show_padding(graph);\n+ return;\n+ }\n+\n+ while (!shown_commit_line) {\n shown_commit_line = graph_next_line(graph, &msgbuf);\n fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);\n if (!shown_commit_line)\n\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"209026","messageId":"7vd2wa1tno.fsf@alter.siamese.dyndns.org","threadId":"32835","inReplyTo":"vpqtxpmae1p.fsf@grenoble-inp.fr","subject":"Re: [PATCH] graph: output padding for merge subsequent parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-08T19:40:11Z","receivedAt":"2013-02-08T19:40:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> John Keeping writes:\n>\n>> diff --git a/graph.c b/graph.c\n>> index 391a712..2a3fc5c 100644\n>> --- a/graph.c\n>> +++ b/graph.c\n>> @@ -1227,6 +1227,16 @@ void graph_show_commit(struct git_graph *graph)\n>> \tif (!graph)\n>> \t\treturn;\n>> \n>> +\t/*\n>> +\t * When showing a diff of a merge against each of its parents, we\n>> +\t * are called once for each parent without graph_update having been\n>> +\t * called. In this case, simply output a single padding line.\n>> +\t */\n>> +\tif (graph_is_commit_finished(graph)) {\n>> +\t\tgraph_show_padding(graph);\n>> +\t\tshown_commit_line = 1;\n>> +\t}\n>> +\n>> \twhile (!shown_commit_line && !graph_is_commit_finished(graph)) {\n>\n> This works, but if we know we're not going to enter the while loop, it\n> seams even easier to do this:\n>\n> --- a/graph.c\n> +++ b/graph.c\n> @@ -1227,7 +1227,17 @@ void graph_show_commit(struct git_graph *graph)\n> if (!graph)\n> return;\n> \n> - while (!shown_commit_line && !graph_is_commit_finished(graph)) {\n> + /*\n> + * When showing a diff of a merge against each of its parents, we\n> + * are called once for each parent without graph_update having been\n> + * called. In this case, simply output a single padding line.\n> + */\n> + if (graph_is_commit_finished(graph)) {\n> + graph_show_padding(graph);\n> + return;\n> + }\n> +\n> + while (!shown_commit_line) {\n> shown_commit_line = graph_next_line(graph, &msgbuf);\n> fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);\n> if (!shown_commit_line)\n\nIn this particular case, with the current state of this function, it\nis probably OK, but an early return like this tend to be a source of\nfuture bugs in the longer term, to make the codeflow skip whatever\nnecessary clean-up that needs to be done after the loop exits.\n"}]}