Hi,

On Sat, 20 Jan 2007, Junio C Hamano wrote:

quoted text
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I think there are two very valid ways.  You determine what you
> >> would spit out as if there is no --reverse, and then reverse the
> >> result, or you do not limit with them to get everthing, reverse
> >> the result and do the counting limit on that reversed list.
> > ...
> >> I do not think you would need to artificially make it limited like your 
> >> patch does if you go this route
> >
> > Why? To see the last commit (which should be output first), I _have_ to 
> > traverse them first, before reversing the order. I thought revs->limited 
> > does exactly that -- traverse all commits first. Am I mistaken?
> 
> I think you are talking about the second semantics; I was
> talking about the first one.  In other words, the one whose
> semantics of:
> 
> 	$ git log --max-count=10 --skip=5 --reverse HEAD
> 
> is to first internally run
> 
> 	$ git log --max-count=10 --skip=5 HEAD
> 
> then reverse the resulting 10 commits and spit them out.

That is exactly what I meant.

quoted text
> Now, "git log --max-count=10 --skip=5" does not need to call
> limit_list().  It needs to traverse the usual date-sorted
> revs->commits for fifteen rounds.

Yes. But I have to traverse this _first_, before even returning a commit 
from get_revision().

I had the impression that limit_list() traversed all commits. But I am 
probably wrong, ain't I?

quoted text
> @@ -1155,6 +1160,8 @@ void prepare_revision_walk(struct rev_info *revs)
>  		sort_in_topological_order_fn(&revs->commits, revs->lifo,
>  					     revs->topo_setter,
>  					     revs->topo_getter);
> +	if (revs->reverse)
> +		revs->commits = reverse_commit_list(revs->commits);
>  }
>  
>  static int rewrite_one(struct rev_info *revs, struct commit **pp)
> 
> This makes the code traverse and grab everything and then
> reverse; the later get_revision() -> get_revision_1() loop skips
> 5, returns 10 and then finally stops.  In other words, this
> gives 10 old commits counting from the 6th oldest one in the
> history.

Okay, I thought that limit_list() honours --skip and --max-count. Looking 
at the code it seems to me that this assumption is wrong.

quoted text
> If we prefer the first semantics, we do not have to traverse and
> grab everything.  That is what I was getting at.
> 
> That is, something like this, with your option parsing change
> (modulo we _might_ want to explicitly mark some of the users
> incompatible), addition of reverse field to struct rev_info,
> moving reverse_commit_list() to a more public place, but without
> making the reverse to imply limited traversal.
> 
> diff --git a/revision.c b/revision.c
> index f2ddd95..161c4c0 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1274,6 +1274,14 @@ struct commit *get_revision(struct rev_info *revs)
>  {
>  	struct commit *c = NULL;
>  
> +	if (revs->reverse) {
> +		/* we were asked to reverse, but haven't reversed the
> +		 * result, yet, so do it here once
> +		 */
> +		revs->commits = reverse_commit_list(revs->commits);
> +		revs->reverse = 0;
> +	}
> +
>  	if (0 < revs->skip_count) {
>  		while ((c = get_revision_1(revs)) != NULL) {
>  			if (revs->skip_count-- <= 0)

But that would not work, would it?

Example:

A - B - C - D

D is the HEAD. Now, when we do not limit_list(), when we get into 
get_revision() for the first time, revs->commits contains _only_ D (we do 
the ancestry walk on-the-fly). So, your code would "reverse" the list 
containing only D, reset the reverse flag. In effect, it would do exactly 
the same as without --reverse.

What I _wanted_, was to walk the ancestry chain first, then just reverse 
the commits, and be done. However, it seems I was utterly mistaken in my 
approach. This should work better:

---
[PATCH] Teach revision machinery about --reverse

The option --reverse reverses the order of the commits.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

 Documentation/git-rev-list.txt |    5 +++++
 revision.c                     |   25 +++++++++++++++++++++++++
 revision.h                     |    3 ++-
 3 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 86c94e7..6bb9f51 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -27,6 +27,7 @@ SYNOPSIS
 	     [ \--pretty | \--header ]
 	     [ \--bisect ]
 	     [ \--merge ]
+	     [ \--reverse ]
 	     <commit>... [ \-- <paths>... ]
 
 DESCRIPTION
@@ -249,6 +250,10 @@ By default, the commits are shown in reverse chronological order.
 	parent comes before all of its children, but otherwise things
 	are still ordered in the commit timestamp order.
 
+--reverse::
+
+	Output the commits in reverse order.
+
 Object Traversal
 ~~~~~~~~~~~~~~~~
 
diff --git a/revision.c b/revision.c
index ebd0250..afc824c 100644
--- a/revision.c
+++ b/revision.c
@@ -1057,6 +1057,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 					git_log_output_encoding = "";
 				continue;
 			}
+			if (!strcmp(arg, "--reverse")) {
+				revs->reverse ^= 1;
+				continue;
+			}
 
 			opts = diff_opt_parse(&revs->diffopt, argv+i, argc-i);
 			if (opts > 0) {
@@ -1285,6 +1289,27 @@ struct commit *get_revision(struct rev_info *revs)
 {
 	struct commit *c = NULL;
 
+	if (revs->reverse) {
+		struct commit_list *list;
+
+		if (revs->reverse == 1) {
+			revs->reverse = 0;
+			list = NULL;
+			while ((c = get_revision(revs)))
+				commit_list_insert(c, &list);
+			revs->commits = list;
+			revs->reverse = 2;
+		}
+
+		if (!revs->commits)
+			return NULL;
+		c = revs->commits->item;
+		list = revs->commits->next;
+		free(revs->commits);
+		revs->commits = list;
+		return c;
+	}
+
 	if (0 < revs->skip_count) {
 		while ((c = get_revision_1(revs)) != NULL) {
 			if (revs->skip_count-- <= 0)
diff --git a/revision.h b/revision.h
index d93481f..5fec184 100644
--- a/revision.h
+++ b/revision.h
@@ -42,7 +42,8 @@ struct rev_info {
 			unpacked:1, /* see also ignore_packed below */
 			boundary:1,
 			left_right:1,
-			parents:1;
+			parents:1,
+			reverse:2;
 
 	/* Diff flags */
 	unsigned int	diff:1,
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html