Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively
Duy Nguyen pclo...@gmail.com writes: On Thu, Jan 30, 2014 at 2:25 AM, Junio C Hamano gits...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively
Duy Nguyen pclo...@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 --
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively
Junio C Hamano gits...@pobox.com writes:
Nguyễn Thái Ngọc Duy pclo...@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 pclo...@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).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively
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 --
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively
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 pclo...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively
Nguyễn Thái Ngọc Duy pclo...@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 pclo...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
