Bug in "git show"?

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

so I wanted to write a script using some git commands, but the output of the git commands is not as expected. I am using

git --version
git version 1.8.3.2.804.g0da7a53
(current origin/master at git://github.com/gitster/git.git)

The command I am trying to use is
git --no-pager show --format="%ad" <commit>

Expected output:

sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53^
Fri Jul 12 12:04:19 2013 -0700

sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53^^
Fri Jul 12 12:04:17 2013 -0700

sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53^^^
Fri Jul 12 12:04:16 2013 -0700

sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53^^^^
Fri Jul 12 12:04:14 2013 -0700

sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53^^^^^
Fri Jul 12 12:04:12 2013 -0700

sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53^^^^^^
Fri Jul 12 12:04:10 2013 -0700

sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53^^^^^^^
Fri Jul 12 12:04:09 2013 -0700

sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53^^^^^^^^
Fri Jul 12 12:04:07 2013 -0700

However I sometimes also get:

sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53
Fri Jul 12 10:49:34 2013 -0700

b/Documentation/RelNotes/1.8.4.txt
index 0e50df8..4250e5a 100644
- --- a/Documentation/RelNotes/1.8.4.txt
+++ b/Documentation/RelNotes/1.8.4.txt
@@ -79,6 +79,13 @@ Foreign interfaces, subsystems and ports.

UI, Workflows & Features

+ * "gitweb" learned to optionally place extra links that point at the
+ levels higher than the Gitweb pages themselves in the breadcrumbs,
+ so that it can be used as part of a larger installation.
+
+ * "git log --format=" now honors i18n.logoutputencoding configuration
+ variable.
+
* The "push.default=simple" mode of "git push" has been updated to behave like "current" without requiring a remote tracking information, when you push to a remote that is different from where

sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53^^^^^^^^
Fri Jul 12 12:04:07 2013 -0700

diff --cc t/t5505-remote.sh
index ee5d65d,8d0f3e3..8f6e392

- --- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@@ -61,25 -62,25 +61,25 @@@ test_expect_success C_LOCALE_OUTPUT 're
'

test_expect_success 'add another remote' '
-(

 -      cd test &&
 -      git remote add -f second ../two &&
 -      tokens_match "origin second" "$(git remote)" &&
 -      check_tracking_branch second master side another &&
 -      git for-each-ref "--format=%(refname)" refs/remotes |
 -      sed -e "/^refs\/remotes\/origin\//d" \

- -e "/^refs\/remotes\/second\//d" >actual &&
- >expect &&

So at some commits, also the diff is shown, which should not happen if you're using --format="%ad" to my understanding.

So far it seems to be deterministic here. (Each commit either always behaves correctly or incorrectly). Initially I suspected it being different for merges or no-merges, but I gut the unexpected behavior for both merge and non-merge commits.

Can somebody confirm this behavior or has any idea, whether I am doing something wrong here?

Thanks,
Stefan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with undefined - http://www.enigmail.net/

iQIcBAEBAgAGBQJR49xHAAoJEJQCPTzLflhqiFcP/0S5M71urjn1Fh5Cz2950wBl cs6+r9wKF5366wv3Ombfh0KrOPQCE7Yv1GWk2r2L008BWrz0wgnsl1xnMnAT00dH 1WzmupWExxwXHAueDisq2qL4dubFrnVxKWvpwxBd3JBbsWNXeTGl5o6dl69zaXdx JHzIHvw1/vrxoCLaflitjcQezuFITOVVoNsYawK12gznjxujONm0sej5TBFUw784 K5KTJNJqxzUf9+Z+88hg2oif7kJlugTIqtH5sRMVwXrkpc12f+HcwROg0srE5ITc 8WpK0s7xgZokUCohMhUXlLAOYJwAZju+K1LExkrQ9T32oP4iAKsJpqcRLAAX6Ig7 OeaWUQ2WX2CfYDExjuV6h+FXAU0qT87iv3PgeZWAnmgDQPUwFboIxuF1Nrpq/FOY Ioe2YOsFOdhmuDjCHEfu0aVNOeejHS8LEkC1IUI/+PzDlEJC/b17SwEdrD4aVCPj RZhz2zI8ZxYHP9ITvMDs1VQRP6jMSAwEtAWg7ac7ypETToOIAxRp2j9Rrjayt8A5 /TZ++wMh7G4Tm/D9+iutQnqzE8E8eiT6i7LgAcDSA3g6oq7/hUzupKU2lc+znO2J wxiwdqYBHWtw1Jc8eaZAJC/NMfxbRzOSCx4lJbO+tpXP0pR/OPIg2f2wUXcAh3zH JC9GdEUVIwN34j2ueoZ8
=G6Oc
-----END PGP SIGNATURE-----

... and this "git show -s" is extraordinarily hard to discover, as it is only documented in "git log --help". Google has been my friend here, but we should really improve that.

This patch series does essentially two things:

* Add a --no-patch synonym for -s. I'm actually wondering why the option wasn't called this way from the beginning.

* Reorganize the doc so that "git show" actually mentions it.

While we're there, there's a reformatting patch, and one to make "git log --no-patch --patch" actually show the patch.

(60aa9cf8f3, Documentation: document show -s, Tue Nov 9 11:12:48 2010 actually acknowledged that the documentation wasn't complete and that something else was needed, this should be it)

Matthieu Moy (5):
diff: allow --no-patch as synonym for -s
diff: allow --patch to override -s/--no-patch Documentation/git-show.txt: include common diff options, like
git-log.txt
Documentation: move description of -s, --no-patch to diff-options.txt Documentation/git-log.txt: capitalize section names

 Documentation/diff-options.txt     | 5 +++++
 Documentation/git-log.txt          | 8 ++++----
 Documentation/git-show.txt         | 9 +++++++++
 Documentation/rev-list-options.txt | 3 ---
 diff.c                             | 7 ++++---
 5 files changed, 22 insertions(+), 10 deletions(-)

1.8.3.1.495.g13f33cf.dirty

This follows the usual convention of having a --no-foo option to negate --foo.

---
 Documentation/rev-list-options.txt | 1 +
 diff.c                             | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

index e157ec3..c128a85 100644
@@ -851,5 +851,6 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 	Show the tree objects in the diff output. This implies '-r'.
 
 -s::
+--no-patch::
 	Suppress diff output.
 endif::git-rev-list[]

index 6578690..6bd821d 100644
@@ -3551,7 +3551,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_NAME;
 	else if (!strcmp(arg, "--name-status"))
 		options->output_format |= DIFF_FORMAT_NAME_STATUS;
-	else if (!strcmp(arg, "-s"))
+	else if (!strcmp(arg, "-s") || !strcmp(arg, "--no-patch"))
 		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
 	else if (!prefixcmp(arg, "--stat"))
 		/* --stat, --stat-width, --stat-name-width, or --stat-count */

1.8.3.1.495.g13f33cf.dirty

---
 diff.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 6bd821d..66a6877 100644
@@ -3515,9 +3515,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	int argcount;
 
 	/* Output format options */
-	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch"))
+	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch")) {
+		options->output_format &= ~DIFF_FORMAT_NO_OUTPUT;
 		options->output_format |= DIFF_FORMAT_PATCH;
-	else if (opt_arg(arg, 'U', "unified", &options->context))
+	} else if (opt_arg(arg, 'U', "unified", &options->context))
 		options->output_format |= DIFF_FORMAT_PATCH;
 	else if (!strcmp(arg, "--raw"))
 		options->output_format |= DIFF_FORMAT_RAW;

1.8.3.1.495.g13f33cf.dirty

No, it isn't. I just didn't notice the -U case.

Yes, this seems to be a better way.

There are other cases like --patch-with-raw, I'll send a reroll.

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

Thanks.

This fixes the issue found by Junio where "git log --no-patch -u" was showing the patch, but not "git log --no-patch -U8". Other patches are unmodified.

Matthieu Moy (5):
diff: allow --no-patch as synonym for -s
diff: allow --patch & cie to override -s/--no-patch Documentation/git-show.txt: include common diff options, like
git-log.txt
Documentation: move description of -s, --no-patch to diff-options.txt Documentation/git-log.txt: capitalize section names

 Documentation/diff-options.txt     |  5 +++++
 Documentation/git-log.txt          |  8 ++++----
 Documentation/git-show.txt         |  9 +++++++++
 Documentation/rev-list-options.txt |  3 ---
 diff.c                             | 30 ++++++++++++++++++------------
 5 files changed, 36 insertions(+), 19 deletions(-)

1.8.3.1.495.g13f33cf.dirty

This follows the usual convention of having a --no-foo option to negate --foo.

---
 Documentation/rev-list-options.txt | 1 +
 diff.c                             | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

index e157ec3..c128a85 100644
@@ -851,5 +851,6 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 	Show the tree objects in the diff output. This implies '-r'.
 
 -s::
+--no-patch::
 	Suppress diff output.
 endif::git-rev-list[]

index 6578690..6bd821d 100644
@@ -3551,7 +3551,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_NAME;
 	else if (!strcmp(arg, "--name-status"))
 		options->output_format |= DIFF_FORMAT_NAME_STATUS;
-	else if (!strcmp(arg, "-s"))
+	else if (!strcmp(arg, "-s") || !strcmp(arg, "--no-patch"))
 		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
 	else if (!prefixcmp(arg, "--stat"))
 		/* --stat, --stat-width, --stat-name-width, or --stat-count */

1.8.3.1.495.g13f33cf.dirty

Compared to v2, I just added tests. Strongly inspired from Jonathan's, but there's one more, and I chose the "modern" indentation style (hence a clean-up patch before, to avoid mixed-style in the same file).

Matthieu Moy (6):
t4000-diff-format.sh: modernize style
diff: allow --no-patch as synonym for -s
diff: allow --patch & cie to override -s/--no-patch Documentation/git-show.txt: include common diff options, like
git-log.txt
Documentation: move description of -s, --no-patch to diff-options.txt Documentation/git-log.txt: capitalize section names

 Documentation/diff-options.txt     |  5 +++++
 Documentation/git-log.txt          |  8 ++++----
 Documentation/git-show.txt         |  9 +++++++++
 Documentation/rev-list-options.txt |  3 ---
 diff.c                             | 30 ++++++++++++++++++------------
 t/t4000-diff-format.sh             | 35 ++++++++++++++++++++++++++---------
 6 files changed, 62 insertions(+), 28 deletions(-)

1.8.3.1.495.g13f33cf.dirty

---
 t/t4000-diff-format.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

index 6ddd469..2b5dffc 100755
@@ -15,17 +15,17 @@ line 3'
 cat path0 >path1
 chmod +x path1
 
-test_expect_success \
-    'update-index --add two files with and without +x.' \
-    'git update-index --add path0 path1'
+test_expect_success 'update-index --add two files with and without +x.' '
+	git update-index --add path0 path1
+'
 
 mv path0 path0-
 sed -e 's/line/Line/' <path0- >path0
 chmod +x path0
 rm -f path1
-test_expect_success \
-    'git diff-files -p after editing work tree.' \
-    'git diff-files -p >current'
+test_expect_success 'git diff-files -p after editing work tree.' '
+	git diff-files -p >actual
+'
 
 # that's as far as it comes
 if [ "$(git config --get core.filemode)" = false ]

@@ -55,8 +55,8 @@ deleted file mode 100755
 -line 3
 EOF
 
-test_expect_success \
-    'validate git diff-files -p output.' \
-    'compare_diff_patch current expected'
+test_expect_success 'validate git diff-files -p output.' '
+	compare_diff_patch expected actual
+'
 
 test_done

1.8.3.1.495.g13f33cf.dirty

This test script can use more cleanup, but as preparation for later patches in this series the above is enough. :) If I forget to do more cleanup as a followup, feel free to kick me.

This follows the usual convention of having a --no-foo option to negate --foo.

---
 Documentation/rev-list-options.txt |  1 +
 diff.c                             |  2 +-
 t/t4000-diff-format.sh             | 12 ++++++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

index e157ec3..c128a85 100644
@@ -851,5 +851,6 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 	Show the tree objects in the diff output. This implies '-r'.
 
 -s::
+--no-patch::
 	Suppress diff output.
 endif::git-rev-list[]

index 6578690..6bd821d 100644
@@ -3551,7 +3551,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_NAME;
 	else if (!strcmp(arg, "--name-status"))
 		options->output_format |= DIFF_FORMAT_NAME_STATUS;
-	else if (!strcmp(arg, "-s"))
+	else if (!strcmp(arg, "-s") || !strcmp(arg, "--no-patch"))
 		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
 	else if (!prefixcmp(arg, "--stat"))
 		/* --stat, --stat-width, --stat-name-width, or --stat-count */

index 2b5dffc..d702575 100755
@@ -59,4 +59,16 @@ test_expect_success 'validate git diff-files -p output.' '
 	compare_diff_patch expected actual
 '
 
+test_expect_success 'git diff-files -s after editing work tree' '
+	git diff-files -s >diff-s-output 2>err &&
+	test_must_be_empty diff-s-output &&
+	test_must_be_empty err
+'
+
+test_expect_success 'git diff-files --no-patch as synonym for -s' '
+	git diff-files --no-patch >diff-np-output 2>err &&
+	test_must_be_empty diff-np-output &&
+	test_must_be_empty err
+'
+
 test_done

1.8.3.1.495.g13f33cf.dirty

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

---
 diff.c                 | 28 +++++++++++++++++-----------
 t/t4000-diff-format.sh |  5 +++++
 2 files changed, 22 insertions(+), 11 deletions(-)

index 6bd821d..66ab714 100644
@@ -3508,6 +3508,11 @@ static int parse_submodule_opt(struct diff_options *options, const char *value)
 	return 1;
 }
 
