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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 29, 2014, 19:25 UTC
Message-ID: <xmqq61p2k2u5.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140128235203.GA7788@lanh>

Duy Nguyen <pclouds@gmail.com> writes:

Show 13 quoted lines

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

Show 14 quoted lines

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