Re: [PATCH] Lose perl dependency. (fwd)

Re: [PATCH] Lose perl dependency. (fwd)

[Johannes Schindelin]

Forwarded, since I forgot to add the list

---------- Forwarded message ----------
Date: Thu, 18 Jan 2007 11:46:05 +0100 (CET)
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Simon 'corecode' Schubert <corecode@fs.ei.tum.de>
Subject: Re: [PATCH] Lose perl dependency.

Hi,

On Thu, 18 Jan 2007, Simon 'corecode' Schubert wrote:

>  for cmt in `git-rev-list --no-merges "$upstream"..ORIG_HEAD \
> -			| @@PERL@@ -e 'print reverse <>'`
> +			| sed -ne '1!G;$p;h'`

Why not teach the revision machinery to output in reverse with 
"--reverse"?

Ciao,
Dscho

Re: [PATCH] Lose perl dependency. (fwd)

[Simon 'corecode' Schubert]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

Johannes Schindelin wrote:
> On Thu, 18 Jan 2007, Simon 'corecode' Schubert wrote:
> 
>>  for cmt in `git-rev-list --no-merges "$upstream"..ORIG_HEAD \
>> -			| @@PERL@@ -e 'print reverse <>'`
>> +			| sed -ne '1!G;$p;h'`
> 
> Why not teach the revision machinery to output in reverse with 
> "--reverse"?

I'm more in favour of "small is beautiful".  Also from looking at the code, this seems to be a bit complicated.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH] Lose perl dependency. (fwd)

[Johannes Schindelin]

Hi,

On Thu, 18 Jan 2007, Simon 'corecode' Schubert wrote:

> Johannes Schindelin wrote:
> > On Thu, 18 Jan 2007, Simon 'corecode' Schubert wrote:
> > 
> > >  for cmt in `git-rev-list --no-merges "$upstream"..ORIG_HEAD \
> > > -			| @@PERL@@ -e 'print reverse <>'`
> > > +			| sed -ne '1!G;$p;h'`
> > 
> > Why not teach the revision machinery to output in reverse with "--reverse"?
> 
> I'm more in favour of "small is beautiful".  Also from looking at the code,
> this seems to be a bit complicated.

I'm more in favour of "less shell dependecy is beautiful". And from what I 
can tell, it should be relatively easy:

---

	14 insertions and 11 deletions stem from moving (and extern'ing) 
	reverse_commit_list() from merge-recursive.c to commit.c

	So the change is actually 9 insertions and one deletion.

 commit.c          |   11 +++++++++++
 commit.h          |    3 +++
 merge-recursive.c |   11 -----------
 revision.c        |    7 +++++++
 revision.h        |    3 ++-
 5 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/commit.c b/commit.c
index f495e2d..2735283 100644
--- a/commit.c
+++ b/commit.c
@@ -1231,3 +1231,14 @@ int in_merge_bases(struct commit *commit, struct commit **reference, int num)
 	}
 	return ret;
 }
+
+struct commit_list *reverse_commit_list(struct commit_list *list)
+{
+	struct commit_list *next = NULL, *current, *backup;
+	for (current = list; current; current = backup) {
+		backup = current->next;
+		current->next = next;
+		next = current;
+	}
+	return next;
+}
diff --git a/commit.h b/commit.h
index b8e6e18..563fe86 100644
--- a/commit.h
+++ b/commit.h
@@ -115,4 +115,7 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,
 		int depth, int shallow_flag, int not_shallow_flag);
 
 int in_merge_bases(struct commit *, struct commit **, int);
+
+extern struct commit_list *reverse_commit_list(struct commit_list *list);
+
 #endif /* COMMIT_H */
diff --git a/merge-recursive.c b/merge-recursive.c
index fa320eb..75fec5b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1162,17 +1162,6 @@ static int merge_trees(struct tree *head,
 	return clean;
 }
 
-static struct commit_list *reverse_commit_list(struct commit_list *list)
-{
-	struct commit_list *next = NULL, *current, *backup;
-	for (current = list; current; current = backup) {
-		backup = current->next;
-		current->next = next;
-		next = current;
-	}
-	return next;
-}
-
 /*
  * Merge the commits h1 and h2, return the resulting virtual
  * commit object and a flag indicating the cleaness of the merge.
diff --git a/revision.c b/revision.c
index ebd0250..6dc00ba 100644
--- a/revision.c
+++ b/revision.c
@@ -1057,6 +1057,11 @@ 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;
+				revs->limited = 1;
+				continue;
+			}
 
 			opts = diff_opt_parse(&revs->diffopt, argv+i, argc-i);
 			if (opts > 0) {
@@ -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)
diff --git a/revision.h b/revision.h
index d93481f..3eb1ce4 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:1;
 
 	/* Diff flags */
 	unsigned int	diff:1,

Re: [PATCH] Lose perl dependency. (fwd)

[Simon 'corecode' Schubert]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

Johannes Schindelin wrote:
>>> Why not teach the revision machinery to output in reverse with "--reverse"?
>> I'm more in favour of "small is beautiful".  Also from looking at the code,
>> this seems to be a bit complicated.
> I'm more in favour of "less shell dependecy is beautiful". And from what I 
> can tell, it should be relatively easy:

fair enough.  looks good!

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH] Lose perl dependency. (fwd)

[Andy Parkins]

On Thursday 2007 January 18 13:57, Johannes Schindelin wrote:

I think this would be clearer if next were renamed...

> +struct commit_list *reverse_commit_list(struct commit_list *list)
> +{
> +	struct commit_list *prev = NULL, *current, *backup;
> +	for (current = list; current; current = backup) {
> +		backup = current->next;
> +		current->next = prev;
> +		prev = current;
> +	}
> +	return next;
> +}

Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE
andyparkins@gmail.com

Re: [PATCH] Lose perl dependency. (fwd)

[Johannes Schindelin]

Hi,

On Thu, 18 Jan 2007, Andy Parkins wrote:

> I think this would be clearer if next were renamed...

Existing code, no bug, won't touch.

Ciao,
Dscho

Re: [PATCH] Lose perl dependency. (fwd)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Thu, 18 Jan 2007, Simon 'corecode' Schubert wrote:
>
>> Johannes Schindelin wrote:
>> > On Thu, 18 Jan 2007, Simon 'corecode' Schubert wrote:
>> > 
>> > >  for cmt in `git-rev-list --no-merges "$upstream"..ORIG_HEAD \
>> > > -			| @@PERL@@ -e 'print reverse <>'`
>> > > +			| sed -ne '1!G;$p;h'`
>> > 
>> > Why not teach the revision machinery to output in reverse with "--reverse"?
>> 
>> I'm more in favour of "small is beautiful".  Also from looking at the code,
>> this seems to be a bit complicated.
>
> I'm more in favour of "less shell dependecy is beautiful". And from what I 
> can tell, it should be relatively easy:
>
> ---
>
> 	14 insertions and 11 deletions stem from moving (and extern'ing) 
> 	reverse_commit_list() from merge-recursive.c to commit.c
>
> 	So the change is actually 9 insertions and one deletion.

I think this is sane but I hate to having to worry about
possible fallouts from giving --reverse in setup_revisions() to
make it available to everybody.  E.g. things like "what happens
when you say "git format-patch --reverse HEAD~3".

Nevertheless, moving reverse_commit_list out of merge-recursive
is a good clean-up.

Re: [PATCH] Lose perl dependency. (fwd)

[Johannes Schindelin]

Hi,

On Fri, 19 Jan 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Hi,
> >
> > On Thu, 18 Jan 2007, Simon 'corecode' Schubert wrote:
> >
> >> Johannes Schindelin wrote:
> >> > On Thu, 18 Jan 2007, Simon 'corecode' Schubert wrote:
> >> > 
> >> > >  for cmt in `git-rev-list --no-merges "$upstream"..ORIG_HEAD \
> >> > > -			| @@PERL@@ -e 'print reverse <>'`
> >> > > +			| sed -ne '1!G;$p;h'`
> >> > 
> >> > Why not teach the revision machinery to output in reverse with "--reverse"?
> >> 
> >> I'm more in favour of "small is beautiful".  Also from looking at the code,
> >> this seems to be a bit complicated.
> >
> > I'm more in favour of "less shell dependecy is beautiful". And from what I 
> > can tell, it should be relatively easy:
> >
> > ---
> >
> > 	14 insertions and 11 deletions stem from moving (and extern'ing) 
> > 	reverse_commit_list() from merge-recursive.c to commit.c
> >
> > 	So the change is actually 9 insertions and one deletion.
> 
> I think this is sane but I hate to having to worry about
> possible fallouts from giving --reverse in setup_revisions() to
> make it available to everybody.  E.g. things like "what happens
> when you say "git format-patch --reverse HEAD~3".

It would

	1) traverse all commits, storing them in a commit_list,
	2) reverse the commits, and then
	3) continue as before.

So I don't really see a problem (after all, you don't have to use it if 
you don't want to). It would need a little longer to start up, since all 
the commits have to be traversed first, but this is inevitable if you want 
to show the last commit first.

> Nevertheless, moving reverse_commit_list out of merge-recursive is a 
> good clean-up.

Not unless we actually use it elsewhere.

Ciao,
Dscho

Re: [PATCH] Lose perl dependency. (fwd)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I think this is sane but I hate to having to worry about
>> possible fallouts from giving --reverse in setup_revisions() to
>> make it available to everybody.  E.g. things like "what happens
>> when you say "git format-patch --reverse HEAD~3".
>
> It would
>
> 	1) traverse all commits, storing them in a commit_list,
> 	2) reverse the commits, and then
> 	3) continue as before.
>
> So I don't really see a problem (after all, you don't have to use it if 
> you don't want to).

Well, I understand what the code does, but what does the above
three steps MEAN to the end users?

In other words, if it does not make sense for format-patch to
take --reverse, maybe we should keep it as an internal option,
just like git-show is the only user of no-walk.  And give option
parsing for it for only selected commands (like rev-list) where
it makes sense.

I am sure you can come up with a reason why the above three
steps are useful for the end user, and it could turn out to be a
very valid reason.

But format-patch was just one example.  I will have to worry
about all the users of revision traversal machinery.

The end result might be "ok, we have spent quite a lot of time
and audited every users of revision machinery and for all of
them --reverse has some valid use cases." and that would be
wonderful.  But the thing is, I hate to having to worry about
that right now.

Re: [PATCH] Lose perl dependency. (fwd)

[Johannes Schindelin]

Hi,

On Fri, 19 Jan 2007, Junio C Hamano wrote:

> In other words, if it does not make sense for format-patch to
> take --reverse, maybe we should keep it as an internal option,
> just like git-show is the only user of no-walk.

Or we just check in cmd_format_patch() (and possibly other users where 
--reverse does not make sense) for revs->reverse and die() upon it.

OTOH we can expect people _not_ to use --reverse with format-patch when 
they don't know what it does! I mean, I don't go and use "ls" with an 
option I saw in the man page, just because it has a cool ring to it.

Ciao,
Dscho

P.S.: Perhaps you should just stop worrying and learn to love --reverse 
;-)

Re: [PATCH] Lose perl dependency. (fwd)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> OTOH we can expect people _not_ to use --reverse with format-patch when 
> they don't know what it does! I mean, I don't go and use "ls" with an 
> option I saw in the man page, just because it has a cool ring to it.
>
> P.S.: Perhaps you should just stop worrying and learn to love --reverse 

I usually do not worry, especially while I am running 'next'
myself.

It's just unintended consequences I am worried about by touching
somewhere deep in the revision machinery after -rc1.

Re: [PATCH] Lose perl dependency. (fwd)

[Johannes Schindelin]

Hi,

On Fri, 19 Jan 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > OTOH we can expect people _not_ to use --reverse with format-patch when 
> > they don't know what it does! I mean, I don't go and use "ls" with an 
> > option I saw in the man page, just because it has a cool ring to it.
> >
> > P.S.: Perhaps you should just stop worrying and learn to love --reverse 
> 
> I usually do not worry, especially while I am running 'next'
> myself.
> 
> It's just unintended consequences I am worried about by touching
> somewhere deep in the revision machinery after -rc1.

I have no problem resending after 1.5.0 if you prefer that.

Ciao,
Dscho

Re: [PATCH] Lose perl dependency. (fwd)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> P.S.: Perhaps you should just stop worrying and learn to love --reverse 
> ;-)

Another thing to think about is how --reverse should interact
with --max-count and --skip (and perhaps --max-age but I am not
sure about that one).

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.

The former is probably more efficient (I do not think you would
need to artificially make it limited like your patch does if you
go this route), while the latter may or may not be more useful
for what the end users would want to do.

For example, "git log -4" would show the topmost four commits.
If you do the former, "git log --reverse -4" would give you the
same four but in the chronological order (we usually show in the
reverse order and --reverse would make it the forward order ;-),
and you do not need to do the limiting for this.  You need to
capture them and reverse them yourself anyway, so not having to
limit may not be a big deal, though.

If you do the latter, you would be able to get the first four
commits in the chronological order.  I do not think that is
usually of much practical value (although people new to git
always seem to ask "how do I get to the root commit" at least
once), but there may be some valid uses for that kind of
behaviour.

Re: [PATCH] Lose perl dependency. (fwd)

[Simon 'corecode' Schubert]

[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]

Junio C Hamano wrote:
> 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.

We were originally coming from replacing a perl -e 'print reverse <>' in git-rebase.  So I'd say the former.  

> If you do the latter, you would be able to get the first four
> commits in the chronological order.  I do not think that is
> usually of much practical value (although people new to git
> always seem to ask "how do I get to the root commit" at least
> once), but there may be some valid uses for that kind of
> behaviour.

But I doubt that "--reverse" would suggest that.

   Commit Ordering
       By default, the commits are shown in reverse chronological order.

so --reverse would mean no-reverse, i.e. forward.  well, acceptable :)  So if --reverse is an option to influence the output after the commit ordering, it is clearly the former.

I don't think the latter makes much sense, anyways.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH] Lose perl dependency. (fwd)

[Johannes Schindelin]

Hi,

On Fri, 19 Jan 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > P.S.: Perhaps you should just stop worrying and learn to love --reverse 
> > ;-)
> 
> Another thing to think about is how --reverse should interact
> with --max-count and --skip (and perhaps --max-age but I am not
> sure about that one).
> 
> 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.

Evidently, I did not even think about the latter. And I guess that most 
people expect the former, too. (Maybe we should make it a flipflop, so 
that "--reverse --reverse" unsets the reverse flag again?

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

Ciao,
Dscho

Re: [PATCH] Lose perl dependency. (fwd)

[Junio C Hamano]

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.

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.

Looking at your patch again,...

@@ -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.

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)

Re: [PATCH] Lose perl dependency. (fwd)

[Johannes Schindelin]

Hi,

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

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

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

> @@ -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.

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

Re: [PATCH] Lose perl dependency. (fwd)

[Robin Rosenberg]

lördag 20 januari 2007 23:04 skrev Johannes Schindelin:
> +--reverse::
> +
> +	Output the commits in reverse order.
> +

The --reverse is really bad naming since the default *is* to list commits in 
reverse order. Name it "--chronological" or something to indicate that you do 
not want the default reverse order. 

Some suggestions:
	--chronological
	--forward
	--noreverse
	--commit-order

-- robin

Re: [PATCH] Lose perl dependency. (fwd)

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 917 bytes --]

Hi,

On Sun, 21 Jan 2007, Robin Rosenberg wrote:

> lördag 20 januari 2007 23:04 skrev Johannes Schindelin:
> > +--reverse::
> > +
> > +	Output the commits in reverse order.
> > +
> 
> The --reverse is really bad naming since the default *is* to list 
> commits in reverse order.

Easy. In my worldview it is _not_ the reverse order. It is the most 
useful order, and thus straight-forward.

> Name it "--chronological" or something to indicate that you do not want 
> the default reverse order.

But it is not chronological. Take for example "git log --topo-order 
--reverse". See? Not chronological. Not even reverse chronological.

> Some suggestions:
> 	--chronological
> 	--forward
> 	--noreverse
> 	--commit-order

All of these have a high "Huh?" effect on me. "forward" is wrong, 
"noreverse" is confusing at best, and commit-order is anything but 
obvious. IOW those names don't solve a problem.

Ciao,
Dscho

Re: [PATCH] Lose perl dependency. (fwd)

[Bill Lear]

How about a '--order' switch?

   --order=chrono[logical]
   --order=rev[erse][-chrono[logical]] (default)

The switches "--reverse" and "--noreverse" are certainly confusing in
the context of a default that is "reverse chronological order".

BTW, my mailer is defaulting to sending mail to the original poster,
CCing others, including the mailing list.  I'm used to simply replying
to the list.  Is this the proper convention here, to reply directly to
humans and CC the list?


Bill

On Sunday, January 21, 2007 at 02:39:59 (+0100) Johannes Schindelin writes:
>Hi,
>
>On Sun, 21 Jan 2007, Robin Rosenberg wrote:
>
>> lördag 20 januari 2007 23:04 skrev Johannes Schindelin:
>> > +--reverse::
>> > +
>> > +	Output the commits in reverse order.
>> > +
>> 
>> The --reverse is really bad naming since the default *is* to list 
>> commits in reverse order.
>
>Easy. In my worldview it is _not_ the reverse order. It is the most 
>useful order, and thus straight-forward.
>
>> Name it "--chronological" or something to indicate that you do not want 
>> the default reverse order.
>
>But it is not chronological. Take for example "git log --topo-order 
>--reverse". See? Not chronological. Not even reverse chronological.
>
>> Some suggestions:
>> 	--chronological
>> 	--forward
>> 	--noreverse
>> 	--commit-order
>
>All of these have a high "Huh?" effect on me. "forward" is wrong, 
>"noreverse" is confusing at best, and commit-order is anything but 
>obvious. IOW those names don't solve a problem.
>
>Ciao,
>Dscho

Re: [PATCH] Lose perl dependency. (fwd)

[Junio C Hamano]

Bill Lear <rael@zopyra.com> writes:

> How about a '--order' switch?
>
>    --order=chrono[logical]
>    --order=rev[erse][-chrono[logical]] (default)
>
> The switches "--reverse" and "--noreverse" are certainly confusing in
> the context of a default that is "reverse chronological order".

I think --reverse is just fine.  It is "reverse" from usual, and
people already know (or they should learn) what the usual order
is.

> BTW, my mailer is defaulting to sending mail to the original poster,
> CCing others, including the mailing list.  I'm used to simply replying
> to the list.  Is this the proper convention here, to reply directly to
> humans and CC the list?

I only speak for myself, but I always prefer to address my
message's To: header to the person I am primarily talking to,
while leaving other people on Cc: line (which usually includes
the list address).

Re: [PATCH] Lose perl dependency. (fwd)

[David Kågedal]

Junio C Hamano <junkio@cox.net> writes:

> Bill Lear <rael@zopyra.com> writes:
>
>> How about a '--order' switch?
>>
>>    --order=chrono[logical]
>>    --order=rev[erse][-chrono[logical]] (default)
>>
>> The switches "--reverse" and "--noreverse" are certainly confusing in
>> the context of a default that is "reverse chronological order".
>
> I think --reverse is just fine.  It is "reverse" from usual, and
> people already know (or they should learn) what the usual order
> is.
>
>> BTW, my mailer is defaulting to sending mail to the original poster,
>> CCing others, including the mailing list.  I'm used to simply replying
>> to the list.  Is this the proper convention here, to reply directly to
>> humans and CC the list?
>
> I only speak for myself, but I always prefer to address my
> message's To: header to the person I am primarily talking to,
> while leaving other people on Cc: line (which usually includes
> the list address).

I, on the other hand, have recently been annoyed by having my inbox
filled with mails that I already can read on the list (actually the
gmane newsgruop).  So there is probably not a single good answer.

-- 
David Kågedal

Re: [PATCH] Lose perl dependency. (fwd)

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1367 bytes --]

Hi,

On Sun, 21 Jan 2007, David Kågedal wrote:

> Junio C Hamano <junkio@cox.net> writes:
> 
> > I only speak for myself, but I always prefer to address my message's 
> > To: header to the person I am primarily talking to, while leaving 
> > other people on Cc: line (which usually includes the list address).

I have to agree: if the mail is adressed directly to me, I am much more 
likely to read it. So I am quite annoyed by answers to my emails, which 
do not have me in To: or Cc:.

> I, on the other hand, have recently been annoyed by having my inbox 
> filled with mails that I already can read on the list (actually the 
> gmane newsgruop).  So there is probably not a single good answer.

Well, RFC 1855 "Netiquette guidelines" states in 2.1.2 "for mail:":

    - Watch cc's when replying.  Don't continue to include
      people if the messages have become a 2-way conversation.

(I myself am guilty of not culling people when no longer quoting them.)

The statement from the RFC obviously assumes that you reply to the author 
of the message (and since it came from a mailing list, if it is of 
interest to the list, you should Cc: that, too).

IMHO it is all to easy to filter duplicate messages, and generally not 
possible to identify replies to _your_ mails when the reply is not sent to 
_you_, but to the list.

IOW I agree with Junio.

Ciao,
Dscho

Re: [PATCH] Lose perl dependency. (fwd)

[Krzysztof Halasa]

David Kågedal <davidk@lysator.liu.se> writes:

> I, on the other hand, have recently been annoyed by having my inbox
> filled with mails that I already can read on the list (actually the
> gmane newsgruop).  So there is probably not a single good answer.

Actually there is, you can put Reply-To: in your messages.
-- 
Krzysztof Halasa

Re: [PATCH] Lose perl dependency. (fwd)

[David Kågedal]

Krzysztof Halasa <khc@pm.waw.pl> writes:

> David Kågedal <davidk@lysator.liu.se> writes:
>
>> I, on the other hand, have recently been annoyed by having my inbox
>> filled with mails that I already can read on the list (actually the
>> gmane newsgruop).  So there is probably not a single good answer.
>
> Actually there is, you can put Reply-To: in your messages.

That is never the correct answer.  If peaple want to reply to me, they
should be able to.  If they want to follow-up to the list, they should
be able to.  I can not decide in advance for them what they want to
do.

-- 
David Kågedal

Re: [PATCH] Lose perl dependency. (fwd)

[Krzysztof Halasa]

David Kågedal <davidk@lysator.liu.se> writes:

>> Actually there is, you can put Reply-To: in your messages.
>
> That is never the correct answer.  If peaple want to reply to me, they
> should be able to.  If they want to follow-up to the list, they should
> be able to.  I can not decide in advance for them what they want to
> do.

Actually it is the correct way to do that. If people "reply to group"
or whatever is it called, the reply goes to "Reply-To" list.
If they "reply to author", the reply goes to "From" address and
"Reply-To" is ignored.
-- 
Krzysztof Halasa

Re: [PATCH] Lose perl dependency. (fwd)

[Randal L. Schwartz]

>>>>> "Krzysztof" == Krzysztof Halasa <khc@pm.waw.pl> writes:

Krzysztof> Actually it is the correct way to do that. If people "reply to group"
Krzysztof> or whatever is it called, the reply goes to "Reply-To" list.
Krzysztof> If they "reply to author", the reply goes to "From" address and
Krzysztof> "Reply-To" is ignored.

Argh.  Such misinformation abounds on this topic!

The only reason a mail client has the ability to ignore the "reply-to"
(which *violates* the RFCs - go check it out) is because of *broken*
(but perhaps well-intentioned) mailing lists that add "reply-to: list".

The proper solution is to *follow* the RFCs, and leave the reply-to alone when
relaying.  If someone wants to reply to the list *and* the person, they can
reply to both the "from (or reply-to)" and "to" addresses, often called a
"wide reply".  If someone wants to reply to just the person, they use the
"reply-to" if it's present, or the "from" if not.  This is *proper* behavior:
it's only some broken mailing lists out there that have caused us to have to
work around it.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: [PATCH] Lose perl dependency. (fwd)

[Krzysztof Halasa]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

> The proper solution is to *follow* the RFCs,

Do you remember RFC #s by chance? Some details maybe?

> and leave the reply-to alone
> when
> relaying.  If someone wants to reply to the list *and* the person, they can
> reply to both the "from (or reply-to)" and "to" addresses, often called a
> "wide reply".  If someone wants to reply to just the person, they use the
> "reply-to" if it's present, or the "from" if not.  This is *proper* behavior:
> it's only some broken mailing lists out there that have caused us to have to
> work around it.

The list doesn't modify the headers in question, at least as far as lkml
is concerned, so I assume you mean some other lists that are broken.

Reply-To is for the author to set, list software shouldn't change it.
While it's normal to reply to "From", "Cc" and "To" (at least here),
some people prefer to be omitted when replying to their mail. How could
they do that if not with Reply-To?

Then, if I see lkml and no author's address in Reply-To, how
could I contact him/her privately if not writing to "From" and
ignoring Reply-To?
-- 
Krzysztof Halasa

Re: [PATCH] Lose perl dependency. (fwd)

[Krzysztof Halasa]

> The list doesn't modify the headers in question, at least as far as lkml
> is concerned

... and git list, too.
-- 
Krzysztof Halasa

Re: [PATCH] Lose perl dependency. (fwd)

[Simon 'corecode' Schubert]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

Johannes Schindelin wrote:
> @@ -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,

I like this.  However, rev_info.reverse needs some documentation.  Or the block in get_revision does:

/*
 * rev_info.reverse is used to note the fact that we want to output the list
 * of revisions in reverse order.  To accomplish this goal, reverse can have
 * different values:
 *  0  do nothing
 *  1  reverse the list
 *  2  internal use:  we have already obtained and reversed the list,
 *     now we only need to yield its items.
 */

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

[PATCH] Teach revision machinery about --reverse

[Johannes Schindelin]

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

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

	On Sun, 21 Jan 2007, Simon 'corecode' Schubert wrote:
	
	> > [the --reverse patch]
	
	Please do not quote parts of the mail you don't really refer to.
	
	> I like this.  However, rev_info.reverse needs some 
	> documentation.  Or the block in get_revision does:
	> 
	> /*
	> * rev_info.reverse is used to note the fact that we want to output the list
	> * of revisions in reverse order.  To accomplish this goal, reverse can have
	> * different values:
	> *  0  do nothing
	> *  1  reverse the list
	> *  2  internal use:  we have already obtained and reversed the list,
	> *     now we only need to yield its items.
	> */
	
	I liked the comment in get_revision() better.
	
	But then an idea just hit me: it might make sense to introduce 
	another flag instead, "no_walk", which says that revs->commits 
	should be walked as is, not walking parents.
	
	And then I saw it already exists. D'oh.

 Documentation/git-rev-list.txt |    5 +++++
 revision.c                     |   20 ++++++++++++++++++++
 revision.h                     |    3 ++-
 3 files changed, 27 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..6d512ff 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,22 @@ struct commit *get_revision(struct rev_info *revs)
 {
 	struct commit *c = NULL;
 
+	if (revs->reverse) {
+		struct commit_list *list;
+
+		revs->reverse = 0;
+		list = NULL;
+		while ((c = get_revision(revs)))
+			commit_list_insert(c, &list);
+		revs->commits = list;
+		revs->no_walk = 1;
+		/* reset flags */
+		while (list) {
+			list->item->object.flags &= ~(ADDED | SEEN | SHOWN);
+			list = list->next;
+		}
+	}
+
 	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..3eb1ce4 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:1;
 
 	/* Diff flags */
 	unsigned int	diff:1,
-- 
1.5.0.rc1.g956c1-dirty