{"thread":{"id":"20657","subject":"RE: interaction between --graph and --simplify-by-decoration","startedAt":"2009-08-18T20:55:54Z","lastAt":"2009-08-21T21:23:26Z","messageCount":14,"participants":["Adam Simpkins","Junio C Hamano","Santi Béjar"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"121163","messageId":"1250628954.114121983@192.168.1.201","threadId":"20657","inReplyTo":null,"subject":"RE: interaction between --graph and --simplify-by-decoration","fromName":"Adam Simpkins","fromEmail":"adam@adamsimpkins.net","sentAt":"2009-08-18T20:55:54Z","receivedAt":"2009-08-18T20:55:54Z","isPatch":false,"sender":{"key":"adam@adamsimpkins.net","avatar":"https://gravatar.com/avatar/d3fd2c0b3e2d2136b56e95726ee03227bee4eb562f627dcd3ebca0623fa05054?d=mp&s=160"},"body":"On Friday, July 31, 2009 4:11am, \"Santi Béjar\" <santi@agolina.net> said:\n> Hello,\n> \n>   I've found that in some cases the --graph and\n> --simplify-by-decoration don't work well together. If you do this in\n> the git.git repository:\n\nThanks for reporting the problem, I apologize for taking so long to\ninvestigate and respond.\n\n\n> * | | f29ac4f (tag: v1.6.3-rc2) GIT 1.6.3-rc2\n>  / /\n> | | *   66996ec Sync with 1.6.2.4\n\n> you can see that f29ac4f looks like it does not have any parents while\n> the correct parent is 66996ec which seems to have no children. But if\n> you omit the --oneline you can see that there are a lot of \"root\"-like\n> commits (f01f109, a48f5d7, f29ac4f,...).\n\nYes, there's a bug in graph_is_interesting().  When processing\nf29ac4f, the graph code thinks that 66996ec isn't interesting and\nwon't get displayed in the output, so it doesn't prepare the graph\nlines to show lines to 66996ec.\n\nI'll submit a patch shortly.\n\n--\nAdam Simpkins\nadam@adamsimpkins.net\n"},{"id":"121184","messageId":"20090818211812.GL8147@facebook.com","threadId":"20657","inReplyTo":"1250628954.114121983@192.168.1.201","subject":"[PATCH] graph API: fix bug in graph_is_interesting()","fromName":"Adam Simpkins","fromEmail":"simpkins@facebook.com","sentAt":"2009-08-18T21:18:12Z","receivedAt":"2009-08-18T21:18:12Z","isPatch":true,"sender":{"key":"simpkins@facebook.com","avatar":null},"body":"Updated graph_is_interesting() to use simplify_commit() to determine if\na commit is interesting, just like get_revision() does.  Previously, it\nwould sometimes incorrectly treat an interesting commit as\nuninteresting.  This resulted in incorrect lines in the graph output.\n\nThis problem was reported by Santi Béjar.  The following command\nwould exhibit the problem before, but now works correctly:\n\n  git log --graph --simplify-by-decoration --oneline v1.6.3.3\n\nPreviously git graph did not display the output for this command\ncorrectly between f29ac4f and 66996ec, among other places.\n\nSigned-off-by: Adam Simpkins <simpkins@facebook.com>\n---\n\nNote that simplify_commit() may modify the revision list.  Calling it\nin graph_is_interesting() can modify the revision list earlier than it\notherwise would be (in get_revision()).  I don't think this should\ncause any problems, but figured I'd point it out in case anyone more\nfamiliar with the code thinks otherwise.\n\n\n graph.c |    5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex e466770..ea21e91 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -286,9 +286,10 @@ static int graph_is_interesting(struct git_graph *graph, struct commit *commit)\n \t}\n \n \t/*\n-\t * Uninteresting and pruned commits won't be printed\n+\t * Otherwise, use simplify_commit() to see if this commit is\n+\t * interesting\n \t */\n-\treturn (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1;\n+\treturn simplify_commit(graph->revs, commit) == commit_show;\n }\n \n static struct commit_list *next_interesting_parent(struct git_graph *graph,\n-- \n1.6.4.314.ge5db\n"},{"id":"121191","messageId":"7vk5103chi.fsf@alter.siamese.dyndns.org","threadId":"20657","inReplyTo":"20090818211812.GL8147@facebook.com","subject":"Re: [PATCH] graph API: fix bug in graph_is_interesting()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-18T23:53:45Z","receivedAt":"2009-08-18T23:53:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Simpkins <simpkins@facebook.com> writes:\n\n> -\treturn (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1;\n> +\treturn simplify_commit(graph->revs, commit) == commit_show;\n\nIf you do this after revision.c finished the traversal (e.g. \"limited\"\ncase), I think it should be Ok.\n\nBut calling simplify_commit() while the traversal is still in progress is\nasking for trouble.  I do not recall the details anymore but when I tried\nto make the \"simplify-merges\" algorithm incremental, I had seen funny\nbreakage caused by calling simplify_commit() twice on the same commit.\n\nI suspect that this change will break the primary traversal.\n"},{"id":"121204","messageId":"20090819022918.GO8147@facebook.com","threadId":"20657","inReplyTo":"7vk5103chi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] graph API: fix bug in graph_is_interesting()","fromName":"Adam Simpkins","fromEmail":"simpkins@facebook.com","sentAt":"2009-08-19T02:29:18Z","receivedAt":"2009-08-19T02:29:18Z","isPatch":true,"sender":{"key":"simpkins@facebook.com","avatar":null},"body":"On Tue, Aug 18, 2009 at 04:53:45PM -0700, Junio C Hamano wrote:\n> Adam Simpkins <simpkins@facebook.com> writes:\n> \n> > -\treturn (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1;\n> > +\treturn simplify_commit(graph->revs, commit) == commit_show;\n> \n> If you do this after revision.c finished the traversal (e.g. \"limited\"\n> case), I think it should be Ok.\n> \n> But calling simplify_commit() while the traversal is still in progress is\n> asking for trouble.  I do not recall the details anymore but when I tried\n> to make the \"simplify-merges\" algorithm incremental, I had seen funny\n> breakage caused by calling simplify_commit() twice on the same commit.\n> \n> I suspect that this change will break the primary traversal.\n\nThe --graph option always enables revs->topo_order, which in turn\nenables revs->limited, so this should always be after limit_list() has\nalready been called.\n\nHowever, it looks like the call to rewrite_parents() is the only\nmodifying operation in simplify_commit(), and all the rest can easily\nbe split out into a separate helper function.  I'll submit another\nversion of the patch that makes the non-modifying simplify_commit()\nbehavior available as a separate function.\n\n-- \nAdam Simpkins\nsimpkins@facebook.com\n"},{"id":"121205","messageId":"20090819023433.GP8147@facebook.com","threadId":"20657","inReplyTo":"20090819022918.GO8147@facebook.com","subject":"Re: [PATCH] graph API: fix bug in graph_is_interesting()","fromName":"Adam Simpkins","fromEmail":"simpkins@facebook.com","sentAt":"2009-08-19T02:34:33Z","receivedAt":"2009-08-19T02:34:33Z","isPatch":true,"sender":{"key":"simpkins@facebook.com","avatar":null},"body":"Previously, graph_is_interesting() did not behave quite the same way as\nthe code in get_revision().  As a result, it would sometimes think\ncommits were uninteresting, even though get_revision() would return\nthem.  This resulted in incorrect lines in the graph output.\n\nThis change creates a get_commit_action() function, which\ngraph_is_interesting() and simplify_commit() both now use to determine\nif a commit will be shown.  It is identical to the old simplify_commit()\nbehavior, except that it never calls rewrite_parents().\n\nThis problem was reported by Santi Béjar.  The following command\nwould exhibit the problem before, but now works correctly:\n\n  git log --graph --simplify-by-decoration --oneline v1.6.3.3\n\nPreviously git graph did not display the output for this command\ncorrectly between f29ac4f and 66996ec, among other places.\n\nSigned-off-by: Adam Simpkins <simpkins@facebook.com>\n---\n graph.c    |    5 +++--\n revision.c |   15 ++++++++++++---\n revision.h |    1 +\n 3 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex e466770..9087f65 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -286,9 +286,10 @@ static int graph_is_interesting(struct git_graph *graph, struct commit *commit)\n \t}\n \n \t/*\n-\t * Uninteresting and pruned commits won't be printed\n+\t * Otherwise, use get_commit_action() to see if this commit is\n+\t * interesting\n \t */\n-\treturn (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1;\n+\treturn get_commit_action(graph->revs, commit) == commit_show;\n }\n \n static struct commit_list *next_interesting_parent(struct git_graph *graph,\ndiff --git a/revision.c b/revision.c\nindex 8ffb661..fe7d522 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1664,7 +1664,7 @@ static inline int want_ancestry(struct rev_info *revs)\n \treturn (revs->rewrite_parents || revs->children.name);\n }\n \n-enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n+enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)\n {\n \tif (commit->object.flags & SHOWN)\n \t\treturn commit_ignore;\n@@ -1692,12 +1692,21 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n \t\t\tif (!commit->parents || !commit->parents->next)\n \t\t\t\treturn commit_ignore;\n \t\t}\n-\t\tif (want_ancestry(revs) && rewrite_parents(revs, commit) < 0)\n-\t\t\treturn commit_error;\n \t}\n \treturn commit_show;\n }\n \n+enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n+{\n+\tenum commit_action action = get_commit_action(revs, commit);\n+\n+\tif (action == commit_show && revs->prune && revs->dense && want_ancestry(revs)) {\n+\t\tif (rewrite_parents(revs, commit) < 0)\n+\t\t\treturn commit_error;\n+\t}\n+\treturn action;\n+}\n+\n static struct commit *get_revision_1(struct rev_info *revs)\n {\n \tif (!revs->commits)\ndiff --git a/revision.h b/revision.h\nindex b10984b..9d0dddb 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -168,6 +168,7 @@ enum commit_action {\n \tcommit_error\n };\n \n+extern enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit);\n extern enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit);\n \n #endif\n-- \n1.6.4.314.g0a482.dirty\n"},{"id":"121212","messageId":"7vhbw41g3f.fsf@alter.siamese.dyndns.org","threadId":"20657","inReplyTo":"20090819023433.GP8147@facebook.com","subject":"Re: [PATCH] graph API: fix bug in graph_is_interesting()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-19T06:18:44Z","receivedAt":"2009-08-19T06:18:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Simpkins <simpkins@facebook.com> writes:\n\n> -enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n> +enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)\n>  {\n>  \tif (commit->object.flags & SHOWN)\n>  \t\treturn commit_ignore;\n> @@ -1692,12 +1692,21 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n>  \t\t\tif (!commit->parents || !commit->parents->next)\n>  \t\t\t\treturn commit_ignore;\n>  \t\t}\n> -\t\tif (want_ancestry(revs) && rewrite_parents(revs, commit) < 0)\n> -\t\t\treturn commit_error;\n>  \t}\n>  \treturn commit_show;\n>  }\n>  \n> +enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n> +{\n> +\tenum commit_action action = get_commit_action(revs, commit);\n> +\n> +\tif (action == commit_show && revs->prune && revs->dense && want_ancestry(revs)) {\n> +\t\tif (rewrite_parents(revs, commit) < 0)\n> +\t\t\treturn commit_error;\n> +\t}\n> +\treturn action;\n> +}\n\nWhen simplify_commit() logic (now called get_comit_action()) decides to\nshow this commit because revs->show_all was specified, we did not rewrite\nits parents, but now we will?\n"},{"id":"121213","messageId":"7v4os41frm.fsf@alter.siamese.dyndns.org","threadId":"20657","inReplyTo":"7vhbw41g3f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] graph API: fix bug in graph_is_interesting()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-19T06:25:49Z","receivedAt":"2009-08-19T06:25:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Adam Simpkins <simpkins@facebook.com> writes:\n>\n>> -enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n>> +enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)\n>>  {\n>>  \tif (commit->object.flags & SHOWN)\n>>  \t\treturn commit_ignore;\n>> @@ -1692,12 +1692,21 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n>>  \t\t\tif (!commit->parents || !commit->parents->next)\n>>  \t\t\t\treturn commit_ignore;\n>>  \t\t}\n>> -\t\tif (want_ancestry(revs) && rewrite_parents(revs, commit) < 0)\n>> -\t\t\treturn commit_error;\n>>  \t}\n>>  \treturn commit_show;\n>>  }\n>>  \n>> +enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n>> +{\n>> +\tenum commit_action action = get_commit_action(revs, commit);\n>> +\n>> +\tif (action == commit_show && revs->prune && revs->dense && want_ancestry(revs)) {\n>> +\t\tif (rewrite_parents(revs, commit) < 0)\n>> +\t\t\treturn commit_error;\n>> +\t}\n>> +\treturn action;\n>> +}\n>\n> When simplify_commit() logic (now called get_comit_action()) decides to\n> show this commit because revs->show_all was specified, we did not rewrite\n> its parents, but now we will?\n\nThat is, here is what I meant...\n\n revision.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 15a2010..efa3b7c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1700,7 +1700,9 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n {\n \tenum commit_action action = get_commit_action(revs, commit);\n \n-\tif (action == commit_show && revs->prune && revs->dense && want_ancestry(revs)) {\n+\tif (action == commit_show &&\n+\t    !revs->show_all &&\n+\t    revs->prune && revs->dense && want_ancestry(revs)) {\n \t\tif (rewrite_parents(revs, commit) < 0)\n \t\t\treturn commit_error;\n \t}\n\nWe may want to add some tests to demonstrate the breakage this fix\naddresses.\n"},{"id":"121292","messageId":"20090819225547.GR8147@facebook.com","threadId":"20657","inReplyTo":"7v4os41frm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] graph API: fix bug in graph_is_interesting()","fromName":"Adam Simpkins","fromEmail":"simpkins@facebook.com","sentAt":"2009-08-19T22:55:47Z","receivedAt":"2009-08-19T22:55:47Z","isPatch":true,"sender":{"key":"simpkins@facebook.com","avatar":null},"body":"On Tue, Aug 18, 2009 at 11:25:49PM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> >\n> > When simplify_commit() logic (now called get_comit_action()) decides to\n> > show this commit because revs->show_all was specified, we did not rewrite\n> > its parents, but now we will?\n> \n> That is, here is what I meant...\n> \n> -\tif (action == commit_show && revs->prune && revs->dense && want_ancestry(revs)) {\n> +\tif (action == commit_show &&\n> +\t    !revs->show_all &&\n> +\t    revs->prune && revs->dense && want_ancestry(revs)) {\n> \n> We may want to add some tests to demonstrate the breakage this fix\n> addresses.\n\nYes, you're right.  Thanks for catching that.  I'll submit a test case\nthat checks this scenario.\n\n-- \nAdam Simpkins\nsimpkins@facebook.com\n"},{"id":"121293","messageId":"20090819225852.GA21187@facebook.com","threadId":"20657","inReplyTo":"20090819225547.GR8147@facebook.com","subject":"[PATCH] Add test case for rev-list --parents --show-all","fromName":"Adam Simpkins","fromEmail":"simpkins@facebook.com","sentAt":"2009-08-19T22:58:52Z","receivedAt":"2009-08-19T22:58:52Z","isPatch":true,"sender":{"key":"simpkins@facebook.com","avatar":null},"body":"This test case ensures that rev-list --parents --show-all gets the\nparent history correct.  Normally, --parents rewrites parent history to\nskip TREESAME parents.  However, --show-all causes TREESAME parents to\nstill be included in the revision list, so the parents should still be\nincluded too.\n\nSigned-off-by: Adam Simpkins <simpkins@facebook.com>\n---\n\nLooking through the code, I believe TREESAME commits are the only ones\naffected by my earlier bug in simplify_commit().\n\n t/t6015-rev-list-show-all-parents.sh |   31 +++++++++++++++++++++++++++++++\n 1 files changed, 31 insertions(+), 0 deletions(-)\n create mode 100644 t/t6015-rev-list-show-all-parents.sh\n\ndiff --git a/t/t6015-rev-list-show-all-parents.sh b/t/t6015-rev-list-show-all-parents.sh\nnew file mode 100644\nindex 0000000..8b146fb\n--- /dev/null\n+++ b/t/t6015-rev-list-show-all-parents.sh\n@@ -0,0 +1,31 @@\n+#!/bin/sh\n+\n+test_description='--show-all --parents does not rewrite TREESAME commits'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'set up --show-all --parents test' '\n+\ttest_commit one foo.txt &&\n+\tcommit1=`git rev-list -1 HEAD` &&\n+\ttest_commit two bar.txt &&\n+\tcommit2=`git rev-list -1 HEAD` &&\n+\ttest_commit three foo.txt &&\n+\tcommit3=`git rev-list -1 HEAD`\n+\t'\n+\n+test_expect_success '--parents rewrites TREESAME parents correctly' '\n+\techo $commit3 $commit1 > expected &&\n+\techo $commit1 >> expected &&\n+\tgit rev-list --parents HEAD -- foo.txt > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+test_expect_success '--parents --show-all does not rewrites TREESAME parents' '\n+\techo $commit3 $commit2 > expected &&\n+\techo $commit2 $commit1 >> expected &&\n+\techo $commit1 >> expected &&\n+\tgit rev-list --parents --show-all HEAD -- foo.txt > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+test_done\n-- \n1.6.0.4\n"},{"id":"121314","messageId":"7v7hwzt94p.fsf@alter.siamese.dyndns.org","threadId":"20657","inReplyTo":"20090819225852.GA21187@facebook.com","subject":"Re: [PATCH] Add test case for rev-list --parents --show-all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-20T04:13:58Z","receivedAt":"2009-08-20T04:13:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Simpkins <simpkins@facebook.com> writes:\n\n> This test case ensures that rev-list --parents --show-all gets the\n> parent history correct.  Normally, --parents rewrites parent history to\n> skip TREESAME parents.  However, --show-all causes TREESAME parents to\n> still be included in the revision list, so the parents should still be\n> included too.\n>\n> Signed-off-by: Adam Simpkins <simpkins@facebook.com>\n> ---\n>\n> Looking through the code, I believe TREESAME commits are the only ones\n> affected by my earlier bug in simplify_commit().\n\nWhat I meant was actually a test for the graph part (i.e. the problem we\nwould see if we did not apply your update to graph_is_interesting()), but\nprotecting the simplify_commit() logic with test from breakage is a good\nthing to do as well.\n\nThanks.\n"},{"id":"121440","messageId":"adf1fd3d0908210839s35da944co4ba4079f6216b44c@mail.gmail.com","threadId":"20657","inReplyTo":"20090819023433.GP8147@facebook.com","subject":"Re: [PATCH] graph API: fix bug in graph_is_interesting()","fromName":"Santi Béjar","fromEmail":"santi@agolina.net","sentAt":"2009-08-21T15:39:16Z","receivedAt":"2009-08-21T15:39:16Z","isPatch":true,"sender":{"key":"santi@agolina.net","avatar":null},"body":"On Wed, Aug 19, 2009 at 4:34 AM, Adam Simpkins<simpkins@facebook.com> wrote:\n> Previously, graph_is_interesting() did not behave quite the same way as\n> the code in get_revision().  As a result, it would sometimes think\n> commits were uninteresting, even though get_revision() would return\n> them.  This resulted in incorrect lines in the graph output.\n>\n> This change creates a get_commit_action() function, which\n> graph_is_interesting() and simplify_commit() both now use to determine\n> if a commit will be shown.  It is identical to the old simplify_commit()\n> behavior, except that it never calls rewrite_parents().\n>\n> This problem was reported by Santi Béjar.  The following command\n> would exhibit the problem before, but now works correctly:\n>\n>  git log --graph --simplify-by-decoration --oneline v1.6.3.3\n>\n> Previously git graph did not display the output for this command\n> correctly between f29ac4f and 66996ec, among other places.\n\nThanks, the fix works (no comments about the code, only the behavior).\n\nOne more thing Junio. In 99af022 (graph API: fix bug in\ngraph_is_interesting(), 2009-08-18) in 'pu' my name is mispelled, it\nseems it was interpreted as latin1, then recoded as UTF8, interpreted\nas latin and recoded to latin1, or in other words the é is 4 bytes\ninstead of two. I've checked this mail and it is latin1, correctly\nspecified in the headers.\n\nThanks,\nSanti\n"},{"id":"121446","messageId":"20090821182034.GW8147@facebook.com","threadId":"20657","inReplyTo":"7v7hwzt94p.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Add tests for rev-list --graph with options that simplify history","fromName":"Adam Simpkins","fromEmail":"simpkins@facebook.com","sentAt":"2009-08-21T18:20:34Z","receivedAt":"2009-08-21T18:20:34Z","isPatch":true,"sender":{"key":"simpkins@facebook.com","avatar":null},"body":"These tests help make sure graph_is_interesting() is doing the right\nthing.\n---\n t/t6016-rev-list-graph-simplify-history.sh |  276 ++++++++++++++++++++++++++++\n 1 files changed, 276 insertions(+), 0 deletions(-)\n create mode 100755 t/t6016-rev-list-graph-simplify-history.sh\n\ndiff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh\nnew file mode 100755\nindex 0000000..5ac8fc9\n--- /dev/null\n+++ b/t/t6016-rev-list-graph-simplify-history.sh\n@@ -0,0 +1,276 @@\n+#!/bin/sh\n+\n+# There's more than one \"correct\" way to represent the history graphically.\n+# These tests depend on the current behavior of the graphing code.  If the\n+# graphing code is ever changed to draw the output differently, these tests\n+# cases will need to be updated to know about the new layout.\n+\n+test_description='--graph and simplified history'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'set up rev-list --graph test' '\n+\t# 3 commits on branch A\n+\ttest_commit A1 foo.txt &&\n+\ttest_commit A2 bar.txt &&\n+\ttest_commit A3 bar.txt &&\n+\tgit branch -m master A &&\n+\n+\t# 2 commits on branch B, started from A1\n+\tgit checkout -b B A1 &&\n+\ttest_commit B1 foo.txt &&\n+\ttest_commit B2 abc.txt &&\n+\n+\t# 2 commits on branch C, started from A2\n+\tgit checkout -b C A2 &&\n+\ttest_commit C1 xyz.txt &&\n+\ttest_commit C2 xyz.txt &&\n+\n+\t# Octopus merge B and C into branch A\n+\tgit checkout A &&\n+\tgit merge B C &&\n+\tgit tag A4\n+\n+\ttest_commit A5 bar.txt &&\n+\n+\t# More commits on C, then merge C into A\n+\tgit checkout C &&\n+\ttest_commit C3 foo.txt &&\n+\ttest_commit C4 bar.txt &&\n+\tgit checkout A &&\n+\tgit merge -s ours C &&\n+\tgit tag A6\n+\n+\ttest_commit A7 bar.txt &&\n+\n+\t# Store commit names in variables for later use\n+\tA1=`git rev-list -1 A1` &&\n+\tA2=`git rev-list -1 A2` &&\n+\tA3=`git rev-list -1 A3` &&\n+\tA4=`git rev-list -1 A4` &&\n+\tA5=`git rev-list -1 A5` &&\n+\tA6=`git rev-list -1 A6` &&\n+\tA7=`git rev-list -1 A7` &&\n+\tB1=`git rev-list -1 B1` &&\n+\tB2=`git rev-list -1 B2` &&\n+\tC1=`git rev-list -1 C1` &&\n+\tC2=`git rev-list -1 C2` &&\n+\tC3=`git rev-list -1 C3` &&\n+\tC4=`git rev-list -1 C4`\n+\t'\n+\n+test_expect_success '--graph --all' '\n+\trm -f expected &&\n+\techo \"* $A7\" >> expected &&\n+\techo \"*   $A6\" >> expected &&\n+\techo \"|\\\\  \" >> expected &&\n+\techo \"| * $C4\" >> expected &&\n+\techo \"| * $C3\" >> expected &&\n+\techo \"* | $A5\" >> expected &&\n+\techo \"| |     \" >> expected &&\n+\techo \"|  \\\\    \" >> expected &&\n+\techo \"*-. \\\\   $A4\" >> expected &&\n+\techo \"|\\\\ \\\\ \\\\  \" >> expected &&\n+\techo \"| | |/  \" >> expected &&\n+\techo \"| | * $C2\" >> expected &&\n+\techo \"| | * $C1\" >> expected &&\n+\techo \"| * | $B2\" >> expected &&\n+\techo \"| * | $B1\" >> expected &&\n+\techo \"* | | $A3\" >> expected &&\n+\techo \"| |/  \" >> expected &&\n+\techo \"|/|   \" >> expected &&\n+\techo \"* | $A2\" >> expected &&\n+\techo \"|/  \" >> expected &&\n+\techo \"* $A1\" >> expected &&\n+\tgit rev-list --graph --all > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+# Make sure the graph_is_interesting() code still realizes\n+# that undecorated merges are interesting, even with --simplify-by-decoration\n+test_expect_success '--graph --simplify-by-decoration' '\n+\trm -f expected &&\n+\tgit tag -d A4\n+\techo \"* $A7\" >> expected &&\n+\techo \"*   $A6\" >> expected &&\n+\techo \"|\\\\  \" >> expected &&\n+\techo \"| * $C4\" >> expected &&\n+\techo \"| * $C3\" >> expected &&\n+\techo \"* | $A5\" >> expected &&\n+\techo \"| |     \" >> expected &&\n+\techo \"|  \\\\    \" >> expected &&\n+\techo \"*-. \\\\   $A4\" >> expected &&\n+\techo \"|\\\\ \\\\ \\\\  \" >> expected &&\n+\techo \"| | |/  \" >> expected &&\n+\techo \"| | * $C2\" >> expected &&\n+\techo \"| | * $C1\" >> expected &&\n+\techo \"| * | $B2\" >> expected &&\n+\techo \"| * | $B1\" >> expected &&\n+\techo \"* | | $A3\" >> expected &&\n+\techo \"| |/  \" >> expected &&\n+\techo \"|/|   \" >> expected &&\n+\techo \"* | $A2\" >> expected &&\n+\techo \"|/  \" >> expected &&\n+\techo \"* $A1\" >> expected &&\n+\tgit rev-list --graph --all --simplify-by-decoration > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+# Get rid of all decorations on branch B, and graph with it simplified away\n+test_expect_success '--graph --simplify-by-decoration prune branch B' '\n+\trm -f expected &&\n+\tgit tag -d B2\n+\tgit tag -d B1\n+\tgit branch -d B\n+\techo \"* $A7\" >> expected &&\n+\techo \"*   $A6\" >> expected &&\n+\techo \"|\\\\  \" >> expected &&\n+\techo \"| * $C4\" >> expected &&\n+\techo \"| * $C3\" >> expected &&\n+\techo \"* | $A5\" >> expected &&\n+\techo \"* |   $A4\" >> expected &&\n+\techo \"|\\\\ \\\\  \" >> expected &&\n+\techo \"| |/  \" >> expected &&\n+\techo \"| * $C2\" >> expected &&\n+\techo \"| * $C1\" >> expected &&\n+\techo \"* | $A3\" >> expected &&\n+\techo \"|/  \" >> expected &&\n+\techo \"* $A2\" >> expected &&\n+\techo \"* $A1\" >> expected &&\n+\tgit rev-list --graph --simplify-by-decoration --all > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+test_expect_success '--graph --full-history -- bar.txt' '\n+\trm -f expected &&\n+\tgit tag -d B2\n+\tgit tag -d B1\n+\tgit branch -d B\n+\techo \"* $A7\" >> expected &&\n+\techo \"*   $A6\" >> expected &&\n+\techo \"|\\\\  \" >> expected &&\n+\techo \"| * $C4\" >> expected &&\n+\techo \"* | $A5\" >> expected &&\n+\techo \"* |   $A4\" >> expected &&\n+\techo \"|\\\\ \\\\  \" >> expected &&\n+\techo \"| |/  \" >> expected &&\n+\techo \"* | $A3\" >> expected &&\n+\techo \"|/  \" >> expected &&\n+\techo \"* $A2\" >> expected &&\n+\tgit rev-list --graph --full-history --all -- bar.txt > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+test_expect_success '--graph --full-history --simplify-merges -- bar.txt' '\n+\trm -f expected &&\n+\tgit tag -d B2\n+\tgit tag -d B1\n+\tgit branch -d B\n+\techo \"* $A7\" >> expected &&\n+\techo \"*   $A6\" >> expected &&\n+\techo \"|\\\\  \" >> expected &&\n+\techo \"| * $C4\" >> expected &&\n+\techo \"* | $A5\" >> expected &&\n+\techo \"* | $A3\" >> expected &&\n+\techo \"|/  \" >> expected &&\n+\techo \"* $A2\" >> expected &&\n+\tgit rev-list --graph --full-history --simplify-merges --all \\\n+\t\t-- bar.txt > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+test_expect_success '--graph -- bar.txt' '\n+\trm -f expected &&\n+\tgit tag -d B2\n+\tgit tag -d B1\n+\tgit branch -d B\n+\techo \"* $A7\" >> expected &&\n+\techo \"* $A5\" >> expected &&\n+\techo \"* $A3\" >> expected &&\n+\techo \"| * $C4\" >> expected &&\n+\techo \"|/  \" >> expected &&\n+\techo \"* $A2\" >> expected &&\n+\tgit rev-list --graph --all -- bar.txt > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+test_expect_success '--graph --sparse -- bar.txt' '\n+\trm -f expected &&\n+\tgit tag -d B2\n+\tgit tag -d B1\n+\tgit branch -d B\n+\techo \"* $A7\" >> expected &&\n+\techo \"* $A6\" >> expected &&\n+\techo \"* $A5\" >> expected &&\n+\techo \"* $A4\" >> expected &&\n+\techo \"* $A3\" >> expected &&\n+\techo \"| * $C4\" >> expected &&\n+\techo \"| * $C3\" >> expected &&\n+\techo \"| * $C2\" >> expected &&\n+\techo \"| * $C1\" >> expected &&\n+\techo \"|/  \" >> expected &&\n+\techo \"* $A2\" >> expected &&\n+\techo \"* $A1\" >> expected &&\n+\tgit rev-list --graph --sparse --all -- bar.txt > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+test_expect_success '--graph ^C4' '\n+\trm -f expected &&\n+\techo \"* $A7\" >> expected &&\n+\techo \"* $A6\" >> expected &&\n+\techo \"* $A5\" >> expected &&\n+\techo \"*   $A4\" >> expected &&\n+\techo \"|\\\\  \" >> expected &&\n+\techo \"| * $B2\" >> expected &&\n+\techo \"| * $B1\" >> expected &&\n+\techo \"* $A3\" >> expected &&\n+\tgit rev-list --graph --all ^C4 > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+test_expect_success '--graph ^C3' '\n+\trm -f expected &&\n+\techo \"* $A7\" >> expected &&\n+\techo \"*   $A6\" >> expected &&\n+\techo \"|\\\\  \" >> expected &&\n+\techo \"| * $C4\" >> expected &&\n+\techo \"* $A5\" >> expected &&\n+\techo \"*   $A4\" >> expected &&\n+\techo \"|\\\\  \" >> expected &&\n+\techo \"| * $B2\" >> expected &&\n+\techo \"| * $B1\" >> expected &&\n+\techo \"* $A3\" >> expected &&\n+\tgit rev-list --graph --all ^C3 > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+# I don't think the ordering of the boundary commits is really\n+# that important, but this test depends on it.  If the ordering ever changes\n+# in the code, we'll need to update this test.\n+test_expect_success '--graph --boundary ^C3' '\n+\trm -f expected &&\n+\techo \"* $A7\" >> expected &&\n+\techo \"*   $A6\" >> expected &&\n+\techo \"|\\\\  \" >> expected &&\n+\techo \"| * $C4\" >> expected &&\n+\techo \"* | $A5\" >> expected &&\n+\techo \"| |     \" >> expected &&\n+\techo \"|  \\\\    \" >> expected &&\n+\techo \"*-. \\\\   $A4\" >> expected &&\n+\techo \"|\\\\ \\\\ \\\\  \" >> expected &&\n+\techo \"| * | | $B2\" >> expected &&\n+\techo \"| * | | $B1\" >> expected &&\n+\techo \"* | | | $A3\" >> expected &&\n+\techo \"o | | | $A2\" >> expected &&\n+\techo \"|/ / /  \" >> expected &&\n+\techo \"o | | $A1\" >> expected &&\n+\techo \" / /  \" >> expected &&\n+\techo \"| o $C3\" >> expected &&\n+\techo \"|/  \" >> expected &&\n+\techo \"o $C2\" >> expected &&\n+\tgit rev-list --graph --boundary --all ^C3 > actual &&\n+\ttest_cmp expected actual\n+\t'\n+\n+test_done\n-- \n1.6.0.4\n"},{"id":"121454","messageId":"7vbpm8exeo.fsf@alter.siamese.dyndns.org","threadId":"20657","inReplyTo":"20090821182034.GW8147@facebook.com","subject":"Re: [PATCH] Add tests for rev-list --graph with options that simplify history","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-21T20:15:27Z","receivedAt":"2009-08-21T20:15:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Simpkins <simpkins@facebook.com> writes:\n\n> These tests help make sure graph_is_interesting() is doing the right\n> thing.\n>\n> Signed-off-by: Adam Simpkins <simpkins@facebook.com>\n> ---\n>  t/t6016-rev-list-graph-simplify-history.sh |  276 ++++++++++++++++++++++++++++\n>  1 files changed, 276 insertions(+), 0 deletions(-)\n>  create mode 100755 t/t6016-rev-list-graph-simplify-history.sh\n>\n> diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh\n> new file mode 100755\n> index 0000000..5ac8fc9\n> --- /dev/null\n> +++ b/t/t6016-rev-list-graph-simplify-history.sh\n> @@ -0,0 +1,276 @@\n> +#!/bin/sh\n> +\n> +# There's more than one \"correct\" way to represent the history graphically.\n> +# These tests depend on the current behavior of the graphing code.  If the\n> +# graphing code is ever changed to draw the output differently, these tests\n> +# cases will need to be updated to know about the new layout.\n\nAn ideal solution to such a problem would be not to write the tests that\nway to require _the exact layout_ of the output.\n\nWhat was the bug you were trying to fix?  Was it that in a simplified\nhistory some arcs are not connected whey they should be?\n\nCan you test that without relying on other aspect (say, commits are marked\nwith '*' right now but a patch might change it to '^' for some commits) of\nthe output?\n\nI am just wondering how feasible it is the problem you are trying to\nsolve, not demanding you to solve it.\n"},{"id":"121466","messageId":"20090821212326.GX8147@facebook.com","threadId":"20657","inReplyTo":"7vbpm8exeo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add tests for rev-list --graph with options that simplify history","fromName":"Adam Simpkins","fromEmail":"simpkins@facebook.com","sentAt":"2009-08-21T21:23:26Z","receivedAt":"2009-08-21T21:23:26Z","isPatch":true,"sender":{"key":"simpkins@facebook.com","avatar":null},"body":"On Fri, Aug 21, 2009 at 01:15:27PM -0700, Junio C Hamano wrote:\n> Adam Simpkins <simpkins@facebook.com> writes:\n> \n> > +# There's more than one \"correct\" way to represent the history graphically.\n> > +# These tests depend on the current behavior of the graphing code.  If the\n> > +# graphing code is ever changed to draw the output differently, these tests\n> > +# cases will need to be updated to know about the new layout.\n> \n> An ideal solution to such a problem would be not to write the tests that\n> way to require _the exact layout_ of the output.\n\nYeah.  In the past I've been hesitant to submit tests for the graph\nbehavior for precisely this reason.  However, having tests that check\nthe exact layout seems better than not having tests at all.\n\n> What was the bug you were trying to fix?  Was it that in a simplified\n> history some arcs are not connected whey they should be?\n\nIt was an issue with a missing arc between two commits that should\nhave been connected.  In the past, other bugs (e.g., the one fixed in\n2ecbd0a0) have caused arcs to appear connected to the wrong commit.\n\n> Can you test that without relying on other aspect (say, commits are marked\n> with '*' right now but a patch might change it to '^' for some commits) of\n> the output?\n> \n> I am just wondering how feasible it is the problem you are trying to\n> solve, not demanding you to solve it.\n\nIn general, it seems like its not worthwhile trying to solve this\nproblem.  I don't expect changes to the graph layout to occur often.\nModifying this test case if and when they do occur seems simpler and\nless error-prone than trying to write code that attempts to anticipate\nchanges we might make in the future.\n\nWhen I wrote this comment, I was thinking more about potential changes\nin the way arcs are drawn in the output, or in the amount of padding.\nThe problem you mentioned (accepting other characters other than '*'\nfor commits) is easier, but I'm still not convinced we should try to\nsolve it.  For example, it's nice that the current code also tests\nthat boundary commits are represented differently than non-boundary\ncommits.  Being too permissive in what we accept could also\npotentially hide bugs in the future.\n\n-- \nAdam Simpkins\nsimpkins@facebook.com\n"}]}