| Message ID | 20221114094114.18986-1-e@80x24.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | format-patch: add --mboxrd alias for --pretty=mboxrd | expand |
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@80x24.org> > --- > 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.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > 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@80x24.org> > > --- > > 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... As I noted in the commit message, I think discussing --pretty=* in the context of format-patch would be confusing for users. > > --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? Again, I prefer to mention --pretty= as little as possible when it comes to format-patch > > + if (mboxrd) > > + rev.commit_format = CMIT_FMT_MBOXRD; > > + It could be something like this: if (rev.commit_format == CMIT_FMT_MBOXRD && !use_stdout) warning("mboxrd without --stdout makes no sense\n"); But I'm on the fence about the warning. > > 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? These two lines were added to t/t4014-format-patch.sh above to test --pretty=mboxrd: + git format-patch --pretty=mboxrd --stdout -1 $C~1..$C >compat && + test_cmp patch compat && > 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. Again, I think discussing --pretty= is confusing to users if they might end up using raw/full/fuller/etc (especially if they're relying on tab completion). I that was the reason --pretty= was never documented in the format-patch manpage (which I had nothing to do with). Which section of the range-diff man page are you referring to?
Eric Wong <e@80x24.org> writes: > As I noted in the commit message, I think discussing --pretty=* > in the context of format-patch would be confusing for users. Sensible. >> > + if (mboxrd) >> > + rev.commit_format = CMIT_FMT_MBOXRD; >> > + > > It could be something like this: > > if (rev.commit_format == CMIT_FMT_MBOXRD && !use_stdout) > warning("mboxrd without --stdout makes no sense\n"); > > But I'm on the fence about the warning. Does it really hurt when generating individual files, or does it naturally degenerate to the same as the plain old mbox, or something? If it does not hurt, then let's not clutter the output with a message that may make the user worried unnecessarily. Having said all that, if --pretty=mboxrd is usable, do we really need the --mboxrd short-hand? If we plan to eventually switch the default output format to mboxrd (which I am assuming your longer term vision), wouldn't it be the traditional format that may need a short-hand when something goes wrong? This change does not seem to be something we cannot live without, and as a step in the direction to move all of us to mboxrd, this feels somewhat a roundabout step. I wonder if it would be more direct to - declare that we will eventually switch to use mboxrd by default; - give a configuration knob to retain the current email as default; - do the usual deprecation dance. After all, between email and mboxrd, the e-mailed patch format is not something we choose per-invocation basis, is it? Picking a suitable format per project and setting it in .git/config, or picking a suitble format for yourself and setting it in ~/.gitconfig, and not having to think about it afterwards may be a better use of our users' time. Thanks.
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index dfcc7da4c211..b080d3c61e31 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -9,7 +9,7 @@ git-format-patch - Prepare patches for e-mail submission SYNOPSIS -------- [verse] -'git format-patch' [-k] [(-o|--output-directory) <dir> | --stdout] +'git format-patch' [-k] [(-o|--output-directory) <dir> | --stdout] [--mboxrd] [--no-thread | --thread[=<style>]] [(--attach|--inline)[=<boundary>] | --no-attach] [-s | --signoff] @@ -145,6 +145,10 @@ include::diff-options.txt[] Print all commits to the standard output in mbox format, instead of creating a file for each one. +--mboxrd:: + Use the robust "mboxrd" format with `--stdout` to escape + "^>+From " lines. + --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? */ + 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 &&
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@80x24.org> --- 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... Documentation/git-format-patch.txt | 6 +++++- builtin/log.c | 7 +++++++ contrib/completion/git-completion.bash | 2 +- t/t4014-format-patch.sh | 6 ++++-- t/t4150-am.sh | 2 +- 5 files changed, 18 insertions(+), 5 deletions(-)