PATCH/RFC: format-patch: Add format.subjectprefixsep to change separators

Some mailing list use "PATCH:" rather than "[PATCH]" to prefix
patches, so introduce a new option to configure:
 - 2 chars that would enclose PATCH (and counters)
 - 1 char that would come just after the PATCH (and counters)
---
This mail has been sent with:
 git -c format.subjectprefixsep=: send-email --annotate --subject-prefix=PATCH/RFC

 Documentation/config.txt                           |  5 ++
 builtin/log.c                                      |  5 ++
 log-tree.c                                         | 23 ++++--
 revision.h                                         |  1 +
 t/t4013-diff-various.sh                            |  2 +
 ...fixsep=:_format-patch_--stdout_initial..master^ | 81 ++++++++++++++++++++++
 ...ixsep=<>_format-patch_--stdout_initial..master^ | 81 ++++++++++++++++++++++
 7 files changed, 194 insertions(+), 4 deletions(-)
 create mode 100644 t/t4013/diff.-c_format.subjectprefixsep=:_format-patch_--stdout_initial..master^
 create mode 100644 t/t4013/diff.-c_format.subjectprefixsep=<>_format-patch_--stdout_initial..master^

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6862e3e..8cf8aac 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1151,6 +1151,11 @@ format.subjectprefix::
 	The default for format-patch is to output files with the '[PATCH]'
 	subject prefix. Use this variable to change that prefix.
 
+format.subjectprefixsep::
+	The default for format-patch is to enclose subjectprefix with '[]'.
+	If only one char is given it is placed after the prefix. If it
+	contains two chars, they enclose the prefix.
+
 format.signature::
 	The default for format-patch is to output a signature containing
 	the Git version number. Use this variable to change that default.
diff --git a/builtin/log.c b/builtin/log.c
index f2a9f01..18a4e7e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -35,6 +35,7 @@ static int decoration_style;
 static int decoration_given;
 static int use_mailmap_config;
 static const char *fmt_patch_subject_prefix = "PATCH";
