bug with git-diff --quiet

bug with git-diff --quiet

[IWAMOTO Toshihiro]

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

Re: bug with git-diff --quiet

[Duy Nguyen]

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

[PATCH 1/3] Move diffcore_skip_stat_unmatch core logic out for reuse later

[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

[PATCH 2/3] diff: do not quit early on stat-dirty files

[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

[PATCH 3/3] diff: turn off skip_stat_unmatch on "diff --cached"

[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

[PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively

[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

Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively

[Junio C Hamano]

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.

Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively

[Junio C Hamano]

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).

Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively

[Duy Nguyen]

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< --

Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively

[Junio C Hamano]

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< --

Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively

[Duy Nguyen]

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

Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively

[Junio C Hamano]

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.