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

[Junio C Hamano <gitster@pobox.com>]

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; ) {