+static void enable_patch_output(int *fmt) {
+	*fmt &= ~DIFF_FORMAT_NO_OUTPUT;
+	*fmt |= DIFF_FORMAT_PATCH;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];

@@ -3515,15 +3520,15 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	int argcount;
 
 	/* Output format options */
-	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch"))
-		options->output_format |= DIFF_FORMAT_PATCH;
-	else if (opt_arg(arg, 'U', "unified", &options->context))
-		options->output_format |= DIFF_FORMAT_PATCH;
+	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch")
+	    || opt_arg(arg, 'U', "unified", &options->context))
+		enable_patch_output(&options->output_format);
 	else if (!strcmp(arg, "--raw"))
 		options->output_format |= DIFF_FORMAT_RAW;
-	else if (!strcmp(arg, "--patch-with-raw"))
-		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW;
-	else if (!strcmp(arg, "--numstat"))
+	else if (!strcmp(arg, "--patch-with-raw")) {
+		enable_patch_output(&options->output_format);
+		options->output_format |= DIFF_FORMAT_RAW;
+	} else if (!strcmp(arg, "--numstat"))
 		options->output_format |= DIFF_FORMAT_NUMSTAT;
 	else if (!strcmp(arg, "--shortstat"))
 		options->output_format |= DIFF_FORMAT_SHORTSTAT;

