RE: interaction between --graph and --simplify-by-decoration

RE: interaction between --graph and --simplify-by-decoration

[Adam Simpkins]

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

[PATCH] graph API: fix bug in graph_is_interesting()

[Adam Simpkins]

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

Re: [PATCH] graph API: fix bug in graph_is_interesting()

[Junio C Hamano]

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.

Re: [PATCH] graph API: fix bug in graph_is_interesting()

[Adam Simpkins]

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

Re: [PATCH] graph API: fix bug in graph_is_interesting()

[Adam Simpkins]

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

Re: [PATCH] graph API: fix bug in graph_is_interesting()

[Junio C Hamano]

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?

Re: [PATCH] graph API: fix bug in graph_is_interesting()

[Junio C Hamano]

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.

Re: [PATCH] graph API: fix bug in graph_is_interesting()

[Adam Simpkins]

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

[PATCH] Add test case for rev-list --parents --show-all

[Adam Simpkins]

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

Re: [PATCH] Add test case for rev-list --parents --show-all

[Junio C Hamano]

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.

Re: [PATCH] graph API: fix bug in graph_is_interesting()

[Santi Béjar]

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

[PATCH] Add tests for rev-list --graph with options that simplify history

[Adam Simpkins]

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

Re: [PATCH] Add tests for rev-list --graph with options that simplify history

[Junio C Hamano]

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.

Re: [PATCH] Add tests for rev-list --graph with options that simplify history

[Adam Simpkins]

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