{"thread":{"id":"21471","subject":"[PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","startedAt":"2009-11-03T21:24:16Z","lastAt":"2009-11-04T18:10:01Z","messageCount":12,"participants":["Stephen Boyd","Junio C Hamano","Jeff King","Björn Gustavsson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"126676","messageId":"1257283456-7007-1-git-send-email-bebarino@gmail.com","threadId":"21471","inReplyTo":null,"subject":"[PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-11-03T21:24:16Z","receivedAt":"2009-11-03T21:24:16Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Previously, the three dash marker (---) would only be added if the diff\noutput format was a patch and diffstat (usually -p and --stat). Now that\npatches are always generated by format-patch regardless of the stat\nformat being used (--stat, --raw, --numstat, etc.), always add the three\ndash marker when a patch is being generated and a stat option is used.\n\nThis allows users to choose the stat format they want and unifies the\nformat of patches with stats. It also make patches easier to apply when\ngenerated by format-patch with non-standard stat options as the stat is\nno longer considered part of the commit message.\n\nSigned-off-by: Stephen Boyd \n---\n\nIt seems that after looking at this series it would be better if the\nmarker is always displayed even when a user has used a non-standard\nstat option with format-patch.\n\nI'm not sure this is wanted though and I guess this could break people's\nscripts. Are people actually using --numstat or --raw to put the stat into\nthe commit message?\n\n log-tree.c | 5 +++--\n ...f-tree_--pretty_--root_--patch-with-raw_initial | 2 +-\n t/t4013/diff.show_--patch-with-raw_side | 2 +-\n 3 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 1618f3c..1871c6c 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -461,8 +461,9 @@ int log_tree_diff_flush(struct rev_info *opt)\n \t\tif ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&\n \t\t opt->verbose_header &&\n \t\t opt->commit_format != CMIT_FMT_ONELINE) {\n-\t\t\tint pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;\n-\t\t\tif ((pch & opt->diffopt.output_format) == pch)\n+\t\t\tint pch = DIFF_FORMAT_PATCH;\n+\t\t\tint fmt = opt->diffopt.output_format;\n+\t\t\tif (pch & fmt && pch ^ fmt)\n \t\t\t\tprintf(\"---\");\n \t\t\tputchar('\\n');\n \t\t}\ndiff --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\nindex a3203bd..2f4fec9 100644\n--- a/t/t4013/diff.diff-tree_--pretty_--root_--patch-with-raw_initial\n+++ b/t/t4013/diff.diff-tree_--pretty_--root_--patch-with-raw_initial\n@@ -4,7 +4,7 @@ Author: A U Thor \n Date: Mon Jun 26 00:00:00 2006 +0000\n \n Initial\n-\n+---\n :000000 100644 0000000000000000000000000000000000000000 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e A\tdir/sub\n :000000 100644 0000000000000000000000000000000000000000 01e79c32a8c99c557f0757da7cb6d65b3414466d A\tfile0\n :000000 100644 0000000000000000000000000000000000000000 01e79c32a8c99c557f0757da7cb6d65b3414466d A\tfile2\ndiff --git a/t/t4013/diff.show_--patch-with-raw_side b/t/t4013/diff.show_--patch-with-raw_side\nindex 221b46a..b7566be 100644\n--- a/t/t4013/diff.show_--patch-with-raw_side\n+++ b/t/t4013/diff.show_--patch-with-raw_side\n@@ -4,7 +4,7 @@ Author: A U Thor \n Date: Mon Jun 26 00:03:00 2006 +0000\n \n Side\n-\n+---\n :100644 100644 35d242b... 7289e35... M\tdir/sub\n :100644 100644 01e79c3... f4615da... M\tfile0\n :000000 100644 0000000... 7289e35... A\tfile3\n-- \n1.6.5.2.181.gd6f41\n"},{"id":"126717","messageId":"7vy6mmltz9.fsf@alter.siamese.dyndns.org","threadId":"21471","inReplyTo":"1257283456-7007-1-git-send-email-bebarino@gmail.com","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-04T05:49:46Z","receivedAt":"2009-11-04T05:49:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Boyd writes:\n\n> Previously, the three dash marker (---) would only be added if the diff\n> output format was a patch and diffstat (usually -p and --stat). Now that\n> patches are always generated by format-patch regardless of the stat\n> format being used (--stat, --raw, --numstat, etc.), always add the three\n> dash marker when a patch is being generated and a stat option is used.\n>\n> This allows users to choose the stat format they want and unifies the\n> format of patches with stats. It also make patches easier to apply when\n> generated by format-patch with non-standard stat options as the stat is\n> no longer considered part of the commit message.\n>\n> Signed-off-by: Stephen Boyd \n> ---\n\nI actually am more worried about 68daa64 from 14 months ago, as I vaguely\nrecall seeing an explicit user request that in some community the diffstat\ninformation is unwanted on their mailing list, and I am reasonably sure\nthat \"-p suppresses --stat\" was done deliberately to satisfy them (even\nthough it may have been a suboptimal UI and --no-stat might have been a\nlot more straightforward).\n\nEven though I personally find the stat information very useful, I would be\nhappier if somebody reverts the bg/format-patch-p-noop series and instead\nfixes the regression caused by 68daa64, and does so without touching any\noutput from the low-level plumbing like diff-tree that may be used by\nscripts.\n\nWith older (say 1.6.0) git, format-patch with the -p option does not give\nthese three-dash lines, and it does look funny. Even though the same\nfunniness appears only when you use --raw or --numstat with the current\ncode, if we fix \"-p\" to suppress the default \"--stat\", this will become an\nissue again.\n\n> I'm not sure this is wanted though and I guess this could break people's\n> scripts. Are people actually using --numstat or --raw to put the stat into\n> the commit message?\n\nI am not worried so much about \"format-patch --any-option\" output; I think\nit is sane to have three-dash line in it and that is what people expect to\nsee.\n\nI however think people used \"diff-tree --pretty --raw\" as a mechanism to\nobtain statistics. A script can easily see where the header is and where\nmessages are (they are four-space indented), and what remains after\nstripping them give you the list of paths each commit touches. --numstat\nwas invented to help this kind of application gather line-level statistics\nmore easily, and I am a bit reluctant to suddenly start giving three-dash\nin their output. It will upset what is reading from the pipe downstream.\n\nIn an ideal world, I would probably say:\n\n * format-patch should have three-dash after the commit message, no matter\n what format the patch is asked for, and it always will give patch text.\n\n * format-patch -p should be reinstated as a way to ask for \"just patch\n text, no diffstat\". Introducing a new option --no-stat _in addition_\n to improve the UI is Ok.\n\n * format-patch -U should not be mistaken as a request to suppress\n diffstat; what 68daa64 _tried_ to do was worthy.\n\n * Other commands of \"log\" family that understand -p/-U to produce\n patch text should also give three-dash after the log message, and no\n three-dash when they don't produce patch text.\n"},{"id":"126722","messageId":"20091104063612.GA24263@coredump.intra.peff.net","threadId":"21471","inReplyTo":"7vy6mmltz9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-04T06:36:13Z","receivedAt":"2009-11-04T06:36:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 03, 2009 at 09:49:46PM -0800, Junio C Hamano wrote:\n\n> I actually am more worried about 68daa64 from 14 months ago, as I vaguely\n> recall seeing an explicit user request that in some community the diffstat\n> information is unwanted on their mailing list, and I am reasonably sure\n> that \"-p suppresses --stat\" was done deliberately to satisfy them (even\n> though it may have been a suboptimal UI and --no-stat might have been a\n> lot more straightforward).\n> \n> Even though I personally find the stat information very useful, I would be\n> happier if somebody reverts the bg/format-patch-p-noop series and instead\n> fixes the regression caused by 68daa64, and does so without touching any\n> output from the low-level plumbing like diff-tree that may be used by\n> scripts.\n\nI agree that 68daa64 is a hack (and I even noted in the commit log that\n\"-p\" is now a no-op). The problem is that we don't have the one critical\nbit of information in cmd_format_patch that we do in diff_opt_parse: was\nthe format set explicitly, or was it a side-effect of -U (or --binary,\nor maybe others).\n\nHere's a patch which fixes the regression, but it feels awfully hack-ish\nto me, as it treats \"-p\" specially in diff_opt (but in a way that no\nother existing code should care about). It would be \"cleaner\" to me to\nhave some infrastructure for keeping an implicit and an explicit format,\nand then merging them at the end. But I don't think we ever care about\nthis explicitness for any other formats, so this is at least simple.\n\nAnother option might be for format-patch to simply parse \"-p\" itself,\nsetting the format and marking an \"explicit\" flag. I'll look into that\nas an alternative.\n\n---\ndiff --git a/builtin-log.c b/builtin-log.c\nindex a359679..3b5819e 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -1034,8 +1034,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)\n \t\tdie(\"--check does not make sense\");\n \n-\tif (!rev.diffopt.output_format\n-\t\t|| rev.diffopt.output_format == DIFF_FORMAT_PATCH)\n+\tif (!rev.diffopt.output_format ||\n+\t (rev.diffopt.output_format == DIFF_FORMAT_PATCH &&\n+\t !rev.diffopt.explicit_patch_format))\n \t\trev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;\n \n \t/* Always generate a patch */\ndiff --git a/diff.c b/diff.c\nindex 9cd9693..9ce5520 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2643,8 +2643,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \tconst char *arg = av[0];\n \n \t/* Output format options */\n-\tif (!strcmp(arg, \"-p\") || !strcmp(arg, \"-u\"))\n+\tif (!strcmp(arg, \"-p\") || !strcmp(arg, \"-u\")) {\n \t\toptions->output_format |= DIFF_FORMAT_PATCH;\n+\t\toptions->explicit_patch_format = 1;\n+\t}\n \telse if (opt_arg(arg, 'U', \"unified\", &options->context))\n \t\toptions->output_format |= DIFF_FORMAT_PATCH;\n \telse if (!strcmp(arg, \"--raw\"))\ndiff --git a/diff.h b/diff.h\nindex 55f3203..406c0c6 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -90,6 +90,7 @@ struct diff_options {\n \tint skip_stat_unmatch;\n \tint line_termination;\n \tint output_format;\n+\tint explicit_patch_format;\n \tint pickaxe_opts;\n \tint rename_score;\n \tint rename_limit;\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex f826348..5689d59 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -455,6 +455,27 @@ test_expect_success 'format-patch respects -U' '\n \n '\n \n+cat > expect << EOF\n+\n+diff --git a/file b/file\n+index 40f36c6..2dc5c23 100644\n+--- a/file\n++++ b/file\n+@@ -14,3 +14,19 @@ C\n+ D\n+ E\n+ F\n++5\n+EOF\n+\n+test_expect_success 'format-patch -p suppresses stat' '\n+\n+\tgit format-patch -p -2 &&\n+\tsed -e \"1,/^$/d\" -e \"/^+5/q\" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&\n+\ttest_cmp expect output\n+\n+'\n+\n test_expect_success 'format-patch from a subdirectory (1)' '\n \tfilename=$(\n \t\trm -rf sub &&\n\n> With older (say 1.6.0) git, format-patch with the -p option does not give\n> these three-dash lines, and it does look funny. Even though the same\n> funniness appears only when you use --raw or --numstat with the current\n> code, if we fix \"-p\" to suppress the default \"--stat\", this will become an\n> issue again.\n\nMy test case checks the current output (i.e., missing dashes). I think\nit should probably have dashes, but that should be fixed in a separate\npatch.\n\n-Peff\n"},{"id":"126726","messageId":"6672d0160911032300w1a2dfdbck5b1db98f2059639b@mail.gmail.com","threadId":"21471","inReplyTo":"7vy6mmltz9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2009-11-04T07:00:58Z","receivedAt":"2009-11-04T07:00:58Z","isPatch":true,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"2009/11/4 Junio C Hamano :\n\n> In an ideal world, I would probably say:\n>\n>  * format-patch should have three-dash after the commit message, no matter\n>   what format the patch is asked for, and it always will give patch text.\n\nI agree.\n\n>  * format-patch -p should be reinstated as a way to ask for \"just patch\n>   text, no diffstat\".  Introducing a new option --no-stat _in addition_\n>   to improve the UI is Ok.\n\nSince -p has been broken for 14 months, is really necessary to reinstate\nit? (Or has the breakage not been reported because the people who care\nstill use a git version older than 14 months?)\n\nWhy not just add a new --no-stat option?\n\n>  * format-patch -U should not be mistaken as a request to suppress\n>   diffstat; what 68daa64 _tried_ to do was worthy.\n\nI agree.\n\n/Björn\n\n-- \nBjörn Gustavsson, Erlang/OTP, Ericsson AB\n"},{"id":"126727","messageId":"20091104071053.GB24263@coredump.intra.peff.net","threadId":"21471","inReplyTo":"20091104063612.GA24263@coredump.intra.peff.net","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-04T07:10:54Z","receivedAt":"2009-11-04T07:10:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2009 at 01:36:13AM -0500, Jeff King wrote:\n\n> Here's a patch which fixes the regression, but it feels awfully hack-ish\n> to me, as it treats \"-p\" specially in diff_opt (but in a way that no\n> other existing code should care about). It would be \"cleaner\" to me to\n> have some infrastructure for keeping an implicit and an explicit format,\n> and then merging them at the end. But I don't think we ever care about\n> this explicitness for any other formats, so this is at least simple.\n> \n> Another option might be for format-patch to simply parse \"-p\" itself,\n> setting the format and marking an \"explicit\" flag. I'll look into that\n> as an alternative.\n\nOK, doing that turned out to be much nicer. Less code, localized to\nformat-patch, and less confusing to read.\n\nThis patch goes on top of master, and terribly conflicts with Björn's\nchanges in the area. But I had the impression you wanted to revert those\nchanges for now anyway, so probably this should go in as a bug fix and\neverything else should be built on top. It actually would be an even\nsmaller change on top of his \"always show patch, even when other formats\nare given\" change, but I didn't want to depend on it.\n\n-- >8 --\nSubject: [PATCH] format-patch: make \"-p\" suppress diffstat\n\nOnce upon a time, format-patch would use its default stat\nplus patch format only when no diff format was given on the\ncommand line. This meant that \"format-patch -p\" would\nsuppress the stat and show just the patch.\n\nCommit 68daa64 changed this to keep the stat format when we\nhad an \"implicit\" patch format, like \"-U5\". As a side\neffect, this meant that an explicit patch format was now\nignored (because cmd_format_patch didn't know the reason\nthat the format was set way down in diff_opt_parse).\n\nThis patch unbreaks what 68daa64 did (while still preserving\nwhat 68daa64 was trying to do), reinstating \"-p\" to suppress\nthe default behavior. We do this by parsing \"-p\" ourselves\nin format-patch, and noting whether it was used explicitly.\n\nSigned-off-by: Jeff King \n---\nThe help text for \"-p\" is up for debate. It can also of course be used\nas \"show patch in addition to other things you already specified on the\ncommand line\". But I wanted to point out that its main use is the side\neffect that it suppresses the default output. I am open to suggestions.\n\n builtin-log.c | 9 +++++++--\n t/t4014-format-patch.sh | 21 +++++++++++++++++++++\n 2 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 207a361..da79047 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -891,6 +891,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tstruct patch_ids ids;\n \tchar *add_signoff = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tint use_patch_format = 0;\n \tconst struct option builtin_format_patch_options[] = {\n \t\t{ OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n \t\t\t \"use [PATCH n/m] even with a single patch\",\n@@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_CALLBACK, 0, \"thread\", &thread, \"style\",\n \t\t\t \"enable message threading, styles: shallow, deep\",\n \t\t\t PARSE_OPT_OPTARG, thread_callback },\n+\t\tOPT_BOOLEAN('p', NULL, &use_patch_format,\n+\t\t\t\"show patch format instead of default (patch + stat)\"),\n \t\tOPT_END()\n \t};\n \n@@ -1027,8 +1030,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (argc > 1)\n \t\tdie (\"unrecognized argument: %s\", argv[1]);\n \n-\tif (!rev.diffopt.output_format\n-\t\t|| rev.diffopt.output_format == DIFF_FORMAT_PATCH)\n+\tif (patch_format)\n+\t\trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n+\telse if (!rev.diffopt.output_format ||\n+\t\t rev.diffopt.output_format == DIFF_FORMAT_PATCH)\n \t\trev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY | DIFF_FORMAT_PATCH;\n \n \tif (!DIFF_OPT_TST(&rev.diffopt, TEXT) && !no_binary_diff)\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 531f5b7..cab6ce2 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -455,6 +455,27 @@ test_expect_success 'format-patch respects -U' '\n \n '\n \n+cat > expect << EOF\n+\n+diff --git a/file b/file\n+index 40f36c6..2dc5c23 100644\n+--- a/file\n++++ b/file\n+@@ -14,3 +14,19 @@ C\n+ D\n+ E\n+ F\n++5\n+EOF\n+\n+test_expect_success 'format-patch -p suppresses stat' '\n+\n+\tgit format-patch -p -2 &&\n+\tsed -e \"1,/^$/d\" -e \"/^+5/q\" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&\n+\ttest_cmp expect output\n+\n+'\n+\n test_expect_success 'format-patch from a subdirectory (1)' '\n \tfilename=$(\n \t\trm -rf sub &&\n-- \n1.6.5.2.308.g2bb5\n"},{"id":"126729","messageId":"20091104071940.GA15011@coredump.intra.peff.net","threadId":"21471","inReplyTo":"20091104071053.GB24263@coredump.intra.peff.net","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-04T07:19:40Z","receivedAt":"2009-11-04T07:19:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2009 at 02:10:54AM -0500, Jeff King wrote:\n\n> Subject: [PATCH] format-patch: make \"-p\" suppress diffstat\n\nUgh. And here is one that actually compiles. Yes, I actually did test\nit, but the one-line typo was sitting uncommitted in my workdir.\n\n-- >8 --\nSubject: [PATCH] format-patch: make \"-p\" suppress diffstat\n\nOnce upon a time, format-patch would use its default stat\nplus patch format only when no diff format was given on the\ncommand line. This meant that \"format-patch -p\" would\nsuppress the stat and show just the patch.\n\nCommit 68daa64 changed this to keep the stat format when we\nhad an \"implicit\" patch format, like \"-U5\". As a side\neffect, this meant that an explicit patch format was now\nignored (because cmd_format_patch didn't know the reason\nthat the format was set way down in diff_opt_parse).\n\nThis patch unbreaks what 68daa64 did (while still preserving\nwhat 68daa64 was trying to do), reinstating \"-p\" to suppress\nthe default behavior. We do this by parsing \"-p\" ourselves\nin format-patch, and noting whether it was used explicitly.\n\nSigned-off-by: Jeff King \n---\n builtin-log.c | 9 +++++++--\n t/t4014-format-patch.sh | 21 +++++++++++++++++++++\n 2 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 207a361..0ff032b 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -891,6 +891,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tstruct patch_ids ids;\n \tchar *add_signoff = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tint use_patch_format = 0;\n \tconst struct option builtin_format_patch_options[] = {\n \t\t{ OPTION_CALLBACK, 'n', \"numbered\", &numbered, NULL,\n \t\t\t \"use [PATCH n/m] even with a single patch\",\n@@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t{ OPTION_CALLBACK, 0, \"thread\", &thread, \"style\",\n \t\t\t \"enable message threading, styles: shallow, deep\",\n \t\t\t PARSE_OPT_OPTARG, thread_callback },\n+\t\tOPT_BOOLEAN('p', NULL, &use_patch_format,\n+\t\t\t\"show patch format instead of default (patch + stat)\"),\n \t\tOPT_END()\n \t};\n \n@@ -1027,8 +1030,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (argc > 1)\n \t\tdie (\"unrecognized argument: %s\", argv[1]);\n \n-\tif (!rev.diffopt.output_format\n-\t\t|| rev.diffopt.output_format == DIFF_FORMAT_PATCH)\n+\tif (use_patch_format)\n+\t\trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n+\telse if (!rev.diffopt.output_format ||\n+\t\t rev.diffopt.output_format == DIFF_FORMAT_PATCH)\n \t\trev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY | DIFF_FORMAT_PATCH;\n \n \tif (!DIFF_OPT_TST(&rev.diffopt, TEXT) && !no_binary_diff)\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 531f5b7..cab6ce2 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -455,6 +455,27 @@ test_expect_success 'format-patch respects -U' '\n \n '\n \n+cat > expect << EOF\n+\n+diff --git a/file b/file\n+index 40f36c6..2dc5c23 100644\n+--- a/file\n++++ b/file\n+@@ -14,3 +14,19 @@ C\n+ D\n+ E\n+ F\n++5\n+EOF\n+\n+test_expect_success 'format-patch -p suppresses stat' '\n+\n+\tgit format-patch -p -2 &&\n+\tsed -e \"1,/^$/d\" -e \"/^+5/q\" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&\n+\ttest_cmp expect output\n+\n+'\n+\n test_expect_success 'format-patch from a subdirectory (1)' '\n \tfilename=$(\n \t\trm -rf sub &&\n-- \n1.6.5.2.308.g2bb5\n"},{"id":"126731","messageId":"7vljimkaxf.fsf@alter.siamese.dyndns.org","threadId":"21471","inReplyTo":"20091104063612.GA24263@coredump.intra.peff.net","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-04T07:26:36Z","receivedAt":"2009-11-04T07:26:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Nov 03, 2009 at 09:49:46PM -0800, Junio C Hamano wrote:\n>\n>> Even though I personally find the stat information very useful, I would be\n>> happier if somebody reverts the bg/format-patch-p-noop series and instead\n>> fixes the regression caused by 68daa64, and does so without touching any\n>> output from the low-level plumbing like diff-tree that may be used by\n>> scripts.\n>\n> I agree that 68daa64 is a hack (and I even noted in the commit log that\n> \"-p\" is now a no-op).\n\nCorrect, and thanks---it was not your fault and I didn't mean to blame\nyou. If anything it was mine.\n\n> The problem is that we don't have the one critical\n> bit of information in cmd_format_patch that we do in diff_opt_parse: was\n> the format set explicitly, or was it a side-effect of -U (or --binary,\n> or maybe others).\n\nThe appoarch your \"how about this\" takes feels right. We did discuss \"set\nof hardwired defaults specific to the individual commands, that are masked\nby set of defaults read from the config, that are in turn masked by set of\ncommand line options\", but I do not think that level of complexity is worth\nfor this \"is it -U or -p\" issue.\n\n> My test case checks the current output (i.e., missing dashes). I think\n> it should probably have dashes, but that should be fixed in a separate\n> patch.\n\nAgreed.\n"},{"id":"126734","messageId":"7vhbtakapf.fsf@alter.siamese.dyndns.org","threadId":"21471","inReplyTo":"6672d0160911032300w1a2dfdbck5b1db98f2059639b@mail.gmail.com","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-04T07:31:24Z","receivedAt":"2009-11-04T07:31:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Björn Gustavsson writes:\n\n> Since -p has been broken for 14 months, is really necessary to reinstate\n> it? (Or has the breakage not been reported because the people who care\n> still use a git version older than 14 months?)\n\nOh, 1.6.0 has the old behaviour and we see many people still on 1.5.X\nseries. Hopefully nobody uses 1.4.X series anymore but I wouldn't be\noverly surprised if somebody raised hand in the next 6 hours after seeing\nthis message ;-)\n\n> Why not just add a new --no-stat option?\n\nI am all for teaching _new_ people to say \"format-patch --no-stat\", but it\nwon't help people who have been happy with 1.6.0 when they update, if they\nhave to change their \"format-patch -p\" to \"format-patch --no-stat\".\n"},{"id":"126735","messageId":"4AF12EC5.4030407@gmail.com","threadId":"21471","inReplyTo":"20091104071940.GA15011@coredump.intra.peff.net","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-11-04T07:35:33Z","receivedAt":"2009-11-04T07:35:33Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"Jeff King wrote:\n>\n> This patch unbreaks what 68daa64 did (while still preserving\n> what 68daa64 was trying to do), reinstating \"-p\" to suppress\n> the default behavior. We do this by parsing \"-p\" ourselves\n> in format-patch, and noting whether it was used explicitly.\n>\n> Signed-off-by: Jeff King \n> ---\n> \n\nThis looks good to me; covering 2 and 3 of Junio's TODO list.\n\n> @@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\t{ OPTION_CALLBACK, 0, \"thread\", &thread, \"style\",\n> \t\t\t \"enable message threading, styles: shallow, deep\",\n> \t\t\t PARSE_OPT_OPTARG, thread_callback },\n> +\t\tOPT_BOOLEAN('p', NULL, &use_patch_format,\n> +\t\t\t\"show patch format instead of default (patch + stat)\"),\n> \t\tOPT_END()\n> \t};\n\nI don't imagine we want this option in the messaging group though. Can \nyou move it up?\n"},{"id":"126736","messageId":"20091104073731.GA15408@coredump.intra.peff.net","threadId":"21471","inReplyTo":"4AF12EC5.4030407@gmail.com","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-04T07:37:31Z","receivedAt":"2009-11-04T07:37:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 03, 2009 at 11:35:33PM -0800, Stephen Boyd wrote:\n\n> >@@ -940,6 +941,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> > \t\t{ OPTION_CALLBACK, 0, \"thread\", &thread, \"style\",\n> > \t\t\t \"enable message threading, styles: shallow, deep\",\n> > \t\t\t PARSE_OPT_OPTARG, thread_callback },\n> >+\t\tOPT_BOOLEAN('p', NULL, &use_patch_format,\n> >+\t\t\t\"show patch format instead of default (patch + stat)\"),\n> > \t\tOPT_END()\n> > \t};\n> \n> I don't imagine we want this option in the messaging group though.\n> Can you move it up?\n\nThanks, good catch. I just tacked it onto the end, forgetting that we\nwere using grouping. Junio, can you tweak it, or do you want a resend?\n\n-Peff\n"},{"id":"126738","messageId":"6672d0160911032349k2e67a4betd04e311dfdcf2c28@mail.gmail.com","threadId":"21471","inReplyTo":"20091104071053.GB24263@coredump.intra.peff.net","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2009-11-04T07:49:48Z","receivedAt":"2009-11-04T07:49:48Z","isPatch":true,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"On Wed, Nov 4, 2009 at 8:10 AM, Jeff King wrote:\n\n> This patch goes on top of master, and terribly conflicts with Björn's\n> changes in the area. But I had the impression you wanted to revert those\n> changes for now anyway, so probably this should go in as a bug fix and\n> everything else should be built on top. It actually would be an even\n> smaller change on top of his \"always show patch, even when other formats\n> are given\" change, but I didn't want to depend on it.\n\nNo problem. I can re-implement my patch series on top of your patch.\n\n/Björn\n-- \nBjörn Gustavsson, Erlang/OTP, Ericsson AB\n"},{"id":"126788","messageId":"7vy6mmgo06.fsf@alter.siamese.dyndns.org","threadId":"21471","inReplyTo":"20091104073731.GA15408@coredump.intra.peff.net","subject":"Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-04T18:10:01Z","receivedAt":"2009-11-04T18:10:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> I don't imagine we want this option in the messaging group though.\n>> Can you move it up?\n>\n> Thanks, good catch. I just tacked it onto the end, forgetting that we\n> were using grouping. Junio, can you tweak it, or do you want a resend?\n\nJust ater \"no-binary\" would be a good place for this to go; done.\n"}]}