threads / patch / 9282

patchDocumentation/git-diff: remove -r from --name-status example

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

## tl;dr

18 messages between Jul 29, 2007 and Jul 29, 2007. Diffs are folded; open one to read it.

replies: 17people: 5as markdown or json

Jeff King· Jul 29, 2007, 00:24 UTC · lore

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

Show changes to Documentation/git-diff.txt +2 −5

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

Linus Torvalds· Jul 29, 2007, 02:06 UTC · re: Jeff King · lore

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

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

Jeff King· Jul 29, 2007, 04:11 UTC · re: Linus Torvalds · lore

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

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

Show 8 quoted lines

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

Show changes to builtin-diff.c +1 −1

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

Linus Torvalds· Jul 29, 2007, 04:27 UTC · re: Jeff King · lore

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

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

Junio C Hamano· Jul 29, 2007, 04:48 UTC · re: Linus Torvalds · lore

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

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

Show 20 quoted lines

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

Jeff King· Jul 29, 2007, 04:56 UTC · re: Junio C Hamano · lore

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

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

David Kastrup· Jul 29, 2007, 08:23 UTC · re: Junio C Hamano · lore

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

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

Show 11 quoted lines

> 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

Junio C Hamano· Jul 29, 2007, 09:36 UTC · re: Junio C Hamano · lore

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

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

Show 19 quoted lines

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

Jeff King· Jul 29, 2007, 09:49 UTC · re: Junio C Hamano · lore

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

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.

Show changes to builtin-diff.c +1 −0

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.

Johannes Schindelin· Jul 29, 2007, 11:14 UTC · re: Jeff King · lore

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

Hi,

On Sun, 29 Jul 2007, Jeff King wrote:

Show 22 quoted lines

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

David Kastrup· Jul 29, 2007, 11:33 UTC · re: Johannes Schindelin · lore

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

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

Show 10 quoted lines

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

Jeff King· Jul 29, 2007, 11:38 UTC · re: Johannes Schindelin · lore

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

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

Show 6 quoted lines

> 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

Johannes Schindelin· Jul 29, 2007, 12:04 UTC · re: Jeff King · lore

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

Hi,

On Sun, 29 Jul 2007, Jeff King wrote:

Show 10 quoted lines

> 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

Jeff King· Jul 29, 2007, 12:19 UTC · re: Johannes Schindelin · lore

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

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

Show 8 quoted lines

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

Show changes to 2 files +2 −1

builtin-diff.c, diff.c

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

Johannes Schindelin· Jul 29, 2007, 12:24 UTC · re: Jeff King · lore

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

Hi,

On Sun, 29 Jul 2007, Jeff King wrote:

Show 14 quoted lines

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

Show 5 quoted lines

> 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

Jeff King· Jul 29, 2007, 12:26 UTC · re: Johannes Schindelin · lore

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

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

Linus Torvalds· Jul 29, 2007, 16:51 UTC · re: Junio C Hamano · lore

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

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

Jeff King· Jul 29, 2007, 04:52 UTC · re: Linus Torvalds · lore

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

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:

Show changes to diff.c +1 −2

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