[PATCH 1/3] mailinfo documentation: accurately describe non -k case

[PATCH 1/3] mailinfo documentation: accurately describe non -k case

[Thomas Rast]

Since its very first description of -k, the documentation for
git-mailinfo claimed that (in the case without -k) after cleaning up
bracketed strings [blah], it would insert [PATCH].

It doesn't; on the contrary, one of the important jobs of mailinfo is
to remove those strings.

Since we're already there, rewrite the paragraph to give a complete
enumeration of all the transformations.  Specifically, it was missing
the whitespace normalization (run of isspace(c) -> ' ') and the
removal of leading ':'.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/git-mailinfo.txt |   25 ++++++++++++++++++-------
 1 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt
index 51dc325..97e7a8e 100644
--- a/Documentation/git-mailinfo.txt
+++ b/Documentation/git-mailinfo.txt
@@ -25,13 +25,24 @@ command directly.  See linkgit:git-am[1] instead.
 OPTIONS
 -------
 -k::
-	Usually the program 'cleans up' the Subject: header line
-	to extract the title line for the commit log message,
-	among which (1) remove 'Re:' or 're:', (2) leading
-	whitespaces, (3) '[' up to ']', typically '[PATCH]', and
-	then prepends "[PATCH] ".  This flag forbids this
-	munging, and is most useful when used to read back
-	'git format-patch -k' output.
+	Usually the program removes email cruft from the Subject:
+	header line to extract the title line for the commit log
+	message.  This option prevents this munging, and is most
+	useful when used to read back 'git format-patch -k' output.
++
+Specifically, the following are removed until none of them remain:
++
+--
+*	Leading and trailing whitespace.
+
+*	Leading `Re:`, `re:`, and `:`.
+
+*	Leading bracketed strings (between `[` and `]`, usually
+	`[PATCH]`).
+--
++
+Finally, runs of whitespace are normalized to a single ASCII space
+character.
 
 -b::
 	When -k is not in effect, all leading strings bracketed with '['
-- 
1.7.9.rc0.168.g3847c

[PATCH 2/3] am: learn passing -b to mailinfo

[Thomas Rast]

git-am could pass -k to mailinfo, but not -b.  Introduce an option
that does so.  We change the meaning of the 'keep' state file, but are
careful not to cause a problem unless you downgrade in the middle of
an 'am' run.

This uncovers a bug in mailinfo -b, hence the failing test.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Someone on IRC asked about git-am support for passing through the -b
flag to mailinfo.  And so it began...

 Documentation/git-am.txt |    3 +++
 git-am.sh                |   11 +++++++----
 t/t4150-am.sh            |   14 ++++++++++++--
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 887466d..ee6cca2 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -40,6 +40,9 @@ OPTIONS
 --keep::
 	Pass `-k` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).
 
+--keep-non-patch::
+	Pass `-b` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).
+
 --keep-cr::
 --no-keep-cr::
 	With `--keep-cr`, call 'git mailsplit' (see linkgit:git-mailsplit[1])
diff --git a/git-am.sh b/git-am.sh
index 1c13b13..8dfad7a 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -15,6 +15,7 @@ q,quiet         be quiet
 s,signoff       add a Signed-off-by line to the commit message
 u,utf8          recode into utf8 (default)
 k,keep          pass -k flag to git-mailinfo
+keep-non-patch  pass -b flag to git-mailinfo
 keep-cr         pass --keep-cr flag to git-mailsplit for mbox format
 no-keep-cr      do not pass --keep-cr flag to git-mailsplit independent of am.keepcr
 c,scissors      strip everything before a scissors line
@@ -386,7 +387,9 @@ do
 	--no-utf8)
 		utf8= ;;
 	-k|--keep)
-		keep=t ;;
+		keep=-k ;;
+	--keep-non-patch)
+		keep=-b ;;
 	-c|--scissors)
 		scissors=t ;;
 	--no-scissors)
@@ -398,7 +401,7 @@ do
 	--abort)
 		abort=t ;;
 	--rebasing)
-		rebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;
+		rebasing=t threeway=t keep=-k scissors=f no_inbody_headers=t ;;
 	-d|--dotest)
 		die "$(gettext "-d option is no longer supported.  Do not use.")"
 		;;
