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 2err + 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
Re: [PATCH v3 3/6] diff: allow --patch cie to override -s/--no-patch
Jonathan Nieder jrnie...@gmail.com 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 2err +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: Jonathan Nieder jrnie...@gmail.com 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
