* RE: interaction between --graph and --simplify-by-decoration @ 2009-08-18 20:55 Adam Simpkins 2009-08-18 21:18 ` [PATCH] graph API: fix bug in graph_is_interesting() Adam Simpkins 0 siblings, 1 reply; 14+ messages in thread From: Adam Simpkins @ 2009-08-18 20:55 UTC (permalink / raw) To: Santi Béjar; +Cc: Git Mailing List On Friday, July 31, 2009 4:11am, "Santi Béjar" <santi@agolina.net> said: > Hello, > > I've found that in some cases the --graph and > --simplify-by-decoration don't work well together. If you do this in > the git.git repository: Thanks for reporting the problem, I apologize for taking so long to investigate and respond. > * | | f29ac4f (tag: v1.6.3-rc2) GIT 1.6.3-rc2 > / / > | | * 66996ec Sync with 1.6.2.4 > you can see that f29ac4f looks like it does not have any parents while > the correct parent is 66996ec which seems to have no children. But if > you omit the --oneline you can see that there are a lot of "root"-like > commits (f01f109, a48f5d7, f29ac4f,...). Yes, there's a bug in graph_is_interesting(). When processing f29ac4f, the graph code thinks that 66996ec isn't interesting and won't get displayed in the output, so it doesn't prepare the graph lines to show lines to 66996ec. I'll submit a patch shortly. -- Adam Simpkins adam@adamsimpkins.net ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] graph API: fix bug in graph_is_interesting() 2009-08-18 20:55 interaction between --graph and --simplify-by-decoration Adam Simpkins @ 2009-08-18 21:18 ` Adam Simpkins 2009-08-18 23:53 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Adam Simpkins @ 2009-08-18 21:18 UTC (permalink / raw) To: Git Mailing List; +Cc: Santi Béjar, Junio C Hamano Updated graph_is_interesting() to use simplify_commit() to determine if a commit is interesting, just like get_revision() does. Previously, it would sometimes incorrectly treat an interesting commit as uninteresting. This resulted in incorrect lines in the graph output. This problem was reported by Santi Béjar. The following command would exhibit the problem before, but now works correctly: git log --graph --simplify-by-decoration --oneline v1.6.3.3 Previously git graph did not display the output for this command correctly between f29ac4f and 66996ec, among other places. Signed-off-by: Adam Simpkins <simpkins@facebook.com> --- Note that simplify_commit() may modify the revision list. Calling it in graph_is_interesting() can modify the revision list earlier than it otherwise would be (in get_revision()). I don't think this should cause any problems, but figured I'd point it out in case anyone more familiar with the code thinks otherwise. graph.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/graph.c b/graph.c index e466770..ea21e91 100644 --- a/graph.c +++ b/graph.c @@ -286,9 +286,10 @@ static int graph_is_interesting(struct git_graph *graph, struct commit *commit) } /* - * Uninteresting and pruned commits won't be printed + * Otherwise, use simplify_commit() to see if this commit is + * interesting */ - return (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1; + return simplify_commit(graph->revs, commit) == commit_show; } static struct commit_list *next_interesting_parent(struct git_graph *graph, -- 1.6.4.314.ge5db ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] graph API: fix bug in graph_is_interesting() 2009-08-18 21:18 ` [PATCH] graph API: fix bug in graph_is_interesting() Adam Simpkins @ 2009-08-18 23:53 ` Junio C Hamano 2009-08-19 2:29 ` Adam Simpkins 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2009-08-18 23:53 UTC (permalink / raw) To: Git Mailing List; +Cc: Santi Béjar Adam Simpkins <simpkins@facebook.com> writes: > - return (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1; > + return simplify_commit(graph->revs, commit) == commit_show; If you do this after revision.c finished the traversal (e.g. "limited" case), I think it should be Ok. But calling simplify_commit() while the traversal is still in progress is asking for trouble. I do not recall the details anymore but when I tried to make the "simplify-merges" algorithm incremental, I had seen funny breakage caused by calling simplify_commit() twice on the same commit. I suspect that this change will break the primary traversal. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] graph API: fix bug in graph_is_interesting() 2009-08-18 23:53 ` Junio C Hamano @ 2009-08-19 2:29 ` Adam Simpkins 2009-08-19 2:34 ` Adam Simpkins 0 siblings, 1 reply; 14+ messages in thread From: Adam Simpkins @ 2009-08-19 2:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Santi Béjar On Tue, Aug 18, 2009 at 04:53:45PM -0700, Junio C Hamano wrote: > Adam Simpkins <simpkins@facebook.com> writes: > > > - return (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1; > > + return simplify_commit(graph->revs, commit) == commit_show; > > If you do this after revision.c finished the traversal (e.g. "limited" > case), I think it should be Ok. > > But calling simplify_commit() while the traversal is still in progress is > asking for trouble. I do not recall the details anymore but when I tried > to make the "simplify-merges" algorithm incremental, I had seen funny > breakage caused by calling simplify_commit() twice on the same commit. > > I suspect that this change will break the primary traversal. The --graph option always enables revs->topo_order, which in turn enables revs->limited, so this should always be after limit_list() has already been called. However, it looks like the call to rewrite_parents() is the only modifying operation in simplify_commit(), and all the rest can easily be split out into a separate helper function. I'll submit another version of the patch that makes the non-modifying simplify_commit() behavior available as a separate function. -- Adam Simpkins simpkins@facebook.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] graph API: fix bug in graph_is_interesting() 2009-08-19 2:29 ` Adam Simpkins @ 2009-08-19 2:34 ` Adam Simpkins 2009-08-19 6:18 ` Junio C Hamano 2009-08-21 15:39 ` [PATCH] graph API: fix bug in graph_is_interesting() Santi Béjar 0 siblings, 2 replies; 14+ messages in thread From: Adam Simpkins @ 2009-08-19 2:34 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Santi Béjar Previously, graph_is_interesting() did not behave quite the same way as the code in get_revision(). As a result, it would sometimes think commits were uninteresting, even though get_revision() would return them. This resulted in incorrect lines in the graph output. This change creates a get_commit_action() function, which graph_is_interesting() and simplify_commit() both now use to determine if a commit will be shown. It is identical to the old simplify_commit() behavior, except that it never calls rewrite_parents(). This problem was reported by Santi Béjar. The following command would exhibit the problem before, but now works correctly: git log --graph --simplify-by-decoration --oneline v1.6.3.3 Previously git graph did not display the output for this command correctly between f29ac4f and 66996ec, among other places. Signed-off-by: Adam Simpkins <simpkins@facebook.com> --- graph.c | 5 +++-- revision.c | 15 ++++++++++++--- revision.h | 1 + 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/graph.c b/graph.c index e466770..9087f65 100644 --- a/graph.c +++ b/graph.c @@ -286,9 +286,10 @@ static int graph_is_interesting(struct git_graph *graph, struct commit *commit) } /* - * Uninteresting and pruned commits won't be printed + * Otherwise, use get_commit_action() to see if this commit is + * interesting */ - return (commit->object.flags & (UNINTERESTING | TREESAME)) ? 0 : 1; + return get_commit_action(graph->revs, commit) == commit_show; } static struct commit_list *next_interesting_parent(struct git_graph *graph, diff --git a/revision.c b/revision.c index 8ffb661..fe7d522 100644 --- a/revision.c +++ b/revision.c @@ -1664,7 +1664,7 @@ static inline int want_ancestry(struct rev_info *revs) return (revs->rewrite_parents || revs->children.name); } -enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit) +enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit) { if (commit->object.flags & SHOWN) return commit_ignore; @@ -1692,12 +1692,21 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit) if (!commit->parents || !commit->parents->next) return commit_ignore; } - if (want_ancestry(revs) && rewrite_parents(revs, commit) < 0) - return commit_error; } return commit_show; } +enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit) +{ + enum commit_action action = get_commit_action(revs, commit); + + if (action == commit_show && revs->prune && revs->dense && want_ancestry(revs)) { + if (rewrite_parents(revs, commit) < 0) + return commit_error; + } + return action; +} + static struct commit *get_revision_1(struct rev_info *revs) { if (!revs->commits) diff --git a/revision.h b/revision.h index b10984b..9d0dddb 100644 --- a/revision.h +++ b/revision.h @@ -168,6 +168,7 @@ enum commit_action { commit_error }; +extern enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit); extern enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit); #endif -- 1.6.4.314.g0a482.dirty ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] graph API: fix bug in graph_is_interesting() 2009-08-19 2:34 ` Adam Simpkins @ 2009-08-19 6:18 ` Junio C Hamano 2009-08-19 6:25 ` Junio C Hamano 2009-08-21 15:39 ` [PATCH] graph API: fix bug in graph_is_interesting() Santi Béjar 1 sibling, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2009-08-19 6:18 UTC (permalink / raw) To: Adam Simpkins; +Cc: Santi Béjar, Git Mailing List Adam Simpkins <simpkins@facebook.com> writes: > -enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit) > +enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit) > { > if (commit->object.flags & SHOWN) > return commit_ignore; > @@ -1692,12 +1692,21 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit) > if (!commit->parents || !commit->parents->next) > return commit_ignore; > } > - if (want_ancestry(revs) && rewrite_parents(revs, commit) < 0) > - return commit_error; > } > return commit_show; > } > > +enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit) > +{ > + enum commit_action action = get_commit_action(revs, commit); > + > + if (action == commit_show && revs->prune && revs->dense && want_ancestry(revs)) { > + if (rewrite_parents(revs, commit) < 0) > + return commit_error; > + } > + return action; > +} When simplify_commit() logic (now called get_comit_action()) decides to show this commit because revs->show_all was specified, we did not rewrite its parents, but now we will? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] graph API: fix bug in graph_is_interesting() 2009-08-19 6:18 ` Junio C Hamano @ 2009-08-19 6:25 ` Junio C Hamano 2009-08-19 22:55 ` Adam Simpkins 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2009-08-19 6:25 UTC (permalink / raw) To: Adam Simpkins; +Cc: Santi Béjar, Git Mailing List Junio C Hamano <gitster@pobox.com> writes: > Adam Simpkins <simpkins@facebook.com> writes: > >> -enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit) >> +enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit) >> { >> if (commit->object.flags & SHOWN) >> return commit_ignore; >> @@ -1692,12 +1692,21 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit) >> if (!commit->parents || !commit->parents->next) >> return commit_ignore; >> } >> - if (want_ancestry(revs) && rewrite_parents(revs, commit) < 0) >> - return commit_error; >> } >> return commit_show; >> } >> >> +enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit) >> +{ >> + enum commit_action action = get_commit_action(revs, commit); >> + >> + if (action == commit_show && revs->prune && revs->dense && want_ancestry(revs)) { >> + if (rewrite_parents(revs, commit) < 0) >> + return commit_error; >> + } >> + return action; >> +} > > When simplify_commit() logic (now called get_comit_action()) decides to > show this commit because revs->show_all was specified, we did not rewrite > its parents, but now we will? That is, here is what I meant... revision.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/revision.c b/revision.c index 15a2010..efa3b7c 100644 --- a/revision.c +++ b/revision.c @@ -1700,7 +1700,9 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit) { enum commit_action action = get_commit_action(revs, commit); - if (action == commit_show && revs->prune && revs->dense && want_ancestry(revs)) { + if (action == commit_show && + !revs->show_all && + revs->prune && revs->dense && want_ancestry(revs)) { if (rewrite_parents(revs, commit) < 0) return commit_error; } We may want to add some tests to demonstrate the breakage this fix addresses. ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] graph API: fix bug in graph_is_interesting() 2009-08-19 6:25 ` Junio C Hamano @ 2009-08-19 22:55 ` Adam Simpkins 2009-08-19 22:58 ` [PATCH] Add test case for rev-list --parents --show-all Adam Simpkins 0 siblings, 1 reply; 14+ messages in thread From: Adam Simpkins @ 2009-08-19 22:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Santi Béjar, Git Mailing List On Tue, Aug 18, 2009 at 11:25:49PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > > When simplify_commit() logic (now called get_comit_action()) decides to > > show this commit because revs->show_all was specified, we did not rewrite > > its parents, but now we will? > > That is, here is what I meant... > > - if (action == commit_show && revs->prune && revs->dense && want_ancestry(revs)) { > + if (action == commit_show && > + !revs->show_all && > + revs->prune && revs->dense && want_ancestry(revs)) { > > We may want to add some tests to demonstrate the breakage this fix > addresses. Yes, you're right. Thanks for catching that. I'll submit a test case that checks this scenario. -- Adam Simpkins simpkins@facebook.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] Add test case for rev-list --parents --show-all 2009-08-19 22:55 ` Adam Simpkins @ 2009-08-19 22:58 ` Adam Simpkins 2009-08-20 4:13 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Adam Simpkins @ 2009-08-19 22:58 UTC (permalink / raw) To: Junio C Hamano, Santi Béjar, Git Mailing List This test case ensures that rev-list --parents --show-all gets the parent history correct. Normally, --parents rewrites parent history to skip TREESAME parents. However, --show-all causes TREESAME parents to still be included in the revision list, so the parents should still be included too. Signed-off-by: Adam Simpkins <simpkins@facebook.com> --- Looking through the code, I believe TREESAME commits are the only ones affected by my earlier bug in simplify_commit(). t/t6015-rev-list-show-all-parents.sh | 31 +++++++++++++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-) create mode 100644 t/t6015-rev-list-show-all-parents.sh diff --git a/t/t6015-rev-list-show-all-parents.sh b/t/t6015-rev-list-show-all-parents.sh new file mode 100644 index 0000000..8b146fb --- /dev/null +++ b/t/t6015-rev-list-show-all-parents.sh @@ -0,0 +1,31 @@ +#!/bin/sh + +test_description='--show-all --parents does not rewrite TREESAME commits' + +. ./test-lib.sh + +test_expect_success 'set up --show-all --parents test' ' + test_commit one foo.txt && + commit1=`git rev-list -1 HEAD` && + test_commit two bar.txt && + commit2=`git rev-list -1 HEAD` && + test_commit three foo.txt && + commit3=`git rev-list -1 HEAD` + ' + +test_expect_success '--parents rewrites TREESAME parents correctly' ' + echo $commit3 $commit1 > expected && + echo $commit1 >> expected && + git rev-list --parents HEAD -- foo.txt > actual && + test_cmp expected actual + ' + +test_expect_success '--parents --show-all does not rewrites TREESAME parents' ' + echo $commit3 $commit2 > expected && + echo $commit2 $commit1 >> expected && + echo $commit1 >> expected && + git rev-list --parents --show-all HEAD -- foo.txt > actual && + test_cmp expected actual + ' + +test_done -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] Add test case for rev-list --parents --show-all 2009-08-19 22:58 ` [PATCH] Add test case for rev-list --parents --show-all Adam Simpkins @ 2009-08-20 4:13 ` Junio C Hamano 2009-08-21 18:20 ` [PATCH] Add tests for rev-list --graph with options that simplify history Adam Simpkins 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2009-08-20 4:13 UTC (permalink / raw) To: Adam Simpkins; +Cc: Santi Béjar, Git Mailing List Adam Simpkins <simpkins@facebook.com> writes: > This test case ensures that rev-list --parents --show-all gets the > parent history correct. Normally, --parents rewrites parent history to > skip TREESAME parents. However, --show-all causes TREESAME parents to > still be included in the revision list, so the parents should still be > included too. > > Signed-off-by: Adam Simpkins <simpkins@facebook.com> > --- > > Looking through the code, I believe TREESAME commits are the only ones > affected by my earlier bug in simplify_commit(). What I meant was actually a test for the graph part (i.e. the problem we would see if we did not apply your update to graph_is_interesting()), but protecting the simplify_commit() logic with test from breakage is a good thing to do as well. Thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] Add tests for rev-list --graph with options that simplify history 2009-08-20 4:13 ` Junio C Hamano @ 2009-08-21 18:20 ` Adam Simpkins 2009-08-21 20:15 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Adam Simpkins @ 2009-08-21 18:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List These tests help make sure graph_is_interesting() is doing the right thing. --- t/t6016-rev-list-graph-simplify-history.sh | 276 ++++++++++++++++++++++++++++ 1 files changed, 276 insertions(+), 0 deletions(-) create mode 100755 t/t6016-rev-list-graph-simplify-history.sh diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh new file mode 100755 index 0000000..5ac8fc9 --- /dev/null +++ b/t/t6016-rev-list-graph-simplify-history.sh @@ -0,0 +1,276 @@ +#!/bin/sh + +# There's more than one "correct" way to represent the history graphically. +# These tests depend on the current behavior of the graphing code. If the +# graphing code is ever changed to draw the output differently, these tests +# cases will need to be updated to know about the new layout. + +test_description='--graph and simplified history' + +. ./test-lib.sh + +test_expect_success 'set up rev-list --graph test' ' + # 3 commits on branch A + test_commit A1 foo.txt && + test_commit A2 bar.txt && + test_commit A3 bar.txt && + git branch -m master A && + + # 2 commits on branch B, started from A1 + git checkout -b B A1 && + test_commit B1 foo.txt && + test_commit B2 abc.txt && + + # 2 commits on branch C, started from A2 + git checkout -b C A2 && + test_commit C1 xyz.txt && + test_commit C2 xyz.txt && + + # Octopus merge B and C into branch A + git checkout A && + git merge B C && + git tag A4 + + test_commit A5 bar.txt && + + # More commits on C, then merge C into A + git checkout C && + test_commit C3 foo.txt && + test_commit C4 bar.txt && + git checkout A && + git merge -s ours C && + git tag A6 + + test_commit A7 bar.txt && + + # Store commit names in variables for later use + A1=`git rev-list -1 A1` && + A2=`git rev-list -1 A2` && + A3=`git rev-list -1 A3` && + A4=`git rev-list -1 A4` && + A5=`git rev-list -1 A5` && + A6=`git rev-list -1 A6` && + A7=`git rev-list -1 A7` && + B1=`git rev-list -1 B1` && + B2=`git rev-list -1 B2` && + C1=`git rev-list -1 C1` && + C2=`git rev-list -1 C2` && + C3=`git rev-list -1 C3` && + C4=`git rev-list -1 C4` + ' + +test_expect_success '--graph --all' ' + rm -f expected && + echo "* $A7" >> expected && + echo "* $A6" >> expected && + echo "|\\ " >> expected && + echo "| * $C4" >> expected && + echo "| * $C3" >> expected && + echo "* | $A5" >> expected && + echo "| | " >> expected && + echo "| \\ " >> expected && + echo "*-. \\ $A4" >> expected && + echo "|\\ \\ \\ " >> expected && + echo "| | |/ " >> expected && + echo "| | * $C2" >> expected && + echo "| | * $C1" >> expected && + echo "| * | $B2" >> expected && + echo "| * | $B1" >> expected && + echo "* | | $A3" >> expected && + echo "| |/ " >> expected && + echo "|/| " >> expected && + echo "* | $A2" >> expected && + echo "|/ " >> expected && + echo "* $A1" >> expected && + git rev-list --graph --all > actual && + test_cmp expected actual + ' + +# Make sure the graph_is_interesting() code still realizes +# that undecorated merges are interesting, even with --simplify-by-decoration +test_expect_success '--graph --simplify-by-decoration' ' + rm -f expected && + git tag -d A4 + echo "* $A7" >> expected && + echo "* $A6" >> expected && + echo "|\\ " >> expected && + echo "| * $C4" >> expected && + echo "| * $C3" >> expected && + echo "* | $A5" >> expected && + echo "| | " >> expected && + echo "| \\ " >> expected && + echo "*-. \\ $A4" >> expected && + echo "|\\ \\ \\ " >> expected && + echo "| | |/ " >> expected && + echo "| | * $C2" >> expected && + echo "| | * $C1" >> expected && + echo "| * | $B2" >> expected && + echo "| * | $B1" >> expected && + echo "* | | $A3" >> expected && + echo "| |/ " >> expected && + echo "|/| " >> expected && + echo "* | $A2" >> expected && + echo "|/ " >> expected && + echo "* $A1" >> expected && + git rev-list --graph --all --simplify-by-decoration > actual && + test_cmp expected actual + ' + +# Get rid of all decorations on branch B, and graph with it simplified away +test_expect_success '--graph --simplify-by-decoration prune branch B' ' + rm -f expected && + git tag -d B2 + git tag -d B1 + git branch -d B + echo "* $A7" >> expected && + echo "* $A6" >> expected && + echo "|\\ " >> expected && + echo "| * $C4" >> expected && + echo "| * $C3" >> expected && + echo "* | $A5" >> expected && + echo "* | $A4" >> expected && + echo "|\\ \\ " >> expected && + echo "| |/ " >> expected && + echo "| * $C2" >> expected && + echo "| * $C1" >> expected && + echo "* | $A3" >> expected && + echo "|/ " >> expected && + echo "* $A2" >> expected && + echo "* $A1" >> expected && + git rev-list --graph --simplify-by-decoration --all > actual && + test_cmp expected actual + ' + +test_expect_success '--graph --full-history -- bar.txt' ' + rm -f expected && + git tag -d B2 + git tag -d B1 + git branch -d B + echo "* $A7" >> expected && + echo "* $A6" >> expected && + echo "|\\ " >> expected && + echo "| * $C4" >> expected && + echo "* | $A5" >> expected && + echo "* | $A4" >> expected && + echo "|\\ \\ " >> expected && + echo "| |/ " >> expected && + echo "* | $A3" >> expected && + echo "|/ " >> expected && + echo "* $A2" >> expected && + git rev-list --graph --full-history --all -- bar.txt > actual && + test_cmp expected actual + ' + +test_expect_success '--graph --full-history --simplify-merges -- bar.txt' ' + rm -f expected && + git tag -d B2 + git tag -d B1 + git branch -d B + echo "* $A7" >> expected && + echo "* $A6" >> expected && + echo "|\\ " >> expected && + echo "| * $C4" >> expected && + echo "* | $A5" >> expected && + echo "* | $A3" >> expected && + echo "|/ " >> expected && + echo "* $A2" >> expected && + git rev-list --graph --full-history --simplify-merges --all \ + -- bar.txt > actual && + test_cmp expected actual + ' + +test_expect_success '--graph -- bar.txt' ' + rm -f expected && + git tag -d B2 + git tag -d B1 + git branch -d B + echo "* $A7" >> expected && + echo "* $A5" >> expected && + echo "* $A3" >> expected && + echo "| * $C4" >> expected && + echo "|/ " >> expected && + echo "* $A2" >> expected && + git rev-list --graph --all -- bar.txt > actual && + test_cmp expected actual + ' + +test_expect_success '--graph --sparse -- bar.txt' ' + rm -f expected && + git tag -d B2 + git tag -d B1 + git branch -d B + echo "* $A7" >> expected && + echo "* $A6" >> expected && + echo "* $A5" >> expected && + echo "* $A4" >> expected && + echo "* $A3" >> expected && + echo "| * $C4" >> expected && + echo "| * $C3" >> expected && + echo "| * $C2" >> expected && + echo "| * $C1" >> expected && + echo "|/ " >> expected && + echo "* $A2" >> expected && + echo "* $A1" >> expected && + git rev-list --graph --sparse --all -- bar.txt > actual && + test_cmp expected actual + ' + +test_expect_success '--graph ^C4' ' + rm -f expected && + echo "* $A7" >> expected && + echo "* $A6" >> expected && + echo "* $A5" >> expected && + echo "* $A4" >> expected && + echo "|\\ " >> expected && + echo "| * $B2" >> expected && + echo "| * $B1" >> expected && + echo "* $A3" >> expected && + git rev-list --graph --all ^C4 > actual && + test_cmp expected actual + ' + +test_expect_success '--graph ^C3' ' + rm -f expected && + echo "* $A7" >> expected && + echo "* $A6" >> expected && + echo "|\\ " >> expected && + echo "| * $C4" >> expected && + echo "* $A5" >> expected && + echo "* $A4" >> expected && + echo "|\\ " >> expected && + echo "| * $B2" >> expected && + echo "| * $B1" >> expected && + echo "* $A3" >> expected && + git rev-list --graph --all ^C3 > actual && + test_cmp expected actual + ' + +# I don't think the ordering of the boundary commits is really +# that important, but this test depends on it. If the ordering ever changes +# in the code, we'll need to update this test. +test_expect_success '--graph --boundary ^C3' ' + rm -f expected && + echo "* $A7" >> expected && + echo "* $A6" >> expected && + echo "|\\ " >> expected && + echo "| * $C4" >> expected && + echo "* | $A5" >> expected && + echo "| | " >> expected && + echo "| \\ " >> expected && + echo "*-. \\ $A4" >> expected && + echo "|\\ \\ \\ " >> expected && + echo "| * | | $B2" >> expected && + echo "| * | | $B1" >> expected && + echo "* | | | $A3" >> expected && + echo "o | | | $A2" >> expected && + echo "|/ / / " >> expected && + echo "o | | $A1" >> expected && + echo " / / " >> expected && + echo "| o $C3" >> expected && + echo "|/ " >> expected && + echo "o $C2" >> expected && + git rev-list --graph --boundary --all ^C3 > actual && + test_cmp expected actual + ' + +test_done -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] Add tests for rev-list --graph with options that simplify history 2009-08-21 18:20 ` [PATCH] Add tests for rev-list --graph with options that simplify history Adam Simpkins @ 2009-08-21 20:15 ` Junio C Hamano 2009-08-21 21:23 ` Adam Simpkins 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2009-08-21 20:15 UTC (permalink / raw) To: Adam Simpkins; +Cc: Git Mailing List Adam Simpkins <simpkins@facebook.com> writes: > These tests help make sure graph_is_interesting() is doing the right > thing. > > Signed-off-by: Adam Simpkins <simpkins@facebook.com> > --- > t/t6016-rev-list-graph-simplify-history.sh | 276 ++++++++++++++++++++++++++++ > 1 files changed, 276 insertions(+), 0 deletions(-) > create mode 100755 t/t6016-rev-list-graph-simplify-history.sh > > diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh > new file mode 100755 > index 0000000..5ac8fc9 > --- /dev/null > +++ b/t/t6016-rev-list-graph-simplify-history.sh > @@ -0,0 +1,276 @@ > +#!/bin/sh > + > +# There's more than one "correct" way to represent the history graphically. > +# These tests depend on the current behavior of the graphing code. If the > +# graphing code is ever changed to draw the output differently, these tests > +# cases will need to be updated to know about the new layout. An ideal solution to such a problem would be not to write the tests that way to require _the exact layout_ of the output. What was the bug you were trying to fix? Was it that in a simplified history some arcs are not connected whey they should be? Can you test that without relying on other aspect (say, commits are marked with '*' right now but a patch might change it to '^' for some commits) of the output? I am just wondering how feasible it is the problem you are trying to solve, not demanding you to solve it. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Add tests for rev-list --graph with options that simplify history 2009-08-21 20:15 ` Junio C Hamano @ 2009-08-21 21:23 ` Adam Simpkins 0 siblings, 0 replies; 14+ messages in thread From: Adam Simpkins @ 2009-08-21 21:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List On Fri, Aug 21, 2009 at 01:15:27PM -0700, Junio C Hamano wrote: > Adam Simpkins <simpkins@facebook.com> writes: > > > +# There's more than one "correct" way to represent the history graphically. > > +# These tests depend on the current behavior of the graphing code. If the > > +# graphing code is ever changed to draw the output differently, these tests > > +# cases will need to be updated to know about the new layout. > > An ideal solution to such a problem would be not to write the tests that > way to require _the exact layout_ of the output. Yeah. In the past I've been hesitant to submit tests for the graph behavior for precisely this reason. However, having tests that check the exact layout seems better than not having tests at all. > What was the bug you were trying to fix? Was it that in a simplified > history some arcs are not connected whey they should be? It was an issue with a missing arc between two commits that should have been connected. In the past, other bugs (e.g., the one fixed in 2ecbd0a0) have caused arcs to appear connected to the wrong commit. > Can you test that without relying on other aspect (say, commits are marked > with '*' right now but a patch might change it to '^' for some commits) of > the output? > > I am just wondering how feasible it is the problem you are trying to > solve, not demanding you to solve it. In general, it seems like its not worthwhile trying to solve this problem. I don't expect changes to the graph layout to occur often. Modifying this test case if and when they do occur seems simpler and less error-prone than trying to write code that attempts to anticipate changes we might make in the future. When I wrote this comment, I was thinking more about potential changes in the way arcs are drawn in the output, or in the amount of padding. The problem you mentioned (accepting other characters other than '*' for commits) is easier, but I'm still not convinced we should try to solve it. For example, it's nice that the current code also tests that boundary commits are represented differently than non-boundary commits. Being too permissive in what we accept could also potentially hide bugs in the future. -- Adam Simpkins simpkins@facebook.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] graph API: fix bug in graph_is_interesting() 2009-08-19 2:34 ` Adam Simpkins 2009-08-19 6:18 ` Junio C Hamano @ 2009-08-21 15:39 ` Santi Béjar 1 sibling, 0 replies; 14+ messages in thread From: Santi Béjar @ 2009-08-21 15:39 UTC (permalink / raw) To: Adam Simpkins, Git Mailing List, Junio C Hamano On Wed, Aug 19, 2009 at 4:34 AM, Adam Simpkins<simpkins@facebook.com> wrote: > Previously, graph_is_interesting() did not behave quite the same way as > the code in get_revision(). As a result, it would sometimes think > commits were uninteresting, even though get_revision() would return > them. This resulted in incorrect lines in the graph output. > > This change creates a get_commit_action() function, which > graph_is_interesting() and simplify_commit() both now use to determine > if a commit will be shown. It is identical to the old simplify_commit() > behavior, except that it never calls rewrite_parents(). > > This problem was reported by Santi Béjar. The following command > would exhibit the problem before, but now works correctly: > > git log --graph --simplify-by-decoration --oneline v1.6.3.3 > > Previously git graph did not display the output for this command > correctly between f29ac4f and 66996ec, among other places. Thanks, the fix works (no comments about the code, only the behavior). One more thing Junio. In 99af022 (graph API: fix bug in graph_is_interesting(), 2009-08-18) in 'pu' my name is mispelled, it seems it was interpreted as latin1, then recoded as UTF8, interpreted as latin and recoded to latin1, or in other words the é is 4 bytes instead of two. I've checked this mail and it is latin1, correctly specified in the headers. Thanks, Santi ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2009-08-21 21:23 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-08-18 20:55 interaction between --graph and --simplify-by-decoration Adam Simpkins 2009-08-18 21:18 ` [PATCH] graph API: fix bug in graph_is_interesting() Adam Simpkins 2009-08-18 23:53 ` Junio C Hamano 2009-08-19 2:29 ` Adam Simpkins 2009-08-19 2:34 ` Adam Simpkins 2009-08-19 6:18 ` Junio C Hamano 2009-08-19 6:25 ` Junio C Hamano 2009-08-19 22:55 ` Adam Simpkins 2009-08-19 22:58 ` [PATCH] Add test case for rev-list --parents --show-all Adam Simpkins 2009-08-20 4:13 ` Junio C Hamano 2009-08-21 18:20 ` [PATCH] Add tests for rev-list --graph with options that simplify history Adam Simpkins 2009-08-21 20:15 ` Junio C Hamano 2009-08-21 21:23 ` Adam Simpkins 2009-08-21 15:39 ` [PATCH] graph API: fix bug in graph_is_interesting() Santi Béjar
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).