{"thread":{"id":"35718","subject":"bug with git-diff --quiet","startedAt":"2014-01-23T02:45:25Z","lastAt":"2014-01-31T16:17:12Z","messageCount":12,"participants":["IWAMOTO Toshihiro","Duy Nguyen","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"233584","messageId":"20140123024525.B726248918@mail.valinux.co.jp","threadId":"35718","inReplyTo":null,"subject":"bug with git-diff --quiet","fromName":"IWAMOTO Toshihiro","fromEmail":"iwamoto@valinux.co.jp","sentAt":"2014-01-23T02:45:25Z","receivedAt":"2014-01-23T02:45:25Z","isPatch":false,"sender":{"key":"iwamoto@valinux.co.jp","avatar":null},"body":"I found \"git-diff --quiet\" returns a zero exit status even if there's\na change.  The following sequence reproduces the bug:\n\n  $ mkdir foo\n  $ cd foo\n  $ git init\n  $ echo a > a.txt\n  $ echo b >b.txt\n  $ git add ?.txt\n  $ git commit\n  $ echo b >> b.txt\n  $ touch a.txt\n  $ git diff --quiet; echo $?\n  \nInterestingly, if you issue \"git-diff --quiet\" again, it returns the\nexpected exit status 1.\n\nThe problem is in the optimization code in run_diff_files().  The\nfunction finds a.txt has different stat(2) data from .git/index and\ncalls diff_change(), which sets DIFF_OPT_HAS_CHANGES.  As the flag\nmakes diff_can_quit_early() return 1, run_diff_files()'s loop finishes\nwithout calling diff_change() for b.txt.\n\nThen, diffcore_std() examines diff_queued_diff and clears\nDIFF_OPT_HAS_CHANGES, because a.txt is unchanged.\nThis is how a change in b.txt is ignored by git-diff --quiet.\n\nHere's a obvious fix for this bug, but I think you can find a better\nfix. Thanks in advance.\n\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 346cac6..0b8c58d 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -105,9 +105,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tint changed;\n \t\tunsigned dirty_submodule = 0;\n \n-\t\tif (diff_can_quit_early(&revs->diffopt))\n-\t\t\tbreak;\n-\n \t\tif (!ce_path_match(ce, &revs->prune_data))\n \t\t\tcontinue;\n \n--\nIWAMOTO Toshihiro\n"},{"id":"233741","messageId":"20140125040315.GA26556@lanh","threadId":"35718","inReplyTo":"20140123024525.B726248918@mail.valinux.co.jp","subject":"Re: bug with git-diff --quiet","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-01-25T04:03:15Z","receivedAt":"2014-01-25T04:03:15Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 23, 2014 at 11:45:25AM +0900, IWAMOTO Toshihiro wrote:\n> I found \"git-diff --quiet\" returns a zero exit status even if there's\n> a change.  The following sequence reproduces the bug:\n> \n>   $ mkdir foo\n>   $ cd foo\n>   $ git init\n>   $ echo a > a.txt\n>   $ echo b >b.txt\n>   $ git add ?.txt\n>   $ git commit\n>   $ echo b >> b.txt\n>   $ touch a.txt\n>   $ git diff --quiet; echo $?\n>   \n> Interestingly, if you issue \"git-diff --quiet\" again, it returns the\n> expected exit status 1.\n\nBecause stat info in index is updated and diff_change() won't be\ncalled again on a.txt.\n\n> The problem is in the optimization code in run_diff_files().  The\n> function finds a.txt has different stat(2) data from .git/index and\n> calls diff_change(), which sets DIFF_OPT_HAS_CHANGES.  As the flag\n> makes diff_can_quit_early() return 1, run_diff_files()'s loop finishes\n> without calling diff_change() for b.txt.\n> \n> Then, diffcore_std() examines diff_queued_diff and clears\n> DIFF_OPT_HAS_CHANGES, because a.txt is unchanged.\n> This is how a change in b.txt is ignored by git-diff --quiet.\n\nThanks for the analysis. Perhaps we could make diff_change test\nwhether a.txt is unchanged so it does not set HAS_CHANGES prematurely?\nMaybe something like below.\n\nBy the time diffcore_skip_stat_unmatch() is called, everything is\ncached, so there's not much of performance regression. We still do\nmemcmp() twice (in diff_filespec_is_identical), but I think that has\nless impact than removing diff_can_quit_early().\n\n-- 8< --\ndiff --git a/diff.c b/diff.c\nindex 6b4cd0e..5226fc0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4697,6 +4697,33 @@ static int diff_filespec_is_identical(struct diff_filespec *one,\n \treturn !memcmp(one->data, two->data, one->size);\n }\n \n+static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n+{\n+\t/*\n+\t * 1. Entries that come from stat info dirtiness\n+\t *    always have both sides (iow, not create/delete),\n+\t *    one side of the object name is unknown, with\n+\t *    the same mode and size.  Keep the ones that\n+\t *    do not match these criteria.  They have real\n+\t *    differences.\n+\t *\n+\t * 2. At this point, the file is known to be modified,\n+\t *    with the same mode and size, and the object\n+\t *    name of one side is unknown.  Need to inspect\n+\t *    the identical contents.\n+\t */\n+\tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n+\t    !DIFF_FILE_VALID(p->two) ||\n+\t    (p->one->sha1_valid && p->two->sha1_valid) ||\n+\t    (p->one->mode != p->two->mode) ||\n+\t    diff_populate_filespec(p->one, 1) ||\n+\t    diff_populate_filespec(p->two, 1) ||\n+\t    (p->one->size != p->two->size) ||\n+\t    !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n {\n \tint i;\n@@ -4707,27 +4734,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \n-\t\t/*\n-\t\t * 1. Entries that come from stat info dirtiness\n-\t\t *    always have both sides (iow, not create/delete),\n-\t\t *    one side of the object name is unknown, with\n-\t\t *    the same mode and size.  Keep the ones that\n-\t\t *    do not match these criteria.  They have real\n-\t\t *    differences.\n-\t\t *\n-\t\t * 2. At this point, the file is known to be modified,\n-\t\t *    with the same mode and size, and the object\n-\t\t *    name of one side is unknown.  Need to inspect\n-\t\t *    the identical contents.\n-\t\t */\n-\t\tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n-\t\t    !DIFF_FILE_VALID(p->two) ||\n-\t\t    (p->one->sha1_valid && p->two->sha1_valid) ||\n-\t\t    (p->one->mode != p->two->mode) ||\n-\t\t    diff_populate_filespec(p->one, 1) ||\n-\t\t    diff_populate_filespec(p->two, 1) ||\n-\t\t    (p->one->size != p->two->size) ||\n-\t\t    !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n+\t\tif (diff_filespec_check_stat_unmatch(p))\n \t\t\tdiff_q(&outq, p);\n \t\telse {\n \t\t\t/*\n@@ -4890,6 +4897,7 @@ void diff_change(struct diff_options *options,\n \t\t unsigned old_dirty_submodule, unsigned new_dirty_submodule)\n {\n \tstruct diff_filespec *one, *two;\n+\tstruct diff_filepair *p;\n \n \tif (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) &&\n \t    is_submodule_ignored(concatpath, options))\n@@ -4916,10 +4924,17 @@ void diff_change(struct diff_options *options,\n \tfill_filespec(two, new_sha1, new_sha1_valid, new_mode);\n \tone->dirty_submodule = old_dirty_submodule;\n \ttwo->dirty_submodule = new_dirty_submodule;\n+\tp = diff_queue(&diff_queued_diff, one, two);\n \n-\tdiff_queue(&diff_queued_diff, one, two);\n-\tif (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n-\t\tDIFF_OPT_SET(options, HAS_CHANGES);\n+\tif (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n+\t\treturn;\n+\n+\tif (DIFF_OPT_TST(options, QUICK) &&\n+\t    options->skip_stat_unmatch &&\n+\t    !diff_filespec_check_stat_unmatch(p))\n+\t\treturn;\n+\n+\tDIFF_OPT_SET(options, HAS_CHANGES);\n }\n \n struct diff_filepair *diff_unmerge(struct diff_options *options, const char *path)\n-- 8< --\n\n> Here's a obvious fix for this bug, but I think you can find a better\n> fix. Thanks in advance.\n> \n> \n> diff --git a/diff-lib.c b/diff-lib.c\n> index 346cac6..0b8c58d 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -105,9 +105,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>  \t\tint changed;\n>  \t\tunsigned dirty_submodule = 0;\n>  \n> -\t\tif (diff_can_quit_early(&revs->diffopt))\n> -\t\t\tbreak;\n> -\n>  \t\tif (!ce_path_match(ce, &revs->prune_data))\n>  \t\t\tcontinue;\n>  \n> --\n> IWAMOTO Toshihiro\n"},{"id":"233744","messageId":"1390632411-3596-1-git-send-email-pclouds@gmail.com","threadId":"35718","inReplyTo":"20140123024525.B726248918@mail.valinux.co.jp","subject":"[PATCH 1/3] Move diffcore_skip_stat_unmatch core logic out for reuse later","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-01-25T06:46:49Z","receivedAt":"2014-01-25T06:46:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n diff.c | 49 ++++++++++++++++++++++++++++---------------------\n 1 file changed, 28 insertions(+), 21 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6b4cd0e..19460ff 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4697,6 +4697,33 @@ static int diff_filespec_is_identical(struct diff_filespec *one,\n \treturn !memcmp(one->data, two->data, one->size);\n }\n \n+static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n+{\n+\t/*\n+\t * 1. Entries that come from stat info dirtiness\n+\t *    always have both sides (iow, not create/delete),\n+\t *    one side of the object name is unknown, with\n+\t *    the same mode and size.  Keep the ones that\n+\t *    do not match these criteria.  They have real\n+\t *    differences.\n+\t *\n+\t * 2. At this point, the file is known to be modified,\n+\t *    with the same mode and size, and the object\n+\t *    name of one side is unknown.  Need to inspect\n+\t *    the identical contents.\n+\t */\n+\tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n+\t    !DIFF_FILE_VALID(p->two) ||\n+\t    (p->one->sha1_valid && p->two->sha1_valid) ||\n+\t    (p->one->mode != p->two->mode) ||\n+\t    diff_populate_filespec(p->one, 1) ||\n+\t    diff_populate_filespec(p->two, 1) ||\n+\t    (p->one->size != p->two->size) ||\n+\t    !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n {\n \tint i;\n@@ -4707,27 +4734,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \n-\t\t/*\n-\t\t * 1. Entries that come from stat info dirtiness\n-\t\t *    always have both sides (iow, not create/delete),\n-\t\t *    one side of the object name is unknown, with\n-\t\t *    the same mode and size.  Keep the ones that\n-\t\t *    do not match these criteria.  They have real\n-\t\t *    differences.\n-\t\t *\n-\t\t * 2. At this point, the file is known to be modified,\n-\t\t *    with the same mode and size, and the object\n-\t\t *    name of one side is unknown.  Need to inspect\n-\t\t *    the identical contents.\n-\t\t */\n-\t\tif (!DIFF_FILE_VALID(p->one) || /* (1) */\n-\t\t    !DIFF_FILE_VALID(p->two) ||\n-\t\t    (p->one->sha1_valid && p->two->sha1_valid) ||\n-\t\t    (p->one->mode != p->two->mode) ||\n-\t\t    diff_populate_filespec(p->one, 1) ||\n-\t\t    diff_populate_filespec(p->two, 1) ||\n-\t\t    (p->one->size != p->two->size) ||\n-\t\t    !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n+\t\tif (diff_filespec_check_stat_unmatch(p))\n \t\t\tdiff_q(&outq, p);\n \t\telse {\n \t\t\t/*\n-- \n1.8.5.2.240.g8478abd\n"},{"id":"233745","messageId":"1390632411-3596-2-git-send-email-pclouds@gmail.com","threadId":"35718","inReplyTo":"1390632411-3596-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/3] diff: do not quit early on stat-dirty files","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-01-25T06:46:50Z","receivedAt":"2014-01-25T06:46:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When QUICK is set (i.e. with --quiet) we try to do as little work as\npossible, stopping after seeing the first change. stat-dirty is\nconsidered a \"change\" but it may turn out not, if no actual content is\nchanged. The actual content test is performed too late in the process\nand the shortcut may be taken prematurely, leading to incorrect return\ncode.\n\nAssume we do \"git diff --quiet\". If we have a stat-dirty file \"a\" and\na really dirty file \"b\". We break the loop in run_diff_files() and\nstop after \"a\" because we have got a \"change\". Later in\ndiffcore_skip_stat_unmatch() we find out \"a\" is actually not\nchanged. But there's nothing else in the diff queue, we incorrectly\ndeclare \"no change\", ignoring the fact that \"b\" is changed.\n\nThis also happens to \"git diff --quiet HEAD\" when it hits\ndiff_can_quit_early() in oneway_diff().\n\nThis patch does the content test earlier in order to keep going if \"a\"\nis unchanged. The test result is cached so that when\ndiffcore_skip_stat_unmatch() is done in the end, we spend no cycles on\nre-testing \"a\".\n\nReported-by: IWAMOTO Toshihiro <iwamoto@valinux.co.jp>\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n diff.c                | 22 +++++++++++++++++-----\n diffcore.h            |  2 ++\n t/t4035-diff-quiet.sh |  6 ++++++\n 3 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 19460ff..ab85f7e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4699,6 +4699,11 @@ static int diff_filespec_is_identical(struct diff_filespec *one,\n \n static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n {\n+\tif (p->done_skip_stat_unmatch)\n+\t\treturn p->skip_stat_unmatch_result;\n+\n+\tp->done_skip_stat_unmatch = 1;\n+\tp->skip_stat_unmatch_result = 0;\n \t/*\n \t * 1. Entries that come from stat info dirtiness\n \t *    always have both sides (iow, not create/delete),\n@@ -4720,8 +4725,8 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)\n \t    diff_populate_filespec(p->two, 1) ||\n \t    (p->one->size != p->two->size) ||\n \t    !diff_filespec_is_identical(p->one, p->two)) /* (2) */\n-\t\treturn 1;\n-\treturn 0;\n+\t\tp->skip_stat_unmatch_result = 1;\n+\treturn p->skip_stat_unmatch_result;\n }\n \n static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)\n@@ -4897,6 +4902,7 @@ void diff_change(struct diff_options *options,\n \t\t unsigned old_dirty_submodule, unsigned new_dirty_submodule)\n {\n \tstruct diff_filespec *one, *two;\n+\tstruct diff_filepair *p;\n \n \tif (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) &&\n \t    is_submodule_ignored(concatpath, options))\n@@ -4923,10 +4929,16 @@ void diff_change(struct diff_options *options,\n \tfill_filespec(two, new_sha1, new_sha1_valid, new_mode);\n \tone->dirty_submodule = old_dirty_submodule;\n \ttwo->dirty_submodule = new_dirty_submodule;\n+\tp = diff_queue(&diff_queued_diff, one, two);\n \n-\tdiff_queue(&diff_queued_diff, one, two);\n-\tif (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n-\t\tDIFF_OPT_SET(options, HAS_CHANGES);\n+\tif (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n+\t\treturn;\n+\n+\tif (DIFF_OPT_TST(options, QUICK) && options->skip_stat_unmatch &&\n+\t    !diff_filespec_check_stat_unmatch(p))\n+\t\treturn;\n+\n+\tDIFF_OPT_SET(options, HAS_CHANGES);\n }\n \n struct diff_filepair *diff_unmerge(struct diff_options *options, const char *path)\ndiff --git a/diffcore.h b/diffcore.h\nindex 1c16c85..6b538bc 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -70,6 +70,8 @@ struct diff_filepair {\n \tunsigned broken_pair : 1;\n \tunsigned renamed_pair : 1;\n \tunsigned is_unmerged : 1;\n+\tunsigned done_skip_stat_unmatch : 1;\n+\tunsigned skip_stat_unmatch_result : 1;\n };\n #define DIFF_PAIR_UNMERGED(p) ((p)->is_unmerged)\n \ndiff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh\nindex 231412d..e8ae2a0 100755\n--- a/t/t4035-diff-quiet.sh\n+++ b/t/t4035-diff-quiet.sh\n@@ -148,4 +148,10 @@ test_expect_success 'git diff --ignore-all-space, both files outside repo' '\n \t)\n '\n \n+test_expect_success 'git diff --quiet ignores stat-change only entries' '\n+\ttest-chmtime +10 a &&\n+\techo modified >>b &&\n+\ttest_expect_code 1 git diff --quiet\n+'\n+\n test_done\n-- \n1.8.5.2.240.g8478abd\n"},{"id":"233746","messageId":"1390632411-3596-3-git-send-email-pclouds@gmail.com","threadId":"35718","inReplyTo":"1390632411-3596-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/3] diff: turn off skip_stat_unmatch on \"diff --cached\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-01-25T06:46:51Z","receivedAt":"2014-01-25T06:46:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"skip_stat_unmatch flag is added in fb13227 (git-diff: squelch \"empty\"\ndiffs - 2007-08-03) to ignore empty diffs caused by stat-only\ndirtiness. In \"diff --cached\" case, stat is not invovled at all. While\nthe code is written in a way that no expensive I/O is done, we still\nneed to move all file pairs from the old queue to the new queue in\ndiffcore_skip_stat_unmatch(). Avoid it.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/diff.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 0f247d2..85f97d7 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -150,9 +150,10 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\t\tperror(\"read_cache_preload\");\n \t\t\treturn -1;\n \t\t}\n-\t} else if (read_cache() < 0) {\n-\t\tperror(\"read_cache\");\n-\t\treturn -1;\n+\t} else {\n+\t\tif (read_cache() < 0)\n+\t\t\treturn error(\"read_cache: %s\", strerror(errno));\n+\t\trevs->diffopt.skip_stat_unmatch = 0;\n \t}\n \treturn run_diff_index(revs, cached);\n }\n-- \n1.8.5.2.240.g8478abd\n"},{"id":"233845","messageId":"1390863568-22656-1-git-send-email-pclouds@gmail.com","threadId":"35718","inReplyTo":"1390632411-3596-3-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-01-27T22:59:28Z","receivedAt":"2014-01-27T22:59:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"skip_stat_unmatch flag is added in fb13227 (git-diff: squelch \"empty\"\ndiffs - 2007-08-03) to ignore empty diffs caused by stat-only\ndirtiness. In some diff case, stat is not involved at all. While\nthe code is written in a way that no expensive I/O is done, we still\nneed to move all file pairs from the old queue to the new queue in\ndiffcore_skip_stat_unmatch().\n\nOnly enable it when worktree is involved: \"diff\" and \"diff <rev>\".\nThis should help track down how skip_stat_unmatch is actually used\nwhen bugs occur.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n This replaces 'diff: turn off skip_stat_unmatch on \"diff --cached\"'\n The previous patch obviously leaves skip_stat_unmatch on in \"diff\n <rev> <rev>\" and maybe other cases.\n\n builtin/diff.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 0f247d2..88542d9 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -150,6 +150,7 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\t\tperror(\"read_cache_preload\");\n \t\t\treturn -1;\n \t\t}\n+\t\trevs->diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n \t} else if (read_cache() < 0) {\n \t\tperror(\"read_cache\");\n \t\treturn -1;\n@@ -252,6 +253,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \t\tperror(\"read_cache_preload\");\n \t\treturn -1;\n \t}\n+\trevs->diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n \treturn run_diff_files(revs, options);\n }\n \n@@ -343,7 +345,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\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;\n \n \t/* Scale to real terminal size and respect statGraphWidth config */\n \trev.diffopt.stat_width = -1;\n-- \n1.8.5.2.240.g8478abd\n"},{"id":"233851","messageId":"xmqqd2jdm1jj.fsf@gitster.dls.corp.google.com","threadId":"35718","inReplyTo":"1390863568-22656-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-27T23:45:52Z","receivedAt":"2014-01-27T23:45:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> skip_stat_unmatch flag is added in fb13227 (git-diff: squelch \"empty\"\n> diffs - 2007-08-03) to ignore empty diffs caused by stat-only\n> dirtiness. In some diff case, stat is not involved at all. While\n> the code is written in a way that no expensive I/O is done, we still\n> need to move all file pairs from the old queue to the new queue in\n> diffcore_skip_stat_unmatch().\n>\n> Only enable it when worktree is involved: \"diff\" and \"diff <rev>\".\n> This should help track down how skip_stat_unmatch is actually used\n> when bugs occur.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  This replaces 'diff: turn off skip_stat_unmatch on \"diff --cached\"'\n>  The previous patch obviously leaves skip_stat_unmatch on in \"diff\n>  <rev> <rev>\" and maybe other cases.\n\nOops, I lost track.  Sorry.\n"},{"id":"233877","messageId":"xmqq7g9jlny6.fsf@gitster.dls.corp.google.com","threadId":"35718","inReplyTo":"xmqqd2jdm1jj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-28T22:51:45Z","receivedAt":"2014-01-28T22:51:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> skip_stat_unmatch flag is added in fb13227 (git-diff: squelch \"empty\"\n>> diffs - 2007-08-03) to ignore empty diffs caused by stat-only\n>> dirtiness. In some diff case, stat is not involved at all. While\n>> the code is written in a way that no expensive I/O is done, we still\n>> need to move all file pairs from the old queue to the new queue in\n>> diffcore_skip_stat_unmatch().\n>>\n>> Only enable it when worktree is involved: \"diff\" and \"diff <rev>\".\n>> This should help track down how skip_stat_unmatch is actually used\n>> when bugs occur.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n>>  This replaces 'diff: turn off skip_stat_unmatch on \"diff --cached\"'\n>>  The previous patch obviously leaves skip_stat_unmatch on in \"diff\n>>  <rev> <rev>\" and maybe other cases.\n>\n> Oops, I lost track.  Sorry.\n\nTogether with {1,2}/3 applied on maint-1.8.4, this sems to break\nt3417 (there may be others, but I didn't have time to check).\n"},{"id":"233878","messageId":"20140128235203.GA7788@lanh","threadId":"35718","inReplyTo":"xmqq7g9jlny6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-01-28T23:52:03Z","receivedAt":"2014-01-28T23:52:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 28, 2014 at 02:51:45PM -0800, Junio C Hamano wrote:\n> >>  This replaces 'diff: turn off skip_stat_unmatch on \"diff --cached\"'\n> >>  The previous patch obviously leaves skip_stat_unmatch on in \"diff\n> >>  <rev> <rev>\" and maybe other cases.\n> >\n> > Oops, I lost track.  Sorry.\n> \n> Together with {1,2}/3 applied on maint-1.8.4, this sems to break\n> t3417 (there may be others, but I didn't have time to check).\n\nMy bad. I thought I covered all cases in my last patch (and didn't\nretest it!). It turns out I should have set skip_stat_unmatch in\nbuiltin_diff_b_f() too. This on top of 3/3 passes the tests\n\n-- 8< --\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 88542d9..8ab5e3d 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -89,6 +89,7 @@ static int builtin_diff_b_f(struct rev_info *revs,\n \tif (blob[0].mode == S_IFINVALID)\n \t\tblob[0].mode = canon_mode(st.st_mode);\n \n+\trevs->diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n \tstuff_change(&revs->diffopt,\n \t\t     blob[0].mode, canon_mode(st.st_mode),\n \t\t     blob[0].sha1, null_sha1,\n-- 8< --\n"},{"id":"233897","messageId":"xmqq61p2k2u5.fsf@gitster.dls.corp.google.com","threadId":"35718","inReplyTo":"20140128235203.GA7788@lanh","subject":"Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-29T19:25:22Z","receivedAt":"2014-01-29T19:25:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Tue, Jan 28, 2014 at 02:51:45PM -0800, Junio C Hamano wrote:\n>> >>  This replaces 'diff: turn off skip_stat_unmatch on \"diff --cached\"'\n>> >>  The previous patch obviously leaves skip_stat_unmatch on in \"diff\n>> >>  <rev> <rev>\" and maybe other cases.\n>> >\n>> > Oops, I lost track.  Sorry.\n>> \n>> Together with {1,2}/3 applied on maint-1.8.4, this sems to break\n>> t3417 (there may be others, but I didn't have time to check).\n>\n> My bad. I thought I covered all cases in my last patch (and didn't\n> retest it!). It turns out I should have set skip_stat_unmatch in\n> builtin_diff_b_f() too. This on top of 3/3 passes the tests\n\nThanks, will squash it in.\n\nThis however shows that the existing test *KNEW* that it was enough\nto check just a few cases (especially, there is no reason to make\nsure that blob vs file-in-working-tree case behaves sanely), because\nthe auto-refresh would kick in for all codepaths.  Now you are\nmaking that assumption invalid, shouldn't the patch also split the\ntests to cover individual cases?\n\n> -- 8< --\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index 88542d9..8ab5e3d 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -89,6 +89,7 @@ static int builtin_diff_b_f(struct rev_info *revs,\n>  \tif (blob[0].mode == S_IFINVALID)\n>  \t\tblob[0].mode = canon_mode(st.st_mode);\n>  \n> +\trevs->diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\n>  \tstuff_change(&revs->diffopt,\n>  \t\t     blob[0].mode, canon_mode(st.st_mode),\n>  \t\t     blob[0].sha1, null_sha1,\n> -- 8< --\n"},{"id":"233916","messageId":"CACsJy8Dd+baRUi0YjnqDXFi-Fv=K9NxwO=YbYuVEfQkdEXWWWQ@mail.gmail.com","threadId":"35718","inReplyTo":"xmqq61p2k2u5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-01-30T05:36:58Z","receivedAt":"2014-01-30T05:36:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jan 30, 2014 at 2:25 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> On Tue, Jan 28, 2014 at 02:51:45PM -0800, Junio C Hamano wrote:\n> This however shows that the existing test *KNEW* that it was enough\n> to check just a few cases (especially, there is no reason to make\n> sure that blob vs file-in-working-tree case behaves sanely), because\n> the auto-refresh would kick in for all codepaths.  Now you are\n> making that assumption invalid, shouldn't the patch also split the\n> tests to cover individual cases?\n\nDrop the last patch, then. It's a \"while at there\" cleanup patch. If\nit's non trivial then it could be taken up later when somebody's\ninterested. I have a few topics ongoing and not sure I'll go through\ndiff.c to identify and write tests for all cases.\n-- \nDuy\n"},{"id":"233978","messageId":"xmqqbnysi0s7.fsf@gitster.dls.corp.google.com","threadId":"35718","inReplyTo":"CACsJy8Dd+baRUi0YjnqDXFi-Fv=K9NxwO=YbYuVEfQkdEXWWWQ@mail.gmail.com","subject":"Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-31T16:17:12Z","receivedAt":"2014-01-31T16:17:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Thu, Jan 30, 2014 at 2:25 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> On Tue, Jan 28, 2014 at 02:51:45PM -0800, Junio C Hamano wrote:\n>> This however shows that the existing test *KNEW* that it was enough\n>> to check just a few cases (especially, there is no reason to make\n>> sure that blob vs file-in-working-tree case behaves sanely), because\n>> the auto-refresh would kick in for all codepaths.  Now you are\n>> making that assumption invalid, shouldn't the patch also split the\n>> tests to cover individual cases?\n>\n> Drop the last patch, then. It's a \"while at there\" cleanup patch. If\n> it's non trivial then it could be taken up later...\n\nI am leaning towards that because...\n\n> ... not sure I'll go through\n> diff.c to identify and write tests for all cases.\n\n... the effort to ensure the correctness of the patch itself\ninvolves the same identification of the cases.\n\nWe know the single place skip-stat-unmatch was assigned used to\ncover all cases, and the patch was to stop covering cases the\nunnecessary assignments are made while making sure the resulting\ncode still covers cases that assignments are necessary.\n"}]}