threads / patch / 21735

patchmailinfo: remove [PATCH...] prefix from Subject regardless of length

Subject: [PATCH] mailinfo: remove [PATCH...] prefix from Subject regardless of length

## tl;dr

6 messages between Nov 24, 2009 and Nov 27, 2009. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Jim Meyering· Nov 24, 2009, 10:58 UTC · lore

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 <meyering@redhat.com>
---
 builtin-mailinfo.c  |   10 ++++++++--
 t/t5100-mailinfo.sh |    9 +++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)
Show changes to 2 files +17 −3

builtin-mailinfo.c, t/t5100-mailinfo.sh

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· Nov 25, 2009, 01:10 UTC · re: Jim Meyering · lore

Re: [PATCH] mailinfo: remove [PATCH...] prefix from Subject regardless of length

Jim Meyering <meyering@redhat.com> 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· Nov 25, 2009, 08:13 UTC · re: Junio C Hamano · lore

[PATCH] git-am: don't ignore --keep (-k) option

Junio C Hamano wrote:
Show 10 quoted lines
> Jim Meyering <meyering@redhat.com> 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 <meyering@redhat.com>
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 <meyering@redhat.com>
---
 git-am.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-am.sh +1 −2
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· Nov 27, 2009, 20:03 UTC · re: Jim Meyering · lore

Re: [PATCH] git-am: don't ignore --keep (-k) option

Jim Meyering <jim@meyering.net> writes:
Show 12 quoted lines
> 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 <meyering@redhat.com>
> 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 <meyering@redhat.com>

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???
Show 19 quoted lines
> ---
>  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· Nov 27, 2009, 20:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-am: don't ignore --keep (-k) option

Junio C Hamano wrote:
Show 18 quoted lines
> Jim Meyering <jim@meyering.net> 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 <meyering@redhat.com>
>> 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 <meyering@redhat.com>
>
> 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· Nov 27, 2009, 21:11 UTC · re: Jim Meyering · lore

Re: [PATCH] git-am: don't ignore --keep (-k) option

Jim Meyering <jim@meyering.net> writes:
Show 7 quoted lines
>> 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.

← back to recent threads