{"thread":{"id":"34740","subject":"[PATCH] git-diff: Clarify operation when not inside a repository.","startedAt":"2013-08-21T17:34:58Z","lastAt":"2013-08-29T15:44:16Z","messageCount":7,"participants":["Dale R. Worley","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"225621","messageId":"201308211734.r7LHYwNh008859@hobgoblin.ariadne.com","threadId":"34740","inReplyTo":null,"subject":"[PATCH] git-diff: Clarify operation when not inside a repository.","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-08-21T17:34:58Z","receivedAt":"2013-08-21T17:34:58Z","isPatch":true,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"Clarify documentation for git-diff: State that when not inside a\nrepository, --no-index is implied (and thus two arguments are\nmandatory).\n\nClarify error message from diff-no-index to inform user that CWD is\nnot inside a repository and thus two arguments are mandatory.\n\nSigned-off-by: Dale Worley \n---\n\n\nThis clarification is to avoid a problem I ran into. I executed 'git\ndiff' in the remote working tree of a repository, and not in the\nrepository directory itself. Because of that, git-diff assumed\ngit-diff --no-index, and executed diff-no-index. Since I hadn't\nprovided paths, diff-no-index produced an error message.\nUnfortunately, the error message presupposes that the decision to\nexecute diff-no-index reflects the user's intention, thus leaving me\nconfused, as the error message is only:\n usage: git diff [--no-index] \nand does not cover the case I intended. This patch changes the\nmessage to notify the user that he is getting --no-index semantics\nbecause he is outside of a repository:\n Not within a git repository:\n usage: git diff [--no-index] \nThe additional line is suppressed if the user specified --no-index.\n\nThe documentation is expanded to state that execution outside of a\nrepository forces --no-index behavior. Previously, the manual implied\nthis but did not state it, making it easy for the user to overlook\nthat it's possible to run git-diff outside of a repository.\n\nDale\n\n\n Documentation/git-diff.txt | 3 ++-\n diff-no-index.c | 6 +++++-\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 78d6d50..9f74989 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -31,7 +31,8 @@ two blob objects, or changes between two files on disk.\n +\n If exactly two paths are given and at least one points outside\n the current repository, 'git diff' will compare the two files /\n-directories. This behavior can be forced by --no-index.\n+directories. This behavior can be forced by --no-index or by \n+executing 'git diff' outside of a working tree.\n \n 'git diff' [--options] --cached [] [--] [...]::\n \ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex e66fdf3..98c5f76 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -215,9 +215,13 @@ void diff_no_index(struct rev_info *revs,\n \t\t path_inside_repo(prefix, argv[i+1])))\n \t\t\treturn;\n \t}\n-\tif (argc != i + 2)\n+\tif (argc != i + 2) {\n+\t if (!no_index) {\n+\t\t fprintf(stderr, \"Not within a git repository:\\n\");\n+\t\t}\n \t\tusagef(\"git diff %s \",\n \t\t no_index ? \"--no-index\" : \"[--no-index]\");\n+\t}\n \n \tdiff_setup(&revs->diffopt);\n \tfor (i = 1; i < argc - 2; ) {\n-- \n1.7.7.6\n"},{"id":"225623","messageId":"xmqqwqneuc69.fsf@gitster.dls.corp.google.com","threadId":"34740","inReplyTo":"201308211734.r7LHYwNh008859@hobgoblin.ariadne.com","subject":"Re: [PATCH] git-diff: Clarify operation when not inside a repository.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-21T18:33:02Z","receivedAt":"2013-08-21T18:33:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"worley@alum.mit.edu (Dale R. Worley) writes:\n\n> Unfortunately, the error message presupposes that the decision to\n> execute diff-no-index reflects the user's intention, thus leaving me\n> confused, as the error message is only:\n> usage: git diff [--no-index] \n> and does not cover the case I intended. This patch changes the\n> message to notify the user that he is getting --no-index semantics\n> because he is outside of a repository:\n> Not within a git repository:\n> usage: git diff [--no-index] \n> The additional line is suppressed if the user specified --no-index.\n\nIt makes perfect sense for your situation, I think.\n\nDo we say \"within\" in other error messages for similar situations?\nMany commands require you to be in a working tree---the ones marked\nas NEED_WORK_TREE in git.c call setup.c::setup_work_tree() to do\nthis check---and the error message phrases \"run in a work tree\". We\nwould want to use the matching phrasing here, too.\n\nFor that matter, as no_index variable knows we didn't get an\nexplicit \"--no-index\", we can be even more explicit, e.g.\n\n\tfatal: If you want to compare two files outside a working\n tree, use \"git diff \".\n\nwhich hopefully will also clarify the consequence of the command,\ni.e. compares two files that are _outside_ a working tree.\n\nI am not sure which one is better, though. Just a random thought\nthat came to my mind while reading your error message.\n\n> The documentation is expanded to state that execution outside of a\n> repository forces --no-index behavior. Previously, the manual implied\n> this but did not state it, making it easy for the user to overlook\n> that it's possible to run git-diff outside of a repository.\n\nI am not sure if \"forced\" is a good description here. An explicit\n\"--no-index\" does force the command to ignore the fact that the\ncommand is run inside a working tree and compare two paths without\ninvolving Git at all, but the behaviour you saw was to fall back to\nthe no-index hack instead of failing (the latter of which is a\nlogical but unfriendly thing to do, as Git is about data managed by\nGit, and running Git command that wants a working tree without\nhaving a working tree). It feels that it is more like \"Also, this\nmode is used when the command is run outside a working tree\" to me.\n\n> Documentation/git-diff.txt | 3 ++-\n> diff-no-index.c | 6 +++++-\n> 2 files changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\n> index 78d6d50..9f74989 100644\n> --- a/Documentation/git-diff.txt\n> +++ b/Documentation/git-diff.txt\n> @@ -31,7 +31,8 @@ two blob objects, or changes between two files on disk.\n> +\n> If exactly two paths are given and at least one points outside\n> the current repository, 'git diff' will compare the two files /\n> -directories. This behavior can be forced by --no-index.\n> +directories. This behavior can be forced by --no-index or by \n> +executing 'git diff' outside of a working tree.\n> \n> 'git diff' [--options] --cached [] [--] [...]::\n> \n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index e66fdf3..98c5f76 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -215,9 +215,13 @@ void diff_no_index(struct rev_info *revs,\n> \t\t path_inside_repo(prefix, argv[i+1])))\n> \t\t\treturn;\n> \t}\n> -\tif (argc != i + 2)\n> +\tif (argc != i + 2) {\n> +\t if (!no_index) {\n> +\t\t fprintf(stderr, \"Not within a git repository:\\n\");\n> +\t\t}\n> \t\tusagef(\"git diff %s \",\n> \t\t no_index ? \"--no-index\" : \"[--no-index]\");\n> +\t}\n> \n> \tdiff_setup(&revs->diffopt);\n> \tfor (i = 1; i < argc - 2; ) {\n"},{"id":"225684","messageId":"201308222031.r7MKVL6O028293@freeze.ariadne.com","threadId":"34740","inReplyTo":"xmqqwqneuc69.fsf@gitster.dls.corp.google.com","subject":"[PATCHv2] git-diff: Clarify operation when not inside a repository.","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-08-22T20:31:21Z","receivedAt":"2013-08-22T20:31:21Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"Clarify documentation for git-diff: State that when not inside a\nrepository, --no-index is implied (and thus two arguments are\nmandatory).\n\nClarify error message from diff-no-index to inform user that CWD is\nnot inside a repository and thus two arguments are mandatory.\n\nSigned-off-by: Dale Worley \n---\n\n\nThe error message has been updated from [PATCH]. \"git diff\" outside a\nrepository now produces:\n\n Not a git repository\n To compare two paths outside a working tree:\n usage: git diff [--no-index] \n\nThis should inform the user of his error regardless of whether he\nintended to perform a within-repository \"git diff\" or an\nout-of-repository \"git diff\".\n\nThis message is closer to the message that other Git commands produce:\n\n fatal: Not a git repository (or any parent up to mount parent )\n Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\n\n\"git diff --no-index\" produces the same message as before (since the\nuser is clearly invoking the non-repository behavior):\n\n usage: git diff --no-index \n\nRegarding the change to git-diff.txt, perhaps \"forced ... by executing\n'git diff' outside of a working tree\" is not the best wording, but it\nshould be clear to the reader that (1) it is possible to execute 'git\ndiff' outside of a working tree, and (2) when doing so, the behavior\nwill be as if '--no-index' was specified.\n\nI've also added some comments for the new code.\n\n\n Documentation/git-diff.txt | 3 ++-\n diff-no-index.c | 12 +++++++++++-\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 78d6d50..9f74989 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -31,7 +31,8 @@ two blob objects, or changes between two files on disk.\n +\n If exactly two paths are given and at least one points outside\n the current repository, 'git diff' will compare the two files /\n-directories. This behavior can be forced by --no-index.\n+directories. This behavior can be forced by --no-index or by \n+executing 'git diff' outside of a working tree.\n \n 'git diff' [--options] --cached [] [--] [...]::\n \ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex e66fdf3..9734ec3 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -215,9 +215,19 @@ void diff_no_index(struct rev_info *revs,\n \t\t path_inside_repo(prefix, argv[i+1])))\n \t\t\treturn;\n \t}\n-\tif (argc != i + 2)\n+\tif (argc != i + 2) {\n+\t if (!no_index) {\n+\t\t /* There was no --no-index and there were not two\n+\t\t\t * paths. It is possible that the user intended\n+\t\t\t * to do an inside-repository operation. */\n+\t\t fprintf(stderr, \"Not a git repository\\n\");\n+\t\t fprintf(stderr,\n+\t\t\t\t\"To compare two paths outside a working tree:\\n\");\n+\t\t}\n+\t\t/* Give the usage message for non-repository usage and exit. */\n \t\tusagef(\"git diff %s \",\n \t\t no_index ? \"--no-index\" : \"[--no-index]\");\n+\t}\n \n \tdiff_setup(&revs->diffopt);\n \tfor (i = 1; i < argc - 2; ) {\n-- \n1.7.7.6\n"},{"id":"225687","messageId":"xmqqioyxqwdr.fsf@gitster.dls.corp.google.com","threadId":"34740","inReplyTo":"201308222031.r7MKVL6O028293@freeze.ariadne.com","subject":"Re: [PATCHv2] git-diff: Clarify operation when not inside a repository.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-22T20:54:40Z","receivedAt":"2013-08-22T20:54:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"worley@alum.mit.edu (Dale R. Worley) writes:\n\n> The error message has been updated from [PATCH]. \"git diff\" outside a\n> repository now produces:\n>\n> Not a git repository\n> To compare two paths outside a working tree:\n> usage: git diff [--no-index] \n>\n> This should inform the user of his error regardless of whether he\n> intended to perform a within-repository \"git diff\" or an\n> out-of-repository \"git diff\".\n>\n> This message is closer to the message that other Git commands produce:\n>\n> fatal: Not a git repository (or any parent up to mount parent )\n> Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\n>\n> \"git diff --no-index\" produces the same message as before (since the\n> user is clearly invoking the non-repository behavior):\n>\n> usage: git diff --no-index \n\nThe above result looks good and I find the reasoning stated here\nvery sound.\n\n> Regarding the change to git-diff.txt, perhaps \"forced ... by executing\n> 'git diff' outside of a working tree\" is not the best wording, but it\n> should be clear to the reader that (1) it is possible to execute 'git\n> diff' outside of a working tree, and (2) when doing so, the behavior\n> will be as if '--no-index' was specified.\n\nThen perhaps we can avoid the confusing \"forced\" by phrasing it like\nso?\n\n This behaviour can be forced by --no-index. Also 'git diff\n ' outside of a working tree can be used to compare\n two named paths.\n\nLet's step back a bit, though. The original text is:\n\n 'git diff' [--options] [--] [...]::\n\n This form is to view the changes you made relative to\n the index (staging area for the next commit). In other\n words, the differences are what you _could_ tell Git to\n further add to the index but you still haven't. You can\n stage these changes by using linkgit:git-add[1].\n +\n If exactly two paths are given and at least one points outside\n the current repository, 'git diff' will compare the two files /\n directories. This behavior can be forced by --no-index.\n\nwhich _primarily_ explains how the index and the working tree\ncontents are compared, but also mixes the description of the\n\"--no-index\" hack, which is quite different. As its name suggests,\nit is not about comparing with the index---in fact, it is not even\nabout Git at all. Just a pair of random paths that do not have\nanything to do with Git are compared.\n\nI suspect that it may be a good idea to split the section altogether\nto reduce confusion like what triggered this thread, e.g.\n\n 'git diff' [--options] [--] [...]::\n\n This form is to view the changes you made relative to\n the index (staging area for the next commit). In other\n words, the differences are what you _could_ tell Git to\n further add to the index but you still haven't. You can\n stage these changes by using linkgit:git-add[1].\n\n 'git diff' --no-index [--options] [--] ::\n\n\t This form is to compare the given two paths on the\n\t filesystem. When run in a working tree controlled by\n\t Git, if at least one of the paths points outside the\n\t working tree, or when run outside a working tree\n\t controlled by Git, you can omit the `--no-index` option.\n\nFor now, I'll queue your version as-is modulo style fixes, while\nwaiting for others to help polishing the documentation better.\n\n> I've also added some comments for the new code.\n\nThanks.\n\n> Documentation/git-diff.txt | 3 ++-\n> diff-no-index.c | 12 +++++++++++-\n> 2 files changed, 13 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\n> index 78d6d50..9f74989 100644\n> --- a/Documentation/git-diff.txt\n> +++ b/Documentation/git-diff.txt\n> @@ -31,7 +31,8 @@ two blob objects, or changes between two files on disk.\n> +\n> If exactly two paths are given and at least one points outside\n> the current repository, 'git diff' will compare the two files /\n> -directories. This behavior can be forced by --no-index.\n> +directories. This behavior can be forced by --no-index or by \n> +executing 'git diff' outside of a working tree.\n> \n> 'git diff' [--options] --cached [] [--] [...]::\n> \n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index e66fdf3..9734ec3 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -215,9 +215,19 @@ void diff_no_index(struct rev_info *revs,\n> \t\t path_inside_repo(prefix, argv[i+1])))\n> \t\t\treturn;\n> \t}\n> -\tif (argc != i + 2)\n> +\tif (argc != i + 2) {\n> +\t if (!no_index) {\n> +\t\t /* There was no --no-index and there were not two\n> +\t\t\t * paths. It is possible that the user intended\n> +\t\t\t * to do an inside-repository operation. */\n> +\t\t fprintf(stderr, \"Not a git repository\\n\");\n> +\t\t fprintf(stderr,\n> +\t\t\t\t\"To compare two paths outside a working tree:\\n\");\n> +\t\t}\n> +\t\t/* Give the usage message for non-repository usage and exit. */\n> \t\tusagef(\"git diff %s \",\n> \t\t no_index ? \"--no-index\" : \"[--no-index]\");\n> +\t}\n> \n> \tdiff_setup(&revs->diffopt);\n> \tfor (i = 1; i < argc - 2; ) {\n"},{"id":"225728","messageId":"201308231811.r7NIBeH9027848@freeze.ariadne.com","threadId":"34740","inReplyTo":"xmqqioyxqwdr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv2] git-diff: Clarify operation when not inside a repository.","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-08-23T18:11:40Z","receivedAt":"2013-08-23T18:11:40Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"> From: Junio C Hamano \n\n> I suspect that it may be a good idea to split the section altogether\n> to reduce confusion like what triggered this thread, e.g.\n> \n> 'git diff' [--options] [--] [...]::\n> \n> This form is to view the changes you made relative to\n> the index (staging area for the next commit). In other\n> words, the differences are what you _could_ tell Git to\n> further add to the index but you still haven't. You can\n> stage these changes by using linkgit:git-add[1].\n> \n> 'git diff' --no-index [--options] [--] ::\n> \n> \t This form is to compare the given two paths on the\n> \t filesystem. When run in a working tree controlled by\n> \t Git, if at least one of the paths points outside the\n> \t working tree, or when run outside a working tree\n> \t controlled by Git, you can omit the `--no-index` option.\n> \n> For now, I'll queue your version as-is modulo style fixes, while\n> waiting for others to help polishing the documentation better.\n\nIt'd difficult to figure out how to describe it well. In my opinion,\nthe problem here is the DWIM nature of the command, which means that\nthere is a lot of interaction between the options that are specified,\nthe number of path arguments, and the circumstances. My preference is\nfor \"do what I say\", that the options restrict the command to operate\nin exactly one way, which determines the way the paths are used (and\nthus their number) and the context in which it can be used. But\nthat's not how git-diff works.\n\nDale\n"},{"id":"226126","messageId":"xmqq38pt8nqm.fsf@gitster.dls.corp.google.com","threadId":"34740","inReplyTo":"201308231811.r7NIBeH9027848@freeze.ariadne.com","subject":"Re: [PATCHv2] git-diff: Clarify operation when not inside a repository.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-28T22:16:49Z","receivedAt":"2013-08-28T22:16:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"worley@alum.mit.edu (Dale R. Worley) writes:\n\n>> For now, I'll queue your version as-is modulo style fixes, while\n>> waiting for others to help polishing the documentation better.\n>\n> It'd difficult to figure out how to describe it well. In my\n> opinion, the problem here is the DWIM nature of the command,\n> ... My preference is ... But that's not how git-diff works.\n\nSo given the constraints, I think this is the best we can do. As nobody\nseems to be helping to polish the text, here is my attempt, on top\nof your patch.\n\n Documentation/git-diff.txt | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex b1630ba..33fbd8c 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -28,11 +28,15 @@ two blob objects, or changes between two files on disk.\n \twords, the differences are what you _could_ tell Git to\n \tfurther add to the index but you still haven't. You can\n \tstage these changes by using linkgit:git-add[1].\n-+\n-If exactly two paths are given and at least one points outside\n-the current repository, 'git diff' will compare the two files /\n-directories. This behavior can be forced by --no-index or by\n-executing 'git diff' outside of a working tree.\n+\n+'git diff' --no-index [--options] [--] [...]::\n+\n+\tThis form is to compare the given two paths on the\n+\tfilesystem. You can omit the `--no-index` option when\n+\trunning the command in a working tree controlled by Git and\n+\tat least one of the paths points outside the working tree,\n+\tor when running the command outside a working tree\n+\tcontrolled by Git.\n \n 'git diff' [--options] --cached [] [--] [...]::\n \n"},{"id":"226180","messageId":"201308291544.r7TFiGjm023154@freeze.ariadne.com","threadId":"34740","inReplyTo":"xmqq38pt8nqm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv2] git-diff: Clarify operation when not inside a repository.","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-08-29T15:44:16Z","receivedAt":"2013-08-29T15:44:16Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"> From: Junio C Hamano \n\n> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\n> index b1630ba..33fbd8c 100644\n> --- a/Documentation/git-diff.txt\n> +++ b/Documentation/git-diff.txt\n> @@ -28,11 +28,15 @@ two blob objects, or changes between two files on disk.\n> \twords, the differences are what you _could_ tell Git to\n> \tfurther add to the index but you still haven't. You can\n> \tstage these changes by using linkgit:git-add[1].\n> -+\n> -If exactly two paths are given and at least one points outside\n> -the current repository, 'git diff' will compare the two files /\n> -directories. This behavior can be forced by --no-index or by\n> -executing 'git diff' outside of a working tree.\n> +\n> +'git diff' --no-index [--options] [--] [...]::\n> +\n> +\tThis form is to compare the given two paths on the\n> +\tfilesystem. You can omit the `--no-index` option when\n> +\trunning the command in a working tree controlled by Git and\n> +\tat least one of the paths points outside the working tree,\n> +\tor when running the command outside a working tree\n> +\tcontrolled by Git.\n\nThat does break out the --no-index case in a clearer way.\n\nDale\n"}]}