@@ -3545,9 +3550,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_CHECKDIFF;
 	else if (!strcmp(arg, "--summary"))
 		options->output_format |= DIFF_FORMAT_SUMMARY;
-	else if (!strcmp(arg, "--patch-with-stat"))
-		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT;
-	else if (!strcmp(arg, "--name-only"))
+	else if (!strcmp(arg, "--patch-with-stat")) {
+		enable_patch_output(&options->output_format);
+		options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	} else if (!strcmp(arg, "--name-only"))
 		options->output_format |= DIFF_FORMAT_NAME;
 	else if (!strcmp(arg, "--name-status"))
 		options->output_format |= DIFF_FORMAT_NAME_STATUS;

@@ -3624,7 +3630,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 
 	/* flags options */
 	else if (!strcmp(arg, "--binary")) {
-		options->output_format |= DIFF_FORMAT_PATCH;
+		enable_patch_output(&options->output_format);
 		DIFF_OPT_SET(options, BINARY);
 	}
 	else if (!strcmp(arg, "--full-index"))

index d702575..4fce12f 100755
@@ -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
+'
+
 test_done

1.8.3.1.495.g13f33cf.dirty

Usage nit: this should say "is unusual" or "In the case of --binary in particular, the name may suggest ...".

[...]

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

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.

