[PATCH] Documentation/git-diff: remove -r from --name-status example

[PATCH] Documentation/git-diff: remove -r from --name-status example

[Jeff King]

Calling 'git-diff --name-status' will recursively show any
changes already, and it has for quite some time (at least as
far back as v1.4.1).

Signed-off-by: Jeff King <peff@peff.net>
---
On Sat, Jul 28, 2007 at 05:26:27PM +0200, Jakub Narebski wrote:

> > How about --name-status?
> 
> Or -r --name-status?

The '-r' now seems to be superfluous. I checked using the following
script:

  mkdir repo && cd repo && git-init &&
    touch root && git-add root && git-commit -m root &&
    mkdir sub && touch sub/file && git-add sub/file &&
    git-diff --cached --name-status

And it correctly reports

  A sub/file

at least since v1.4.1. I didn't look further, but the example is from
the 0.99 era, so I suspect this behavior was changed with the
libification of the revision machinery and the reworking of git-diff.

Or maybe I just totally don't understand what '-r' is supposed to be
doing.

 Documentation/git-diff.txt |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 639b969..b1f5e7f 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -102,17 +102,14 @@ Limiting the diff output::
 +
 ------------
 $ git diff --diff-filter=MRC            <1>
-$ git diff --name-status -r             <2>
+$ git diff --name-status                <2>
 $ git diff arch/i386 include/asm-i386   <3>
 ------------
 +
 <1> show only modification, rename and copy, but not addition
 nor deletion.
 <2> show only names and the nature of change, but not actual
-diff output.  --name-status disables usual patch generation
-which in turn also disables recursive behavior, so without -r
-you would only see the directory name if there is a change in a
-file in a subdirectory.
+diff output.
 <3> limit diff output to named subtrees.
 
 Munging the diff output::
-- 
1.5.3.rc3.845.g88e3-dirty

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Linus Torvalds]

On Sat, 28 Jul 2007, Jeff King wrote:
> 
> The '-r' now seems to be superfluous.

For diffing against (or using) the index, the "-r" is superfluous.

Why? Because the index is always the *full* list of files. It's "flat".

However, when you diff two trees, the -r makes a difference.

So I think you'd find a difference if you actually diffed two commits 
with "git diff tree2..tree2".

		Linus

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Jeff King]

On Sat, Jul 28, 2007 at 07:06:38PM -0700, Linus Torvalds wrote:

> For diffing against (or using) the index, the "-r" is superfluous.
> 
> Why? Because the index is always the *full* list of files. It's "flat".
> 
> However, when you diff two trees, the -r makes a difference.
> 
> So I think you'd find a difference if you actually diffed two commits 
> with "git diff tree2..tree2".

Ah...right you are.

So if I "git diff" two commits with --raw or --name-status, we don't
recurse into recurse into subdirectories (because they are actually
subtrees). If I "git diff" a commit against the index using --raw or
--name-status, I we do recurse (since the index is actually flat). But
if I "git diff" using -p, --stat, or --summary, it _does_ recurse no
matter what I'm diffing.

Does anybody else find this behavior confusing? I can understand why
diff-tree might not recurse by default, but I wonder if porcelain like
git-diff should try to be a little more consistent and always recurse.

Something like:

diff --git a/builtin-diff.c b/builtin-diff.c
index 7f367b6..b48121e 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -233,6 +233,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			die("diff_setup_done failed");
 	}
 	rev.diffopt.allow_external = 1;
