revwalk: fix uninteresting revs sometimes not limiting graphwalk

When we want to limit our graphwalk, we use the heuristic of checking
whether the newest limiting (uninteresting) revision is newer than the
oldest interesting revision. We do so by inspecting whether the first
item's commit time of the user-supplied list of revisions is newer than
the last added interesting revision. This is wrong though, as the user
supplied list is in no way guaranteed to be sorted by increasing commit
dates. This could lead us to abort the revwalk early before applying all
relevant limiting revisions, outputting revisions which should in fact
have been hidden.

Fix the heuristic by instead checking whether _any_ of the limiting
commits was made earlier than the last interesting commit. Add a test.

10 files changed, 53 insertions, 24 deletions

diff --git a/src/revwalk.c b/src/revwalk.c
index b1aa8f91a..eb228a522 100644
--- a/src/revwalk.c
+++ b/src/revwalk.c
@@ -375,20 +375,6 @@ static int add_parents_to_list(git_revwalk *walk, git_commit_list_node *commit,
 	return 0;
 }
 
-static int everybody_uninteresting(git_commit_list *orig)
-{
-	git_commit_list *list = orig;
-
-	while (list) {
-		git_commit_list_node *commit = list->item;
-		list = list->next;
-		if (!commit->uninteresting)
-			return 0;
-	}
-
-	return 1;
-}
-
 /* How many unintersting commits we want to look at after we run out of interesting ones */
 #define SLOP 5
 
@@ -398,16 +384,15 @@ static int still_interesting(git_commit_list *list, int64_t time, int slop)
 	if (!list)
 		return 0;
 
-	/*
-	 * If the destination list has commits with an earlier date
-	 * than our source we want to continue looking.
-	 */
-	if (time <= list->item->time)
-		return SLOP;
-
-	/* If we find interesting commits, we reset the slop count */
-	if (!everybody_uninteresting(list))
-		return SLOP;
+	for (; list; list = list->next) {
+		/*
+		 * If the destination list has commits with an earlier date than
+		 * our source or if it still contains interesting commits we
+		 * want to continue looking.
+		 */
+		if (!list->item->uninteresting || list->item->time > time)
+			return SLOP;
+	}
 
 	/* Everything's uninteresting, reduce the count */
 	return slop - 1;
diff --git a/tests/resources/revwalk.git/HEAD b/tests/resources/revwalk.git/HEAD
new file mode 100644
index 000000000..cb089cd89
--- /dev/null
+++ b/tests/resources/revwalk.git/HEAD
@@ -0,0 +1 @@
+ref: refs/heads/master
diff --git a/tests/resources/revwalk.git/config b/tests/resources/revwalk.git/config
new file mode 100644
index 000000000..7c968c3b5
--- /dev/null
+++ b/tests/resources/revwalk.git/config
@@ -0,0 +1,6 @@
+[core]
+	bare = true
+	repositoryformatversion = 0
+	filemode = false
+	symlinks = false
+	ignorecase = true
diff --git a/tests/resources/revwalk.git/description b/tests/resources/revwalk.git/description
new file mode 100644
index 000000000..498b267a8
--- /dev/null
+++ b/tests/resources/revwalk.git/description
@@ -0,0 +1 @@
+Unnamed repository; edit this file 'description' to name the repository.
diff --git a/tests/resources/revwalk.git/objects/info/packs b/tests/resources/revwalk.git/objects/info/packs
new file mode 100644
index 000000000..d8d85b895
--- /dev/null
+++ b/tests/resources/revwalk.git/objects/info/packs
@@ -0,0 +1,2 @@
+P pack-9adacb9971981a1a3264fd473da5b800f2715959.pack
+
diff --git a/tests/resources/revwalk.git/objects/pack/pack-9adacb9971981a1a3264fd473da5b800f2715959.idx b/tests/resources/revwalk.git/objects/pack/pack-9adacb9971981a1a3264fd473da5b800f2715959.idx
new file mode 100644
index 000000000..e157b386e
--- /dev/null
+++ b/tests/resources/revwalk.git/objects/pack/pack-9adacb9971981a1a3264fd473da5b800f2715959.idx
diff --git a/tests/resources/revwalk.git/objects/pack/pack-9adacb9971981a1a3264fd473da5b800f2715959.pack b/tests/resources/revwalk.git/objects/pack/pack-9adacb9971981a1a3264fd473da5b800f2715959.pack
new file mode 100644
index 000000000..2a61f9403
--- /dev/null
+++ b/tests/resources/revwalk.git/objects/pack/pack-9adacb9971981a1a3264fd473da5b800f2715959.pack
diff --git a/tests/resources/revwalk.git/packed-refs b/tests/resources/revwalk.git/packed-refs
new file mode 100644
index 000000000..905a3db51
--- /dev/null
+++ b/tests/resources/revwalk.git/packed-refs
@@ -0,0 +1,7 @@
+# pack-refs with: peeled fully-peeled sorted 
+3ae0f53011bdb7e68f99bde4943449f36c1c318a refs/heads/A
+061978578d7c9ff2ba92dd36d31fd8d809871030 refs/heads/B
+743398b425d6c216d6cfaae3786b5bc436393ae5 refs/heads/C
+790ba0facf6fd103699a5c40cd19dad277ff49cd refs/heads/D
+d3d783066cf7d95def6844b9c5118c1e7bcce7df refs/heads/E
+d3d783066cf7d95def6844b9c5118c1e7bcce7df refs/heads/master
diff --git a/tests/resources/revwalk.git/refs/.gitkeep b/tests/resources/revwalk.git/refs/.gitkeep
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/tests/resources/revwalk.git/refs/.gitkeep
diff --git a/tests/revwalk/basic.c b/tests/revwalk/basic.c
index 6a23701f3..1106bf4ce 100644
--- a/tests/revwalk/basic.c
+++ b/tests/revwalk/basic.c
@@ -555,3 +555,30 @@ void test_revwalk_basic__old_hidden_commit_two(void)
 
 	cl_git_fail_with(GIT_ITEROVER, git_revwalk_next(&oid, _walk));
 }
+
+/*
+ * Ensure that we correctly hide all parent commits of a newer
+ * commit when first hiding older commits.
+ *
+ * % git rev-list D ^B ^A ^E
+ * 790ba0facf6fd103699a5c40cd19dad277ff49cd
+ * b82cee5004151ae0c4f82b69fb71b87477664b6f
+ */
+void test_revwalk_basic__newer_hidden_commit_hides_old_commits(void)
+{
+	git_oid oid;
+
+	revwalk_basic_setup_walk("revwalk.git");
+
+	cl_git_pass(git_revwalk_push_ref(_walk, "refs/heads/D"));
+	cl_git_pass(git_revwalk_hide_ref(_walk, "refs/heads/B"));
+	cl_git_pass(git_revwalk_hide_ref(_walk, "refs/heads/A"));
+	cl_git_pass(git_revwalk_hide_ref(_walk, "refs/heads/E"));
+
+	cl_git_pass(git_revwalk_next(&oid, _walk));
+	cl_assert(git_oid_streq(&oid, "b82cee5004151ae0c4f82b69fb71b87477664b6f"));
+	cl_git_pass(git_revwalk_next(&oid, _walk));
+	cl_assert(git_oid_streq(&oid, "790ba0facf6fd103699a5c40cd19dad277ff49cd"));
+
+	cl_git_fail_with(GIT_ITEROVER, git_revwalk_next(&oid, _walk));
+}