{"thread":{"id":"35509","subject":"[PATCH] diff: don't read index when --no-index is given","startedAt":"2013-12-09T12:05:45Z","lastAt":"2013-12-16T19:42:42Z","messageCount":30,"participants":["Thomas Gummerer","Jonathan Nieder","Jens Lehmann","Junio C Hamano","Torsten Bögershausen","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"231787","messageId":"1386590745-4412-1-git-send-email-t.gummerer@gmail.com","threadId":"35509","inReplyTo":null,"subject":"[PATCH] diff: don't read index when --no-index is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-09T12:05:45Z","receivedAt":"2013-12-09T12:05:45Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"git diff --no-index ... currently reads the index, during setup, when\ncalling gitmodules_config(). In the usual case this gives us some\nperformance drawbacks, but it's especially annoying if there is a broken\nindex file.\n\nAvoid calling the unnecessary gitmodules_config() when the --no-index\noption is given. Also add a test to guard against similar breakages in the future.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/diff.c | 13 +++++++++++--\n t/t4053-diff-no-index.sh | 6 ++++++\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex adb93a9..47c0833 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -257,7 +257,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint blobs = 0, paths = 0;\n \tconst char *path = NULL;\n \tstruct blobinfo blob[2];\n-\tint nongit;\n+\tint nongit, no_index = 0;\n \tint result = 0;\n \n \t/*\n@@ -282,9 +282,18 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t *\n \t * Other cases are errors.\n \t */\n+\tfor (i = 1; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\"))\n+\t\t\tbreak;\n+\t\tif (!strcmp(argv[i], \"--no-index\")) {\n+\t\t\tno_index = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n \n \tprefix = setup_git_directory_gently(&nongit);\n-\tgitmodules_config();\n+\tif (!no_index)\n+\t\tgitmodules_config();\n \tgit_config(git_diff_ui_config, NULL);\n \n \tinit_revisions(&rev, prefix);\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 979e983..a24ae4d 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -29,4 +29,10 @@ test_expect_success 'git diff --no-index relative path outside repo' '\n \t)\n '\n \n+test_expect_success 'git diff --no-index with broken index' '\n+\tcd repo &&\n+\techo broken >.git/index &&\n+\ttest_expect_code 0 git diff --no-index a ../non/git/a\n+'\n+\n test_done\n-- \n1.8.4.2\n"},{"id":"231789","messageId":"20131209151609.GA14841@google.com","threadId":"35509","inReplyTo":"1386590745-4412-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH] diff: don't read index when --no-index is given","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-09T15:16:09Z","receivedAt":"2013-12-09T15:16:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Gummerer wrote:\n\n> git diff --no-index ... currently reads the index, during setup, when\n> calling gitmodules_config(). In the usual case this gives us some\n> performance drawbacks,\n\nMakes sense.\n\n> but it's especially annoying if there is a broken\n> index file.\n\nIs this really a normal case? It makes sense that as a side-effect it\nis easier to use \"git diff --no-index\" as a general-purpose tool while\ninvestigating a broken repo, but I would have thought that quickly\nlearning a repo is broken is a good thing in any case.\n\nA little more information about the context where this came up would\nbe helpful, I guess.\n\n[...]\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n[...]\n> @@ -282,9 +282,18 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t *\n> \t * Other cases are errors.\n> \t */\n> +\tfor (i = 1; i < argc; i++) {\n> +\t\tif (!strcmp(argv[i], \"--\"))\n> +\t\t\tbreak;\n> +\t\tif (!strcmp(argv[i], \"--no-index\")) {\n> +\t\t\tno_index = 1;\n> +\t\t\tbreak;\n> +\t\t}\n\nsetup_revisions() uses the same logic that doesn't handle options that\ntake arguments in their \"unstuck\" form (e.g., \"--word-diff-regex --\"),\nso this is probably not a regression, though I haven't checked. :)\n\n[...]\n> \tprefix = setup_git_directory_gently(&nongit);\n> -\tgitmodules_config();\n> +\tif (!no_index)\n> +\t\tgitmodules_config();\n\nPerhaps we can improve performance and behavior by skipping the\nsetup_git_directory_gently() call, too?\n\nThat would help with the repairing-broken-repository case by\nworking even if .git/config or .git/HEAD is broken, and I think\nit is more intuitive that the repository-local configuration is\nignored by \"git diff --no-index\". It would also help with\nperformance by avoiding some filesystem access.\n\n[...]\n> +test_expect_success 'git diff --no-index with broken index' '\n> +\tcd repo &&\n> +\techo broken >.git/index &&\n> +\ttest_expect_code 0 git diff --no-index a ../non/git/a\n\nClever. I wouldn't use \"test_expect_code 0\", since that's\nalready implied by including the \"git diff --no-index\" call\nin the && chain.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"231790","messageId":"52A5FD7A.9030801@web.de","threadId":"35509","inReplyTo":"20131209151609.GA14841@google.com","subject":"Re: [PATCH] diff: don't read index when --no-index is given","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-12-09T17:27:22Z","receivedAt":"2013-12-09T17:27:22Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 09.12.2013 16:16, schrieb Jonathan Nieder:\n> Thomas Gummerer wrote:\n> \n>> git diff --no-index ... currently reads the index, during setup, when\n>> calling gitmodules_config(). In the usual case this gives us some\n>> performance drawbacks,\n> \n> Makes sense.\n\nHmm, but this will disable the submodule specific ignore configuration\noptions defined in the .gitmodules file, no? (E.g. when diffing two\ndirectories containing submodules)\n\n>> but it's especially annoying if there is a broken\n>> index file.\n>\n> Is this really a normal case? It makes sense that as a side-effect it\n> is easier to use \"git diff --no-index\" as a general-purpose tool while\n> investigating a broken repo, but I would have thought that quickly\n> learning a repo is broken is a good thing in any case.\n\nBut I agree that dying with \"index file corrupt\" is a bit strange when\ncalling diff with --no-index. Wouldn't adding a \"gently\" option (which\ncould then warn instead of dying) to gitmodules_config() be a better\nsolution here?\n"},{"id":"231796","messageId":"20131209190635.GQ29959@google.com","threadId":"35509","inReplyTo":"52A5FD7A.9030801@web.de","subject":"Re: [PATCH] diff: don't read index when --no-index is given","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-09T19:06:35Z","receivedAt":"2013-12-09T19:06:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jens Lehmann wrote:\n> Am 09.12.2013 16:16, schrieb Jonathan Nieder:\n>> Thomas Gummerer wrote:\n\n>>> git diff --no-index ... currently reads the index, during setup, when\n>>> calling gitmodules_config(). In the usual case this gives us some\n>>> performance drawbacks,\n>>\n>> Makes sense.\n>\n> Hmm, but this will disable the submodule specific ignore configuration\n> options defined in the .gitmodules file, no? (E.g. when diffing two\n> directories containing submodules)\n\nYes. That seems like a good behavior.\n\n\"git diff --no-index\" was invented as just a fancy version of 'diff\n-uR\", without any awareness of the current git repository. That means\nthat at least in principle, .gitmodules and .gitignore should not\naffect it.\n\n[...]\n> Wouldn't adding a \"gently\" option (which\n> could then warn instead of dying) to gitmodules_config() be a better\n> solution here?\n\nI don't think so.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"231798","messageId":"87siu1n8g6.fsf@gmail.com","threadId":"35509","inReplyTo":"20131209151609.GA14841@google.com","subject":"Re: [PATCH] diff: don't read index when --no-index is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-09T19:14:01Z","receivedAt":"2013-12-09T19:14:01Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Jonathan Nieder writes:\n\n> Thomas Gummerer wrote:\n>\n>> git diff --no-index ... currently reads the index, during setup, when\n>> calling gitmodules_config(). In the usual case this gives us some\n>> performance drawbacks,\n>\n> Makes sense.\n>\n>> but it's especially annoying if there is a broken\n>> index file.\n>\n> Is this really a normal case? It makes sense that as a side-effect it\n> is easier to use \"git diff --no-index\" as a general-purpose tool while\n> investigating a broken repo, but I would have thought that quickly\n> learning a repo is broken is a good thing in any case.\n>\n> A little more information about the context where this came up would\n> be helpful, I guess.\n\nIt came up while I was working on index-v5, where I had to investigate\nquite a few repositories where the index was broken, especially when I\nwas changing the index format slightly. For example I would take one\nversion, use test-dump-cache-tree to dump the cache tree to a file,\nchange the format slightly, use test-dump-cache-tree again, and check\nthe difference with \"git diff --no-index\".\n\nThis might not be a very common use case, but maybe the patch might help\nsomeone else too. (In addition to the performance improvements)\n\nI'm not sure how much diff --no-index is used normally, but when the\nindex is broken that would be detected relatively soon anyway. I'm not\nso worried about one more command working when it's broken.\n\n> [...]\n>> --- a/builtin/diff.c\n>> +++ b/builtin/diff.c\n> [...]\n>> @@ -282,9 +282,18 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>> \t *\n>> \t * Other cases are errors.\n>> \t */\n>> +\tfor (i = 1; i < argc; i++) {\n>> +\t\tif (!strcmp(argv[i], \"--\"))\n>> +\t\t\tbreak;\n>> +\t\tif (!strcmp(argv[i], \"--no-index\")) {\n>> +\t\t\tno_index = 1;\n>> +\t\t\tbreak;\n>> +\t\t}\n>\n> setup_revisions() uses the same logic that doesn't handle options that\n> take arguments in their \"unstuck\" form (e.g., \"--word-diff-regex --\"),\n> so this is probably not a regression, though I haven't checked. :)\n\nThe same logic is used in diff_no_index(), so I think it should be fine.\n\n> [...]\n>> \tprefix = setup_git_directory_gently(&nongit);\n>> -\tgitmodules_config();\n>> +\tif (!no_index)\n>> +\t\tgitmodules_config();\n>\n> Perhaps we can improve performance and behavior by skipping the\n> setup_git_directory_gently() call, too?\n>\n> That would help with the repairing-broken-repository case by\n> working even if .git/config or .git/HEAD is broken, and I think\n> it is more intuitive that the repository-local configuration is\n> ignored by \"git diff --no-index\". It would also help with\n> performance by avoiding some filesystem access.\n\nYes, I think that would make sense, thanks. I tested it, and it didn't\nchange the performance, but it's still a good change for the broken\nrepository case. Will change in the re-roll and add a test for that.\n\n> [...]\n>> +test_expect_success 'git diff --no-index with broken index' '\n>> +\tcd repo &&\n>> +\techo broken >.git/index &&\n>> +\ttest_expect_code 0 git diff --no-index a ../non/git/a\n>\n> Clever. I wouldn't use \"test_expect_code 0\", since that's\n> already implied by including the \"git diff --no-index\" call\n> in the && chain.\n\nThanks, will change. Thanks a lot for your review. Will send a re-roll\nsoon.\n\n> Thanks and hope that helps,\n> Jonathan\n\n--\nThomas\n"},{"id":"231799","messageId":"20131209192000.GS29959@google.com","threadId":"35509","inReplyTo":"87siu1n8g6.fsf@gmail.com","subject":"Re: [PATCH] diff: don't read index when --no-index is given","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-09T19:20:00Z","receivedAt":"2013-12-09T19:20:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Gummerer wrote:\n\n> For example I would take one\n> version, use test-dump-cache-tree to dump the cache tree to a file,\n> change the format slightly, use test-dump-cache-tree again, and check\n> the difference with \"git diff --no-index\".\n\nMakes a lot of sense. Thanks for explaining.\n\nRegards,\nJonathan\n"},{"id":"231807","messageId":"xmqqwqjdydga.fsf@gitster.dls.corp.google.com","threadId":"35509","inReplyTo":"1386590745-4412-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH] diff: don't read index when --no-index is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-09T20:30:29Z","receivedAt":"2013-12-09T20:30:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> git diff --no-index ... currently reads the index, during setup, when\n> calling gitmodules_config(). In the usual case this gives us some\n> performance drawbacks, but it's especially annoying if there is a broken\n> index file.\n>\n> Avoid calling the unnecessary gitmodules_config() when the --no-index\n> option is given. Also add a test to guard against similar breakages in the future.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n> builtin/diff.c | 13 +++++++++++--\n> t/t4053-diff-no-index.sh | 6 ++++++\n> 2 files changed, 17 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index adb93a9..47c0833 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -257,7 +257,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \tint blobs = 0, paths = 0;\n> \tconst char *path = NULL;\n> \tstruct blobinfo blob[2];\n> -\tint nongit;\n> +\tint nongit, no_index = 0;\n> \tint result = 0;\n> \n> \t/*\n> @@ -282,9 +282,18 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t *\n> \t * Other cases are errors.\n> \t */\n> +\tfor (i = 1; i < argc; i++) {\n> +\t\tif (!strcmp(argv[i], \"--\"))\n> +\t\t\tbreak;\n> +\t\tif (!strcmp(argv[i], \"--no-index\")) {\n> +\t\t\tno_index = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nThis seems to duplicate only half the logic at the beginning of\ndiff_no_index(), right? E.g., running \"git diff /var/tmp/[12]\"\ninside a working tree that is controlled by a Git repository when\n/var/tmp/ is outside, we do want to behave as if the command line\nwere \"git diff --no-index /var/tmp/[12]\", but this half duplication\nmakes these two behave differently, no?\n\nI think the issue you are trying to address is worth tackling, but I\nwonder if a bit of preparatory refactoring is necessary to avoid the\npartial duplication.\n\n> \tprefix = setup_git_directory_gently(&nongit);\n> -\tgitmodules_config();\n> +\tif (!no_index)\n> +\t\tgitmodules_config();\n> \tgit_config(git_diff_ui_config, NULL);\n> \n> \tinit_revisions(&rev, prefix);\n> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n> index 979e983..a24ae4d 100755\n> --- a/t/t4053-diff-no-index.sh\n> +++ b/t/t4053-diff-no-index.sh\n> @@ -29,4 +29,10 @@ test_expect_success 'git diff --no-index relative path outside repo' '\n> \t)\n> '\n> \n> +test_expect_success 'git diff --no-index with broken index' '\n> +\tcd repo &&\n> +\techo broken >.git/index &&\n> +\ttest_expect_code 0 git diff --no-index a ../non/git/a\n> +'\n> +\n> test_done\n"},{"id":"231810","messageId":"1386621634-25444-1-git-send-email-t.gummerer@gmail.com","threadId":"35509","inReplyTo":"20131209192000.GS29959@google.com","subject":"[PATCH v2] diff: don't read index when --no-index is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-09T20:40:34Z","receivedAt":"2013-12-09T20:40:34Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"git diff --no-index ... currently reads the index, during setup, when\ncalling gitmodules_config(). This results in worse performance when\nthe index is not actually needed. This patch avoids calling\ngitmodules_config() when the --no-index option is given. The times for\nexecuting \"git diff --no-index\" in the WebKit repository are improved as\nfollows:\n\nTest HEAD~3 HEAD\n------------------------------------------------------------------\n4001.1: diff --no-index 0.24(0.15+0.09) 0.01(0.00+0.00) -95.8%\n\nAn additional improvement of this patch is that \"git diff --no-index\" no\nlonger breaks when the index file is corrupt, which makes it possible to\nuse it for investigating the broken repository.\n\nTo improve the possible usage as investigation tool for broken\nrepositories, setup_git_directory_gently() is also not called when the\n--no-index option is given.\n\nAlso add a test to guard against future breakages, and a performance\ntest to show the improvements.\n\nSigned-off-by: Thomas Gummerer \n---\n\nThanks to Jonathan and Jens for comments on the previous round.\nChanges:\n - Don't all setup_git_directory_gently when --no-index is given\n - Add performance test\n - Commit message improvements\n\n builtin/diff.c | 16 +++++++++++++---\n t/perf/p4001-diff-no-index.sh | 17 +++++++++++++++++\n t/t4053-diff-no-index.sh | 6 ++++++\n 3 files changed, 36 insertions(+), 3 deletions(-)\n create mode 100755 t/perf/p4001-diff-no-index.sh\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex adb93a9..5f09a0b 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -257,7 +257,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint blobs = 0, paths = 0;\n \tconst char *path = NULL;\n \tstruct blobinfo blob[2];\n-\tint nongit;\n+\tint nongit, no_index = 0;\n \tint result = 0;\n \n \t/*\n@@ -282,9 +282,19 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t *\n \t * Other cases are errors.\n \t */\n+\tfor (i = 1; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\"))\n+\t\t\tbreak;\n+\t\tif (!strcmp(argv[i], \"--no-index\")) {\n+\t\t\tno_index = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n \n-\tprefix = setup_git_directory_gently(&nongit);\n-\tgitmodules_config();\n+\tif (!no_index) {\n+\t\tprefix = setup_git_directory_gently(&nongit);\n+\t\tgitmodules_config();\n+\t}\n \tgit_config(git_diff_ui_config, NULL);\n \n \tinit_revisions(&rev, prefix);\ndiff --git a/t/perf/p4001-diff-no-index.sh b/t/perf/p4001-diff-no-index.sh\nnew file mode 100755\nindex 0000000..81c7aa0\n--- /dev/null\n+++ b/t/perf/p4001-diff-no-index.sh\n@@ -0,0 +1,17 @@\n+#!/bin/sh\n+\n+test_description=\"Test diff --no-index performance\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+file1=$(git ls-files | tail -n 2 | head -1)\n+file2=$(git ls-files | tail -n 1 | head -1)\n+\n+test_perf \"diff --no-index\" \"\n+\tgit diff --no-index $file1 $file2 >/dev/null\n+\"\n+\n+test_done\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 979e983..d3dbf6b 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -29,4 +29,10 @@ test_expect_success 'git diff --no-index relative path outside repo' '\n \t)\n '\n \n+test_expect_success 'git diff --no-index with broken index' '\n+\tcd repo &&\n+\techo broken >.git/index &&\n+\tgit diff --no-index a ../non/git/a &&\n+'\n+\n test_done\n-- \n1.8.5.4.g8639e57\n"},{"id":"231814","messageId":"52A62E4B.8080001@web.de","threadId":"35509","inReplyTo":"1386621634-25444-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v2] diff: don't read index when --no-index is given","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-12-09T20:55:39Z","receivedAt":"2013-12-09T20:55:39Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-12-09 21.40, Thomas Gummerer wrote:\n> \n> +test_expect_success 'git diff --no-index with broken index' '\n> +\tcd repo &&\n> +\techo broken >.git/index &&\n> +\tgit diff --no-index a ../non/git/a &&\n ^^\nI'm confused: Does this work with the trailing && ?\n\n\n(and when we use\n\"cd repo\"\nit could be good to use a sub-shell (even when this is the last test case)\n"},{"id":"231816","messageId":"CAPig+cT_TXEAV-Jb_1N8vhKyenpEJLpw+7J45bsQ6aycdO3ftg@mail.gmail.com","threadId":"35509","inReplyTo":"1386621634-25444-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v2] diff: don't read index when --no-index is given","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-12-09T20:57:21Z","receivedAt":"2013-12-09T20:57:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 9, 2013 at 3:40 PM, Thomas Gummerer wrote:\n> git diff --no-index ... currently reads the index, during setup, when\n> calling gitmodules_config(). This results in worse performance when\n> the index is not actually needed. This patch avoids calling\n> gitmodules_config() when the --no-index option is given. The times for\n> executing \"git diff --no-index\" in the WebKit repository are improved as\n> follows:\n>\n> Test HEAD~3 HEAD\n> ------------------------------------------------------------------\n> 4001.1: diff --no-index 0.24(0.15+0.09) 0.01(0.00+0.00) -95.8%\n>\n> An additional improvement of this patch is that \"git diff --no-index\" no\n> longer breaks when the index file is corrupt, which makes it possible to\n> use it for investigating the broken repository.\n>\n> To improve the possible usage as investigation tool for broken\n> repositories, setup_git_directory_gently() is also not called when the\n> --no-index option is given.\n>\n> Also add a test to guard against future breakages, and a performance\n> test to show the improvements.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n> index 979e983..d3dbf6b 100755\n> --- a/t/t4053-diff-no-index.sh\n> +++ b/t/t4053-diff-no-index.sh\n> @@ -29,4 +29,10 @@ test_expect_success 'git diff --no-index relative path outside repo' '\n> )\n> '\n>\n> +test_expect_success 'git diff --no-index with broken index' '\n> + cd repo &&\n> + echo broken >.git/index &&\n> + git diff --no-index a ../non/git/a &&\n> +'\n\nStray && on the last line of the test.\n\nAlso, don't you want to do the 'cd' and subsequent commands inside a\nsubshell so that tests added after this one won't have to worry about\ncd'ing back to the top-level?\n\n> +\n> test_done\n> --\n> 1.8.5.4.g8639e57\n"},{"id":"231822","messageId":"87ppp5n2xp.fsf@gmail.com","threadId":"35509","inReplyTo":"xmqqwqjdydga.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] diff: don't read index when --no-index is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-09T21:13:06Z","receivedAt":"2013-12-09T21:13:06Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"\n[Sorry for not seeing this before sending out v2.]\n\nJunio C Hamano writes:\n\n> Thomas Gummerer writes:\n>\n>> git diff --no-index ... currently reads the index, during setup, when\n>> calling gitmodules_config(). In the usual case this gives us some\n>> performance drawbacks, but it's especially annoying if there is a broken\n>> index file.\n>>\n>> Avoid calling the unnecessary gitmodules_config() when the --no-index\n>> option is given. Also add a test to guard against similar breakages in the future.\n>>\n>> Signed-off-by: Thomas Gummerer \n>> ---\n>> builtin/diff.c | 13 +++++++++++--\n>> t/t4053-diff-no-index.sh | 6 ++++++\n>> 2 files changed, 17 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/builtin/diff.c b/builtin/diff.c\n>> index adb93a9..47c0833 100644\n>> --- a/builtin/diff.c\n>> +++ b/builtin/diff.c\n>> @@ -257,7 +257,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>> \tint blobs = 0, paths = 0;\n>> \tconst char *path = NULL;\n>> \tstruct blobinfo blob[2];\n>> -\tint nongit;\n>> +\tint nongit, no_index = 0;\n>> \tint result = 0;\n>>\n>> \t/*\n>> @@ -282,9 +282,18 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>> \t *\n>> \t * Other cases are errors.\n>> \t */\n>> +\tfor (i = 1; i < argc; i++) {\n>> +\t\tif (!strcmp(argv[i], \"--\"))\n>> +\t\t\tbreak;\n>> +\t\tif (!strcmp(argv[i], \"--no-index\")) {\n>> +\t\t\tno_index = 1;\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t}\n>\n> This seems to duplicate only half the logic at the beginning of\n> diff_no_index(), right? E.g., running \"git diff /var/tmp/[12]\"\n> inside a working tree that is controlled by a Git repository when\n> /var/tmp/ is outside, we do want to behave as if the command line\n> were \"git diff --no-index /var/tmp/[12]\", but this half duplication\n> makes these two behave differently, no?\n\nYes you're right, I missed that. \"git diff /var/tmp/[12]\" inside a\nworking tree with a broken index has the same problems, which should be\nfixed too. I'll try to fix that and send a new patch afterwards.\n\n> I think the issue you are trying to address is worth tackling, but I\n> wonder if a bit of preparatory refactoring is necessary to avoid the\n> partial duplication.\n>\n>> \tprefix = setup_git_directory_gently(&nongit);\n>> -\tgitmodules_config();\n>> +\tif (!no_index)\n>> +\t\tgitmodules_config();\n>> \tgit_config(git_diff_ui_config, NULL);\n>>\n>> \tinit_revisions(&rev, prefix);\n>> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n>> index 979e983..a24ae4d 100755\n>> --- a/t/t4053-diff-no-index.sh\n>> +++ b/t/t4053-diff-no-index.sh\n>> @@ -29,4 +29,10 @@ test_expect_success 'git diff --no-index relative path outside repo' '\n>> \t)\n>> '\n>>\n>> +test_expect_success 'git diff --no-index with broken index' '\n>> +\tcd repo &&\n>> +\techo broken >.git/index &&\n>> +\ttest_expect_code 0 git diff --no-index a ../non/git/a\n>> +'\n>> +\n>> test_done\n\n--\nThomas\n"},{"id":"231824","messageId":"87mwk9n2r4.fsf@gmail.com","threadId":"35509","inReplyTo":"CAPig+cT_TXEAV-Jb_1N8vhKyenpEJLpw+7J45bsQ6aycdO3ftg@mail.gmail.com","subject":"Re: [PATCH v2] diff: don't read index when --no-index is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-09T21:17:03Z","receivedAt":"2013-12-09T21:17:03Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Dec 9, 2013 at 3:40 PM, Thomas Gummerer wrote:\n>> git diff --no-index ... currently reads the index, during setup, when\n>> calling gitmodules_config(). This results in worse performance when\n>> the index is not actually needed. This patch avoids calling\n>> gitmodules_config() when the --no-index option is given. The times for\n>> executing \"git diff --no-index\" in the WebKit repository are improved as\n>> follows:\n>>\n>> Test HEAD~3 HEAD\n>> ------------------------------------------------------------------\n>> 4001.1: diff --no-index 0.24(0.15+0.09) 0.01(0.00+0.00) -95.8%\n>>\n>> An additional improvement of this patch is that \"git diff --no-index\" no\n>> longer breaks when the index file is corrupt, which makes it possible to\n>> use it for investigating the broken repository.\n>>\n>> To improve the possible usage as investigation tool for broken\n>> repositories, setup_git_directory_gently() is also not called when the\n>> --no-index option is given.\n>>\n>> Also add a test to guard against future breakages, and a performance\n>> test to show the improvements.\n>>\n>> Signed-off-by: Thomas Gummerer \n>> ---\n>> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n>> index 979e983..d3dbf6b 100755\n>> --- a/t/t4053-diff-no-index.sh\n>> +++ b/t/t4053-diff-no-index.sh\n>> @@ -29,4 +29,10 @@ test_expect_success 'git diff --no-index relative path outside repo' '\n>> )\n>> '\n>>\n>> +test_expect_success 'git diff --no-index with broken index' '\n>> + cd repo &&\n>> + echo broken >.git/index &&\n>> + git diff --no-index a ../non/git/a &&\n>> +'\n>\n> Stray && on the last line of the test.\n>\n> Also, don't you want to do the 'cd' and subsequent commands inside a\n> subshell so that tests added after this one won't have to worry about\n> cd'ing back to the top-level?\n\nThanks both to you and Torsten for catching both issues, I'll fix them\nin a re-roll.\n\n>> +\n>> test_done\n>> --\n>> 1.8.5.4.g8639e57\n\n--\nThomas\n"},{"id":"231858","messageId":"1386697935-18635-1-git-send-email-t.gummerer@gmail.com","threadId":"35509","inReplyTo":"xmqqwqjdydga.fsf@gitster.dls.corp.google.com","subject":"[PATCH v3 1/2] diff: move no-index detection to builtin/diff.c","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-10T17:52:14Z","receivedAt":"2013-12-10T17:52:14Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Currently the --no-index option is parsed in diff_no_index(). Move the\ndetection if a no-index diff should be executed to builtin/diff.c, where\nwe can use it for executing diff_no_index() conditionally. This will\nalso allow us to execute other operations conditionally, which will be\ndone in the next patch.\n\nSigned-off-by: Thomas Gummerer \n---\n\nThanks to Junio, Torsten and Eric for comments on the previous round.\nI've added this refactoring patch, to avoid the partial duplication of\nthe logic. I've also fixed the tests, that now use a sub-shell for\nexecuting and fix the stray && at the end of the test.\n\n builtin/diff.c | 45 ++++++++++++++++++++++++++++++++++++++++++---\n diff-no-index.c | 48 +++---------------------------------------------\n diff.h | 2 +-\n 3 files changed, 46 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex adb93a9..7220b2c 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -257,7 +257,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint blobs = 0, paths = 0;\n \tconst char *path = NULL;\n \tstruct blobinfo blob[2];\n-\tint nongit;\n+\tint nongit, no_index = 0;\n \tint result = 0;\n \n \t/*\n@@ -283,14 +283,53 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t * Other cases are errors.\n \t */\n \n+\t/* Were we asked to do --no-index explicitly? */\n+\tfor (i = 1; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\ti++;\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!strcmp(argv[i], \"--no-index\"))\n+\t\t\tno_index = 1;\n+\t\tif (argv[i][0] != '-')\n+\t\t\tbreak;\n+\t}\n+\n \tprefix = setup_git_directory_gently(&nongit);\n+\t/*\n+\t * Treat git diff with at least one path outside of the\n+\t * repo the same as if the command would have been executed\n+\t * outside of a git repository. In this case it behaves\n+\t * the same way as \"git diff --no-index \", which acts\n+\t * as a colourful \"diff\" replacement.\n+\t */\n+\tnongit |= (argc == i + 2) && !(path_inside_repo(prefix, argv[i]) &&\n+\t\t\t\t path_inside_repo(prefix, argv[i + 1]));\n \tgitmodules_config();\n \tgit_config(git_diff_ui_config, NULL);\n \n \tinit_revisions(&rev, prefix);\n \n-\t/* If this is a no-index diff, just run it and exit there. */\n-\tdiff_no_index(&rev, argc, argv, nongit, prefix);\n+\tif (no_index || nongit) {\n+\t\tif (argc != i + 2) {\n+\t\t\tif (!no_index) {\n+\t\t\t\t/*\n+\t\t\t\t * There was no --no-index and there were not two\n+\t\t\t\t * paths. It is possible that the user intended\n+\t\t\t\t * to do an inside-repository operation.\n+\t\t\t\t */\n+\t\t\t\tfprintf(stderr, \"Not a git repository\\n\");\n+\t\t\t\tfprintf(stderr,\n+\t\t\t\t\t\"To compare two paths outside a working tree:\\n\");\n+\t\t\t}\n+\t\t\t/* Give the usage message for non-repository usage and exit. */\n+\t\t\tusagef(\"git diff %s \",\n+\t\t\t no_index ? \"--no-index\" : \"[--no-index]\");\n+\n+\t\t}\n+\t\t/* If this is a no-index diff, just run it and exit there. */\n+\t\tdiff_no_index(&rev, argc, argv, nongit, no_index, prefix);\n+\t}\n \n \t/* Otherwise, we are doing the usual \"git\" diff */\n \trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 00a8eef..78e3090 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -181,56 +181,14 @@ static int queue_diff(struct diff_options *o,\n \t}\n }\n \n-void diff_no_index(struct rev_info *revs,\n-\t\t int argc, const char **argv,\n-\t\t int nongit, const char *prefix)\n+int diff_no_index(struct rev_info *revs,\n+\t\t int argc, const char **argv,\n+\t\t int nongit, int no_index, const char *prefix)\n {\n \tint i, prefixlen;\n-\tint no_index = 0;\n \tunsigned deprecated_show_diff_q_option_used = 0;\n \tconst char *paths[2];\n \n-\t/* Were we asked to do --no-index explicitly? */\n-\tfor (i = 1; i < argc; i++) {\n-\t\tif (!strcmp(argv[i], \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (!strcmp(argv[i], \"--no-index\"))\n-\t\t\tno_index = 1;\n-\t\tif (argv[i][0] != '-')\n-\t\t\tbreak;\n-\t}\n-\n-\tif (!no_index && !nongit) {\n-\t\t/*\n-\t\t * Inside a git repository, without --no-index. Only\n-\t\t * when a path outside the repository is given,\n-\t\t * e.g. \"git diff /var/tmp/[12]\", or \"git diff\n-\t\t * Makefile /var/tmp/Makefile\", allow it to be used as\n-\t\t * a colourful \"diff\" replacement.\n-\t\t */\n-\t\tif ((argc != i + 2) ||\n-\t\t (path_inside_repo(prefix, argv[i]) &&\n-\t\t path_inside_repo(prefix, argv[i+1])))\n-\t\t\treturn;\n-\t}\n-\tif (argc != i + 2) {\n-\t\tif (!no_index) {\n-\t\t\t/*\n-\t\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\t */\n-\t\t\tfprintf(stderr, \"Not a git repository\\n\");\n-\t\t\tfprintf(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 \t\tint j;\ndiff --git a/diff.h b/diff.h\nindex e342325..3e1828a 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -330,7 +330,7 @@ extern int diff_flush_patch_id(struct diff_options *, unsigned char *);\n \n extern int diff_result_code(struct diff_options *, int);\n \n-extern void diff_no_index(struct rev_info *, int, const char **, int, const char *);\n+extern int diff_no_index(struct rev_info *, int, const char **, int, int, const char *);\n \n extern int index_differs_from(const char *def, int diff_flags);\n \n-- \n1.8.5.4.g8639e57\n"},{"id":"231859","messageId":"1386697935-18635-2-git-send-email-t.gummerer@gmail.com","threadId":"35509","inReplyTo":"1386697935-18635-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v3 2/2] diff: don't read index when --no-index is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-10T17:52:15Z","receivedAt":"2013-12-10T17:52:15Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"git diff --no-index ... currently reads the index, during setup, when\ncalling gitmodules_config(). This results in worse performance when the\nindex is not actually needed. This patch avoids calling\ngitmodules_config() when the --no-index option is given. The times for\nexecuting \"git diff --no-index\" in the WebKit repository are improved as\nfollows:\n\nTest HEAD~3 HEAD\n------------------------------------------------------------------\n4001.1: diff --no-index 0.24(0.15+0.09) 0.01(0.00+0.00) -95.8%\n\nAn additional improvement of this patch is that \"git diff --no-index\" no\nlonger breaks when the index file is corrupt, which makes it possible to\nuse it for investigating the broken repository.\n\nTo improve the possible usage as investigation tool for broken\nrepositories, setup_git_directory_gently() is also not called when the\n--no-index option is given.\n\nAlso add a test to guard against future breakages, and a performance\ntest to show the improvements.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/diff.c | 7 +++++--\n t/perf/p4001-diff-no-index.sh | 17 +++++++++++++++++\n t/t4053-diff-no-index.sh | 15 +++++++++++++++\n 3 files changed, 37 insertions(+), 2 deletions(-)\n create mode 100755 t/perf/p4001-diff-no-index.sh\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 7220b2c..9ce8bec 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -295,7 +295,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \t}\n \n-\tprefix = setup_git_directory_gently(&nongit);\n+\tif (!no_index)\n+\t\tprefix = setup_git_directory_gently(&nongit);\n+\n \t/*\n \t * Treat git diff with at least one path outside of the\n \t * repo the same as if the command would have been executed\n@@ -305,7 +307,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t */\n \tnongit |= (argc == i + 2) && !(path_inside_repo(prefix, argv[i]) &&\n \t\t\t\t path_inside_repo(prefix, argv[i + 1]));\n-\tgitmodules_config();\n+\tif (!no_index && !nongit)\n+\t\tgitmodules_config();\n \tgit_config(git_diff_ui_config, NULL);\n \n \tinit_revisions(&rev, prefix);\ndiff --git a/t/perf/p4001-diff-no-index.sh b/t/perf/p4001-diff-no-index.sh\nnew file mode 100755\nindex 0000000..81c7aa0\n--- /dev/null\n+++ b/t/perf/p4001-diff-no-index.sh\n@@ -0,0 +1,17 @@\n+#!/bin/sh\n+\n+test_description=\"Test diff --no-index performance\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+file1=$(git ls-files | tail -n 2 | head -1)\n+file2=$(git ls-files | tail -n 1 | head -1)\n+\n+test_perf \"diff --no-index\" \"\n+\tgit diff --no-index $file1 $file2 >/dev/null\n+\"\n+\n+test_done\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 979e983..077c775 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -29,4 +29,19 @@ test_expect_success 'git diff --no-index relative path outside repo' '\n \t)\n '\n \n+test_expect_success 'git diff --no-index with broken index' '\n+\t(\n+\t\tcd repo &&\n+\t\techo broken >.git/index &&\n+\t\tgit diff --no-index a ../non/git/a\n+\t)\n+'\n+\n+test_expect_success 'git diff outside repo with broken index' '\n+\t(\n+\t\tcd repo &&\n+\t\tgit diff ../non/git/a ../non/git/b\n+\t)\n+'\n+\n test_done\n-- \n1.8.5.4.g8639e57\n"},{"id":"231860","messageId":"20131210181655.GB2311@google.com","threadId":"35509","inReplyTo":"1386697935-18635-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v3 1/2] diff: move no-index detection to builtin/diff.c","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-10T18:16:55Z","receivedAt":"2013-12-10T18:16:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(pruning cc list)\nHi,\n\nThomas Gummerer wrote:\n\n> Currently the --no-index option is parsed in diff_no_index(). Move the\n> detection if a no-index diff should be executed to builtin/diff.c\n\nNo functional change intended, I assume?\n\n[...]\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -283,14 +283,53 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n[...]\n> \tprefix = setup_git_directory_gently(&nongit);\n> +\t/*\n> +\t * Treat git diff with at least one path outside of the\n> +\t * repo the same as if the command would have been executed\n> +\t * outside of a git repository. In this case it behaves\n> +\t * the same way as \"git diff --no-index \", which acts\n> +\t * as a colourful \"diff\" replacement.\n> +\t */\n> +\tnongit |= (argc == i + 2) && !(path_inside_repo(prefix, argv[i]) &&\n> +\t\t\t\t path_inside_repo(prefix, argv[i + 1]));\n\nI would find this easier to read as\n\n\tif (argc == i + 2 &&\n\t (!path_inside_repo(prefix, argv[i]) ||\n\t !path_inside_repo(prefix, argv[i + 1])))\n\t\tnongit = 1;\n\nOr maybe using a different variable than 'nongit':\n\n #define DIFF_NO_INDEX_EXPLICIT 1\n #define DIFF_NO_INDEX_IMPLICIT 2\n\n\t...\n\tif (argc == i + 2 && ...)\n\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n\n[...]\n> \tgitmodules_config();\n> \tgit_config(git_diff_ui_config, NULL);\n> \n> \tinit_revisions(&rev, prefix);\n> \n> -\t/* If this is a no-index diff, just run it and exit there. */\n> -\tdiff_no_index(&rev, argc, argv, nongit, prefix);\n> +\tif (no_index || nongit) {\n[...]\n> +\t}\n\nOk.\n\n[...]\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -181,56 +181,14 @@ static int queue_diff(struct diff_options *o,\n> \t}\n> }\n> \n> -void diff_no_index(struct rev_info *revs,\n> +int diff_no_index(struct rev_info *revs,\n\nWhy the change in return type?\n\nHope that helps,\nJonathan\n"},{"id":"231861","messageId":"20131210181840.GC2311@google.com","threadId":"35509","inReplyTo":"1386697935-18635-2-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v3 2/2] diff: don't read index when --no-index is given","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-10T18:18:40Z","receivedAt":"2013-12-10T18:18:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Gummerer wrote:\n\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -295,7 +295,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\t\tbreak;\n> \t}\n> \n> -\tprefix = setup_git_directory_gently(&nongit);\n> +\tif (!no_index)\n> +\t\tprefix = setup_git_directory_gently(&nongit);\n\nWhat is the value of nongit in the no_index case?\n"},{"id":"231862","messageId":"87eh5kmtlq.fsf@gmail.com","threadId":"35509","inReplyTo":"20131210181655.GB2311@google.com","subject":"Re: [PATCH v3 1/2] diff: move no-index detection to builtin/diff.c","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-10T18:46:57Z","receivedAt":"2013-12-10T18:46:57Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Jonathan Nieder writes:\n\n> (pruning cc list)\n> Hi,\n>\n> Thomas Gummerer wrote:\n>\n>> Currently the --no-index option is parsed in diff_no_index(). Move the\n>> detection if a no-index diff should be executed to builtin/diff.c\n>\n> No functional change intended, I assume?\n\nCorrect, I thought I'd split the refactoring from the functionality\nchanges.\n\n> [...]\n>> --- a/builtin/diff.c\n>> +++ b/builtin/diff.c\n>> @@ -283,14 +283,53 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> [...]\n>> \tprefix = setup_git_directory_gently(&nongit);\n>> +\t/*\n>> +\t * Treat git diff with at least one path outside of the\n>> +\t * repo the same as if the command would have been executed\n>> +\t * outside of a git repository. In this case it behaves\n>> +\t * the same way as \"git diff --no-index \", which acts\n>> +\t * as a colourful \"diff\" replacement.\n>> +\t */\n>> +\tnongit |= (argc == i + 2) && !(path_inside_repo(prefix, argv[i]) &&\n>> +\t\t\t\t path_inside_repo(prefix, argv[i + 1]));\n>\n> I would find this easier to read as\n>\n> \tif (argc == i + 2 &&\n> \t (!path_inside_repo(prefix, argv[i]) ||\n> \t !path_inside_repo(prefix, argv[i + 1])))\n> \t\tnongit = 1;\n>\n> Or maybe using a different variable than 'nongit':\n>\n> #define DIFF_NO_INDEX_EXPLICIT 1\n> #define DIFF_NO_INDEX_IMPLICIT 2\n>\n> \t...\n> \tif (argc == i + 2 && ...)\n> \t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n\nThanks, that makes sense, will change in the re-roll.\n\n> [...]\n>> \tgitmodules_config();\n>> \tgit_config(git_diff_ui_config, NULL);\n>>\n>> \tinit_revisions(&rev, prefix);\n>>\n>> -\t/* If this is a no-index diff, just run it and exit there. */\n>> -\tdiff_no_index(&rev, argc, argv, nongit, prefix);\n>> +\tif (no_index || nongit) {\n> [...]\n>> +\t}\n>\n> Ok.\n>\n> [...]\n>> --- a/diff-no-index.c\n>> +++ b/diff-no-index.c\n>> @@ -181,56 +181,14 @@ static int queue_diff(struct diff_options *o,\n>> \t}\n>> }\n>>\n>> -void diff_no_index(struct rev_info *revs,\n>> +int diff_no_index(struct rev_info *revs,\n>\n> Why the change in return type?\n\nThat was an oversight, no change is required. Will fix in the\nre-roll. Thanks for taking the time for reviewing this.\n\n> Hope that helps,\n> Jonathan\n\n--\nThomas\n"},{"id":"231865","messageId":"87bo0omt7e.fsf@gmail.com","threadId":"35509","inReplyTo":"20131210181840.GC2311@google.com","subject":"Re: [PATCH v3 2/2] diff: don't read index when --no-index is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-10T18:55:33Z","receivedAt":"2013-12-10T18:55:33Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Jonathan Nieder writes:\n\n> Thomas Gummerer wrote:\n>\n>> --- a/builtin/diff.c\n>> +++ b/builtin/diff.c\n>> @@ -295,7 +295,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>> \t\t\tbreak;\n>> \t}\n>>\n>> -\tprefix = setup_git_directory_gently(&nongit);\n>> +\tif (!no_index)\n>> +\t\tprefix = setup_git_directory_gently(&nongit);\n>\n> What is the value of nongit in the no_index case?\n\nIn the no_index case it doesn't matter, since no_index is always checked\nfirst. The only other place where it is used, when no_index is set\nis after diff_no_index, which can't be reached if no_index is set. I\ncould initialize nongit to 0, but I don't think that would change\nanything.\n\nI've also seen that the no_index and nongit parameters to diff_no_index\nare not needed anymore, and will remove them in the re-roll therefore.\n\nI'll wait to see if there are any more comments and then send a re-roll.\n\nThanks!\n\n--\nThomas\n"},{"id":"231897","messageId":"1386755923-22132-1-git-send-email-t.gummerer@gmail.com","threadId":"35509","inReplyTo":"20131210181655.GB2311@google.com","subject":"[PATCH v4 1/2] diff: move no-index detection to builtin/diff.c","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-11T09:58:42Z","receivedAt":"2013-12-11T09:58:42Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Currently the --no-index option is parsed in diff_no_index(). Move the\ndetection if a no-index diff should be executed to builtin/diff.c, where\nwe can use it for executing diff_no_index() conditionally. This will\nalso allow us to execute other operations conditionally, which will be\ndone in the next patch.\n\nThere are no functional changes.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Thomas Gummerer \n---\n\nThanks to Jonathan for the suggestions in the previous round.\nSince the last roud I've made the no_index detection easier to read,\nand use two different values for no_index to indicate whether the\nno_index option is given explicitly, or if a no_index diff should be\nexecuted implicitly.\n\n builtin/diff.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---\n diff-no-index.c | 44 +-------------------------------------------\n diff.h | 2 +-\n 3 files changed, 51 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex adb93a9..da69e4a 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -16,6 +16,9 @@\n #include \"submodule.h\"\n #include \"sha1-array.h\"\n \n+#define DIFF_NO_INDEX_EXPLICIT 1\n+#define DIFF_NO_INDEX_IMPLICIT 2\n+\n struct blobinfo {\n \tunsigned char sha1[20];\n \tconst char *name;\n@@ -257,7 +260,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint blobs = 0, paths = 0;\n \tconst char *path = NULL;\n \tstruct blobinfo blob[2];\n-\tint nongit;\n+\tint nongit = 0, no_index = 0;\n \tint result = 0;\n \n \t/*\n@@ -283,14 +286,57 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t * Other cases are errors.\n \t */\n \n+\t/* Were we asked to do --no-index explicitly? */\n+\tfor (i = 1; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\ti++;\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!strcmp(argv[i], \"--no-index\"))\n+\t\t\tno_index = DIFF_NO_INDEX_EXPLICIT;\n+\t\tif (argv[i][0] != '-')\n+\t\t\tbreak;\n+\t}\n+\n \tprefix = setup_git_directory_gently(&nongit);\n+\t/*\n+\t * Treat git diff with at least one path outside of the\n+\t * repo the same as if the command would have been executed\n+\t * outside of a git repository. In this case it behaves\n+\t * the same way as \"git diff --no-index \", which acts\n+\t * as a colourful \"diff\" replacement.\n+\t */\n+\tif (nongit || ((argc == i + 2) &&\n+\t\t (!path_inside_repo(prefix, argv[i]) ||\n+\t\t\t!path_inside_repo(prefix, argv[i + 1]))))\n+\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n+\n \tgitmodules_config();\n \tgit_config(git_diff_ui_config, NULL);\n \n \tinit_revisions(&rev, prefix);\n \n-\t/* If this is a no-index diff, just run it and exit there. */\n-\tdiff_no_index(&rev, argc, argv, nongit, prefix);\n+\tif (no_index) {\n+\t\tif (argc != i + 2) {\n+\t\t\tif (no_index == DIFF_NO_INDEX_IMPLICIT) {\n+\t\t\t\t/*\n+\t\t\t\t * There was no --no-index and there were not two\n+\t\t\t\t * paths. It is possible that the user intended\n+\t\t\t\t * to do an inside-repository operation.\n+\t\t\t\t */\n+\t\t\t\tfprintf(stderr, \"Not a git repository\\n\");\n+\t\t\t\tfprintf(stderr,\n+\t\t\t\t\t\"To compare two paths outside a working tree:\\n\");\n+\t\t\t}\n+\t\t\t/* Give the usage message for non-repository usage and exit. */\n+\t\t\tusagef(\"git diff %s \",\n+\t\t\t no_index == DIFF_NO_INDEX_EXPLICIT ?\n+\t\t\t\t\t\"--no-index\" : \"[--no-index]\");\n+\n+\t\t}\n+\t\t/* If this is a no-index diff, just run it and exit there. */\n+\t\tdiff_no_index(&rev, argc, argv, prefix);\n+\t}\n \n \t/* Otherwise, we are doing the usual \"git\" diff */\n \trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 00a8eef..33e5982 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -183,54 +183,12 @@ static int queue_diff(struct diff_options *o,\n \n void diff_no_index(struct rev_info *revs,\n \t\t int argc, const char **argv,\n-\t\t int nongit, const char *prefix)\n+\t\t const char *prefix)\n {\n \tint i, prefixlen;\n-\tint no_index = 0;\n \tunsigned deprecated_show_diff_q_option_used = 0;\n \tconst char *paths[2];\n \n-\t/* Were we asked to do --no-index explicitly? */\n-\tfor (i = 1; i < argc; i++) {\n-\t\tif (!strcmp(argv[i], \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (!strcmp(argv[i], \"--no-index\"))\n-\t\t\tno_index = 1;\n-\t\tif (argv[i][0] != '-')\n-\t\t\tbreak;\n-\t}\n-\n-\tif (!no_index && !nongit) {\n-\t\t/*\n-\t\t * Inside a git repository, without --no-index. Only\n-\t\t * when a path outside the repository is given,\n-\t\t * e.g. \"git diff /var/tmp/[12]\", or \"git diff\n-\t\t * Makefile /var/tmp/Makefile\", allow it to be used as\n-\t\t * a colourful \"diff\" replacement.\n-\t\t */\n-\t\tif ((argc != i + 2) ||\n-\t\t (path_inside_repo(prefix, argv[i]) &&\n-\t\t path_inside_repo(prefix, argv[i+1])))\n-\t\t\treturn;\n-\t}\n-\tif (argc != i + 2) {\n-\t\tif (!no_index) {\n-\t\t\t/*\n-\t\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\t */\n-\t\t\tfprintf(stderr, \"Not a git repository\\n\");\n-\t\t\tfprintf(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 \t\tint j;\ndiff --git a/diff.h b/diff.h\nindex e342325..de105d3 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -330,7 +330,7 @@ extern int diff_flush_patch_id(struct diff_options *, unsigned char *);\n \n extern int diff_result_code(struct diff_options *, int);\n \n-extern void diff_no_index(struct rev_info *, int, const char **, int, const char *);\n+extern void diff_no_index(struct rev_info *, int, const char **, const char *);\n \n extern int index_differs_from(const char *def, int diff_flags);\n \n-- \n1.8.5.4.g8639e57\n"},{"id":"231898","messageId":"1386755923-22132-2-git-send-email-t.gummerer@gmail.com","threadId":"35509","inReplyTo":"1386755923-22132-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v4 2/2] diff: don't read index when --no-index is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-11T09:58:43Z","receivedAt":"2013-12-11T09:58:43Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"git diff --no-index ... currently reads the index, during setup, when\ncalling gitmodules_config(). This results in worse performance when the\nindex is not actually needed. This patch avoids calling\ngitmodules_config() when the --no-index option is given. The times for\nexecuting \"git diff --no-index\" in the WebKit repository are improved as\nfollows:\n\nTest HEAD~3 HEAD\n------------------------------------------------------------------\n4001.1: diff --no-index 0.24(0.15+0.09) 0.01(0.00+0.00) -95.8%\n\nAn additional improvement of this patch is that \"git diff --no-index\" no\nlonger breaks when the index file is corrupt, which makes it possible to\nuse it for investigating the broken repository.\n\nTo improve the possible usage as investigation tool for broken\nrepositories, setup_git_directory_gently() is also not called when the\n--no-index option is given.\n\nAlso add a test to guard against future breakages, and a performance\ntest to show the improvements.\n\nSigned-off-by: Thomas Gummerer \n---\n builtin/diff.c | 7 +++++--\n t/perf/p4001-diff-no-index.sh | 22 ++++++++++++++++++++++\n t/t4053-diff-no-index.sh | 15 +++++++++++++++\n 3 files changed, 42 insertions(+), 2 deletions(-)\n create mode 100755 t/perf/p4001-diff-no-index.sh\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex da69e4a..ea1dd65 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -298,7 +298,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \t}\n \n-\tprefix = setup_git_directory_gently(&nongit);\n+\tif (!no_index)\n+\t\tprefix = setup_git_directory_gently(&nongit);\n+\n \t/*\n \t * Treat git diff with at least one path outside of the\n \t * repo the same as if the command would have been executed\n@@ -311,7 +313,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t!path_inside_repo(prefix, argv[i + 1]))))\n \t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n \n-\tgitmodules_config();\n+\tif (!no_index)\n+\t\tgitmodules_config();\n \tgit_config(git_diff_ui_config, NULL);\n \n \tinit_revisions(&rev, prefix);\ndiff --git a/t/perf/p4001-diff-no-index.sh b/t/perf/p4001-diff-no-index.sh\nnew file mode 100755\nindex 0000000..683be69\n--- /dev/null\n+++ b/t/perf/p4001-diff-no-index.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+\n+test_description=\"Test diff --no-index performance\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+file1=$(git ls-files | tail -n 2 | head -1)\n+file2=$(git ls-files | tail -n 1 | head -1)\n+\n+test_expect_success \"empty files, so they take no time to diff\" \"\n+\techo >$file1 &&\n+\techo >$file2\n+\"\n+\n+test_perf \"diff --no-index\" \"\n+\tgit diff --no-index $file1 $file2 >/dev/null\n+\"\n+\n+test_done\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 979e983..077c775 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -29,4 +29,19 @@ test_expect_success 'git diff --no-index relative path outside repo' '\n \t)\n '\n \n+test_expect_success 'git diff --no-index with broken index' '\n+\t(\n+\t\tcd repo &&\n+\t\techo broken >.git/index &&\n+\t\tgit diff --no-index a ../non/git/a\n+\t)\n+'\n+\n+test_expect_success 'git diff outside repo with broken index' '\n+\t(\n+\t\tcd repo &&\n+\t\tgit diff ../non/git/a ../non/git/b\n+\t)\n+'\n+\n test_done\n-- \n1.8.5.4.g8639e57\n"},{"id":"231960","messageId":"xmqqwqj9stos.fsf@gitster.dls.corp.google.com","threadId":"35509","inReplyTo":"1386755923-22132-2-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v4 2/2] diff: don't read index when --no-index is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-12T20:25:23Z","receivedAt":"2013-12-12T20:25:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> git diff --no-index ... currently reads the index, during setup, when\n> calling gitmodules_config(). This results in worse performance when the\n> index is not actually needed. This patch avoids calling\n> gitmodules_config() when the --no-index option is given. The times for\n> executing \"git diff --no-index\" in the WebKit repository are improved as\n> follows:\n>\n> Test HEAD~3 HEAD\n> ------------------------------------------------------------------\n> 4001.1: diff --no-index 0.24(0.15+0.09) 0.01(0.00+0.00) -95.8%\n>\n> An additional improvement of this patch is that \"git diff --no-index\" no\n> longer breaks when the index file is corrupt, which makes it possible to\n> use it for investigating the broken repository.\n>\n> To improve the possible usage as investigation tool for broken\n> repositories, setup_git_directory_gently() is also not called when the\n> --no-index option is given.\n>\n> Also add a test to guard against future breakages, and a performance\n> test to show the improvements.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n\nLooks reasonable.\n\nThanks. Will queue.\n\n> builtin/diff.c | 7 +++++--\n> t/perf/p4001-diff-no-index.sh | 22 ++++++++++++++++++++++\n> t/t4053-diff-no-index.sh | 15 +++++++++++++++\n> 3 files changed, 42 insertions(+), 2 deletions(-)\n> create mode 100755 t/perf/p4001-diff-no-index.sh\n>\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index da69e4a..ea1dd65 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -298,7 +298,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\t\tbreak;\n> \t}\n> \n> -\tprefix = setup_git_directory_gently(&nongit);\n> +\tif (!no_index)\n> +\t\tprefix = setup_git_directory_gently(&nongit);\n> +\n> \t/*\n> \t * Treat git diff with at least one path outside of the\n> \t * repo the same as if the command would have been executed\n> @@ -311,7 +313,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\t\t!path_inside_repo(prefix, argv[i + 1]))))\n> \t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n> \n> -\tgitmodules_config();\n> +\tif (!no_index)\n> +\t\tgitmodules_config();\n> \tgit_config(git_diff_ui_config, NULL);\n> \n> \tinit_revisions(&rev, prefix);\n> diff --git a/t/perf/p4001-diff-no-index.sh b/t/perf/p4001-diff-no-index.sh\n> new file mode 100755\n> index 0000000..683be69\n> --- /dev/null\n> +++ b/t/perf/p4001-diff-no-index.sh\n> @@ -0,0 +1,22 @@\n> +#!/bin/sh\n> +\n> +test_description=\"Test diff --no-index performance\"\n> +\n> +. ./perf-lib.sh\n> +\n> +test_perf_large_repo\n> +test_checkout_worktree\n> +\n> +file1=$(git ls-files | tail -n 2 | head -1)\n> +file2=$(git ls-files | tail -n 1 | head -1)\n> +\n> +test_expect_success \"empty files, so they take no time to diff\" \"\n> +\techo >$file1 &&\n> +\techo >$file2\n> +\"\n> +\n> +test_perf \"diff --no-index\" \"\n> +\tgit diff --no-index $file1 $file2 >/dev/null\n> +\"\n> +\n> +test_done\n> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n> index 979e983..077c775 100755\n> --- a/t/t4053-diff-no-index.sh\n> +++ b/t/t4053-diff-no-index.sh\n> @@ -29,4 +29,19 @@ test_expect_success 'git diff --no-index relative path outside repo' '\n> \t)\n> '\n> \n> +test_expect_success 'git diff --no-index with broken index' '\n> +\t(\n> +\t\tcd repo &&\n> +\t\techo broken >.git/index &&\n> +\t\tgit diff --no-index a ../non/git/a\n> +\t)\n> +'\n> +\n> +test_expect_success 'git diff outside repo with broken index' '\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit diff ../non/git/a ../non/git/b\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"231985","messageId":"20131214004347.GZ2311@google.com","threadId":"35509","inReplyTo":"1386755923-22132-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v4 1/2] diff: move no-index detection to builtin/diff.c","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-14T00:43:47Z","receivedAt":"2013-12-14T00:43:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nThomas Gummerer wrote:\n\n> Signed-off-by: Thomas Gummerer \n\nThanks, and sorry for the slow follow-up.\n\n[...]\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -16,6 +16,9 @@\n[...]\n> @@ -283,14 +286,57 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t * Other cases are errors.\n> \t */\n> \n> +\t/* Were we asked to do --no-index explicitly? */\n> +\tfor (i = 1; i < argc; i++) {\n> +\t\tif (!strcmp(argv[i], \"--\")) {\n> +\t\t\ti++;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tif (!strcmp(argv[i], \"--no-index\"))\n> +\t\t\tno_index = DIFF_NO_INDEX_EXPLICIT;\n\nNeat.\n\n[...]\n> +\t/*\n> +\t * Treat git diff with at least one path outside of the\n> +\t * repo the same as if the command would have been executed\n> +\t * outside of a git repository. In this case it behaves\n> +\t * the same way as \"git diff --no-index \", which acts\n> +\t * as a colourful \"diff\" replacement.\n> +\t */\n> +\tif (nongit || ((argc == i + 2) &&\n> +\t\t (!path_inside_repo(prefix, argv[i]) ||\n> +\t\t\t!path_inside_repo(prefix, argv[i + 1]))))\n> +\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n\nWhat happens if I run 'git diff --no-index /tmp git.c git.c' from\nwithin a git repository? With this, I fear I will get\n\n\tNot a git repository\n\tTo compare two paths outside a working tree:\n\tusage: git diff [--no-index] \n\ninstead of the expected\n\n\tusage: git diff --no-index \n\nSomething like\n\n\tif (no_index)\n\t\t;\n\telse if (nongit)\n\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n\telse if (argc != i + 2)\n\t\t;\n\telse if (!path_inside_repo(prefix, argv[i]) ||\n\t\t !path_inside_repo(prefix, argv[i + 1]))\n\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n\nshould work. Or:\n\n\tif (!no_index) {\n\t\t/*\n\t\t * Treat git diff with ...\n\t\t */\n\t\tif (nongit || ...)\n\t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n\t}\n\nOr the '!no_index &&' condition inserted some other way.\n\n> -\t/* If this is a no-index diff, just run it and exit there. */\n> -\tdiff_no_index(&rev, argc, argv, nongit, prefix);\n> +\tif (no_index) {\n> +\t\tif (argc != i + 2) {\n[...]\n> +\t\t\t/* Give the usage message for non-repository usage and exit. */\n> +\t\t\tusagef(\"git diff %s \",\n> +\t\t\t no_index == DIFF_NO_INDEX_EXPLICIT ?\n> +\t\t\t\t\t\"--no-index\" : \"[--no-index]\");\n> +\n> +\t\t}\n> +\t\t/* If this is a no-index diff, just run it and exit there. */\n> +\t\tdiff_no_index(&rev, argc, argv, prefix);\n> +\t}\n\nPerhaps, to avoid some nesting:\n\n\t/* A no-index diff takes exactly two path arguments. */\n\tif (no_index && argc != i + 2) {\n\t\t...\n\t\tusagef(...);\n\t}\n\n\tif (no_index)\n\t\t/* Just run the diff and exit. */\n\t\tdiff_no_index(...);\n\nJonathan\n"},{"id":"231986","messageId":"20131214004456.GA2311@google.com","threadId":"35509","inReplyTo":"1386755923-22132-2-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v4 2/2] diff: don't read index when --no-index is given","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-12-14T00:44:56Z","receivedAt":"2013-12-14T00:44:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Thomas Gummerer wrote:\n\n> Also add a test to guard against future breakages, and a performance\n> test to show the improvements.\n\nVery nice.\n\n> Signed-off-by: Thomas Gummerer \n\nReviewed-by: Jonathan Nieder \n"},{"id":"231997","messageId":"87zjo3lnn9.fsf@gmail.com","threadId":"35509","inReplyTo":"20131214004347.GZ2311@google.com","subject":"Re: [PATCH v4 1/2] diff: move no-index detection to builtin/diff.c","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-14T10:42:18Z","receivedAt":"2013-12-14T10:42:18Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Jonathan Nieder writes:\n\n> Hi,\n>\n> Thomas Gummerer wrote:\n>\n>> Signed-off-by: Thomas Gummerer \n>\n> Thanks, and sorry for the slow follow-up.\n>\n> [...]\n>> --- a/builtin/diff.c\n>> +++ b/builtin/diff.c\n>> @@ -16,6 +16,9 @@\n> [...]\n>> @@ -283,14 +286,57 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>> \t * Other cases are errors.\n>> \t */\n>>\n>> +\t/* Were we asked to do --no-index explicitly? */\n>> +\tfor (i = 1; i < argc; i++) {\n>> +\t\tif (!strcmp(argv[i], \"--\")) {\n>> +\t\t\ti++;\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t\tif (!strcmp(argv[i], \"--no-index\"))\n>> +\t\t\tno_index = DIFF_NO_INDEX_EXPLICIT;\n>\n> Neat.\n>\n> [...]\n>> +\t/*\n>> +\t * Treat git diff with at least one path outside of the\n>> +\t * repo the same as if the command would have been executed\n>> +\t * outside of a git repository. In this case it behaves\n>> +\t * the same way as \"git diff --no-index \", which acts\n>> +\t * as a colourful \"diff\" replacement.\n>> +\t */\n>> +\tif (nongit || ((argc == i + 2) &&\n>> +\t\t (!path_inside_repo(prefix, argv[i]) ||\n>> +\t\t\t!path_inside_repo(prefix, argv[i + 1]))))\n>> +\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n>\n> What happens if I run 'git diff --no-index /tmp git.c git.c' from\n> within a git repository? With this, I fear I will get\n\nThanks, I've missed that one. It only happens when run outside a git\nrepository, but the same comments still apply. Will fix and send a\nre-roll.\n\n> \tNot a git repository\n> \tTo compare two paths outside a working tree:\n> \tusage: git diff [--no-index] \n>\n> instead of the expected\n>\n> \tusage: git diff --no-index \n>\n> Something like\n>\n> \tif (no_index)\n> \t\t;\n> \telse if (nongit)\n> \t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n> \telse if (argc != i + 2)\n> \t\t;\n> \telse if (!path_inside_repo(prefix, argv[i]) ||\n> \t\t !path_inside_repo(prefix, argv[i + 1]))\n> \t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n>\n> should work. Or:\n>\n> \tif (!no_index) {\n> \t\t/*\n> \t\t * Treat git diff with ...\n> \t\t */\n> \t\tif (nongit || ...)\n> \t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n> \t}\n>\n> Or the '!no_index &&' condition inserted some other way.\n>\n>> -\t/* If this is a no-index diff, just run it and exit there. */\n>> -\tdiff_no_index(&rev, argc, argv, nongit, prefix);\n>> +\tif (no_index) {\n>> +\t\tif (argc != i + 2) {\n> [...]\n>> +\t\t\t/* Give the usage message for non-repository usage and exit. */\n>> +\t\t\tusagef(\"git diff %s \",\n>> +\t\t\t no_index == DIFF_NO_INDEX_EXPLICIT ?\n>> +\t\t\t\t\t\"--no-index\" : \"[--no-index]\");\n>> +\n>> +\t\t}\n>> +\t\t/* If this is a no-index diff, just run it and exit there. */\n>> +\t\tdiff_no_index(&rev, argc, argv, prefix);\n>> +\t}\n>\n> Perhaps, to avoid some nesting:\n>\n> \t/* A no-index diff takes exactly two path arguments. */\n> \tif (no_index && argc != i + 2) {\n> \t\t...\n> \t\tusagef(...);\n> \t}\n>\n> \tif (no_index)\n> \t\t/* Just run the diff and exit. */\n> \t\tdiff_no_index(...);\n>\n> Jonathan\n\nThanks, will change in the re-roll.\n\n--\nThomas\n"},{"id":"232022","messageId":"1387026441-14539-1-git-send-email-t.gummerer@gmail.com","threadId":"35509","inReplyTo":"20131214004347.GZ2311@google.com","subject":"[PATCH v5 1/2] diff: move no-index detection to builtin/diff.c","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-14T13:07:20Z","receivedAt":"2013-12-14T13:07:20Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Currently the --no-index option is parsed in diff_no_index(). Move the\ndetection if a no-index diff should be executed to builtin/diff.c, where\nwe can use it for executing diff_no_index() conditionally. This will\nalso allow us to execute other operations conditionally, which will be\ndone in the next patch.\n\nThere are no functional changes.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Thomas Gummerer \n---\n\nThanks Jonathan for reviewing the last round, this version addresses\nthose comments, fixing the error message when git diff --no-index is\nrun outside of a git directory and avoids some nesting as suggested.\n\n builtin/diff.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++---\n diff-no-index.c | 44 +-------------------------------------------\n diff.h | 2 +-\n 3 files changed, 52 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex adb93a9..f49a938 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -16,6 +16,9 @@\n #include \"submodule.h\"\n #include \"sha1-array.h\"\n \n+#define DIFF_NO_INDEX_EXPLICIT 1\n+#define DIFF_NO_INDEX_IMPLICIT 2\n+\n struct blobinfo {\n \tunsigned char sha1[20];\n \tconst char *name;\n@@ -257,7 +260,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint blobs = 0, paths = 0;\n \tconst char *path = NULL;\n \tstruct blobinfo blob[2];\n-\tint nongit;\n+\tint nongit = 0, no_index = 0;\n \tint result = 0;\n \n \t/*\n@@ -283,14 +286,58 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t * Other cases are errors.\n \t */\n \n+\t/* Were we asked to do --no-index explicitly? */\n+\tfor (i = 1; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\ti++;\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!strcmp(argv[i], \"--no-index\"))\n+\t\t\tno_index = DIFF_NO_INDEX_EXPLICIT;\n+\t\tif (argv[i][0] != '-')\n+\t\t\tbreak;\n+\t}\n+\n \tprefix = setup_git_directory_gently(&nongit);\n+\tif (!no_index) {\n+\t\t/*\n+\t\t * Treat git diff with at least one path outside of the\n+\t\t * repo the same as if the command would have been executed\n+\t\t * outside of a git repository. In this case it behaves\n+\t\t * the same way as \"git diff --no-index \", which acts\n+\t\t * as a colourful \"diff\" replacement.\n+\t\t */\n+\t\tif (nongit || ((argc == i + 2) &&\n+\t\t\t (!path_inside_repo(prefix, argv[i]) ||\n+\t\t\t\t!path_inside_repo(prefix, argv[i + 1]))))\n+\t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n+\t}\n+\n \tgitmodules_config();\n \tgit_config(git_diff_ui_config, NULL);\n \n \tinit_revisions(&rev, prefix);\n \n-\t/* If this is a no-index diff, just run it and exit there. */\n-\tdiff_no_index(&rev, argc, argv, nongit, prefix);\n+\tif (no_index && argc != i + 2) {\n+\t\tif (no_index == DIFF_NO_INDEX_IMPLICIT) {\n+\t\t\t/*\n+\t\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\t */\n+\t\t\tfprintf(stderr, \"Not a git repository\\n\");\n+\t\t\tfprintf(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 == DIFF_NO_INDEX_EXPLICIT ?\n+\t\t \"--no-index\" : \"[--no-index]\");\n+\n+\t}\n+\tif (no_index)\n+\t\t/* If this is a no-index diff, just run it and exit there. */\n+\t\tdiff_no_index(&rev, argc, argv, prefix);\n \n \t/* Otherwise, we are doing the usual \"git\" diff */\n \trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 00a8eef..33e5982 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -183,54 +183,12 @@ static int queue_diff(struct diff_options *o,\n \n void diff_no_index(struct rev_info *revs,\n \t\t int argc, const char **argv,\n-\t\t int nongit, const char *prefix)\n+\t\t const char *prefix)\n {\n \tint i, prefixlen;\n-\tint no_index = 0;\n \tunsigned deprecated_show_diff_q_option_used = 0;\n \tconst char *paths[2];\n \n-\t/* Were we asked to do --no-index explicitly? */\n-\tfor (i = 1; i < argc; i++) {\n-\t\tif (!strcmp(argv[i], \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (!strcmp(argv[i], \"--no-index\"))\n-\t\t\tno_index = 1;\n-\t\tif (argv[i][0] != '-')\n-\t\t\tbreak;\n-\t}\n-\n-\tif (!no_index && !nongit) {\n-\t\t/*\n-\t\t * Inside a git repository, without --no-index. Only\n-\t\t * when a path outside the repository is given,\n-\t\t * e.g. \"git diff /var/tmp/[12]\", or \"git diff\n-\t\t * Makefile /var/tmp/Makefile\", allow it to be used as\n-\t\t * a colourful \"diff\" replacement.\n-\t\t */\n-\t\tif ((argc != i + 2) ||\n-\t\t (path_inside_repo(prefix, argv[i]) &&\n-\t\t path_inside_repo(prefix, argv[i+1])))\n-\t\t\treturn;\n-\t}\n-\tif (argc != i + 2) {\n-\t\tif (!no_index) {\n-\t\t\t/*\n-\t\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\t */\n-\t\t\tfprintf(stderr, \"Not a git repository\\n\");\n-\t\t\tfprintf(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 \t\tint j;\ndiff --git a/diff.h b/diff.h\nindex e342325..de105d3 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -330,7 +330,7 @@ extern int diff_flush_patch_id(struct diff_options *, unsigned char *);\n \n extern int diff_result_code(struct diff_options *, int);\n \n-extern void diff_no_index(struct rev_info *, int, const char **, int, const char *);\n+extern void diff_no_index(struct rev_info *, int, const char **, const char *);\n \n extern int index_differs_from(const char *def, int diff_flags);\n \n-- \n1.8.5.4.g8639e57\n"},{"id":"232023","messageId":"1387026441-14539-2-git-send-email-t.gummerer@gmail.com","threadId":"35509","inReplyTo":"1387026441-14539-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v5 2/2] diff: don't read index when --no-index is given","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-14T13:07:21Z","receivedAt":"2013-12-14T13:07:21Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"git diff --no-index ... currently reads the index, during setup, when\ncalling gitmodules_config(). This results in worse performance when the\nindex is not actually needed. This patch avoids calling\ngitmodules_config() when the --no-index option is given. The times for\nexecuting \"git diff --no-index\" in the WebKit repository are improved as\nfollows:\n\nTest HEAD~3 HEAD\n------------------------------------------------------------------\n4001.1: diff --no-index 0.24(0.15+0.09) 0.01(0.00+0.00) -95.8%\n\nAn additional improvement of this patch is that \"git diff --no-index\" no\nlonger breaks when the index file is corrupt, which makes it possible to\nuse it for investigating the broken repository.\n\nTo improve the possible usage as investigation tool for broken\nrepositories, setup_git_directory_gently() is also not called when the\n--no-index option is given.\n\nAlso add a test to guard against future breakages, and a performance\ntest to show the improvements.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Thomas Gummerer \n---\n builtin/diff.c | 5 +++--\n t/perf/p4001-diff-no-index.sh | 22 ++++++++++++++++++++++\n t/t4053-diff-no-index.sh | 15 +++++++++++++++\n 3 files changed, 40 insertions(+), 2 deletions(-)\n create mode 100755 t/perf/p4001-diff-no-index.sh\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex f49a938..a8569a4 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -298,8 +298,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \t}\n \n-\tprefix = setup_git_directory_gently(&nongit);\n \tif (!no_index) {\n+\t\tprefix = setup_git_directory_gently(&nongit);\n \t\t/*\n \t\t * Treat git diff with at least one path outside of the\n \t\t * repo the same as if the command would have been executed\n@@ -313,7 +313,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n \t}\n \n-\tgitmodules_config();\n+\tif (!no_index)\n+\t\tgitmodules_config();\n \tgit_config(git_diff_ui_config, NULL);\n \n \tinit_revisions(&rev, prefix);\ndiff --git a/t/perf/p4001-diff-no-index.sh b/t/perf/p4001-diff-no-index.sh\nnew file mode 100755\nindex 0000000..683be69\n--- /dev/null\n+++ b/t/perf/p4001-diff-no-index.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+\n+test_description=\"Test diff --no-index performance\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+file1=$(git ls-files | tail -n 2 | head -1)\n+file2=$(git ls-files | tail -n 1 | head -1)\n+\n+test_expect_success \"empty files, so they take no time to diff\" \"\n+\techo >$file1 &&\n+\techo >$file2\n+\"\n+\n+test_perf \"diff --no-index\" \"\n+\tgit diff --no-index $file1 $file2 >/dev/null\n+\"\n+\n+test_done\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 979e983..077c775 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -29,4 +29,19 @@ test_expect_success 'git diff --no-index relative path outside repo' '\n \t)\n '\n \n+test_expect_success 'git diff --no-index with broken index' '\n+\t(\n+\t\tcd repo &&\n+\t\techo broken >.git/index &&\n+\t\tgit diff --no-index a ../non/git/a\n+\t)\n+'\n+\n+test_expect_success 'git diff outside repo with broken index' '\n+\t(\n+\t\tcd repo &&\n+\t\tgit diff ../non/git/a ../non/git/b\n+\t)\n+'\n+\n test_done\n-- \n1.8.5.4.g8639e57\n"},{"id":"232057","messageId":"xmqqtxe8pu05.fsf@gitster.dls.corp.google.com","threadId":"35509","inReplyTo":"87zjo3lnn9.fsf@gmail.com","subject":"Re: [PATCH v4 1/2] diff: move no-index detection to builtin/diff.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-16T17:48:10Z","receivedAt":"2013-12-16T17:48:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n>> What happens if I run 'git diff --no-index /tmp git.c git.c' from\n>> within a git repository? With this, I fear I will get\n>\n> Thanks, I've missed that one. It only happens when run outside a git\n> repository, but the same comments still apply. Will fix and send a\n> re-roll.\n\nPlease don't, as the last round has already been pushed on 'next'.\n\nAn incremental change on top would also illustrate more clearly what\nbreakage needed to be fixed, which would be another good thing. It\ncould even come with a new test that makes sure that the above\ncommand line is diagnosed correctly as a mistake ;-).\n\nThanks.\n\n>\n>> \tNot a git repository\n>> \tTo compare two paths outside a working tree:\n>> \tusage: git diff [--no-index] \n>>\n>> instead of the expected\n>>\n>> \tusage: git diff --no-index \n>>\n>> Something like\n>>\n>> \tif (no_index)\n>> \t\t;\n>> \telse if (nongit)\n>> \t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n>> \telse if (argc != i + 2)\n>> \t\t;\n>> \telse if (!path_inside_repo(prefix, argv[i]) ||\n>> \t\t !path_inside_repo(prefix, argv[i + 1]))\n>> \t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n>>\n>> should work. Or:\n>>\n>> \tif (!no_index) {\n>> \t\t/*\n>> \t\t * Treat git diff with ...\n>> \t\t */\n>> \t\tif (nongit || ...)\n>> \t\t\tno_index = DIFF_NO_INDEX_IMPLICIT;\n>> \t}\n>>\n>> Or the '!no_index &&' condition inserted some other way.\n>>\n>>> -\t/* If this is a no-index diff, just run it and exit there. */\n>>> -\tdiff_no_index(&rev, argc, argv, nongit, prefix);\n>>> +\tif (no_index) {\n>>> +\t\tif (argc != i + 2) {\n>> [...]\n>>> +\t\t\t/* Give the usage message for non-repository usage and exit. */\n>>> +\t\t\tusagef(\"git diff %s \",\n>>> +\t\t\t no_index == DIFF_NO_INDEX_EXPLICIT ?\n>>> +\t\t\t\t\t\"--no-index\" : \"[--no-index]\");\n>>> +\n>>> +\t\t}\n>>> +\t\t/* If this is a no-index diff, just run it and exit there. */\n>>> +\t\tdiff_no_index(&rev, argc, argv, prefix);\n>>> +\t}\n>>\n>> Perhaps, to avoid some nesting:\n>>\n>> \t/* A no-index diff takes exactly two path arguments. */\n>> \tif (no_index && argc != i + 2) {\n>> \t\t...\n>> \t\tusagef(...);\n>> \t}\n>>\n>> \tif (no_index)\n>> \t\t/* Just run the diff and exit. */\n>> \t\tdiff_no_index(...);\n>>\n>> Jonathan\n>\n> Thanks, will change in the re-roll.\n>\n> --\n> Thomas\n"},{"id":"232067","messageId":"1387221810-32374-1-git-send-email-t.gummerer@gmail.com","threadId":"35509","inReplyTo":"xmqqtxe8pu05.fsf@gitster.dls.corp.google.com","subject":"[PATCH 1/2] diff: add test for --no-index executed outside repo","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-16T19:23:29Z","receivedAt":"2013-12-16T19:23:29Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"470faf9 diff: move no-index detection to builtin/diff.c breaks the error\nmessage for \"git diff --no-index\", when the command is executed outside\nof a git repository and the wrong number of arguments are given. 6df5762\ndiff: don't read index when --no-index is given fixes the problem.\n\nAdd a test to guard against similar breakages in the future.\n\nSigned-off-by: Thomas Gummerer \n---\n\n>> Thanks, I've missed that one. It only happens when run outside a git\n>> repository, but the same comments still apply. Will fix and send a\n>> re-roll.\n>\n> Please don't, as the last round has already been pushed on 'next'.\n\nSorry about that, should have checked first.\n\n> An incremental change on top would also illustrate more clearly what\n> breakage needed to be fixed, which would be another good thing. It\n> could even come with a new test that makes sure that the above\n> command line is diagnosed correctly as a mistake ;-).\n\nThe breakage is actually fixed with the second patch as described in\nthe commit message above, so here is just a test against future\nbreakages. This test only works when the test root is outside of a\ngit repository, as otherwise nongit will not be set. Is there another\nway to write it?\n\nt/t4053-diff-no-index.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 077c775..eb4f380 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -44,4 +44,11 @@ test_expect_success 'git diff outside repo with broken index' '\n \t)\n '\n \n+test_expect_success 'git diff --no-index executed outside repo gives correct error message' '\n+\trm -rf .git &&\n+\ttest_must_fail git diff --no-index a b b 2>actual.err &&\n+\techo \"usage: git diff --no-index \" >expect.err &&\n+\ttest_cmp expect.err actual.err\n+'\n+\n test_done\n-- \n1.8.5.4.g8639e57\n"},{"id":"232068","messageId":"1387221810-32374-2-git-send-email-t.gummerer@gmail.com","threadId":"35509","inReplyTo":"1387221810-32374-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH 2/2] diff: avoid some nesting","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2013-12-16T19:23:30Z","receivedAt":"2013-12-16T19:23:30Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Avoid some nesting in builtin/diff.c, to make the code easier to\nread. No functional changes.\n\nHelped-by: Jonathan Nieder \nSigned-off-by: Thomas Gummerer \n---\n\nThis is based on comments by Jonathan on the version that is already\nnext.\n\nbuiltin/diff.c | 35 +++++++++++++++++------------------\n 1 file changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex ea1dd65..24d6271 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -319,27 +319,26 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \n \tinit_revisions(&rev, prefix);\n \n-\tif (no_index) {\n-\t\tif (argc != i + 2) {\n-\t\t\tif (no_index == DIFF_NO_INDEX_IMPLICIT) {\n-\t\t\t\t/*\n-\t\t\t\t * There was no --no-index and there were not two\n-\t\t\t\t * paths. It is possible that the user intended\n-\t\t\t\t * to do an inside-repository operation.\n-\t\t\t\t */\n-\t\t\t\tfprintf(stderr, \"Not a git repository\\n\");\n-\t\t\t\tfprintf(stderr,\n-\t\t\t\t\t\"To compare two paths outside a working tree:\\n\");\n-\t\t\t}\n-\t\t\t/* Give the usage message for non-repository usage and exit. */\n-\t\t\tusagef(\"git diff %s \",\n-\t\t\t no_index == DIFF_NO_INDEX_EXPLICIT ?\n-\t\t\t\t\t\"--no-index\" : \"[--no-index]\");\n-\n+\tif (no_index && argc != i + 2) {\n+\t\tif (no_index == DIFF_NO_INDEX_IMPLICIT) {\n+\t\t\t/*\n+\t\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\t */\n+\t\t\tfprintf(stderr, \"Not a git repository\\n\");\n+\t\t\tfprintf(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 == DIFF_NO_INDEX_EXPLICIT ?\n+\t\t \"--no-index\" : \"[--no-index]\");\n+\n+\t}\n+\tif (no_index)\n \t\t/* If this is a no-index diff, just run it and exit there. */\n \t\tdiff_no_index(&rev, argc, argv, prefix);\n-\t}\n \n \t/* Otherwise, we are doing the usual \"git\" diff */\n \trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n-- \n1.8.5.4.g8639e57\n"},{"id":"232073","messageId":"xmqqob4goa4t.fsf@gitster.dls.corp.google.com","threadId":"35509","inReplyTo":"1387221810-32374-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH 1/2] diff: add test for --no-index executed outside repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-16T19:42:42Z","receivedAt":"2013-12-16T19:42:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> 470faf9 diff: move no-index detection to builtin/diff.c breaks the error\n> message for \"git diff --no-index\", when the command is executed outside\n> of a git repository and the wrong number of arguments are given. 6df5762\n> diff: don't read index when --no-index is given fixes the problem.\n>\n> Add a test to guard against similar breakages in the future.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n>\n>>> Thanks, I've missed that one. It only happens when run outside a git\n>>> repository, but the same comments still apply. Will fix and send a\n>>> re-roll.\n>>\n>> Please don't, as the last round has already been pushed on 'next'.\n>\n> Sorry about that, should have checked first.\n>\n>> An incremental change on top would also illustrate more clearly what\n>> breakage needed to be fixed, which would be another good thing. It\n>> could even come with a new test that makes sure that the above\n>> command line is diagnosed correctly as a mistake ;-).\n>\n> The breakage is actually fixed with the second patch as described in\n> the commit message above, so here is just a test against future\n> breakages. This test only works when the test root is outside of a\n> git repository, as otherwise nongit will not be set. Is there another\n> way to write it?\n\nPerhaps use CEILING, like this (untested)?\n\n\tmkdir -p test-outside/non/git &&\n (\n GIT_CEILING_DIRECTORIES=$TRASH_DIRECTORY/test-outside &&\n\t\texport GIT_CEILING_DIRECTORIES &&\n\t\tcd test-outside/non/git &&\n\t\tdo whatever non-git thing here\n\t)\n"}]}