+	rev.diffopt.recursive = 1;
 
 	/* Do we have --cached and not have a pending object, then
 	 * default to HEAD by hand.  Eek.

For comparison, whatchanged, show, and format-patch are already always
recursive. log is not.

-Peff

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Linus Torvalds]

On Sun, 29 Jul 2007, Jeff King wrote:
> 
> So if I "git diff" two commits with --raw or --name-status, we don't
> recurse into recurse into subdirectories (because they are actually
> subtrees).

Exactly.

As it pertains to "git diff", none of this actually makes sense, and I 
think it might make sense to make "git diff" automatically set 
"recursive", exactly because "git diff" sometimes operates on trees, and 
sometimes on the index.

> If I "git diff" a commit against the index using --raw or
> --name-status, I we do recurse (since the index is actually flat). But
> if I "git diff" using -p, --stat, or --summary, it _does_ recurse no
> matter what I'm diffing.

When you ask for patches, it has to recurse, because there is no way to 
show a patch for a directory. So yes, anything non-raw will always enable 
recursing.

The fact that "--name-status" is not considered a "patch" is an 
implementation detail, and I would _almost_ suggest that we just make it 
always recurse, and leave thenon-recursing case for _just_ "--raw". But 
that is a separate decision.

> Does anybody else find this behavior confusing? I can understand why
> diff-tree might not recurse by default, but I wonder if porcelain like
> git-diff should try to be a little more consistent and always recurse.

I do agree.

The behaviour is obviously historical, and comes from "git diff" being 
just a shell-script wrapper around the different versions of diffing trees 
and indexes etc.

So it makes sense in that historical setting (and realizing that the 
"HEAD<->index" and "index<->files" cases were really a totally different 
operations), but it makes no sense in the modern world where people don't 
even *know* about "git diff-tree", but just use "git diff" for everything.

So:

> Something like:

Ack. Patch looks fine, makes sense, and is obviously good.

It *is* a change in behaviour, though, so I can understand if Junio 
doesn't think it's appropriate this late in the 1.5.3 series.

> For comparison, whatchanged, show, and format-patch are already always
> recursive. log is not.

Yeah. The other cases already default to patches, so they get the 
recursive from there.

"git log", of course, defaults to no output at all, so the only way to get 
non-recursive behaviour is to ask for "--raw", and then having to specify 
explicitly whether to get recursion or not make sense. Once you want raw 
output, it really _is_ your choice.

Whether we should make "--name-status" default to "-r" is worth 
discussing. I don't have any really strong opinion, although I _suspect_ 
that we should. The non-recursive case is useful, but in a very limited 
sense, and I think we might as well limit it to just the --raw case.

		Linus

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

>> Does anybody else find this behavior confusing? I can understand why
>> diff-tree might not recurse by default, but I wonder if porcelain like
>> git-diff should try to be a little more consistent and always recurse.
>
> I do agree.
>
> The behaviour is obviously historical, and comes from "git diff" being 
> just a shell-script wrapper around the different versions of diffing trees 
> and indexes etc.
>
> So it makes sense in that historical setting (and realizing that the 
> "HEAD<->index" and "index<->files" cases were really a totally different 
> operations), but it makes no sense in the modern world where people don't 
> even *know* about "git diff-tree", but just use "git diff" for everything.
>
> So:
>
>> Something like:
>
> Ack. Patch looks fine, makes sense, and is obviously good.

That makes it two of us.  ... eh, excuse me, there is one issue
I mention at the end.

> It *is* a change in behaviour, though, so I can understand if Junio 
> doesn't think it's appropriate this late in the 1.5.3 series.

One minor objection I do have is that, just as a matter of
principle, in order to avoid setting precedence of making a
fundamental semantics change in late -rc stage in the game, we
should not swallow it.  I do not mind if this were clearly a
good change.

However, I think Jeff's patch always makes it recursive even
when the user asks for --raw, which makes it inappropriate for
inclusion whether before or after 1.5.3.

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Jeff King]

On Sat, Jul 28, 2007 at 09:48:15PM -0700, Junio C Hamano wrote:

> However, I think Jeff's patch always makes it recursive even
> when the user asks for --raw, which makes it inappropriate for
> inclusion whether before or after 1.5.3.

Right, that's the point. git-diff is currently inconsistent (unless you
understand that index comparisons are always recursed, and tree
comparisons need it explicitly -- but part of the point of git-diff is
to abstract those sorts of details), so this attempts to harmonize the
behavior no matter what you're diffing (whether it be --name-status or
--raw).

If you really want not to recurse, then you have to know you are
comparing trees anyway, at which point it makes sense to use the
git-diff-tree plumbing.

-Peff

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> One minor objection I do have is that, just as a matter of
> principle, in order to avoid setting precedence of making a
> fundamental semantics change in late -rc stage in the game, we
> should not swallow it.  I do not mind if this were clearly a
> good change.
>
> However, I think Jeff's patch always makes it recursive even
> when the user asks for --raw, which makes it inappropriate for
> inclusion whether before or after 1.5.3.

Personally, I think -r should be the default while one can use
--directory to switch it off.

Whether --raw should imply --directory might be a different question.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> ...
>> Ack. Patch looks fine, makes sense, and is obviously good.
>
> That makes it two of us.  ... eh, excuse me, there is one issue
> I mention at the end.
>
>> It *is* a change in behaviour, though, so I can understand if Junio 
>> doesn't think it's appropriate this late in the 1.5.3 series.
>
> One minor objection I do have is that, just as a matter of
> principle, in order to avoid setting precedence of making a
> fundamental semantics change in late -rc stage in the game, we
> should not swallow it.  I do not mind if this were clearly a
> good change.
>
> However, I think Jeff's patch always makes it recursive even
> when the user asks for --raw, which makes it inappropriate for
> inclusion whether before or after 1.5.3.

Actually, I would like to take this back.  The highest level UI
"git-diff" is for the end-users, and most of them typically do
not seem to want non recursive "birds-eye" view of changes.

The top-level non-recursive behaviour is not always useful as a
birds-eye view.  Even in Linux kernel context, when you are
interested in one area e.g. i386, the answer the non recursive
behaviour gives us would not show what you typically want to see
(i.e. include/asm-i386 and arch/i386), but it stops at the
"toplevel", i.e. include and arch.

In addition, it is not like our tree diff is too slow and not
recursing into lower level makes it usable.

In short, I think recursive behaviour by default makes sense for
"git diff".

When people want to use lower level git-diff-* family, I think
the traditional behaviour is fine.  We _might_ want to default
the --name-status and --name-only to recursive behaviour as
well, but (1) that is an independent topic, and (2) we should
re-examine what the appropriate recursiveness for the other
options.

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Jeff King]

On Sun, Jul 29, 2007 at 02:36:23AM -0700, Junio C Hamano wrote:

> In short, I think recursive behaviour by default makes sense for
> "git diff".

Your reasoning sounds sane to me.

Here is the patch with a commit message more succint than all of these
emails.

-- >8 --
git-diff: turn on recursion by default

The tree recursion behavior of git-diff may appear
inconsistent to the user because it depends on the format of
the patch as well as whether one is diffing between trees or
against the index.

Since git-diff is a porcelain wrapper for low-level diff
commands, it makes sense for its behavior to be consistent
no matter what is being diffed.  This patch turns on
recursion in all cases.

diff --git a/builtin-diff.c b/builtin-diff.c
index 7f367b6..b48121e 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -233,6 +233,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			die("diff_setup_done failed");
 	}
 	rev.diffopt.allow_external = 1;
+	rev.diffopt.recursive = 1;
 
 	/* Do we have --cached and not have a pending object, then
 	 * default to HEAD by hand.  Eek.

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Johannes Schindelin]

Hi,

On Sun, 29 Jul 2007, Jeff King wrote:

> -- >8 --
> git-diff: turn on recursion by default
> 
> The tree recursion behavior of git-diff may appear
> inconsistent to the user because it depends on the format of
> the patch as well as whether one is diffing between trees or
> against the index.
> 
> Since git-diff is a porcelain wrapper for low-level diff
> commands, it makes sense for its behavior to be consistent
> no matter what is being diffed.  This patch turns on
> recursion in all cases.
> 
> diff --git a/builtin-diff.c b/builtin-diff.c
> index 7f367b6..b48121e 100644
> --- a/builtin-diff.c
> +++ b/builtin-diff.c
> @@ -233,6 +233,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  			die("diff_setup_done failed");
>  	}
>  	rev.diffopt.allow_external = 1;
> +	rev.diffopt.recursive = 1;

How about

	if (!rev.diffopt.quiet)
		rev.diffopt.recursive = 1;

instead?

Ciao,
Dscho

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[David Kastrup]

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


>>  	}
>>  	rev.diffopt.allow_external = 1;
>> +	rev.diffopt.recursive = 1;
>
> How about
>
> 	if (!rev.diffopt.quiet)
> 		rev.diffopt.recursive = 1;
>
> instead?

I think that the optimization "don't descend if we can figure this out
at the top level" should be rather implemented at the program flow
level than in the option processing, and quietness does not actually
play into this: _any_ diff operation can skip trees with identical
top-level SHA1.

So the above optimization should not cause a performance difference
(if it does, this is better fixed elsewhere), and makes it obscure to
guess the "-q" behavior which should be more or less equivalent to
--exit-status >/dev/null

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Jeff King]

On Sun, Jul 29, 2007 at 12:14:49PM +0100, Johannes Schindelin wrote:

> How about
> 
> 	if (!rev.diffopt.quiet)
> 		rev.diffopt.recursive = 1;
> 
> instead?

Can you explain?

My assumption is that you want the diff machinery to exit as quickly as
possible. But the "recursive" option is _already_ turned on for most
output formats in that case, so I had assumed that it was the quiet
option itself which caused the early return, not a failure to set
"recursive".

If you don't want recursive set for quiet runs, I think there is an
error in diff.c, which sets the output format from quiet to NO_FORMAT
only _after_ having set recursive based on the output format.

-Peff

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Johannes Schindelin]

Hi,

On Sun, 29 Jul 2007, Jeff King wrote:

> On Sun, Jul 29, 2007 at 12:14:49PM +0100, Johannes Schindelin wrote:
> 
> > How about
> > 
> > 	if (!rev.diffopt.quiet)
> > 		rev.diffopt.recursive = 1;
> > 
> > instead?
> 
> Can you explain?

The idea is this: when "--quiet" was given, we do not output anything, and 
therefore do not have to recurse into the directories, because we already 
know that there are differences when a _tree_ is different.  I do not 
remember all details of the "--quiet" implementation, but I think that it

- exits early (as you said)

- does not turn on "recursive" to avoid unnecessary work.

Imagine something like this: tree "a" and "b" contain 100,000 elements 
each, which are identical except for the last entry.  "--quiet" does not 
need to check the 99,999 elements before that one, since the tree hashes 
are already different.

Of course, this reasoning breaks down fatally when you specify something 
like "--ignore-whitespace", but those cases should turn on recursive 
explicitely, so that the performance penalty of "recursive = 1" does not 
percolate back to the (much more common) trivial cases.

Ciao,
Dscho

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Jeff King]

On Sun, Jul 29, 2007 at 01:04:13PM +0100, Johannes Schindelin wrote:

> The idea is this: when "--quiet" was given, we do not output anything, and 
> therefore do not have to recurse into the directories, because we already 
> know that there are differences when a _tree_ is different.  I do not 
> remember all details of the "--quiet" implementation, but I think that it
> 
> - exits early (as you said)
> 
> - does not turn on "recursive" to avoid unnecessary work.

OK, looking through the code, this works _sometimes_. If I say "git-diff
--quiet" then it will not recurse. If I say "git-diff -p --quiet" then
it will (even though we never show the -p output).

Since --quiet supersedes all output formats, I think it probably should
just clear the recursive option entirely. In which case rather than
special-casing quiet to avoid recursion in git-diff, we can simply turn
on recursion before parsing options (and it will get turned off
correctly by any diff options that need to do so).

Something like:

diff --git a/builtin-diff.c b/builtin-diff.c
index 7f367b6..e6d10bd 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -229,6 +229,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		argc = setup_revisions(argc, argv, &rev, NULL);
 	if (!rev.diffopt.output_format) {
 		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+		rev.diffopt.recursive = 1;
 		if (diff_setup_done(&rev.diffopt) < 0)
 			die("diff_setup_done failed");
 	}
diff --git a/diff.c b/diff.c
index a5fc56b..aeae1a3 100644
--- a/diff.c
+++ b/diff.c
@@ -2182,6 +2182,7 @@ int diff_setup_done(struct diff_options *options)
 	if (options->quiet) {
 		options->output_format = DIFF_FORMAT_NO_OUTPUT;
 		options->exit_with_status = 1;
+		options->recursive = 0;
 	}
 
 	/*

But maybe that doesn't work because some of the options need recursion
turned on, even if --quiet is specified. I have to admit, there are a
lot of code paths here and I'm not sure how all of them interact with
the recursive option. So perhaps it is better to just special case
options->quiet as you suggested.

-Peff

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Johannes Schindelin]

Hi,

On Sun, 29 Jul 2007, Jeff King wrote:

> On Sun, Jul 29, 2007 at 01:04:13PM +0100, Johannes Schindelin wrote:
> 
> > The idea is this: when "--quiet" was given, we do not output anything, and 
> > therefore do not have to recurse into the directories, because we already 
> > know that there are differences when a _tree_ is different.  I do not 
> > remember all details of the "--quiet" implementation, but I think that it
> > 
> > - exits early (as you said)
> > 
> > - does not turn on "recursive" to avoid unnecessary work.
> 
> OK, looking through the code, this works _sometimes_. If I say "git-diff
> --quiet" then it will not recurse. If I say "git-diff -p --quiet" then
> it will (even though we never show the -p output).

That is expected behaviour.  If you ask for it, it should be done (however 
silly it might be).

> Since --quiet supersedes all output formats, I think it probably should 
> just clear the recursive option entirely. In which case rather than 
> special-casing quiet to avoid recursion in git-diff, we can simply turn 
> on recursion before parsing options (and it will get turned off 
> correctly by any diff options that need to do so).

No.  Just think about "git diff -p -w --quiet".  In some cases it _has_ to 
recurse.

Ciao,
Dscho

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Jeff King]

On Sun, Jul 29, 2007 at 01:24:20PM +0100, Johannes Schindelin wrote:

> No.  Just think about "git diff -p -w --quiet".  In some cases it _has_ to 
> recurse.

OK, I'm convinced. It should be as you originally suggested.

-Peff

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Linus Torvalds]

On Sat, 28 Jul 2007, Junio C Hamano wrote:
> 
> However, I think Jeff's patch always makes it recursive even
> when the user asks for --raw, which makes it inappropriate for
> inclusion whether before or after 1.5.3.

Well, that was kind of the point. Yes, it makrs it recursive even for 
--raw, but the fact is, "git diff" is _always_ recursive (even for --raw) 
for most common usage: anything that involves the index.

So "git diff" is fundamentally different from "git log" and "git show" and 
friends. "git diff" can work on the files and index, which "git log" and 
"git show" never do.

		Linus

Re: [PATCH] Documentation/git-diff: remove -r from --name-status example

[Jeff King]

On Sat, Jul 28, 2007 at 09:27:18PM -0700, Linus Torvalds wrote:

> The fact that "--name-status" is not considered a "patch" is an 
> implementation detail, and I would _almost_ suggest that we just make it 
> always recurse, and leave thenon-recursing case for _just_ "--raw". But 
> that is a separate decision.

I think it makes some sense, but I'm not sure it is worth it for two
reasons:
  1. --raw and --name-status are very linked in the diff code. Turning
     on recursion for --name-status without --raw will be somewhat
     hairy.
  2. If all of the porcelain tools turn on recursion anyway, that
     implementation detail just goes away anyway.

On the other hand, it looks like we just set options->recursive = 1 for
most formats anyway. So doing "git-log --raw" will not recurse, but
"git-log --raw -p" will recurse for _both_ the raw and the log formats.
Which I think is a bit non-intuitive, but it certainly makes the patch
much simpler:

diff --git a/diff.c b/diff.c
index a5fc56b..3137cac 100644
--- a/diff.c
+++ b/diff.c
@@ -2151,6 +2151,7 @@ int diff_setup_done(struct diff_options *options)
 				      DIFF_FORMAT_DIFFSTAT |
 				      DIFF_FORMAT_SHORTSTAT |
 				      DIFF_FORMAT_SUMMARY |
+				      DIFF_FORMAT_NAME_STATUS |
 				      DIFF_FORMAT_CHECKDIFF))
 		options->recursive = 1;
 	/*

> It *is* a change in behaviour, though, so I can understand if Junio 
> doesn't think it's appropriate this late in the 1.5.3 series.

Agreed.

> "git log", of course, defaults to no output at all, so the only way to get 
> non-recursive behaviour is to ask for "--raw", and then having to specify 
> explicitly whether to get recursion or not make sense. Once you want raw 
> output, it really _is_ your choice.

I wonder if git-log should match the behavior of the other commands. I
have often see git-whatchanged explained as the equivalent of "git-log
-p" but that's not exactly true. Similarly, git-show has been explained
as "git-log -p for the first commit." And using "-p" they are basically
the same. But it means I can't just replace all use of "git-whatchanged"
with "git-log" and get the exact same behavior.  I think it makes to
harmonize these little "gotchas".

-Peff