[PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat

Previously, the three dash marker (---) would only be added if the diff
output format was a patch and diffstat (usually -p and --stat). Now that
patches are always generated by format-patch regardless of the stat
format being used (--stat, --raw, --numstat, etc.), always add the three
dash marker when a patch is being generated and a stat option is used.

This allows users to choose the stat format they want and unifies the
format of patches with stats. It also make patches easier to apply when
generated by format-patch with non-standard stat options as the stat is
no longer considered part of the commit message.

Signed-off-by: Stephen Boyd <redacted>
---

It seems that after looking at this series it would be better if the
marker is always displayed even when a user has used a non-standard
stat option with format-patch.

I'm not sure this is wanted though and I guess this could break people's
scripts. Are people actually using --numstat or --raw to put the stat into
the commit message?

 log-tree.c                                         |    5 +++--
 ...f-tree_--pretty_--root_--patch-with-raw_initial |    2 +-
 t/t4013/diff.show_--patch-with-raw_side            |    2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 1618f3c..1871c6c 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -461,8 +461,9 @@ int log_tree_diff_flush(struct rev_info *opt)
 		if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
 		    opt->verbose_header &&
 		    opt->commit_format != CMIT_FMT_ONELINE) {
-			int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
-			if ((pch & opt->diffopt.output_format) == pch)
+			int pch = DIFF_FORMAT_PATCH;
+			int fmt = opt->diffopt.output_format;
+			if (pch & fmt && pch ^ fmt)
 				printf("---");
 			putchar('\n');
 		}

diff --git a/t/t4013/diff.diff-tree_--pretty_--root_--patch-with-raw_initial b/t/t4013/diff.diff-tree_--pretty_--root_--patch-with-raw_initial
index a3203bd..2f4fec9 100644
--- a/t/t4013/diff.diff-tree_--pretty_--root_--patch-with-raw_initial
+++ b/t/t4013/diff.diff-tree_--pretty_--root_--patch-with-raw_initial

@@ -4,7 +4,7 @@ Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:00:00 2006 +0000
 
     Initial
-
+---
 :000000 100644 0000000000000000000000000000000000000000 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e A	dir/sub
 :000000 100644 0000000000000000000000000000000000000000 01e79c32a8c99c557f0757da7cb6d65b3414466d A	file0
 :000000 100644 0000000000000000000000000000000000000000 01e79c32a8c99c557f0757da7cb6d65b3414466d A	file2

diff --git a/t/t4013/diff.show_--patch-with-raw_side b/t/t4013/diff.show_--patch-with-raw_side
index 221b46a..b7566be 100644
--- a/t/t4013/diff.show_--patch-with-raw_side
+++ b/t/t4013/diff.show_--patch-with-raw_side

@@ -4,7 +4,7 @@ Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:03:00 2006 +0000
 
     Side
-
+---
 :100644 100644 35d242b... 7289e35... M	dir/sub
 :100644 100644 01e79c3... f4615da... M	file0
 :000000 100644 0000000... 7289e35... A	file3

-- 
1.6.5.2.181.gd6f41

Stephen Boyd [off-list ref] writes:

Previously, the three dash marker (---) would only be added if the diff
output format was a patch and diffstat (usually -p and --stat). Now that
patches are always generated by format-patch regardless of the stat
format being used (--stat, --raw, --numstat, etc.), always add the three
dash marker when a patch is being generated and a stat option is used.

This allows users to choose the stat format they want and unifies the
format of patches with stats. It also make patches easier to apply when
generated by format-patch with non-standard stat options as the stat is
no longer considered part of the commit message.

Signed-off-by: Stephen Boyd <redacted>
---

I actually am more worried about 68daa64 from 14 months ago, as I vaguely
recall seeing an explicit user request that in some community the diffstat
information is unwanted on their mailing list, and I am reasonably sure
that "-p suppresses --stat" was done deliberately to satisfy them (even
though it may have been a suboptimal UI and --no-stat might have been a
lot more straightforward).

