{"thread":{"id":"58798","subject":"[PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd","startedAt":"2022-11-14T09:51:13Z","lastAt":"2022-12-23T01:35:01Z","messageCount":6,"participants":["Eric Wong","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"467232","messageId":"20221114094114.18986-1-e@80x24.org","threadId":"58798","inReplyTo":null,"subject":"[PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-11-14T09:41:14Z","receivedAt":"2022-11-14T09:51:13Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"mboxrd is a superior output format when used with --stdout and\nneeds more exposure.  Including pretty-formats.txt would be\nexcessive, since documenting --pretty= for `git format-patch'\nwould likely be confusing to users.\n\nInstead of documenting --pretty, add an --mboxrd alias to save\nkeystrokes and improve documentation.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n Also, --mboxrd without --stdout makes no sense to me,\n so I'm considering warning on it...\n\n Side note: some of the OPT_* switches might be misplaced\n under the \"Messaging\" OPT_GROUP...\n\n Documentation/git-format-patch.txt     | 6 +++++-\n builtin/log.c                          | 7 +++++++\n contrib/completion/git-completion.bash | 2 +-\n t/t4014-format-patch.sh                | 6 ++++--\n t/t4150-am.sh                          | 2 +-\n 5 files changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex dfcc7da4c211..b080d3c61e31 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -9,7 +9,7 @@ git-format-patch - Prepare patches for e-mail submission\n SYNOPSIS\n --------\n [verse]\n-'git format-patch' [-k] [(-o|--output-directory) <dir> | --stdout]\n+'git format-patch' [-k] [(-o|--output-directory) <dir> | --stdout] [--mboxrd]\n \t\t   [--no-thread | --thread[=<style>]]\n \t\t   [(--attach|--inline)[=<boundary>] | --no-attach]\n \t\t   [-s | --signoff]\n@@ -145,6 +145,10 @@ include::diff-options.txt[]\n \tPrint all commits to the standard output in mbox format,\n \tinstead of creating a file for each one.\n \n+--mboxrd::\n+\tUse the robust \"mboxrd\" format with `--stdout` to escape\n+\t\"^>+From \" lines.\n+\n --attach[=<boundary>]::\n \tCreate multipart/mixed attachment, the first part of\n \twhich is the commit message and the patch itself in the\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 5eafcf26b49b..13f5deb7a5c0 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1872,6 +1872,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tstruct strbuf rdiff2 = STRBUF_INIT;\n \tstruct strbuf rdiff_title = STRBUF_INIT;\n \tint creation_factor = -1;\n+\tint mboxrd = 0;\n \n \tconst struct option builtin_format_patch_options[] = {\n \t\tOPT_CALLBACK_F('n', \"numbered\", &numbered, NULL,\n@@ -1883,6 +1884,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('s', \"signoff\", &do_signoff, N_(\"add a Signed-off-by trailer\")),\n \t\tOPT_BOOL(0, \"stdout\", &use_stdout,\n \t\t\t    N_(\"print patches to standard out\")),\n+\t\tOPT_BOOL(0, \"mboxrd\", &mboxrd,\n+\t\t\t    N_(\"use the robust mboxrd format with --stdout\")),\n \t\tOPT_BOOL(0, \"cover-letter\", &cover_letter,\n \t\t\t    N_(\"generate a cover letter\")),\n \t\tOPT_BOOL(0, \"numbered-files\", &just_numbers,\n@@ -2106,6 +2109,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t\t  rev.diffopt.close_file, \"--output\",\n \t\t\t\t  !!output_directory, \"--output-directory\");\n \n+\t/* should we warn on --mboxrd w/o --stdout? */\n+\tif (mboxrd)\n+\t\trev.commit_format = CMIT_FMT_MBOXRD;\n+\n \tif (use_stdout) {\n \t\tsetup_pager();\n \t} else if (!rev.diffopt.close_file) {\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex ba5c395d2d80..f6e2fbdcfa68 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1815,7 +1815,7 @@ _git_fetch ()\n \n __git_format_patch_extra_options=\"\n \t--full-index --not --all --no-prefix --src-prefix=\n-\t--dst-prefix= --notes\n+\t--dst-prefix= --notes --mboxrd\n \"\n \n _git_format_patch ()\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex de1da4673da9..69ed8b1ffaa1 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2281,7 +2281,7 @@ test_expect_success 'format-patch --attach cover-letter only is non-multipart' '\n \ttest_line_count = 1 output\n '\n \n-test_expect_success 'format-patch --pretty=mboxrd' '\n+test_expect_success 'format-patch --mboxrd' '\n \tsp=\" \" &&\n \tcat >msg <<-INPUT_END &&\n \tmboxrd should escape the body\n@@ -2316,7 +2316,9 @@ test_expect_success 'format-patch --pretty=mboxrd' '\n \tINPUT_END\n \n \tC=$(git commit-tree HEAD^^{tree} -p HEAD <msg) &&\n-\tgit format-patch --pretty=mboxrd --stdout -1 $C~1..$C >patch &&\n+\tgit format-patch --mboxrd --stdout -1 $C~1..$C >patch &&\n+\tgit format-patch --pretty=mboxrd --stdout -1 $C~1..$C >compat &&\n+\ttest_cmp patch compat &&\n \tgit grep -h --no-index -A11 \\\n \t\t\"^>From could trip up a loose mbox parser\" patch >actual &&\n \ttest_cmp expect actual\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex cdad4b688078..9a128c16a6ee 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -1033,7 +1033,7 @@ test_expect_success 'am --patch-format=mboxrd handles mboxrd' '\n \t>From extra escape for reversibility\n \tINPUT_END\n \tgit commit -F msg &&\n-\tgit format-patch --pretty=mboxrd --stdout -1 >mboxrd1 &&\n+\tgit format-patch --mboxrd --stdout -1 >mboxrd1 &&\n \tgrep \"^>From could trip up a loose mbox parser\" mboxrd1 &&\n \tgit checkout -f first &&\n \tgit am --patch-format=mboxrd mboxrd1 &&\n"},{"id":"467256","messageId":"221114.86leodlbix.gmgdl@evledraar.gmail.com","threadId":"58798","inReplyTo":"20221114094114.18986-1-e@80x24.org","subject":"Re: [PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-14T16:53:40Z","receivedAt":"2022-11-14T16:58:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 14 2022, Eric Wong wrote:\n\n> mboxrd is a superior output format when used with --stdout and\n> needs more exposure.  Including pretty-formats.txt would be\n> excessive, since documenting --pretty= for `git format-patch'\n> would likely be confusing to users.\n>\n> Instead of documenting --pretty, add an --mboxrd alias to save\n> keystrokes and improve documentation.\n>\n> Signed-off-by: Eric Wong <e@80x24.org>\n> ---\n>  Also, --mboxrd without --stdout makes no sense to me,\n>  so I'm considering warning on it...\n>\n>  Side note: some of the OPT_* switches might be misplaced\n>  under the \"Messaging\" OPT_GROUP...\n\nMakes sense, but....\n\n> +--mboxrd::\n> +\tUse the robust \"mboxrd\" format with `--stdout` to escape\n> +\t\"^>+From \" lines.\n> +\n\n...Rather than repeat ourselves, shouldn't we (or in addition to this)\nlink to a manpage that discusses the --pretty=* formats. I was going to\nsay that you can also use the \"ifdef\" asciidoc syntax, but for one\nparagraph that's probably overkill...\n\n>  --attach[=<boundary>]::\n>  \tCreate multipart/mixed attachment, the first part of\n>  \twhich is the commit message and the patch itself in the\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 5eafcf26b49b..13f5deb7a5c0 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1872,6 +1872,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>  \tstruct strbuf rdiff2 = STRBUF_INIT;\n>  \tstruct strbuf rdiff_title = STRBUF_INIT;\n>  \tint creation_factor = -1;\n> +\tint mboxrd = 0;\n>  \n>  \tconst struct option builtin_format_patch_options[] = {\n>  \t\tOPT_CALLBACK_F('n', \"numbered\", &numbered, NULL,\n> @@ -1883,6 +1884,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>  \t\tOPT_BOOL('s', \"signoff\", &do_signoff, N_(\"add a Signed-off-by trailer\")),\n>  \t\tOPT_BOOL(0, \"stdout\", &use_stdout,\n>  \t\t\t    N_(\"print patches to standard out\")),\n> +\t\tOPT_BOOL(0, \"mboxrd\", &mboxrd,\n> +\t\t\t    N_(\"use the robust mboxrd format with --stdout\")),\n>  \t\tOPT_BOOL(0, \"cover-letter\", &cover_letter,\n>  \t\t\t    N_(\"generate a cover letter\")),\n>  \t\tOPT_BOOL(0, \"numbered-files\", &just_numbers,\n> @@ -2106,6 +2109,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t  rev.diffopt.close_file, \"--output\",\n>  \t\t\t\t  !!output_directory, \"--output-directory\");\n>  \n> +\t/* should we warn on --mboxrd w/o --stdout? */\n\nDoes that go for --pretty=mboxrd too?\n\n> +\tif (mboxrd)\n> +\t\trev.commit_format = CMIT_FMT_MBOXRD;\n> +\n>  \tif (use_stdout) {\n>  \t\tsetup_pager();\n>  \t} else if (!rev.diffopt.close_file) {\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index ba5c395d2d80..f6e2fbdcfa68 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1815,7 +1815,7 @@ _git_fetch ()\n>  \n>  __git_format_patch_extra_options=\"\n>  \t--full-index --not --all --no-prefix --src-prefix=\n> -\t--dst-prefix= --notes\n> +\t--dst-prefix= --notes --mboxrd\n>  \"\n>  \n>  _git_format_patch ()\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index de1da4673da9..69ed8b1ffaa1 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -2281,7 +2281,7 @@ test_expect_success 'format-patch --attach cover-letter only is non-multipart' '\n>  \ttest_line_count = 1 output\n>  '\n>  \n> -test_expect_success 'format-patch --pretty=mboxrd' '\n> +test_expect_success 'format-patch --mboxrd' '\n>  \tsp=\" \" &&\n>  \tcat >msg <<-INPUT_END &&\n>  \tmboxrd should escape the body\n> @@ -2316,7 +2316,9 @@ test_expect_success 'format-patch --pretty=mboxrd' '\n>  \tINPUT_END\n>  \n>  \tC=$(git commit-tree HEAD^^{tree} -p HEAD <msg) &&\n> -\tgit format-patch --pretty=mboxrd --stdout -1 $C~1..$C >patch &&\n> +\tgit format-patch --mboxrd --stdout -1 $C~1..$C >patch &&\n> +\tgit format-patch --pretty=mboxrd --stdout -1 $C~1..$C >compat &&\n> +\ttest_cmp patch compat &&\n>  \tgit grep -h --no-index -A11 \\\n>  \t\t\"^>From could trip up a loose mbox parser\" patch >actual &&\n>  \ttest_cmp expect actual\n> diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> index cdad4b688078..9a128c16a6ee 100755\n> --- a/t/t4150-am.sh\n> +++ b/t/t4150-am.sh\n> @@ -1033,7 +1033,7 @@ test_expect_success 'am --patch-format=mboxrd handles mboxrd' '\n>  \t>From extra escape for reversibility\n>  \tINPUT_END\n>  \tgit commit -F msg &&\n> -\tgit format-patch --pretty=mboxrd --stdout -1 >mboxrd1 &&\n> +\tgit format-patch --mboxrd --stdout -1 >mboxrd1 &&\n>  \tgrep \"^>From could trip up a loose mbox parser\" mboxrd1 &&\n>  \tgit checkout -f first &&\n>  \tgit am --patch-format=mboxrd mboxrd1 &&\n\nDoesn't this leave us without coverage for the --pretty=mboxrd variant?\n\nI must admit I'm not a big outright fan of this, the \"log-like\" is\nconfusing enough, with those accepting some options, ignoring others\netc, now we're adding command-specific aliases too...\n\nWhy not just document that we accept --pretty=<some subset>? E.g. see\n\"range-diff\"'s docs for an existing case where we discuss accepting a\nlimited subset.\n"},{"id":"467261","messageId":"20221114205909.GA14736@dcvr","threadId":"58798","inReplyTo":"221114.86leodlbix.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-11-14T20:59:09Z","receivedAt":"2022-11-14T20:59:13Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> On Mon, Nov 14 2022, Eric Wong wrote:\n> \n> > mboxrd is a superior output format when used with --stdout and\n> > needs more exposure.  Including pretty-formats.txt would be\n> > excessive, since documenting --pretty= for `git format-patch'\n> > would likely be confusing to users.\n> >\n> > Instead of documenting --pretty, add an --mboxrd alias to save\n> > keystrokes and improve documentation.\n> >\n> > Signed-off-by: Eric Wong <e@80x24.org>\n> > ---\n> >  Also, --mboxrd without --stdout makes no sense to me,\n> >  so I'm considering warning on it...\n> >\n> >  Side note: some of the OPT_* switches might be misplaced\n> >  under the \"Messaging\" OPT_GROUP...\n> \n> Makes sense, but....\n> \n> > +--mboxrd::\n> > +\tUse the robust \"mboxrd\" format with `--stdout` to escape\n> > +\t\"^>+From \" lines.\n> > +\n> \n> ...Rather than repeat ourselves, shouldn't we (or in addition to this)\n> link to a manpage that discusses the --pretty=* formats. I was going to\n> say that you can also use the \"ifdef\" asciidoc syntax, but for one\n> paragraph that's probably overkill...\n\nAs I noted in the commit message, I think discussing --pretty=*\nin the context of format-patch would be confusing for users.\n\n> >  --attach[=<boundary>]::\n> >  \tCreate multipart/mixed attachment, the first part of\n> >  \twhich is the commit message and the patch itself in the\n> > diff --git a/builtin/log.c b/builtin/log.c\n> > index 5eafcf26b49b..13f5deb7a5c0 100644\n> > --- a/builtin/log.c\n> > +++ b/builtin/log.c\n> > @@ -1872,6 +1872,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> >  \tstruct strbuf rdiff2 = STRBUF_INIT;\n> >  \tstruct strbuf rdiff_title = STRBUF_INIT;\n> >  \tint creation_factor = -1;\n> > +\tint mboxrd = 0;\n> >  \n> >  \tconst struct option builtin_format_patch_options[] = {\n> >  \t\tOPT_CALLBACK_F('n', \"numbered\", &numbered, NULL,\n> > @@ -1883,6 +1884,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> >  \t\tOPT_BOOL('s', \"signoff\", &do_signoff, N_(\"add a Signed-off-by trailer\")),\n> >  \t\tOPT_BOOL(0, \"stdout\", &use_stdout,\n> >  \t\t\t    N_(\"print patches to standard out\")),\n> > +\t\tOPT_BOOL(0, \"mboxrd\", &mboxrd,\n> > +\t\t\t    N_(\"use the robust mboxrd format with --stdout\")),\n> >  \t\tOPT_BOOL(0, \"cover-letter\", &cover_letter,\n> >  \t\t\t    N_(\"generate a cover letter\")),\n> >  \t\tOPT_BOOL(0, \"numbered-files\", &just_numbers,\n> > @@ -2106,6 +2109,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> >  \t\t\t\t  rev.diffopt.close_file, \"--output\",\n> >  \t\t\t\t  !!output_directory, \"--output-directory\");\n> >  \n> > +\t/* should we warn on --mboxrd w/o --stdout? */\n> \n> Does that go for --pretty=mboxrd too?\n\nAgain, I prefer to mention --pretty= as little as possible\nwhen it comes to format-patch\n\n> > +\tif (mboxrd)\n> > +\t\trev.commit_format = CMIT_FMT_MBOXRD;\n> > +\n\nIt could be something like this:\n\n\tif (rev.commit_format == CMIT_FMT_MBOXRD && !use_stdout)\n\t\twarning(\"mboxrd without --stdout makes no sense\\n\");\n\nBut I'm on the fence about the warning.\n\n> >  \tif (use_stdout) {\n> >  \t\tsetup_pager();\n> >  \t} else if (!rev.diffopt.close_file) {\n> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> > index ba5c395d2d80..f6e2fbdcfa68 100644\n> > --- a/contrib/completion/git-completion.bash\n> > +++ b/contrib/completion/git-completion.bash\n> > @@ -1815,7 +1815,7 @@ _git_fetch ()\n> >  \n> >  __git_format_patch_extra_options=\"\n> >  \t--full-index --not --all --no-prefix --src-prefix=\n> > -\t--dst-prefix= --notes\n> > +\t--dst-prefix= --notes --mboxrd\n> >  \"\n> >  \n> >  _git_format_patch ()\n> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> > index de1da4673da9..69ed8b1ffaa1 100755\n> > --- a/t/t4014-format-patch.sh\n> > +++ b/t/t4014-format-patch.sh\n> > @@ -2281,7 +2281,7 @@ test_expect_success 'format-patch --attach cover-letter only is non-multipart' '\n> >  \ttest_line_count = 1 output\n> >  '\n> >  \n> > -test_expect_success 'format-patch --pretty=mboxrd' '\n> > +test_expect_success 'format-patch --mboxrd' '\n> >  \tsp=\" \" &&\n> >  \tcat >msg <<-INPUT_END &&\n> >  \tmboxrd should escape the body\n> > @@ -2316,7 +2316,9 @@ test_expect_success 'format-patch --pretty=mboxrd' '\n> >  \tINPUT_END\n> >  \n> >  \tC=$(git commit-tree HEAD^^{tree} -p HEAD <msg) &&\n> > -\tgit format-patch --pretty=mboxrd --stdout -1 $C~1..$C >patch &&\n> > +\tgit format-patch --mboxrd --stdout -1 $C~1..$C >patch &&\n> > +\tgit format-patch --pretty=mboxrd --stdout -1 $C~1..$C >compat &&\n> > +\ttest_cmp patch compat &&\n> >  \tgit grep -h --no-index -A11 \\\n> >  \t\t\"^>From could trip up a loose mbox parser\" patch >actual &&\n> >  \ttest_cmp expect actual\n> > diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n> > index cdad4b688078..9a128c16a6ee 100755\n> > --- a/t/t4150-am.sh\n> > +++ b/t/t4150-am.sh\n> > @@ -1033,7 +1033,7 @@ test_expect_success 'am --patch-format=mboxrd handles mboxrd' '\n> >  \t>From extra escape for reversibility\n> >  \tINPUT_END\n> >  \tgit commit -F msg &&\n> > -\tgit format-patch --pretty=mboxrd --stdout -1 >mboxrd1 &&\n> > +\tgit format-patch --mboxrd --stdout -1 >mboxrd1 &&\n> >  \tgrep \"^>From could trip up a loose mbox parser\" mboxrd1 &&\n> >  \tgit checkout -f first &&\n> >  \tgit am --patch-format=mboxrd mboxrd1 &&\n> \n> Doesn't this leave us without coverage for the --pretty=mboxrd variant?\n\nThese two lines were added to t/t4014-format-patch.sh above to\ntest --pretty=mboxrd:\n\n+\tgit format-patch --pretty=mboxrd --stdout -1 $C~1..$C >compat &&\n+\ttest_cmp patch compat &&\n\n> I must admit I'm not a big outright fan of this, the \"log-like\" is\n> confusing enough, with those accepting some options, ignoring others\n> etc, now we're adding command-specific aliases too...\n> \n> Why not just document that we accept --pretty=<some subset>? E.g. see\n> \"range-diff\"'s docs for an existing case where we discuss accepting a\n> limited subset.\n\nAgain, I think discussing --pretty= is confusing to users\nif they might end up using raw/full/fuller/etc\n(especially if they're relying on tab completion).\n\nI that was the reason --pretty= was never documented in the\nformat-patch manpage (which I had nothing to do with).\n\nWhich section of the range-diff man page are you referring to?\n"},{"id":"469245","messageId":"xmqqv8m9wd7r.fsf@gitster.g","threadId":"58798","inReplyTo":"20221114205909.GA14736@dcvr","subject":"Re: [PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-18T04:24:56Z","receivedAt":"2022-12-18T04:25:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> As I noted in the commit message, I think discussing --pretty=*\n> in the context of format-patch would be confusing for users.\n\nSensible.\n\n>> > +\tif (mboxrd)\n>> > +\t\trev.commit_format = CMIT_FMT_MBOXRD;\n>> > +\n>\n> It could be something like this:\n>\n> \tif (rev.commit_format == CMIT_FMT_MBOXRD && !use_stdout)\n> \t\twarning(\"mboxrd without --stdout makes no sense\\n\");\n>\n> But I'm on the fence about the warning.\n\nDoes it really hurt when generating individual files, or does it\nnaturally degenerate to the same as the plain old mbox, or\nsomething?  If it does not hurt, then let's not clutter the output\nwith a message that may make the user worried unnecessarily.\n\nHaving said all that, if --pretty=mboxrd is usable, do we really\nneed the --mboxrd short-hand?  If we plan to eventually switch the\ndefault output format to mboxrd (which I am assuming your longer\nterm vision), wouldn't it be the traditional format that may need a\nshort-hand when something goes wrong?\n\nThis change does not seem to be something we cannot live without,\nand as a step in the direction to move all of us to mboxrd, this\nfeels somewhat a roundabout step.  I wonder if it would be more\ndirect to\n\n - declare that we will eventually switch to use mboxrd by default;\n\n - give a configuration knob to retain the current email as default;\n\n - do the usual deprecation dance.\n\nAfter all, between email and mboxrd, the e-mailed patch format is\nnot something we choose per-invocation basis, is it?\n\nPicking a suitable format per project and setting it in .git/config,\nor picking a suitble format for yourself and setting it in\n~/.gitconfig, and not having to think about it afterwards may be a\nbetter use of our users' time.\n\nThanks.\n\n"},{"id":"469478","messageId":"20221222201619.M435985@dcvr","threadId":"58798","inReplyTo":"xmqqv8m9wd7r.fsf@gitster.g","subject":"[PATCH v2] format-patch: support format.mboxrd with --stdout","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-12-22T20:16:19Z","receivedAt":"2022-12-22T20:16:27Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <e@80x24.org> writes:\n> >> > +\tif (mboxrd)\n> >> > +\t\trev.commit_format = CMIT_FMT_MBOXRD;\n> >> > +\n> >\n> > It could be something like this:\n> >\n> > \tif (rev.commit_format == CMIT_FMT_MBOXRD && !use_stdout)\n> > \t\twarning(\"mboxrd without --stdout makes no sense\\n\");\n> >\n> > But I'm on the fence about the warning.\n\nOK, so the warning won't be necessary if using format.mboxrd\nvia config.\n\n> Does it really hurt when generating individual files, or does it\n> naturally degenerate to the same as the plain old mbox, or\n> something?  If it does not hurt, then let's not clutter the output\n> with a message that may make the user worried unnecessarily.\n\nmboxrd is bad for individual files since it can leave '>From'\nescaping if applied w/o `--patch-format mboxrd\".  My revised\npatch only enables mboxrd if --stdout is in use.\n\n> Having said all that, if --pretty=mboxrd is usable, do we really\n> need the --mboxrd short-hand?  If we plan to eventually switch the\n> default output format to mboxrd (which I am assuming your longer\n> term vision), wouldn't it be the traditional format that may need a\n> short-hand when something goes wrong?\n\nActually, I never considered making mboxrd a config file knob.\nBut yes, it seems like a good idea.\n\nI hadn't considered making it the default, actually.  The current\n`--pretty=email --stdout' is probably \"good enough\" for many\nusers since the \"From \"-splitting in mailsplit is strict and\nlegitimate commit messages are unlikely to have splittable \"From \"\nlines.\n\n> This change does not seem to be something we cannot live without,\n\nThat double negative confused me :x\n\n\"This change seems to be something we can live without\"\n\nYes, I agree.\n\n> and as a step in the direction to move all of us to mboxrd, this\n> feels somewhat a roundabout step.  I wonder if it would be more\n> direct to\n> \n>  - declare that we will eventually switch to use mboxrd by default;\n> \n>  - give a configuration knob to retain the current email as default;\n> \n>  - do the usual deprecation dance.\n\nI don't know if changing the default+deprecation is necessary,\nbut thanks for suggesting a config knob.\n\n> After all, between email and mboxrd, the e-mailed patch format is\n> not something we choose per-invocation basis, is it?\n\nRight.\n\n> Picking a suitable format per project and setting it in .git/config,\n> or picking a suitble format for yourself and setting it in\n> ~/.gitconfig, and not having to think about it afterwards may be a\n> better use of our users' time.\n\nAgreed.  Thanks for the suggestions.\n\n------------8<-------------\nSubject: [PATCH v2] format-patch: support format.mboxrd with --stdout\n\nmboxrd is a more robust output format when used with --stdout\nand needs more exposure.  Introducing this config knob lets\nusers choose the more robust format for all their --stdout\nuses.\n\nRelying on --pretty=mboxrd and including all of pretty-formats.txt\nin the `git format-patch' documentation would likely be\nconfusing to users.  Furthermore, this setting is useful across\nmultiple invocations.  So introduce `format.mboxrd' as a boolean\nconfiguration knob that changes the default --pretty=email format\nto --pretty=mboxrd when (and only when) --stdout is in use.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\nInterdiff:\n  diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt\n  index c7303d8d9f..3bd78269e2 100644\n  --- a/Documentation/config/format.txt\n  +++ b/Documentation/config/format.txt\n  @@ -139,3 +139,7 @@ For example,\n   ------------\n   +\n   will only show notes from `refs/notes/bar`.\n  +\n  +format.mboxrd::\n  +\tA boolean value which enables the robust \"mboxrd\" format when\n  +\t`--stdout` is in use to escape \"^>+From \" lines.\n  diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n  index b080d3c61e..dfcc7da4c2 100644\n  --- a/Documentation/git-format-patch.txt\n  +++ b/Documentation/git-format-patch.txt\n  @@ -9,7 +9,7 @@ git-format-patch - Prepare patches for e-mail submission\n   SYNOPSIS\n   --------\n   [verse]\n  -'git format-patch' [-k] [(-o|--output-directory) <dir> | --stdout] [--mboxrd]\n  +'git format-patch' [-k] [(-o|--output-directory) <dir> | --stdout]\n   \t\t   [--no-thread | --thread[=<style>]]\n   \t\t   [(--attach|--inline)[=<boundary>] | --no-attach]\n   \t\t   [-s | --signoff]\n  @@ -145,10 +145,6 @@ include::diff-options.txt[]\n   \tPrint all commits to the standard output in mbox format,\n   \tinstead of creating a file for each one.\n   \n  ---mboxrd::\n  -\tUse the robust \"mboxrd\" format with `--stdout` to escape\n  -\t\"^>+From \" lines.\n  -\n   --attach[=<boundary>]::\n   \tCreate multipart/mixed attachment, the first part of\n   \twhich is the commit message and the patch itself in the\n  diff --git a/builtin/log.c b/builtin/log.c\n  index 387560cc89..057e299c24 100644\n  --- a/builtin/log.c\n  +++ b/builtin/log.c\n  @@ -52,6 +52,7 @@ static int decoration_style;\n   static int decoration_given;\n   static int use_mailmap_config = 1;\n   static unsigned int force_in_body_from;\n  +static int stdout_mboxrd;\n   static const char *fmt_patch_subject_prefix = \"PATCH\";\n   static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;\n   static const char *fmt_pretty;\n  @@ -1077,6 +1078,10 @@ static int git_format_config(const char *var, const char *value, void *cb)\n   \t\tcover_from_description_mode = parse_cover_from_description(value);\n   \t\treturn 0;\n   \t}\n  +\tif (!strcmp(var, \"format.mboxrd\")) {\n  +\t\tstdout_mboxrd = git_config_bool(var, value);\n  +\t\treturn 0;\n  +\t}\n   \n   \treturn git_log_config(var, value, cb);\n   }\n  @@ -1871,7 +1876,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n   \tstruct strbuf rdiff2 = STRBUF_INIT;\n   \tstruct strbuf rdiff_title = STRBUF_INIT;\n   \tint creation_factor = -1;\n  -\tint mboxrd = 0;\n   \n   \tconst struct option builtin_format_patch_options[] = {\n   \t\tOPT_CALLBACK_F('n', \"numbered\", &numbered, NULL,\n  @@ -1883,8 +1887,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n   \t\tOPT_BOOL('s', \"signoff\", &do_signoff, N_(\"add a Signed-off-by trailer\")),\n   \t\tOPT_BOOL(0, \"stdout\", &use_stdout,\n   \t\t\t    N_(\"print patches to standard out\")),\n  -\t\tOPT_BOOL(0, \"mboxrd\", &mboxrd,\n  -\t\t\t    N_(\"use the robust mboxrd format with --stdout\")),\n   \t\tOPT_BOOL(0, \"cover-letter\", &cover_letter,\n   \t\t\t    N_(\"generate a cover letter\")),\n   \t\tOPT_BOOL(0, \"numbered-files\", &just_numbers,\n  @@ -2108,8 +2110,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n   \t\t\t\t  rev.diffopt.close_file, \"--output\",\n   \t\t\t\t  !!output_directory, \"--output-directory\");\n   \n  -\t/* should we warn on --mboxrd w/o --stdout? */\n  -\tif (mboxrd)\n  +\tif (use_stdout && stdout_mboxrd)\n   \t\trev.commit_format = CMIT_FMT_MBOXRD;\n   \n   \tif (use_stdout) {\n  diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n  index 73cbbd0ff8..dc95c34cc8 100644\n  --- a/contrib/completion/git-completion.bash\n  +++ b/contrib/completion/git-completion.bash\n  @@ -1835,7 +1835,7 @@ _git_fetch ()\n   \n   __git_format_patch_extra_options=\"\n   \t--full-index --not --all --no-prefix --src-prefix=\n  -\t--dst-prefix= --notes --mboxrd\n  +\t--dst-prefix= --notes\n   \"\n   \n   _git_format_patch ()\n  diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n  index 69ed8b1ffa..012f155e10 100755\n  --- a/t/t4014-format-patch.sh\n  +++ b/t/t4014-format-patch.sh\n  @@ -2281,7 +2281,7 @@ test_expect_success 'format-patch --attach cover-letter only is non-multipart' '\n   \ttest_line_count = 1 output\n   '\n   \n  -test_expect_success 'format-patch --mboxrd' '\n  +test_expect_success '-c format.mboxrd format-patch' '\n   \tsp=\" \" &&\n   \tcat >msg <<-INPUT_END &&\n   \tmboxrd should escape the body\n  @@ -2316,7 +2316,7 @@ test_expect_success 'format-patch --mboxrd' '\n   \tINPUT_END\n   \n   \tC=$(git commit-tree HEAD^^{tree} -p HEAD <msg) &&\n  -\tgit format-patch --mboxrd --stdout -1 $C~1..$C >patch &&\n  +\tgit -c format.mboxrd format-patch --stdout -1 $C~1..$C >patch &&\n   \tgit format-patch --pretty=mboxrd --stdout -1 $C~1..$C >compat &&\n   \ttest_cmp patch compat &&\n   \tgit grep -h --no-index -A11 \\\n  diff --git a/t/t4150-am.sh b/t/t4150-am.sh\n  index 9a128c16a6..7646e856d5 100755\n  --- a/t/t4150-am.sh\n  +++ b/t/t4150-am.sh\n  @@ -1033,7 +1033,7 @@ test_expect_success 'am --patch-format=mboxrd handles mboxrd' '\n   \t>From extra escape for reversibility\n   \tINPUT_END\n   \tgit commit -F msg &&\n  -\tgit format-patch --mboxrd --stdout -1 >mboxrd1 &&\n  +\tgit -c format.mboxrd format-patch --stdout -1 >mboxrd1 &&\n   \tgrep \"^>From could trip up a loose mbox parser\" mboxrd1 &&\n   \tgit checkout -f first &&\n   \tgit am --patch-format=mboxrd mboxrd1 &&\n\n Documentation/config/format.txt | 4 ++++\n builtin/log.c                   | 8 ++++++++\n t/t4014-format-patch.sh         | 6 ++++--\n t/t4150-am.sh                   | 2 +-\n 4 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/format.txt b/Documentation/config/format.txt\nindex c7303d8d9f..3bd78269e2 100644\n--- a/Documentation/config/format.txt\n+++ b/Documentation/config/format.txt\n@@ -139,3 +139,7 @@ For example,\n ------------\n +\n will only show notes from `refs/notes/bar`.\n+\n+format.mboxrd::\n+\tA boolean value which enables the robust \"mboxrd\" format when\n+\t`--stdout` is in use to escape \"^>+From \" lines.\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 89447a5083..057e299c24 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -52,6 +52,7 @@ static int decoration_style;\n static int decoration_given;\n static int use_mailmap_config = 1;\n static unsigned int force_in_body_from;\n+static int stdout_mboxrd;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;\n static const char *fmt_pretty;\n@@ -1077,6 +1078,10 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t\tcover_from_description_mode = parse_cover_from_description(value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"format.mboxrd\")) {\n+\t\tstdout_mboxrd = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \n \treturn git_log_config(var, value, cb);\n }\n@@ -2105,6 +2110,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t\t  rev.diffopt.close_file, \"--output\",\n \t\t\t\t  !!output_directory, \"--output-directory\");\n \n+\tif (use_stdout && stdout_mboxrd)\n+\t\trev.commit_format = CMIT_FMT_MBOXRD;\n+\n \tif (use_stdout) {\n \t\tsetup_pager();\n \t} else if (!rev.diffopt.close_file) {\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex de1da4673d..012f155e10 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2281,7 +2281,7 @@ test_expect_success 'format-patch --attach cover-letter only is non-multipart' '\n \ttest_line_count = 1 output\n '\n \n-test_expect_success 'format-patch --pretty=mboxrd' '\n+test_expect_success '-c format.mboxrd format-patch' '\n \tsp=\" \" &&\n \tcat >msg <<-INPUT_END &&\n \tmboxrd should escape the body\n@@ -2316,7 +2316,9 @@ test_expect_success 'format-patch --pretty=mboxrd' '\n \tINPUT_END\n \n \tC=$(git commit-tree HEAD^^{tree} -p HEAD <msg) &&\n-\tgit format-patch --pretty=mboxrd --stdout -1 $C~1..$C >patch &&\n+\tgit -c format.mboxrd format-patch --stdout -1 $C~1..$C >patch &&\n+\tgit format-patch --pretty=mboxrd --stdout -1 $C~1..$C >compat &&\n+\ttest_cmp patch compat &&\n \tgit grep -h --no-index -A11 \\\n \t\t\"^>From could trip up a loose mbox parser\" patch >actual &&\n \ttest_cmp expect actual\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex cdad4b6880..7646e856d5 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -1033,7 +1033,7 @@ test_expect_success 'am --patch-format=mboxrd handles mboxrd' '\n \t>From extra escape for reversibility\n \tINPUT_END\n \tgit commit -F msg &&\n-\tgit format-patch --pretty=mboxrd --stdout -1 >mboxrd1 &&\n+\tgit -c format.mboxrd format-patch --stdout -1 >mboxrd1 &&\n \tgrep \"^>From could trip up a loose mbox parser\" mboxrd1 &&\n \tgit checkout -f first &&\n \tgit am --patch-format=mboxrd mboxrd1 &&\n"},{"id":"469485","messageId":"xmqqedsqrjgg.fsf@gitster.g","threadId":"58798","inReplyTo":"20221222201619.M435985@dcvr","subject":"Re: [PATCH v2] format-patch: support format.mboxrd with --stdout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-23T01:34:55Z","receivedAt":"2022-12-23T01:35:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> Subject: [PATCH v2] format-patch: support format.mboxrd with --stdout\n>\n> mboxrd is a more robust output format when used with --stdout\n> and needs more exposure.  Introducing this config knob lets\n> users choose the more robust format for all their --stdout\n> uses.\n>\n> Relying on --pretty=mboxrd and including all of pretty-formats.txt\n> in the `git format-patch' documentation would likely be\n> confusing to users.  Furthermore, this setting is useful across\n> multiple invocations.  So introduce `format.mboxrd' as a boolean\n> configuration knob that changes the default --pretty=email format\n> to --pretty=mboxrd when (and only when) --stdout is in use.\n\nSounds sensible.\n"}]}