[PATCH] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[PATCH] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Junio C Hamano]

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(-)

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
-- 
2.44.0-651-g21306a098c

Re: [PATCH] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Dragan Simic]

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

Re: [PATCH] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Phillip Wood]

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

Re: [PATCH] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Junio C Hamano]

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' '

Re: [PATCH] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Dragan Simic]

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/

Re: [PATCH] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Jeff King]

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

Re: [PATCH] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Junio C Hamano]

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.

[PATCH v2] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Junio C Hamano]

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") if they wanted to.

This can of course be (ab)used to make the prefix "[WIP PATCH]" by
passing "--rfc=WIP".  Passing an empty string, i.e., "--rfc=", is
the same as "--no-rfc" to override an option given earlier on the
same command line.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Range-diff against v1:
1:  4077ed52e8 ! 1:  bffe0055d0 format-patch: allow --rfc to optionally take a value, like --rfc=WIP
    @@ Commit message
     
         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").
    +    "--rfc=RFC") if they wanted to.
     
         This can of course be (ab)used to make the prefix "[WIP PATCH]" by
    -    passing "--rfc=WIP".
    +    passing "--rfc=WIP".  Passing an empty string, i.e., "--rfc=", is
    +    the same as "--no-rfc" to override an option given earlier on the
    +    same command line.
     
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    @@ Documentation/git-format-patch.txt: the patches (with a value of e.g. "PATCH my-
     +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.
    ++is not complete yet ("WIP" stands for "Work In Progress").
      
      -v <n>::
      --reroll-count=<n>::
    @@ builtin/log.c: static int subject_prefix_callback(const struct option *opt, cons
     +{
     +	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;
     +}
     +
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      		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_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")),
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      		strbuf_addf(&sprefix, " v%s", reroll_count);
     
      ## t/t4014-format-patch.sh ##
    -@@ t/t4014-format-patch.sh: test_expect_success '--rfc' '
    +@@ t/t4014-format-patch.sh: test_expect_success 'empty subject prefix does not have extra space' '
      	test_cmp expect actual
      '
      
    -+test_expect_success '--rfc=WIP' '
    +-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 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' '
    - 	cat >expect <<-\EOF &&
    - 	Subject: [RFC PATCH foobar 1/1] header with . in it

 Documentation/git-format-patch.txt | 15 ++++++++++-----
 builtin/log.c                      | 22 ++++++++++++++++++----
 t/t4014-format-patch.sh            | 21 +++++++++++++++++++--
 3 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 728bb3821c..e553810b1e 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 ("WIP" stands for "Work In Progress").
 
 -v <n>::
 --reroll-count=<n>::
diff --git a/builtin/log.c b/builtin/log.c
index c0a8bb95e9..0e9c84e51d 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;
+
+	rfc = opt->value;
+	strbuf_reset(rfc);
+	if (!unset)
+		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_("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")),
@@ -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..645c4189f9 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1368,13 +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 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 &&
+	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' '
-- 
2.45.0-rc0

Re: [PATCH v2] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Phillip Wood]

Hi Junio

On 19/04/2024 23:01, 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") if they wanted to.
> 
> This can of course be (ab)used to make the prefix "[WIP PATCH]" by
> passing "--rfc=WIP".  Passing an empty string, i.e., "--rfc=", is
> the same as "--no-rfc" to override an option given earlier on the
> same command line.

The changes in this version all look good to me, I've left one comment 
below.

> @@ -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;

Looking at this again, do we really need an strbuf here? I think we 
could we use "const char *" instead as we always store a static string 
in it which would avoid a memory leak from not calling strbuf_release().

Best Wishes

Phillip

Re: [PATCH v2] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> +	struct strbuf rfc = STRBUF_INIT;
>
> Looking at this again, do we really need an strbuf here? I think we
> could we use "const char *" instead as we always store a static string
> in it which would avoid a memory leak from not calling
> strbuf_release().

Thanks for spotting the leak ;-)

And indeed, unlike sprefix, we do not need it to be an editable
strbuf.

Should be sufficient to squash in something like this.

 builtin/log.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git c/builtin/log.c w/builtin/log.c
index 0e9c84e51d..5c1c6f9b15 100644
--- c/builtin/log.c
+++ w/builtin/log.c
@@ -1497,12 +1497,11 @@ static int subject_prefix_callback(const struct option *opt, const char *arg,
 static int rfc_callback(const struct option *opt, const char *arg,
 			int unset)
 {
-	struct strbuf *rfc;
+	const char **rfc = opt->value;
 
-	rfc = opt->value;
-	strbuf_reset(rfc);
+	*rfc = opt->value;
 	if (!unset)
-		strbuf_addstr(rfc, arg ? arg : "RFC");
+		*rfc = arg ? arg : "RFC";
 	return 0;
 }
 
@@ -1919,7 +1918,7 @@ 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;
+	const char *rfc = NULL;
 	int creation_factor = -1;
 
 	const struct option builtin_format_patch_options[] = {
@@ -2064,8 +2063,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.len)
-		strbuf_insertf(&sprefix, 0, "%s ", rfc.buf);
+	if (rfc)
+		strbuf_insertf(&sprefix, 0, "%s ", rfc);
 
 	if (reroll_count) {
 		strbuf_addf(&sprefix, " v%s", reroll_count);

[PATCH v3 0/2] format-patch --rfc=WIP

[Junio C Hamano]

So here is the hopefully final round.  I think the first one is
good, but the second one is of dubious and possibly negative value
(for the reasons, see its proposed log message), so I am undecided.

Junio C Hamano (2):
  format-patch: allow --rfc to optionally take a value, like --rfc=WIP
  format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

 Documentation/git-format-patch.txt | 19 ++++++++++++++-----
 builtin/log.c                      | 25 +++++++++++++++++++++----
 t/t4014-format-patch.sh            | 28 +++++++++++++++++++++++++++-
 3 files changed, 62 insertions(+), 10 deletions(-)

Range-diff against v2:
1:  bffe0055d0 ! 1:  a874b85cb5 format-patch: allow --rfc to optionally take a value, like --rfc=WIP
    @@ builtin/log.c: static int subject_prefix_callback(const struct option *opt, cons
     +static int rfc_callback(const struct option *opt, const char *arg,
     +			int unset)
     +{
    -+	struct strbuf *rfc;
    ++	const char **rfc = opt->value;
     +
    -+	rfc = opt->value;
    -+	strbuf_reset(rfc);
    ++	*rfc = opt->value;
     +	if (!unset)
    -+		strbuf_addstr(rfc, arg ? arg : "RFC");
    ++		*rfc = arg ? arg : "RFC";
     +	return 0;
     +}
     +
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      	struct strbuf rdiff2 = STRBUF_INIT;
      	struct strbuf rdiff_title = STRBUF_INIT;
      	struct strbuf sprefix = STRBUF_INIT;
    -+	struct strbuf rfc = STRBUF_INIT;
    ++	const char *rfc = NULL;
      	int creation_factor = -1;
     -	int rfc = 0;
      
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      			    N_("cover-from-description-mode"),
      			    N_("generate parts of a cover letter based on a branch's description")),
     @@ builtin/log.c: 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)
    + 	if (rfc)
     -		strbuf_insertstr(&sprefix, 0, "RFC ");
    -+	if (rfc.len)
    -+		strbuf_insertf(&sprefix, 0, "%s ", rfc.buf);
    ++		strbuf_insertf(&sprefix, 0, "%s ", rfc);
      
      	if (reroll_count) {
      		strbuf_addf(&sprefix, " v%s", reroll_count);
-:  ---------- > 2:  6b82e903b6 format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]
-- 
2.45.0-rc0

[PATCH v3 1/2] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Junio C Hamano]

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") if they wanted to.

