{"thread":{"id":"30833","subject":"[PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","startedAt":"2012-06-18T19:28:24Z","lastAt":"2012-06-21T16:55:12Z","messageCount":13,"participants":["Tim Henigan","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"193798","messageId":"1340047704-8752-1-git-send-email-tim.henigan@gmail.com","threadId":"30833","inReplyTo":null,"subject":"[PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-06-18T19:28:24Z","receivedAt":"2012-06-18T19:28:24Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"When running 'git diff --quiet ', if file1 or file2\nis outside the repository, it will exit(0) even if the files differ.\nIt should exit(1) when they differ.\n\nSigned-off-by: Tim Henigan \n---\n\n\nv3 improves the test coverage to include variations of 'diff --quiet'\nwhere one or both of the files are outside the repository. Tests for\n'--ignore-space-at-eol' and '--ignore-all-space' are included as well.\n\n\n diff-no-index.c | 3 +-\n t/t4035-diff-quiet.sh | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 80 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex f0b0010..b935d2a 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -273,5 +273,6 @@ void diff_no_index(struct rev_info *revs,\n \t * The return code for --no-index imitates diff(1):\n \t * 0 = no changes, 1 = changes, else error\n \t */\n-\texit(revs->diffopt.found_changes);\n+\tint result = diff_result_code(&revs->diffopt, 0);\n+\texit(result);\n }\ndiff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh\nindex cdb9202..33d8980 100755\n--- a/t/t4035-diff-quiet.sh\n+++ b/t/t4035-diff-quiet.sh\n@@ -10,7 +10,22 @@ test_expect_success 'setup' '\n \tgit commit -m first &&\n \techo 2 >b &&\n \tgit add . &&\n-\tgit commit -a -m second\n+\tgit commit -a -m second &&\n+\tmkdir -p test-outside/repo && (\n+\t\tcd test-outside/repo &&\n+\t\tgit init &&\n+\t\techo \"1 1\" > a &&\n+\t\tgit add . &&\n+\t\tgit commit -m 1\n+\t) &&\n+\tmkdir -p test-outside/no-repo && (\n+\t\tcd test-outside/no-repo &&\n+\t\techo \"1 1\" >a &&\n+\t\techo \"1 1\" >matching-file &&\n+\t\techo \"1 1 \" >trailing-space &&\n+\t\techo \"1 1\" >extra-space &&\n+\t\techo \"2\" >never-match\n+\t)\n '\n \n test_expect_success 'git diff-tree HEAD^ HEAD' '\n@@ -77,4 +92,66 @@ test_expect_success 'git diff-index --cached HEAD' '\n \t}\n '\n \n+test_expect_success 'git diff, one file outside repo' '\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd test-outside/repo &&\n+\t\ttest_expect_code 0 git diff --quiet a \"$TRASH_DIRECTORY/test-outside/no-repo/matching-file\" &&\n+\t\ttest_expect_code 1 git diff --quiet a \"$TRASH_DIRECTORY/test-outside/no-repo/extra-space\"\n+\t)\n+'\n+\n+test_expect_success 'git diff, both files outside repo' '\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd test-outside/no-repo &&\n+\t\ttest_expect_code 0 git diff --quiet a matching-file &&\n+\t\ttest_expect_code 1 git diff --quiet a extra-space\n+\t)\n+'\n+\n+test_expect_success 'git diff --ignore-space-at-eol, one file outside repo' '\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd test-outside/repo &&\n+\t\ttest_expect_code 0 git diff --quiet --ignore-space-at-eol a \"$TRASH_DIRECTORY/test-outside/no-repo/trailing-space\" &&\n+\t\ttest_expect_code 1 git diff --quiet --ignore-space-at-eol a \"$TRASH_DIRECTORY/test-outside/no-repo/extra-space\"\n+\t)\n+'\n+\n+test_expect_success 'git diff --ignore-space-at-eol, both files outside repo' '\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd test-outside/no-repo &&\n+\t\ttest_expect_code 0 git diff --quiet --ignore-space-at-eol a trailing-space &&\n+\t\ttest_expect_code 1 git diff --quiet --ignore-space-at-eol a extra-space\n+\t)\n+'\n+\n+test_expect_success 'git diff --ignore-all-space, one file outside repo' '\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd test-outside/repo &&\n+\t\ttest_expect_code 0 git diff --quiet --ignore-all-space a \"$TRASH_DIRECTORY/test-outside/no-repo/trailing-space\" &&\n+\t\ttest_expect_code 0 git diff --quiet --ignore-all-space a \"$TRASH_DIRECTORY/test-outside/no-repo/extra-space\" &&\n+\t\ttest_expect_code 1 git diff --quiet --ignore-all-space a \"$TRASH_DIRECTORY/test-outside/no-repo/never-match\"\n+\t)\n+'\n+\n+test_expect_success 'git diff --ignore-all-space, both files outside repo' '\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\tcd test-outside/no-repo &&\n+\t\ttest_expect_code 0 git diff --quiet --ignore-all-space a trailing-space &&\n+\t\ttest_expect_code 0 git diff --quiet --ignore-all-space a extra-space &&\n+\t\ttest_expect_code 1 git diff --quiet --ignore-all-space a never-match\n+\t)\n+'\n+\n test_done\n-- \n1.7.11.rc3.6.g5532165\n"},{"id":"193799","messageId":"20120618194530.GA10725@sigill.intra.peff.net","threadId":"30833","inReplyTo":"1340047704-8752-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-18T19:45:30Z","receivedAt":"2012-06-18T19:45:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 18, 2012 at 03:28:24PM -0400, Tim Henigan wrote:\n\n> When running 'git diff --quiet ', if file1 or file2\n> is outside the repository, it will exit(0) even if the files differ.\n> It should exit(1) when they differ.\n\nCan we explain in the commit message a bit about why the patch works? If\nI hadn't just dug into this a few days ago, the patch would be somewhat\nconfusing. Maybe something like:\n\n The problem comes from checking diff_options's found_changes member to\n see whether any changes were found. This flag is set only when we\n actually run xdiff and it finds a change. However, the diff machinery\n will optimize out the actual xdiff call when it is not necessary\n (i.e., when we are doing a straight byte-for-byte comparison and do\n not care about the output). As a result, this flag was never set, and\n we must check the HAS_CHANGES flag instead, just like the regular\n index-aware diff code paths do.\n\nI am also tempted to suggest that found_changes be renamed to\nxdiff_found_changes or something similar, because it really is quite\nmisleading as-is.\n\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index f0b0010..b935d2a 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -273,5 +273,6 @@ void diff_no_index(struct rev_info *revs,\n> \t * The return code for --no-index imitates diff(1):\n> \t * 0 = no changes, 1 = changes, else error\n> \t */\n> -\texit(revs->diffopt.found_changes);\n> +\tint result = diff_result_code(&revs->diffopt, 0);\n> +\texit(result);\n\nThis is a declaration-after-statement, no? We try to stick to C89, which\ndoes not allow this.\n\nI don't see any reason why the extra variable could not be removed\nentirely:\n\n exit(diff_result_code(&revs->diffopt, 0));\n\n-Peff\n"},{"id":"193801","messageId":"7vr4tc2xhy.fsf@alter.siamese.dyndns.org","threadId":"30833","inReplyTo":"1340047704-8752-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-18T20:09:13Z","receivedAt":"2012-06-18T20:09:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> When running 'git diff --quiet ', if file1 or file2\n> is outside the repository, it will exit(0) even if the files differ.\n> It should exit(1) when they differ.\n>\n> Signed-off-by: Tim Henigan \n> ---\n>\n>\n> v3 improves the test coverage to include variations of 'diff --quiet'\n> where one or both of the files are outside the repository. Tests for\n> '--ignore-space-at-eol' and '--ignore-all-space' are included as well.\n>\n>\n> diff-no-index.c | 3 +-\n> t/t4035-diff-quiet.sh | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 2 files changed, 80 insertions(+), 2 deletions(-)\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index f0b0010..b935d2a 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -273,5 +273,6 @@ void diff_no_index(struct rev_info *revs,\n> \t * The return code for --no-index imitates diff(1):\n> \t * 0 = no changes, 1 = changes, else error\n> \t */\n> -\texit(revs->diffopt.found_changes);\n> +\tint result = diff_result_code(&revs->diffopt, 0);\n> +\texit(result);\n> }\n\nDecl-after-stmt.\n\n> diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh\n> index cdb9202..33d8980 100755\n> --- a/t/t4035-diff-quiet.sh\n> +++ b/t/t4035-diff-quiet.sh\n> @@ -10,7 +10,22 @@ test_expect_success 'setup' '\n> \tgit commit -m first &&\n> \techo 2 >b &&\n> \tgit add . &&\n> -\tgit commit -a -m second\n> +\tgit commit -a -m second &&\n> +\tmkdir -p test-outside/repo && (\n> +\t\tcd test-outside/repo &&\n> +\t\tgit init &&\n> +\t\techo \"1 1\" > a &&\n\nPlease drop extra SP between \">\" and \"a\".\n\n> +\t\tgit add . &&\n> +\t\tgit commit -m 1\n> +\t) &&\n> +\tmkdir -p test-outside/no-repo && (\n> +\t\tcd test-outside/no-repo &&\n> +\t\techo \"1 1\" >a &&\n> +\t\techo \"1 1\" >matching-file &&\n> +\t\techo \"1 1 \" >trailing-space &&\n> +\t\techo \"1 1\" >extra-space &&\n> +\t\techo \"2\" >never-match\n> +\t)\n\nThe inspiration of using CEILING comes from the existing t7810-grep\ntest, and I would have preferred if you used the same non/git for a\nnon-git repository for easier greppability (\"git grep CEIL t/\" to\nnotice the use of the technique and then \"git grep non/git t/\" to\nverify, for example).\n\n> '\n> \n> test_expect_success 'git diff-tree HEAD^ HEAD' '\n> @@ -77,4 +92,66 @@ test_expect_success 'git diff-index --cached HEAD' '\n> \t}\n> '\n> \n> +test_expect_success 'git diff, one file outside repo' '\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n\nDo you even need these two lines for this test? your test runs\ninside test-outside/repo that _is_ a git repository, and that\nrepository knows that ../no-repo is not part of it already.\n\n> +\t\tcd test-outside/repo &&\n> +\t\ttest_expect_code 0 git diff --quiet a \"$TRASH_DIRECTORY/test-outside/no-repo/matching-file\" &&\n> +\t\ttest_expect_code 1 git diff --quiet a \"$TRASH_DIRECTORY/test-outside/no-repo/extra-space\"\n> +\t)\n> +'\n> +\n> +test_expect_success 'git diff, both files outside repo' '\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd test-outside/no-repo &&\n> +\t\ttest_expect_code 0 git diff --quiet a matching-file &&\n> +\t\ttest_expect_code 1 git diff --quiet a extra-space\n\nThis one does need the ceiling to prevent git from finding the trash\ndirectory.\n\n> +\t)\n> +'\n> +\n> +test_expect_success 'git diff --ignore-space-at-eol, one file outside repo' '\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n\nThis one does not, I think (please correct me; I am not being very careful).\n\n> +\t\tcd test-outside/repo &&\n> +\t\ttest_expect_code 0 git diff --quiet --ignore-space-at-eol a \"$TRASH_DIRECTORY/test-outside/no-repo/trailing-space\" &&\n> +\t\ttest_expect_code 1 git diff --quiet --ignore-space-at-eol a \"$TRASH_DIRECTORY/test-outside/no-repo/extra-space\"\n> +\t)\n> +'\n> +\n> +test_expect_success 'git diff --ignore-space-at-eol, both files outside repo' '\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd test-outside/no-repo &&\n> +\t\ttest_expect_code 0 git diff --quiet --ignore-space-at-eol a trailing-space &&\n> +\t\ttest_expect_code 1 git diff --quiet --ignore-space-at-eol a extra-space\n> +\t)\n> +'\n> +\n> +test_expect_success 'git diff --ignore-all-space, one file outside repo' '\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n\nThis one does not, I think (please correct me; I am not being very careful).\n\n> +\t\tcd test-outside/repo &&\n> +\t\ttest_expect_code 0 git diff --quiet --ignore-all-space a \"$TRASH_DIRECTORY/test-outside/no-repo/trailing-space\" &&\n> +\t\ttest_expect_code 0 git diff --quiet --ignore-all-space a \"$TRASH_DIRECTORY/test-outside/no-repo/extra-space\" &&\n> +\t\ttest_expect_code 1 git diff --quiet --ignore-all-space a \"$TRASH_DIRECTORY/test-outside/no-repo/never-match\"\n> +\t)\n> +'\n> +\n> +test_expect_success 'git diff --ignore-all-space, both files outside repo' '\n> +\t(\n> +\t\tGIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n> +\t\texport GIT_CEILING_DIRECTORIES &&\n> +\t\tcd test-outside/no-repo &&\n> +\t\ttest_expect_code 0 git diff --quiet --ignore-all-space a trailing-space &&\n> +\t\ttest_expect_code 0 git diff --quiet --ignore-all-space a extra-space &&\n> +\t\ttest_expect_code 1 git diff --quiet --ignore-all-space a never-match\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"193836","messageId":"CAFouethcrw3vOF7SPwHxjH4ABmF8U1df0MfyzcUGq2yTYxs4ow@mail.gmail.com","threadId":"30833","inReplyTo":"7vr4tc2xhy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-06-19T13:05:40Z","receivedAt":"2012-06-19T13:05:40Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Mon, Jun 18, 2012 at 4:09 PM, Junio C Hamano wrote:\n> Tim Henigan writes:\n>\n>> When running 'git diff --quiet ', if file1 or file2\n>> is outside the repository, it will exit(0) even if the files differ.\n>> It should exit(1) when they differ.\n>> -     exit(revs->diffopt.found_changes);\n>> +     int result = diff_result_code(&revs->diffopt, 0);\n>> +     exit(result);\n>>  }\n>\n> Decl-after-stmt.\n\nWill eliminate intermediate variable in v4. Thanks to both you and\nJeff for pointing this out.\n\n\n>> +             echo \"1 1\" > a &&\n>\n> Please drop extra SP between \">\" and \"a\".\n\nWill fix in v4.\n\n\n>> +             git add . &&\n>> +             git commit -m 1\n>> +     ) &&\n>> +     mkdir -p test-outside/no-repo && (\n>> +             cd test-outside/no-repo &&\n>> +             echo \"1 1\" >a &&\n>> +             echo \"1 1\" >matching-file &&\n>> +             echo \"1 1 \" >trailing-space &&\n>> +             echo \"1   1\" >extra-space &&\n>> +             echo \"2\" >never-match\n>> +     )\n>\n> The inspiration of using CEILING comes from the existing t7810-grep\n> test, and I would have preferred if you used the same non/git for a\n> non-git repository for easier greppability (\"git grep CEIL t/\" to\n> notice the use of the technique and then \"git grep non/git t/\" to\n> verify, for example).\n\nOkay. I still need the non/git directory to be outside the test\nrepo's path, so the new layout will be:\n\n $TRASH_DIRECTORY/\n test-outside/\n repo/\n non/git/\n\nThis adds an extra layer to the non git paths, but won't cause any problems.\n\n\n>> +test_expect_success 'git diff, one file outside repo' '\n>> +     (\n>> +             GIT_CEILING_DIRECTORIES=\"$TRASH_DIRECTORY/test-outside\" &&\n>> +             export GIT_CEILING_DIRECTORIES &&\n>\n> Do you even need these two lines for this test?  your test runs\n> inside test-outside/repo that _is_ a git repository, and that\n> repository knows that ../no-repo is not part of it already.\n\nYou are correct, CEILING does not need to be set for tests where one\nfile is inside 'test-outside/repo'.\n\nAs a side note, I found that these tests fail if a relative path is\nused for the file in 'non/git'. In other words, this passes:\n\n test_expect_code 0 git diff --quiet a\n\"$TRASH_DIRECTORY/test-outside/non/git/matching-file\"\n\nbut this fails:\n\n test_expect_code 0 git diff --quiet a ../non/git/matching-file\n\nThis surprised me, but I have not investigated any further.\n"},{"id":"193837","messageId":"20120619135814.GA3210@sigill.intra.peff.net","threadId":"30833","inReplyTo":"CAFouethcrw3vOF7SPwHxjH4ABmF8U1df0MfyzcUGq2yTYxs4ow@mail.gmail.com","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-19T13:58:15Z","receivedAt":"2012-06-19T13:58:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 19, 2012 at 09:05:40AM -0400, Tim Henigan wrote:\n\n> As a side note, I found that these tests fail if a relative path is\n> used for the file in 'non/git'. In other words, this passes:\n> \n> test_expect_code 0 git diff --quiet a\n> \"$TRASH_DIRECTORY/test-outside/non/git/matching-file\"\n> \n> but this fails:\n> \n> test_expect_code 0 git diff --quiet a ../non/git/matching-file\n> \n> This surprised me, but I have not investigated any further.\n\nThe problem is that path_outside_repo in diff-no-index.c does not bother\nhandling relative paths at all, and just assumes they are inside the\nrepository. This is obviously not true if the path starts with \"..\", in\nwhich case you would need to compare the number of \"..\" with the current\ndepth in the repository.\n\nprefix_path already does this (and is what generates the later\n\"../non/git/matching-file is not in the repository\" message). We could\nperhaps get rid of path_outside_repo and just re-use prefix_path's\nlogic, something like (not tested):\n\ndiff --git a/cache.h b/cache.h\nindex 0b7ddee..0736bfb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -411,6 +411,7 @@ extern const char *prefix_filename(const char *prefix, int len, const char *path\n extern int check_filename(const char *prefix, const char *name);\n extern void verify_filename(const char *prefix, const char *name);\n extern void verify_non_filename(const char *prefix, const char *name);\n+extern int path_inside_repo(const char *prefix, const char *path);\n \n #define INIT_DB_QUIET 0x0001\n \ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 6911196..9a1b459 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -151,23 +151,6 @@ static int queue_diff(struct diff_options *o,\n \t}\n }\n \n-static int path_outside_repo(const char *path)\n-{\n-\tconst char *work_tree;\n-\tsize_t len;\n-\n-\tif (!is_absolute_path(path))\n-\t\treturn 0;\n-\twork_tree = get_git_work_tree();\n-\tif (!work_tree)\n-\t\treturn 1;\n-\tlen = strlen(work_tree);\n-\tif (strncmp(path, work_tree, len) ||\n-\t (path[len] != '\\0' && path[len] != '/'))\n-\t\treturn 1;\n-\treturn 0;\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@@ -197,8 +180,8 @@ void diff_no_index(struct rev_info *revs,\n \t\t * a colourful \"diff\" replacement.\n \t\t */\n \t\tif ((argc != i + 2) ||\n-\t\t (!path_outside_repo(argv[i]) &&\n-\t\t !path_outside_repo(argv[i+1])))\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)\ndiff --git a/setup.c b/setup.c\nindex 731851a..2cfa037 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -4,7 +4,7 @@\n static int inside_git_dir = -1;\n static int inside_work_tree = -1;\n \n-char *prefix_path(const char *prefix, int len, const char *path)\n+static char *prefix_path_gently(const char *prefix, int len, const char *path)\n {\n \tconst char *orig = path;\n \tchar *sanitized;\n@@ -31,7 +31,8 @@ char *prefix_path(const char *prefix, int len, const char *path)\n \t\tif (strncmp(sanitized, work_tree, len) ||\n \t\t (len > root_len && sanitized[len] != '\\0' && sanitized[len] != '/')) {\n \t\terror_out:\n-\t\t\tdie(\"'%s' is outside repository\", orig);\n+\t\t\tfree(sanitized);\n+\t\t\treturn NULL;\n \t\t}\n \t\tif (sanitized[len] == '/')\n \t\t\tlen++;\n@@ -40,6 +41,25 @@ char *prefix_path(const char *prefix, int len, const char *path)\n \treturn sanitized;\n }\n \n+char *prefix_path(const char *prefix, int len, const char *path)\n+{\n+\tchar *r = prefix_path_gently(prefix, len, path);\n+\tif (!r)\n+\t\tdie(\"'%s' is outside repository\", path);\n+\treturn r;\n+}\n+\n+int path_inside_repo(const char *prefix, const char *path)\n+{\n+\tint len = prefix ? strlen(prefix) : 0;\n+\tchar *r = prefix_path_gently(prefix, len, path);\n+\tif (r) {\n+\t\tfree(r);\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n int check_filename(const char *prefix, const char *arg)\n {\n \tconst char *name;\n"},{"id":"193849","messageId":"CAFouetgRq1qkqJmThJJeu=Mdx9jS0c9dw7NPSwuJUOSpskCY2A@mail.gmail.com","threadId":"30833","inReplyTo":"20120619135814.GA3210@sigill.intra.peff.net","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-06-19T16:47:08Z","receivedAt":"2012-06-19T16:47:08Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Tue, Jun 19, 2012 at 9:58 AM, Jeff King wrote:\n> On Tue, Jun 19, 2012 at 09:05:40AM -0400, Tim Henigan wrote:\n>\n>> As a side note, I found that these tests fail if a relative path is\n>> used for the file in 'non/git'.  In other words, this passes:\n>>\n>>     test_expect_code 0 git diff --quiet a\n>> \"$TRASH_DIRECTORY/test-outside/non/git/matching-file\"\n>>\n>> but this fails:\n>>\n>>     test_expect_code 0 git diff --quiet a ../non/git/matching-file\n>>\n>> This surprised me, but I have not investigated any further.\n>\n> The problem is that path_outside_repo in diff-no-index.c does not bother\n> handling relative paths at all, and just assumes they are inside the\n> repository. This is obviously not true if the path starts with \"..\", in\n> which case you would need to compare the number of \"..\" with the current\n> depth in the repository.\n>\n> prefix_path already does this (and is what generates the later\n> \"../non/git/matching-file is not in the repository\" message). We could\n> perhaps get rid of path_outside_repo and just re-use prefix_path's\n> logic, something like (not tested):\n\nWith your patch applied, I was able to use relative paths in my tests.\n I also confirmed that all the t4*.sh tests pass.\n\nFor what its worth, your patch looks correct to me. Existing\nconsumers of 'prefix_path' should get the same results as before and\nthe one added xmalloc is paired with a free.\n"},{"id":"193932","messageId":"CAFouetgXkqJPYwjr5ob5ed_ooL-D56zXyjnOAWrVPdt_eZqw7g@mail.gmail.com","threadId":"30833","inReplyTo":"CAFouetgRq1qkqJmThJJeu=Mdx9jS0c9dw7NPSwuJUOSpskCY2A@mail.gmail.com","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-06-20T13:38:15Z","receivedAt":"2012-06-20T13:38:15Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Tue, Jun 19, 2012 at 12:47 PM, Tim Henigan wrote:\n> On Tue, Jun 19, 2012 at 9:58 AM, Jeff King wrote:\n>> On Tue, Jun 19, 2012 at 09:05:40AM -0400, Tim Henigan wrote:\n>>\n>>> As a side note, I found that these tests fail if a relative path is\n>>> used for the file in 'non/git'.  In other words, this passes:\n>>>\n>>>     test_expect_code 0 git diff --quiet a\n>>> \"$TRASH_DIRECTORY/test-outside/non/git/matching-file\"\n>>>\n>>> but this fails:\n>>>\n>>>     test_expect_code 0 git diff --quiet a ../non/git/matching-file\n>>>\n>>> This surprised me, but I have not investigated any further.\n>>\n>> The problem is that path_outside_repo in diff-no-index.c does not bother\n>> handling relative paths at all, and just assumes they are inside the\n>> repository. This is obviously not true if the path starts with \"..\", in\n>> which case you would need to compare the number of \"..\" with the current\n>> depth in the repository.\n>>\n>> prefix_path already does this (and is what generates the later\n>> \"../non/git/matching-file is not in the repository\" message). We could\n>> perhaps get rid of path_outside_repo and just re-use prefix_path's\n>> logic, something like (not tested):\n>\n> With your patch applied, I was able to use relative paths in my tests.\n>  I also confirmed that all the t4*.sh tests pass.\n>\n> For what its worth, your patch looks correct to me.  Existing\n> consumers of 'prefix_path' should get the same results as before and\n> the one added xmalloc is paired with a free.\n\nJeff,\n\nAre you planning to send this patch to the list? If not, can I\ninclude it as 1 of 2 before my patch? If we go that route, I'm not\nsure how to properly show you as the author...\n\nAlso, in an earlier email [1] you mentioned that it may be a good idea\nto rename 'found_changes' to something like 'xdiff_found_changes'. I\nlike the idea...I could submit this change as another patch in the\nseries, if you have no objections.\n\nThanks again for your review and help.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/200160/focus=200163\n"},{"id":"193936","messageId":"20120620160607.GA12856@sigill.intra.peff.net","threadId":"30833","inReplyTo":"CAFouetgXkqJPYwjr5ob5ed_ooL-D56zXyjnOAWrVPdt_eZqw7g@mail.gmail.com","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-20T16:06:08Z","receivedAt":"2012-06-20T16:06:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 20, 2012 at 09:38:15AM -0400, Tim Henigan wrote:\n\n> > With your patch applied, I was able to use relative paths in my tests.\n> >  I also confirmed that all the t4*.sh tests pass.\n> >\n> > For what its worth, your patch looks correct to me.  Existing\n> > consumers of 'prefix_path' should get the same results as before and\n> > the one added xmalloc is paired with a free.\n> \n> Jeff,\n> \n> Are you planning to send this patch to the list? If not, can I\n> include it as 1 of 2 before my patch? If we go that route, I'm not\n> sure how to properly show you as the author...\n\nI'd probably get to it eventually, but I haven't touched it since I sent\nit. If you want to include some tests and package it with a commit\nmessage, that would make me very happy.\n\nYou can override the author by including a \"From: \" header as the first\nline in the body of the email (which git-am will use rather than the\nidentity in the email's From header). If you use git-send-email, it will\ndo this automatically when the patch author does not match your\nidentity.\n\nI didn't sign-off the original, but please feel free to include my\nsign-off, as well as add your own. And note your own contributions in\nthe commit message. So the resulting email would be something like:\n\n\n From: Tim Henigan \n Date: ...\n Subject: [PATCH 1/2] diff: handle relative paths in no-index\n\n From: Jeff King \n\n ... some commit message body ...\n\n Tests and commit message by Tim Henigan.\n\n Signed-off-by: Jeff King \n Signed-off-by: Tim Henigan \n ---\n ... the actual patch ...\n\n> Also, in an earlier email [1] you mentioned that it may be a good idea\n> to rename 'found_changes' to something like 'xdiff_found_changes'. I\n> like the idea...I could submit this change as another patch in the\n> series, if you have no objections.\n\nFine by me. I think \"xdiff_found_changes\" is not quite accurate; it is\nreally \"did builtin_diff find any changes?\" since we might never call\ninto xdiff (e.g., for binary files). I'm not sure what the best name is.\n\n-Peff\n"},{"id":"193965","messageId":"7vpq8tvn5i.fsf@alter.siamese.dyndns.org","threadId":"30833","inReplyTo":"20120620160607.GA12856@sigill.intra.peff.net","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-20T18:44:25Z","receivedAt":"2012-06-20T18:44:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Fine by me. I think \"xdiff_found_changes\" is not quite accurate; it is\n> really \"did builtin_diff find any changes?\" since we might never call\n> into xdiff (e.g., for binary files). I'm not sure what the best name is.\n\n\"diffopt.found_changes\" is clear enough for me.\n"},{"id":"193966","messageId":"20120620185237.GA31520@sigill.intra.peff.net","threadId":"30833","inReplyTo":"7vpq8tvn5i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-20T18:52:37Z","receivedAt":"2012-06-20T18:52:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 20, 2012 at 11:44:25AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Fine by me. I think \"xdiff_found_changes\" is not quite accurate; it is\n> > really \"did builtin_diff find any changes?\" since we might never call\n> > into xdiff (e.g., for binary files). I'm not sure what the best name is.\n> \n> \"diffopt.found_changes\" is clear enough for me.\n\nI thought this bug would be enough to show that diffopt.found_changes is\nnot clear enough. It is the source of the original bug (the code should\nhave been using HAS_CHANGES instead of found_changes), and it gave at\nleast one of the bug investigators (i.e., me) quite a bit of confusion\nto understand why found_changes was not being set when diff_flush found\nchanges.\n\nIOW, as a naive reader of the \"struct diff_options\", how do I understand\nthe difference between HAS_CHANGES and found_changes?\n\n-Peff\n"},{"id":"193968","messageId":"7vipelvlg7.fsf@alter.siamese.dyndns.org","threadId":"30833","inReplyTo":"20120620185237.GA31520@sigill.intra.peff.net","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-20T19:21:12Z","receivedAt":"2012-06-20T19:21:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I thought this bug would be enough to show that diffopt.found_changes is\n> not clear enough. It is the source of the original bug (the code should\n> have been using HAS_CHANGES instead of found_changes), and it gave at\n> least one of the bug investigators (i.e., me) quite a bit of confusion\n> to understand why found_changes was not being set when diff_flush found\n> changes.\n\nI think when found_changes was introduced so that diff can indicate\nmore than what HAS_CHANGES (i.e. there is a blob-level difference\nexists) can represent, the patch forgot to update the no-index\ncodepath.\n\n> IOW, as a naive reader of the \"struct diff_options\", how do I understand\n> the difference between HAS_CHANGES and found_changes?\n\nHAS_CHANGES and found_changes should be implementation detail of\ndiff_result_code() and as long as we do not add outside users of it,\nthe names should not matter too much. If we were to rename them,\nHAS_CHANGES should also be made more descriptive to hint what it\nmeans (\"object level difference exists\"), I would think. Given the\nrecent discussion on \"diff/log -L ,\", found_changes\nwould mean \"content level change that the caller cares about\nexists\".\n"},{"id":"194037","messageId":"CAFouethNTzcWq_YKzGz+jRTeCjKZEC2ZYMZuQxkF+5AOTC=x-A@mail.gmail.com","threadId":"30833","inReplyTo":"20120620160607.GA12856@sigill.intra.peff.net","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-06-21T15:07:12Z","receivedAt":"2012-06-21T15:07:12Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Wed, Jun 20, 2012 at 12:06 PM, Jeff King wrote:\n> On Wed, Jun 20, 2012 at 09:38:15AM -0400, Tim Henigan wrote:\n>>\n>> Are you planning to send this patch to the list?  If not, can I\n>> include it as 1 of 2 before my patch?  If we go that route, I'm not\n>> sure how to properly show you as the author...\n>\n> I'd probably get to it eventually, but I haven't touched it since I sent\n> it. If you want to include some tests and package it with a commit\n> message, that would make me very happy.\n\nThanks, I will do that.\n\nIt looks like the best place to add tests is t4010-diff-pathspec.sh.\nThe only cases not tested through other means appear to be:\n\n git diff \n git diff \n\nOther pathspec variations seem to be covered extensively by other\ntests (mostly as a side effect). Am I missing other variations that\nshould be checked?\n\nIn both cases shown above, we are simply verifying that giving a\nrelative path to diff does not cause it to abort. So it may be\nsufficient to only test one of the above.\n\nThe tests that I added to t4035-diff-quiet.sh already cover both of\nthe cases listed above. Is it worthwhile to duplicate some of those\ntests in t4010?\n"},{"id":"194039","messageId":"7vsjdosiz3.fsf@alter.siamese.dyndns.org","threadId":"30833","inReplyTo":"CAFouethNTzcWq_YKzGz+jRTeCjKZEC2ZYMZuQxkF+5AOTC=x-A@mail.gmail.com","subject":"Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet ' finds changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-21T16:55:12Z","receivedAt":"2012-06-21T16:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan writes:\n\n> It looks like the best place to add tests is t4010-diff-pathspec.sh.\n\nWell, pathspecs are all about limiting the changes in the repository\nby patterns, while \"diff --no-index\" is about exact pathnames, so I\nam not sure if that is a good place to put them. Isn't there a test\nscript that is dedicated to the \"diff --no-index\" codepath (perhaps\n4053) that is more appropriate for doing this?\n"}]}