Re: [PATCH] git-diff: Clarify operation when not inside a repository.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 21, 2013, 18:33 UTC
- Message-ID
- <xmqqwqneuc69.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <201308211734.r7LHYwNh008859@hobgoblin.ariadne.com>
worley@alum.mit.edu (Dale R. Worley) writes:
Show 10 quoted lines
> 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.
Show 37 quoted lines
> 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; ) {