[PATCH] git-diff: Clarify operation when not inside a repository.

[PATCH] git-diff: Clarify operation when not inside a repository.

[Dale R. Worley]

Clarify documentation for git-diff:  State that when not inside a
repository, --no-index is implied (and thus two arguments are
mandatory).

Clarify error message from diff-no-index to inform user that CWD is
not inside a repository and thus two arguments are mandatory.

Signed-off-by: Dale Worley <worley@ariadne.com>
---


This clarification is to avoid a problem I ran into.  I executed 'git
diff' in the remote working tree of a repository, and not in the
repository directory itself.  Because of that, git-diff assumed
git-diff --no-index, and executed diff-no-index.  Since I hadn't
provided paths, diff-no-index produced an error message.
Unfortunately, the error message presupposes that the decision to
execute diff-no-index reflects the user's intention, thus leaving me
confused, as the error message is only:
    usage: git diff [--no-index] <path> <path>
and does not cover the case I intended.  This patch changes the
message to notify the user that he is getting --no-index semantics
because he is outside of a repository:
    Not within a git repository:
    usage: git diff [--no-index] <path> <path>
The additional line is suppressed if the user specified --no-index.

The documentation is expanded to state that execution outside of a
repository forces --no-index behavior.  Previously, the manual implied
this but did not state it, making it easy for the user to overlook
that it's possible to run git-diff outside of a repository.

Dale


 Documentation/git-diff.txt |    3 ++-
 diff-no-index.c            |    6 +++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 78d6d50..9f74989 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -31,7 +31,8 @@ two blob objects, or changes between two files on disk.
 +
 If exactly two paths are given and at least one points outside
 the current repository, 'git diff' will compare the two files /
-directories. This behavior can be forced by --no-index.
+directories. This behavior can be forced by --no-index or by 
+executing 'git diff' outside of a working tree.
 
 'git diff' [--options] --cached [<commit>] [--] [<path>...]::
 
diff --git a/diff-no-index.c b/diff-no-index.c
index e66fdf3..98c5f76 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -215,9 +215,13 @@ void diff_no_index(struct rev_info *revs,
 		     path_inside_repo(prefix, argv[i+1])))
 			return;
 	}
