Re: [PATCH v3 3/6] diff: allow --patch & cie to override -s/--no-patch
Matthieu Moy wrote:
> Jonathan Nieder writes:
>> 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.
Ah, so you mean: "The case of --binary deserves extra attention
because ...".
"is particular" would be an unusual expression, meaning something like
"is made of particles". It's a weird case in English usage where a
word commonly appears attached to a noun ("This particular case") but
cannot be used as the RHS of "is" ("This case is particular").
[...]
>> 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 ...
Sure. The point of "--patch --no-patch" is to test that ordering is
respected. The point of "--no-patch --patch-with-stat" is so we
remember that there are options other than --patch that should
override --no-patch, for example if this code is ever converted to
parse_options some day.
> I'll send a reroll tomorrow.
Thanks for the quick and thoughtful work.
--
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 v3 3/6] diff: allow --patch & cie to override -s/--no-patch
Jonathan Nieder writes: > 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. >> --- 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/ -- 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 v3 3/6] diff: allow --patch & cie to override -s/--no-patch
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" or "In the case of --binary in particular, the name may suggest ...". > a normal patch into a binary patch, but it actually already enables patch > output when normally disabled (e.g. "git log --binary" displays a patch), > hence it makes sense that "git show --no-patch --binary" display the > binary patch. [...] > --- 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"? A couple of other test ideas: - "git diff-files --patch --no-patch" - "git diff-files -s --patch-with-stat" Aside from that, the patch looks good. Thanks, Jonathan -- 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