Oops, right.

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/

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").

[...]

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.

Thanks for the quick and thoughtful work.

One real fix since v3: there was an incorrect file name in a test (noticed by Jonathan). Now, all actual outputs are called "actual".

Other than that: extra tests, and style fixes in commit messages.

Matthieu Moy (6):
t4000-diff-format.sh: modernize style
diff: allow --no-patch as synonym for -s
diff: allow --patch & cie to override -s/--no-patch Documentation/git-show.txt: include common diff options, like
git-log.txt
Documentation: move description of -s, --no-patch to diff-options.txt Documentation/git-log.txt: capitalize section names

 Documentation/diff-options.txt     |  5 ++++
 Documentation/git-log.txt          |  8 +++----
 Documentation/git-show.txt         |  9 +++++++
 Documentation/rev-list-options.txt |  3 ---
 diff.c                             | 30 ++++++++++++++----------
 t/t4000-diff-format.sh             | 48 +++++++++++++++++++++++++++++++-------
 6 files changed, 75 insertions(+), 28 deletions(-)

1.8.3.1.495.g13f33cf.dirty

---
 t/t4000-diff-format.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

index 6ddd469..2b5dffc 100755
@@ -15,17 +15,17 @@ line 3'
 cat path0 >path1
 chmod +x path1
 
-test_expect_success \
-    'update-index --add two files with and without +x.' \
-    'git update-index --add path0 path1'
+test_expect_success 'update-index --add two files with and without +x.' '
+	git update-index --add path0 path1
+'
 
 mv path0 path0-
 sed -e 's/line/Line/' <path0- >path0
 chmod +x path0
 rm -f path1
-test_expect_success \
-    'git diff-files -p after editing work tree.' \
-    'git diff-files -p >current'
+test_expect_success 'git diff-files -p after editing work tree.' '
+	git diff-files -p >actual
+'
 
 # that's as far as it comes
 if [ "$(git config --get core.filemode)" = false ]

@@ -55,8 +55,8 @@ deleted file mode 100755
 -line 3
 EOF
 
-test_expect_success \
-    'validate git diff-files -p output.' \
-    'compare_diff_patch current expected'
+test_expect_success 'validate git diff-files -p output.' '
+	compare_diff_patch expected actual
+'
 
 test_done

1.8.3.1.495.g13f33cf.dirty

