{"thread":{"id":"20122","subject":"\"mailinfo: Remove only one set of square brackets\" considered harmful","startedAt":"2009-07-15T15:30:58Z","lastAt":"2009-07-15T22:31:44Z","messageCount":4,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"118033","messageId":"alpine.LFD.2.01.0907150821280.13838@localhost.localdomain","threadId":"20122","inReplyTo":null,"subject":"\"mailinfo: Remove only one set of square brackets\" considered harmful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-15T15:30:58Z","receivedAt":"2009-07-15T15:30:58Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nSo I see why Andreas did it, and I don't disagree violently, BUT...\n\nThe fact is, we have mailing lists etc that add their own headers to the \nsubject, and they know they can add things in brackets. The most obvious \nexample is the Linux kernel security list, which adds a prefix of\n\n\t\"[Security] \"\n\nto the subject line in order to stand out (I'm on other lists that do \nthis too, but those generally don'thave patches).\n\nSo I have emails witgh subjects like\n\n\tSubject: [Security] [patch] random: make get_random_int() more random\n\nbut I also have people who do the same thing themselves, eg:\n\n\tSubject: [PATCH -rc] [BUGFIX] x86: fix kernel_trap_sp()\n\tSubject: [BUGFIX][PATCH] fix bad page removal from LRU (Was Re: [RFC][PATCH] ..\n\nso people did kind of depend on the \"remove square brackets\" behavior.\n\nSure, I end up editing the subject lines (and in that last example I would \nhave had to anyway), but I'm not so sure this was a good change.\n\nThe commit log says:\n\n    However, since format-patch only adds one set of square brackets,\n    this behaviour is quite easily undesrstood and defended while the\n    previous behaviour is not.\n\nbut sadly, Andreas totally missed the fact that we're not talking about \njust format-patch, and that the whole bracket removal is about emails \nin general.\n\nSo I'd suggest at least a setting to reinstate the previous behavior.\n\n\t\t\tLinus\n"},{"id":"118048","messageId":"7vljmpegy4.fsf@alter.siamese.dyndns.org","threadId":"20122","inReplyTo":"alpine.LFD.2.01.0907150821280.13838@localhost.localdomain","subject":"Re: \"mailinfo: Remove only one set of square brackets\" considered harmful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-15T22:09:55Z","receivedAt":"2009-07-15T22:09:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> So I see why Andreas did it, and I don't disagree violently, BUT...\n>\n> The fact is, we have mailing lists etc that add their own headers to the \n> subject, and they know they can add things in brackets. The most obvious \n> example is the Linux kernel security list, which adds a prefix of\n>\n> \t\"[Security] \"\n>\n> to the subject line in order to stand out (I'm on other lists that do \n> this too, but those generally don'thave patches).\n>>\n> So I have emails witgh subjects like\n>\n> \tSubject: [Security] [patch] random: make get_random_int() more random\n>\n> but I also have people who do the same thing themselves, eg:\n>\n> \tSubject: [PATCH -rc] [BUGFIX] x86: fix kernel_trap_sp()\n> \tSubject: [BUGFIX][PATCH] fix bad page removal from LRU (Was Re: [RFC][PATCH] ..\n>\n> so people did kind of depend on the \"remove square brackets\" behavior.\n\nThanks.  The reason why I have merged some questionable stuff (including\nthis one) early in this cycle was exactly because we would want to catch\nreal world breakages caused by such changes.\n\nEven though it is silly not to rely on already well established\nconventions such as X-Mailing-List and List-ID but instead to waste\nprecious real estate at the initial part of the Subject in this century\nmerely for list identification purposes, this change regresses the end\nresult.\n"},{"id":"118051","messageId":"7vfxcxefyn.fsf@alter.siamese.dyndns.org","threadId":"20122","inReplyTo":"7vljmpegy4.fsf@alter.siamese.dyndns.org","subject":"Re: \"mailinfo: Remove only one set of square brackets\" considered harmful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-15T22:31:12Z","receivedAt":"2009-07-15T22:31:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n>\n>> So I see why Andreas did it, and I don't disagree violently, BUT...\n>>\n>> The fact is, we have mailing lists etc that add their own headers to the \n>> subject, and they know they can add things in brackets. The most obvious \n>> example is the Linux kernel security list, which adds a prefix of\n>>\n>> \t\"[Security] \"\n>>\n>> to the subject line in order to stand out (I'm on other lists that do \n>> this too, but those generally don'thave patches).\n>>>\n>> So I have emails witgh subjects like\n>>\n>> \tSubject: [Security] [patch] random: make get_random_int() more random\n>>\n>> but I also have people who do the same thing themselves, eg:\n>>\n>> \tSubject: [PATCH -rc] [BUGFIX] x86: fix kernel_trap_sp()\n>> \tSubject: [BUGFIX][PATCH] fix bad page removal from LRU (Was Re: [RFC][PATCH] ..\n>>\n>> so people did kind of depend on the \"remove square brackets\" behavior.\n>\n> Thanks.  The reason why I have merged some questionable stuff (including\n> this one) early in this cycle was exactly because we would want to catch\n> real world breakages caused by such changes.\n>\n> Even though it is silly not to rely on already well established\n> conventions such as X-Mailing-List and List-ID but instead to waste\n> precious real estate at the initial part of the Subject in this century\n> merely for list identification purposes, this change regresses the end\n> result.\n\nI've reverted Andreas's patch for now, but it may not be a bad idea to\nresurrect it like this with an option.\n\n Documentation/git-mailinfo.txt |    7 +++++-\n builtin-mailinfo.c             |   49 +++++++++++++++++++++++----------------\n 2 files changed, 35 insertions(+), 21 deletions(-)\n\ndiff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt\nindex 8d95aaa..d800aea 100644\n--- a/Documentation/git-mailinfo.txt\n+++ b/Documentation/git-mailinfo.txt\n@@ -8,7 +8,7 @@ git-mailinfo - Extracts patch and authorship from a single e-mail message\n \n SYNOPSIS\n --------\n-'git mailinfo' [-k] [-u | --encoding=<encoding> | -n] <msg> <patch>\n+'git mailinfo' [-k|-b] [-u | --encoding=<encoding> | -n] <msg> <patch>\n \n \n DESCRIPTION\n@@ -32,6 +32,11 @@ OPTIONS\n \tmunging, and is most useful when used to read back\n \t'git-format-patch -k' output.\n \n+-b::\n+\tWhen -k is not in effect, all leading strings bracketed with '['\n+\tand ']' pairs are stripped.  This option limits the stripping to\n+\tonly the pairs whose bracketed string contains the word \"PATCH\".\n+\n -u::\n \tThe commit log message, author name and author email are\n \ttaken from the e-mail, and after minimally decoding MIME\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex 92637ac..a5949e2 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -10,6 +10,7 @@\n static FILE *cmitmsg, *patchfile, *fin, *fout;\n \n static int keep_subject;\n+static int keep_non_patch_brackets_in_subject;\n static const char *metainfo_charset;\n static struct strbuf line = STRBUF_INIT;\n static struct strbuf name = STRBUF_INIT;\n@@ -219,35 +220,41 @@ static int is_multipart_boundary(const struct strbuf *line)\n \n static void cleanup_subject(struct strbuf *subject)\n {\n-\tchar *pos;\n-\tsize_t remove;\n-\twhile (subject->len) {\n-\t\tswitch (*subject->buf) {\n+\tsize_t at = 0;\n+\n+\twhile (at < subject->len) {\n+\t\tchar *pos;\n+\t\tsize_t remove;\n+\n+\t\tswitch (subject->buf[at]) {\n \t\tcase 'r': case 'R':\n-\t\t\tif (subject->len <= 3)\n+\t\t\tif (subject->len <= at + 3)\n \t\t\t\tbreak;\n-\t\t\tif (!memcmp(subject->buf + 1, \"e:\", 2)) {\n-\t\t\t\tstrbuf_remove(subject, 0, 3);\n+\t\t\tif (!memcmp(subject->buf + at + 1, \"e:\", 2)) {\n+\t\t\t\tstrbuf_remove(subject, at, 3);\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tat++;\n \t\t\tbreak;\n \t\tcase ' ': case '\\t': case ':':\n-\t\t\tstrbuf_remove(subject, 0, 1);\n+\t\t\tstrbuf_remove(subject, at, 1);\n \t\t\tcontinue;\n \t\tcase '[':\n-\t\t\tif ((pos = strchr(subject->buf, ']'))) {\n-\t\t\t\tremove = pos - subject->buf;\n-\t\t\t\tif (remove <= (subject->len - remove) * 2) {\n-\t\t\t\t\tstrbuf_remove(subject, 0, remove + 1);\n-\t\t\t\t\tcontinue;\n-\t\t\t\t}\n-\t\t\t} else\n-\t\t\t\tstrbuf_remove(subject, 0, 1);\n-\t\t\tbreak;\n+\t\t\tpos = strchr(subject->buf + at, ']');\n+\t\t\tif (!pos)\n+\t\t\t\tbreak;\n+\t\t\tremove = pos - subject->buf + at + 1;\n+\t\t\tif (!keep_non_patch_brackets_in_subject ||\n+\t\t\t    (7 <= remove &&\n+\t\t\t     memmem(subject->buf + at, remove, \"PATCH\", 5)))\n+\t\t\t\tstrbuf_remove(subject, at, remove);\n+\t\t\telse\n+\t\t\t\tat += remove;\n+\t\t\tcontinue;\n \t\t}\n-\t\tstrbuf_trim(subject);\n-\t\treturn;\n+\t\tbreak;\n \t}\n+\tstrbuf_trim(subject);\n }\n \n static void cleanup_space(struct strbuf *sb)\n@@ -931,7 +938,7 @@ static int mailinfo(FILE *in, FILE *out, int ks, const char *encoding,\n }\n \n static const char mailinfo_usage[] =\n-\t\"git mailinfo [-k] [-u | --encoding=<encoding> | -n] msg patch <mail >info\";\n+\t\"git mailinfo [-k|-b] [-u | --encoding=<encoding> | -n] msg patch <mail >info\";\n \n int cmd_mailinfo(int argc, const char **argv, const char *prefix)\n {\n@@ -948,6 +955,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)\n \twhile (1 < argc && argv[1][0] == '-') {\n \t\tif (!strcmp(argv[1], \"-k\"))\n \t\t\tkeep_subject = 1;\n+\t\telse if (!strcmp(argv[1], \"-b\"))\n+\t\t\tkeep_non_patch_brackets_in_subject = 1;\n \t\telse if (!strcmp(argv[1], \"-u\"))\n \t\t\tmetainfo_charset = def_charset;\n \t\telse if (!strcmp(argv[1], \"-n\"))\n"},{"id":"118052","messageId":"alpine.LFD.2.01.0907151520380.13838@localhost.localdomain","threadId":"20122","inReplyTo":"7vljmpegy4.fsf@alter.siamese.dyndns.org","subject":"Re: \"mailinfo: Remove only one set of square brackets\" considered harmful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-15T22:31:44Z","receivedAt":"2009-07-15T22:31:44Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 15 Jul 2009, Junio C Hamano wrote:\n> \n> Even though it is silly not to rely on already well established\n> conventions such as X-Mailing-List and List-ID but instead to waste\n> precious real estate at the initial part of the Subject in this century\n> merely for list identification purposes, this change regresses the end\n> result.\n\nNote that at least for the security list, it's not about \"list \nidentification\", but simply to make the thing stand out in peoples \nmailboxes.\n\nBut I do agree that it's not perfect, and I would like to make the bracket \nremoval stricter. Andreas' patch was a step in that direction. But \ndifferent users would probably have different requirements.\n\nFor example, from a strictly git tools perspective, it's not even \"any \nsquare bracket\", and some people might want to remove only a single \nbracketed level, and only if it starts with \"[PATCH\".\n\nFor other uses, we want to remove the \"Re: \" at the beginning (and some \ncrazy email readers use language/locale-specific versions like \"Vs:\" for \nthat), and examples like the above \"[Security]\" from the security list.\n\nSo maybe the right solution is to allow people to configure this somehow.\n\n\t\tLinus\n"}]}