Re: [PATCH v3 3/6] diff: allow --patch & cie to override -s/--no-patch

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 15, 2013, 18:23 UTC
Message-ID: <vpqvc4badhf.fsf@anie.imag.fr>
In-Reply-To: <20130715180908.GE14690@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 7 quoted lines

> Matthieu Moy wrote:
>
>> All options that trigger a patch output now override --no-patch.
>>
>> The case of --binary is particular as the name may suggest that it turns
>
> Usage nit: this should say "is unusual"

I don't get it. The point is not that --binary is unusual, but that it is a particular case that deserves extra attention.

> or "In the case of --binary in particular, the name may suggest ...".

Not really. I'd use "in particular" if --binary was a sub-case of the others, but here I'm precisely saying that it may not be.

Show 11 quoted lines

>> --- a/t/t4000-diff-format.sh
>> +++ b/t/t4000-diff-format.sh
>> @@ -71,4 +71,9 @@ test_expect_success 'git diff-files --no-patch as synonym for -s' '
>>  	test_must_be_empty err
>>  '
>>  
>> +test_expect_success 'git diff-files --no-patch --patch shows the patch' '
>> +	git diff-files --no-patch --patch >diff-np-output 2>err &&
>> +	compare_diff_patch expected actual
>
> Shouldn't that be "compare_diff_patch expected diff-np-output"?

Oops, right.

> A couple of other test ideas:
>
>  - "git diff-files --patch --no-patch"
>  - "git diff-files -s --patch-with-stat"

I'd rather avoid having a too long list here, or we'll end-up testing all combinations of options ...

I'll send a reroll tomorrow.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/