* [PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd
@ 2022-11-14 9:41 Eric Wong
2022-11-14 16:53 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 6+ messages in thread
From: Eric Wong @ 2022-11-14 9:41 UTC (permalink / raw)
To: git
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(-)
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 &&
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd
2022-11-14 9:41 [PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd Eric Wong
@ 2022-11-14 16:53 ` Ævar Arnfjörð Bjarmason
2022-11-14 20:59 ` Eric Wong
0 siblings, 1 reply; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-14 16:53 UTC (permalink / raw)
To: Eric Wong; +Cc: git
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd
2022-11-14 16:53 ` Ævar Arnfjörð Bjarmason
@ 2022-11-14 20:59 ` Eric Wong
2022-12-18 4:24 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Eric Wong @ 2022-11-14 20:59 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git
Æ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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd
2022-11-14 20:59 ` Eric Wong
@ 2022-12-18 4:24 ` Junio C Hamano
2022-12-22 20:16 ` [PATCH v2] format-patch: support format.mboxrd with --stdout Eric Wong
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2022-12-18 4:24 UTC (permalink / raw)
To: Eric Wong; +Cc: Ævar Arnfjörð Bjarmason, git
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] format-patch: support format.mboxrd with --stdout
2022-12-18 4:24 ` Junio C Hamano
@ 2022-12-22 20:16 ` Eric Wong
2022-12-23 1:34 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Eric Wong @ 2022-12-22 20:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git
Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> >> > + 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.
OK, so the warning won't be necessary if using format.mboxrd
via config.
> 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.
mboxrd is bad for individual files since it can leave '>From'
escaping if applied w/o `--patch-format mboxrd". My revised
patch only enables mboxrd if --stdout is in use.
> 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?
Actually, I never considered making mboxrd a config file knob.
But yes, it seems like a good idea.
I hadn't considered making it the default, actually. The current
`--pretty=email --stdout' is probably "good enough" for many
users since the "From "-splitting in mailsplit is strict and
legitimate commit messages are unlikely to have splittable "From "
lines.
> This change does not seem to be something we cannot live without,
That double negative confused me :x
"This change seems to be something we can live without"
Yes, I agree.
> 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.
I don't know if changing the default+deprecation is necessary,
but thanks for suggesting a config knob.
> After all, between email and mboxrd, the e-mailed patch format is
> not something we choose per-invocation basis, is it?
Right.
> 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.
Agreed. Thanks for the suggestions.
------------8<-------------
Subject: [PATCH v2] format-patch: support format.mboxrd with --stdout
mboxrd is a more robust output format when used with --stdout
and needs more exposure. Introducing this config knob lets
users choose the more robust format for all their --stdout
uses.
Relying on --pretty=mboxrd and including all of pretty-formats.txt
in the `git format-patch' documentation would likely be
confusing to users. Furthermore, this setting is useful across
multiple invocations. So introduce `format.mboxrd' as a boolean
configuration knob that changes the default --pretty=email format
to --pretty=mboxrd when (and only when) --stdout is in use.
Signed-off-by: Eric Wong <e@80x24.org>
---
Interdiff:
diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index c7303d8d9f..3bd78269e2 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -139,3 +139,7 @@ For example,
------------
+
will only show notes from `refs/notes/bar`.
+
+format.mboxrd::
+ A boolean value which enables the robust "mboxrd" format when
+ `--stdout` is in use to escape "^>+From " lines.
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index b080d3c61e..dfcc7da4c2 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] [--mboxrd]
+'git format-patch' [-k] [(-o|--output-directory) <dir> | --stdout]
[--no-thread | --thread[=<style>]]
[(--attach|--inline)[=<boundary>] | --no-attach]
[-s | --signoff]
@@ -145,10 +145,6 @@ 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 387560cc89..057e299c24 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -52,6 +52,7 @@ static int decoration_style;
static int decoration_given;
static int use_mailmap_config = 1;
static unsigned int force_in_body_from;
+static int stdout_mboxrd;
static const char *fmt_patch_subject_prefix = "PATCH";
static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;
static const char *fmt_pretty;
@@ -1077,6 +1078,10 @@ static int git_format_config(const char *var, const char *value, void *cb)
cover_from_description_mode = parse_cover_from_description(value);
return 0;
}
+ if (!strcmp(var, "format.mboxrd")) {
+ stdout_mboxrd = git_config_bool(var, value);
+ return 0;
+ }
return git_log_config(var, value, cb);
}
@@ -1871,7 +1876,6 @@ 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,8 +1887,6 @@ 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,
@@ -2108,8 +2110,7 @@ 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)
+ if (use_stdout && stdout_mboxrd)
rev.commit_format = CMIT_FMT_MBOXRD;
if (use_stdout) {
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 73cbbd0ff8..dc95c34cc8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1835,7 +1835,7 @@ _git_fetch ()
__git_format_patch_extra_options="
--full-index --not --all --no-prefix --src-prefix=
- --dst-prefix= --notes --mboxrd
+ --dst-prefix= --notes
"
_git_format_patch ()
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 69ed8b1ffa..012f155e10 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 --mboxrd' '
+test_expect_success '-c format.mboxrd format-patch' '
sp=" " &&
cat >msg <<-INPUT_END &&
mboxrd should escape the body
@@ -2316,7 +2316,7 @@ test_expect_success 'format-patch --mboxrd' '
INPUT_END
C=$(git commit-tree HEAD^^{tree} -p HEAD <msg) &&
- git format-patch --mboxrd --stdout -1 $C~1..$C >patch &&
+ git -c format.mboxrd format-patch --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 \
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 9a128c16a6..7646e856d5 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 --mboxrd --stdout -1 >mboxrd1 &&
+ git -c format.mboxrd format-patch --stdout -1 >mboxrd1 &&
grep "^>From could trip up a loose mbox parser" mboxrd1 &&
git checkout -f first &&
git am --patch-format=mboxrd mboxrd1 &&
Documentation/config/format.txt | 4 ++++
builtin/log.c | 8 ++++++++
t/t4014-format-patch.sh | 6 ++++--
t/t4150-am.sh | 2 +-
4 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index c7303d8d9f..3bd78269e2 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -139,3 +139,7 @@ For example,
------------
+
will only show notes from `refs/notes/bar`.
+
+format.mboxrd::
+ A boolean value which enables the robust "mboxrd" format when
+ `--stdout` is in use to escape "^>+From " lines.
diff --git a/builtin/log.c b/builtin/log.c
index 89447a5083..057e299c24 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -52,6 +52,7 @@ static int decoration_style;
static int decoration_given;
static int use_mailmap_config = 1;
static unsigned int force_in_body_from;
+static int stdout_mboxrd;
static const char *fmt_patch_subject_prefix = "PATCH";
static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;
static const char *fmt_pretty;
@@ -1077,6 +1078,10 @@ static int git_format_config(const char *var, const char *value, void *cb)
cover_from_description_mode = parse_cover_from_description(value);
return 0;
}
+ if (!strcmp(var, "format.mboxrd")) {
+ stdout_mboxrd = git_config_bool(var, value);
+ return 0;
+ }
return git_log_config(var, value, cb);
}
@@ -2105,6 +2110,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
rev.diffopt.close_file, "--output",
!!output_directory, "--output-directory");
+ if (use_stdout && stdout_mboxrd)
+ rev.commit_format = CMIT_FMT_MBOXRD;
+
if (use_stdout) {
setup_pager();
} else if (!rev.diffopt.close_file) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index de1da4673d..012f155e10 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 '-c format.mboxrd format-patch' '
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 -c format.mboxrd format-patch --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 cdad4b6880..7646e856d5 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 -c format.mboxrd format-patch --stdout -1 >mboxrd1 &&
grep "^>From could trip up a loose mbox parser" mboxrd1 &&
git checkout -f first &&
git am --patch-format=mboxrd mboxrd1 &&
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] format-patch: support format.mboxrd with --stdout
2022-12-22 20:16 ` [PATCH v2] format-patch: support format.mboxrd with --stdout Eric Wong
@ 2022-12-23 1:34 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2022-12-23 1:34 UTC (permalink / raw)
To: Eric Wong; +Cc: Ævar Arnfjörð Bjarmason, git
Eric Wong <e@80x24.org> writes:
> Subject: [PATCH v2] format-patch: support format.mboxrd with --stdout
>
> mboxrd is a more robust output format when used with --stdout
> and needs more exposure. Introducing this config knob lets
> users choose the more robust format for all their --stdout
> uses.
>
> Relying on --pretty=mboxrd and including all of pretty-formats.txt
> in the `git format-patch' documentation would likely be
> confusing to users. Furthermore, this setting is useful across
> multiple invocations. So introduce `format.mboxrd' as a boolean
> configuration knob that changes the default --pretty=email format
> to --pretty=mboxrd when (and only when) --stdout is in use.
Sounds sensible.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-23 1:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-14 9:41 [PATCH] format-patch: add --mboxrd alias for --pretty=mboxrd Eric Wong
2022-11-14 16:53 ` Ævar Arnfjörð Bjarmason
2022-11-14 20:59 ` Eric Wong
2022-12-18 4:24 ` Junio C Hamano
2022-12-22 20:16 ` [PATCH v2] format-patch: support format.mboxrd with --stdout Eric Wong
2022-12-23 1:34 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).