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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 22, 2013, 20:54 UTC
Message-ID: <xmqqioyxqwdr.fsf@gitster.dls.corp.google.com>
In-Reply-To: <201308222031.r7MKVL6O028293@freeze.ariadne.com>

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

Show 20 quoted lines

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

Show 5 quoted lines

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

Show 43 quoted lines

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