Bug in "git show"?

Bug in "git show"?

[Stefan Beller]

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

diff --git a/Documentation/RelNotes/1.8.4.txt
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-----

Re: Bug in "git show"?

[Matthieu Moy]

Stefan Beller <stefanbeller@googlemail.com> writes:

> However I sometimes also get:
> sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53
> Fri Jul 12 10:49:34 2013 -0700
>
> diff --git a/Documentation/RelNotes/1.8.4.txt
> 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

"git show" will show the diff by default. For merge commits, it shows
the --cc diff which is often empty, hence the behavior you see.

You want to use "git show -s", which suppresses the patch output.

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

Re: Bug in "git show"?

[Stefan Beller]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

On 07/15/2013 02:12 PM, Matthieu Moy wrote:
> Stefan Beller <stefanbeller@googlemail.com> writes:
> 
>> However I sometimes also get:
>> sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53
>> Fri Jul 12 10:49:34 2013 -0700
>>
>> diff --git a/Documentation/RelNotes/1.8.4.txt
>> 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
> 
> "git show" will show the diff by default. For merge commits, it shows
> the --cc diff which is often empty, hence the behavior you see.
> 
> You want to use "git show -s", which suppresses the patch output.
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[PATCH 0/5] Make "git show -s" easier to discover for users

[Matthieu Moy]

> Stefan Beller <stefanbeller@googlemail.com> writes:
>
>> However I sometimes also get:
>> sb@sb:~/OSS/git$ git show --format="%ad" 0da7a53
>> Fri Jul 12 10:49:34 2013 -0700
>>
>> diff --git a/Documentation/RelNotes/1.8.4.txt
>> 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
>
> "git show" will show the diff by default. For merge commits, it shows
> the --cc diff which is often empty, hence the behavior you see.
>
> You want to use "git show -s", which suppresses the patch output.

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

[PATCH 1/5] diff: allow --no-patch as synonym for -s

[Matthieu Moy]

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

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/rev-list-options.txt | 1 +
 diff.c                             | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index e157ec3..c128a85 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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[]
diff --git a/diff.c b/diff.c
index 6578690..6bd821d 100644
--- a/diff.c
+++ b/diff.c
@@ -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

[PATCH 2/5] diff: allow --patch to override -s/--no-patch

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 diff.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 6bd821d..66a6877 100644
--- a/diff.c
+++ b/diff.c
@@ -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

Re: [PATCH 2/5] diff: allow --patch to override -s/--no-patch

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>  diff.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 6bd821d..66a6877 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -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;

I am wondering if the difference after this patch between "-p" and
"-U8" is deliberate, or just an accident coming from the way the
original was written in ee1e5412 (git diff: support "-U" and
"--unified" options properly, 2006-05-13).

If the original were written in this way:

	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch") ||
            opt_arg(arg, 'U', "unified", &options->context))
  		options->output_format |= DIFF_FORMAT_PATCH;

I suspect you would have given the additional

		options->output_format &= ~DIFF_FORMAT_NO_OUTPUT;

to the whole thing, without singling out "-U8".  Or am I missing
some good reason why "--no-patch -U8" should not produce a patch,
while "--no-patch -p" and "-U8" should both should?

Re: [PATCH 2/5] diff: allow --patch to override -s/--no-patch

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> I am wondering if the difference after this patch between "-p" and
> "-U8" is deliberate, or just an accident coming from the way the
> original was written in ee1e5412 (git diff: support "-U" and
> "--unified" options properly, 2006-05-13).

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

> If the original were written in this way:
>
> 	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch") ||
>             opt_arg(arg, 'U', "unified", &options->context))
>   		options->output_format |= DIFF_FORMAT_PATCH;

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/

Re: [PATCH 2/5] diff: allow --patch to override -s/--no-patch

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I am wondering if the difference after this patch between "-p" and
>> "-U8" is deliberate, or just an accident coming from the way the
>> original was written in ee1e5412 (git diff: support "-U" and
>> "--unified" options properly, 2006-05-13).
>
> No, it isn't. I just didn't notice the -U case.
>
>> If the original were written in this way:
>>
>> 	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch") ||
>>             opt_arg(arg, 'U', "unified", &options->context))
>>   		options->output_format |= DIFF_FORMAT_PATCH;
>
> Yes, this seems to be a better way.
>
> There are other cases like --patch-with-raw, I'll send a reroll.

Thanks.

[PATCH v2 0/5] Make "git show -s" easier to discover for users

[Matthieu Moy]

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

[PATCH v2 1/5] diff: allow --no-patch as synonym for -s

[Matthieu Moy]

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

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/rev-list-options.txt | 1 +
 diff.c                             | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index e157ec3..c128a85 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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[]
