[PATCH] docs: warn about possible '=' in clean/smudge filter process values

[PATCH] docs: warn about possible '=' in clean/smudge filter process values

[larsxschneider]

From: Lars Schneider <larsxschneider@gmail.com>

A pathname value in a clean/smudge filter process "key=value" pair can
contain the '=' character (introduced in edcc858). Make the user aware
of this issue in the docs, add a corresponding test case, and fix the
issue in filter process value parser of the example implementation in
contrib.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 Documentation/gitattributes.txt        |  4 +++-
 contrib/long-running-filter/example.pl |  8 ++++++--
 t/t0021-conversion.sh                  | 20 ++++++++++----------
 t/t0021/rot13-filter.pl                |  8 ++++++--
 4 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 976243a63e..e0b66c1220 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -435,7 +435,9 @@ to filter relative to the repository root. Right after the flush packet
 Git sends the content split in zero or more pkt-line packets and a
 flush packet to terminate content. Please note, that the filter
 must not send any response before it received the content and the
-final flush packet.
+final flush packet. Also note that the "value" of a "key=value" pair
+can contain the "=" character whereas the key would never contain
+that character.
 ------------------------
 packet:          git> command=smudge
 packet:          git> pathname=path/testfile.dat
diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
index 39457055a5..a677569ddd 100755
--- a/contrib/long-running-filter/example.pl
+++ b/contrib/long-running-filter/example.pl
@@ -81,8 +81,12 @@ packet_txt_write("capability=smudge");
 packet_flush();

 while (1) {
-	my ($command)  = packet_txt_read() =~ /^command=([^=]+)$/;
-	my ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;
+	my ($command)  = packet_txt_read() =~ /^command=(.+)$/;
+	my ($pathname) = packet_txt_read() =~ /^pathname=(.+)$/;
+
+	if ( $pathname eq "" ) {
+		die "bad pathname '$pathname'";
+	}

 	packet_bin_read();

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 4ea534e9fa..f3a0df2add 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -93,7 +93,7 @@ test_expect_success setup '
 	git checkout -- test test.t test.i &&

 	echo "content-test2" >test2.o &&
-	echo "content-test3 - filename with special characters" >"test3 '\''sq'\'',\$x.o"
+	echo "content-test3 - filename with special characters" >"test3 '\''sq'\'',\$x=.o"
 '

 script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
@@ -359,12 +359,12 @@ test_expect_success PERL 'required process filter should filter data' '
 		cp "$TEST_ROOT/test.o" test.r &&
 		cp "$TEST_ROOT/test2.o" test2.r &&
 		mkdir testsubdir &&
-		cp "$TEST_ROOT/test3 '\''sq'\'',\$x.o" "testsubdir/test3 '\''sq'\'',\$x.r" &&
+		cp "$TEST_ROOT/test3 '\''sq'\'',\$x=.o" "testsubdir/test3 '\''sq'\'',\$x=.r" &&
 		>test4-empty.r &&

 		S=$(file_size test.r) &&
 		S2=$(file_size test2.r) &&
-		S3=$(file_size "testsubdir/test3 '\''sq'\'',\$x.r") &&
+		S3=$(file_size "testsubdir/test3 '\''sq'\'',\$x=.r") &&

 		filter_git add . &&
 		cat >expected.log <<-EOF &&
@@ -373,7 +373,7 @@ test_expect_success PERL 'required process filter should filter data' '
 			IN: clean test.r $S [OK] -- OUT: $S . [OK]
 			IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
 			IN: clean test4-empty.r 0 [OK] -- OUT: 0  [OK]
-			IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+			IN: clean testsubdir/test3 '\''sq'\'',\$x=.r $S3 [OK] -- OUT: $S3 . [OK]
 			STOP
 		EOF
 		test_cmp_count expected.log rot13-filter.log &&
@@ -385,23 +385,23 @@ test_expect_success PERL 'required process filter should filter data' '
 			IN: clean test.r $S [OK] -- OUT: $S . [OK]
 			IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
 			IN: clean test4-empty.r 0 [OK] -- OUT: 0  [OK]
-			IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+			IN: clean testsubdir/test3 '\''sq'\'',\$x=.r $S3 [OK] -- OUT: $S3 . [OK]
 			IN: clean test.r $S [OK] -- OUT: $S . [OK]
 			IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
 			IN: clean test4-empty.r 0 [OK] -- OUT: 0  [OK]
-			IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+			IN: clean testsubdir/test3 '\''sq'\'',\$x=.r $S3 [OK] -- OUT: $S3 . [OK]
 			STOP
 		EOF
 		test_cmp_count expected.log rot13-filter.log &&

-		rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x.r" &&
+		rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x=.r" &&

 		filter_git checkout --quiet --no-progress . &&
 		cat >expected.log <<-EOF &&
 			START
 			init handshake complete
 			IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
-			IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+			IN: smudge testsubdir/test3 '\''sq'\'',\$x=.r $S3 [OK] -- OUT: $S3 . [OK]
 			STOP
 		EOF
 		test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -422,14 +422,14 @@ test_expect_success PERL 'required process filter should filter data' '
 			IN: smudge test.r $S [OK] -- OUT: $S . [OK]
 			IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
 			IN: smudge test4-empty.r 0 [OK] -- OUT: 0  [OK]
-			IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+			IN: smudge testsubdir/test3 '\''sq'\'',\$x=.r $S3 [OK] -- OUT: $S3 . [OK]
 			STOP
 		EOF
 		test_cmp_exclude_clean expected.log rot13-filter.log &&

 		test_cmp_committed_rot13 "$TEST_ROOT/test.o" test.r &&
 		test_cmp_committed_rot13 "$TEST_ROOT/test2.o" test2.r &&
-		test_cmp_committed_rot13 "$TEST_ROOT/test3 '\''sq'\'',\$x.o" "testsubdir/test3 '\''sq'\'',\$x.r"
+		test_cmp_committed_rot13 "$TEST_ROOT/test3 '\''sq'\'',\$x=.o" "testsubdir/test3 '\''sq'\'',\$x=.r"
 	)
 '

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 4d5697ee51..617f581e56 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -109,14 +109,18 @@ print $debug "init handshake complete\n";
 $debug->flush();

 while (1) {
-	my ($command) = packet_txt_read() =~ /^command=([^=]+)$/;
+	my ($command) = packet_txt_read() =~ /^command=(.+)$/;
 	print $debug "IN: $command";
 	$debug->flush();

-	my ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;
+	my ($pathname) = packet_txt_read() =~ /^pathname=(.+)$/;
 	print $debug " $pathname";
 	$debug->flush();

+	if ( $pathname eq "" ) {
+		die "bad pathname '$pathname'";
+	}
+
 	# Flush
 	packet_bin_read();

--
2.11.0

Re: [PATCH] docs: warn about possible '=' in clean/smudge filter process values

[Jeff King]

On Sat, Dec 03, 2016 at 08:45:16PM +0100, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> A pathname value in a clean/smudge filter process "key=value" pair can
> contain the '=' character (introduced in edcc858). Make the user aware
> of this issue in the docs, add a corresponding test case, and fix the
> issue in filter process value parser of the example implementation in
> contrib.

Yeah, I just naturally assumed it would work this way, as it's pretty
standard in our other key=value protocols. But certainly it's reasonable
to document it (and to keep the t0021 filter as a good example).

-Peff

Re: [PATCH] docs: warn about possible '=' in clean/smudge filter process values

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Dec 03, 2016 at 08:45:16PM +0100, larsxschneider@gmail.com wrote:
>
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> A pathname value in a clean/smudge filter process "key=value" pair can
>> contain the '=' character (introduced in edcc858). Make the user aware
>> of this issue in the docs, add a corresponding test case, and fix the
>> issue in filter process value parser of the example implementation in
>> contrib.
>
> Yeah, I just naturally assumed it would work this way, as it's pretty
> standard in our other key=value protocols. But certainly it's reasonable
> to document it (and to keep the t0021 filter as a good example).

There are diplomatic ways and other ways to say the same thing, it
seems, and yours is almost always more diplomatic than mine ;-) 

My knee-jerk reaction last night that I didn't send was "I would
understand if the new test were about quotes or whitespaces, but is
it even useful to make sure that the value can have an equal sign in
it?"