Make 'git show' more useful

Make 'git show' more useful

[Linus Torvalds]

For some reason, I ended up doing

	git show HEAD~5..

as an odd way of asking for a log. I realize I should just have used "git 
log", but at the same time it does make perfect conceptual sense. After 
all, you _could_ have done

	git show HEAD HEAD~1 HEAD~2 HEAD~3 HEAD~4

and saying "git show HEAD~5.." is pretty natural. It's not like "git show" 
only ever showed a single commit (or other object) before either! So 
conceptually, giving a commit range is a very sensible operation, even 
though you'd traditionally have used "git log" for that.

However, doing that currently results in an error

	fatal: object ranges do not make sense when not walking revisions

which admittedly _also_ makes perfect sense - from an internal git 
implementation standpoint in 'revision.c'.

However, I think that asking to show a range makes sense to a user, while 
saying "object ranges no not make sense when not walking revisions" only 
makes sense to a git developer.

So on the whole, of the two different "makes perfect sense" behaviors, I 
think I originally picked the wrong one. And quite frankly, I don't really 
see anybody actually _depending_ on that error case. So why not change it?

So rather than error out, just turn that non-walking error case into a 
"silently turn on walking" instead.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

This is a total throw-away patch. I'm not going to re-send. Take it or 
not, I don't care, I spent more time writing this discussion than I did on 
the patch. I just happened to notice this behavior, and decided I probably 
would prefer this behavior. But it doesn't really matter in any kind of 
big picture.

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

and it will actually turn into a really odd way of saying "git show HEAD" 
(notice: _not_ "HEAD~5". HEAD). And even with this change you can do the 
reverse:

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

it it will first implicitly turn off walking ("git show"), then it will 
implicitly turn it on again (the commit range triggers the code in this 
patch), and then _after_ it has seen the commit range it will explicitly 
turn off walking again.

In other words, regardless of this patch you can do crazy things. I really 
don't think anybody cares. This patch is not meant to disable crazy 
things, it's meant to enable a reasonably sane user behavior.

 revision.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/revision.c b/revision.c
index a31434b..9f5dac5 100644
--- a/revision.c
+++ b/revision.c
@@ -133,7 +133,7 @@ void mark_parents_uninteresting(struct commit *commit)
 static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode)
 {
 	if (revs->no_walk && (obj->flags & UNINTERESTING))
-		die("object ranges do not make sense when not walking revisions");
+		revs->no_walk = 0;
 	if (revs->reflog_info && obj->type == OBJ_COMMIT &&
 			add_reflog_for_walk(revs->reflog_info,
 				(struct commit *)obj, name))

Re: Make 'git show' more useful

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> 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 ;-)

[PATCH] Re: Make 'git show' more useful

[Paolo Bonzini]

> 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 <bonzini@gnu.org>
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

Re: [PATCH] Re: Make 'git show' more useful

[Linus Torvalds]

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

Re: [PATCH] Re: Make 'git show' more useful

[Johannes Schindelin]

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"

Re: [PATCH] Re: Make 'git show' more useful

[Linus Torvalds]

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

Re: [PATCH] Re: Make 'git show' more useful

[Paolo Bonzini]

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

Re: [PATCH] Re: Make 'git show' more useful

[Johannes Schindelin]

Hi,

On Tue, 14 Jul 2009, Paolo Bonzini 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.
> 
> Very nice.  Can you add a test for that?

Unfortunately, no; I am very short on time.

Ciao,
Dscho

[PATCH] t4202-log.sh: Test git log --no-walk sort order

[Michael J Gruber]

'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 <git@drmicha.warpmail.net>
---
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

Re: [PATCH] t4202-log.sh: Test git log --no-walk sort order

[Johannes Sixt]

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

Re: [PATCH] t4202-log.sh: Test git log --no-walk sort order

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 14.07.2009 14:21:
> 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:

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

[PATCHv2] t4202-log.sh: Test git log --no-walk sort order

[Michael J Gruber]

'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 <git@drmicha.warpmail.net>
---
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 <j.sixt@viscovery.net>
...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

Re: [PATCHv2] t4202-log.sh: Test git log --no-walk sort order

[Johannes Schindelin]

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

Re: [PATCHv2] t4202-log.sh: Test git log --no-walk sort order

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 14.07.2009 16:13:
> 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).

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

[PATCH] t4202-log.sh: Test git log --no-walk sort order

[Michael J Gruber]

'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 <git@drmicha.warpmail.net>
---
 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

Re: [PATCH] t4202-log.sh: Test git log --no-walk sort order

[Johannes Schindelin]

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 <git@drmicha.warpmail.net>
> ---
>  t/t4202-log.sh |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> v3: For completeness' sake, use git show -s.

Thanks!

Ciao,
Dscho

Re: Make 'git show' more useful

[Johannes Schindelin]

Hi,

On Mon, 13 Jul 2009, Linus Torvalds wrote:

> For some reason, I ended up doing
> 
> 	git show HEAD~5..
> 
> as an odd way of asking for a log. I realize I should just have used "git 
> log", but at the same time it does make perfect conceptual sense.

For some reason, I wrote exactly this patch some time ago, but I forgot 
why I did not send it.  Probably because I did not want to appear as a 
total moron in public.  Which apparently I end up doing anyway.

So count me in on the supporters' side of this patch,
Dscho