diff --git a/diff.c b/diff.c
index 6578690..6bd821d 100644
--- a/diff.c
+++ b/diff.c
@@ -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

Re: [PATCH v2 1/5] diff: allow --no-patch as synonym for -s

[Jonathan Nieder]

Matthieu Moy wrote:

> --- a/diff.c
> +++ b/diff.c
> @@ -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;

Very nice idea.

Tests?  E.g.:

diff --git i/t/t4000-diff-format.sh w/t/t4000-diff-format.sh
index 6ddd469..0fa7380 100755
--- i/t/t4000-diff-format.sh
+++ w/t/t4000-diff-format.sh
@@ -59,4 +59,18 @@ test_expect_success \
     'validate git diff-files -p output.' \
     'compare_diff_patch current expected'
 
+test_expect_success \
+    'git diff-files -s after editing work tree' \
+    '>empty &&
+    git diff-files -s >diff-s-output 2>err &&
+    test_cmp empty diff-s-output &&
+    test_cmp empty err'
+
+test_expect_success \
+    'git diff-files --no-patch as synonym for -s' \
+    '>empty &&
+    git diff-files --no-patch >diff-np-output 2>err &&
+    test_cmp empty diff-np-output &&
+    test_cmp empty err'
+
 test_done

[PATCH v3 0/6] Make "git show -s" easier to discover for users

[Matthieu Moy]

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

[PATCH v3 1/6] t4000-diff-format.sh: modernize style

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 t/t4000-diff-format.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index 6ddd469..2b5dffc 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -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

Re: [PATCH v3 1/6] t4000-diff-format.sh: modernize style

[Jonathan Nieder]

Matthieu Moy wrote:

> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>  t/t4000-diff-format.sh | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

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.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH v3 2/6] diff: allow --no-patch as synonym for -s

[Matthieu Moy]

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

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/rev-list-options.txt |  1 +
 diff.c                             |  2 +-
 t/t4000-diff-format.sh             | 12 ++++++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index e157ec3..c128a85 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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[]
diff --git a/diff.c b/diff.c
index 6578690..6bd821d 100644
--- a/diff.c
+++ b/diff.c
@@ -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 */
diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index 2b5dffc..d702575 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -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

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

[Matthieu Moy]

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.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 diff.c                 | 28 +++++++++++++++++-----------
 t/t4000-diff-format.sh |  5 +++++
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/diff.c b/diff.c
index 6bd821d..66ab714 100644
--- a/diff.c
+++ b/diff.c
@@ -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"))
diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index d702575..4fce12f 100755
--- 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
+'
+
 test_done
-- 
1.8.3.1.495.g13f33cf.dirty

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

[Jonathan Nieder]

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

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

[Matthieu Moy]

Jonathan Nieder <jrnieder@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 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/

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

[Jonathan Nieder]

Matthieu Moy wrote:
> Jonathan Nieder <jrnieder@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.

[PATCH v4 0/6] Make "git show -s" easier to discover for users

[Matthieu Moy]

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

[PATCH v4 1/6] t4000-diff-format.sh: modernize style

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 t/t4000-diff-format.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index 6ddd469..2b5dffc 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -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

[PATCH v4 2/6] diff: allow --no-patch as synonym for -s

[Matthieu Moy]

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

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/rev-list-options.txt |  1 +
 diff.c                             |  2 +-
 t/t4000-diff-format.sh             | 12 ++++++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index e157ec3..c128a85 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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[]
diff --git a/diff.c b/diff.c
index 6578690..6bd821d 100644
--- a/diff.c
+++ b/diff.c
@@ -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 */
diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index 2b5dffc..3b9a9ae 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -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

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

[Matthieu Moy]

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.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 diff.c                 | 28 +++++++++++++++++-----------
 t/t4000-diff-format.sh | 18 ++++++++++++++++++
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/diff.c b/diff.c
index 6bd821d..66ab714 100644
--- a/diff.c
+++ b/diff.c
@@ -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"))
diff --git a/t/t4000-diff-format.sh b/t/t4000-diff-format.sh
index 3b9a9ae..8de36b7 100755
--- a/t/t4000-diff-format.sh
+++ b/t/t4000-diff-format.sh
@@ -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

[PATCH v4 4/6] Documentation/git-show.txt: include common diff options, like git-log.txt

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-show.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/git-show.txt b/Documentation/git-show.txt
index ae4edcc..4e617e6 100644
--- a/Documentation/git-show.txt
+++ b/Documentation/git-show.txt
@@ -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

[PATCH v4 5/6] Documentation: move description of -s, --no-patch to diff-options.txt

[Matthieu Moy]

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.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/diff-options.txt     | 5 +++++
 Documentation/rev-list-options.txt | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 87e92d6..bbed2cd 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index c128a85..e632e85 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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

