# [PATCH] docs: warn about possible '=' in clean/smudge filter process values 3 messages from 2016-12-03 to 2016-12-05. Participants: larsxschneider@gmail.com, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/44608 ## larsxschneider@gmail.com, 2016-12-03 19:45 Subject: [PATCH] docs: warn about possible '=' in clean/smudge filter process values Message-ID: <20161203194516.12879-1-larsxschneider@gmail.com> URL: https://gitlist.dev/e/20161203194516.12879-1-larsxschneider%40gmail.com ``` From: Lars Schneider 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 --- 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 ``` ## Jeff King, 2016-12-05 13:12 Subject: Re: [PATCH] docs: warn about possible '=' in clean/smudge filter process values Message-ID: <20161205131219.kdq4zafith26vro2@sigill.intra.peff.net> URL: https://gitlist.dev/e/20161205131219.kdq4zafith26vro2%40sigill.intra.peff.net In-Reply-To: <20161203194516.12879-1-larsxschneider@gmail.com> ``` On Sat, Dec 03, 2016 at 08:45:16PM +0100, larsxschneider@gmail.com wrote: > From: Lars Schneider > > 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, 2016-12-05 17:49 Subject: Re: [PATCH] docs: warn about possible '=' in clean/smudge filter process values Message-ID: URL: https://gitlist.dev/e/xmqqfum25kr0.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20161205131219.kdq4zafith26vro2@sigill.intra.peff.net> ``` Jeff King writes: > On Sat, Dec 03, 2016 at 08:45:16PM +0100, larsxschneider@gmail.com wrote: > >> From: Lars Schneider >> >> 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?" ```