This follows the usual convention of having a --no-foo option to negate --foo.

---
 Documentation/rev-list-options.txt |  1 +
 diff.c                             |  2 +-
 t/t4000-diff-format.sh             | 12 ++++++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

index e157ec3..c128a85 100644
@@ -851,5 +851,6 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 	Show the tree objects in the diff output. This implies '-r'.
 
 -s::
+--no-patch::
 	Suppress diff output.
 endif::git-rev-list[]

index 6578690..6bd821d 100644
@@ -3551,7 +3551,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_NAME;
 	else if (!strcmp(arg, "--name-status"))
 		options->output_format |= DIFF_FORMAT_NAME_STATUS;
-	else if (!strcmp(arg, "-s"))
+	else if (!strcmp(arg, "-s") || !strcmp(arg, "--no-patch"))
 		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
 	else if (!prefixcmp(arg, "--stat"))
 		/* --stat, --stat-width, --stat-name-width, or --stat-count */

index 2b5dffc..3b9a9ae 100755
@@ -59,4 +59,16 @@ test_expect_success 'validate git diff-files -p output.' '
 	compare_diff_patch expected actual
 '
 
+test_expect_success 'git diff-files -s after editing work tree' '
+	git diff-files -s >actual 2>err &&
+	test_must_be_empty actual &&
+	test_must_be_empty err
+'
+
+test_expect_success 'git diff-files --no-patch as synonym for -s' '
+	git diff-files --no-patch >actual 2>err &&
+	test_must_be_empty actual &&
+	test_must_be_empty err
+'
+
 test_done

1.8.3.1.495.g13f33cf.dirty

All options that trigger a patch output now override --no-patch.

The case of --binary deserves extra attention: the name may suggest that it turns 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 for "git show --no-patch --binary" to display the binary patch.

---
 diff.c                 | 28 +++++++++++++++++-----------
 t/t4000-diff-format.sh | 18 ++++++++++++++++++
 2 files changed, 35 insertions(+), 11 deletions(-)

index 6bd821d..66ab714 100644
@@ -3508,6 +3508,11 @@ static int parse_submodule_opt(struct diff_options *options, const char *value)
 	return 1;
 }
 
+static void enable_patch_output(int *fmt) {
+	*fmt &= ~DIFF_FORMAT_NO_OUTPUT;
+	*fmt |= DIFF_FORMAT_PATCH;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];

@@ -3515,15 +3520,15 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	int argcount;
 
 	/* Output format options */
-	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch"))
-		options->output_format |= DIFF_FORMAT_PATCH;
-	else if (opt_arg(arg, 'U', "unified", &options->context))
-		options->output_format |= DIFF_FORMAT_PATCH;
+	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch")
+	    || opt_arg(arg, 'U', "unified", &options->context))
+		enable_patch_output(&options->output_format);
 	else if (!strcmp(arg, "--raw"))
 		options->output_format |= DIFF_FORMAT_RAW;
-	else if (!strcmp(arg, "--patch-with-raw"))
-		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW;
-	else if (!strcmp(arg, "--numstat"))
+	else if (!strcmp(arg, "--patch-with-raw")) {
+		enable_patch_output(&options->output_format);
+		options->output_format |= DIFF_FORMAT_RAW;
+	} else if (!strcmp(arg, "--numstat"))
 		options->output_format |= DIFF_FORMAT_NUMSTAT;
 	else if (!strcmp(arg, "--shortstat"))
 		options->output_format |= DIFF_FORMAT_SHORTSTAT;

@@ -3545,9 +3550,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_CHECKDIFF;
 	else if (!strcmp(arg, "--summary"))
 		options->output_format |= DIFF_FORMAT_SUMMARY;
-	else if (!strcmp(arg, "--patch-with-stat"))
-		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT;
-	else if (!strcmp(arg, "--name-only"))
+	else if (!strcmp(arg, "--patch-with-stat")) {
+		enable_patch_output(&options->output_format);
+		options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	} else if (!strcmp(arg, "--name-only"))
 		options->output_format |= DIFF_FORMAT_NAME;
 	else if (!strcmp(arg, "--name-status"))
 		options->output_format |= DIFF_FORMAT_NAME_STATUS;

@@ -3624,7 +3630,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 
 	/* flags options */
 	else if (!strcmp(arg, "--binary")) {
-		options->output_format |= DIFF_FORMAT_PATCH;
+		enable_patch_output(&options->output_format);
 		DIFF_OPT_SET(options, BINARY);
 	}
 	else if (!strcmp(arg, "--full-index"))