[PATCH v4 6/6] Documentation/git-log.txt: capitalize section names

[Matthieu Moy]

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

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-log.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 2ea79ba..2eda5e4 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -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

Re: [PATCH v4 0/6] Make "git show -s" easier to discover for users

[Jonathan Nieder]

Matthieu Moy wrote:

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

Thanks!  I think this series is ready now.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v4 0/6] Make "git show -s" easier to discover for users

[Stefan Beller]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

On 07/16/2013 07:46 PM, Jonathan Nieder wrote:
> Matthieu Moy wrote:
> 
>> 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(-)
> 
> Thanks!  I think this series is ready now.
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks as well. :)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: [PATCH v4 0/6] Make "git show -s" easier to discover for users

[Junio C Hamano]

Thanks.

[PATCH v3 4/6] Documentation/git-show.txt: include common diff options, like git-log.txt

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-show.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/git-show.txt b/Documentation/git-show.txt
index ae4edcc..4e617e6 100644
--- a/Documentation/git-show.txt
+++ b/Documentation/git-show.txt
@@ -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

[PATCH v3 5/6] Documentation: move description of -s, --no-patch to diff-options.txt

[Matthieu Moy]

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.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/diff-options.txt     | 5 +++++
 Documentation/rev-list-options.txt | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 87e92d6..bbed2cd 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index c128a85..e632e85 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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

[PATCH v3 6/6] Documentation/git-log.txt: capitalize section names

[Matthieu Moy]

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

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-log.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 2ea79ba..2eda5e4 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -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

Re: [PATCH v3 6/6] Documentation/git-log.txt: capitalize section names

[Jonathan Nieder]

Matthieu Moy wrote:

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

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.

[PATCH v2 2/5] diff: allow --patch & cie to override -s/--no-patch

[Matthieu Moy]

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.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
This is the one which changed.

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

diff --git a/diff.c b/diff.c
index 6bd821d..66ab714 100644
--- a/diff.c
+++ b/diff.c
@@ -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

[PATCH v2 3/5] Documentation/git-show.txt: include common diff options, like git-log.txt

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-show.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/git-show.txt b/Documentation/git-show.txt
index ae4edcc..4e617e6 100644
--- a/Documentation/git-show.txt
+++ b/Documentation/git-show.txt
@@ -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

[PATCH v2 4/5] Documentation: move description of -s, --no-patch to diff-options.txt

[Matthieu Moy]

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.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/diff-options.txt     | 5 +++++
 Documentation/rev-list-options.txt | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 87e92d6..bbed2cd 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index c128a85..e632e85 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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

[PATCH v2 5/5] Documentation/git-log.txt: capitalize section names

[Matthieu Moy]

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

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-log.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 2ea79ba..2eda5e4 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -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

[PATCH 3/5] Documentation/git-show.txt: include common diff options, like git-log.txt

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-show.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/git-show.txt b/Documentation/git-show.txt
index ae4edcc..4e617e6 100644
--- a/Documentation/git-show.txt
+++ b/Documentation/git-show.txt
@@ -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

Re: [PATCH 3/5] Documentation/git-show.txt: include common diff options, like git-log.txt

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>  Documentation/git-show.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/git-show.txt b/Documentation/git-show.txt
> index ae4edcc..4e617e6 100644
> --- a/Documentation/git-show.txt
> +++ b/Documentation/git-show.txt
> @@ -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
>  --------

This is a good start; the output should match what you would get for
git-log(1) with the above.

But we would need to say, unlike "log" whose default is not to show
any patch, "show" that works on a commit defaults to "--cc"
somewhere.

Other than that the whole series looks sensible to me.  Thanks.

Re: [PATCH 3/5] Documentation/git-show.txt: include common diff options, like git-log.txt

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>> ---
>>  Documentation/git-show.txt | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/Documentation/git-show.txt b/Documentation/git-show.txt
>> index ae4edcc..4e617e6 100644
>> --- a/Documentation/git-show.txt
>> +++ b/Documentation/git-show.txt
>> @@ -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
>>  --------
>
> This is a good start; the output should match what you would get for
> git-log(1) with the above.
>
> But we would need to say, unlike "log" whose default is not to show
> any patch, "show" that works on a commit defaults to "--cc"
> somewhere.
>
> Other than that the whole series looks sensible to me.  Thanks.

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

[PATCH 4/5] Documentation: move description of -s, --no-patch to diff-options.txt

[Matthieu Moy]

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.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
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(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 87e92d6..bbed2cd 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index c128a85..e632e85 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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

[PATCH 5/5] Documentation/git-log.txt: capitalize section names

[Matthieu Moy]

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

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-log.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 2ea79ba..2eda5e4 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -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