diff: do not quit early on stat-dirty files
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
parent
fceb907225
commit
f34b205f6c
20
diff.c
20
diff.c
@ -4594,6 +4594,11 @@ static int diff_filespec_is_identical(struct diff_filespec *one,
|
|||||||
|
|
||||||
static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)
|
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
|
* 1. Entries that come from stat info dirtiness
|
||||||
* always have both sides (iow, not create/delete),
|
* always have both sides (iow, not create/delete),
|
||||||
@ -4615,8 +4620,8 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p)
|
|||||||
diff_populate_filespec(p->two, 1) ||
|
diff_populate_filespec(p->two, 1) ||
|
||||||
(p->one->size != p->two->size) ||
|
(p->one->size != p->two->size) ||
|
||||||
!diff_filespec_is_identical(p->one, p->two)) /* (2) */
|
!diff_filespec_is_identical(p->one, p->two)) /* (2) */
|
||||||
return 1;
|
p->skip_stat_unmatch_result = 1;
|
||||||
return 0;
|
return p->skip_stat_unmatch_result;
|
||||||
}
|
}
|
||||||
|
|
||||||
static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)
|
static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)
|
||||||
@ -4792,6 +4797,7 @@ void diff_change(struct diff_options *options,
|
|||||||
unsigned old_dirty_submodule, unsigned new_dirty_submodule)
|
unsigned old_dirty_submodule, unsigned new_dirty_submodule)
|
||||||
{
|
{
|
||||||
struct diff_filespec *one, *two;
|
struct diff_filespec *one, *two;
|
||||||
|
struct diff_filepair *p;
|
||||||
|
|
||||||
if (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) &&
|
if (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) &&
|
||||||
is_submodule_ignored(concatpath, options))
|
is_submodule_ignored(concatpath, options))
|
||||||
@ -4818,9 +4824,15 @@ void diff_change(struct diff_options *options,
|
|||||||
fill_filespec(two, new_sha1, new_sha1_valid, new_mode);
|
fill_filespec(two, new_sha1, new_sha1_valid, new_mode);
|
||||||
one->dirty_submodule = old_dirty_submodule;
|
one->dirty_submodule = old_dirty_submodule;
|
||||||
two->dirty_submodule = new_dirty_submodule;
|
two->dirty_submodule = new_dirty_submodule;
|
||||||
|
p = diff_queue(&diff_queued_diff, one, two);
|
||||||
|
|
||||||
|
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_queue(&diff_queued_diff, one, two);
|
|
||||||
if (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
|
|
||||||
DIFF_OPT_SET(options, HAS_CHANGES);
|
DIFF_OPT_SET(options, HAS_CHANGES);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -70,6 +70,8 @@ struct diff_filepair {
|
|||||||
unsigned broken_pair : 1;
|
unsigned broken_pair : 1;
|
||||||
unsigned renamed_pair : 1;
|
unsigned renamed_pair : 1;
|
||||||
unsigned is_unmerged : 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)
|
#define DIFF_PAIR_UNMERGED(p) ((p)->is_unmerged)
|
||||||
|
|
||||||
|
@ -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
|
test_done
|
||||||
|
Loading…
Reference in New Issue
Block a user