index 3b9a9ae..8de36b7 100755
@@ -71,4 +71,22 @@ 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 >actual &&
+	compare_diff_patch expected actual
+'
+
+test_expect_success 'git diff-files --no-patch --patch-with-raw shows the patch and raw data' '
+	git diff-files --no-patch --patch-with-raw >actual &&
+	grep -q "^:100644 100755 .* 0000000000000000000000000000000000000000 M	path0\$" actual &&
+	tail -n +4 actual >actual-patch &&
+	compare_diff_patch expected actual-patch
+'
+
+test_expect_success 'git diff-files --patch --no-patch does not show the patch' '
+	git diff-files --patch --no-patch >actual 2>err &&
+	test_must_be_empty actual &&
+	test_must_be_empty err
+'
+
 test_done

1.8.3.1.495.g13f33cf.dirty

---
 Documentation/git-show.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

index ae4edcc..4e617e6 100644
@@ -45,6 +45,15 @@ include::pretty-options.txt[]
 include::pretty-formats.txt[]
 
 
+COMMON DIFF OPTIONS
+-------------------
+
+:git-log: 1
+include::diff-options.txt[]
+
+include::diff-generate-patch.txt[]
+
+
 EXAMPLES
 --------
 

1.8.3.1.495.g13f33cf.dirty

Technically, "-s, --no-patch" is implemented in diff.c ("git diff --no-patch" is essentially useless, but valid). From the user point of view, this allows the documentation to show up in "git show --help", which is one of the most useful use of the option.

While we're there, add a sentence explaining why the option can be useful.

---
 Documentation/diff-options.txt     | 5 +++++
 Documentation/rev-list-options.txt | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

index 87e92d6..bbed2cd 100644
@@ -26,6 +26,11 @@ ifndef::git-format-patch[]
 	{git-diff? This is the default.}
 endif::git-format-patch[]
 
+-s::
+--no-patch::
+	Suppress diff output. Useful for commands like `git show` that
+	show the patch by default, or to cancel the effect of `--patch`.
+
 -U<n>::
 --unified=<n>::
 	Generate diffs with <n> lines of context instead of

index c128a85..e632e85 100644
@@ -849,8 +849,4 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 -t::
 
 	Show the tree objects in the diff output. This implies '-r'.
-
--s::
---no-patch::
-	Suppress diff output.
 endif::git-rev-list[]

1.8.3.1.495.g13f33cf.dirty

This is the convention in most other files and even at the beginning of git-log.txt

---
 Documentation/git-log.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 2ea79ba..2eda5e4 100644
@@ -97,7 +97,7 @@ include::rev-list-options.txt[]
 
 include::pretty-formats.txt[]
 
-Common diff options
+COMMON DIFF OPTIONS
 -------------------
 
 :git-log: 1

@@ -105,7 +105,7 @@ include::diff-options.txt[]
 
 include::diff-generate-patch.txt[]
 
-Examples
+EXAMPLES
 --------
 `git log --no-merges`::
 

@@ -161,12 +161,12 @@ git log -L '/int main/',/^}/:main.c::
 `git log -3`::
 	Limits the number of commits to show to 3.
 
-Discussion
+DISCUSSION
 ----------
 
 include::i18n.txt[]
 
-Configuration
+CONFIGURATION
 -------------
 
 See linkgit:git-config[1] for core variables and linkgit:git-diff[1]

1.8.3.1.495.g13f33cf.dirty

Thanks! I think this series is ready now.

Thanks as well. :)

Thanks.

---
 Documentation/git-show.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

index ae4edcc..4e617e6 100644
@@ -45,6 +45,15 @@ include::pretty-options.txt[]
 include::pretty-formats.txt[]
 
 
+COMMON DIFF OPTIONS
+-------------------
+
+:git-log: 1
+include::diff-options.txt[]
+
+include::diff-generate-patch.txt[]
+
+
 EXAMPLES
 --------
 

1.8.3.1.495.g13f33cf.dirty

Technically, "-s, --no-patch" is implemented in diff.c ("git diff --no-patch" is essentially useless, but valid). From the user point of view, this allows the documentation to show up in "git show --help", which is one of the most useful use of the option.

While we're there, add a sentence explaining why the option can be useful.

---
 Documentation/diff-options.txt     | 5 +++++
 Documentation/rev-list-options.txt | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

index 87e92d6..bbed2cd 100644
@@ -26,6 +26,11 @@ ifndef::git-format-patch[]
 	{git-diff? This is the default.}
 endif::git-format-patch[]
 
+-s::
+--no-patch::
+	Suppress diff output. Useful for commands like `git show` that
+	show the patch by default, or to cancel the effect of `--patch`.
+
 -U<n>::
 --unified=<n>::
 	Generate diffs with <n> lines of context instead of

index c128a85..e632e85 100644
@@ -849,8 +849,4 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 -t::
 
 	Show the tree objects in the diff output. This implies '-r'.
