Re: Make 'git show' more useful

Linus Torvalds [off-list ref] writes:

For some reason, I ended up doing

	git show HEAD~5..

as an odd way of asking for a log.

Heh, I like that already.  Thanks ;-)

And, admittedly, you can _already_ do this by just adding "--do-walk"
whenever you specify a range. And equally admittedly, you can already
confuse git by adding the "--no-walk" _after_ specifying the range,
ie you can do this:

	git log HEAD~5.. --no-walk

Even without the change you could do

	git show --do-walk HEAD~5.. --no-walk

But then why do we want --do-walk and --no-walk?  You can always use "git
rev-parse" instead of "git rev-list --no-walk" (just check that the output 
is a single SHA1 id), and I don't think it is so important to be able
to say "git log --no-walk" instead of "git log -1".

They are not tested either.  Just gitk cares about --no-walk... to prevent
the user from giving it.

So, what about squashing this with Linus's patch?  (This is meant to be
squashed, which is why this text is not in a cover letter).  Still:

Signed-off-by: Paolo Bonzini <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 Documentation/git-rev-list.txt     |    1 -
 Documentation/rev-list-options.txt |    8 --------
 revision.c                         |   13 ++-----------
 gitk-git/gitk                      |    1 -+
 3 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 1c9cc28..b02cf54 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt

@@ -44,7 +44,6 @@ SYNOPSIS
 	     [ \--merge ]
 	     [ \--reverse ]
 	     [ \--walk-reflogs ]
-	     [ \--no-walk ] [ \--do-walk ]
 	     <commit>... [ \-- <paths>... ]
 
 DESCRIPTION

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 11eec94..d137e32 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt

@@ -624,11 +624,3 @@ These options are mostly targeted for packing of git repositories.
 
 	Only useful with '--objects'; print the object IDs that are not
 	in packs.
-
---no-walk::
-
-	Only show the given revs, but do not traverse their ancestors.
-
---do-walk::
-
-	Overrides a previous --no-walk.

diff --git a/revision.c b/revision.c
index a31434b..8b1a385 100644
--- a/revision.c
+++ b/revision.c

@@ -993,8 +993,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	/* pseudo revision arguments */
 	if (!strcmp(arg, "--all") || !strcmp(arg, "--branches") ||
 	    !strcmp(arg, "--tags") || !strcmp(arg, "--remotes") ||
-	    !strcmp(arg, "--reflog") || !strcmp(arg, "--not") ||
-	    !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk"))
+	    !strcmp(arg, "--reflog") || !strcmp(arg, "--not"))
 	{
 		unkv[(*unkc)++] = arg;
 		return 1;

@@ -1273,14 +1272,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 				flags ^= UNINTERESTING;
 				continue;
 			}
