threads / patch / 44608

patchdocs: warn about possible '=' in clean/smudge filter process values

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

## tl;dr

3 messages between Dec 3, 2016 and Dec 5, 2016. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

larsxschneider@gmail.com· Dec 3, 2016, 19:45 UTC · lore
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(-)
Show changes to 4 files +25 −16

Documentation/gitattributes.txt, contrib/long-running-filter/example.pl, t/t0021-conversion.sh, t/t0021/rot13-filter.pl

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
Jeff King· Dec 5, 2016, 13:12 UTC · re: larsxschneider@gmail.com · lore

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

On Sat, Dec 03, 2016 at 08:45:16PM +0100, larsxschneider@gmail.com wrote:
Show 7 quoted lines
> 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
Junio C Hamano· Dec 5, 2016, 17:49 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> 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?"

← back to recent threads