-	if (argc != i + 2)
+	if (argc != i + 2) {
+	        if (!no_index) {
+		        fprintf(stderr, "Not within a git repository:\n");
+		}
 		usagef("git diff %s <path> <path>",
 		       no_index ? "--no-index" : "[--no-index]");
+	}
 
 	diff_setup(&revs->diffopt);
 	for (i = 1; i < argc - 2; ) {
-- 
1.7.7.6

Re: [PATCH] git-diff: Clarify operation when not inside a repository.

[Junio C Hamano]

worley@alum.mit.edu (Dale R. Worley) writes:

> Unfortunately, the error message presupposes that the decision to
> execute diff-no-index reflects the user's intention, thus leaving me
> confused, as the error message is only:
>     usage: git diff [--no-index] <path> <path>
> and does not cover the case I intended.  This patch changes the
> message to notify the user that he is getting --no-index semantics
> because he is outside of a repository:
>     Not within a git repository:
>     usage: git diff [--no-index] <path> <path>
> The additional line is suppressed if the user specified --no-index.

It makes perfect sense for your situation, I think.

Do we say "within" in other error messages for similar situations?
Many commands require you to be in a working tree---the ones marked
as NEED_WORK_TREE in git.c call setup.c::setup_work_tree() to do
this check---and the error message phrases "run in a work tree".  We
would want to use the matching phrasing here, too.

For that matter, as no_index variable knows we didn't get an
explicit "--no-index", we can be even more explicit, e.g.

	fatal: If you want to compare two files outside a working
        tree, use "git diff <fileA> <fileB>".

which hopefully will also clarify the consequence of the command,
i.e. compares two files that are _outside_ a working tree.

I am not sure which one is better, though.  Just a random thought
that came to my mind while reading your error message.

> The documentation is expanded to state that execution outside of a
> repository forces --no-index behavior.  Previously, the manual implied
> this but did not state it, making it easy for the user to overlook
> that it's possible to run git-diff outside of a repository.

I am not sure if "forced" is a good description here.  An explicit
"--no-index" does force the command to ignore the fact that the
command is run inside a working tree and compare two paths without
involving Git at all, but the behaviour you saw was to fall back to
the no-index hack instead of failing (the latter of which is a
logical but unfriendly thing to do, as Git is about data managed by
Git, and running Git command that wants a working tree without
having a working tree).  It feels that it is more like "Also, this
mode is used when the command is run outside a working tree" to me.

>  Documentation/git-diff.txt |    3 ++-
>  diff-no-index.c            |    6 +++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
> index 78d6d50..9f74989 100644
> --- a/Documentation/git-diff.txt
> +++ b/Documentation/git-diff.txt
> @@ -31,7 +31,8 @@ two blob objects, or changes between two files on disk.
>  +
>  If exactly two paths are given and at least one points outside
>  the current repository, 'git diff' will compare the two files /
> -directories. This behavior can be forced by --no-index.
> +directories. This behavior can be forced by --no-index or by 
> +executing 'git diff' outside of a working tree.
>  
>  'git diff' [--options] --cached [<commit>] [--] [<path>...]::
>  
> diff --git a/diff-no-index.c b/diff-no-index.c
> index e66fdf3..98c5f76 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -215,9 +215,13 @@ void diff_no_index(struct rev_info *revs,
>  		     path_inside_repo(prefix, argv[i+1])))
>  			return;
>  	}
> -	if (argc != i + 2)
> +	if (argc != i + 2) {
> +	        if (!no_index) {
> +		        fprintf(stderr, "Not within a git repository:\n");
> +		}
>  		usagef("git diff %s <path> <path>",
>  		       no_index ? "--no-index" : "[--no-index]");
> +	}
>  
>  	diff_setup(&revs->diffopt);
>  	for (i = 1; i < argc - 2; ) {

[PATCHv2] git-diff: Clarify operation when not inside a repository.

[Dale R. Worley]

Clarify documentation for git-diff:  State that when not inside a
repository, --no-index is implied (and thus two arguments are
mandatory).

Clarify error message from diff-no-index to inform user that CWD is
not inside a repository and thus two arguments are mandatory.

Signed-off-by: Dale Worley <worley@ariadne.com>
---


The error message has been updated from [PATCH].  "git diff" outside a
repository now produces:

    Not a git repository
    To compare two paths outside a working tree:
    usage: git diff [--no-index] <path> <path>

This should inform the user of his error regardless of whether he
intended to perform a within-repository "git diff" or an
out-of-repository "git diff".

This message is closer to the message that other Git commands produce:

    fatal: Not a git repository (or any parent up to mount parent )
    Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).

"git diff --no-index" produces the same message as before (since the
user is clearly invoking the non-repository behavior):

    usage: git diff --no-index <path> <path>

Regarding the change to git-diff.txt, perhaps "forced ... by executing
'git diff' outside of a working tree" is not the best wording, but it
should be clear to the reader that (1) it is possible to execute 'git
diff' outside of a working tree, and (2) when doing so, the behavior
will be as if '--no-index' was specified.

I've also added some comments for the new code.


 Documentation/git-diff.txt |    3 ++-
 diff-no-index.c            |   12 +++++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 78d6d50..9f74989 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -31,7 +31,8 @@ two blob objects, or changes between two files on disk.
 +
 If exactly two paths are given and at least one points outside
 the current repository, 'git diff' will compare the two files /
-directories. This behavior can be forced by --no-index.
+directories. This behavior can be forced by --no-index or by 
+executing 'git diff' outside of a working tree.
 
 'git diff' [--options] --cached [<commit>] [--] [<path>...]::
 
diff --git a/diff-no-index.c b/diff-no-index.c
index e66fdf3..9734ec3 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -215,9 +215,19 @@ void diff_no_index(struct rev_info *revs,
 		     path_inside_repo(prefix, argv[i+1])))
 			return;
 	}