Even though I personally find the stat information very useful, I would be
happier if somebody reverts the bg/format-patch-p-noop series and instead
fixes the regression caused by 68daa64, and does so without touching any
output from the low-level plumbing like diff-tree that may be used by
scripts.

With older (say 1.6.0) git, format-patch with the -p option does not give
these three-dash lines, and it does look funny.  Even though the same
funniness appears only when you use --raw or --numstat with the current
code, if we fix "-p" to suppress the default "--stat", this will become an
issue again.

I'm not sure this is wanted though and I guess this could break people's
scripts. Are people actually using --numstat or --raw to put the stat into
the commit message?

I am not worried so much about "format-patch --any-option" output; I think
it is sane to have three-dash line in it and that is what people expect to
see.

I however think people used "diff-tree --pretty --raw" as a mechanism to
obtain statistics.  A script can easily see where the header is and where
messages are (they are four-space indented), and what remains after
stripping them give you the list of paths each commit touches.  --numstat
was invented to help this kind of application gather line-level statistics
more easily, and I am a bit reluctant to suddenly start giving three-dash
in their output.  It will upset what is reading from the pipe downstream.

In an ideal world, I would probably say:

 * format-patch should have three-dash after the commit message, no matter
   what format the patch is asked for, and it always will give patch text.

 * format-patch -p should be reinstated as a way to ask for "just patch
   text, no diffstat".  Introducing a new option --no-stat _in addition_
   to improve the UI is Ok.

 * format-patch -U<n> should not be mistaken as a request to suppress
   diffstat; what 68daa64 _tried_ to do was worthy.

 * Other commands of "log" family that understand -p/-U<n> to produce
   patch text should also give three-dash after the log message, and no
   three-dash when they don't produce patch text.

On Tue, Nov 03, 2009 at 09:49:46PM -0800, Junio C Hamano wrote:

I actually am more worried about 68daa64 from 14 months ago, as I vaguely
recall seeing an explicit user request that in some community the diffstat
information is unwanted on their mailing list, and I am reasonably sure
that "-p suppresses --stat" was done deliberately to satisfy them (even
though it may have been a suboptimal UI and --no-stat might have been a
lot more straightforward).

Even though I personally find the stat information very useful, I would be
happier if somebody reverts the bg/format-patch-p-noop series and instead
fixes the regression caused by 68daa64, and does so without touching any
output from the low-level plumbing like diff-tree that may be used by
scripts.

I agree that 68daa64 is a hack (and I even noted in the commit log that
"-p" is now a no-op). The problem is that we don't have the one critical
bit of information in cmd_format_patch that we do in diff_opt_parse: was
the format set explicitly, or was it a side-effect of -U (or --binary,
or maybe others).

Here's a patch which fixes the regression, but it feels awfully hack-ish
to me, as it treats "-p" specially in diff_opt (but in a way that no
other existing code should care about). It would be "cleaner" to me to
have some infrastructure for keeping an implicit and an explicit format,
and then merging them at the end. But I don't think we ever care about
this explicitness for any other formats, so this is at least simple.

Another option might be for format-patch to simply parse "-p" itself,
setting the format and marking an "explicit" flag. I'll look into that
as an alternative.

---

diff --git a/builtin-log.c b/builtin-log.c
index a359679..3b5819e 100644
--- a/builtin-log.c
+++ b/builtin-log.c

@@ -1034,8 +1034,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
 		die("--check does not make sense");
 
-	if (!rev.diffopt.output_format
-		|| rev.diffopt.output_format == DIFF_FORMAT_PATCH)
+	if (!rev.diffopt.output_format ||
+	   (rev.diffopt.output_format == DIFF_FORMAT_PATCH &&
+	    !rev.diffopt.explicit_patch_format))
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
 
 	/* Always generate a patch */

diff --git a/diff.c b/diff.c
index 9cd9693..9ce5520 100644
--- a/diff.c
+++ b/diff.c

