From: Junio C Hamano <gitster@pobox•com>
To: worley@alum•mit.edu (Dale R. Worley)
Cc: git@vger•kernel.org
Subject: Re: [PATCH] git-diff: Clarify operation when not inside a repository.
Date: Wed, 21 Aug 2013 11:33:02 -0700 [thread overview]
Message-ID: <xmqqwqneuc69.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <201308211734.r7LHYwNh008859@hobgoblin.ariadne.com> (Dale R. Worley's message of "Wed, 21 Aug 2013 13:34:58 -0400")
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; ) {
next prev parent reply other threads:[~2013-08-21 18:33 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-21 17:34 [PATCH] git-diff: Clarify operation when not inside a repository Dale R. Worley
2013-08-21 18:33 ` Junio C Hamano [this message]
2013-08-22 20:31 ` [PATCHv2] " Dale R. Worley
2013-08-22 20:54 ` Junio C Hamano
2013-08-23 18:11 ` Dale R. Worley
2013-08-28 22:16 ` Junio C Hamano
2013-08-29 15:44 ` Dale R. Worley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqwqneuc69.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=worley@alum$(echo .)mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox