| Message ID | xmqqzftqnuxq.fsf@gitster.g (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | format-patch: allow --rfc to optionally take a value, like --rfc=WIP | expand |
Hello Junio, Please see my comments below. On 2024-04-19 00:54, Junio C Hamano wrote: > With the "--rfc" option, we can tweak the "[PATCH]" (or whatever > string specified with the "--subject-prefix" option, instead of > "PATCH") that we prefix the title of the commit with into "[RFC > PATCH]", but some projects may want "[rfc PATCH]". Adding a new > option, e.g., "--rfc-lowercase", to support such need every time > somebody wants to use different strings would lead to insanity of > accumulating unbounded number of such options. > > Allow an optional value specified for the option, so that users can > use "--rfc=rfc" (think of "--rfc" without value as a short-hand for > "--rfc=RFC"). > > This can of course be (ab)used to make the prefix "[WIP PATCH]" by > passing "--rfc=WIP". Aren't we also going to allow patch subject prefixes such as "[PATCH RESEND]" to be produced this way? Please see my earlier response [1] for a possible solution. [1] https://lore.kernel.org/git/84dcb80be916f85cbb6a4b99aea0d76b@manjaro.org/ > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > Documentation/git-format-patch.txt | 15 ++++++++++----- > builtin/log.c | 22 ++++++++++++++++++---- > t/t4014-format-patch.sh | 9 +++++++++ > 3 files changed, 37 insertions(+), 9 deletions(-) > > diff --git a/Documentation/git-format-patch.txt > b/Documentation/git-format-patch.txt > index 728bb3821c..8d634f5b1b 100644 > --- a/Documentation/git-format-patch.txt > +++ b/Documentation/git-format-patch.txt > @@ -20,7 +20,7 @@ SYNOPSIS > [--in-reply-to=<message-id>] [--suffix=.<sfx>] > [--ignore-if-in-upstream] [--always] > [--cover-from-description=<mode>] > - [--rfc] [--subject-prefix=<subject-prefix>] > + [--rfc[=<rfc>]] [--subject-prefix=<subject-prefix>] > [(--reroll-count|-v) <n>] > [--to=<email>] [--cc=<email>] > [--[no-]cover-letter] [--quiet] > @@ -238,10 +238,15 @@ the patches (with a value of e.g. "PATCH > my-project"). > value of the `format.filenameMaxLength` configuration > variable, or 64 if unconfigured. > > ---rfc:: > - Prepends "RFC" to the subject prefix (producing "RFC PATCH" by > - default). RFC means "Request For Comments"; use this when sending > - an experimental patch for discussion rather than application. > +--rfc[=<rfc>]:: > + Prepends the string _<rfc>_ (defaults to "RFC") to > + the subject prefix. As the subject prefix defaults to > + "PATCH", you'll get "RFC PATCH" by default. > ++ > +RFC means "Request For Comments"; use this when sending > +an experimental patch for discussion rather than application. > +"--rfc=WIP" may also be a useful way to indicate that a patch > +is not complete yet. > > -v <n>:: > --reroll-count=<n>:: > diff --git a/builtin/log.c b/builtin/log.c > index c0a8bb95e9..2d6e0f3688 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1494,6 +1494,18 @@ static int subject_prefix_callback(const struct > option *opt, const char *arg, > return 0; > } > > +static int rfc_callback(const struct option *opt, const char *arg, > + int unset) > +{ > + struct strbuf *rfc; > + > + BUG_ON_OPT_NEG(unset); > + rfc = opt->value; > + strbuf_reset(rfc); > + strbuf_addstr(rfc, arg ? arg : "RFC"); > + return 0; > +} "subject_prefix = 1;" should also be in this function, to prevent '--rfc="" -k' from being a valid combination of parameters. Checking "rfc.len" later in the code checks the length of the string passed for '--rfc="<string>"', which can be zero, instead of checking for the existence of "--rfc" as an option on the command-line, so the "subject_prefix" flag also needs to be set. > + > static int numbered_cmdline_opt = 0; > > static int numbered_callback(const struct option *opt, const char > *arg, > @@ -1907,8 +1919,8 @@ int cmd_format_patch(int argc, const char > **argv, const char *prefix) > struct strbuf rdiff2 = STRBUF_INIT; > struct strbuf rdiff_title = STRBUF_INIT; > struct strbuf sprefix = STRBUF_INIT; > + struct strbuf rfc = STRBUF_INIT; > int creation_factor = -1; > - int rfc = 0; > > const struct option builtin_format_patch_options[] = { > OPT_CALLBACK_F('n', "numbered", &numbered, NULL, > @@ -1932,7 +1944,9 @@ int cmd_format_patch(int argc, const char > **argv, const char *prefix) > N_("mark the series as Nth re-roll")), > OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max, > N_("max length of output filename")), > - OPT_BOOL(0, "rfc", &rfc, N_("use [RFC PATCH] instead of [PATCH]")), > + OPT_CALLBACK_F(0, "rfc", &rfc, N_("extra"), > + N_("add <extra> (default 'RFC') before 'PATCH'"), > + PARSE_OPT_NONEG|PARSE_OPT_OPTARG, rfc_callback), > OPT_STRING(0, "cover-from-description", &cover_from_description_arg, > N_("cover-from-description-mode"), > N_("generate parts of a cover letter based on a branch's > description")), > @@ -2050,8 +2064,8 @@ int cmd_format_patch(int argc, const char > **argv, const char *prefix) > if (cover_from_description_arg) > cover_from_description_mode = > parse_cover_from_description(cover_from_description_arg); > > - if (rfc) > - strbuf_insertstr(&sprefix, 0, "RFC "); > + if (rfc.len) > + strbuf_insertf(&sprefix, 0, "%s ", rfc.buf); > > if (reroll_count) { > strbuf_addf(&sprefix, " v%s", reroll_count); > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh > index e37a1411ee..905858da35 100755 > --- a/t/t4014-format-patch.sh > +++ b/t/t4014-format-patch.sh > @@ -1377,6 +1377,15 @@ test_expect_success '--rfc' ' > test_cmp expect actual > ' > > +test_expect_success '--rfc=WIP' ' > + cat >expect <<-\EOF && > + Subject: [WIP PATCH 1/1] header with . in it > + EOF > + git format-patch -n -1 --stdout --rfc=WIP >patch && > + grep "^Subject:" patch >actual && > + test_cmp expect actual > +' > + > test_expect_success '--rfc does not overwrite prefix' ' > cat >expect <<-\EOF && > Subject: [RFC PATCH foobar 1/1] header with . in it
Hi Junio On 18/04/2024 23:54, Junio C Hamano wrote: > With the "--rfc" option, we can tweak the "[PATCH]" (or whatever > string specified with the "--subject-prefix" option, instead of > "PATCH") that we prefix the title of the commit with into "[RFC > PATCH]", but some projects may want "[rfc PATCH]". Adding a new > option, e.g., "--rfc-lowercase", to support such need every time > somebody wants to use different strings would lead to insanity of > accumulating unbounded number of such options. > > Allow an optional value specified for the option, so that users can > use "--rfc=rfc" (think of "--rfc" without value as a short-hand for > "--rfc=RFC"). > > This can of course be (ab)used to make the prefix "[WIP PATCH]" by > passing "--rfc=WIP". I think being able to customize the subject prefix in this way is a good idea and makes it easy to say [RESEND PATCH program] with --rfc=RESEND when format.subjectPrefix is set. We could add a separate option to as "--rfc" already exists I think it is reasonable to extend it. (I'm also not a good name for such an option would be "--subject-prefix-prefix" kind of describes what it does but is far from ideal) > - [--rfc] [--subject-prefix=<subject-prefix>] > + [--rfc[=<rfc>]] [--subject-prefix=<subject-prefix>] Nit: in the documentation we use "<rfc>" for the placeholder but in the code we use "<extra>" > ---rfc:: > - Prepends "RFC" to the subject prefix (producing "RFC PATCH" by > - default). RFC means "Request For Comments"; use this when sending > - an experimental patch for discussion rather than application. > +--rfc[=<rfc>]:: > + Prepends the string _<rfc>_ (defaults to "RFC") to > + the subject prefix. As the subject prefix defaults to > + "PATCH", you'll get "RFC PATCH" by default. > ++ > +RFC means "Request For Comments"; use this when sending > +an experimental patch for discussion rather than application. > +"--rfc=WIP" may also be a useful way to indicate that a patch > +is not complete yet. This reworded description is good > const struct option builtin_format_patch_options[] = { > OPT_CALLBACK_F('n', "numbered", &numbered, NULL, > @@ -1932,7 +1944,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > N_("mark the series as Nth re-roll")), > OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max, > N_("max length of output filename")), > - OPT_BOOL(0, "rfc", &rfc, N_("use [RFC PATCH] instead of [PATCH]")), > + OPT_CALLBACK_F(0, "rfc", &rfc, N_("extra"), > + N_("add <extra> (default 'RFC') before 'PATCH'"), > + PARSE_OPT_NONEG|PARSE_OPT_OPTARG, rfc_callback), This is a change in behavior as it looks like we previously accepted "--no-rfc" is that deliberate? Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: >> - [--rfc] [--subject-prefix=<subject-prefix>] >> + [--rfc[=<rfc>]] [--subject-prefix=<subject-prefix>] > > Nit: in the documentation we use "<rfc>" for the placeholder but in > the code we use "<extra>" You're right. Will fix. >> @@ -1932,7 +1944,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) >> N_("mark the series as Nth re-roll")), >> OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max, >> N_("max length of output filename")), >> - OPT_BOOL(0, "rfc", &rfc, N_("use [RFC PATCH] instead of [PATCH]")), >> + OPT_CALLBACK_F(0, "rfc", &rfc, N_("extra"), >> + N_("add <extra> (default 'RFC') before 'PATCH'"), >> + PARSE_OPT_NONEG|PARSE_OPT_OPTARG, rfc_callback), > > This is a change in behavior as it looks like we previously accepted > "--no-rfc" is that deliberate? I just matched the subject-prefix without thinking. Will fix. Here is what I plan to squash in, but we are about to enter the pre-release stabilization period, so the progress on this new feature will have to slow down. Thanks. builtin/log.c | 10 +++++----- t/t4014-format-patch.sh | 16 ++++++++++++---- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git i/builtin/log.c w/builtin/log.c index 2d6e0f3688..0e9c84e51d 100644 --- i/builtin/log.c +++ w/builtin/log.c @@ -1499,10 +1499,10 @@ static int rfc_callback(const struct option *opt, const char *arg, { struct strbuf *rfc; - BUG_ON_OPT_NEG(unset); rfc = opt->value; strbuf_reset(rfc); - strbuf_addstr(rfc, arg ? arg : "RFC"); + if (!unset) + strbuf_addstr(rfc, arg ? arg : "RFC"); return 0; } @@ -1944,9 +1944,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) N_("mark the series as Nth re-roll")), OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max, N_("max length of output filename")), - OPT_CALLBACK_F(0, "rfc", &rfc, N_("extra"), - N_("add <extra> (default 'RFC') before 'PATCH'"), - PARSE_OPT_NONEG|PARSE_OPT_OPTARG, rfc_callback), + OPT_CALLBACK_F(0, "rfc", &rfc, N_("rfc"), + N_("add <rfc> (default 'RFC') before 'PATCH'"), + PARSE_OPT_OPTARG, rfc_callback), OPT_STRING(0, "cover-from-description", &cover_from_description_arg, N_("cover-from-description-mode"), N_("generate parts of a cover letter based on a branch's description")), diff --git i/t/t4014-format-patch.sh w/t/t4014-format-patch.sh index 905858da35..645c4189f9 100755 --- i/t/t4014-format-patch.sh +++ w/t/t4014-format-patch.sh @@ -1368,22 +1368,30 @@ test_expect_success 'empty subject prefix does not have extra space' ' test_cmp expect actual ' -test_expect_success '--rfc' ' +test_expect_success '--rfc and --no-rfc' ' cat >expect <<-\EOF && Subject: [RFC PATCH 1/1] header with . in it EOF git format-patch -n -1 --stdout --rfc >patch && grep "^Subject:" patch >actual && - test_cmp expect actual + test_cmp expect actual && + git format-patch -n -1 --stdout --rfc --no-rfc >patch && + sed -e "s/RFC //" expect >expect-raw && + grep "^Subject:" patch >actual && + test_cmp expect-raw actual ' -test_expect_success '--rfc=WIP' ' +test_expect_success '--rfc=WIP and --rfc=' ' cat >expect <<-\EOF && Subject: [WIP PATCH 1/1] header with . in it EOF git format-patch -n -1 --stdout --rfc=WIP >patch && grep "^Subject:" patch >actual && - test_cmp expect actual + test_cmp expect actual && + git format-patch -n -1 --stdout --rfc --rfc= >patch && + sed -e "s/WIP //" expect >expect-raw && + grep "^Subject:" patch >actual && + test_cmp expect-raw actual ' test_expect_success '--rfc does not overwrite prefix' '
On Thu, Apr 18, 2024 at 03:54:25PM -0700, Junio C Hamano wrote: > ---rfc:: > - Prepends "RFC" to the subject prefix (producing "RFC PATCH" by > - default). RFC means "Request For Comments"; use this when sending > - an experimental patch for discussion rather than application. > +--rfc[=<rfc>]:: > + Prepends the string _<rfc>_ (defaults to "RFC") to > + the subject prefix. As the subject prefix defaults to > + "PATCH", you'll get "RFC PATCH" by default. > ++ > +RFC means "Request For Comments"; use this when sending > +an experimental patch for discussion rather than application. > +"--rfc=WIP" may also be a useful way to indicate that a patch > +is not complete yet. It's probably worth spelling out WIP here, too, like: ...is not complete yet ("WIP" stands for "Work In Progress"). These are necessarily going to be project-specific phrases, and readers of the manpage may not be familiar with our conventions. -Peff
Jeff King <peff@peff.net> writes: > On Thu, Apr 18, 2024 at 03:54:25PM -0700, Junio C Hamano wrote: > >> ---rfc:: >> - Prepends "RFC" to the subject prefix (producing "RFC PATCH" by >> - default). RFC means "Request For Comments"; use this when sending >> - an experimental patch for discussion rather than application. >> +--rfc[=<rfc>]:: >> + Prepends the string _<rfc>_ (defaults to "RFC") to >> + the subject prefix. As the subject prefix defaults to >> + "PATCH", you'll get "RFC PATCH" by default. >> ++ >> +RFC means "Request For Comments"; use this when sending >> +an experimental patch for discussion rather than application. >> +"--rfc=WIP" may also be a useful way to indicate that a patch >> +is not complete yet. > > It's probably worth spelling out WIP here, too, like: > > ...is not complete yet ("WIP" stands for "Work In Progress"). > > These are necessarily going to be project-specific phrases, and readers > of the manpage may not be familiar with our conventions. Good point. Will squash in.
Hello Junio, On 2024-04-19 19:03, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: >>> @@ -1932,7 +1944,9 @@ int cmd_format_patch(int argc, const char >>> **argv, const char *prefix) >>> N_("mark the series as Nth re-roll")), >>> OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max, >>> N_("max length of output filename")), >>> - OPT_BOOL(0, "rfc", &rfc, N_("use [RFC PATCH] instead of >>> [PATCH]")), >>> + OPT_CALLBACK_F(0, "rfc", &rfc, N_("extra"), >>> + N_("add <extra> (default 'RFC') before 'PATCH'"), >>> + PARSE_OPT_NONEG|PARSE_OPT_OPTARG, rfc_callback), >> >> This is a change in behavior as it looks like we previously accepted >> "--no-rfc" is that deliberate? > > I just matched the subject-prefix without thinking. Will fix. > > Here is what I plan to squash in, but we are about to enter the > pre-release stabilization period, so the progress on this new > feature will have to slow down. Let me remind you about the need to also support "[PATCH RESEND]", for example, in this new feature. Please see my earlier response [1] for a possible solution. Even some instructions for submitting patches, in some projects, specify "[PATCH RESEND]" as the expected prefix, not "[RESEND PATCH]". Thus, suffixes for the prefix should be supported. [1] https://lore.kernel.org/git/84dcb80be916f85cbb6a4b99aea0d76b@manjaro.org/
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 728bb3821c..8d634f5b1b 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -20,7 +20,7 @@ SYNOPSIS [--in-reply-to=<message-id>] [--suffix=.<sfx>] [--ignore-if-in-upstream] [--always] [--cover-from-description=<mode>] - [--rfc] [--subject-prefix=<subject-prefix>] + [--rfc[=<rfc>]] [--subject-prefix=<subject-prefix>] [(--reroll-count|-v) <n>] [--to=<email>] [--cc=<email>] [--[no-]cover-letter] [--quiet] @@ -238,10 +238,15 @@ the patches (with a value of e.g. "PATCH my-project"). value of the `format.filenameMaxLength` configuration variable, or 64 if unconfigured. ---rfc:: - Prepends "RFC" to the subject prefix (producing "RFC PATCH" by - default). RFC means "Request For Comments"; use this when sending - an experimental patch for discussion rather than application. +--rfc[=<rfc>]:: + Prepends the string _<rfc>_ (defaults to "RFC") to + the subject prefix. As the subject prefix defaults to + "PATCH", you'll get "RFC PATCH" by default. ++ +RFC means "Request For Comments"; use this when sending +an experimental patch for discussion rather than application. +"--rfc=WIP" may also be a useful way to indicate that a patch +is not complete yet. -v <n>:: --reroll-count=<n>:: diff --git a/builtin/log.c b/builtin/log.c index c0a8bb95e9..2d6e0f3688 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1494,6 +1494,18 @@ static int subject_prefix_callback(const struct option *opt, const char *arg, return 0; } +static int rfc_callback(const struct option *opt, const char *arg, + int unset) +{ + struct strbuf *rfc; + + BUG_ON_OPT_NEG(unset); + rfc = opt->value; + strbuf_reset(rfc); + strbuf_addstr(rfc, arg ? arg : "RFC"); + return 0; +} + static int numbered_cmdline_opt = 0; static int numbered_callback(const struct option *opt, const char *arg, @@ -1907,8 +1919,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) struct strbuf rdiff2 = STRBUF_INIT; struct strbuf rdiff_title = STRBUF_INIT; struct strbuf sprefix = STRBUF_INIT; + struct strbuf rfc = STRBUF_INIT; int creation_factor = -1; - int rfc = 0; const struct option builtin_format_patch_options[] = { OPT_CALLBACK_F('n', "numbered", &numbered, NULL, @@ -1932,7 +1944,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) N_("mark the series as Nth re-roll")), OPT_INTEGER(0, "filename-max-length", &fmt_patch_name_max, N_("max length of output filename")), - OPT_BOOL(0, "rfc", &rfc, N_("use [RFC PATCH] instead of [PATCH]")), + OPT_CALLBACK_F(0, "rfc", &rfc, N_("extra"), + N_("add <extra> (default 'RFC') before 'PATCH'"), + PARSE_OPT_NONEG|PARSE_OPT_OPTARG, rfc_callback), OPT_STRING(0, "cover-from-description", &cover_from_description_arg, N_("cover-from-description-mode"), N_("generate parts of a cover letter based on a branch's description")), @@ -2050,8 +2064,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (cover_from_description_arg) cover_from_description_mode = parse_cover_from_description(cover_from_description_arg); - if (rfc) - strbuf_insertstr(&sprefix, 0, "RFC "); + if (rfc.len) + strbuf_insertf(&sprefix, 0, "%s ", rfc.buf); if (reroll_count) { strbuf_addf(&sprefix, " v%s", reroll_count); diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index e37a1411ee..905858da35 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -1377,6 +1377,15 @@ test_expect_success '--rfc' ' test_cmp expect actual ' +test_expect_success '--rfc=WIP' ' + cat >expect <<-\EOF && + Subject: [WIP PATCH 1/1] header with . in it + EOF + git format-patch -n -1 --stdout --rfc=WIP >patch && + grep "^Subject:" patch >actual && + test_cmp expect actual +' + test_expect_success '--rfc does not overwrite prefix' ' cat >expect <<-\EOF && Subject: [RFC PATCH foobar 1/1] header with . in it
With the "--rfc" option, we can tweak the "[PATCH]" (or whatever string specified with the "--subject-prefix" option, instead of "PATCH") that we prefix the title of the commit with into "[RFC PATCH]", but some projects may want "[rfc PATCH]". Adding a new option, e.g., "--rfc-lowercase", to support such need every time somebody wants to use different strings would lead to insanity of accumulating unbounded number of such options. Allow an optional value specified for the option, so that users can use "--rfc=rfc" (think of "--rfc" without value as a short-hand for "--rfc=RFC"). This can of course be (ab)used to make the prefix "[WIP PATCH]" by passing "--rfc=WIP". Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/git-format-patch.txt | 15 ++++++++++----- builtin/log.c | 22 ++++++++++++++++++---- t/t4014-format-patch.sh | 9 +++++++++ 3 files changed, 37 insertions(+), 9 deletions(-)