@@ -571,8 +574,8 @@ then
 else
 	utf8=-n
 fi
-if test "$(cat "$dotest/keep")" = t
-then
+keep=$(cat "$dotest/keep")
+if test "$keep" = t
 	keep=-k
 fi
 case "$(cat "$dotest/scissors")" in
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index d7d9ccc..7e7c83c 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -237,7 +237,7 @@ test_expect_success 'am stays in branch' '
 
 test_expect_success 'am --signoff does not add Signed-off-by: line if already there' '
 	git format-patch --stdout HEAD^ >patch3 &&
-	sed -e "/^Subject/ s,\[PATCH,Re: Re: Re: & 1/5 v2," patch3 >patch4 &&
+	sed -e "/^Subject/ s,\[PATCH,Re: Re: Re: & 1/5 v2] [foo," patch3 >patch4 &&
 	rm -fr .git/rebase-apply &&
 	git reset --hard &&
 	git checkout HEAD^ &&
@@ -259,7 +259,17 @@ test_expect_success 'am --keep really keeps the subject' '
 	git am --keep patch4 &&
 	! test -d .git/rebase-apply &&
 	git cat-file commit HEAD >actual &&
-	grep "Re: Re: Re: \[PATCH 1/5 v2\] third" actual
+	grep "Re: Re: Re: \[PATCH 1/5 v2\] \[foo\] third" actual
+'
+
+test_expect_failure 'am --keep-non-patch really keeps the non-patch part' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
+	git checkout HEAD^ &&
+	git am --keep-non-patch patch4 &&
+	! test -d .git/rebase-apply &&
+	git cat-file commit HEAD >actual &&
+	grep "^\[foo\] third" actual
 '
 
 test_expect_success 'am -3 falls back to 3-way merge' '
-- 
1.7.9.rc0.168.g3847c

Re: [PATCH 2/3] am: learn passing -b to mailinfo

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> @@ -571,8 +574,8 @@ then
>  else
>  	utf8=-n
>  fi
> -if test "$(cat "$dotest/keep")" = t
> -then
> +keep=$(cat "$dotest/keep")
> +if test "$keep" = t
>  	keep=-k
>  fi

Curious.

Who writes 't' to $dotest/keep after this patch is applied?

I also do not want to worry about "echo" portability issues that may come
from an existing

	echo "$keep" >"$dotest/keep"

that this patch does not touch.

I suspect that this patch was not tested in a way to exercise this
codepath; shell would have barfed when seeing the lack of "then" here, no?

Re: [PATCH 2/3] am: learn passing -b to mailinfo

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@student.ethz.ch> writes:
>
>> @@ -571,8 +574,8 @@ then
>>  else
>>  	utf8=-n
>>  fi
>> -if test "$(cat "$dotest/keep")" = t
>> -then
>> +keep=$(cat "$dotest/keep")
>> +if test "$keep" = t
>>  	keep=-k
>>  fi
>
> Curious.
>
> Who writes 't' to $dotest/keep after this patch is applied?

Nobody; like the commit message says, I was just trying to help users
upgrading from one version to the next in the middle of an 'am', which
almost worked except for

> I suspect that this patch was not tested in a way to exercise this
> codepath; shell would have barfed when seeing the lack of "then" here, no?

(ouch)

> I also do not want to worry about "echo" portability issues that may come
> from an existing
>
> 	echo "$keep" >"$dotest/keep"
>
> that this patch does not touch.

Good point, thanks.  I'll reroll with printf.  Should I keep the
upgrade path compatibility?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[PATCH v2 1/2] am: learn passing -b to mailinfo

[Thomas Rast]

git-am could pass -k to mailinfo, but not -b.  Introduce an option
that does so.  We change the meaning of the 'keep' state file, but are
careful not to cause a problem unless you downgrade in the middle of
an 'am' run.

This uncovers a bug in mailinfo -b, hence the failing test.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

This fixes the broken 'if', and the use of 'echo' with an argument
that starts with '-'.

 Documentation/git-am.txt |    3 +++
 git-am.sh                |   12 ++++++++----
 t/t4150-am.sh            |   14 ++++++++++++--
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 887466d..ee6cca2 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -40,6 +40,9 @@ OPTIONS
 --keep::
 	Pass `-k` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).
 