This can of course be (ab)used to make the prefix "[WIP PATCH]" by
passing "--rfc=WIP".  Passing an empty string, i.e., "--rfc=", is
the same as "--no-rfc" to override an option given earlier on the
same command line.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-format-patch.txt | 15 ++++++++++-----
 builtin/log.c                      | 19 ++++++++++++++++---
 t/t4014-format-patch.sh            | 21 +++++++++++++++++++--
 3 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 728bb3821c..e553810b1e 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 ("WIP" stands for "Work In Progress").
 
 -v <n>::
 --reroll-count=<n>::
diff --git a/builtin/log.c b/builtin/log.c
index c0a8bb95e9..5c1c6f9b15 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1494,6 +1494,17 @@ 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)
+{
+	const char **rfc = opt->value;
+
+	*rfc = opt->value;
+	if (!unset)
+		*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 +1918,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;
+	const char *rfc = NULL;
 	int creation_factor = -1;
-	int rfc = 0;
 
 	const struct option builtin_format_patch_options[] = {
 		OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
@@ -1932,7 +1943,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_("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")),
@@ -2051,7 +2064,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		cover_from_description_mode = parse_cover_from_description(cover_from_description_arg);
 
 	if (rfc)
-		strbuf_insertstr(&sprefix, 0, "RFC ");
+		strbuf_insertf(&sprefix, 0, "%s ", rfc);
 
 	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..645c4189f9 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1368,13 +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 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 &&
+	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' '
-- 
2.45.0-rc0

[PATCH v3 2/2] format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

[Junio C Hamano]

In the previous step, the "--rfc" option of "format-patch" learned
to take an optional string value to prepend to the subject prefix,
so that --rfc=WIP can give "[WIP PATCH]".  This commit shows that
the mechanism can be extended easily to allow "--rfc=-(WIP)" [*1*]
to signal that the extra string is to be appended instead of getting
prepended, resulting in "[PATCH (WIP)]".

Having worked on the patch, I am personally not 100% on board with
this part of the feature myself, and that is why this update is a
separate step from the main "--rfc takes an optional string value"
step.

If a way to prepend an arbitrary adornment is added to the system,
and people can now say "--rfc=RESEND" to produce "[RESEND PATCH]",
those who used to use "[PATCH RESEND]" by tweaking the subject
prefix in other ways [*2*] would do one of three things:

 (1) keep using their existing ways and keep sending their message
     with the "[RESEND PATCH]" prefix.

 (2) rejoice in the new automation, switch to use "--rfc=RESEND",
     and start sending their messages with "[RESEND PATCH]" prefix
     instead of "[PATCH RESEND]" prefix.

 (3) complain and demand a way to append instead of prepend so that
     they can use an automation to produce "[PATCH RESEND]" prefix.

I do not believe in adding slightly different ways that allow users
to be "original" when such differences do not make the world better
in meaningful ways [*3*], and I expect there are many more folks who
share that sentiment and go to route (2) than those who go to route
(3).  If my expectation is true, it means that this patch goes in a
wrong direction, and I would be happy to drop it.


[Footnote]

 *1* The syntax takes inspiration from Perl's three-arg open syntax
     that uses pipes "open fh, '|-', 'cmd'", where the dash signals
     "the other stuff comes here".

 *2* ... because "--rfc" in released versions does not even take any
     string value to prepend, let alone append, to begin with.

 *3* https://lore.kernel.org/git/b4d2b3faaf2914b7083327d5a4be3905@manjaro.org/
     gathered some stats to observe that "[RFC PATCH]" is more
     common than "[PATCH RFC]" by a wide margin, while trying to see
     how common "[RESEND PATCH]" (or "[PATCH RESED]") were used (the
     answer: much less common).  But it wouldn't have needed to
     count "[PATCH RFC]" and "[RFC PATCH]" separately if using a
     prefix and not a suffix (or vice versa) were established more
     firmly as the standard practice.

     It is a fine example that useless diversity making needless
     work.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-format-patch.txt | 4 ++++
 builtin/log.c                      | 8 ++++++--
 t/t4014-format-patch.sh            | 9 +++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index e553810b1e..dbccb210cd 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -247,6 +247,10 @@ 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 ("WIP" stands for "Work In Progress").
++
+If the string _<rfc>_ begins with a dash ("`-`"), the rest of the
+_<rfc>_ string is appended to the subject prefix instead, e.g.,
+`--rfc='-(WIP)'` results in "PATCH (WIP)".
 
 -v <n>::
 --reroll-count=<n>::
diff --git a/builtin/log.c b/builtin/log.c
index 5c1c6f9b15..dd11953260 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2063,8 +2063,12 @@ 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_insertf(&sprefix, 0, "%s ", rfc);
+	if (rfc) {
+		if (rfc[0] == '-')
+			strbuf_addf(&sprefix, " %s", rfc + 1);
+		else
+			strbuf_insertf(&sprefix, 0, "%s ", rfc);
+	}
 
 	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 645c4189f9..fcbde15b16 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1394,6 +1394,15 @@ test_expect_success '--rfc=WIP and --rfc=' '
 	test_cmp expect-raw actual
 '
 
+test_expect_success '--rfc=-(WIP) appends' '
+	cat >expect <<-\EOF &&
+	Subject: [PATCH (WIP) 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
-- 
2.45.0-rc0

Re: [PATCH v3 2/2] format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

[Dragan Simic]

Hello Junio,

On 2024-04-21 20:59, Junio C Hamano wrote:
> In the previous step, the "--rfc" option of "format-patch" learned
> to take an optional string value to prepend to the subject prefix,
> so that --rfc=WIP can give "[WIP PATCH]".  This commit shows that
> the mechanism can be extended easily to allow "--rfc=-(WIP)" [*1*]
> to signal that the extra string is to be appended instead of getting
> prepended, resulting in "[PATCH (WIP)]".
> 
> Having worked on the patch, I am personally not 100% on board with
> this part of the feature myself, and that is why this update is a
> separate step from the main "--rfc takes an optional string value"
> step.
> 
> If a way to prepend an arbitrary adornment is added to the system,
> and people can now say "--rfc=RESEND" to produce "[RESEND PATCH]",
> those who used to use "[PATCH RESEND]" by tweaking the subject
> prefix in other ways [*2*] would do one of three things:

There are even more issues, such as the grammar-related ones.  Let
me explain, please, as accurately as I can do that as a non-native
English speaker who spent many years studying English grammar.

Writing "RFC PATCH" puts "RFC" into the role of an adjective, which
is fine.  The same obviously applies to "WIP PATCH".  On the other
hand, writing "RESEND PATCH" puts "RESEND" into its only possible
role, which is transitive verb, and results in "RESEND PATCH" that
serves as a very simple sentence in imperative mood.  I'm not sure
that, strictly technically speaking, having simple sentences as the
prefixes is the desired outcome.

Technically, we should use "RE-SENT PATCH" to be on the correct side
from the grammar perspective, with "RE-SENT" serving as an adjective.
Before you ask, no, we can't use "RESENT" there, because its meaning
is completely different.  However, nobody uses "RE-SENT PATCH", or
at least I haven't seen it used yet.

When it comes to "PATCH RESEND", "RESEND" remains in its transitive
verb role, but "PATCH", as a noun, becomes a modifier of the verb.
Thus, the resulting meaning of "PATCH RESEND" becomes something like
"resend an item that's a patch", but not written in form of a simple
(or less simple) sentence.  Strictly technically, a noun should only
modify another noun, but bending English grammar a bit this way is
much better than actually having a simple sentence as a prefix.

With all this in mind, I think that allowing the "--rfc=-<string>"
form is the way to go.  Computer lingo often bends English grammar
to a certain degree, to achieve compactness, but bending and dumbing
it down more that it's actually necessary isn't something that we
should embrace.

I hope all this makes sense.

>  (1) keep using their existing ways and keep sending their message
>      with the "[RESEND PATCH]" prefix.
> 
>  (2) rejoice in the new automation, switch to use "--rfc=RESEND",
>      and start sending their messages with "[RESEND PATCH]" prefix
>      instead of "[PATCH RESEND]" prefix.
> 
>  (3) complain and demand a way to append instead of prepend so that
>      they can use an automation to produce "[PATCH RESEND]" prefix.
> 
> I do not believe in adding slightly different ways that allow users
> to be "original" when such differences do not make the world better
> in meaningful ways [*3*], and I expect there are many more folks who
> share that sentiment and go to route (2) than those who go to route
> (3).  If my expectation is true, it means that this patch goes in a
> wrong direction, and I would be happy to drop it.
> 
> 
> [Footnote]
> 
>  *1* The syntax takes inspiration from Perl's three-arg open syntax
>      that uses pipes "open fh, '|-', 'cmd'", where the dash signals
>      "the other stuff comes here".
> 
>  *2* ... because "--rfc" in released versions does not even take any
>      string value to prepend, let alone append, to begin with.
> 
>  *3* 
> https://lore.kernel.org/git/b4d2b3faaf2914b7083327d5a4be3905@manjaro.org/
>      gathered some stats to observe that "[RFC PATCH]" is more
>      common than "[PATCH RFC]" by a wide margin, while trying to see
>      how common "[RESEND PATCH]" (or "[PATCH RESED]") were used (the
>      answer: much less common).  But it wouldn't have needed to
>      count "[PATCH RFC]" and "[RFC PATCH]" separately if using a
>      prefix and not a suffix (or vice versa) were established more
>      firmly as the standard practice.
> 
>      It is a fine example that useless diversity making needless
>      work.

Re: [PATCH v3 2/2] format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

[Phillip Wood]

Hi Dragan

On 21/04/2024 20:37, Dragan Simic wrote:
> Hello Junio,
> 
> On 2024-04-21 20:59, Junio C Hamano wrote:
>> In the previous step, the "--rfc" option of "format-patch" learned
>> to take an optional string value to prepend to the subject prefix,
>> so that --rfc=WIP can give "[WIP PATCH]".  This commit shows that
>> the mechanism can be extended easily to allow "--rfc=-(WIP)" [*1*]
>> to signal that the extra string is to be appended instead of getting
>> prepended, resulting in "[PATCH (WIP)]".
>>
>> Having worked on the patch, I am personally not 100% on board with
>> this part of the feature myself, and that is why this update is a
>> separate step from the main "--rfc takes an optional string value"
>> step.
>>
>> If a way to prepend an arbitrary adornment is added to the system,
>> and people can now say "--rfc=RESEND" to produce "[RESEND PATCH]",
>> those who used to use "[PATCH RESEND]" by tweaking the subject
>> prefix in other ways [*2*] would do one of three things:
> 
> There are even more issues, such as the grammar-related ones. 

I think it is best to view the subject prefix as a list of space 
separated labels or keywords rather than part of a grammatically correct 
sentence.

Best Wishes

Phillip

  Let
> me explain, please, as accurately as I can do that as a non-native
> English speaker who spent many years studying English grammar.
> 
> Writing "RFC PATCH" puts "RFC" into the role of an adjective, which
> is fine.  The same obviously applies to "WIP PATCH".  On the other
> hand, writing "RESEND PATCH" puts "RESEND" into its only possible
> role, which is transitive verb, and results in "RESEND PATCH" that
> serves as a very simple sentence in imperative mood.  I'm not sure
> that, strictly technically speaking, having simple sentences as the
> prefixes is the desired outcome.
> 
> Technically, we should use "RE-SENT PATCH" to be on the correct side
> from the grammar perspective, with "RE-SENT" serving as an adjective.
> Before you ask, no, we can't use "RESENT" there, because its meaning
> is completely different.  However, nobody uses "RE-SENT PATCH", or
> at least I haven't seen it used yet.
> 
> When it comes to "PATCH RESEND", "RESEND" remains in its transitive
> verb role, but "PATCH", as a noun, becomes a modifier of the verb.
> Thus, the resulting meaning of "PATCH RESEND" becomes something like
> "resend an item that's a patch", but not written in form of a simple
> (or less simple) sentence.  Strictly technically, a noun should only
> modify another noun, but bending English grammar a bit this way is
> much better than actually having a simple sentence as a prefix.
> 
> With all this in mind, I think that allowing the "--rfc=-<string>"
> form is the way to go.  Computer lingo often bends English grammar
> to a certain degree, to achieve compactness, but bending and dumbing
> it down more that it's actually necessary isn't something that we
> should embrace.
> 
> I hope all this makes sense.
> 
>>  (1) keep using their existing ways and keep sending their message
>>      with the "[RESEND PATCH]" prefix.
>>
>>  (2) rejoice in the new automation, switch to use "--rfc=RESEND",
>>      and start sending their messages with "[RESEND PATCH]" prefix
>>      instead of "[PATCH RESEND]" prefix.
>>
>>  (3) complain and demand a way to append instead of prepend so that
>>      they can use an automation to produce "[PATCH RESEND]" prefix.
>>
>> I do not believe in adding slightly different ways that allow users
>> to be "original" when such differences do not make the world better
>> in meaningful ways [*3*], and I expect there are many more folks who
>> share that sentiment and go to route (2) than those who go to route
>> (3).  If my expectation is true, it means that this patch goes in a
>> wrong direction, and I would be happy to drop it.
>>
>>
>> [Footnote]
>>
>>  *1* The syntax takes inspiration from Perl's three-arg open syntax
>>      that uses pipes "open fh, '|-', 'cmd'", where the dash signals
>>      "the other stuff comes here".
>>
>>  *2* ... because "--rfc" in released versions does not even take any
>>      string value to prepend, let alone append, to begin with.
>>
>>  *3* 
>> https://lore.kernel.org/git/b4d2b3faaf2914b7083327d5a4be3905@manjaro.org/
>>      gathered some stats to observe that "[RFC PATCH]" is more
>>      common than "[PATCH RFC]" by a wide margin, while trying to see
>>      how common "[RESEND PATCH]" (or "[PATCH RESED]") were used (the
>>      answer: much less common).  But it wouldn't have needed to
>>      count "[PATCH RFC]" and "[RFC PATCH]" separately if using a
>>      prefix and not a suffix (or vice versa) were established more
>>      firmly as the standard practice.
>>
>>      It is a fine example that useless diversity making needless
>>      work.

Re: [PATCH v3 2/2] format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

[Dragan Simic]

Hello Phillip,

On 2024-04-24 12:17, Phillip Wood wrote:
> On 21/04/2024 20:37, Dragan Simic wrote:
>> On 2024-04-21 20:59, Junio C Hamano wrote:
>>> In the previous step, the "--rfc" option of "format-patch" learned
>>> to take an optional string value to prepend to the subject prefix,
>>> so that --rfc=WIP can give "[WIP PATCH]".  This commit shows that
>>> the mechanism can be extended easily to allow "--rfc=-(WIP)" [*1*]
>>> to signal that the extra string is to be appended instead of getting
>>> prepended, resulting in "[PATCH (WIP)]".
>>> 
>>> Having worked on the patch, I am personally not 100% on board with
>>> this part of the feature myself, and that is why this update is a
>>> separate step from the main "--rfc takes an optional string value"
>>> step.
>>> 
>>> If a way to prepend an arbitrary adornment is added to the system,
>>> and people can now say "--rfc=RESEND" to produce "[RESEND PATCH]",
>>> those who used to use "[PATCH RESEND]" by tweaking the subject
>>> prefix in other ways [*2*] would do one of three things:
>> 
>> There are even more issues, such as the grammar-related ones.
> 
> I think it is best to view the subject prefix as a list of space
> separated labels or keywords rather than part of a grammatically
> correct sentence.

With all due respect, I strongly disagree.  Viewing it that way and
letting "[RESEND PATCH]" be accepted as correct (and even enforced
a bit) is exactly what I below referred to as embracing the bending
of English grammar beyond what's actually necessary.

Please, let me remind you that I spent more than a couple of years
on English Wikipedia, writing new and expanding already existing
computing-related articles, during which time I participated in more
than a few grammar-related discussions.  All that makes me more
"sensitive" to breaking the English grammar rules when that actually
isn't necessary or beneficial.

>  Let
>> me explain, please, as accurately as I can do that as a non-native
>> English speaker who spent many years studying English grammar.
>> 
>> Writing "RFC PATCH" puts "RFC" into the role of an adjective, which
>> is fine.  The same obviously applies to "WIP PATCH".  On the other
>> hand, writing "RESEND PATCH" puts "RESEND" into its only possible
>> role, which is transitive verb, and results in "RESEND PATCH" that
>> serves as a very simple sentence in imperative mood.  I'm not sure
>> that, strictly technically speaking, having simple sentences as the
>> prefixes is the desired outcome.
>> 
>> Technically, we should use "RE-SENT PATCH" to be on the correct side
>> from the grammar perspective, with "RE-SENT" serving as an adjective.
>> Before you ask, no, we can't use "RESENT" there, because its meaning
>> is completely different.  However, nobody uses "RE-SENT PATCH", or
>> at least I haven't seen it used yet.
>> 
>> When it comes to "PATCH RESEND", "RESEND" remains in its transitive
>> verb role, but "PATCH", as a noun, becomes a modifier of the verb.
>> Thus, the resulting meaning of "PATCH RESEND" becomes something like
>> "resend an item that's a patch", but not written in form of a simple
>> (or less simple) sentence.  Strictly technically, a noun should only
>> modify another noun, but bending English grammar a bit this way is
>> much better than actually having a simple sentence as a prefix.
>> 
>> With all this in mind, I think that allowing the "--rfc=-<string>"
>> form is the way to go.  Computer lingo often bends English grammar
>> to a certain degree, to achieve compactness, but bending and dumbing
>> it down more that it's actually necessary isn't something that we
>> should embrace.
>> 
>> I hope all this makes sense.
>> 
>>>  (1) keep using their existing ways and keep sending their message
>>>      with the "[RESEND PATCH]" prefix.
>>> 
>>>  (2) rejoice in the new automation, switch to use "--rfc=RESEND",
>>>      and start sending their messages with "[RESEND PATCH]" prefix
>>>      instead of "[PATCH RESEND]" prefix.
>>> 
>>>  (3) complain and demand a way to append instead of prepend so that
>>>      they can use an automation to produce "[PATCH RESEND]" prefix.
>>> 
>>> I do not believe in adding slightly different ways that allow users
>>> to be "original" when such differences do not make the world better
>>> in meaningful ways [*3*], and I expect there are many more folks who
>>> share that sentiment and go to route (2) than those who go to route
>>> (3).  If my expectation is true, it means that this patch goes in a
>>> wrong direction, and I would be happy to drop it.
>>> 
>>> 
>>> [Footnote]
>>> 
>>>  *1* The syntax takes inspiration from Perl's three-arg open syntax
>>>      that uses pipes "open fh, '|-', 'cmd'", where the dash signals
>>>      "the other stuff comes here".
>>> 
>>>  *2* ... because "--rfc" in released versions does not even take any
>>>      string value to prepend, let alone append, to begin with.
>>> 
>>>  *3* 
>>> https://lore.kernel.org/git/b4d2b3faaf2914b7083327d5a4be3905@manjaro.org/
>>>      gathered some stats to observe that "[RFC PATCH]" is more
>>>      common than "[PATCH RFC]" by a wide margin, while trying to see
>>>      how common "[RESEND PATCH]" (or "[PATCH RESED]") were used (the
>>>      answer: much less common).  But it wouldn't have needed to
>>>      count "[PATCH RFC]" and "[RFC PATCH]" separately if using a
>>>      prefix and not a suffix (or vice versa) were established more
>>>      firmly as the standard practice.
>>> 
>>>      It is a fine example that useless diversity making needless
>>>      work.

[PATCH v4 0/2] format-patch --rfc=WIP

[Junio C Hamano]

In the previous iteration, I botched the implementation of the
"--no-rfc" case and somehow the test was "lucky" enough to leave
uninitialized pieces of memory filled with '\0's and did not catch
it.

The proposed log message for the second one has been reworded, and
the documentation (obliquely) cautions against abusing the suffix
form just to be different.

Junio C Hamano (2):
  format-patch: allow --rfc to optionally take a value, like --rfc=WIP
  format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

 Documentation/git-format-patch.txt | 21 ++++++++++++++++-----
 builtin/log.c                      | 27 +++++++++++++++++++++++----
 t/t4014-format-patch.sh            | 28 +++++++++++++++++++++++++++-
 3 files changed, 66 insertions(+), 10 deletions(-)

Range-diff against v3:
1:  7e022c85f7 ! 1:  30798e1211 format-patch: allow --rfc to optionally take a value, like --rfc=WIP
    @@ builtin/log.c: static int subject_prefix_callback(const struct option *opt, cons
     +	const char **rfc = opt->value;
     +
     +	*rfc = opt->value;
    -+	if (!unset)
    ++	if (unset)
    ++		*rfc = NULL;
    ++	else
     +		*rfc = arg ? arg : "RFC";
     +	return 0;
     +}
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      			    N_("cover-from-description-mode"),
      			    N_("generate parts of a cover letter based on a branch's description")),
     @@ builtin/log.c: 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)
    +-	if (rfc)
     -		strbuf_insertstr(&sprefix, 0, "RFC ");
    ++	if (rfc && rfc[0])
     +		strbuf_insertf(&sprefix, 0, "%s ", rfc);
      
      	if (reroll_count) {
2:  474041bdf8 ! 2:  fdbe198cd7 format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]
    @@ Commit message
     
         In the previous step, the "--rfc" option of "format-patch" learned
         to take an optional string value to prepend to the subject prefix,
    -    so that --rfc=WIP can give "[WIP PATCH]".  This commit shows that
    -    the mechanism can be extended easily to allow "--rfc=-(WIP)" [*1*]
    -    to signal that the extra string is to be appended instead of getting
    -    prepended, resulting in "[PATCH (WIP)]".
    -
    -    Having worked on the patch, I am personally not 100% on board with
    -    this part of the feature myself, and that is why this update is a
    -    separate step from the main "--rfc takes an optional string value"
    -    step.
    -
    -    If a way to prepend an arbitrary adornment is added to the system,
    -    and people can now say "--rfc=RESEND" to produce "[RESEND PATCH]",
    -    those who used to use "[PATCH RESEND]" by tweaking the subject
    -    prefix in other ways [*2*] would do one of three things:
    -
    -     (1) keep using their existing ways and keep sending their message
    -         with the "[RESEND PATCH]" prefix.
    -
    -     (2) rejoice in the new automation, switch to use "--rfc=RESEND",
    -         and start sending their messages with "[RESEND PATCH]" prefix
    -         instead of "[PATCH RESEND]" prefix.
    +    so that --rfc=WIP can give "[WIP PATCH]".
     
    -     (3) complain and demand a way to append instead of prepend so that
    -         they can use an automation to produce "[PATCH RESEND]" prefix.
    +    There may be cases in which the extra string wants to come after the
    +    subject prefix.  Extend the mechanism to allow "--rfc=-(WIP)" [*] to
    +    signal that the extra string is to be appended instead of getting
    +    prepended, resulting in "[PATCH (WIP)]".
     
    -    I do not believe in adding slightly different ways that allow users
    -    to be "original" when such differences do not make the world better
    -    in meaningful ways [*3*], and I expect there are many more folks who
    -    share that sentiment and go to route (2) than those who go to route
    -    (3).  If my expectation is true, it means that this patch goes in a
    -    wrong direction, and I would be happy to drop it.
    +    In the documentation, discourage (ab)using "--rfc=-RFC" to say
    +    "[PATCH RFC]" just to be different, when "[RFC PATCH]" is the norm.
     
         [Footnote]
     
    -     *1* The syntax takes inspiration from Perl's three-arg open syntax
    -         that uses pipes "open fh, '|-', 'cmd'", where the dash signals
    -         "the other stuff comes here".
    -
    -     *2* ... because "--rfc" in released versions does not even take any
    -         string value to prepend, let alone append, to begin with.
    -
    -     *3* https://lore.kernel.org/git/b4d2b3faaf2914b7083327d5a4be3905@manjaro.org/
    -         gathered some stats to observe that "[RFC PATCH]" is more
    -         common than "[PATCH RFC]" by a wide margin, while trying to see
    -         how common "[RESEND PATCH]" (or "[PATCH RESED]") were used (the
    -         answer: much less common).  But it wouldn't have needed to
    -         count "[PATCH RFC]" and "[RFC PATCH]" separately if using a
    -         prefix and not a suffix (or vice versa) were established more
    -         firmly as the standard practice.
    -
    -         It is a fine example that useless diversity making needless
    -         work.
    +     * The syntax takes inspiration from Perl's open syntax that opens
    +       pipes "open fh, '|-', 'cmd'", where the dash signals "the other
    +       stuff comes here".
     
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    @@ Documentation/git-format-patch.txt: RFC means "Request For Comments"; use this w
      "--rfc=WIP" may also be a useful way to indicate that a patch
      is not complete yet ("WIP" stands for "Work In Progress").
     ++
    -+If the string _<rfc>_ begins with a dash ("`-`"), the rest of the
    -+_<rfc>_ string is appended to the subject prefix instead, e.g.,
    -+`--rfc='-(WIP)'` results in "PATCH (WIP)".
    ++If the convention of the receiving community for a particular extra
    ++string is to have it _after_ the subject prefix, the string _<rfc>_
    ++can be prefixed with a dash ("`-`") to signal that the the rest of
    ++the _<rfc>_ string should be appended to the subject prefix instead,
    ++e.g., `--rfc='-(WIP)'` results in "PATCH (WIP)".
      
      -v <n>::
      --reroll-count=<n>::
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      	if (cover_from_description_arg)
      		cover_from_description_mode = parse_cover_from_description(cover_from_description_arg);
      
    --	if (rfc)
    +-	if (rfc && rfc[0])
     -		strbuf_insertf(&sprefix, 0, "%s ", rfc);
    -+	if (rfc) {
    ++	if (rfc && rfc[0]) {
     +		if (rfc[0] == '-')
     +			strbuf_addf(&sprefix, " %s", rfc + 1);
     +		else
-- 
2.45.0-rc0-3-g00e10ef10e

[PATCH v4 1/2] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Junio C Hamano]

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") if they wanted to.

This can of course be (ab)used to make the prefix "[WIP PATCH]" by
passing "--rfc=WIP".  Passing an empty string, i.e., "--rfc=", is
the same as "--no-rfc" to override an option given earlier on the
same command line.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-format-patch.txt | 15 ++++++++++-----
 builtin/log.c                      | 23 +++++++++++++++++++----
 t/t4014-format-patch.sh            | 21 +++++++++++++++++++--
 3 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 728bb3821c..e553810b1e 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 ("WIP" stands for "Work In Progress").
 
 -v <n>::
 --reroll-count=<n>::
diff --git a/builtin/log.c b/builtin/log.c
index c0a8bb95e9..97ca885b33 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1494,6 +1494,19 @@ 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)
+{
+	const char **rfc = opt->value;
+
+	*rfc = opt->value;
+	if (unset)
+		*rfc = NULL;
+	else
+		*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 +1920,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;
+	const char *rfc = NULL;
 	int creation_factor = -1;
-	int rfc = 0;
 
 	const struct option builtin_format_patch_options[] = {
 		OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
@@ -1932,7 +1945,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_("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")),
@@ -2050,8 +2065,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 && rfc[0])
+		strbuf_insertf(&sprefix, 0, "%s ", rfc);
 
 	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..645c4189f9 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1368,13 +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 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 &&
+	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' '
-- 
2.45.0-rc0-3-g00e10ef10e

Re: [PATCH v4 1/2] format-patch: allow --rfc to optionally take a value, like --rfc=WIP

[Phillip Wood]

Hi Junio

This version looks good to me

Best Wishes

Phillip

On 23/04/2024 18:52, 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") if they wanted to.
> 
> This can of course be (ab)used to make the prefix "[WIP PATCH]" by
> passing "--rfc=WIP".  Passing an empty string, i.e., "--rfc=", is
> the same as "--no-rfc" to override an option given earlier on the
> same command line.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   Documentation/git-format-patch.txt | 15 ++++++++++-----
>   builtin/log.c                      | 23 +++++++++++++++++++----
>   t/t4014-format-patch.sh            | 21 +++++++++++++++++++--
>   3 files changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 728bb3821c..e553810b1e 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 ("WIP" stands for "Work In Progress").
>   
>   -v <n>::
>   --reroll-count=<n>::
> diff --git a/builtin/log.c b/builtin/log.c
> index c0a8bb95e9..97ca885b33 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1494,6 +1494,19 @@ 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)
> +{
> +	const char **rfc = opt->value;
> +
> +	*rfc = opt->value;
> +	if (unset)
> +		*rfc = NULL;
> +	else
> +		*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 +1920,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;
> +	const char *rfc = NULL;
>   	int creation_factor = -1;
> -	int rfc = 0;
>   
>   	const struct option builtin_format_patch_options[] = {
>   		OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
> @@ -1932,7 +1945,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_("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")),
> @@ -2050,8 +2065,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 && rfc[0])
> +		strbuf_insertf(&sprefix, 0, "%s ", rfc);
>   
>   	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..645c4189f9 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1368,13 +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 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 &&
> +	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' '

[PATCH v4 2/2] format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

[Junio C Hamano]

In the previous step, the "--rfc" option of "format-patch" learned
to take an optional string value to prepend to the subject prefix,
so that --rfc=WIP can give "[WIP PATCH]".

There may be cases in which the extra string wants to come after the
subject prefix.  Extend the mechanism to allow "--rfc=-(WIP)" [*] to
signal that the extra string is to be appended instead of getting
prepended, resulting in "[PATCH (WIP)]".

In the documentation, discourage (ab)using "--rfc=-RFC" to say
"[PATCH RFC]" just to be different, when "[RFC PATCH]" is the norm.

[Footnote]

 * The syntax takes inspiration from Perl's open syntax that opens
   pipes "open fh, '|-', 'cmd'", where the dash signals "the other
   stuff comes here".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-format-patch.txt | 6 ++++++
 builtin/log.c                      | 8 ++++++--
 t/t4014-format-patch.sh            | 9 +++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index e553810b1e..369af2c4a7 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -247,6 +247,12 @@ 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 ("WIP" stands for "Work In Progress").
++
+If the convention of the receiving community for a particular extra
+string is to have it _after_ the subject prefix, the string _<rfc>_
+can be prefixed with a dash ("`-`") to signal that the the rest of
+the _<rfc>_ string should be appended to the subject prefix instead,
+e.g., `--rfc='-(WIP)'` results in "PATCH (WIP)".
 
 -v <n>::
 --reroll-count=<n>::
diff --git a/builtin/log.c b/builtin/log.c
index 97ca885b33..4750e480e6 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2065,8 +2065,12 @@ 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 && rfc[0])
-		strbuf_insertf(&sprefix, 0, "%s ", rfc);
+	if (rfc && rfc[0]) {
+		if (rfc[0] == '-')
+			strbuf_addf(&sprefix, " %s", rfc + 1);
+		else
+			strbuf_insertf(&sprefix, 0, "%s ", rfc);
+	}
 
 	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 645c4189f9..fcbde15b16 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1394,6 +1394,15 @@ test_expect_success '--rfc=WIP and --rfc=' '
 	test_cmp expect-raw actual
 '
 
+test_expect_success '--rfc=-(WIP) appends' '
+	cat >expect <<-\EOF &&
+	Subject: [PATCH (WIP) 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
-- 
2.45.0-rc0-3-g00e10ef10e

Re: [PATCH v4 2/2] format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

[Phillip Wood]

Hi Junio

On 23/04/2024 18:52, Junio C Hamano wrote:
> In the previous step, the "--rfc" option of "format-patch" learned
> to take an optional string value to prepend to the subject prefix,
> so that --rfc=WIP can give "[WIP PATCH]".
> 
> There may be cases in which the extra string wants to come after the
> subject prefix.  Extend the mechanism to allow "--rfc=-(WIP)" [*] to
> signal that the extra string is to be appended instead of getting
> prepended, resulting in "[PATCH (WIP)]".
> 
> In the documentation, discourage (ab)using "--rfc=-RFC" to say
> "[PATCH RFC]" just to be different, when "[RFC PATCH]" is the norm.
> 
> [Footnote]
> 
>   * The syntax takes inspiration from Perl's open syntax that opens
>     pipes "open fh, '|-', 'cmd'", where the dash signals "the other
>     stuff comes here".

I'm not convinced this is a good idea as I'm not sure how adding "RFC" 
at the end of the subject prefix makes the world better than just having 
at the start of the prefix and I find using "-" to do that quite confusing.

Best Wishes

Phillip

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   Documentation/git-format-patch.txt | 6 ++++++
>   builtin/log.c                      | 8 ++++++--
>   t/t4014-format-patch.sh            | 9 +++++++++
>   3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index e553810b1e..369af2c4a7 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -247,6 +247,12 @@ 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 ("WIP" stands for "Work In Progress").
> ++
> +If the convention of the receiving community for a particular extra
> +string is to have it _after_ the subject prefix, the string _<rfc>_
> +can be prefixed with a dash ("`-`") to signal that the the rest of
> +the _<rfc>_ string should be appended to the subject prefix instead,
> +e.g., `--rfc='-(WIP)'` results in "PATCH (WIP)".
>   
>   -v <n>::
>   --reroll-count=<n>::
> diff --git a/builtin/log.c b/builtin/log.c
> index 97ca885b33..4750e480e6 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -2065,8 +2065,12 @@ 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 && rfc[0])
> -		strbuf_insertf(&sprefix, 0, "%s ", rfc);
> +	if (rfc && rfc[0]) {
> +		if (rfc[0] == '-')
> +			strbuf_addf(&sprefix, " %s", rfc + 1);
> +		else
> +			strbuf_insertf(&sprefix, 0, "%s ", rfc);
> +	}
>   
>   	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 645c4189f9..fcbde15b16 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1394,6 +1394,15 @@ test_expect_success '--rfc=WIP and --rfc=' '
>   	test_cmp expect-raw actual
>   '
>   
> +test_expect_success '--rfc=-(WIP) appends' '
> +	cat >expect <<-\EOF &&
> +	Subject: [PATCH (WIP) 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

Re: [PATCH v4 2/2] format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I'm not convinced this is a good idea as I'm not sure how adding "RFC"
> at the end of the subject prefix makes the world better than just
> having at the start of the prefix and I find using "-" to do that
> quite confusing.

I am not convinced it is a good idea, either.  "PATCH (WIP)" was the
best example I could come up with.  I am also a fan of "a list of
space separated labels or keywords" you mentioned, but *if* a
project convention somewhere is to have them before "PATCH", then it
is not entirely unreasonable to wish to have a way to prepend these
labels.

But I am fine to drop it for the sake of simplicity.  It would help
discourage users from trying to be "original" in a way that does not
make a material difference.  If a project comes with a concrete need
to prepend, the patch is always resurrectable from the list archive.

As to the syntax, I think "-" is a fairly good way to indicate
whether it goes to the front or back.  When told to "Combine '-RFC'
and 'PATCH'", I expect that most people would give 'PATCH-RFC' and
not '-RFC PATCH'.

Thanks.

Re: [PATCH v4 2/2] format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

[Dragan Simic]

Hello Junio,

On 2024-04-24 17:25, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> I'm not convinced this is a good idea as I'm not sure how adding "RFC"
>> at the end of the subject prefix makes the world better than just
>> having at the start of the prefix and I find using "-" to do that
>> quite confusing.
> 
> I am not convinced it is a good idea, either.  "PATCH (WIP)" was the
> best example I could come up with.  I am also a fan of "a list of
> space separated labels or keywords" you mentioned, but *if* a
> project convention somewhere is to have them before "PATCH", then it
> is not entirely unreasonable to wish to have a way to prepend these
> labels.
> 
> But I am fine to drop it for the sake of simplicity.  It would help
> discourage users from trying to be "original" in a way that does not
> make a material difference.  If a project comes with a concrete need
> to prepend, the patch is always resurrectable from the list archive.

Yes, it would help with discouraging the users from becoming
"inventive", but would also promote the rot of English grammar,
as I already tried to explain. [1][2]

I'm always for simplicity, unless it actually results in some
possibly negative effects.

[1] 
https://lore.kernel.org/git/f9aae9692493e4b722ce9f38de73c810@manjaro.org/
[2] 
https://lore.kernel.org/git/115acd1529d9529ef5bb095c074ad83d@manjaro.org/

> As to the syntax, I think "-" is a fairly good way to indicate
> whether it goes to the front or back.  When told to "Combine '-RFC'
> and 'PATCH'", I expect that most people would give 'PATCH-RFC' and
> not '-RFC PATCH'.

I find the syntax just fine.

Re: [PATCH v4 2/2] format-patch: "--rfc=-(WIP)" appends to produce [PATCH (WIP)]

[Dragan Simic]

Hello Phillip,

On 2024-04-24 12:16, Phillip Wood wrote:
> On 23/04/2024 18:52, Junio C Hamano wrote:
>> In the previous step, the "--rfc" option of "format-patch" learned
>> to take an optional string value to prepend to the subject prefix,
>> so that --rfc=WIP can give "[WIP PATCH]".
>> 
>> There may be cases in which the extra string wants to come after the
>> subject prefix.  Extend the mechanism to allow "--rfc=-(WIP)" [*] to
>> signal that the extra string is to be appended instead of getting
>> prepended, resulting in "[PATCH (WIP)]".
>> 
>> In the documentation, discourage (ab)using "--rfc=-RFC" to say
>> "[PATCH RFC]" just to be different, when "[RFC PATCH]" is the norm.
>> 
>> [Footnote]
>> 
>>   * The syntax takes inspiration from Perl's open syntax that opens
>>     pipes "open fh, '|-', 'cmd'", where the dash signals "the other
>>     stuff comes here".
> 
> I'm not convinced this is a good idea as I'm not sure how adding "RFC"
> at the end of the subject prefix makes the world better than just
> having at the start of the prefix and I find using "-" to do that
> quite confusing.

Please, read my earlier responses [1][2] to see why does this
feature actually make the world a bit better.  To sum it up, just
as there's bit rot, there's also English grammar rot, which we
shouldn't embrace or promote.

[1] 
https://lore.kernel.org/git/f9aae9692493e4b722ce9f38de73c810@manjaro.org/
[2] 
https://lore.kernel.org/git/115acd1529d9529ef5bb095c074ad83d@manjaro.org/

>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>   Documentation/git-format-patch.txt | 6 ++++++
>>   builtin/log.c                      | 8 ++++++--
>>   t/t4014-format-patch.sh            | 9 +++++++++
>>   3 files changed, 21 insertions(+), 2 deletions(-)
>> 
>> diff --git a/Documentation/git-format-patch.txt 
>> b/Documentation/git-format-patch.txt
>> index e553810b1e..369af2c4a7 100644
>> --- a/Documentation/git-format-patch.txt
>> +++ b/Documentation/git-format-patch.txt
>> @@ -247,6 +247,12 @@ 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 ("WIP" stands for "Work In Progress").
>> ++
>> +If the convention of the receiving community for a particular extra
>> +string is to have it _after_ the subject prefix, the string _<rfc>_
>> +can be prefixed with a dash ("`-`") to signal that the the rest of
>> +the _<rfc>_ string should be appended to the subject prefix instead,
>> +e.g., `--rfc='-(WIP)'` results in "PATCH (WIP)".
>>     -v <n>::
>>   --reroll-count=<n>::
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 97ca885b33..4750e480e6 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -2065,8 +2065,12 @@ 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 && rfc[0])
>> -		strbuf_insertf(&sprefix, 0, "%s ", rfc);
>> +	if (rfc && rfc[0]) {
>> +		if (rfc[0] == '-')
>> +			strbuf_addf(&sprefix, " %s", rfc + 1);
>> +		else
>> +			strbuf_insertf(&sprefix, 0, "%s ", rfc);
>> +	}
>>     	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 645c4189f9..fcbde15b16 100755
>> --- a/t/t4014-format-patch.sh
>> +++ b/t/t4014-format-patch.sh
>> @@ -1394,6 +1394,15 @@ test_expect_success '--rfc=WIP and --rfc=' '
>>   	test_cmp expect-raw actual
>>   '
>>   +test_expect_success '--rfc=-(WIP) appends' '
>> +	cat >expect <<-\EOF &&
>> +	Subject: [PATCH (WIP) 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