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

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

[Jim Meyering]

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(-)

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

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

[Junio C Hamano]

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.

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

[Jim Meyering]

Junio C Hamano wrote:
> 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(-)

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

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

[Junio C Hamano]

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.

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

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

[Jim Meyering]

Junio C Hamano wrote:
> 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.

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

[Junio C Hamano]

Jim Meyering <jim@meyering.net> 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.