+--keep-non-patch::
+	Pass `-b` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).
+
 --keep-cr::
 --no-keep-cr::
 	With `--keep-cr`, call 'git mailsplit' (see linkgit:git-mailsplit[1])
diff --git a/git-am.sh b/git-am.sh
index 1c13b13..b8adde7 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -15,6 +15,7 @@ q,quiet         be quiet
 s,signoff       add a Signed-off-by line to the commit message
 u,utf8          recode into utf8 (default)
 k,keep          pass -k flag to git-mailinfo
+keep-non-patch  pass -b flag to git-mailinfo
 keep-cr         pass --keep-cr flag to git-mailsplit for mbox format
 no-keep-cr      do not pass --keep-cr flag to git-mailsplit independent of am.keepcr
 c,scissors      strip everything before a scissors line
@@ -386,7 +387,9 @@ do
 	--no-utf8)
 		utf8= ;;
 	-k|--keep)
-		keep=t ;;
+		keep=-k ;;
+	--keep-non-patch)
+		keep=-b ;;
 	-c|--scissors)
 		scissors=t ;;
 	--no-scissors)
@@ -398,7 +401,7 @@ do
 	--abort)
 		abort=t ;;
 	--rebasing)
-		rebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;
+		rebasing=t threeway=t keep=-k scissors=f no_inbody_headers=t ;;
 	-d|--dotest)
 		die "$(gettext "-d option is no longer supported.  Do not use.")"
 		;;
@@ -529,7 +532,7 @@ else
 	echo "$threeway" >"$dotest/threeway"
 	echo "$sign" >"$dotest/sign"
 	echo "$utf8" >"$dotest/utf8"
-	echo "$keep" >"$dotest/keep"
+	printf "%s" "$keep" >"$dotest/keep"
 	echo "$scissors" >"$dotest/scissors"
 	echo "$no_inbody_headers" >"$dotest/no_inbody_headers"
 	echo "$GIT_QUIET" >"$dotest/quiet"
@@ -571,7 +574,8 @@ then
 else
 	utf8=-n
 fi
-if test "$(cat "$dotest/keep")" = t
+keep=$(cat "$dotest/keep")
+if test "$keep" = t
 then
 	keep=-k
 fi
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index d7d9ccc..7e7c83c 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -237,7 +237,7 @@ test_expect_success 'am stays in branch' '
 
 test_expect_success 'am --signoff does not add Signed-off-by: line if already there' '
 	git format-patch --stdout HEAD^ >patch3 &&
-	sed -e "/^Subject/ s,\[PATCH,Re: Re: Re: & 1/5 v2," patch3 >patch4 &&
+	sed -e "/^Subject/ s,\[PATCH,Re: Re: Re: & 1/5 v2] [foo," patch3 >patch4 &&
 	rm -fr .git/rebase-apply &&
 	git reset --hard &&
 	git checkout HEAD^ &&
@@ -259,7 +259,17 @@ test_expect_success 'am --keep really keeps the subject' '
 	git am --keep patch4 &&
 	! test -d .git/rebase-apply &&
 	git cat-file commit HEAD >actual &&
-	grep "Re: Re: Re: \[PATCH 1/5 v2\] third" actual
+	grep "Re: Re: Re: \[PATCH 1/5 v2\] \[foo\] third" actual
+'
+
+test_expect_failure 'am --keep-non-patch really keeps the non-patch part' '
+	rm -fr .git/rebase-apply &&
+	git reset --hard &&
+	git checkout HEAD^ &&
+	git am --keep-non-patch patch4 &&
+	! test -d .git/rebase-apply &&
+	git cat-file commit HEAD >actual &&
+	grep "^\[foo\] third" actual
 '
 
 test_expect_success 'am -3 falls back to 3-way merge' '
-- 
1.7.9.rc0.168.g3847c

[PATCH v2 2/2] mailinfo: with -b, keep space after [foo]

[Thomas Rast]

The logic for the -b mode, where [PATCH] is dropped but [foo] is not,
silently ate all spaces after the ].