+static const char *fmt_patch_subject_prefix_sep = "[]";
 static const char *fmt_pretty;
 
 static const char * const builtin_log_usage[] = {
@@ -109,6 +110,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 	rev->abbrev_commit = default_abbrev_commit;
 	rev->show_root_diff = default_show_root;
 	rev->subject_prefix = fmt_patch_subject_prefix;
+	rev->subject_prefix_sep = fmt_patch_subject_prefix_sep;
 	DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
 
 	if (default_date_mode)
@@ -374,6 +376,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return git_config_string(&fmt_pretty, var, value);
 	if (!strcmp(var, "format.subjectprefix"))
 		return git_config_string(&fmt_patch_subject_prefix, var, value);
+	if (!strcmp(var, "format.subjectprefixsep"))
+		return git_config_string(&fmt_patch_subject_prefix_sep, var, value);
 	if (!strcmp(var, "log.abbrevcommit")) {
 		default_abbrev_commit = git_config_bool(var, value);
 		return 0;
@@ -1259,6 +1263,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.max_parents = 1;
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 	rev.subject_prefix = fmt_patch_subject_prefix;
+	rev.subject_prefix_sep = fmt_patch_subject_prefix_sep;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.def = "HEAD";
 	s_r_opt.revarg_opt = REVARG_COMMITTISH;
diff --git a/log-tree.c b/log-tree.c
index 7f0890e..8449d19 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -269,22 +269,37 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 	const char *subject = NULL;
 	const char *extra_headers = opt->extra_headers;
 	const char *name = sha1_to_hex(commit->object.sha1);
+	char subject_sep_pre[2];
+	char subject_sep_post[2];
+
+	subject_sep_pre[0] = subject_sep_post[0] = '\0';
+	if (strlen(opt->subject_prefix_sep) > 1) {
+		subject_sep_pre[0] = opt->subject_prefix_sep[0];
+		subject_sep_post[0] = opt->subject_prefix_sep[1];
+	} else {
+		subject_sep_post[0] = opt->subject_prefix_sep[0];
+	}
+	subject_sep_pre[1] = subject_sep_post[1] = '\0';
 
 	*need_8bit_cte_p = 0; /* unknown */
 	if (opt->total > 0) {
 		static char buffer[64];
 		snprintf(buffer, sizeof(buffer),
-			 "Subject: [%s%s%0*d/%d] ",
+			 "Subject: %s%s%s%0*d/%d%s ",
+			 subject_sep_pre,
 			 opt->subject_prefix,
 			 *opt->subject_prefix ? " " : "",
 			 digits_in_number(opt->total),
-			 opt->nr, opt->total);
+			 opt->nr, opt->total,
+			 subject_sep_post);
 		subject = buffer;
 	} else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {
 		static char buffer[256];
 		snprintf(buffer, sizeof(buffer),
-			 "Subject: [%s] ",
-			 opt->subject_prefix);
+			 "Subject: %s%s%s ",
+			 subject_sep_pre,
+			 opt->subject_prefix,
+			 subject_sep_post);
 		subject = buffer;
 	} else {
 		subject = "Subject: ";
diff --git a/revision.h b/revision.h
index 9cb5adc..baae88f 100644
--- a/revision.h
+++ b/revision.h
@@ -162,6 +162,7 @@ struct rev_info {
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;
+	const char	*subject_prefix_sep;
 	int		no_inline;
 	int		show_log_size;
 	struct string_list *mailmap;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 6ec6072..d2257ab 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -289,6 +289,8 @@ format-patch --inline --stdout --subject-prefix=TESTCASE initial..master
 config format.subjectprefix DIFFERENT_PREFIX
 format-patch --inline --stdout initial..master^^
 format-patch --stdout --cover-letter -n initial..master^
+-c format.subjectprefixsep=: format-patch --stdout initial..master^
+-c format.subjectprefixsep=<> format-patch --stdout initial..master^
 
 diff --abbrev initial..side
 diff -r initial..side
diff --git a/t/t4013/diff.-c_format.subjectprefixsep=:_format-patch_--stdout_initial..master^ b/t/t4013/diff.-c_format.subjectprefixsep=:_format-patch_--stdout_initial..master^
new file mode 100644
index 0000000..b4a741b
--- /dev/null
+++ b/t/t4013/diff.-c_format.subjectprefixsep=:_format-patch_--stdout_initial..master^
@@ -0,0 +1,81 @@
+$ git -c format.subjectprefixsep=: format-patch --stdout initial..master^
+From 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44 Mon Sep 17 00:00:00 2001
+From: A U Thor <author@example.com>
+Date: Mon, 26 Jun 2006 00:01:00 +0000
+Subject: DIFFERENT_PREFIX 1/2: Second
+
+This is the second commit.
+---
+ dir/sub | 2 ++
+ file0   | 3 +++
+ file2   | 3 ---
+ 3 files changed, 5 insertions(+), 3 deletions(-)
+ delete mode 100644 file2
+
+diff --git a/dir/sub b/dir/sub
+index 35d242b..8422d40 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++C
++D
+diff --git a/file0 b/file0
+index 01e79c3..b414108 100644
+--- a/file0
++++ b/file0
+@@ -1,3 +1,6 @@
+ 1
+ 2
+ 3
++4
++5
++6
+diff --git a/file2 b/file2
+deleted file mode 100644
+index 01e79c3..0000000
+--- a/file2
++++ /dev/null
+@@ -1,3 +0,0 @@
+-1
+-2
+-3
+-- 
+g-i-t--v-e-r-s-i-o-n
+
+
+From 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 Mon Sep 17 00:00:00 2001
+From: A U Thor <author@example.com>
+Date: Mon, 26 Jun 2006 00:02:00 +0000
+Subject: DIFFERENT_PREFIX 2/2: Third
+
+---
+ dir/sub | 2 ++
+ file1   | 3 +++
+ 2 files changed, 5 insertions(+)
+ create mode 100644 file1
+
+diff --git a/dir/sub b/dir/sub
+index 8422d40..cead32e 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -2,3 +2,5 @@ A
+ B
+ C
+ D
++E
++F
+diff --git a/file1 b/file1
+new file mode 100644
+index 0000000..b1e6722
+--- /dev/null
++++ b/file1
+@@ -0,0 +1,3 @@
++A
++B
++C
+-- 
+g-i-t--v-e-r-s-i-o-n
+
+$
diff --git a/t/t4013/diff.-c_format.subjectprefixsep=<>_format-patch_--stdout_initial..master^ b/t/t4013/diff.-c_format.subjectprefixsep=<>_format-patch_--stdout_initial..master^
new file mode 100644
index 0000000..c0305df
--- /dev/null
+++ b/t/t4013/diff.-c_format.subjectprefixsep=<>_format-patch_--stdout_initial..master^
@@ -0,0 +1,81 @@
+$ git -c format.subjectprefixsep=<> format-patch --stdout initial..master^
+From 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44 Mon Sep 17 00:00:00 2001
+From: A U Thor <author@example.com>
+Date: Mon, 26 Jun 2006 00:01:00 +0000
+Subject: <DIFFERENT_PREFIX 1/2> Second
+
+This is the second commit.
+---
+ dir/sub | 2 ++
+ file0   | 3 +++
+ file2   | 3 ---
+ 3 files changed, 5 insertions(+), 3 deletions(-)
+ delete mode 100644 file2
+
+diff --git a/dir/sub b/dir/sub
+index 35d242b..8422d40 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++C
++D
+diff --git a/file0 b/file0
+index 01e79c3..b414108 100644
+--- a/file0
++++ b/file0
+@@ -1,3 +1,6 @@
+ 1
+ 2
+ 3
++4
++5
++6
+diff --git a/file2 b/file2
+deleted file mode 100644
+index 01e79c3..0000000
+--- a/file2
++++ /dev/null
+@@ -1,3 +0,0 @@
+-1
+-2
+-3
+-- 
+g-i-t--v-e-r-s-i-o-n
+
+
+From 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 Mon Sep 17 00:00:00 2001
+From: A U Thor <author@example.com>
+Date: Mon, 26 Jun 2006 00:02:00 +0000
+Subject: <DIFFERENT_PREFIX 2/2> Third
+
+---
+ dir/sub | 2 ++
+ file1   | 3 +++
+ 2 files changed, 5 insertions(+)
+ create mode 100644 file1
+
+diff --git a/dir/sub b/dir/sub
+index 8422d40..cead32e 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -2,3 +2,5 @@ A
+ B
+ C
+ D
++E
++F
+diff --git a/file1 b/file1
+new file mode 100644
+index 0000000..b1e6722
+--- /dev/null
++++ b/file1
+@@ -0,0 +1,3 @@
++A
++B
++C
+-- 
+g-i-t--v-e-r-s-i-o-n
+
+$

-- 
2.2.1.213.gdc6c5b9

Junio C Hamano· Jan 4, 2015, 19:55 UTC · re: Marc Finet · lore

Re: PATCH/RFC: format-patch: Add format.subjectprefixsep to change separators

Marc Finet <m.dreadlock@gmail.com> writes:

> Some mailing list use "PATCH:" rather than "[PATCH]" to prefix
> patches, so introduce a new option to configure:
>  - 2 chars that would enclose PATCH (and counters)
>  - 1 char that would come just after the PATCH (and counters)
> ---
> This mail has been sent with:
>  git -c format.subjectprefixsep=: send-email --annotate --subject-prefix=PATCH/RFC

A few comments:

 - "Some mailing lists" may want to say "[PATCH v3 #4 of 10]" or
   somesuch; as a customization mechanism, the approach this patch
   takes falls way short to be useful.  "--subject=<format>" option
   where <format> is similar the "log --format" options, e.g.

   --subject="[PATCH% v #%N of %T] %s"

   with format-patch specific set of substitutions (in the above
   example, %v stands for patch version, %N patch number and %T
   total number of patches in the series) may be a better way to go.

 - Do not add configuration variable before you add command line
   option.  Add option first and then when option proves useful you
   can have the corresponding variable, not the other way around.
   Make sure that the comamnd line option overrides configuration
   variable while adding the variable in the second step of such a
   patch series.

Having said all that.

Re: PATCH/RFC: format-patch: Add format.subjectprefixsep to change separators

On Sun, Jan 04, 2015 at 11:55:24AM -0800, Junio C Hamano wrote:

> Marc Finet <m.dreadlock@gmail.com> writes:
> 
> > Some mailing list use "PATCH:" rather than "[PATCH]" to prefix
> > patches, so introduce a new option to configure:
> >  - 2 chars that would enclose PATCH (and counters)
> >  - 1 char that would come just after the PATCH (and counters)
> > ---
> > This mail has been sent with:
> >  git -c format.subjectprefixsep=: send-email --annotate --subject-prefix=PATCH/RFC
> 
> A few comments:
> 
>  - "Some mailing lists" may want to say "[PATCH v3 #4 of 10]" or
>    somesuch; as a customization mechanism, the approach this patch
>    takes falls way short to be useful.  "--subject=<format>" option
>    where <format> is similar the "log --format" options, e.g.
> 
>    --subject="[PATCH% v #%N of %T] %s"
> 
>    with format-patch specific set of substitutions (in the above
>    example, %v stands for patch version, %N patch number and %T
>    total number of patches in the series) may be a better way to go.

In fact the log-tree.c::log_write_email_headers() has two cases
depending on the number of patches to send. So we need either two (or
three) options or we need to implement (because AFAIK it does not exists
yet) conditionals. Both seemed to me a little bit overkill here.
 

>  - Do not add configuration variable before you add command line
>    option.  Add option first and then when option proves useful you
>    can have the corresponding variable, not the other way around.
>    Make sure that the comamnd line option overrides configuration
>    variable while adding the variable in the second step of such a
>    patch series.

Ok.

> Having said all that.
> 
> What are these mailing lists and why are they using non-standard
> convention?  Back when Git was young, we would have added more knobs
> to adjust the behaviour to existing prevailing convention, but now
> Git is older than X% of projects that use Git where the number X is
> a pretty large number.  Perhaps just like they (whichever mailing
> lists they are) switched out of Subversion or CVS and started using
> Git to come to the modern world, maybe it is time they switch their
> convention as well?

So, I do not know what to do now:
 - stick to [PATCH]
 - try one of the two first alternatives above (multiple options or
   implement conditionals)
 - re-work this patch by implementing the command line option, creating
   an other patch to use a configuration option, and hope it would be
   accepted because it makes sense to some people. The only advantage of
   using PATCH: rather than [PATCH] is that 1 char is saved :|. Making
   the subject less 'aggressive' is a feature but not necessarily an
   advantage.

Thanks,

Marc.