{"thread":{"id":"21735","subject":"[PATCH] mailinfo: remove [PATCH...] prefix from Subject regardless of length","startedAt":"2009-11-24T10:58:07Z","lastAt":"2009-11-27T21:11:31Z","messageCount":6,"participants":["Jim Meyering","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"128242","messageId":"874ookp4u8.fsf@meyering.net","threadId":"21735","inReplyTo":null,"subject":"[PATCH] mailinfo: remove [PATCH...] prefix from Subject regardless of length","fromName":"Jim Meyering","fromEmail":"meyering@redhat.com","sentAt":"2009-11-24T10:58:07Z","receivedAt":"2009-11-24T10:58:07Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\nBefore this change, a [...] prefix would be removed only as long as\nits length did not exceed 2/3 of the subject length. Now, when the\nbracketed quantity starts with PATCH, it is removed unconditionally.\nOtherwise, the existing behavior remains unchanged.\n\nWhile with a bare \"PATCH M/N\" prefix, this inconsistency shows up only\non a subject of length 2 or 1 (assuming one-digit M and N), if you set\nformat.subjectprefix to the name of a project or sub-project, the\nminimum affected length may be substantially larger.\n\nContrast the behavior before:\n\n for i in 1234 123 12 1 ''; do echo 'Subject: [PATCH 1/1] '$i \\\n | git mailinfo m p | head -1; done\n Subject: 1234\n Subject: 123\n Subject: [PATCH 1/1] 12\n Subject: [PATCH 1/1] 1\n Subject: [PATCH 1/1]\n\nand after this change:\n\n for i in 1234 123 12 1 ''; do echo 'Subject: [PATCH 1/1] '$i \\\n | ./git mailinfo m p | head -1; done\n Subject: 1234\n Subject: 123\n Subject: 12\n Subject: 1\n Subject:\n\nAlong the way, I added a \"const\" to indicate that the \"header\"\narray itself is constant.\n\nSigned-off-by: Jim Meyering \n---\n builtin-mailinfo.c | 10 ++++++++--\n t/t5100-mailinfo.sh | 9 +++++++++\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex 3c4f075..f07bca6 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -237,9 +237,15 @@ static void cleanup_subject(struct strbuf *subject)\n \t\t\tstrbuf_remove(subject, 0, 1);\n \t\t\tcontinue;\n \t\tcase '[':\n+\t\t\t/* If there's a [...] enclosed prefix, always remove\n+\t\t\t it when it starts with \"PATCH\". If it does not\n+\t\t\t start with PATCH, remove the bracketed quantity\n+\t\t\t only as long as that removes no more than 2/3 of\n+\t\t\t the length. */\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\tif (remove <= (subject->len - remove) * 2\n+\t\t\t\t || !prefixcmp (subject->buf + 1, \"PATCH\")) {\n \t\t\t\t\tstrbuf_remove(subject, 0, remove + 1);\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n@@ -265,7 +271,7 @@ static void cleanup_space(struct strbuf *sb)\n }\n\n static void decode_header(struct strbuf *line);\n-static const char *header[MAX_HDR_PARSED] = {\n+static const char *const header[MAX_HDR_PARSED] = {\n \t\"From\",\"Subject\",\"Date\",\n };\n\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex ebc36c1..86fb116 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -89,4 +89,13 @@ test_expect_success 'mailinfo on from header without name works' '\n\n '\n\n+test_expect_success 'mailinfo strips [PATCH... even on very short Subject' '\n+\n+\tprintf \"Subject: [PATCH 1/1] ..\\n\" > in &&\n+\tprintf \"Subject: ..\\n\\n\" > expect &&\n+\tgit mailinfo /dev/null /dev/null < in > out &&\n+\ttest_cmp expect out\n+\n+'\n+\n test_done\n--\n1.6.6.rc0.236.ge0b94\n"},{"id":"128272","messageId":"7vmy2b76ji.fsf@alter.siamese.dyndns.org","threadId":"21735","inReplyTo":"874ookp4u8.fsf@meyering.net","subject":"Re: [PATCH] mailinfo: remove [PATCH...] prefix from Subject regardless of length","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-25T01:10:57Z","receivedAt":"2009-11-25T01:10:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> Before this change, a [...] prefix would be removed only as long as\n> its length did not exceed 2/3 of the subject length. Now, when the\n> bracketed quantity starts with PATCH, it is removed unconditionally.\n> Otherwise, the existing behavior remains unchanged.\n\nThanks, I think this is a good idea in general, but have two comments.\n\n - I am not sure how this should play with 17635fc (mailinfo: -b option\n keeps [bracketed] strings that is not a [PATCH] marker, 2009-07-15).\n\n - Regardless of interaction with 17635fc, Things like [RFC PATCH]\n [SECURITY PATCH] might want a similar treatment.\n"},{"id":"128286","messageId":"87638zm38r.fsf_-_@meyering.net","threadId":"21735","inReplyTo":"7vmy2b76ji.fsf@alter.siamese.dyndns.org","subject":"[PATCH] git-am: don't ignore --keep (-k) option","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-11-25T08:13:08Z","receivedAt":"2009-11-25T08:13:08Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering writes:\n>> Before this change, a [...] prefix would be removed only as long as\n>> its length did not exceed 2/3 of the subject length. Now, when the\n>> bracketed quantity starts with PATCH, it is removed unconditionally.\n>> Otherwise, the existing behavior remains unchanged.\n>\n> Thanks, I think this is a good idea in general, but have two comments.\n>\n> - I am not sure how this should play with 17635fc (mailinfo: -b option\n> keeps [bracketed] strings that is not a [PATCH] marker, 2009-07-15).\n\nAh ha! I see you've already scratched this itch,\nand more thoroughly, to boot. Also, I prefer your\nremoval of the hard-to-describe 2/3 threshold.\n\n> - Regardless of interaction with 17635fc, Things like [RFC PATCH]\n> [SECURITY PATCH] might want a similar treatment.\n\nAs your patch does.\n\nI started looking at git-am.sh and spotted what appears to be a typo.\nThere is only that one use of $keep_subject, so its value currently\ncomes from the environment.\n\n>From 02f7e6433b5db8b18a4cccf58c302159c2f54fa5 Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Wed, 25 Nov 2009 09:10:46 +0100\nSubject: [PATCH] git-am: don't ignore --keep (-k) option\n\nFix typo in variable name: s/keep_subject/keep/.\n\nSigned-off-by: Jim Meyering \n---\n git-am.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 151512a..f353e73 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -578,7 +578,7 @@ do\n \t\t\tsed -e '1,/^$/d' >\"$dotest/msg-clean\"\n \t\telse\n \t\t\tSUBJECT=\"$(sed -n '/^Subject/ s/Subject: //p' \"$dotest/info\")\"\n-\t\t\tcase \"$keep_subject\" in -k) SUBJECT=\"[PATCH] $SUBJECT\" ;; esac\n+\t\t\tcase \"$keep\" in -k) SUBJECT=\"[PATCH] $SUBJECT\" ;; esac\n\n \t\t\t(printf '%s\\n\\n' \"$SUBJECT\"; cat \"$dotest/msg\") |\n \t\t\t\tgit stripspace > \"$dotest/msg-clean\"\n--\n1.6.6.rc0.236.ge0b94\n"},{"id":"128597","messageId":"7vy6lrka69.fsf@alter.siamese.dyndns.org","threadId":"21735","inReplyTo":"87638zm38r.fsf_-_@meyering.net","subject":"Re: [PATCH] git-am: don't ignore --keep (-k) option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-27T20:03:10Z","receivedAt":"2009-11-27T20:03:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> I started looking at git-am.sh and spotted what appears to be a typo.\n> There is only that one use of $keep_subject, so its value currently\n> comes from the environment.\n>\n> From 02f7e6433b5db8b18a4cccf58c302159c2f54fa5 Mon Sep 17 00:00:00 2001\n> From: Jim Meyering \n> Date: Wed, 25 Nov 2009 09:10:46 +0100\n> Subject: [PATCH] git-am: don't ignore --keep (-k) option\n>\n> Fix typo in variable name: s/keep_subject/keep/.\n>\n> Signed-off-by: Jim Meyering \n\nAt the level of \"what does each line of the code do\", this is a fix, but\nas we do a lot more than just stripping \"[PATCH] \" from the beginning of\nthe Subject: line these days, I think we are better off declaring defeat\nin this particular codepath and not doing anything here.\n\nAdding \"[PATCH] \" is no longer \"keeping the original subject\" anyway. It\nis \"without knowing what we already stripped, adding one random string\nthat could have been what we removed\".\n\nI also have to wonder why $dotest/info does not have the [PATCH] or\nwhatever prefix that we were told not to strip in this codepath. After\nall, we are running \"git mailinfo\" with $keep option to produce that file,\nso if that part is working correctly, we shouldn't even have to have this\n\"add [PATCH] back\" trick to begin with.\n\nWhat am I missing???\n\n> ---\n> git-am.sh | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-am.sh b/git-am.sh\n> index 151512a..f353e73 100755\n> --- a/git-am.sh\n> +++ b/git-am.sh\n> @@ -578,7 +578,7 @@ do\n> \t\t\tsed -e '1,/^$/d' >\"$dotest/msg-clean\"\n> \t\telse\n> \t\t\tSUBJECT=\"$(sed -n '/^Subject/ s/Subject: //p' \"$dotest/info\")\"\n> -\t\t\tcase \"$keep_subject\" in -k) SUBJECT=\"[PATCH] $SUBJECT\" ;; esac\n> +\t\t\tcase \"$keep\" in -k) SUBJECT=\"[PATCH] $SUBJECT\" ;; esac\n>\n> \t\t\t(printf '%s\\n\\n' \"$SUBJECT\"; cat \"$dotest/msg\") |\n> \t\t\t\tgit stripspace > \"$dotest/msg-clean\"\n> --\n> 1.6.6.rc0.236.ge0b94\n"},{"id":"128600","messageId":"87y6lr203s.fsf@meyering.net","threadId":"21735","inReplyTo":"7vy6lrka69.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-am: don't ignore --keep (-k) option","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-11-27T20:17:59Z","receivedAt":"2009-11-27T20:17:59Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Jim Meyering writes:\n>> I started looking at git-am.sh and spotted what appears to be a typo.\n>> There is only that one use of $keep_subject, so its value currently\n>> comes from the environment.\n>>\n>> From 02f7e6433b5db8b18a4cccf58c302159c2f54fa5 Mon Sep 17 00:00:00 2001\n>> From: Jim Meyering \n>> Date: Wed, 25 Nov 2009 09:10:46 +0100\n>> Subject: [PATCH] git-am: don't ignore --keep (-k) option\n>>\n>> Fix typo in variable name: s/keep_subject/keep/.\n>>\n>> Signed-off-by: Jim Meyering \n>\n> At the level of \"what does each line of the code do\", this is a fix, but\n> as we do a lot more than just stripping \"[PATCH] \" from the beginning of\n> the Subject: line these days, I think we are better off declaring defeat\n> in this particular codepath and not doing anything here.\n\nSounds fine to me.\nGlad you're keeping everything in perspective.\n"},{"id":"128607","messageId":"7vfx7zk70c.fsf@alter.siamese.dyndns.org","threadId":"21735","inReplyTo":"87y6lr203s.fsf@meyering.net","subject":"Re: [PATCH] git-am: don't ignore --keep (-k) option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-27T21:11:31Z","receivedAt":"2009-11-27T21:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n>> At the level of \"what does each line of the code do\", this is a fix, but\n>> as we do a lot more than just stripping \"[PATCH] \" from the beginning of\n>> the Subject: line these days, I think we are better off declaring defeat\n>> in this particular codepath and not doing anything here.\n>\n> Sounds fine to me.\n> Glad you're keeping everything in perspective.\n\nWhat the case statement tries to do is _wrong_; \"mailinfo -k\" keeps the\noriginal prefix and all the case statement does is to add an extra [PATCH]\nthat did not exist anywhere in the original on top of that.\n\nWhat is funny is that the case statement has been trying to do a wrong\nthing from day-one, ever since the script was introduced in d1c5f2a (Add\ngit-am, applymbox replacement., 2005-10-07). That version uses $keep to\nhold -k or empty, gives that to mailinfo for producing $dotest/info, and\nit has the same case statement that switches on $keep_subject nobody sets\nto add an extra \"[PATCH]\" in front. Luckily, due to the typo you found,\nnobody was bitten by the bug, and your patch will break things for people\nby enabling it ;-).\n\nThanks for noticing this one. It began an innocent bug nobody noticed,\nbut it is embarrassing that we carefully _maintained_ that code nobody\ntriggers for four years.\n"}]}