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

Subsystems: the rest

5 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:16

Junio C Hamano [off-list ref] writes:

Adam Simpkins [off-list ref] writes:

quoted

-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()

From: Adam Simpkins <hidden>
Date: 2016-06-15 22:47:17

On Tue, Aug 18, 2009 at 11:25:49PM -0700, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

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

From: Adam Simpkins <hidden>
Date: 2016-06-15 22:47:17

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 <redacted>
---

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:17

Adam Simpkins [off-list ref] 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 <redacted>
---

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.

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

From: Adam Simpkins <hidden>
Date: 2016-06-15 22:47:17

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help