-	if (argc != i + 2)
+	if (argc != i + 2) {
+	        if (!no_index) {
+		        /* There was no --no-index and there were not two
+			 * paths.  It is possible that the user intended
+			 * to do an inside-repository operation. */
+		        fprintf(stderr, "Not a git repository\n");
+		        fprintf(stderr,
+				"To compare two paths outside a working tree:\n");
+		}
+		/* Give the usage message for non-repository usage and exit. */
 		usagef("git diff %s <path> <path>",
 		       no_index ? "--no-index" : "[--no-index]");
+	}
 
 	diff_setup(&revs->diffopt);
 	for (i = 1; i < argc - 2; ) {
-- 
1.7.7.6

Re: [PATCHv2] git-diff: Clarify operation when not inside a repository.

[Junio C Hamano]

worley@alum.mit.edu (Dale R. Worley) writes:

> The error message has been updated from [PATCH].  "git diff" outside a
> repository now produces:
>
>     Not a git repository
>     To compare two paths outside a working tree:
>     usage: git diff [--no-index] <path> <path>
>
> This should inform the user of his error regardless of whether he
> intended to perform a within-repository "git diff" or an
> out-of-repository "git diff".
>
> This message is closer to the message that other Git commands produce:
>
>     fatal: Not a git repository (or any parent up to mount parent )
>     Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
>
> "git diff --no-index" produces the same message as before (since the
> user is clearly invoking the non-repository behavior):
>
>     usage: git diff --no-index <path> <path>

The above result looks good and I find the reasoning stated here
very sound.

> Regarding the change to git-diff.txt, perhaps "forced ... by executing
> 'git diff' outside of a working tree" is not the best wording, but it
> should be clear to the reader that (1) it is possible to execute 'git
> diff' outside of a working tree, and (2) when doing so, the behavior
> will be as if '--no-index' was specified.

Then perhaps we can avoid the confusing "forced" by phrasing it like
so?

    This behaviour can be forced by --no-index.  Also 'git diff
    <path> <path>' outside of a working tree can be used to compare
    two named paths.

Let's step back a bit, though.  The original text is:

    'git diff' [--options] [--] [<path>...]::

            This form is to view the changes you made relative to
            the index (staging area for the next commit).  In other
            words, the differences are what you _could_ tell Git to
            further add to the index but you still haven't.  You can
            stage these changes by using linkgit:git-add[1].
    +
    If exactly two paths are given and at least one points outside
    the current repository, 'git diff' will compare the two files /
    directories. This behavior can be forced by --no-index.

which _primarily_ explains how the index and the working tree
contents are compared, but also mixes the description of the
"--no-index" hack, which is quite different.  As its name suggests,
it is not about comparing with the index---in fact, it is not even
about Git at all.  Just a pair of random paths that do not have
anything to do with Git are compared.

I suspect that it may be a good idea to split the section altogether
to reduce confusion like what triggered this thread, e.g.

    'git diff' [--options] [--] [<path>...]::

            This form is to view the changes you made relative to
            the index (staging area for the next commit).  In other
            words, the differences are what you _could_ tell Git to
            further add to the index but you still haven't.  You can
            stage these changes by using linkgit:git-add[1].

    'git diff' --no-index [--options] [--] <path> <path>::

	    This form is to compare the given two paths on the
	    filesystem.  When run in a working tree controlled by
	    Git, if at least one of the paths points outside the
	    working tree, or when run outside a working tree
	    controlled by Git, you can omit the `--no-index` option.

For now, I'll queue your version as-is modulo style fixes, while
waiting for others to help polishing the documentation better.

> I've also added some comments for the new code.

Thanks.

>  Documentation/git-diff.txt |    3 ++-
>  diff-no-index.c            |   12 +++++++++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
> index 78d6d50..9f74989 100644
> --- a/Documentation/git-diff.txt
> +++ b/Documentation/git-diff.txt
> @@ -31,7 +31,8 @@ two blob objects, or changes between two files on disk.
>  +
>  If exactly two paths are given and at least one points outside
>  the current repository, 'git diff' will compare the two files /
> -directories. This behavior can be forced by --no-index.
> +directories. This behavior can be forced by --no-index or by 
> +executing 'git diff' outside of a working tree.
>  
>  'git diff' [--options] --cached [<commit>] [--] [<path>...]::
>  
> diff --git a/diff-no-index.c b/diff-no-index.c
> index e66fdf3..9734ec3 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -215,9 +215,19 @@ void diff_no_index(struct rev_info *revs,
>  		     path_inside_repo(prefix, argv[i+1])))
>  			return;
>  	}
> -	if (argc != i + 2)
> +	if (argc != i + 2) {
> +	        if (!no_index) {
> +		        /* There was no --no-index and there were not two
> +			 * paths.  It is possible that the user intended
> +			 * to do an inside-repository operation. */
> +		        fprintf(stderr, "Not a git repository\n");
> +		        fprintf(stderr,
> +				"To compare two paths outside a working tree:\n");
> +		}
> +		/* Give the usage message for non-repository usage and exit. */
>  		usagef("git diff %s <path> <path>",
>  		       no_index ? "--no-index" : "[--no-index]");
> +	}
>  
>  	diff_setup(&revs->diffopt);
>  	for (i = 1; i < argc - 2; ) {

Re: [PATCHv2] git-diff: Clarify operation when not inside a repository.

[Dale R. Worley]

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

> I suspect that it may be a good idea to split the section altogether
> to reduce confusion like what triggered this thread, e.g.
> 
>     'git diff' [--options] [--] [<path>...]::
> 
>             This form is to view the changes you made relative to
>             the index (staging area for the next commit).  In other
>             words, the differences are what you _could_ tell Git to
>             further add to the index but you still haven't.  You can
>             stage these changes by using linkgit:git-add[1].
> 
>     'git diff' --no-index [--options] [--] <path> <path>::
> 
> 	    This form is to compare the given two paths on the
> 	    filesystem.  When run in a working tree controlled by
> 	    Git, if at least one of the paths points outside the
> 	    working tree, or when run outside a working tree
> 	    controlled by Git, you can omit the `--no-index` option.
> 
> For now, I'll queue your version as-is modulo style fixes, while
> waiting for others to help polishing the documentation better.

It'd difficult to figure out how to describe it well.  In my opinion,
the problem here is the DWIM nature of the command, which means that
there is a lot of interaction between the options that are specified,
the number of path arguments, and the circumstances.  My preference is
for "do what I say", that the options restrict the command to operate
in exactly one way, which determines the way the paths are used (and
thus their number) and the context in which it can be used.  But
that's not how git-diff works.

Dale

Re: [PATCHv2] git-diff: Clarify operation when not inside a repository.

[Junio C Hamano]

worley@alum.mit.edu (Dale R. Worley) writes:

>> For now, I'll queue your version as-is modulo style fixes, while
>> waiting for others to help polishing the documentation better.
>
> It'd difficult to figure out how to describe it well.  In my
> opinion, the problem here is the DWIM nature of the command,
> ... My preference is ... But that's not how git-diff works.

So given the constraints, I think this is the best we can do.  As nobody
seems to be helping to polish the text, here is my attempt, on top
of your patch.

 Documentation/git-diff.txt | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index b1630ba..33fbd8c 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -28,11 +28,15 @@ two blob objects, or changes between two files on disk.
 	words, the differences are what you _could_ tell Git to
 	further add to the index but you still haven't.  You can
 	stage these changes by using linkgit:git-add[1].
-+
-If exactly two paths are given and at least one points outside
-the current repository, 'git diff' will compare the two files /
-directories. This behavior can be forced by --no-index or by
-executing 'git diff' outside of a working tree.
+
+'git diff' --no-index [--options] [--] [<path>...]::
+
+	This form is to compare the given two paths on the
+	filesystem.  You can omit the `--no-index` option when
+	running the command in a working tree controlled by Git and
+	at least one of the paths points outside the working tree,
+	or when running the command outside a working tree
+	controlled by Git.
 
 'git diff' [--options] --cached [<commit>] [--] [<path>...]::
 

Re: [PATCHv2] git-diff: Clarify operation when not inside a repository.

[Dale R. Worley]

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

> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
> index b1630ba..33fbd8c 100644
> --- a/Documentation/git-diff.txt
> +++ b/Documentation/git-diff.txt
> @@ -28,11 +28,15 @@ two blob objects, or changes between two files on disk.
>  	words, the differences are what you _could_ tell Git to
>  	further add to the index but you still haven't.  You can
>  	stage these changes by using linkgit:git-add[1].
> -+
> -If exactly two paths are given and at least one points outside
> -the current repository, 'git diff' will compare the two files /
> -directories. This behavior can be forced by --no-index or by
> -executing 'git diff' outside of a working tree.
> +
> +'git diff' --no-index [--options] [--] [<path>...]::
> +
> +	This form is to compare the given two paths on the
> +	filesystem.  You can omit the `--no-index` option when
> +	running the command in a working tree controlled by Git and
> +	at least one of the paths points outside the working tree,
> +	or when running the command outside a working tree
> +	controlled by Git.

That does break out the --no-index case in a clearer way.

Dale