@@ -2643,8 +2643,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	const char *arg = av[0];
 
 	/* Output format options */
-	if (!strcmp(arg, "-p") || !strcmp(arg, "-u"))
+	if (!strcmp(arg, "-p") || !strcmp(arg, "-u")) {
 		options->output_format |= DIFF_FORMAT_PATCH;
+		options->explicit_patch_format = 1;
+	}
 	else if (opt_arg(arg, 'U', "unified", &options->context))
 		options->output_format |= DIFF_FORMAT_PATCH;
 	else if (!strcmp(arg, "--raw"))

diff --git a/diff.h b/diff.h
index 55f3203..406c0c6 100644
--- a/diff.h
+++ b/diff.h

@@ -90,6 +90,7 @@ struct diff_options {
 	int skip_stat_unmatch;
 	int line_termination;
 	int output_format;
+	int explicit_patch_format;
 	int pickaxe_opts;
 	int rename_score;
 	int rename_limit;

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index f826348..5689d59 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh

@@ -455,6 +455,27 @@ test_expect_success 'format-patch respects -U' '
 
 '
 
+cat > expect << EOF
+
+diff --git a/file b/file
+index 40f36c6..2dc5c23 100644
+--- a/file
++++ b/file
+@@ -14,3 +14,19 @@ C
+ D
+ E
+ F
++5
+EOF
+
+test_expect_success 'format-patch -p suppresses stat' '
+
+	git format-patch -p -2 &&
+	sed -e "1,/^$/d" -e "/^+5/q" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&
+	test_cmp expect output
+
+'
+
 test_expect_success 'format-patch from a subdirectory (1)' '
 	filename=$(
 		rm -rf sub &&

With older (say 1.6.0) git, format-patch with the -p option does not give
these three-dash lines, and it does look funny.  Even though the same
funniness appears only when you use --raw or --numstat with the current
code, if we fix "-p" to suppress the default "--stat", this will become an
issue again.

My test case checks the current output (i.e., missing dashes). I think
it should probably have dashes, but that should be fixed in a separate
patch.

-Peff

2009/11/4 Junio C Hamano [off-list ref]:

In an ideal world, I would probably say:

 * format-patch should have three-dash after the commit message, no matter
  what format the patch is asked for, and it always will give patch text.

I agree.

 * format-patch -p should be reinstated as a way to ask for "just patch
  text, no diffstat".  Introducing a new option --no-stat _in addition_
  to improve the UI is Ok.

Since -p has been broken for 14 months, is really necessary to reinstate
it? (Or has the breakage not been reported because the people who care
still use a git version older than 14 months?)

Why not just add a new --no-stat option?

 * format-patch -U<n> should not be mistaken as a request to suppress
  diffstat; what 68daa64 _tried_ to do was worthy.

I agree.

/Björn

-- 
Björn Gustavsson, Erlang/OTP, Ericsson AB

On Wed, Nov 04, 2009 at 01:36:13AM -0500, Jeff King wrote:

Here's a patch which fixes the regression, but it feels awfully hack-ish
to me, as it treats "-p" specially in diff_opt (but in a way that no
other existing code should care about). It would be "cleaner" to me to
have some infrastructure for keeping an implicit and an explicit format,
and then merging them at the end. But I don't think we ever care about
this explicitness for any other formats, so this is at least simple.

Another option might be for format-patch to simply parse "-p" itself,
setting the format and marking an "explicit" flag. I'll look into that
as an alternative.

OK, doing that turned out to be much nicer. Less code, localized to
format-patch, and less confusing to read.

This patch goes on top of master, and terribly conflicts with Björn's
changes in the area. But I had the impression you wanted to revert those
changes for now anyway, so probably this should go in as a bug fix and
everything else should be built on top. It actually would be an even
smaller change on top of his "always show patch, even when other formats
are given" change, but I didn't want to depend on it.

-- >8 --
Subject: [PATCH] format-patch: make "-p" suppress diffstat

Once upon a time, format-patch would use its default stat
plus patch format only when no diff format was given on the
command line. This meant that "format-patch -p" would
suppress the stat and show just the patch.

Commit 68daa64 changed this to keep the stat format when we
had an "implicit" patch format, like "-U5". As a side
effect, this meant that an explicit patch format was now
ignored (because cmd_format_patch didn't know the reason
that the format was set way down in diff_opt_parse).

This patch unbreaks what 68daa64 did (while still preserving
what 68daa64 was trying to do), reinstating "-p" to suppress
the default behavior. We do this by parsing "-p" ourselves
in format-patch, and noting whether it was used explicitly.

Signed-off-by: Jeff King <redacted>
---
The help text for "-p" is up for debate. It can also of course be used
as "show patch in addition to other things you already specified on the
command line". But I wanted to point out that its main use is the side
effect that it suppresses the default output. I am open to suggestions.

 builtin-log.c           |    9 +++++++--
 t/t4014-format-patch.sh |   21 +++++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 207a361..da79047 100644
--- a/builtin-log.c
+++ b/builtin-log.c

@@ -891,6 +891,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct patch_ids ids;
 	char *add_signoff = NULL;
 	struct strbuf buf = STRBUF_INIT;
+	int use_patch_format = 0;
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
 			    "use [PATCH n/m] even with a single patch",

@@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 0, "thread", &thread, "style",
 			    "enable message threading, styles: shallow, deep",
 			    PARSE_OPT_OPTARG, thread_callback },
+		OPT_BOOLEAN('p', NULL, &use_patch_format,
+			"show patch format instead of default (patch + stat)"),
 		OPT_END()
 	};
 

