* bug with git-diff --quiet @ 2014-01-23 2:45 IWAMOTO Toshihiro 2014-01-25 4:03 ` Duy Nguyen 2014-01-25 6:46 ` [PATCH 1/3] Move diffcore_skip_stat_unmatch core logic out for reuse later Nguyễn Thái Ngọc Duy 0 siblings, 2 replies; 12+ messages in thread From: IWAMOTO Toshihiro @ 2014-01-23 2:45 UTC (permalink / raw) To: git I found "git-diff --quiet" returns a zero exit status even if there's a change. The following sequence reproduces the bug: $ mkdir foo $ cd foo $ git init $ echo a > a.txt $ echo b >b.txt $ git add ?.txt $ git commit $ echo b >> b.txt $ touch a.txt $ git diff --quiet; echo $? Interestingly, if you issue "git-diff --quiet" again, it returns the expected exit status 1. The problem is in the optimization code in run_diff_files(). The function finds a.txt has different stat(2) data from .git/index and calls diff_change(), which sets DIFF_OPT_HAS_CHANGES. As the flag makes diff_can_quit_early() return 1, run_diff_files()'s loop finishes without calling diff_change() for b.txt. Then, diffcore_std() examines diff_queued_diff and clears DIFF_OPT_HAS_CHANGES, because a.txt is unchanged. This is how a change in b.txt is ignored by git-diff --quiet. Here's a obvious fix for this bug, but I think you can find a better fix. Thanks in advance. diff --git a/diff-lib.c b/diff-lib.c index 346cac6..0b8c58d 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -105,9 +105,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option) int changed; unsigned dirty_submodule = 0; - if (diff_can_quit_early(&revs->diffopt)) - break; - if (!ce_path_match(ce, &revs->prune_data)) continue; -- IWAMOTO Toshihiro ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: bug with git-diff --quiet 2014-01-23 2:45 bug with git-diff --quiet IWAMOTO Toshihiro @ 2014-01-25 4:03 ` Duy Nguyen 2014-01-25 6:46 ` [PATCH 1/3] Move diffcore_skip_stat_unmatch core logic out for reuse later Nguyễn Thái Ngọc Duy 1 sibling, 0 replies; 12+ messages in thread From: Duy Nguyen @ 2014-01-25 4:03 UTC (permalink / raw) To: IWAMOTO Toshihiro; +Cc: git On Thu, Jan 23, 2014 at 11:45:25AM +0900, IWAMOTO Toshihiro wrote: > I found "git-diff --quiet" returns a zero exit status even if there's > a change. The following sequence reproduces the bug: > > $ mkdir foo > $ cd foo > $ git init > $ echo a > a.txt > $ echo b >b.txt > $ git add ?.txt > $ git commit > $ echo b >> b.txt > $ touch a.txt > $ git diff --quiet; echo $? > > Interestingly, if you issue "git-diff --quiet" again, it returns the > expected exit status 1. Because stat info in index is updated and diff_change() won't be called again on a.txt. > The problem is in the optimization code in run_diff_files(). The > function finds a.txt has different stat(2) data from .git/index and > calls diff_change(), which sets DIFF_OPT_HAS_CHANGES. As the flag > makes diff_can_quit_early() return 1, run_diff_files()'s loop finishes > without calling diff_change() for b.txt. > > Then, diffcore_std() examines diff_queued_diff and clears > DIFF_OPT_HAS_CHANGES, because a.txt is unchanged. > This is how a change in b.txt is ignored by git-diff --quiet. Thanks for the analysis. Perhaps we could make diff_change test whether a.txt is unchanged so it does not set HAS_CHANGES prematurely? Maybe something like below. By the time diffcore_skip_stat_unmatch() is called, everything is cached, so there's not much of performance regression. We still do memcmp() twice (in diff_filespec_is_identical), but I think that has less impact than removing diff_can_quit_early(). -- 8< -- diff --git a/diff.c b/diff.c index 6b4cd0e..5226fc0 100644 --- a/diff.c +++ b/diff.c @@ -4697,6 +4697,33 @@ static int diff_filespec_is_identical(struct diff_filespec *one, return !memcmp(one->data, two->data, one->size); } +static int diff_filespec_check_stat_unmatch(struct diff_filepair *p) +{ + /* + * 1. Entries that come from stat info dirtiness + * always have both sides (iow, not create/delete), + * one side of the object name is unknown, with + * the same mode and size. Keep the ones that + * do not match these criteria. They have real + * differences. + * + * 2. At this point, the file is known to be modified, + * with the same mode and size, and the object + * name of one side is unknown. Need to inspect + * the identical contents. + */ + if (!DIFF_FILE_VALID(p->one) || /* (1) */ + !DIFF_FILE_VALID(p->two) || + (p->one->sha1_valid && p->two->sha1_valid) || + (p->one->mode != p->two->mode) || + diff_populate_filespec(p->one, 1) || + diff_populate_filespec(p->two, 1) || + (p->one->size != p->two->size) || + !diff_filespec_is_identical(p->one, p->two)) /* (2) */ + return 1; + return 0; +} + static void diffcore_skip_stat_unmatch(struct diff_options *diffopt) { int i; @@ -4707,27 +4734,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt) for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - /* - * 1. Entries that come from stat info dirtiness - * always have both sides (iow, not create/delete), - * one side of the object name is unknown, with - * the same mode and size. Keep the ones that - * do not match these criteria. They have real - * differences. - * - * 2. At this point, the file is known to be modified, - * with the same mode and size, and the object - * name of one side is unknown. Need to inspect - * the identical contents. - */ - if (!DIFF_FILE_VALID(p->one) || /* (1) */ - !DIFF_FILE_VALID(p->two) || - (p->one->sha1_valid && p->two->sha1_valid) || - (p->one->mode != p->two->mode) || - diff_populate_filespec(p->one, 1) || - diff_populate_filespec(p->two, 1) || - (p->one->size != p->two->size) || - !diff_filespec_is_identical(p->one, p->two)) /* (2) */ + if (diff_filespec_check_stat_unmatch(p)) diff_q(&outq, p); else { /* @@ -4890,6 +4897,7 @@ void diff_change(struct diff_options *options, unsigned old_dirty_submodule, unsigned new_dirty_submodule) { struct diff_filespec *one, *two; + struct diff_filepair *p; if (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) && is_submodule_ignored(concatpath, options)) @@ -4916,10 +4924,17 @@ void diff_change(struct diff_options *options, fill_filespec(two, new_sha1, new_sha1_valid, new_mode); one->dirty_submodule = old_dirty_submodule; two->dirty_submodule = new_dirty_submodule; + p = diff_queue(&diff_queued_diff, one, two); - diff_queue(&diff_queued_diff, one, two); - if (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) - DIFF_OPT_SET(options, HAS_CHANGES); + if (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) + return; + + if (DIFF_OPT_TST(options, QUICK) && + options->skip_stat_unmatch && + !diff_filespec_check_stat_unmatch(p)) + return; + + DIFF_OPT_SET(options, HAS_CHANGES); } struct diff_filepair *diff_unmerge(struct diff_options *options, const char *path) -- 8< -- > Here's a obvious fix for this bug, but I think you can find a better > fix. Thanks in advance. > > > diff --git a/diff-lib.c b/diff-lib.c > index 346cac6..0b8c58d 100644 > --- a/diff-lib.c > +++ b/diff-lib.c > @@ -105,9 +105,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option) > int changed; > unsigned dirty_submodule = 0; > > - if (diff_can_quit_early(&revs->diffopt)) > - break; > - > if (!ce_path_match(ce, &revs->prune_data)) > continue; > > -- > IWAMOTO Toshihiro ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 1/3] Move diffcore_skip_stat_unmatch core logic out for reuse later 2014-01-23 2:45 bug with git-diff --quiet IWAMOTO Toshihiro 2014-01-25 4:03 ` Duy Nguyen @ 2014-01-25 6:46 ` Nguyễn Thái Ngọc Duy 2014-01-25 6:46 ` [PATCH 2/3] diff: do not quit early on stat-dirty files Nguyễn Thái Ngọc Duy 2014-01-25 6:46 ` [PATCH 3/3] diff: turn off skip_stat_unmatch on "diff --cached" Nguyễn Thái Ngọc Duy 1 sibling, 2 replies; 12+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2014-01-25 6:46 UTC (permalink / raw) To: git; +Cc: iwamoto, Junio C Hamano, Nguyễn Thái Ngọc Duy Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- diff.c | 49 ++++++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/diff.c b/diff.c index 6b4cd0e..19460ff 100644 --- a/diff.c +++ b/diff.c @@ -4697,6 +4697,33 @@ static int diff_filespec_is_identical(struct diff_filespec *one, return !memcmp(one->data, two->data, one->size); } +static int diff_filespec_check_stat_unmatch(struct diff_filepair *p) +{ + /* + * 1. Entries that come from stat info dirtiness + * always have both sides (iow, not create/delete), + * one side of the object name is unknown, with + * the same mode and size. Keep the ones that + * do not match these criteria. They have real + * differences. + * + * 2. At this point, the file is known to be modified, + * with the same mode and size, and the object + * name of one side is unknown. Need to inspect + * the identical contents. + */ + if (!DIFF_FILE_VALID(p->one) || /* (1) */ + !DIFF_FILE_VALID(p->two) || + (p->one->sha1_valid && p->two->sha1_valid) || + (p->one->mode != p->two->mode) || + diff_populate_filespec(p->one, 1) || + diff_populate_filespec(p->two, 1) || + (p->one->size != p->two->size) || + !diff_filespec_is_identical(p->one, p->two)) /* (2) */ + return 1; + return 0; +} + static void diffcore_skip_stat_unmatch(struct diff_options *diffopt) { int i; @@ -4707,27 +4734,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt) for (i = 0; i < q->nr; i++) { struct diff_filepair *p = q->queue[i]; - /* - * 1. Entries that come from stat info dirtiness - * always have both sides (iow, not create/delete), - * one side of the object name is unknown, with - * the same mode and size. Keep the ones that - * do not match these criteria. They have real - * differences. - * - * 2. At this point, the file is known to be modified, - * with the same mode and size, and the object - * name of one side is unknown. Need to inspect - * the identical contents. - */ - if (!DIFF_FILE_VALID(p->one) || /* (1) */ - !DIFF_FILE_VALID(p->two) || - (p->one->sha1_valid && p->two->sha1_valid) || - (p->one->mode != p->two->mode) || - diff_populate_filespec(p->one, 1) || - diff_populate_filespec(p->two, 1) || - (p->one->size != p->two->size) || - !diff_filespec_is_identical(p->one, p->two)) /* (2) */ + if (diff_filespec_check_stat_unmatch(p)) diff_q(&outq, p); else { /* -- 1.8.5.2.240.g8478abd ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] diff: do not quit early on stat-dirty files 2014-01-25 6:46 ` [PATCH 1/3] Move diffcore_skip_stat_unmatch core logic out for reuse later Nguyễn Thái Ngọc Duy @ 2014-01-25 6:46 ` Nguyễn Thái Ngọc Duy 2014-01-25 6:46 ` [PATCH 3/3] diff: turn off skip_stat_unmatch on "diff --cached" Nguyễn Thái Ngọc Duy 1 sibling, 0 replies; 12+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2014-01-25 6:46 UTC (permalink / raw) To: git; +Cc: iwamoto, Junio C Hamano, Nguyễn Thái Ngọc Duy When QUICK is set (i.e. with --quiet) we try to do as little work as possible, stopping after seeing the first change. stat-dirty is considered a "change" but it may turn out not, if no actual content is changed. The actual content test is performed too late in the process and the shortcut may be taken prematurely, leading to incorrect return code. Assume we do "git diff --quiet". If we have a stat-dirty file "a" and a really dirty file "b". We break the loop in run_diff_files() and stop after "a" because we have got a "change". Later in diffcore_skip_stat_unmatch() we find out "a" is actually not changed. But there's nothing else in the diff queue, we incorrectly declare "no change", ignoring the fact that "b" is changed. This also happens to "git diff --quiet HEAD" when it hits diff_can_quit_early() in oneway_diff(). This patch does the content test earlier in order to keep going if "a" is unchanged. The test result is cached so that when diffcore_skip_stat_unmatch() is done in the end, we spend no cycles on re-testing "a". Reported-by: IWAMOTO Toshihiro <iwamoto@valinux.co.jp> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- diff.c | 22 +++++++++++++++++----- diffcore.h | 2 ++ t/t4035-diff-quiet.sh | 6 ++++++ 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/diff.c b/diff.c index 19460ff..ab85f7e 100644 --- a/diff.c +++ b/diff.c @@ -4699,6 +4699,11 @@ static int diff_filespec_is_identical(struct diff_filespec *one, static int diff_filespec_check_stat_unmatch(struct diff_filepair *p) { + if (p->done_skip_stat_unmatch) + return p->skip_stat_unmatch_result; + + p->done_skip_stat_unmatch = 1; + p->skip_stat_unmatch_result = 0; /* * 1. Entries that come from stat info dirtiness * always have both sides (iow, not create/delete), @@ -4720,8 +4725,8 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p) diff_populate_filespec(p->two, 1) || (p->one->size != p->two->size) || !diff_filespec_is_identical(p->one, p->two)) /* (2) */ - return 1; - return 0; + p->skip_stat_unmatch_result = 1; + return p->skip_stat_unmatch_result; } static void diffcore_skip_stat_unmatch(struct diff_options *diffopt) @@ -4897,6 +4902,7 @@ void diff_change(struct diff_options *options, unsigned old_dirty_submodule, unsigned new_dirty_submodule) { struct diff_filespec *one, *two; + struct diff_filepair *p; if (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) && is_submodule_ignored(concatpath, options)) @@ -4923,10 +4929,16 @@ void diff_change(struct diff_options *options, fill_filespec(two, new_sha1, new_sha1_valid, new_mode); one->dirty_submodule = old_dirty_submodule; two->dirty_submodule = new_dirty_submodule; + p = diff_queue(&diff_queued_diff, one, two); - diff_queue(&diff_queued_diff, one, two); - if (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) - DIFF_OPT_SET(options, HAS_CHANGES); + if (DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) + return; + + if (DIFF_OPT_TST(options, QUICK) && options->skip_stat_unmatch && + !diff_filespec_check_stat_unmatch(p)) + return; + + DIFF_OPT_SET(options, HAS_CHANGES); } struct diff_filepair *diff_unmerge(struct diff_options *options, const char *path) diff --git a/diffcore.h b/diffcore.h index 1c16c85..6b538bc 100644 --- a/diffcore.h +++ b/diffcore.h @@ -70,6 +70,8 @@ struct diff_filepair { unsigned broken_pair : 1; unsigned renamed_pair : 1; unsigned is_unmerged : 1; + unsigned done_skip_stat_unmatch : 1; + unsigned skip_stat_unmatch_result : 1; }; #define DIFF_PAIR_UNMERGED(p) ((p)->is_unmerged) diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh index 231412d..e8ae2a0 100755 --- a/t/t4035-diff-quiet.sh +++ b/t/t4035-diff-quiet.sh @@ -148,4 +148,10 @@ test_expect_success 'git diff --ignore-all-space, both files outside repo' ' ) ' +test_expect_success 'git diff --quiet ignores stat-change only entries' ' + test-chmtime +10 a && + echo modified >>b && + test_expect_code 1 git diff --quiet +' + test_done -- 1.8.5.2.240.g8478abd ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] diff: turn off skip_stat_unmatch on "diff --cached" 2014-01-25 6:46 ` [PATCH 1/3] Move diffcore_skip_stat_unmatch core logic out for reuse later Nguyễn Thái Ngọc Duy 2014-01-25 6:46 ` [PATCH 2/3] diff: do not quit early on stat-dirty files Nguyễn Thái Ngọc Duy @ 2014-01-25 6:46 ` Nguyễn Thái Ngọc Duy 2014-01-27 22:59 ` [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively Nguyễn Thái Ngọc Duy 1 sibling, 1 reply; 12+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2014-01-25 6:46 UTC (permalink / raw) To: git; +Cc: iwamoto, Junio C Hamano, Nguyễn Thái Ngọc Duy skip_stat_unmatch flag is added in fb13227 (git-diff: squelch "empty" diffs - 2007-08-03) to ignore empty diffs caused by stat-only dirtiness. In "diff --cached" case, stat is not invovled at all. While the code is written in a way that no expensive I/O is done, we still need to move all file pairs from the old queue to the new queue in diffcore_skip_stat_unmatch(). Avoid it. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/diff.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/builtin/diff.c b/builtin/diff.c index 0f247d2..85f97d7 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -150,9 +150,10 @@ static int builtin_diff_index(struct rev_info *revs, perror("read_cache_preload"); return -1; } - } else if (read_cache() < 0) { - perror("read_cache"); - return -1; + } else { + if (read_cache() < 0) + return error("read_cache: %s", strerror(errno)); + revs->diffopt.skip_stat_unmatch = 0; } return run_diff_index(revs, cached); } -- 1.8.5.2.240.g8478abd ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively 2014-01-25 6:46 ` [PATCH 3/3] diff: turn off skip_stat_unmatch on "diff --cached" Nguyễn Thái Ngọc Duy @ 2014-01-27 22:59 ` Nguyễn Thái Ngọc Duy 2014-01-27 23:45 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2014-01-27 22:59 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy skip_stat_unmatch flag is added in fb13227 (git-diff: squelch "empty" diffs - 2007-08-03) to ignore empty diffs caused by stat-only dirtiness. In some diff case, stat is not involved at all. While the code is written in a way that no expensive I/O is done, we still need to move all file pairs from the old queue to the new queue in diffcore_skip_stat_unmatch(). Only enable it when worktree is involved: "diff" and "diff <rev>". This should help track down how skip_stat_unmatch is actually used when bugs occur. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- This replaces 'diff: turn off skip_stat_unmatch on "diff --cached"' The previous patch obviously leaves skip_stat_unmatch on in "diff <rev> <rev>" and maybe other cases. builtin/diff.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/diff.c b/builtin/diff.c index 0f247d2..88542d9 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -150,6 +150,7 @@ static int builtin_diff_index(struct rev_info *revs, perror("read_cache_preload"); return -1; } + revs->diffopt.skip_stat_unmatch = !!diff_auto_refresh_index; } else if (read_cache() < 0) { perror("read_cache"); return -1; @@ -252,6 +253,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv perror("read_cache_preload"); return -1; } + revs->diffopt.skip_stat_unmatch = !!diff_auto_refresh_index; return run_diff_files(revs, options); } @@ -343,7 +345,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix) diff_no_index(&rev, argc, argv, prefix); /* Otherwise, we are doing the usual "git" diff */ - rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index; /* Scale to real terminal size and respect statGraphWidth config */ rev.diffopt.stat_width = -1; -- 1.8.5.2.240.g8478abd ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively 2014-01-27 22:59 ` [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively Nguyễn Thái Ngọc Duy @ 2014-01-27 23:45 ` Junio C Hamano 2014-01-28 22:51 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2014-01-27 23:45 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > skip_stat_unmatch flag is added in fb13227 (git-diff: squelch "empty" > diffs - 2007-08-03) to ignore empty diffs caused by stat-only > dirtiness. In some diff case, stat is not involved at all. While > the code is written in a way that no expensive I/O is done, we still > need to move all file pairs from the old queue to the new queue in > diffcore_skip_stat_unmatch(). > > Only enable it when worktree is involved: "diff" and "diff <rev>". > This should help track down how skip_stat_unmatch is actually used > when bugs occur. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > This replaces 'diff: turn off skip_stat_unmatch on "diff --cached"' > The previous patch obviously leaves skip_stat_unmatch on in "diff > <rev> <rev>" and maybe other cases. Oops, I lost track. Sorry. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively 2014-01-27 23:45 ` Junio C Hamano @ 2014-01-28 22:51 ` Junio C Hamano 2014-01-28 23:52 ` Duy Nguyen 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2014-01-28 22:51 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > >> skip_stat_unmatch flag is added in fb13227 (git-diff: squelch "empty" >> diffs - 2007-08-03) to ignore empty diffs caused by stat-only >> dirtiness. In some diff case, stat is not involved at all. While >> the code is written in a way that no expensive I/O is done, we still >> need to move all file pairs from the old queue to the new queue in >> diffcore_skip_stat_unmatch(). >> >> Only enable it when worktree is involved: "diff" and "diff <rev>". >> This should help track down how skip_stat_unmatch is actually used >> when bugs occur. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> >> --- >> This replaces 'diff: turn off skip_stat_unmatch on "diff --cached"' >> The previous patch obviously leaves skip_stat_unmatch on in "diff >> <rev> <rev>" and maybe other cases. > > Oops, I lost track. Sorry. Together with {1,2}/3 applied on maint-1.8.4, this sems to break t3417 (there may be others, but I didn't have time to check). ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively 2014-01-28 22:51 ` Junio C Hamano @ 2014-01-28 23:52 ` Duy Nguyen 2014-01-29 19:25 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Duy Nguyen @ 2014-01-28 23:52 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Tue, Jan 28, 2014 at 02:51:45PM -0800, Junio C Hamano wrote: > >> This replaces 'diff: turn off skip_stat_unmatch on "diff --cached"' > >> The previous patch obviously leaves skip_stat_unmatch on in "diff > >> <rev> <rev>" and maybe other cases. > > > > Oops, I lost track. Sorry. > > Together with {1,2}/3 applied on maint-1.8.4, this sems to break > t3417 (there may be others, but I didn't have time to check). My bad. I thought I covered all cases in my last patch (and didn't retest it!). It turns out I should have set skip_stat_unmatch in builtin_diff_b_f() too. This on top of 3/3 passes the tests -- 8< -- diff --git a/builtin/diff.c b/builtin/diff.c index 88542d9..8ab5e3d 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -89,6 +89,7 @@ static int builtin_diff_b_f(struct rev_info *revs, if (blob[0].mode == S_IFINVALID) blob[0].mode = canon_mode(st.st_mode); + revs->diffopt.skip_stat_unmatch = !!diff_auto_refresh_index; stuff_change(&revs->diffopt, blob[0].mode, canon_mode(st.st_mode), blob[0].sha1, null_sha1, -- 8< -- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively 2014-01-28 23:52 ` Duy Nguyen @ 2014-01-29 19:25 ` Junio C Hamano 2014-01-30 5:36 ` Duy Nguyen 0 siblings, 1 reply; 12+ messages in thread From: Junio C Hamano @ 2014-01-29 19:25 UTC (permalink / raw) To: Duy Nguyen; +Cc: git Duy Nguyen <pclouds@gmail.com> writes: > On Tue, Jan 28, 2014 at 02:51:45PM -0800, Junio C Hamano wrote: >> >> This replaces 'diff: turn off skip_stat_unmatch on "diff --cached"' >> >> The previous patch obviously leaves skip_stat_unmatch on in "diff >> >> <rev> <rev>" and maybe other cases. >> > >> > Oops, I lost track. Sorry. >> >> Together with {1,2}/3 applied on maint-1.8.4, this sems to break >> t3417 (there may be others, but I didn't have time to check). > > My bad. I thought I covered all cases in my last patch (and didn't > retest it!). It turns out I should have set skip_stat_unmatch in > builtin_diff_b_f() too. This on top of 3/3 passes the tests Thanks, will squash it in. This however shows that the existing test *KNEW* that it was enough to check just a few cases (especially, there is no reason to make sure that blob vs file-in-working-tree case behaves sanely), because the auto-refresh would kick in for all codepaths. Now you are making that assumption invalid, shouldn't the patch also split the tests to cover individual cases? > -- 8< -- > diff --git a/builtin/diff.c b/builtin/diff.c > index 88542d9..8ab5e3d 100644 > --- a/builtin/diff.c > +++ b/builtin/diff.c > @@ -89,6 +89,7 @@ static int builtin_diff_b_f(struct rev_info *revs, > if (blob[0].mode == S_IFINVALID) > blob[0].mode = canon_mode(st.st_mode); > > + revs->diffopt.skip_stat_unmatch = !!diff_auto_refresh_index; > stuff_change(&revs->diffopt, > blob[0].mode, canon_mode(st.st_mode), > blob[0].sha1, null_sha1, > -- 8< -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively 2014-01-29 19:25 ` Junio C Hamano @ 2014-01-30 5:36 ` Duy Nguyen 2014-01-31 16:17 ` Junio C Hamano 0 siblings, 1 reply; 12+ messages in thread From: Duy Nguyen @ 2014-01-30 5:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List On Thu, Jan 30, 2014 at 2:25 AM, Junio C Hamano <gitster@pobox.com> wrote: >> On Tue, Jan 28, 2014 at 02:51:45PM -0800, Junio C Hamano wrote: > This however shows that the existing test *KNEW* that it was enough > to check just a few cases (especially, there is no reason to make > sure that blob vs file-in-working-tree case behaves sanely), because > the auto-refresh would kick in for all codepaths. Now you are > making that assumption invalid, shouldn't the patch also split the > tests to cover individual cases? Drop the last patch, then. It's a "while at there" cleanup patch. If it's non trivial then it could be taken up later when somebody's interested. I have a few topics ongoing and not sure I'll go through diff.c to identify and write tests for all cases. -- Duy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively 2014-01-30 5:36 ` Duy Nguyen @ 2014-01-31 16:17 ` Junio C Hamano 0 siblings, 0 replies; 12+ messages in thread From: Junio C Hamano @ 2014-01-31 16:17 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List Duy Nguyen <pclouds@gmail.com> writes: > On Thu, Jan 30, 2014 at 2:25 AM, Junio C Hamano <gitster@pobox.com> wrote: >>> On Tue, Jan 28, 2014 at 02:51:45PM -0800, Junio C Hamano wrote: >> This however shows that the existing test *KNEW* that it was enough >> to check just a few cases (especially, there is no reason to make >> sure that blob vs file-in-working-tree case behaves sanely), because >> the auto-refresh would kick in for all codepaths. Now you are >> making that assumption invalid, shouldn't the patch also split the >> tests to cover individual cases? > > Drop the last patch, then. It's a "while at there" cleanup patch. If > it's non trivial then it could be taken up later... I am leaning towards that because... > ... not sure I'll go through > diff.c to identify and write tests for all cases. ... the effort to ensure the correctness of the patch itself involves the same identification of the cases. We know the single place skip-stat-unmatch was assigned used to cover all cases, and the patch was to stop covering cases the unnecessary assignments are made while making sure the resulting code still covers cases that assignments are necessary. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-01-31 16:17 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-01-23 2:45 bug with git-diff --quiet IWAMOTO Toshihiro 2014-01-25 4:03 ` Duy Nguyen 2014-01-25 6:46 ` [PATCH 1/3] Move diffcore_skip_stat_unmatch core logic out for reuse later Nguyễn Thái Ngọc Duy 2014-01-25 6:46 ` [PATCH 2/3] diff: do not quit early on stat-dirty files Nguyễn Thái Ngọc Duy 2014-01-25 6:46 ` [PATCH 3/3] diff: turn off skip_stat_unmatch on "diff --cached" Nguyễn Thái Ngọc Duy 2014-01-27 22:59 ` [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively Nguyễn Thái Ngọc Duy 2014-01-27 23:45 ` Junio C Hamano 2014-01-28 22:51 ` Junio C Hamano 2014-01-28 23:52 ` Duy Nguyen 2014-01-29 19:25 ` Junio C Hamano 2014-01-30 5:36 ` Duy Nguyen 2014-01-31 16:17 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.