Fix this by keeping the next isspace() character, if there is any.
Being more thorough is pointless, as the later cleanup_space() call
will normalize any sequence of whitespace to a single ' '.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Unchanged from last time.

 builtin/mailinfo.c |   11 ++++++++++-
 t/t4150-am.sh      |    2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index bfb32b7..eaf9e15 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -250,8 +250,17 @@ static void cleanup_subject(struct strbuf *subject)
 			    (7 <= remove &&
 			     memmem(subject->buf + at, remove, "PATCH", 5)))
 				strbuf_remove(subject, at, remove);
-			else
+			else {
 				at += remove;
+				/*
+				 * If the input had a space after the ], keep
+				 * it.  We don't bother with finding the end of
+				 * the space, since we later normalize it
+				 * anyway.
+				 */
+				if (isspace(subject->buf[at]))
+					at += 1;
+			}
 			continue;
 		}
 		break;
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 7e7c83c..8807b60 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -262,7 +262,7 @@ test_expect_success 'am --keep really keeps the subject' '
 	grep "Re: Re: Re: \[PATCH 1/5 v2\] \[foo\] third" actual
 '
 
-test_expect_failure 'am --keep-non-patch really keeps the non-patch part' '
+test_expect_success 'am --keep-non-patch really keeps the non-patch part' '
 	rm -fr .git/rebase-apply &&
 	git reset --hard &&
 	git checkout HEAD^ &&
-- 
1.7.9.rc0.168.g3847c

Re: [PATCH v2 1/2] am: learn passing -b to mailinfo

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> git-am could pass -k to mailinfo, but not -b.  Introduce an option
> that does so.  We change the meaning of the 'keep' state file, but are
> careful not to cause a problem unless you downgrade in the middle of
> an 'am' run.
>
> This uncovers a bug in mailinfo -b, hence the failing test.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>
> This fixes the broken 'if', and the use of 'echo' with an argument
> that starts with '-'.