@@ -1027,8 +1030,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
 
-	if (!rev.diffopt.output_format
-		|| rev.diffopt.output_format == DIFF_FORMAT_PATCH)
+	if (patch_format)
+		rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+	else if (!rev.diffopt.output_format ||
+		  rev.diffopt.output_format == DIFF_FORMAT_PATCH)
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY | DIFF_FORMAT_PATCH;
 
 	if (!DIFF_OPT_TST(&rev.diffopt, TEXT) && !no_binary_diff)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 531f5b7..cab6ce2 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh

@@ -455,6 +455,27 @@ test_expect_success 'format-patch respects -U' '
 
 '
 
+cat > expect << EOF
+
+diff --git a/file b/file
+index 40f36c6..2dc5c23 100644
+--- a/file
++++ b/file
+@@ -14,3 +14,19 @@ C
+ D
+ E
+ F
++5
+EOF
+
+test_expect_success 'format-patch -p suppresses stat' '
+
+	git format-patch -p -2 &&
+	sed -e "1,/^$/d" -e "/^+5/q" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&
+	test_cmp expect output
+
+'
+
 test_expect_success 'format-patch from a subdirectory (1)' '
 	filename=$(
 		rm -rf sub &&

-- 
1.6.5.2.308.g2bb5

On Wed, Nov 04, 2009 at 02:10:54AM -0500, Jeff King wrote:

Subject: [PATCH] format-patch: make "-p" suppress diffstat

Ugh. And here is one that actually compiles. Yes, I actually did test
it, but the one-line typo was sitting uncommitted in my workdir.

-- >8 --
Subject: [PATCH] format-patch: make "-p" suppress diffstat

Once upon a time, format-patch would use its default stat
plus patch format only when no diff format was given on the
command line. This meant that "format-patch -p" would
suppress the stat and show just the patch.

Commit 68daa64 changed this to keep the stat format when we
had an "implicit" patch format, like "-U5". As a side
effect, this meant that an explicit patch format was now
ignored (because cmd_format_patch didn't know the reason
that the format was set way down in diff_opt_parse).

This patch unbreaks what 68daa64 did (while still preserving
what 68daa64 was trying to do), reinstating "-p" to suppress
the default behavior. We do this by parsing "-p" ourselves
in format-patch, and noting whether it was used explicitly.

Signed-off-by: Jeff King <redacted>
---
 builtin-log.c           |    9 +++++++--
 t/t4014-format-patch.sh |   21 +++++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 207a361..0ff032b 100644
--- a/builtin-log.c
+++ b/builtin-log.c

@@ -891,6 +891,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct patch_ids ids;
 	char *add_signoff = NULL;
 	struct strbuf buf = STRBUF_INIT;
+	int use_patch_format = 0;
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
 			    "use [PATCH n/m] even with a single patch",

@@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 0, "thread", &thread, "style",
 			    "enable message threading, styles: shallow, deep",
 			    PARSE_OPT_OPTARG, thread_callback },
