[PATCH] filter-branch: strip pgp signature in commit messages

df062010 (filter-branch: avoid passing commit message through sed)
introduced a regression when filtering gpg signed commits.  The gpgsig
header is multi-line and contains an empty line.  Although the signature
is indented, making the line a whitespace only line, this still results
in $header_line being empty, causing the “skip header lines” loop to
exit.

The rest of the commit object is then re-used as the rewritten commit
message, causing the new message to include the signature of the
original commit.

Signed-off-by: James McCoy <redacted>
---
 git-filter-branch.sh     | 14 +++++++++++---
 t/t7003-filter-branch.sh | 14 ++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5b3f63d..dd49b13 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh

@@ -347,10 +347,18 @@ while read commit parents; do
 	fi
 
 	{
-		while read -r header_line && test -n "$header_line"
+		while read -r header_line &&
+			( test -n "$header_line" || test -n "$gpg_signature" )
 		do
-			# skip header lines...
-			:;
+			# skip header lines... but track whether we are in a
+			# PGP signature, since it will have a whitespace only
+			# line which causes $header_line to be empty
+			if [ "${header_line#gpgsig}" != "$header_line" ]; then
+				gpg_signature=1
+			elif test -n "$gpg_signature" &&
+				expr "$header_line" : ".*END PGP" >/dev/null; then
+				gpg_signature=
+			fi
 		done
 		# and output the actual commit message
 		cat

diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 855afda..377c648 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh

@@ -2,6 +2,7 @@
 
 test_description='git filter-branch'
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
 
 test_expect_success 'setup' '
 	test_commit A &&

@@ -292,6 +293,19 @@ test_expect_success 'Tag name filtering strips gpg signature' '
 	test_cmp expect actual
 '
 
+test_expect_success GPG 'Filtering retains message of gpg signed commit' '
+	mkdir gpg &&
+	touch gpg/foo &&
+	git add gpg &&
+	test_tick &&
+	git commit -S -m "Adding gpg" &&
+
+	git log -1 --format="%s" > expect &&
+	git filter-branch -f --msg-filter "cat" &&
+	git log -1 --format="%s" > actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'Tag name filtering allows slashes in tag names' '
 	git tag -m tag-with-slash X/1 &&
 	git cat-file tag X/1 | sed -e s,X/1,X/2, > expect &&

-- 
2.6.1


-- 
James
GPG Key: 4096R/331BA3DB 2011-12-05 James McCoy <vega.james@gmail.com>

James McCoy venit, vidit, dixit 08.10.2015 07:01:

df062010 (filter-branch: avoid passing commit message through sed)
introduced a regression when filtering gpg signed commits.  The gpgsig
header is multi-line and contains an empty line.  Although the signature
is indented, making the line a whitespace only line, this still results
in $header_line being empty, causing the “skip header lines” loop to
exit.

The rest of the commit object is then re-used as the rewritten commit
message, causing the new message to include the signature of the
original commit.

I had to wrap my head around that commit message quite a bit and ended
up testing the issue myself. So the catch is:

df062010 (filter-branch: avoid passing commit message through sed)
introduced a regression when filtering gpg signed commits. As a
consequence, "filter-branch --msg-filter cat" (which should leave the
commit message unchanged) spills the signature (without the BEGIN line,
but with the END line) into (in front of) the original commit message.

The reason is that although...

... original commit.

Fix this by keeping track of the in/out-of signature state when parsing
the header lines.


[No, this does not alleviate my dislike for the commit signature
implementation, and I have not checked the patch - the test looks good
to me, though.]

Michael J Gruber venit, vidit, dixit 08.10.2015 10:15:

James McCoy venit, vidit, dixit 08.10.2015 07:01:

...

[No, this does not alleviate my dislike for the commit signature
implementation, and I have not checked the patch - the test looks good
to me, though.]

OK, now grumpy ol' Mike actually tested the patch with all our tests
that filter-branch something. All is good, and the new test catches the
regression when run without the patch.

I do think that the parser still has a problem that it had before
already: it does not distinguish between an empty line and an all white
space line (or else we didn't have a problem here at all).

In that sense, the patch is wrong, it does not correct the parser
deficiency. But it alleviates it for the special case of embedded
signatures, which currently is the only exceptional case that I am aware
of. It's not guaranteed to stay like that, of course. So maybe, one
should amend the commit message by saying that.

Michael

Michael J Gruber venit, vidit, dixit 08.10.2015 10:43:

Michael J Gruber venit, vidit, dixit 08.10.2015 10:15:

quoted

James McCoy venit, vidit, dixit 08.10.2015 07:01:

...

quoted

[No, this does not alleviate my dislike for the commit signature
implementation, and I have not checked the patch - the test looks good
to me, though.]

OK, now grumpy ol' Mike actually tested the patch with all our tests
that filter-branch something. All is good, and the new test catches the
regression when run without the patch.

I do think that the parser still has a problem that it had before
already: it does not distinguish between an empty line and an all white
space line (or else we didn't have a problem here at all).

In that sense, the patch is wrong, it does not correct the parser
deficiency. But it alleviates it for the special case of embedded
signatures, which currently is the only exceptional case that I am aware
of. It's not guaranteed to stay like that, of course. So maybe, one
should amend the commit message by saying that.

Michael

... or do the right thing:

diff --git i/git-filter-branch.sh w/git-filter-branch.sh
index 5777947..27c9c54 100755
--- i/git-filter-branch.sh
+++ w/git-filter-branch.sh

@@ -377,7 +377,7 @@ while read commit parents; do
 	fi

 	{
-		while read -r header_line && test -n "$header_line"
+		while IFS='' read -r header_line && test -n "$header_line"
 		do
 			# skip header lines...
 			:;

Not tested for POSIX etc., maybe we need a bare IFS inside a {} block
instead. In any case, we need to tell read not to split by words.

Michael

On Thu, Oct 08, 2015 at 12:00:54PM +0200, Michael J Gruber wrote:

Michael J Gruber venit, vidit, dixit 08.10.2015 10:43:

quoted

Michael J Gruber venit, vidit, dixit 08.10.2015 10:15:

quoted

James McCoy venit, vidit, dixit 08.10.2015 07:01:

...

quoted

[No, this does not alleviate my dislike for the commit signature
implementation, and I have not checked the patch - the test looks good
to me, though.]

OK, now grumpy ol' Mike actually tested the patch with all our tests
that filter-branch something. All is good, and the new test catches the
regression when run without the patch.

I do think that the parser still has a problem that it had before
already: it does not distinguish between an empty line and an all white
space line (or else we didn't have a problem here at all).

In that sense, the patch is wrong, it does not correct the parser
deficiency. But it alleviates it for the special case of embedded
signatures, which currently is the only exceptional case that I am aware
of. It's not guaranteed to stay like that, of course. So maybe, one
should amend the commit message by saying that.

Michael

... or do the right thing:

Indeed.  This fixes the actual problem of not consuming the entire
header, rather than the specific instance of the problem I encountered.

As far as I can tell, this should be fine in terms of POSIX.

Cheers,
-- 
James
GPG Key: 4096R/331BA3DB 2011-12-05 James McCoy [off-list ref]