-			if (!strcmp(arg, "--no-walk")) {
-				revs->no_walk = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--do-walk")) {
-				revs->no_walk = 0;
-				continue;
-			}
 
 			opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);
 			if (opts > 0) {

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 4604c83..984d30a 100644
--- a/gitk-git/gitk
+++ b/gitk-git/gitk

@@ -169,7 +169,7 @@ proc parseviewargs {n arglist} {
 	    "--name-only" - "--name-status" - "--color" - "--color-words" -
 	    "--log-size" - "--pretty=*" - "--decorate" - "--abbrev-commit" -
 	    "--cc" - "-z" - "--header" - "--parents" - "--boundary" -
-	    "--no-color" - "-g" - "--walk-reflogs" - "--no-walk" -
+	    "--no-color" - "-g" - "--walk-reflogs" -
 	    "--timestamp" - "relative-date" - "--date=*" - "--stdin" -
 	    "--objects" - "--objects-edge" - "--reverse" {
 		# These cause our parsing of git log's output to fail, or else

-- 
1.6.2.5

On Tue, 14 Jul 2009, Paolo Bonzini wrote:

So, what about squashing this with Linus's patch?  (This is meant to be
squashed, which is why this text is not in a cover letter).

I wouldn't squash it.

That said, in the original commit that introduced "no_walk" (ba1d4505), I 
said

    I was going to add "--no-walk" as a real argument flag to git-rev-list
    too, but I'm not sure anybody actually needs it. Although it might be
    useful for porcelain, so I left the door open.

and I never actually did it. That was Apr 15, 2006.

The actual "--no-walk" flag was then added over a year later by Dsco, in 
commit 8e64006eee ("Teach revision machinery about --no-walk").

Doing a "git log -p -S--no-walk", I have to admit that I don't find a 
single actual _use_ of --no-walk. And it obviously wasn't even exported 
until a year after it was internally implemented.

So I have to agree with the fact that "--no-walk" and "--do-walk" seem to 
be pretty worthless as command line switches.  Removing them might be a 
good thing.

However, doing some googling, I do actually find examples of it on the 
web. And some of them even appear valid:

	second_parent=$(git rev-list --no-walk --parents $newrev | sed 's/ /\n/g' | grep -v $newrev | tail --lines=1)

because you can't use "git rev-parse" with --parents (of course, I'm not 
at all clear on why it doesn't do

	second_parent=$(git rev-parse "$newrev"^2)

but that's really immaterial - the point is that "git rev-parse" is _not_ 
a replacement for "git rev-list --no-walk").

So I dunno. I think we might as well leave --no-walk and --do-walk around, 
even though they are of dubious value. They do mirror the internal 
revision walking logic very directly. 

			Linus

Hi,

On Mon, 13 Jul 2009, Linus Torvalds wrote:

The actual "--no-walk" flag was then added over a year later by Dsco, in 
commit 8e64006eee ("Teach revision machinery about --no-walk").

Doing a "git log -p -S--no-walk", I have to admit that I don't find a 
single actual _use_ of --no-walk.

Actually, I remember very precisely why I introduced it.  The difference 
between "git log --no-walk a b c" and "git show a b c" is that "git log" 
sorts the commits by commit time, which is pretty important in my case.

So I'd appreciate not doing away with that option.

Thanks,
Dscho "whose name has an h in it"

On Tue, 14 Jul 2009, Johannes Schindelin wrote:

Dscho "whose name has an h in it"

Picky, picky.

At least your name isn't Russell, in which case you'd be losing that final 
'l' _all_ the time due to the world-wide shortage of said letters. The 'h' 
goes missing only during temporary outages.

		Linus

quoted

The actual "--no-walk" flag was then added over a year later by Dsco, in
commit 8e64006eee ("Teach revision machinery about --no-walk").

Doing a "git log -p -S--no-walk", I have to admit that I don't find a
single actual _use_ of --no-walk.

Actually, I remember very precisely why I introduced it.  The difference
between "git log --no-walk a b c" and "git show a b c" is that "git log"
sorts the commits by commit time, which is pretty important in my case.

Very nice.  Can you add a test for that?

(OTOH do-walk seems to be superseded by Linus's patch).

Paolo

Hi,

On Tue, 14 Jul 2009, Paolo Bonzini wrote:

quoted

quoted

The actual "--no-walk" flag was then added over a year later by 
Dsco, in commit 8e64006eee ("Teach revision machinery about 
--no-walk").

Doing a "git log -p -S--no-walk", I have to admit that I don't find 
a single actual _use_ of --no-walk.

Actually, I remember very precisely why I introduced it.  The 
difference between "git log --no-walk a b c" and "git show a b c" is 
that "git log" sorts the commits by commit time, which is pretty 
important in my case.

Very nice.  Can you add a test for that?

Unfortunately, no; I am very short on time.

Ciao,
Dscho

'git log --no-walk' sorts commits by commit time whereas 'git show' does
not. Document this by two tests so that we never forget why ba1d450
(Tentative built-in "git show", 2006-04-15) introduced it and
8e64006 (Teach revision machinery about --no-walk, 2007-07-24) exposed
it as an option argument.

Signed-off-by: Michael J Gruber <redacted>
---
Not much to add here, besides the fact that the actual test target
should justify testing log as well as show here.

Based off master.

 t/t4202-log.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index aad3894..10ad5d2 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh

@@ -149,6 +149,29 @@ test_expect_success 'git log --follow' '
 
 '
 
+cat > expect << EOF
+804a787 sixth
+394ef78 fifth
+5d31159 fourth
+EOF
+test_expect_success 'git log --no-walk <commits> sorts by commit time' '
+	git log --no-walk --oneline 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
+cat > expect << EOF
+5d31159 fourth
+ein
+804a787 sixth
+a/two
+394ef78 fifth
+a/two
+EOF
+test_expect_success 'git show <commits> does not sort by commit time' '
+	git show --oneline --name-only 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup case sensitivity tests' '
 	echo case >one &&
 	test_tick &&

-- 
1.6.3.3.483.g4f5e

Michael J Gruber schrieb:

+test_expect_success 'git log --no-walk <commits> sorts by commit time' '

...

+test_expect_success 'git show <commits> does not sort by commit time' '

Thanks, but sorry that I'm nit-picking here: You say what git show does
not do, but shouldn't you say what git show should do?

	'git show shows commits in command line order'

-- Hannes

Johannes Sixt venit, vidit, dixit 14.07.2009 14:21:

Michael J Gruber schrieb:

quoted

+test_expect_success 'git log --no-walk <commits> sorts by commit time' '

...

quoted

+test_expect_success 'git show <commits> does not sort by commit time' '

Thanks, but sorry that I'm nit-picking here:

This not being my first patch, I don't expect any patch to go through
without nit-picking here - which, in most cases, is really a good thing ;)

You say what git show does
not do, but shouldn't you say what git show should do?

	'git show shows commits in command line order'

Well, I assumed "does not sort" == "leaves as is". But, even if it is,
still "does not sort by commit time" != "leaves as is". v2 upcoming...

Michael

'git log --no-walk' sorts commits by commit time whereas 'git show' does
not (it leaves them as given on the command line). Document this by two
tests so that we never forget why ba1d450 (Tentative built-in "git
show", 2006-04-15) introduced it and 8e64006 (Teach revision machinery
about --no-walk, 2007-07-24) exposed it as an option argument.

Signed-off-by: Michael J Gruber <redacted>
---
v2: Make it clearer (in the commmit message and the test description) that git
show leaves the commits as specified.

Nit-picked-by: Johannes Sixt [off-list ref]
...but he was right!

 t/t4202-log.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index aad3894..72ba42c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh

@@ -149,6 +149,29 @@ test_expect_success 'git log --follow' '
 
 '
 
+cat > expect << EOF
+804a787 sixth
+394ef78 fifth
+5d31159 fourth
+EOF
+test_expect_success 'git log --no-walk <commits> sorts by commit time' '
+	git log --no-walk --oneline 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
+cat > expect << EOF
+5d31159 fourth
+ein
+804a787 sixth
+a/two
+394ef78 fifth
+a/two
+EOF
+test_expect_success 'git show <commits> leaves list of commits as given' '
+	git show --oneline --name-only 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup case sensitivity tests' '
 	echo case >one &&
 	test_tick &&

-- 
1.6.3.3.483.g4f5e

Hi,

On Tue, 14 Jul 2009, Michael J Gruber wrote:

'git log --no-walk' sorts commits by commit time whereas 'git show' does
not (it leaves them as given on the command line). Document this by two
tests so that we never forget why ba1d450 (Tentative built-in "git
show", 2006-04-15) introduced it and 8e64006 (Teach revision machinery
about --no-walk, 2007-07-24) exposed it as an option argument.

Thanks.

+cat > expect << EOF
+5d31159 fourth
+ein
+804a787 sixth
+a/two
+394ef78 fifth
+a/two
+EOF
+test_expect_success 'git show <commits> leaves list of commits as given' '
+	git show --oneline --name-only 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'

Just to hazard a guess: you probably used --name-only to avoid having the 
whole diff in the output, right?  In that case, you might want to use -s 
in the future (I do not think this needs fixing in this patch).

Ciao,
Dscho

Johannes Schindelin venit, vidit, dixit 14.07.2009 16:13:

Hi,

On Tue, 14 Jul 2009, Michael J Gruber wrote:

quoted

'git log --no-walk' sorts commits by commit time whereas 'git show' does
not (it leaves them as given on the command line). Document this by two
tests so that we never forget why ba1d450 (Tentative built-in "git
show", 2006-04-15) introduced it and 8e64006 (Teach revision machinery
about --no-walk, 2007-07-24) exposed it as an option argument.

Thanks.

quoted

+cat > expect << EOF
+5d31159 fourth
+ein
+804a787 sixth
+a/two
+394ef78 fifth
+a/two
+EOF
+test_expect_success 'git show <commits> leaves list of commits as given' '
+	git show --oneline --name-only 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'

Just to hazard a guess: you probably used --name-only to avoid having the 
whole diff in the output, right?  In that case, you might want to use -s 
in the future (I do not think this needs fixing in this patch).

Yes, exactly. I was looking for "--no-p". And I was looking really hard!
I didn't see this in git-log.1 nor git-show.1 nor git-diff.1. Now,
looking again, I find it in git-diff-tree.1. Grrmml. With hindsight,
it's clear that all diff-tree options apply.

It seems that more of git-diff-tree.txt should show up in the man pages
for diff, log and show (i.e. be in diff-*.txt) or at least be
referenced. What do you think?

Michael

'git log --no-walk' sorts commits by commit time whereas 'git show' does
not (it leaves them as given on the command line). Document this by two
tests so that we never forget why ba1d450 (Tentative built-in "git
show", 2006-04-15) introduced it and 8e64006 (Teach revision machinery
about --no-walk, 2007-07-24) exposed it as an option argument.

Signed-off-by: Michael J Gruber <redacted>
---
 t/t4202-log.sh |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

v3: For completeness' sake, use git show -s.

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index aad3894..48e0088 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh

@@ -149,6 +149,26 @@ test_expect_success 'git log --follow' '
 
 '
 
+cat > expect << EOF
+804a787 sixth
+394ef78 fifth
+5d31159 fourth
+EOF
+test_expect_success 'git log --no-walk <commits> sorts by commit time' '
+	git log --no-walk --oneline 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
+cat > expect << EOF
+5d31159 fourth
+804a787 sixth
+394ef78 fifth
+EOF
+test_expect_success 'git show <commits> leaves list of commits as given' '
+	git show --oneline -s 5d31159 804a787 394ef78 > actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup case sensitivity tests' '
 	echo case >one &&
 	test_tick &&

-- 
1.6.3.3.483.g4f5e

Hi,

On Fri, 17 Jul 2009, Michael J Gruber wrote:

'git log --no-walk' sorts commits by commit time whereas 'git show' does
not (it leaves them as given on the command line). Document this by two
tests so that we never forget why ba1d450 (Tentative built-in "git
show", 2006-04-15) introduced it and 8e64006 (Teach revision machinery
about --no-walk, 2007-07-24) exposed it as an option argument.

Signed-off-by: Michael J Gruber <redacted>
---
 t/t4202-log.sh |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

v3: For completeness' sake, use git show -s.

Thanks!

Ciao,
Dscho