threads / discuss / 20102

Make 'git show' more useful

Subject: Make 'git show' more useful

## tl;dr

17 messages between Jul 13, 2009 and Jul 17, 2009.

replies: 16people: 6as markdown or json

Linus Torvalds· Jul 13, 2009, 21:41 UTC · lore
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))
Junio C Hamano· Jul 13, 2009, 22:11 UTC · re: Linus Torvalds · lore

Re: Make 'git show' more useful

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 5 quoted lines
> 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 ;-)
Paolo Bonzini· Jul 13, 2009, 23:43 UTC · re: Junio C Hamano · lore

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

Show 6 quoted lines
> 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
Linus Torvalds· Jul 14, 2009, 00:00 UTC · re: Paolo Bonzini · lore

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

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
Johannes Schindelin· Jul 14, 2009, 01:25 UTC · re: Linus Torvalds · lore

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

Hi,
On Mon, 13 Jul 2009, Linus Torvalds wrote:
Show 5 quoted lines
> 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"

Linus Torvalds· Jul 14, 2009, 01:47 UTC · re: Johannes Schindelin · lore

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

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
Paolo Bonzini· Jul 14, 2009, 06:25 UTC · re: Johannes Schindelin · lore

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

Show 9 quoted lines
>> 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
Johannes Schindelin· Jul 14, 2009, 10:44 UTC · re: Paolo Bonzini · lore

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

Hi,
On Tue, 14 Jul 2009, Paolo Bonzini wrote:
Show 13 quoted lines
> > > 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

Michael J Gruber· Jul 14, 2009, 12:08 UTC · re: Johannes Schindelin · lore

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

'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
Johannes Sixt· Jul 14, 2009, 12:21 UTC · re: Michael J Gruber · lore

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

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
Michael J Gruber· Jul 14, 2009, 12:38 UTC · re: Johannes Sixt · lore

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

Johannes Sixt venit, vidit, dixit 14.07.2009 14:21:
Show 6 quoted lines
> 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
Michael J Gruber· Jul 14, 2009, 12:45 UTC · re: Johannes Sixt · lore

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

'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
Johannes Schindelin· Jul 14, 2009, 14:13 UTC · re: Michael J Gruber · lore

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

Hi,
On Tue, 14 Jul 2009, Michael J Gruber wrote:
Show 5 quoted lines
> '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.
Show 12 quoted lines
> +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

Michael J Gruber· Jul 14, 2009, 14:28 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin venit, vidit, dixit 14.07.2009 16:13:
Show 28 quoted lines
> 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
Michael J Gruber· Jul 17, 2009, 14:28 UTC · re: Johannes Schindelin · lore

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

'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
Johannes Schindelin· Jul 17, 2009, 14:53 UTC · re: Michael J Gruber · lore

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

Hi,
On Fri, 17 Jul 2009, Michael J Gruber wrote:
Show 12 quoted lines
> '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

Johannes Schindelin· Jul 13, 2009, 23:42 UTC · re: Linus Torvalds · lore

Re: Make 'git show' more useful

Hi,
On Mon, 13 Jul 2009, Linus Torvalds wrote:
Show 6 quoted lines
> 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

← back to recent threads