-
--s::
---no-patch::
-	Suppress diff output.
 endif::git-rev-list[]

1.8.3.1.495.g13f33cf.dirty

This is the convention in other files and even at the beginning of git-log.txt

---
 Documentation/git-log.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 2ea79ba..2eda5e4 100644
@@ -97,7 +97,7 @@ include::rev-list-options.txt[]
 
 include::pretty-formats.txt[]
 
-Common diff options
+COMMON DIFF OPTIONS
 -------------------
 
 :git-log: 1

@@ -105,7 +105,7 @@ include::diff-options.txt[]
 
 include::diff-generate-patch.txt[]
 
-Examples
+EXAMPLES
 --------
 `git log --no-merges`::
 

@@ -161,12 +161,12 @@ git log -L '/int main/',/^}/:main.c::
 `git log -3`::
 	Limits the number of commits to show to 3.
 
-Discussion
+DISCUSSION
 ----------
 
 include::i18n.txt[]
 
-Configuration
+CONFIGURATION
 -------------
 
 See linkgit:git-config[1] for core variables and linkgit:git-diff[1]

1.8.3.1.495.g13f33cf.dirty

The docs aren't so consistent on this, but I agree that it makes sense to at least be consistent within the generated git-log.html. :)

Generally the series looks very good. Thanks for taking this on.

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

---
This is the one which changed.

 diff.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

index 6bd821d..66ab714 100644
@@ -3508,6 +3508,11 @@ static int parse_submodule_opt(struct diff_options *options, const char *value)
 	return 1;
 }
 
+static void enable_patch_output(int *fmt) {
+	*fmt &= ~DIFF_FORMAT_NO_OUTPUT;
+	*fmt |= DIFF_FORMAT_PATCH;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];

@@ -3515,15 +3520,15 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	int argcount;
 
 	/* Output format options */
-	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch"))
-		options->output_format |= DIFF_FORMAT_PATCH;
-	else if (opt_arg(arg, 'U', "unified", &options->context))
-		options->output_format |= DIFF_FORMAT_PATCH;
+	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch")
+	    || opt_arg(arg, 'U', "unified", &options->context))
+		enable_patch_output(&options->output_format);
 	else if (!strcmp(arg, "--raw"))
 		options->output_format |= DIFF_FORMAT_RAW;
-	else if (!strcmp(arg, "--patch-with-raw"))
-		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW;
-	else if (!strcmp(arg, "--numstat"))
+	else if (!strcmp(arg, "--patch-with-raw")) {
+		enable_patch_output(&options->output_format);
+		options->output_format |= DIFF_FORMAT_RAW;
+	} else if (!strcmp(arg, "--numstat"))
 		options->output_format |= DIFF_FORMAT_NUMSTAT;
 	else if (!strcmp(arg, "--shortstat"))
 		options->output_format |= DIFF_FORMAT_SHORTSTAT;

@@ -3545,9 +3550,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_CHECKDIFF;
 	else if (!strcmp(arg, "--summary"))
 		options->output_format |= DIFF_FORMAT_SUMMARY;
-	else if (!strcmp(arg, "--patch-with-stat"))
-		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT;
-	else if (!strcmp(arg, "--name-only"))
+	else if (!strcmp(arg, "--patch-with-stat")) {
+		enable_patch_output(&options->output_format);
+		options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	} else if (!strcmp(arg, "--name-only"))
 		options->output_format |= DIFF_FORMAT_NAME;
 	else if (!strcmp(arg, "--name-status"))
 		options->output_format |= DIFF_FORMAT_NAME_STATUS;

@@ -3624,7 +3630,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 
 	/* flags options */
 	else if (!strcmp(arg, "--binary")) {
-		options->output_format |= DIFF_FORMAT_PATCH;
+		enable_patch_output(&options->output_format);
 		DIFF_OPT_SET(options, BINARY);
 	}
 	else if (!strcmp(arg, "--full-index"))

1.8.3.1.495.g13f33cf.dirty

---
 Documentation/git-show.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

index ae4edcc..4e617e6 100644
@@ -45,6 +45,15 @@ include::pretty-options.txt[]
 include::pretty-formats.txt[]
 
 
+COMMON DIFF OPTIONS
+-------------------
+
+:git-log: 1
+include::diff-options.txt[]
+
+include::diff-generate-patch.txt[]
+
+
 EXAMPLES
 --------
 

1.8.3.1.495.g13f33cf.dirty

Technically, "-s, --no-patch" is implemented in diff.c ("git diff --no-patch" is essentially useless, but valid). From the user point of view, this allows the documentation to show up in "git show --help", which is one of the most useful use of the option.

While we're there, add a sentence explaining why the option can be useful.

---
 Documentation/diff-options.txt     | 5 +++++
 Documentation/rev-list-options.txt | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

