On Mon, Nov 14 2022, Eric Wong wrote: > mboxrd is a superior output format when used with --stdout and > needs more exposure. Including pretty-formats.txt would be > excessive, since documenting --pretty= for `git format-patch' > would likely be confusing to users. > > Instead of documenting --pretty, add an --mboxrd alias to save > keystrokes and improve documentation. > > Signed-off-by: Eric Wong <e@xxxxxxxxx> > --- > Also, --mboxrd without --stdout makes no sense to me, > so I'm considering warning on it... > > Side note: some of the OPT_* switches might be misplaced > under the "Messaging" OPT_GROUP... Makes sense, but.... > +--mboxrd:: > + Use the robust "mboxrd" format with `--stdout` to escape > + "^>+From " lines. > + ...Rather than repeat ourselves, shouldn't we (or in addition to this) link to a manpage that discusses the --pretty=* formats. I was going to say that you can also use the "ifdef" asciidoc syntax, but for one paragraph that's probably overkill... > --attach[=<boundary>]:: > Create multipart/mixed attachment, the first part of > which is the commit message and the patch itself in the > diff --git a/builtin/log.c b/builtin/log.c > index 5eafcf26b49b..13f5deb7a5c0 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1872,6 +1872,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > struct strbuf rdiff2 = STRBUF_INIT; > struct strbuf rdiff_title = STRBUF_INIT; > int creation_factor = -1; > + int mboxrd = 0; > > const struct option builtin_format_patch_options[] = { > OPT_CALLBACK_F('n', "numbered", &numbered, NULL, > @@ -1883,6 +1884,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > OPT_BOOL('s', "signoff", &do_signoff, N_("add a Signed-off-by trailer")), > OPT_BOOL(0, "stdout", &use_stdout, > N_("print patches to standard out")), > + OPT_BOOL(0, "mboxrd", &mboxrd, > + N_("use the robust mboxrd format with --stdout")), > OPT_BOOL(0, "cover-letter", &cover_letter, > N_("generate a cover letter")), > OPT_BOOL(0, "numbered-files", &just_numbers, > @@ -2106,6 +2109,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > rev.diffopt.close_file, "--output", > !!output_directory, "--output-directory"); > > + /* should we warn on --mboxrd w/o --stdout? */ Does that go for --pretty=mboxrd too? > + if (mboxrd) > + rev.commit_format = CMIT_FMT_MBOXRD; > + > if (use_stdout) { > setup_pager(); > } else if (!rev.diffopt.close_file) { > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index ba5c395d2d80..f6e2fbdcfa68 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -1815,7 +1815,7 @@ _git_fetch () > > __git_format_patch_extra_options=" > --full-index --not --all --no-prefix --src-prefix= > - --dst-prefix= --notes > + --dst-prefix= --notes --mboxrd > " > > _git_format_patch () > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh > index de1da4673da9..69ed8b1ffaa1 100755 > --- a/t/t4014-format-patch.sh > +++ b/t/t4014-format-patch.sh > @@ -2281,7 +2281,7 @@ test_expect_success 'format-patch --attach cover-letter only is non-multipart' ' > test_line_count = 1 output > ' > > -test_expect_success 'format-patch --pretty=mboxrd' ' > +test_expect_success 'format-patch --mboxrd' ' > sp=" " && > cat >msg <<-INPUT_END && > mboxrd should escape the body > @@ -2316,7 +2316,9 @@ test_expect_success 'format-patch --pretty=mboxrd' ' > INPUT_END > > C=$(git commit-tree HEAD^^{tree} -p HEAD <msg) && > - git format-patch --pretty=mboxrd --stdout -1 $C~1..$C >patch && > + git format-patch --mboxrd --stdout -1 $C~1..$C >patch && > + git format-patch --pretty=mboxrd --stdout -1 $C~1..$C >compat && > + test_cmp patch compat && > git grep -h --no-index -A11 \ > "^>From could trip up a loose mbox parser" patch >actual && > test_cmp expect actual > diff --git a/t/t4150-am.sh b/t/t4150-am.sh > index cdad4b688078..9a128c16a6ee 100755 > --- a/t/t4150-am.sh > +++ b/t/t4150-am.sh > @@ -1033,7 +1033,7 @@ test_expect_success 'am --patch-format=mboxrd handles mboxrd' ' > >From extra escape for reversibility > INPUT_END > git commit -F msg && > - git format-patch --pretty=mboxrd --stdout -1 >mboxrd1 && > + git format-patch --mboxrd --stdout -1 >mboxrd1 && > grep "^>From could trip up a loose mbox parser" mboxrd1 && > git checkout -f first && > git am --patch-format=mboxrd mboxrd1 && Doesn't this leave us without coverage for the --pretty=mboxrd variant? I must admit I'm not a big outright fan of this, the "log-like" is confusing enough, with those accepting some options, ignoring others etc, now we're adding command-specific aliases too... Why not just document that we accept --pretty=<some subset>? E.g. see "range-diff"'s docs for an existing case where we discuss accepting a limited subset.