After re-reading the code that parses the command line options given to
"am" and the previous invocation state we read from $dotest/*, however, I
think the way this change uses $keep makes things somewhat inconsistent
and harder to follow.

Currently the variables are given abstract meaning (e.g. "are we told to
record to utf8? yes or no") when we parse our command line options and
read from the previous invocation state, and then based on that abstract
meaning, a later code decides what exact option we throw at the git
commands we invoke (e.g. "utf8=t" -> "-u").

How about doing something like this instead at least for now?  It might be
better to decide when we parse our options and $dotest/* immediately what
options we give to the git commands we run (which your patch does but only
to $keep option), but that kind of change (1) belongs to a separate topic
and should be done consistently to all options, and (2) I am not convinced
if it is necessarily a good change.

Thanks.

diff --git a/git-am.sh b/git-am.sh
index 6cdd591..8b755d9 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -15,6 +15,7 @@ q,quiet         be quiet
 s,signoff       add a Signed-off-by line to the commit message
 u,utf8          recode into utf8 (default)
 k,keep          pass -k flag to git-mailinfo
+keep-non-patch  pass -b flag to git-mailinfo
 keep-cr         pass --keep-cr flag to git-mailsplit for mbox format
 no-keep-cr      do not pass --keep-cr flag to git-mailsplit independent of am.keepcr
 c,scissors      strip everything before a scissors line
@@ -345,6 +346,8 @@ do
 		utf8= ;;
 	-k|--keep)
 		keep=t ;;
+	--keep-non-patch)
+		keep=b ;;
 	-c|--scissors)
 		scissors=t ;;
 	--no-scissors)
@@ -522,16 +525,25 @@ case "$resolved" in
 	fi
 esac
 
+# Now, decide what command line options we will give to the git
+# commands we invoke, based on the result of parsing command line
+# options and previous invocation state stored in $dotest/ files.
+
 if test "$(cat "$dotest/utf8")" = t
 then
 	utf8=-u
 else
 	utf8=-n
 fi
-if test "$(cat "$dotest/keep")" = t
-then
-	keep=-k
-fi
+keep=$(cat "$dotest/keep")
+case "$keep" in
+t)
+	keep=-k ;;
+b)
+	keep=-b ;;
+*)
+	keep= ;;
+esac
 case "$(cat "$dotest/keepcr")" in
 t)
 	keepcr=--keep-cr ;;

Re: [PATCH v2 1/2] am: learn passing -b to mailinfo

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> After re-reading the code that parses the command line options given to
> "am" and the previous invocation state we read from $dotest/*, however, I
> think the way this change uses $keep makes things somewhat inconsistent
> and harder to follow.
>
> Currently the variables are given abstract meaning (e.g. "are we told to
> record to utf8? yes or no") when we parse our command line options and
> read from the previous invocation state, and then based on that abstract
> meaning, a later code decides what exact option we throw at the git
> commands we invoke (e.g. "utf8=t" -> "-u").
>
> How about doing something like this instead at least for now?  It might be
> better to decide when we parse our options and $dotest/* immediately what
> options we give to the git commands we run (which your patch does but only
> to $keep option), but that kind of change (1) belongs to a separate topic
> and should be done consistently to all options, and (2) I am not convinced
> if it is necessarily a good change.

Yes, at second glance it's probably better to remain consistent.  I
didn't like it at first because it's layering complexity on it, but you
are right, the existing code follows the same pattern.

> diff --git a/git-am.sh b/git-am.sh
> index 6cdd591..8b755d9 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -15,6 +15,7 @@ q,quiet         be quiet
>  s,signoff       add a Signed-off-by line to the commit message
>  u,utf8          recode into utf8 (default)
>  k,keep          pass -k flag to git-mailinfo
> +keep-non-patch  pass -b flag to git-mailinfo
>  keep-cr         pass --keep-cr flag to git-mailsplit for mbox format
>  no-keep-cr      do not pass --keep-cr flag to git-mailsplit independent of am.keepcr
>  c,scissors      strip everything before a scissors line
> @@ -345,6 +346,8 @@ do
>  		utf8= ;;
>  	-k|--keep)
>  		keep=t ;;
> +	--keep-non-patch)
> +		keep=b ;;
>  	-c|--scissors)
>  		scissors=t ;;
>  	--no-scissors)
> @@ -522,16 +525,25 @@ case "$resolved" in
>  	fi
>  esac
>  
> +# Now, decide what command line options we will give to the git
> +# commands we invoke, based on the result of parsing command line
> +# options and previous invocation state stored in $dotest/ files.
> +
>  if test "$(cat "$dotest/utf8")" = t
>  then
>  	utf8=-u
>  else
>  	utf8=-n
>  fi
> -if test "$(cat "$dotest/keep")" = t
> -then
> -	keep=-k
> -fi
> +keep=$(cat "$dotest/keep")
> +case "$keep" in
> +t)
> +	keep=-k ;;
> +b)
> +	keep=-b ;;
> +*)
> +	keep= ;;
> +esac
>  case "$(cat "$dotest/keepcr")" in
>  t)
>  	keepcr=--keep-cr ;;

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[PATCH 3/3] mailinfo: with -b, keep space after [foo]

[Thomas Rast]

The logic for the -b mode, where [PATCH] is dropped but [foo] is not,
silently ate all spaces after the ].

Fix this by keeping the next isspace() character, if there is any.
Being more thorough is pointless, as the later cleanup_space() call
will normalize any sequence of whitespace to a single ' '.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin/mailinfo.c |   11 ++++++++++-
 t/t4150-am.sh      |    2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index bfb32b7..eaf9e15 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -250,8 +250,17 @@ static void cleanup_subject(struct strbuf *subject)
 			    (7 <= remove &&
 			     memmem(subject->buf + at, remove, "PATCH", 5)))
 				strbuf_remove(subject, at, remove);
-			else
+			else {
 				at += remove;
+				/*
+				 * If the input had a space after the ], keep
+				 * it.  We don't bother with finding the end of
+				 * the space, since we later normalize it
+				 * anyway.
+				 */
+				if (isspace(subject->buf[at]))
+					at += 1;
+			}
 			continue;
 		}
 		break;
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 7e7c83c..8807b60 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -262,7 +262,7 @@ test_expect_success 'am --keep really keeps the subject' '
 	grep "Re: Re: Re: \[PATCH 1/5 v2\] \[foo\] third" actual
 '
 
-test_expect_failure 'am --keep-non-patch really keeps the non-patch part' '
+test_expect_success 'am --keep-non-patch really keeps the non-patch part' '
 	rm -fr .git/rebase-apply &&
 	git reset --hard &&
 	git checkout HEAD^ &&
-- 
1.7.9.rc0.168.g3847c