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,


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

Ciao,
Dscho

-

[ message continues ]

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

cheers
  simon

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

[ message continues ]

Hi,


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 ...
[ message continues ]

fair enough.  looks good!

cheers
  simon

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

[ message continues ]

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


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

[ message continues ]

Hi,


Existing code, no bug, won't touch.

Ciao,
Dscho
-

[ message continues ]

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.

-

[ message continues ]

Hi,


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 

Not unless we actually use it elsewhere.

Ciao,
Dscho

-

[ message continues ]

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.

-

[ message continues ]

Hi,


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

-

[ message continues ]

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.

-

[ message continues ]

Hi,


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

Ciao,
Dscho

-

[ message continues ]

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.

-

[ message continues ]

We were originally coming from replacing a perl -e 'print reverse <>' in =

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 :)  S=
o if --reverse is an option to influence the output after the commit orde=
ring, it is clearly the former.

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

cheers
  simon

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

[ message continues ]

Hi,


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 

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

-

[ message continues ]