rev-list: fix --reverse interaction with --parents

--reverse did not interact well with --parents, as the included test
case shows: in a history like

  A--B.
   \   \
    `C--M--D

the command

  git rev-list --reverse --parents --full-history HEAD

erroneously lists D as having no parents at all.  (Without --reverse,
it correctly lists M.)

This is caused by the machinery driving --reverse: it first grabs all
commits through the normal routines, then runs them through the same
routines again, effectively simplifying them twice.

Fix this by moving the --reverse one level up, into get_revision().
This way we can cleanly grab all commits via the normal calls, then
just pop them off the list one by one without interfering with
get_revision_internal().

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

3 files changed, 60 insertions, 21 deletions

diff --git a/revision.c b/revision.c
index 0aaa4c10b9..d797f05c93 100644
--- a/revision.c
+++ b/revision.c
@@ -1786,26 +1786,6 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 		return c;
 	}
 
-	if (revs->reverse) {
-		int limit = -1;
-
-		if (0 <= revs->max_count) {
-			limit = revs->max_count;
-			if (0 < revs->skip_count)
-				limit += revs->skip_count;
-		}
-		l = NULL;
-		while ((c = get_revision_1(revs))) {
-			commit_list_insert(c, &l);
-			if ((0 < limit) && !--limit)
-				break;
-		}
-		revs->commits = l;
-		revs->reverse = 0;
-		revs->max_count = -1;
-		c = NULL;
-	}
-
 	/*
 	 * Now pick up what they want to give us
 	 */
@@ -1878,7 +1858,23 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 
 struct commit *get_revision(struct rev_info *revs)
 {
-	struct commit *c = get_revision_internal(revs);
+	struct commit *c;
+	struct commit_list *reversed;
+
+	if (revs->reverse) {
+		reversed = NULL;
+		while ((c = get_revision_internal(revs))) {
+			commit_list_insert(c, &reversed);
+		}
+		revs->commits = reversed;
+		revs->reverse = 0;
+		revs->reverse_output_stage = 1;
+	}
+
+	if (revs->reverse_output_stage)
+		return pop_commit(&revs->commits);
+
+	c = get_revision_internal(revs);
 	if (c && revs->graph)
 		graph_update(revs->graph, c);
 	return c;
diff --git a/revision.h b/revision.h
index dfa06b5210..b818cea76b 100644
--- a/revision.h
+++ b/revision.h
@@ -53,6 +53,7 @@ struct rev_info {
 			rewrite_parents:1,
 			print_parents:1,
 			reverse:1,
+			reverse_output_stage:1,
 			cherry_pick:1,
 			first_parent_only:1;
 
diff --git a/t/t6013-rev-list-reverse-parents.sh b/t/t6013-rev-list-reverse-parents.sh
new file mode 100755
index 0000000000..d294466427
--- /dev/null
+++ b/t/t6013-rev-list-reverse-parents.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='--reverse combines with --parents'
+
+. ./test-lib.sh
+
+
+commit () {
+	test_tick &&
+	echo $1 > foo &&
+	git add foo &&
+	git commit -m "$1"
+}
+
+test_expect_success 'set up --reverse example' '
+	commit one &&
+	git tag root &&
+	commit two &&
+	git checkout -b side HEAD^ &&
+	commit three &&
+	git checkout master &&
+	git merge -s ours side &&
+	commit five
+	'
+
+test_expect_success '--reverse --parents --full-history combines correctly' '
+	git rev-list --parents --full-history master -- foo |
+		tac > expected &&
+	git rev-list --reverse --parents --full-history master -- foo \
+		> actual &&
+	test_cmp actual expected
+	'
+
+test_expect_success '--boundary does too' '
+	git rev-list --boundary --parents --full-history master ^root -- foo |
+		tac > expected &&
+	git rev-list --boundary --reverse --parents --full-history \
+		master ^root -- foo > actual &&
+	test_cmp actual expected
+	'
+
+test_done