{"thread":{"id":"31628","subject":"[PATCH] graph: avoid infinite loop in graph_show_commit()","startedAt":"2012-09-22T14:24:40Z","lastAt":"2012-09-25T18:11:17Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Michal Kiedrowicz","Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"199739","messageId":"1348323880-3751-1-git-send-email-pclouds@gmail.com","threadId":"31628","inReplyTo":null,"subject":"[PATCH] graph: avoid infinite loop in graph_show_commit()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-09-22T14:24:40Z","receivedAt":"2012-09-22T14:24:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The loop can be triggered with \"git diff-tree --graph commit\" where\nthe commit is a non-merge. It goes like this\n\n - graph_show_commit\n - graph_next_line\n - graph_output_padding_line\n\nThe last function quits because graph->commit is NULL, but\ngraph_next_line() does not return \"shown\", so the loop in\ngraph_show_commit keeps going.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Corner case. Nobody sane would do that. But still worth plugging.\n\n graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/graph.c b/graph.c\nindex e864fe2..1735b26 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -1224,7 +1224,7 @@ void graph_show_commit(struct git_graph *graph)\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint shown_commit_line = 0;\n \n-\tif (!graph)\n+\tif (!graph || !graph->commit)\n \t\treturn;\n \n \twhile (!shown_commit_line) {\n-- \n1.7.12.1.389.gc2218b5\n"},{"id":"199769","messageId":"loom.20120923T135253-178@post.gmane.org","threadId":"31628","inReplyTo":"1348323880-3751-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] graph: avoid infinite loop in graph_show_commit()","fromName":"Michal Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-09-23T11:55:50Z","receivedAt":"2012-09-23T11:55:50Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Nguyễn Thái Ngọc Duy gmail.com> writes:\n\n> \n> The loop can be triggered with \"git diff-tree --graph commit\" where\n> the commit is a non-merge. It goes like this\n\n\nIsn't this the same issue as in \nhttp://article.gmane.org/gmane.comp.version-control.git/123979\n? (with slightly different fix)\n"},{"id":"199771","messageId":"CACsJy8ApYKOU8v_-HkUC5uOb8gsheugKaXKMjbm0_-ygW_4jiQ@mail.gmail.com","threadId":"31628","inReplyTo":"loom.20120923T135253-178@post.gmane.org","subject":"Re: [PATCH] graph: avoid infinite loop in graph_show_commit()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-09-23T12:14:51Z","receivedAt":"2012-09-23T12:14:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Sep 23, 2012 at 6:55 PM, Michal Kiedrowicz\n wrote:\n> Nguyễn Thái Ngọc Duy gmail.com> writes:\n>\n>>\n>> The loop can be triggered with \"git diff-tree --graph commit\" where\n>> the commit is a non-merge. It goes like this\n>\n>\n> Isn't this the same issue as in\n> http://article.gmane.org/gmane.comp.version-control.git/123979\n> ? (with slightly different fix)\n\nI don't know. I'm not familiar enough with graph.c to tell. Maybe Adam\ncan have a look?\n\nThe patch that is cut out is\nhttp://article.gmane.org/gmane.comp.version-control.git/206205\n-- \nDuy\n"},{"id":"199866","messageId":"7vtxunvvr4.fsf@alter.siamese.dyndns.org","threadId":"31628","inReplyTo":"CACsJy8ApYKOU8v_-HkUC5uOb8gsheugKaXKMjbm0_-ygW_4jiQ@mail.gmail.com","subject":"Re: [PATCH] graph: avoid infinite loop in graph_show_commit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-24T23:36:31Z","receivedAt":"2012-09-24T23:36:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On Sun, Sep 23, 2012 at 6:55 PM, Michal Kiedrowicz\n> wrote:\n>> Nguyễn Thái Ngọc Duy gmail.com> writes:\n>>\n>>>\n>>> The loop can be triggered with \"git diff-tree --graph commit\" where\n>>> the commit is a non-merge. It goes like this\n>>\n>>\n>> Isn't this the same issue as in\n>> http://article.gmane.org/gmane.comp.version-control.git/123979\n>> ? (with slightly different fix)\n>\n> I don't know. I'm not familiar enough with graph.c to tell. Maybe Adam\n> can have a look?\n\nHas either of you tried the patch with the problematic case the\nother patch tries to solve? Michal's old patch does smell like it\nis going in the better direction in that it stops looping when we\nknow we would only be showing the padding, which is a sign that we\nare done with showing the commit.\n\nBut I didn't look at it too closely. I'd prefer to see the\nassert(0) turned into die(\"BUG: internal error\") at the end of\ngraph_next_line() to catch these cases. Also I am not sure if\nassignment of the return value from graph_next_line() to\nshown_comit_line in the loop is correct (shouldn't it be OR'ing it\nin, so that \"we have shown the information on this commit\" is not\nlost when the function adds things after showing the commit???)\n"},{"id":"199901","messageId":"7v6272vupm.fsf@alter.siamese.dyndns.org","threadId":"31628","inReplyTo":"7vtxunvvr4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] graph: avoid infinite loop in graph_show_commit()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-25T18:11:17Z","receivedAt":"2012-09-25T18:11:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Has either of you tried the patch with the problematic case the\n> other patch tries to solve? Michal's old patch does smell like it\n> is going in the better direction in that it stops looping when we\n> know we would only be showing the padding, which is a sign that we\n> are done with showing the commit.\n\nI think this should suffice. I do not know if Michal's patch is the\nright fix, though. It appears to me that \"--graph\" assumes one\ncommit is shown only once, but \"diff-tree -m\" and friends want to\nshow a merge commit twice and is fundamentally incompatible with the\nassumption. We might be off either fixing that in the \"graph\" code\n(not with a band-aid like patches from you two to make it punt), or\nforbidding the combination altogether.\n\n\n t/t4202-log.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 71be59d..14f73e3 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -264,6 +264,16 @@ test_expect_success 'log --graph with merge' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log --raw --graph -m with merge' '\n+\tgit log --raw --graph --oneline -m master | head -n 500 >actual &&\n+\tgrep \"initial\" actual\n+'\n+\n+test_expect_success 'diff-tree --graph' '\n+\tgit diff-tree --graph master^ | head -n 500 >actual &&\n+\tgrep \"one\" actual\n+'\n+\n cat > expect <<\\EOF\n * commit master\n |\\ Merge: A B\n-- \n1.7.12.1.451.gb433296\n"}]}