{"thread":{"id":"44608","subject":"[PATCH] docs: warn about possible '=' in clean/smudge filter process values","startedAt":"2016-12-03T19:45:34Z","lastAt":"2016-12-05T17:50:08Z","messageCount":3,"participants":["larsxschneider@gmail.com","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"306877","messageId":"20161203194516.12879-1-larsxschneider@gmail.com","threadId":"44608","inReplyTo":null,"subject":"[PATCH] docs: warn about possible '=' in clean/smudge filter process values","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-12-03T19:45:16Z","receivedAt":"2016-12-03T19:45:34Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nA pathname value in a clean/smudge filter process \"key=value\" pair can\ncontain the '=' character (introduced in edcc858). Make the user aware\nof this issue in the docs, add a corresponding test case, and fix the\nissue in filter process value parser of the example implementation in\ncontrib.\n\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n Documentation/gitattributes.txt        |  4 +++-\n contrib/long-running-filter/example.pl |  8 ++++++--\n t/t0021-conversion.sh                  | 20 ++++++++++----------\n t/t0021/rot13-filter.pl                |  8 ++++++--\n 4 files changed, 25 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 976243a63e..e0b66c1220 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -435,7 +435,9 @@ to filter relative to the repository root. Right after the flush packet\n Git sends the content split in zero or more pkt-line packets and a\n flush packet to terminate content. Please note, that the filter\n must not send any response before it received the content and the\n-final flush packet.\n+final flush packet. Also note that the \"value\" of a \"key=value\" pair\n+can contain the \"=\" character whereas the key would never contain\n+that character.\n ------------------------\n packet:          git> command=smudge\n packet:          git> pathname=path/testfile.dat\ndiff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl\nindex 39457055a5..a677569ddd 100755\n--- a/contrib/long-running-filter/example.pl\n+++ b/contrib/long-running-filter/example.pl\n@@ -81,8 +81,12 @@ packet_txt_write(\"capability=smudge\");\n packet_flush();\n\n while (1) {\n-\tmy ($command)  = packet_txt_read() =~ /^command=([^=]+)$/;\n-\tmy ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;\n+\tmy ($command)  = packet_txt_read() =~ /^command=(.+)$/;\n+\tmy ($pathname) = packet_txt_read() =~ /^pathname=(.+)$/;\n+\n+\tif ( $pathname eq \"\" ) {\n+\t\tdie \"bad pathname '$pathname'\";\n+\t}\n\n \tpacket_bin_read();\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 4ea534e9fa..f3a0df2add 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -93,7 +93,7 @@ test_expect_success setup '\n \tgit checkout -- test test.t test.i &&\n\n \techo \"content-test2\" >test2.o &&\n-\techo \"content-test3 - filename with special characters\" >\"test3 '\\''sq'\\'',\\$x.o\"\n+\techo \"content-test3 - filename with special characters\" >\"test3 '\\''sq'\\'',\\$x=.o\"\n '\n\n script='s/^\\$Id: \\([0-9a-f]*\\) \\$/\\1/p'\n@@ -359,12 +359,12 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\tcp \"$TEST_ROOT/test.o\" test.r &&\n \t\tcp \"$TEST_ROOT/test2.o\" test2.r &&\n \t\tmkdir testsubdir &&\n-\t\tcp \"$TEST_ROOT/test3 '\\''sq'\\'',\\$x.o\" \"testsubdir/test3 '\\''sq'\\'',\\$x.r\" &&\n+\t\tcp \"$TEST_ROOT/test3 '\\''sq'\\'',\\$x=.o\" \"testsubdir/test3 '\\''sq'\\'',\\$x=.r\" &&\n \t\t>test4-empty.r &&\n\n \t\tS=$(file_size test.r) &&\n \t\tS2=$(file_size test2.r) &&\n-\t\tS3=$(file_size \"testsubdir/test3 '\\''sq'\\'',\\$x.r\") &&\n+\t\tS3=$(file_size \"testsubdir/test3 '\\''sq'\\'',\\$x=.r\") &&\n\n \t\tfilter_git add . &&\n \t\tcat >expected.log <<-EOF &&\n@@ -373,7 +373,7 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\t\tIN: clean test.r $S [OK] -- OUT: $S . [OK]\n \t\t\tIN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]\n \t\t\tIN: clean test4-empty.r 0 [OK] -- OUT: 0  [OK]\n-\t\t\tIN: clean testsubdir/test3 '\\''sq'\\'',\\$x.r $S3 [OK] -- OUT: $S3 . [OK]\n+\t\t\tIN: clean testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n \t\t\tSTOP\n \t\tEOF\n \t\ttest_cmp_count expected.log rot13-filter.log &&\n@@ -385,23 +385,23 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\t\tIN: clean test.r $S [OK] -- OUT: $S . [OK]\n \t\t\tIN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]\n \t\t\tIN: clean test4-empty.r 0 [OK] -- OUT: 0  [OK]\n-\t\t\tIN: clean testsubdir/test3 '\\''sq'\\'',\\$x.r $S3 [OK] -- OUT: $S3 . [OK]\n+\t\t\tIN: clean testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n \t\t\tIN: clean test.r $S [OK] -- OUT: $S . [OK]\n \t\t\tIN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]\n \t\t\tIN: clean test4-empty.r 0 [OK] -- OUT: 0  [OK]\n-\t\t\tIN: clean testsubdir/test3 '\\''sq'\\'',\\$x.r $S3 [OK] -- OUT: $S3 . [OK]\n+\t\t\tIN: clean testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n \t\t\tSTOP\n \t\tEOF\n \t\ttest_cmp_count expected.log rot13-filter.log &&\n\n-\t\trm -f test2.r \"testsubdir/test3 '\\''sq'\\'',\\$x.r\" &&\n+\t\trm -f test2.r \"testsubdir/test3 '\\''sq'\\'',\\$x=.r\" &&\n\n \t\tfilter_git checkout --quiet --no-progress . &&\n \t\tcat >expected.log <<-EOF &&\n \t\t\tSTART\n \t\t\tinit handshake complete\n \t\t\tIN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]\n-\t\t\tIN: smudge testsubdir/test3 '\\''sq'\\'',\\$x.r $S3 [OK] -- OUT: $S3 . [OK]\n+\t\t\tIN: smudge testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n \t\t\tSTOP\n \t\tEOF\n \t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n@@ -422,14 +422,14 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\t\tIN: smudge test.r $S [OK] -- OUT: $S . [OK]\n \t\t\tIN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]\n \t\t\tIN: smudge test4-empty.r 0 [OK] -- OUT: 0  [OK]\n-\t\t\tIN: smudge testsubdir/test3 '\\''sq'\\'',\\$x.r $S3 [OK] -- OUT: $S3 . [OK]\n+\t\t\tIN: smudge testsubdir/test3 '\\''sq'\\'',\\$x=.r $S3 [OK] -- OUT: $S3 . [OK]\n \t\t\tSTOP\n \t\tEOF\n \t\ttest_cmp_exclude_clean expected.log rot13-filter.log &&\n\n \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test.o\" test.r &&\n \t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test2.o\" test2.r &&\n-\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test3 '\\''sq'\\'',\\$x.o\" \"testsubdir/test3 '\\''sq'\\'',\\$x.r\"\n+\t\ttest_cmp_committed_rot13 \"$TEST_ROOT/test3 '\\''sq'\\'',\\$x=.o\" \"testsubdir/test3 '\\''sq'\\'',\\$x=.r\"\n \t)\n '\n\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex 4d5697ee51..617f581e56 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -109,14 +109,18 @@ print $debug \"init handshake complete\\n\";\n $debug->flush();\n\n while (1) {\n-\tmy ($command) = packet_txt_read() =~ /^command=([^=]+)$/;\n+\tmy ($command) = packet_txt_read() =~ /^command=(.+)$/;\n \tprint $debug \"IN: $command\";\n \t$debug->flush();\n\n-\tmy ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;\n+\tmy ($pathname) = packet_txt_read() =~ /^pathname=(.+)$/;\n \tprint $debug \" $pathname\";\n \t$debug->flush();\n\n+\tif ( $pathname eq \"\" ) {\n+\t\tdie \"bad pathname '$pathname'\";\n+\t}\n+\n \t# Flush\n \tpacket_bin_read();\n\n--\n2.11.0\n\n"},{"id":"306933","messageId":"20161205131219.kdq4zafith26vro2@sigill.intra.peff.net","threadId":"44608","inReplyTo":"20161203194516.12879-1-larsxschneider@gmail.com","subject":"Re: [PATCH] docs: warn about possible '=' in clean/smudge filter process values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-05T13:12:19Z","receivedAt":"2016-12-05T13:12:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 03, 2016 at 08:45:16PM +0100, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider <larsxschneider@gmail.com>\n> \n> A pathname value in a clean/smudge filter process \"key=value\" pair can\n> contain the '=' character (introduced in edcc858). Make the user aware\n> of this issue in the docs, add a corresponding test case, and fix the\n> issue in filter process value parser of the example implementation in\n> contrib.\n\nYeah, I just naturally assumed it would work this way, as it's pretty\nstandard in our other key=value protocols. But certainly it's reasonable\nto document it (and to keep the t0021 filter as a good example).\n\n-Peff\n"},{"id":"306939","messageId":"xmqqfum25kr0.fsf@gitster.mtv.corp.google.com","threadId":"44608","inReplyTo":"20161205131219.kdq4zafith26vro2@sigill.intra.peff.net","subject":"Re: [PATCH] docs: warn about possible '=' in clean/smudge filter process values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-05T17:49:39Z","receivedAt":"2016-12-05T17:50:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Dec 03, 2016 at 08:45:16PM +0100, larsxschneider@gmail.com wrote:\n>\n>> From: Lars Schneider <larsxschneider@gmail.com>\n>> \n>> A pathname value in a clean/smudge filter process \"key=value\" pair can\n>> contain the '=' character (introduced in edcc858). Make the user aware\n>> of this issue in the docs, add a corresponding test case, and fix the\n>> issue in filter process value parser of the example implementation in\n>> contrib.\n>\n> Yeah, I just naturally assumed it would work this way, as it's pretty\n> standard in our other key=value protocols. But certainly it's reasonable\n> to document it (and to keep the t0021 filter as a good example).\n\nThere are diplomatic ways and other ways to say the same thing, it\nseems, and yours is almost always more diplomatic than mine ;-) \n\nMy knee-jerk reaction last night that I didn't send was \"I would\nunderstand if the new test were about quotes or whitespaces, but is\nit even useful to make sure that the value can have an equal sign in\nit?\"\n\n\n"}]}