# [PATCH] mailinfo: remove [PATCH...] prefix from Subject regardless of length 6 messages from 2009-11-24 to 2009-11-27. Participants: Jim Meyering, Junio C Hamano. Thread: https://gitlist.dev/t/21735 ## Jim Meyering, 2009-11-24 10:58 Subject: [PATCH] mailinfo: remove [PATCH...] prefix from Subject regardless of length Message-ID: <874ookp4u8.fsf@meyering.net> URL: https://gitlist.dev/e/874ookp4u8.fsf%40meyering.net ``` Before this change, a [...] prefix would be removed only as long as its length did not exceed 2/3 of the subject length. Now, when the bracketed quantity starts with PATCH, it is removed unconditionally. Otherwise, the existing behavior remains unchanged. While with a bare "PATCH M/N" prefix, this inconsistency shows up only on a subject of length 2 or 1 (assuming one-digit M and N), if you set format.subjectprefix to the name of a project or sub-project, the minimum affected length may be substantially larger. Contrast the behavior before: for i in 1234 123 12 1 ''; do echo 'Subject: [PATCH 1/1] '$i \ | git mailinfo m p | head -1; done Subject: 1234 Subject: 123 Subject: [PATCH 1/1] 12 Subject: [PATCH 1/1] 1 Subject: [PATCH 1/1] and after this change: for i in 1234 123 12 1 ''; do echo 'Subject: [PATCH 1/1] '$i \ | ./git mailinfo m p | head -1; done Subject: 1234 Subject: 123 Subject: 12 Subject: 1 Subject: Along the way, I added a "const" to indicate that the "header" array itself is constant. Signed-off-by: Jim Meyering --- builtin-mailinfo.c | 10 ++++++++-- t/t5100-mailinfo.sh | 9 +++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c index 3c4f075..f07bca6 100644 --- a/builtin-mailinfo.c +++ b/builtin-mailinfo.c @@ -237,9 +237,15 @@ static void cleanup_subject(struct strbuf *subject) strbuf_remove(subject, 0, 1); continue; case '[': + /* If there's a [...] enclosed prefix, always remove + it when it starts with "PATCH". If it does not + start with PATCH, remove the bracketed quantity + only as long as that removes no more than 2/3 of + the length. */ if ((pos = strchr(subject->buf, ']'))) { remove = pos - subject->buf; - if (remove <= (subject->len - remove) * 2) { + if (remove <= (subject->len - remove) * 2 + || !prefixcmp (subject->buf + 1, "PATCH")) { strbuf_remove(subject, 0, remove + 1); continue; } @@ -265,7 +271,7 @@ static void cleanup_space(struct strbuf *sb) } static void decode_header(struct strbuf *line); -static const char *header[MAX_HDR_PARSED] = { +static const char *const header[MAX_HDR_PARSED] = { "From","Subject","Date", }; diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh index ebc36c1..86fb116 100755 --- a/t/t5100-mailinfo.sh +++ b/t/t5100-mailinfo.sh @@ -89,4 +89,13 @@ test_expect_success 'mailinfo on from header without name works' ' ' +test_expect_success 'mailinfo strips [PATCH... even on very short Subject' ' + + printf "Subject: [PATCH 1/1] ..\n" > in && + printf "Subject: ..\n\n" > expect && + git mailinfo /dev/null /dev/null < in > out && + test_cmp expect out + +' + test_done -- 1.6.6.rc0.236.ge0b94 ``` ## Junio C Hamano, 2009-11-25 01:10 Subject: Re: [PATCH] mailinfo: remove [PATCH...] prefix from Subject regardless of length Message-ID: <7vmy2b76ji.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmy2b76ji.fsf%40alter.siamese.dyndns.org In-Reply-To: <874ookp4u8.fsf@meyering.net> ``` Jim Meyering writes: > Before this change, a [...] prefix would be removed only as long as > its length did not exceed 2/3 of the subject length. Now, when the > bracketed quantity starts with PATCH, it is removed unconditionally. > Otherwise, the existing behavior remains unchanged. Thanks, I think this is a good idea in general, but have two comments. - I am not sure how this should play with 17635fc (mailinfo: -b option keeps [bracketed] strings that is not a [PATCH] marker, 2009-07-15). - Regardless of interaction with 17635fc, Things like [RFC PATCH] [SECURITY PATCH] might want a similar treatment. ``` ## Jim Meyering, 2009-11-25 08:13 Subject: [PATCH] git-am: don't ignore --keep (-k) option Message-ID: <87638zm38r.fsf_-_@meyering.net> URL: https://gitlist.dev/e/87638zm38r.fsf_-_%40meyering.net In-Reply-To: <7vmy2b76ji.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > Jim Meyering writes: >> Before this change, a [...] prefix would be removed only as long as >> its length did not exceed 2/3 of the subject length. Now, when the >> bracketed quantity starts with PATCH, it is removed unconditionally. >> Otherwise, the existing behavior remains unchanged. > > Thanks, I think this is a good idea in general, but have two comments. > > - I am not sure how this should play with 17635fc (mailinfo: -b option > keeps [bracketed] strings that is not a [PATCH] marker, 2009-07-15). Ah ha! I see you've already scratched this itch, and more thoroughly, to boot. Also, I prefer your removal of the hard-to-describe 2/3 threshold. > - Regardless of interaction with 17635fc, Things like [RFC PATCH] > [SECURITY PATCH] might want a similar treatment. As your patch does. I started looking at git-am.sh and spotted what appears to be a typo. There is only that one use of $keep_subject, so its value currently comes from the environment. >From 02f7e6433b5db8b18a4cccf58c302159c2f54fa5 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Wed, 25 Nov 2009 09:10:46 +0100 Subject: [PATCH] git-am: don't ignore --keep (-k) option Fix typo in variable name: s/keep_subject/keep/. Signed-off-by: Jim Meyering --- git-am.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/git-am.sh b/git-am.sh index 151512a..f353e73 100755 --- a/git-am.sh +++ b/git-am.sh @@ -578,7 +578,7 @@ do sed -e '1,/^$/d' >"$dotest/msg-clean" else SUBJECT="$(sed -n '/^Subject/ s/Subject: //p' "$dotest/info")" - case "$keep_subject" in -k) SUBJECT="[PATCH] $SUBJECT" ;; esac + case "$keep" in -k) SUBJECT="[PATCH] $SUBJECT" ;; esac (printf '%s\n\n' "$SUBJECT"; cat "$dotest/msg") | git stripspace > "$dotest/msg-clean" -- 1.6.6.rc0.236.ge0b94 ``` ## Junio C Hamano, 2009-11-27 20:03 Subject: Re: [PATCH] git-am: don't ignore --keep (-k) option Message-ID: <7vy6lrka69.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy6lrka69.fsf%40alter.siamese.dyndns.org In-Reply-To: <87638zm38r.fsf_-_@meyering.net> ``` Jim Meyering writes: > I started looking at git-am.sh and spotted what appears to be a typo. > There is only that one use of $keep_subject, so its value currently > comes from the environment. > > From 02f7e6433b5db8b18a4cccf58c302159c2f54fa5 Mon Sep 17 00:00:00 2001 > From: Jim Meyering > Date: Wed, 25 Nov 2009 09:10:46 +0100 > Subject: [PATCH] git-am: don't ignore --keep (-k) option > > Fix typo in variable name: s/keep_subject/keep/. > > Signed-off-by: Jim Meyering At the level of "what does each line of the code do", this is a fix, but as we do a lot more than just stripping "[PATCH] " from the beginning of the Subject: line these days, I think we are better off declaring defeat in this particular codepath and not doing anything here. Adding "[PATCH] " is no longer "keeping the original subject" anyway. It is "without knowing what we already stripped, adding one random string that could have been what we removed". I also have to wonder why $dotest/info does not have the [PATCH] or whatever prefix that we were told not to strip in this codepath. After all, we are running "git mailinfo" with $keep option to produce that file, so if that part is working correctly, we shouldn't even have to have this "add [PATCH] back" trick to begin with. What am I missing??? > --- > git-am.sh | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/git-am.sh b/git-am.sh > index 151512a..f353e73 100755 > --- a/git-am.sh > +++ b/git-am.sh > @@ -578,7 +578,7 @@ do > sed -e '1,/^$/d' >"$dotest/msg-clean" > else > SUBJECT="$(sed -n '/^Subject/ s/Subject: //p' "$dotest/info")" > - case "$keep_subject" in -k) SUBJECT="[PATCH] $SUBJECT" ;; esac > + case "$keep" in -k) SUBJECT="[PATCH] $SUBJECT" ;; esac > > (printf '%s\n\n' "$SUBJECT"; cat "$dotest/msg") | > git stripspace > "$dotest/msg-clean" > -- > 1.6.6.rc0.236.ge0b94 ``` ## Jim Meyering, 2009-11-27 20:17 Subject: Re: [PATCH] git-am: don't ignore --keep (-k) option Message-ID: <87y6lr203s.fsf@meyering.net> URL: https://gitlist.dev/e/87y6lr203s.fsf%40meyering.net In-Reply-To: <7vy6lrka69.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > Jim Meyering writes: >> I started looking at git-am.sh and spotted what appears to be a typo. >> There is only that one use of $keep_subject, so its value currently >> comes from the environment. >> >> From 02f7e6433b5db8b18a4cccf58c302159c2f54fa5 Mon Sep 17 00:00:00 2001 >> From: Jim Meyering >> Date: Wed, 25 Nov 2009 09:10:46 +0100 >> Subject: [PATCH] git-am: don't ignore --keep (-k) option >> >> Fix typo in variable name: s/keep_subject/keep/. >> >> Signed-off-by: Jim Meyering > > At the level of "what does each line of the code do", this is a fix, but > as we do a lot more than just stripping "[PATCH] " from the beginning of > the Subject: line these days, I think we are better off declaring defeat > in this particular codepath and not doing anything here. Sounds fine to me. Glad you're keeping everything in perspective. ``` ## Junio C Hamano, 2009-11-27 21:11 Subject: Re: [PATCH] git-am: don't ignore --keep (-k) option Message-ID: <7vfx7zk70c.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfx7zk70c.fsf%40alter.siamese.dyndns.org In-Reply-To: <87y6lr203s.fsf@meyering.net> ``` Jim Meyering writes: >> At the level of "what does each line of the code do", this is a fix, but >> as we do a lot more than just stripping "[PATCH] " from the beginning of >> the Subject: line these days, I think we are better off declaring defeat >> in this particular codepath and not doing anything here. > > Sounds fine to me. > Glad you're keeping everything in perspective. What the case statement tries to do is _wrong_; "mailinfo -k" keeps the original prefix and all the case statement does is to add an extra [PATCH] that did not exist anywhere in the original on top of that. What is funny is that the case statement has been trying to do a wrong thing from day-one, ever since the script was introduced in d1c5f2a (Add git-am, applymbox replacement., 2005-10-07). That version uses $keep to hold -k or empty, gives that to mailinfo for producing $dotest/info, and it has the same case statement that switches on $keep_subject nobody sets to add an extra "[PATCH]" in front. Luckily, due to the typo you found, nobody was bitten by the bug, and your patch will break things for people by enabling it ;-). Thanks for noticing this one. It began an innocent bug nobody noticed, but it is embarrassing that we carefully _maintained_ that code nobody triggers for four years. ```