index 87e92d6..bbed2cd 100644
@@ -26,6 +26,11 @@ ifndef::git-format-patch[]
 	{git-diff? This is the default.}
 endif::git-format-patch[]
 
+-s::
+--no-patch::
+	Suppress diff output. Useful for commands like `git show` that
+	show the patch by default, or to cancel the effect of `--patch`.
+
 -U<n>::
 --unified=<n>::
 	Generate diffs with <n> lines of context instead of

index c128a85..e632e85 100644
@@ -849,8 +849,4 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 -t::
 
 	Show the tree objects in the diff output. This implies '-r'.
-
--s::
---no-patch::
-	Suppress diff output.
 endif::git-rev-list[]

1.8.3.1.495.g13f33cf.dirty

This is the convention in other files and even at the beginning of git-log.txt

---
 Documentation/git-log.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 2ea79ba..2eda5e4 100644
@@ -97,7 +97,7 @@ include::rev-list-options.txt[]
 
 include::pretty-formats.txt[]
 
-Common diff options
+COMMON DIFF OPTIONS
 -------------------
 
 :git-log: 1

@@ -105,7 +105,7 @@ include::diff-options.txt[]
 
 include::diff-generate-patch.txt[]
 
-Examples
+EXAMPLES
 --------
 `git log --no-merges`::
 

@@ -161,12 +161,12 @@ git log -L '/int main/',/^}/:main.c::
 `git log -3`::
 	Limits the number of commits to show to 3.
 
-Discussion
+DISCUSSION
 ----------
 
 include::i18n.txt[]
 
-Configuration
+CONFIGURATION
 -------------
 
 See linkgit:git-config[1] for core variables and linkgit:git-diff[1]

1.8.3.1.495.g13f33cf.dirty

---
 Documentation/git-show.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

index ae4edcc..4e617e6 100644
@@ -45,6 +45,15 @@ include::pretty-options.txt[]
 include::pretty-formats.txt[]
 
 
+COMMON DIFF OPTIONS
+-------------------
+
+:git-log: 1
+include::diff-options.txt[]
+
+include::diff-generate-patch.txt[]
+
+
 EXAMPLES
 --------
 

1.8.3.1.495.g13f33cf.dirty

Actually, we say "log message and textual diff" with "merge commit ... diff-tree --cc", so please scratch the above comment (I think we should however stop saying "in a special format" there---when the combined diff was invented, the format may have been novelty, but it no longer is).

Technically, "-s, --no-patch" is implemented in diff.c ("git diff --no-patch" is essentially useless, but valid). From the user point of view, this allows the documentation to show up in "git show --help", which is one of the most useful use of the option.

While we're there, add a sentence explaining why the option can be useful.

---
It might make sense to surround this with ifndef::git-diff[], but since "git diff --no-patch" actually works, I've left it unconditional to be technically correct.

 Documentation/diff-options.txt     | 5 +++++
 Documentation/rev-list-options.txt | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

index 87e92d6..bbed2cd 100644
@@ -26,6 +26,11 @@ ifndef::git-format-patch[]
 	{git-diff? This is the default.}
 endif::git-format-patch[]
 
+-s::
+--no-patch::
+	Suppress diff output. Useful for commands like `git show` that
+	show the patch by default, or to cancel the effect of `--patch`.
+
 -U<n>::
 --unified=<n>::
 	Generate diffs with <n> lines of context instead of

index c128a85..e632e85 100644
@@ -849,8 +849,4 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 -t::
 
 	Show the tree objects in the diff output. This implies '-r'.
-
--s::
---no-patch::
-	Suppress diff output.
 endif::git-rev-list[]

1.8.3.1.495.g13f33cf.dirty

This is the convention in other files and even at the beginning of git-log.txt

---
 Documentation/git-log.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 2ea79ba..2eda5e4 100644
@@ -97,7 +97,7 @@ include::rev-list-options.txt[]
 
 include::pretty-formats.txt[]
 
-Common diff options
+COMMON DIFF OPTIONS
 -------------------
 
 :git-log: 1

@@ -105,7 +105,7 @@ include::diff-options.txt[]
 
 include::diff-generate-patch.txt[]
 
-Examples
+EXAMPLES
 --------
 `git log --no-merges`::
 

@@ -161,12 +161,12 @@ git log -L '/int main/',/^}/:main.c::
 `git log -3`::
 	Limits the number of commits to show to 3.
 
-Discussion
+DISCUSSION
 ----------
 
 include::i18n.txt[]
 
-Configuration
+CONFIGURATION
 -------------
 
 See linkgit:git-config[1] for core variables and linkgit:git-diff[1]

1.8.3.1.495.g13f33cf.dirty

Thanks, that's exactly what I am looking for.
Stefan