+		OPT_BOOLEAN('p', NULL, &use_patch_format,
+			"show patch format instead of default (patch + stat)"),
 		OPT_END()
 	};
 

@@ -1027,8 +1030,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
 
-	if (!rev.diffopt.output_format
-		|| rev.diffopt.output_format == DIFF_FORMAT_PATCH)
+	if (use_patch_format)
+		rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+	else if (!rev.diffopt.output_format ||
+		  rev.diffopt.output_format == DIFF_FORMAT_PATCH)
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY | DIFF_FORMAT_PATCH;
 
 	if (!DIFF_OPT_TST(&rev.diffopt, TEXT) && !no_binary_diff)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 531f5b7..cab6ce2 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh

@@ -455,6 +455,27 @@ test_expect_success 'format-patch respects -U' '
 
 '
 
+cat > expect << EOF
+
+diff --git a/file b/file
+index 40f36c6..2dc5c23 100644
+--- a/file
++++ b/file
+@@ -14,3 +14,19 @@ C
+ D
+ E
+ F
++5
+EOF
+
+test_expect_success 'format-patch -p suppresses stat' '
+
+	git format-patch -p -2 &&
+	sed -e "1,/^$/d" -e "/^+5/q" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&
+	test_cmp expect output
+
+'
+
 test_expect_success 'format-patch from a subdirectory (1)' '
 	filename=$(
 		rm -rf sub &&

-- 
1.6.5.2.308.g2bb5

Jeff King wrote:

This patch unbreaks what 68daa64 did (while still preserving
what 68daa64 was trying to do), reinstating "-p" to suppress
the default behavior. We do this by parsing "-p" ourselves
in format-patch, and noting whether it was used explicitly.

Signed-off-by: Jeff King <redacted>
---
  

This looks good to me; covering 2 and 3 of Junio's TODO list.

I don't imagine we want this option in the messaging group though. Can 
you move it up?

On Tue, Nov 03, 2009 at 11:35:33PM -0800, Stephen Boyd wrote:

quoted

@@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)

		{ OPTION_CALLBACK, 0, "thread", &thread, "style",
			    "enable message threading, styles: shallow, deep",
			    PARSE_OPT_OPTARG, thread_callback },
+		OPT_BOOLEAN('p', NULL, &use_patch_format,
+			"show patch format instead of default (patch + stat)"),
		OPT_END()
	};

I don't imagine we want this option in the messaging group though.
Can you move it up?

Thanks, good catch. I just tacked it onto the end, forgetting that we
were using grouping. Junio, can you tweak it, or do you want a resend?

-Peff

On Wed, Nov 4, 2009 at 8:10 AM, Jeff King [off-list ref] wrote:

This patch goes on top of master, and terribly conflicts with Björn's
changes in the area. But I had the impression you wanted to revert those
changes for now anyway, so probably this should go in as a bug fix and
everything else should be built on top. It actually would be an even
smaller change on top of his "always show patch, even when other formats
are given" change, but I didn't want to depend on it.

No problem. I can re-implement my patch series on top of your patch.

/Björn
-- 
Björn Gustavsson, Erlang/OTP, Ericsson AB