{"thread":{"id":"46998","subject":"[PATCH 0/6] Create Git/Packet.pm","startedAt":"2017-10-19T12:30:44Z","lastAt":"2017-11-05T12:50:45Z","messageCount":32,"participants":["Christian Couder","Stefan Beller","Junio C Hamano","Philip Oakley","Johannes Schindelin","Jacob Keller","Bryan Turner","Lars Schneider","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"330632","messageId":"20171019123030.17338-1-chriscool@tuxfamily.org","threadId":"46998","inReplyTo":null,"subject":"[PATCH 0/6] Create Git/Packet.pm","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-19T12:30:24Z","receivedAt":"2017-10-19T12:30:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Goal\n~~~~\n\nPacket related functions in Perl can be useful to write new filters or\nto debug or test existing filters. So instead of having them in\nt0021/rot13-filter.pl, let's extract them into a new Git/Packet.pm\nmodule.\n\nLinks\n~~~~~\n\nThis patch series has been extracted from previous \"Add initial\nexperimental external ODB support\" patch series.\n\nVersion 1, 2, 3, 4, 5 and 6 of this previous series are on the mailing\nlist here:\n\nhttps://public-inbox.org/git/20160613085546.11784-1-chriscool@tuxfamily.org/\nhttps://public-inbox.org/git/20160628181933.24620-1-chriscool@tuxfamily.org/\nhttps://public-inbox.org/git/20161130210420.15982-1-chriscool@tuxfamily.org/\nhttps://public-inbox.org/git/20170620075523.26961-1-chriscool@tuxfamily.org/\nhttps://public-inbox.org/git/20170803091926.1755-1-chriscool@tuxfamily.org/\nhttps://public-inbox.org/git/20170916080731.13925-1-chriscool@tuxfamily.org/\n\nThey are also available in the following branches:\n\nhttps://github.com/chriscool/git/commits/gl-external-odb12\nhttps://github.com/chriscool/git/commits/gl-external-odb22\nhttps://github.com/chriscool/git/commits/gl-external-odb61\nhttps://github.com/chriscool/git/commits/gl-external-odb239\nhttps://github.com/chriscool/git/commits/gl-external-odb373\nhttps://github.com/chriscool/git/commits/gl-external-odb411\n\n\nChristian Couder (6):\n t0021/rot13-filter: refactor packet reading functions\n t0021/rot13-filter: improve 'if .. elsif .. else' style\n t0021/rot13-filter: improve error message\n t0021/rot13-filter: add packet_initialize()\n t0021/rot13-filter: add capability functions\n Add Git/Packet.pm from parts of t0021/rot13-filter.pl\n\n perl/Git/Packet.pm | 118 ++++++++++++++++++++++++++++++++++++++++++++++++\n t/t0021/rot13-filter.pl | 110 +++++++++-----------------------------------\n 2 files changed, 140 insertions(+), 88 deletions(-)\n create mode 100644 perl/Git/Packet.pm\n\n-- \n2.15.0.rc1.106.g7e97f58a41\n\n"},{"id":"330633","messageId":"20171019123030.17338-2-chriscool@tuxfamily.org","threadId":"46998","inReplyTo":"20171019123030.17338-1-chriscool@tuxfamily.org","subject":"[PATCH 1/6] t0021/rot13-filter: refactor packet reading functions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-19T12:30:25Z","receivedAt":"2017-10-19T12:30:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"To make it possible in a following commit to move packet\nreading and writing functions into a Packet.pm module,\nlet's refactor these functions, so they don't handle\nprinting debug output and exiting.\n\nSigned-off-by: Christian Couder \n---\n t/t0021/rot13-filter.pl | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex ad685d92f8..e4495a52f3 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -60,8 +60,7 @@ sub packet_bin_read {\n \tmy $bytes_read = read STDIN, $buffer, 4;\n \tif ( $bytes_read == 0 ) {\n \t\t# EOF - Git stopped talking to us!\n-\t\tprint $debug \"STOP\\n\";\n-\t\texit();\n+\t\treturn ( -1, \"\" );\n \t}\n \telsif ( $bytes_read != 4 ) {\n \t\tdie \"invalid packet: '$buffer'\";\n@@ -85,7 +84,7 @@ sub packet_bin_read {\n \n sub packet_txt_read {\n \tmy ( $res, $buf ) = packet_bin_read();\n-\tunless ( $buf eq '' or $buf =~ s/\\n$// ) {\n+\tunless ( $res == -1 or $buf eq '' or $buf =~ s/\\n$// ) {\n \t\tdie \"A non-binary line MUST be terminated by an LF.\";\n \t}\n \treturn ( $res, $buf );\n@@ -131,7 +130,12 @@ print $debug \"init handshake complete\\n\";\n $debug->flush();\n \n while (1) {\n-\tmy ( $command ) = packet_txt_read() =~ /^command=(.+)$/;\n+\tmy ( $res, $command ) = packet_txt_read();\n+\tif ( $res == -1 ) {\n+\t\tprint $debug \"STOP\\n\";\n+\t\texit();\n+\t}\n+\t$command =~ s/^command=//;\n \tprint $debug \"IN: $command\";\n \t$debug->flush();\n \n-- \n2.15.0.rc1.106.g7e97f58a41\n\n"},{"id":"330634","messageId":"20171019123030.17338-4-chriscool@tuxfamily.org","threadId":"46998","inReplyTo":"20171019123030.17338-1-chriscool@tuxfamily.org","subject":"[PATCH 3/6] t0021/rot13-filter: improve error message","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-19T12:30:27Z","receivedAt":"2017-10-19T12:30:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"If there is no new line at the end of something it receives,\nthe packet_txt_read() function die()s, but it's difficult to\ndebug without much context.\n\nLet's give a bit more information when that happens.\n\nSigned-off-by: Christian Couder \n---\n t/t0021/rot13-filter.pl | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex 82882392ae..3b3da8a03d 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -82,7 +82,8 @@ sub packet_bin_read {\n sub packet_txt_read {\n \tmy ( $res, $buf ) = packet_bin_read();\n \tunless ( $res == -1 or $buf eq '' or $buf =~ s/\\n$// ) {\n-\t\tdie \"A non-binary line MUST be terminated by an LF.\";\n+\t\tdie \"A non-binary line MUST be terminated by an LF.\\n\"\n+\t\t . \"Received: '$buf'\";\n \t}\n \treturn ( $res, $buf );\n }\n-- \n2.15.0.rc1.106.g7e97f58a41\n\n"},{"id":"330635","messageId":"20171019123030.17338-6-chriscool@tuxfamily.org","threadId":"46998","inReplyTo":"20171019123030.17338-1-chriscool@tuxfamily.org","subject":"[PATCH 5/6] t0021/rot13-filter: add capability functions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-19T12:30:29Z","receivedAt":"2017-10-19T12:30:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Add functions to help read and write capabilities.\nThese functions will be reused in following patches.\n\nSigned-off-by: Christian Couder \n---\n t/t0021/rot13-filter.pl | 40 ++++++++++++++++++++++++++++++++--------\n 1 file changed, 32 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex 278fc6f534..ba18b207c6 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -116,20 +116,44 @@ sub packet_initialize {\n \tpacket_flush();\n }\n \n+sub packet_read_capabilities {\n+\tmy @cap;\n+\twhile (1) {\n+\t\tmy ( $res, $buf ) = packet_bin_read();\n+\t\treturn ( $res, @cap ) if ( $res != 0 );\n+\t\tunless ( $buf =~ s/\\n$// ) {\n+\t\t\tdie \"A non-binary line MUST be terminated by an LF.\\n\"\n+\t\t\t . \"Received: '$buf'\";\n+\t\t}\n+\t\tdie \"bad capability buf: '$buf'\" unless ( $buf =~ s/capability=// );\n+\t\tpush @cap, $buf;\n+\t}\n+}\n+\n+sub packet_read_and_check_capabilities {\n+\tmy @local_caps = @_;\n+\tmy @remote_res_caps = packet_read_capabilities();\n+\tmy $res = shift @remote_res_caps;\n+\tmy %remote_caps = map { $_ => 1 } @remote_res_caps;\n+\tforeach (@local_caps) {\n+ \tdie \"'$_' capability not available\" unless (exists($remote_caps{$_}));\n+\t}\n+\treturn $res;\n+}\n+\n+sub packet_write_capabilities {\n+\tpacket_txt_write( \"capability=\" . $_ ) foreach (@_);\n+\tpacket_flush();\n+}\n+\n print $debug \"START\\n\";\n $debug->flush();\n \n packet_initialize(\"git-filter\", 2);\n \n-( packet_txt_read() eq ( 0, \"capability=clean\" ) ) || die \"bad capability\";\n-( packet_txt_read() eq ( 0, \"capability=smudge\" ) ) || die \"bad capability\";\n-( packet_txt_read() eq ( 0, \"capability=delay\" ) ) || die \"bad capability\";\n-( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad capability end\";\n+packet_read_and_check_capabilities(\"clean\", \"smudge\", \"delay\");\n+packet_write_capabilities(@capabilities);\n \n-foreach (@capabilities) {\n-\tpacket_txt_write( \"capability=\" . $_ );\n-}\n-packet_flush();\n print $debug \"init handshake complete\\n\";\n $debug->flush();\n \n-- \n2.15.0.rc1.106.g7e97f58a41\n\n"},{"id":"330636","messageId":"20171019123030.17338-7-chriscool@tuxfamily.org","threadId":"46998","inReplyTo":"20171019123030.17338-1-chriscool@tuxfamily.org","subject":"[PATCH 6/6] Add Git/Packet.pm from parts of t0021/rot13-filter.pl","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-19T12:30:30Z","receivedAt":"2017-10-19T12:30:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"And while at it let's simplify t0021/rot13-filter.pl by\nusing Git/Packet.pm.\n\nThis will make it possible to reuse packet related\nfunctions in other test scripts.\n\nSigned-off-by: Christian Couder \n---\n perl/Git/Packet.pm | 118 ++++++++++++++++++++++++++++++++++++++++++++++++\n t/t0021/rot13-filter.pl | 94 ++------------------------------------\n 2 files changed, 121 insertions(+), 91 deletions(-)\n create mode 100644 perl/Git/Packet.pm\n\ndiff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm\nnew file mode 100644\nindex 0000000000..b1e67477a0\n--- /dev/null\n+++ b/perl/Git/Packet.pm\n@@ -0,0 +1,118 @@\n+package Git::Packet;\n+use 5.008;\n+use strict;\n+use warnings;\n+BEGIN {\n+\trequire Exporter;\n+\tif ($] < 5.008003) {\n+\t\t*import = \\&Exporter::import;\n+\t} else {\n+\t\t# Exporter 5.57 which supports this invocation was\n+\t\t# released with perl 5.8.3\n+\t\tExporter->import('import');\n+\t}\n+}\n+\n+our @EXPORT = qw(\n+\t\t\tpacket_bin_read\n+\t\t\tpacket_txt_read\n+\t\t\tpacket_bin_write\n+\t\t\tpacket_txt_write\n+\t\t\tpacket_flush\n+\t\t\tpacket_initialize\n+\t\t\tpacket_read_capabilities\n+\t\t\tpacket_write_capabilities\n+\t\t\tpacket_read_and_check_capabilities\n+\t\t);\n+our @EXPORT_OK = @EXPORT;\n+\n+sub packet_bin_read {\n+\tmy $buffer;\n+\tmy $bytes_read = read STDIN, $buffer, 4;\n+\tif ( $bytes_read == 0 ) {\n+\t\t# EOF - Git stopped talking to us!\n+\t\treturn ( -1, \"\" );\n+\t} elsif ( $bytes_read != 4 ) {\n+\t\tdie \"invalid packet: '$buffer'\";\n+\t}\n+\tmy $pkt_size = hex($buffer);\n+\tif ( $pkt_size == 0 ) {\n+\t\treturn ( 1, \"\" );\n+\t} elsif ( $pkt_size > 4 ) {\n+\t\tmy $content_size = $pkt_size - 4;\n+\t\t$bytes_read = read STDIN, $buffer, $content_size;\n+\t\tif ( $bytes_read != $content_size ) {\n+\t\t\tdie \"invalid packet ($content_size bytes expected; $bytes_read bytes read)\";\n+\t\t}\n+\t\treturn ( 0, $buffer );\n+\t} else {\n+\t\tdie \"invalid packet size: $pkt_size\";\n+\t}\n+}\n+\n+sub packet_txt_read {\n+\tmy ( $res, $buf ) = packet_bin_read();\n+\tunless ( $res == -1 or $buf eq '' or $buf =~ s/\\n$// ) {\n+\t\tdie \"A non-binary line MUST be terminated by an LF.\\n\"\n+\t\t . \"Received: '$buf'\";\n+\t}\n+\treturn ( $res, $buf );\n+}\n+\n+sub packet_bin_write {\n+\tmy $buf = shift;\n+\tprint STDOUT sprintf( \"%04x\", length($buf) + 4 );\n+\tprint STDOUT $buf;\n+\tSTDOUT->flush();\n+}\n+\n+sub packet_txt_write {\n+\tpacket_bin_write( $_[0] . \"\\n\" );\n+}\n+\n+sub packet_flush {\n+\tprint STDOUT sprintf( \"%04x\", 0 );\n+\tSTDOUT->flush();\n+}\n+\n+sub packet_initialize {\n+\tmy ($name, $version) = @_;\n+\n+\t( packet_txt_read() eq ( 0, $name . \"-client\" ) )\t|| die \"bad initialize\";\n+\t( packet_txt_read() eq ( 0, \"version=\" . $version ) )\t|| die \"bad version\";\n+\t( packet_bin_read() eq ( 1, \"\" ) )\t\t\t|| die \"bad version end\";\n+\n+\tpacket_txt_write( $name . \"-server\" );\n+\tpacket_txt_write( \"version=\" . $version );\n+\tpacket_flush();\n+}\n+\n+sub packet_read_capabilities {\n+\tmy @cap;\n+\twhile (1) {\n+\t\tmy ( $res, $buf ) = packet_bin_read();\n+\t\treturn ( $res, @cap ) if ( $res != 0 );\n+\t\tunless ( $buf =~ s/\\n$// ) {\n+\t\t\tdie \"A non-binary line MUST be terminated by an LF.\\n\"\n+\t\t\t . \"Received: '$buf'\";\n+\t\t}\n+\t\tdie \"bad capability buf: '$buf'\" unless ( $buf =~ s/capability=// );\n+\t\tpush @cap, $buf;\n+\t}\n+}\n+\n+sub packet_read_and_check_capabilities {\n+\tmy @local_caps = @_;\n+\tmy @remote_res_caps = packet_read_capabilities();\n+\tmy $res = shift @remote_res_caps;\n+\tmy %remote_caps = map { $_ => 1 } @remote_res_caps;\n+\tforeach (@local_caps) {\n+\t\tdie \"'$_' capability not available\" unless (exists($remote_caps{$_}));\n+\t}\n+\treturn $res;\n+}\n+\n+sub packet_write_capabilities {\n+\tpacket_txt_write( \"capability=\" . $_ ) foreach (@_);\n+\tpacket_flush();\n+}\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex ba18b207c6..2e8ad4d496 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -30,9 +30,12 @@\n # to the \"list_available_blobs\" response.\n #\n \n+use 5.008;\n+use lib (split(/:/, $ENV{GITPERLLIB}));\n use strict;\n use warnings;\n use IO::File;\n+use Git::Packet;\n \n my $MAX_PACKET_CONTENT_SIZE = 65516;\n my $log_file = shift @ARGV;\n@@ -55,97 +58,6 @@ sub rot13 {\n \treturn $str;\n }\n \n-sub packet_bin_read {\n-\tmy $buffer;\n-\tmy $bytes_read = read STDIN, $buffer, 4;\n-\tif ( $bytes_read == 0 ) {\n-\t\t# EOF - Git stopped talking to us!\n-\t\treturn ( -1, \"\" );\n-\t} elsif ( $bytes_read != 4 ) {\n-\t\tdie \"invalid packet: '$buffer'\";\n-\t}\n-\tmy $pkt_size = hex($buffer);\n-\tif ( $pkt_size == 0 ) {\n-\t\treturn ( 1, \"\" );\n-\t} elsif ( $pkt_size > 4 ) {\n-\t\tmy $content_size = $pkt_size - 4;\n-\t\t$bytes_read = read STDIN, $buffer, $content_size;\n-\t\tif ( $bytes_read != $content_size ) {\n-\t\t\tdie \"invalid packet ($content_size bytes expected; $bytes_read bytes read)\";\n-\t\t}\n-\t\treturn ( 0, $buffer );\n-\t} else {\n-\t\tdie \"invalid packet size: $pkt_size\";\n-\t}\n-}\n-\n-sub packet_txt_read {\n-\tmy ( $res, $buf ) = packet_bin_read();\n-\tunless ( $res == -1 or $buf eq '' or $buf =~ s/\\n$// ) {\n-\t\tdie \"A non-binary line MUST be terminated by an LF.\\n\"\n-\t\t . \"Received: '$buf'\";\n-\t}\n-\treturn ( $res, $buf );\n-}\n-\n-sub packet_bin_write {\n-\tmy $buf = shift;\n-\tprint STDOUT sprintf( \"%04x\", length($buf) + 4 );\n-\tprint STDOUT $buf;\n-\tSTDOUT->flush();\n-}\n-\n-sub packet_txt_write {\n-\tpacket_bin_write( $_[0] . \"\\n\" );\n-}\n-\n-sub packet_flush {\n-\tprint STDOUT sprintf( \"%04x\", 0 );\n-\tSTDOUT->flush();\n-}\n-\n-sub packet_initialize {\n-\tmy ($name, $version) = @_;\n-\n-\t( packet_txt_read() eq ( 0, $name . \"-client\" ) ) || die \"bad initialize\";\n-\t( packet_txt_read() eq ( 0, \"version=\" . $version ) ) || die \"bad version\";\n-\t( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad version end\";\n-\n-\tpacket_txt_write( $name . \"-server\" );\n-\tpacket_txt_write( \"version=\" . $version );\n-\tpacket_flush();\n-}\n-\n-sub packet_read_capabilities {\n-\tmy @cap;\n-\twhile (1) {\n-\t\tmy ( $res, $buf ) = packet_bin_read();\n-\t\treturn ( $res, @cap ) if ( $res != 0 );\n-\t\tunless ( $buf =~ s/\\n$// ) {\n-\t\t\tdie \"A non-binary line MUST be terminated by an LF.\\n\"\n-\t\t\t . \"Received: '$buf'\";\n-\t\t}\n-\t\tdie \"bad capability buf: '$buf'\" unless ( $buf =~ s/capability=// );\n-\t\tpush @cap, $buf;\n-\t}\n-}\n-\n-sub packet_read_and_check_capabilities {\n-\tmy @local_caps = @_;\n-\tmy @remote_res_caps = packet_read_capabilities();\n-\tmy $res = shift @remote_res_caps;\n-\tmy %remote_caps = map { $_ => 1 } @remote_res_caps;\n-\tforeach (@local_caps) {\n- \tdie \"'$_' capability not available\" unless (exists($remote_caps{$_}));\n-\t}\n-\treturn $res;\n-}\n-\n-sub packet_write_capabilities {\n-\tpacket_txt_write( \"capability=\" . $_ ) foreach (@_);\n-\tpacket_flush();\n-}\n-\n print $debug \"START\\n\";\n $debug->flush();\n \n-- \n2.15.0.rc1.106.g7e97f58a41\n\n"},{"id":"330637","messageId":"20171019123030.17338-3-chriscool@tuxfamily.org","threadId":"46998","inReplyTo":"20171019123030.17338-1-chriscool@tuxfamily.org","subject":"[PATCH 2/6] t0021/rot13-filter: improve 'if .. elsif .. else' style","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-19T12:30:26Z","receivedAt":"2017-10-19T12:31:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Before further refactoring the \"t0021/rot13-filter.pl\" script,\nlet's modernize the style of its 'if .. elsif .. else' clauses\nto improve its readability by making it more similar to our\nother perl scripts.\n\nSigned-off-by: Christian Couder \n---\n t/t0021/rot13-filter.pl | 39 +++++++++++++--------------------------\n 1 file changed, 13 insertions(+), 26 deletions(-)\n\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex e4495a52f3..82882392ae 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -61,23 +61,20 @@ sub packet_bin_read {\n \tif ( $bytes_read == 0 ) {\n \t\t# EOF - Git stopped talking to us!\n \t\treturn ( -1, \"\" );\n-\t}\n-\telsif ( $bytes_read != 4 ) {\n+\t} elsif ( $bytes_read != 4 ) {\n \t\tdie \"invalid packet: '$buffer'\";\n \t}\n \tmy $pkt_size = hex($buffer);\n \tif ( $pkt_size == 0 ) {\n \t\treturn ( 1, \"\" );\n-\t}\n-\telsif ( $pkt_size > 4 ) {\n+\t} elsif ( $pkt_size > 4 ) {\n \t\tmy $content_size = $pkt_size - 4;\n \t\t$bytes_read = read STDIN, $buffer, $content_size;\n \t\tif ( $bytes_read != $content_size ) {\n \t\t\tdie \"invalid packet ($content_size bytes expected; $bytes_read bytes read)\";\n \t\t}\n \t\treturn ( 0, $buffer );\n-\t}\n-\telse {\n+\t} else {\n \t\tdie \"invalid packet size: $pkt_size\";\n \t}\n }\n@@ -165,8 +162,7 @@ while (1) {\n \t\t$debug->flush();\n \t\tpacket_txt_write(\"status=success\");\n \t\tpacket_flush();\n-\t}\n-\telse {\n+\t} else {\n \t\tmy ( $pathname ) = packet_txt_read() =~ /^pathname=(.+)$/;\n \t\tprint $debug \" $pathname\";\n \t\t$debug->flush();\n@@ -205,17 +201,13 @@ while (1) {\n \t\tmy $output;\n \t\tif ( exists $DELAY{$pathname} and exists $DELAY{$pathname}{\"output\"} ) {\n \t\t\t$output = $DELAY{$pathname}{\"output\"}\n-\t\t}\n-\t\telsif ( $pathname eq \"error.r\" or $pathname eq \"abort.r\" ) {\n+\t\t} elsif ( $pathname eq \"error.r\" or $pathname eq \"abort.r\" ) {\n \t\t\t$output = \"\";\n-\t\t}\n-\t\telsif ( $command eq \"clean\" and grep( /^clean$/, @capabilities ) ) {\n+\t\t} elsif ( $command eq \"clean\" and grep( /^clean$/, @capabilities ) ) {\n \t\t\t$output = rot13($input);\n-\t\t}\n-\t\telsif ( $command eq \"smudge\" and grep( /^smudge$/, @capabilities ) ) {\n+\t\t} elsif ( $command eq \"smudge\" and grep( /^smudge$/, @capabilities ) ) {\n \t\t\t$output = rot13($input);\n-\t\t}\n-\t\telse {\n+\t\t} else {\n \t\t\tdie \"bad command '$command'\";\n \t\t}\n \n@@ -224,25 +216,21 @@ while (1) {\n \t\t\t$debug->flush();\n \t\t\tpacket_txt_write(\"status=error\");\n \t\t\tpacket_flush();\n-\t\t}\n-\t\telsif ( $pathname eq \"abort.r\" ) {\n+\t\t} elsif ( $pathname eq \"abort.r\" ) {\n \t\t\tprint $debug \"[ABORT]\\n\";\n \t\t\t$debug->flush();\n \t\t\tpacket_txt_write(\"status=abort\");\n \t\t\tpacket_flush();\n-\t\t}\n-\t\telsif ( $command eq \"smudge\" and\n+\t\t} elsif ( $command eq \"smudge\" and\n \t\t\texists $DELAY{$pathname} and\n-\t\t\t$DELAY{$pathname}{\"requested\"} == 1\n-\t\t) {\n+\t\t\t$DELAY{$pathname}{\"requested\"} == 1 ) {\n \t\t\tprint $debug \"[DELAYED]\\n\";\n \t\t\t$debug->flush();\n \t\t\tpacket_txt_write(\"status=delayed\");\n \t\t\tpacket_flush();\n \t\t\t$DELAY{$pathname}{\"requested\"} = 2;\n \t\t\t$DELAY{$pathname}{\"output\"} = $output;\n-\t\t}\n-\t\telse {\n+\t\t} else {\n \t\t\tpacket_txt_write(\"status=success\");\n \t\t\tpacket_flush();\n \n@@ -262,8 +250,7 @@ while (1) {\n \t\t\t\tprint $debug \".\";\n \t\t\t\tif ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {\n \t\t\t\t\t$output = substr( $output, $MAX_PACKET_CONTENT_SIZE );\n-\t\t\t\t}\n-\t\t\t\telse {\n+\t\t\t\t} else {\n \t\t\t\t\t$output = \"\";\n \t\t\t\t}\n \t\t\t}\n-- \n2.15.0.rc1.106.g7e97f58a41\n\n"},{"id":"330638","messageId":"20171019123030.17338-5-chriscool@tuxfamily.org","threadId":"46998","inReplyTo":"20171019123030.17338-1-chriscool@tuxfamily.org","subject":"[PATCH 4/6] t0021/rot13-filter: add packet_initialize()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-19T12:30:28Z","receivedAt":"2017-10-19T12:31:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Let's refactor the code to initialize communication into its own\npacket_initialize() function, so that we can reuse this\nfunctionality in following patches.\n\nSigned-off-by: Christian Couder \n---\n t/t0021/rot13-filter.pl | 20 +++++++++++++-------\n 1 file changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex 3b3da8a03d..278fc6f534 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -104,16 +104,22 @@ sub packet_flush {\n \tSTDOUT->flush();\n }\n \n+sub packet_initialize {\n+\tmy ($name, $version) = @_;\n+\n+\t( packet_txt_read() eq ( 0, $name . \"-client\" ) ) || die \"bad initialize\";\n+\t( packet_txt_read() eq ( 0, \"version=\" . $version ) ) || die \"bad version\";\n+\t( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad version end\";\n+\n+\tpacket_txt_write( $name . \"-server\" );\n+\tpacket_txt_write( \"version=\" . $version );\n+\tpacket_flush();\n+}\n+\n print $debug \"START\\n\";\n $debug->flush();\n \n-( packet_txt_read() eq ( 0, \"git-filter-client\" ) ) || die \"bad initialize\";\n-( packet_txt_read() eq ( 0, \"version=2\" ) ) || die \"bad version\";\n-( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad version end\";\n-\n-packet_txt_write(\"git-filter-server\");\n-packet_txt_write(\"version=2\");\n-packet_flush();\n+packet_initialize(\"git-filter\", 2);\n \n ( packet_txt_read() eq ( 0, \"capability=clean\" ) ) || die \"bad capability\";\n ( packet_txt_read() eq ( 0, \"capability=smudge\" ) ) || die \"bad capability\";\n-- \n2.15.0.rc1.106.g7e97f58a41\n\n"},{"id":"330694","messageId":"CAGZ79kZMCBAQhfu0N=dopCfifT1Mc5qWBY7wMNYeRzD+OTo4Sw@mail.gmail.com","threadId":"46998","inReplyTo":"20171019123030.17338-2-chriscool@tuxfamily.org","subject":"Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T22:01:01Z","receivedAt":"2017-10-19T22:01:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 5:30 AM, Christian Couder\n wrote:\n> To make it possible in a following commit to move packet\n> reading and writing functions into a Packet.pm module,\n> let's refactor these functions, so they don't handle\n> printing debug output and exiting.\n>\n> Signed-off-by: Christian Couder \n> ---\n\nIt looks good to me, though I am no perl expert by far.\n"},{"id":"330695","messageId":"CAGZ79kYTLrLJ22cn+yYbV8sTK-NAaeF4EcnnBj29Tt5Q7URi1g@mail.gmail.com","threadId":"46998","inReplyTo":"20171019123030.17338-7-chriscool@tuxfamily.org","subject":"Re: [PATCH 6/6] Add Git/Packet.pm from parts of t0021/rot13-filter.pl","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-19T22:06:02Z","receivedAt":"2017-10-19T22:06:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 19, 2017 at 5:30 AM, Christian Couder\n wrote:\n> And while at it let's simplify t0021/rot13-filter.pl by\n> using Git/Packet.pm.\n>\n> This will make it possible to reuse packet related\n> functions in other test scripts.\n>\n> Signed-off-by: Christian Couder \n> ---\n> perl/Git/Packet.pm | 118 ++++++++++++++++++++++++++++++++++++++++++++++++\n> t/t0021/rot13-filter.pl | 94 ++------------------------------------\n> 2 files changed, 121 insertions(+), 91 deletions(-)\n> create mode 100644 perl/Git/Packet.pm\n>\n> diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm\n> new file mode 100644\n> index 0000000000..b1e67477a0\n> --- /dev/null\n> +++ b/perl/Git/Packet.pm\n> @@ -0,0 +1,118 @@\n> +package Git::Packet;\n> +use 5.008;\n\n+Avar, who knows more about perl versioning.\n\nAs we move it from our tests to the official part of the code\nwe'd want to keep aligned to the rest of the perl parts\nof the code base?\n\nThis whole series looks good to me as a perl bystander.\n\nThanks,\nStefan\n"},{"id":"330793","messageId":"xmqq4lqsf17n.fsf@gitster.mtv.corp.google.com","threadId":"46998","inReplyTo":"20171019123030.17338-2-chriscool@tuxfamily.org","subject":"Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-22T00:58:04Z","receivedAt":"2017-10-22T00:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> To make it possible in a following commit to move packet\n> reading and writing functions into a Packet.pm module,\n> let's refactor these functions, so they don't handle\n> printing debug output and exiting.\n>\n> Signed-off-by: Christian Couder \n> ---\n> t/t0021/rot13-filter.pl | 12 ++++++++----\n> 1 file changed, 8 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\n> index ad685d92f8..e4495a52f3 100644\n> --- a/t/t0021/rot13-filter.pl\n> +++ b/t/t0021/rot13-filter.pl\n> @@ -60,8 +60,7 @@ sub packet_bin_read {\n> \tmy $bytes_read = read STDIN, $buffer, 4;\n> \tif ( $bytes_read == 0 ) {\n> \t\t# EOF - Git stopped talking to us!\n> -\t\tprint $debug \"STOP\\n\";\n> -\t\texit();\n> +\t\treturn ( -1, \"\" );\n> \t}\n> \telsif ( $bytes_read != 4 ) {\n> \t\tdie \"invalid packet: '$buffer'\";\n> @@ -85,7 +84,7 @@ sub packet_bin_read {\n> \n> sub packet_txt_read {\n> \tmy ( $res, $buf ) = packet_bin_read();\n> -\tunless ( $buf eq '' or $buf =~ s/\\n$// ) {\n> +\tunless ( $res == -1 or $buf eq '' or $buf =~ s/\\n$// ) {\n> \t\tdie \"A non-binary line MUST be terminated by an LF.\";\n> \t}\n> \treturn ( $res, $buf );\n> @@ -131,7 +130,12 @@ print $debug \"init handshake complete\\n\";\n> $debug->flush();\n> \n> while (1) {\n> -\tmy ( $command ) = packet_txt_read() =~ /^command=(.+)$/;\n> +\tmy ( $res, $command ) = packet_txt_read();\n> +\tif ( $res == -1 ) {\n> +\t\tprint $debug \"STOP\\n\";\n> +\t\texit();\n> +\t}\n> +\t$command =~ s/^command=//;\n> \tprint $debug \"IN: $command\";\n> \t$debug->flush();\n\nThis was not an issue in the old code which died upon unexpected EOF\ninside the lowest-level helper packet_bin_read(), but now you have\none call to packet_bin_read() and many calls to packet_txt_read()\nwhose return value is not checked for this new condition you are\nallowing packet_bin_read() to return. This step taken alone is a\nregression---let's see how the remainder of the series updates the\ncallers to compensate.\n\nI initially thought that it may be more Perl-ish to return undef or\nstring instead of returning a 2-element list, but this code needs to\ndistinguish three conditions (i.e. a normal string that is 0 or more\nbytes long, a flush, and an EOF), so that is not sufficient. Perl\nexperts on the list still may be able to suggest a better way than\nthe current one to do so, but that is outside the scope of this\nrefactoring.\n\nThanks for starting to work on this.\n\n"},{"id":"330794","messageId":"xmqqvaj8dlyz.fsf@gitster.mtv.corp.google.com","threadId":"46998","inReplyTo":"20171019123030.17338-5-chriscool@tuxfamily.org","subject":"Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-22T01:12:36Z","receivedAt":"2017-10-22T01:12:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> +sub packet_initialize {\n> +\tmy ($name, $version) = @_;\n> +\n> +\t( packet_txt_read() eq ( 0, $name . \"-client\" ) ) || die \"bad initialize\";\n> +\t( packet_txt_read() eq ( 0, \"version=\" . $version ) ) || die \"bad version\";\n> +\t( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad version end\";\n\nThis is not a new issue and it is a bit surprising that nobody\nnoticed when edcc8581 (\"convert: add filter..process\noption\", 2016-10-16) was reviewed, but the above is quite broken.\npacket_txt_read() returns a 2-element list, and on the right hand\nside of \"eq\" also has a list with (two, elements), but this takes\nthe last element of the list on each side, and compares them. The\nelading 0/1 we see above do not matter, which means we do not require\nto see a flush at the end of the version---a simple empty string or\nan EOF would do, which is definitely not what we want.\n\n\t#!/usr/bin/perl\n\tsub p {\n\t\tmy ($res, $buf) = @_;\n\t\treturn ($res, $buf);\n\t}\n\tif (p(0, \"\") eq (-1, 2, 3, \"\")) { print \"ok\\n\"; }\n\nIt is fine to leave the original code broken at this step while we\npurely move the lines around, and hopefully this will be corrected\nin a later step in the series (I am responding as I read on, so I do\nnot yet know at which patch that happens in this series).\n\nThanks.\n"},{"id":"330795","messageId":"xmqqefpwdkf9.fsf@gitster.mtv.corp.google.com","threadId":"46998","inReplyTo":"20171019123030.17338-6-chriscool@tuxfamily.org","subject":"Re: [PATCH 5/6] t0021/rot13-filter: add capability functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-22T01:46:02Z","receivedAt":"2017-10-22T01:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Add functions to help read and write capabilities.\n> These functions will be reused in following patches.\n\nOne more thing that is more noteworthy (read: do not forget to\ndescribe it in the proposed log message) is that the original used\nto require capabilities to come in a fixed order.\n\nThe new code allows these capabilities to be declared in any order,\nit even allows duplicates (intended? shouldn't we be flagging it as\nan error?), the helper can require a set of capabilities we do want\nto see and fail if the remote doesn't declare any one of them\n(good).\n\nIt does not check if the remote declares any capability we do not\nknow about (intended? the original noticed this situation and error\nout---shouldn't the more generalized helper that is meant to be\nreusable allow us to do so, too?).\n\n\tSide note: my answer to the last question is \"it is OK and\n\teven desirable to ignore the fact that they claim to support\n\ta capability we do not know about\", but I may be mistaken.\n\tThe reasoning and the conclusion that is consistent with\n\twhat the code does should be described in any case.\n\n> +sub packet_read_capabilities {\n> +\tmy @cap;\n> +\twhile (1) {\n> +\t\tmy ( $res, $buf ) = packet_bin_read();\n> +\t\treturn ( $res, @cap ) if ( $res != 0 );\n\nThe original had the same \"'list eq list' does not do what you may\nthink it does\" issue. This one corrects it, which is good.\n\nI am not sure if ($res != 0) is correct though. What should happen\nwhen you get an unexpected EOF at this point? The original would\nhave died; this ignores and continues.\n\n> +\t\tunless ( $buf =~ s/\\n$// ) {\n> +\t\t\tdie \"A non-binary line MUST be terminated by an LF.\\n\"\n> +\t\t\t . \"Received: '$buf'\";\n> +\t\t}\n\nIt may make sense to extract this in a small helper and call it from\nhere and from packet_txt_read().\n\n> +\t\tdie \"bad capability buf: '$buf'\" unless ( $buf =~ s/capability=// );\n\nThis may merely be a style thing, but I somehow find statement\nmodifiers hard to follow, unless its condition is almost always\ntrue. If you follow the logic in a loop and see \"die\" at the\nbeginning, a normal thing to expect is that there were conditionals\nthat said \"continue\" (eh, 'next' or 'redo') to catch the normal case\nand the control would reach \"die\" only under exceptional error\ncases, but hiding a rare error condition after 'unless' statement\nmodifier breaks that expectation.\n\n> +\t\tpush @cap, $buf;\n> +\t}\n> +}\n> +\n> +sub packet_read_and_check_capabilities {\n> +\tmy @local_caps = @_;\n> +\tmy @remote_res_caps = packet_read_capabilities();\n> +\tmy $res = shift @remote_res_caps;\n> +\tmy %remote_caps = map { $_ => 1 } @remote_res_caps;\n\nFYI:\n\n\tmy ($res, @remote_caps) = packet_read_capabilities();\n\tmy %remote_caps = map { $_ => 1 } @remote_caps;\n\nmay be more conventional way.\n\n> +\tforeach (@local_caps) {\n> + \tdie \"'$_' capability not available\" unless (exists($remote_caps{$_}));\n> +\t}\n\nIt is good that we can now accept capabilities in any order and\nstill enforce all the required capabilities are supported by the\nother side. It deserves a mention in the proposed log message.\n\n> +\treturn $res;\n> +}\n> +\n> +sub packet_write_capabilities {\n> +\tpacket_txt_write( \"capability=\" . $_ ) foreach (@_);\n> +\tpacket_flush();\n> +}\n> +\n> print $debug \"START\\n\";\n> $debug->flush();\n> \n> packet_initialize(\"git-filter\", 2);\n> \n> -( packet_txt_read() eq ( 0, \"capability=clean\" ) ) || die \"bad capability\";\n> -( packet_txt_read() eq ( 0, \"capability=smudge\" ) ) || die \"bad capability\";\n> -( packet_txt_read() eq ( 0, \"capability=delay\" ) ) || die \"bad capability\";\n> -( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad capability end\";\n> +packet_read_and_check_capabilities(\"clean\", \"smudge\", \"delay\");\n> +packet_write_capabilities(@capabilities);\n\nNeither the original nor the rewrite ensures that @capabilities we\nask to the other side to activate is a subset of what the other side\nactually declared.\n\nFixing this is a bit more involved than \"refactor what we have\", but\nprobably is part of \"refactor so that we can reuse in other\nsituations\". You'd want to return the list of caps received from\npacket_read_and_check_capabilities() and make sure @capabilities is\na subset of that before writing them out to the other side to\nrequest.\n\nThanks.\n"},{"id":"330796","messageId":"xmqq8tg4djkm.fsf@gitster.mtv.corp.google.com","threadId":"46998","inReplyTo":"20171019123030.17338-1-chriscool@tuxfamily.org","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-22T02:04:25Z","receivedAt":"2017-10-22T02:04:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Goal\n> ~~~~\n\nTotally offtopic, but is it only me who finds these \"section\nheaders\" in cover letters from some people irritating and/or\njarring? It perhaps is because I view the cover letter more as a\npart of conversation, not a presentation. And when you walk up to\nsomebody and start a conversation, you do not declare section\nheaders ;-) \n\nSaying \"I want to be able to do these things in the future, and here\nis to prepare for that future\" at the beginning nevertheless is a\ngood thing. It gives us readers an overall vision we can agree to\n(or be against, or offer alternatives) and sets expectations on what\nthe series would do and where it stops and leaves the remainder to\nfollow-up work.\n\n> Packet related functions in Perl can be useful to write new filters or\n> to debug or test existing filters. So instead of having them in\n> t0021/rot13-filter.pl, let's extract them into a new Git/Packet.pm\n> module.\n\nI left some comments on individual patches to point out places that\nmay need improvements. I agree with the overall direction.\n\nThanks for starting this topic.\n"},{"id":"330833","messageId":"8F505EA8D16F4A0F890AE9DEEB91434B@PhilipOakley","threadId":"46998","inReplyTo":"xmqq8tg4djkm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2017-10-23T12:26:41Z","receivedAt":"2017-10-23T12:26:47Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi Junio,\n\nFrom: \"Junio C Hamano\" \n> Christian Couder writes:\n>\n>> Goal\n>> ~~~~\n>\n> Totally offtopic, but is it only me who finds these \"section\n> headers\" in cover letters from some people irritating and/or\n> jarring?\n\nPersonally I find that, for significant patch series, that clearly breaking \nout these distinct sections is of advantage. At this stage (the very first \npatch 0/n) there is no specific conversation, so the subject line is a short \n'hello' to the topic, and then the contributor is (it is to be hoped) \nsetting out their proposal in a clear manner.\n\nSo I do like these headings for larger series, though there is some \njudgement to be made as to when the subject line alone is sufficient.\n\nAs a separate follow on, one thing that does annoy me is that in subsequent \nversions of the various patch series, folk tend to drop all explanation of \nwhy the series is of any relevance, leaving just the 'changed since last \ntime' part. This means that new readers who try and pick up / review / \ncontribute to a series later on in its development are not told the purpose. \nWhen the list is active it can, accidentally, do a disservice to the \npotential contributors who may feel that only core contributors are able to \ncontribute.\n\nWhether this series needed a Goal heading is separate discussion ;-)\n\n> .. It perhaps is because I view the cover letter more as a\n> part of conversation, not a presentation. And when you walk up to\n> somebody and start a conversation, you do not declare section\n> headers ;-)\n>\n> Saying \"I want to be able to do these things in the future, and here\n> is to prepare for that future\" at the beginning nevertheless is a\n> good thing. It gives us readers an overall vision we can agree to\n> (or be against, or offer alternatives) and sets expectations on what\n> the series would do and where it stops and leaves the remainder to\n> follow-up work.\n>\n>> Packet related functions in Perl can be useful to write new filters or\n>> to debug or test existing filters. So instead of having them in\n>> t0021/rot13-filter.pl, let's extract them into a new Git/Packet.pm\n>> module.\n>\n> I left some comments on individual patches to point out places that\n> may need improvements. I agree with the overall direction.\n>\n> Thanks for starting this topic.\n--\nPhilip \n\n"},{"id":"331047","messageId":"alpine.DEB.2.21.1.1710260008270.37495@virtualbox","threadId":"46998","inReplyTo":"20171019123030.17338-1-chriscool@tuxfamily.org","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-25T23:10:05Z","receivedAt":"2017-10-25T23:10:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian,\n\nOn Thu, 19 Oct 2017, Christian Couder wrote:\n\n> Add Git/Packet.pm from parts of t0021/rot13-filter.pl\n> \n> perl/Git/Packet.pm | 118 ++++++++++++++++++++++++++++++++++++++++++++++++\n\nThis change, together with forcing t0021/rot13-filter.pl to use\nGit/Packet.pm, breaks the test suite on Windows:\n\n\thttps://travis-ci.org/git/git/jobs/292461846\n\nThere are actually two problems, one of which is fixed by\n\n-- snip --\ndiff --git a/perl/Makefile b/perl/Makefile\nindex 15d96fcc7a5..f657de20e39 100644\n--- a/perl/Makefile\n+++ b/perl/Makefile\n@@ -30,6 +30,7 @@ instdir_SQ = $(subst ','\\'',$(prefix)/lib)\n modules += Git\n modules += Git/I18N\n modules += Git/IndexInfo\n+modules += Git/Packet\n modules += Git/SVN\n modules += Git/SVN/Memoize/YAML\n modules += Git/SVN/Fetcher\n-- snap --\n\nYou will want to pick this up in the next iteration. (You simply did not\nnotice because you did not build with NO_PERL_MAKEMAKER.)\n\nHowever, that *still* does not fix the test for me: note how in the\nverbose output recorded in Travis (see the link above), Perl fails to find\nthe Perl modules and then says:\n\nCan't locate Git/Packet.pm in @INC (you may need to install the Git::Packet module) (@INC contains: C \\BuildAgent\\_work\\5\\s\\perl\\blib\\lib;C \\BuildAgent\\_work\\5\\s\\perl\\blib\\arch\\auto\\Git /usr/lib/perl5/site_perl [..]\n\nNote that the correct blib path starts with `C:\\BuildAgent\\_work` and\nthe line\n\n\tuse lib (split(/:/, $ENV{GITPERLLIB}));\n\nsplits off the drive letter from the rest of the path. Obviously, this\nfails to Do The Right Thing, and simply points to Yet Another Portability\nProblem with Git's reliance on Unix scripting.\n\nBut why is it a Windows path in the first place? We take pains at using\nonly Unix-style paths in Git's test suite after all.\n\nWell, this one is easy. We call git.exe, which is a pure Win32 executable\n(i.e. it *wants* Windows paths, even in the environment passed to it) and\ngit.exe in turn calls Perl to interpret rot13-filter.pl. So on the way to\ngit.exe, GITPERLLIB is converted by the MSYS2 runtime into a Windows-style\npath list. And then it is not converted back when we call Perl.\n\nAs a workaround, I used a trick to exclude GITPERLLIB from being converted\nby MSYS2: setting the environment variable MSYS2_ENV_CONV_EXCL=GITPERLLIB\n\"fixed\" the test for me (with above patch thrown in). I also set the test\nin the Visual Studio Team Services build definition that runs those tests\ntriggered by Travis.\n\nIf your patch makes it into Git's `master`, we may have to hardcode that\nMSYS2_ENV_CONV_EXCL=GITPERLLIB (or augment any existing\nMSYS2_ENV_CONV_EXCL), so that other Windows developers do not have to\nstumble over the same thing and then spend 3 hours to debug it.\n\nCiao,\nDscho\n"},{"id":"331065","messageId":"xmqq4lqmfoy7.fsf@gitster.mtv.corp.google.com","threadId":"46998","inReplyTo":"alpine.DEB.2.21.1.1710260008270.37495@virtualbox","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-26T05:38:56Z","receivedAt":"2017-10-26T05:39:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Note that the correct blib path starts with `C:\\BuildAgent\\_work` and\n> the line\n>\n> \tuse lib (split(/:/, $ENV{GITPERLLIB}));\n>\n> splits off the drive letter from the rest of the path. Obviously, this\n> fails to Do The Right Thing, and simply points to Yet Another Portability\n> Problem with Git's reliance on Unix scripting.\n\nIn our C code, we have \"#define PATH_SEP ';'\", and encourage our\ncode to be careful and use it. Is there something similar for Perl\nscripts, I wonder.\n\nI notice that t/{t0202,t9000,t9700}/test.pl share the same\nsplit(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use\nthe non-platform convention to accomodate the use of split(/:/)\ncertainly is a workaround, but it does feel dirty.\n\nIt is hard to imagine that we were the first people who wants to\nsplit the value of a variable into a list, where the value is a list\nof paths, concatenated into a single string with a delimiter that\nmay be platform specific. I wonder if we are going against a best\npractice established in the Perl world, simply because we don't know\nabout it (i.e. basically, it would say \"don't split at a colon\nbecause not all world is Unix; use $this_module instead\", similar to\n\"don't split at a slash, use File::Spec instead to extract path\ncomponents\").\n\n"},{"id":"331081","messageId":"CA+P7+xpcC7Daw5k3=XFyZRsqiBib6yY2YzLpjYfcS7yqfUzTpg@mail.gmail.com","threadId":"46998","inReplyTo":"xmqq4lqmfoy7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-10-26T09:07:03Z","receivedAt":"2017-10-26T09:07:31Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n>\n>> Note that the correct blib path starts with `C:\\BuildAgent\\_work` and\n>> the line\n>>\n>> use lib (split(/:/, $ENV{GITPERLLIB}));\n>>\n>> splits off the drive letter from the rest of the path. Obviously, this\n>> fails to Do The Right Thing, and simply points to Yet Another Portability\n>> Problem with Git's reliance on Unix scripting.\n>\n> In our C code, we have \"#define PATH_SEP ';'\", and encourage our\n> code to be careful and use it. Is there something similar for Perl\n> scripts, I wonder.\n>\n> I notice that t/{t0202,t9000,t9700}/test.pl share the same\n> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use\n> the non-platform convention to accomodate the use of split(/:/)\n> certainly is a workaround, but it does feel dirty.\n>\n> It is hard to imagine that we were the first people who wants to\n> split the value of a variable into a list, where the value is a list\n> of paths, concatenated into a single string with a delimiter that\n> may be platform specific. I wonder if we are going against a best\n> practice established in the Perl world, simply because we don't know\n> about it (i.e. basically, it would say \"don't split at a colon\n> because not all world is Unix; use $this_module instead\", similar to\n> \"don't split at a slash, use File::Spec instead to extract path\n> components\").\n>\n\nI thought there was a way to do this in File::Spec, but that's only\nfor splitting regular paths, and not for splitting a list of paths\nseparated by \":\" or \";\"\n\nWe probably should find a better solution to allow this to work with\nwindows style paths...? I know that python provides os.pathsep, but I\nhaven't seen an equivalent for perl yet.\n\nThe Env[1] core modules suggests using $Config::Config{path_sep}[2]..\nmaybe we should be using this?\n\nThanks,\nJake\n\n[1] https://perldoc.perl.org/Env.html\n[2] https://perldoc.perl.org/Config.html\n"},{"id":"331082","messageId":"CAGyf7-Ho3eY8sE2ME+p-JdR9OUQGFc43TmjHpueJgwCBSznN5w@mail.gmail.com","threadId":"46998","inReplyTo":"CA+P7+xpcC7Daw5k3=XFyZRsqiBib6yY2YzLpjYfcS7yqfUzTpg@mail.gmail.com","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2017-10-26T09:08:26Z","receivedAt":"2017-10-26T09:08:36Z","isPatch":true,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller wrote:\n> On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano wrote:\n>> Johannes Schindelin writes:\n>>\n>>> Note that the correct blib path starts with `C:\\BuildAgent\\_work` and\n>>> the line\n>>>\n>>> use lib (split(/:/, $ENV{GITPERLLIB}));\n>>>\n>>> splits off the drive letter from the rest of the path. Obviously, this\n>>> fails to Do The Right Thing, and simply points to Yet Another Portability\n>>> Problem with Git's reliance on Unix scripting.\n>>\n>> In our C code, we have \"#define PATH_SEP ';'\", and encourage our\n>> code to be careful and use it. Is there something similar for Perl\n>> scripts, I wonder.\n>>\n>> I notice that t/{t0202,t9000,t9700}/test.pl share the same\n>> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use\n>> the non-platform convention to accomodate the use of split(/:/)\n>> certainly is a workaround, but it does feel dirty.\n>>\n>> It is hard to imagine that we were the first people who wants to\n>> split the value of a variable into a list, where the value is a list\n>> of paths, concatenated into a single string with a delimiter that\n>> may be platform specific. I wonder if we are going against a best\n>> practice established in the Perl world, simply because we don't know\n>> about it (i.e. basically, it would say \"don't split at a colon\n>> because not all world is Unix; use $this_module instead\", similar to\n>> \"don't split at a slash, use File::Spec instead to extract path\n>> components\").\n>>\n>\n> I thought there was a way to do this in File::Spec, but that's only\n> for splitting regular paths, and not for splitting a list of paths\n> separated by \":\" or \";\"\n>\n> We probably should find a better solution to allow this to work with\n> windows style paths...? I know that python provides os.pathsep, but I\n> haven't seen an equivalent for perl yet.\n>\n> The Env[1] core modules suggests using $Config::Config{path_sep}[2]..\n> maybe we should be using this?\n>\n> Thanks,\n> Jake\n>\n> [1] https://perldoc.perl.org/Env.html\n> [2] https://perldoc.perl.org/Config.html\n"},{"id":"331083","messageId":"CAGyf7-EjKaHgwkN9trO4mFvba9odbWCzA9Jh0Pk6ZE6FOskOYg@mail.gmail.com","threadId":"46998","inReplyTo":"CA+P7+xpcC7Daw5k3=XFyZRsqiBib6yY2YzLpjYfcS7yqfUzTpg@mail.gmail.com","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2017-10-26T09:12:52Z","receivedAt":"2017-10-26T09:12:59Z","isPatch":true,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller wrote:\n> On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano wrote:\n>> Johannes Schindelin writes:\n>>\n>>> Note that the correct blib path starts with `C:\\BuildAgent\\_work` and\n>>> the line\n>>>\n>>> use lib (split(/:/, $ENV{GITPERLLIB}));\n>>>\n>>> splits off the drive letter from the rest of the path. Obviously, this\n>>> fails to Do The Right Thing, and simply points to Yet Another Portability\n>>> Problem with Git's reliance on Unix scripting.\n>>\n>> In our C code, we have \"#define PATH_SEP ';'\", and encourage our\n>> code to be careful and use it. Is there something similar for Perl\n>> scripts, I wonder.\n>>\n>> I notice that t/{t0202,t9000,t9700}/test.pl share the same\n>> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use\n>> the non-platform convention to accomodate the use of split(/:/)\n>> certainly is a workaround, but it does feel dirty.\n>>\n>> It is hard to imagine that we were the first people who wants to\n>> split the value of a variable into a list, where the value is a list\n>> of paths, concatenated into a single string with a delimiter that\n>> may be platform specific. I wonder if we are going against a best\n>> practice established in the Perl world, simply because we don't know\n>> about it (i.e. basically, it would say \"don't split at a colon\n>> because not all world is Unix; use $this_module instead\", similar to\n>> \"don't split at a slash, use File::Spec instead to extract path\n>> components\").\n>>\n>\n> I thought there was a way to do this in File::Spec, but that's only\n> for splitting regular paths, and not for splitting a list of paths\n> separated by \":\" or \";\"\n>\n> We probably should find a better solution to allow this to work with\n> windows style paths...? I know that python provides os.pathsep, but I\n> haven't seen an equivalent for perl yet.\n>\n> The Env[1] core modules suggests using $Config::Config{path_sep}[2]..\n> maybe we should be using this?\n\nI was testing this recently on the Perl included with Git for Windows\nand it returns : for the path separator even though it's on Windows,\nso I don't think that would work. The Perl in Git for Windows seems to\nwant UNIX-style inputs (something Dscho seemed to allude to in his\nresponse earlier.). I'm not sure why it's that way, but he probably\nknows.\n\nBryan\n\n(Pardon my previous blank message; Gmail fail.)\n\n>\n> Thanks,\n> Jake\n>\n> [1] https://perldoc.perl.org/Env.html\n> [2] https://perldoc.perl.org/Config.html\n"},{"id":"331136","messageId":"xmqqr2tpcn6g.fsf@gitster.mtv.corp.google.com","threadId":"46998","inReplyTo":"xmqqvaj8dlyz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-27T02:57:43Z","receivedAt":"2017-10-27T02:57:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> It is fine to leave the original code broken at this step while we\n> purely move the lines around, and hopefully this will be corrected\n> in a later step in the series (I am responding as I read on, so I do\n> not yet know at which patch that happens in this series).\n\nActually, I think you'd probably be better off if you fixed these\nbroken comparisions that does (@list1 eq @list2) very early in the\nseries, perhaps as [PATCH 0.8/6]. \n\nI am sure Perl experts among us on the list can come up with a\ncleaner and better implementation of compare_lists sub I am adding\nhere, but in the meantime, here is what I would start with if I were\nworking on this topic.\n\nThanks.\n\n t/t0021/rot13-filter.pl | 35 ++++++++++++++++++++++++++++-------\n 1 file changed, 28 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex ad685d92f8..9bf5a756af 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -107,21 +107,42 @@ sub packet_flush {\n \tSTDOUT->flush();\n }\n \n+sub compare_lists {\n+\tmy ($expect, @result) = @_;\n+\tmy $ix;\n+\tif (scalar @$expect != scalar @result) {\n+\t\treturn undef;\n+\t}\n+\tfor ($ix = 0; $ix < $#result; $ix++) {\n+\t\tif ($expect->[$ix] ne $result[$ix]) {\n+\t\t\treturn undef;\n+\t\t}\n+\t}\n+\treturn 1;\n+}\n+\n print $debug \"START\\n\";\n $debug->flush();\n \n-( packet_txt_read() eq ( 0, \"git-filter-client\" ) ) || die \"bad initialize\";\n-( packet_txt_read() eq ( 0, \"version=2\" ) ) || die \"bad version\";\n-( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad version end\";\n+compare_lists([0, \"git-filter-client\"], packet_txt_read()) ||\n+\tdie \"bad initialize\";\n+compare_lists([0, \"version=2\"], packet_txt_read()) ||\n+\tdie \"bad version\";\n+compare_lists([1, \"\"], packet_bin_read()) ||\n+\tdie \"bad version end\";\n \n packet_txt_write(\"git-filter-server\");\n packet_txt_write(\"version=2\");\n packet_flush();\n \n-( packet_txt_read() eq ( 0, \"capability=clean\" ) ) || die \"bad capability\";\n-( packet_txt_read() eq ( 0, \"capability=smudge\" ) ) || die \"bad capability\";\n-( packet_txt_read() eq ( 0, \"capability=delay\" ) ) || die \"bad capability\";\n-( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad capability end\";\n+compare_lists([0, \"capability=clean\"], packet_txt_read()) ||\n+\tdie \"bad capability\";\n+compare_lists([0, \"capability=smudge\"], packet_txt_read()) ||\n+\tdie \"bad capability\";\n+compare_lists([0, \"capability=delay\"], packet_txt_read()) ||\n+\tdie \"bad capability\";\n+compare_lists([1, \"\"], packet_bin_read()) ||\n+\tdie \"bad capability end\";\n \n foreach (@capabilities) {\n \tpacket_txt_write( \"capability=\" . $_ );\n"},{"id":"331139","messageId":"CAP8UFD34O7RyXo0QErrQ0A-gbKRy8m6ChP_osQm-SdmD3qtx9g@mail.gmail.com","threadId":"46998","inReplyTo":"xmqqr2tpcn6g.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-27T05:07:42Z","receivedAt":"2017-10-27T05:07:48Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Oct 27, 2017 at 4:57 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> It is fine to leave the original code broken at this step while we\n>> purely move the lines around, and hopefully this will be corrected\n>> in a later step in the series (I am responding as I read on, so I do\n>> not yet know at which patch that happens in this series).\n>\n> Actually, I think you'd probably be better off if you fixed these\n> broken comparisions that does (@list1 eq @list2) very early in the\n> series, perhaps as [PATCH 0.8/6].\n\nYeah, it's better to fix the comparisons first.\n\n> I am sure Perl experts among us on the list can come up with a\n> cleaner and better implementation of compare_lists sub I am adding\n> here, but in the meantime, here is what I would start with if I were\n> working on this topic.\n\nThanks for the patch,\nChristian.\n"},{"id":"331156","messageId":"alpine.DEB.2.21.1.1710271626410.6482@virtualbox","threadId":"46998","inReplyTo":"xmqq4lqmfoy7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-27T15:05:22Z","receivedAt":"2017-10-27T15:06:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 26 Oct 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Note that the correct blib path starts with `C:\\BuildAgent\\_work` and\n> > the line\n> >\n> > \tuse lib (split(/:/, $ENV{GITPERLLIB}));\n> >\n> > splits off the drive letter from the rest of the path. Obviously, this\n> > fails to Do The Right Thing, and simply points to Yet Another Portability\n> > Problem with Git's reliance on Unix scripting.\n> \n> In our C code, we have \"#define PATH_SEP ';'\", and encourage our\n> code to be careful and use it. Is there something similar for Perl\n> scripts, I wonder.\n> \n> I notice that t/{t0202,t9000,t9700}/test.pl share the same\n> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use\n> the non-platform convention to accomodate the use of split(/:/)\n> certainly is a workaround, but it does feel dirty.\n\nIt is not only dirty, it *still* does not work with that workaround: Note\nthat GITPERLLIB is *also* set in the bin-wrappers/*...\n\nAnd even then, it does not work: somewhere along the way, the path is\nconverted to a Windows path with backslashes, and for some reason MSYS2\nPerl fails to handle that.\n\nThe best workaround I found in Git's source code (yes, it took me 2h to\ninvestigate this littly problem, but at least I got an in-depth\nunderstanding of the issue) was to pass BLIB_DIR instead, and not perform\na split but generate the two paths to include in Perl instead. Of course,\nthat would break out-of-tree usage of GITPERLLIB...\n\nThat's why I settled on the out-of-tree workaround that Windows\ncontributors will have to somehow figure out (as if it was not hard enough\nalready to contribute to Git for Windows...).\n\n> It is hard to imagine that we were the first people who wants to\n> split the value of a variable into a list, where the value is a list\n> of paths, concatenated into a single string with a delimiter that\n> may be platform specific.\n\nNo, the test suite has plenty of use cases for that. It usually works.\n\nThe problem is that t0021 contains very complex code that goes back and\nforth between the C layer and the scripted layer. At one stage, the\npseudo-Unix paths are converted to Windows paths, with drive prefix and\nbackslashes, separated by semicolons. And somewhere along the lines, this\ncannot be converted back.\n\nI *think* that it happens when the bin-wrapper for git.exe is executed\nfrom inside Git itself, or some such.\n\n> I wonder if we are going against a best practice established in the Perl\n> world, simply because we don't know about it (i.e. basically, it would\n> say \"don't split at a colon because not all world is Unix; use\n> $this_module instead\", similar to \"don't split at a slash, use\n> File::Spec instead to extract path components\").\n\nWe go against best practices by having crucial parts of Git implemented as\nPerl scripts. But you knew that ;-)\n\nCiao,\nDscho\n"},{"id":"331166","messageId":"alpine.DEB.2.21.1.1710271707290.6482@virtualbox","threadId":"46998","inReplyTo":"CAGyf7-EjKaHgwkN9trO4mFvba9odbWCzA9Jh0Pk6ZE6FOskOYg@mail.gmail.com","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-27T15:09:12Z","receivedAt":"2017-10-27T15:11:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Bryan,\n\nOn Thu, 26 Oct 2017, Bryan Turner wrote:\n\n> On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller wrote:\n> > On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano wrote:\n> >> Johannes Schindelin writes:\n> >>\n> >>> Note that the correct blib path starts with `C:\\BuildAgent\\_work` and\n> >>> the line\n> >>>\n> >>> use lib (split(/:/, $ENV{GITPERLLIB}));\n> >>>\n> >>> splits off the drive letter from the rest of the path. Obviously, this\n> >>> fails to Do The Right Thing, and simply points to Yet Another Portability\n> >>> Problem with Git's reliance on Unix scripting.\n> >>\n> >> In our C code, we have \"#define PATH_SEP ';'\", and encourage our\n> >> code to be careful and use it. Is there something similar for Perl\n> >> scripts, I wonder.\n> >>\n> >> I notice that t/{t0202,t9000,t9700}/test.pl share the same\n> >> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use\n> >> the non-platform convention to accomodate the use of split(/:/)\n> >> certainly is a workaround, but it does feel dirty.\n> >>\n> >> It is hard to imagine that we were the first people who wants to\n> >> split the value of a variable into a list, where the value is a list\n> >> of paths, concatenated into a single string with a delimiter that\n> >> may be platform specific. I wonder if we are going against a best\n> >> practice established in the Perl world, simply because we don't know\n> >> about it (i.e. basically, it would say \"don't split at a colon\n> >> because not all world is Unix; use $this_module instead\", similar to\n> >> \"don't split at a slash, use File::Spec instead to extract path\n> >> components\").\n> >>\n> >\n> > I thought there was a way to do this in File::Spec, but that's only\n> > for splitting regular paths, and not for splitting a list of paths\n> > separated by \":\" or \";\"\n> >\n> > We probably should find a better solution to allow this to work with\n> > windows style paths...? I know that python provides os.pathsep, but I\n> > haven't seen an equivalent for perl yet.\n> >\n> > The Env[1] core modules suggests using $Config::Config{path_sep}[2]..\n> > maybe we should be using this?\n> \n> I was testing this recently on the Perl included with Git for Windows\n> and it returns : for the path separator even though it's on Windows,\n> so I don't think that would work. The Perl in Git for Windows seems to\n> want UNIX-style inputs (something Dscho seemed to allude to in his\n> response earlier.). I'm not sure why it's that way, but he probably\n> knows.\n\nMSYS2 Perl is essentially Cygwin's Perl ported over to MSYS2. And Cygwin\ntries to keep everything as pseudo Unix as possible, to make it easier to\nport software (if you think Git's source code is the only source code\nwoefully unprepared for semicolons as path separators, you just need to\nbuy me a few beers to hear plenty of war stories).\n\nCiao,\nDscho\n"},{"id":"331222","messageId":"09393EDB-19A3-4244-A88F-68EDA8470898@gmail.com","threadId":"46998","inReplyTo":"xmqqr2tpcn6g.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-28T14:59:00Z","receivedAt":"2017-10-28T14:59:08Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 27 Oct 2017, at 04:57, Junio C Hamano wrote:\n> \n> Junio C Hamano writes:\n> \n>> It is fine to leave the original code broken at this step while we\n>> purely move the lines around, and hopefully this will be corrected\n>> in a later step in the series (I am responding as I read on, so I do\n>> not yet know at which patch that happens in this series).\n> \n> Actually, I think you'd probably be better off if you fixed these\n> broken comparisions that does (@list1 eq @list2) very early in the\n> series, perhaps as [PATCH 0.8/6]. \n> \n> I am sure Perl experts among us on the list can come up with a\n> cleaner and better implementation of compare_lists sub I am adding\n> here, but in the meantime, here is what I would start with if I were\n> working on this topic.\n> \n\nThanks for spotting this! I had some trouble applying the patch in\nthis email. I got an error about \"corrupt patch at line 54\"\n\nBTW: I am using this little snippet to apply patches from the mailing:\n\n PATCH=$(curl -L --silent https://public-inbox.org/git/xmqqr2tpcn6g.fsf@gitster.mtv.corp.google.com/raw); \n ((printf '%s' \"$PATCH\" | git am -3) || (git am --abort; printf '%s' \"$PATCH\" | git apply)) && \n echo && echo \"Patch successfully applied\"\n\nDoes this look sensible to you?\n\nI applied the patch \"manually\" and the result looks good to me.\nThe patch looks good to me, too, but I am by no means a Perl\nexpert.\n\nThanks,\nLars\n\n\n\n\n> Thanks.\n> \n> t/t0021/rot13-filter.pl | 35 ++++++++++++++++++++++++++++-------\n> 1 file changed, 28 insertions(+), 7 deletions(-)\n> \n> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\n> index ad685d92f8..9bf5a756af 100644\n> --- a/t/t0021/rot13-filter.pl\n> +++ b/t/t0021/rot13-filter.pl\n> @@ -107,21 +107,42 @@ sub packet_flush {\n> \tSTDOUT->flush();\n> }\n> \n> +sub compare_lists {\n> +\tmy ($expect, @result) = @_;\n> +\tmy $ix;\n> +\tif (scalar @$expect != scalar @result) {\n> +\t\treturn undef;\n> +\t}\n> +\tfor ($ix = 0; $ix < $#result; $ix++) {\n> +\t\tif ($expect->[$ix] ne $result[$ix]) {\n> +\t\t\treturn undef;\n> +\t\t}\n> +\t}\n> +\treturn 1;\n> +}\n> +\n> print $debug \"START\\n\";\n> $debug->flush();\n> \n> -( packet_txt_read() eq ( 0, \"git-filter-client\" ) ) || die \"bad initialize\";\n> -( packet_txt_read() eq ( 0, \"version=2\" ) ) || die \"bad version\";\n> -( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad version end\";\n> +compare_lists([0, \"git-filter-client\"], packet_txt_read()) ||\n> +\tdie \"bad initialize\";\n> +compare_lists([0, \"version=2\"], packet_txt_read()) ||\n> +\tdie \"bad version\";\n> +compare_lists([1, \"\"], packet_bin_read()) ||\n> +\tdie \"bad version end\";\n> \n> packet_txt_write(\"git-filter-server\");\n> packet_txt_write(\"version=2\");\n> packet_flush();\n> \n> -( packet_txt_read() eq ( 0, \"capability=clean\" ) ) || die \"bad capability\";\n> -( packet_txt_read() eq ( 0, \"capability=smudge\" ) ) || die \"bad capability\";\n> -( packet_txt_read() eq ( 0, \"capability=delay\" ) ) || die \"bad capability\";\n> -( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad capability end\";\n> +compare_lists([0, \"capability=clean\"], packet_txt_read()) ||\n> +\tdie \"bad capability\";\n> +compare_lists([0, \"capability=smudge\"], packet_txt_read()) ||\n> +\tdie \"bad capability\";\n> +compare_lists([0, \"capability=delay\"], packet_txt_read()) ||\n> +\tdie \"bad capability\";\n> +compare_lists([1, \"\"], packet_bin_read()) ||\n> +\tdie \"bad capability end\";\n> \n> foreach (@capabilities) {\n> \tpacket_txt_write( \"capability=\" . $_ );\n\n"},{"id":"331249","messageId":"xmqqfua2byje.fsf@gitster.mtv.corp.google.com","threadId":"46998","inReplyTo":"09393EDB-19A3-4244-A88F-68EDA8470898@gmail.com","subject":"Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-29T00:14:29Z","receivedAt":"2017-10-29T00:14:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> BTW: I am using this little snippet to apply patches from the mailing:\n>\n> PATCH=$(curl -L --silent https://public-inbox.org/git/xmqqr2tpcn6g.fsf@gitster.mtv.corp.google.com/raw); \n> ((printf '%s' \"$PATCH\" | git am -3) || (git am --abort; printf '%s' \"$PATCH\" | git apply)) && \n> echo && echo \"Patch successfully applied\"\n>\n> Does this look sensible to you?\n\nSensible? I have no answer. I wouldn't trust printf well enough to\nstuff large value in a shell variable and feeding it myself in the\nfirst place, but I would not be surprised if you did\n\n\tVAR=$(command) && printf '%s' \"$VAR\" >output\n\nand ended up with a file with an incomplete line at the end.\n\n"},{"id":"331300","messageId":"xmqqvaix8o6k.fsf@gitster.mtv.corp.google.com","threadId":"46998","inReplyTo":"20171019123030.17338-1-chriscool@tuxfamily.org","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-30T00:38:43Z","receivedAt":"2017-10-30T00:38:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I've queued this from Dscho; please take it into consideration when\nyou reroll.\n\nThanks.\n\n-- >8 --\nFrom: Johannes Schindelin \nDate: Sun, 29 Oct 2017 16:17:06 +0100\nSubject: [PATCH] fixup! Git/Packet.pm: extract parts of t0021/rot13-filter.pl\n for reuse\n\nThe patch introducing Git/Packet.pm forgot the NO_PERL_MAKEMAKER part.\nBreaking the test suite on Windows.\n\nSigned-off-by: Johannes Schindelin \n---\n perl/Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/perl/Makefile b/perl/Makefile\nindex 15d96fcc7a..4a74a493e6 100644\n--- a/perl/Makefile\n+++ b/perl/Makefile\n@@ -37,6 +37,7 @@ modules += Git/SVN/Editor\n modules += Git/SVN/GlobSpec\n modules += Git/SVN/Log\n modules += Git/SVN/Migration\n+modules += Git/SVN/Packet\n modules += Git/SVN/Prompt\n modules += Git/SVN/Ra\n modules += Git/SVN/Utils\n-- \n2.15.0-rc2-267-g7d3ed0014a\n\n"},{"id":"331330","messageId":"CAP8UFD3qjmSrCJ1OE6x=sitbm7LqRB3aSm42aV4h=gAG853PhA@mail.gmail.com","threadId":"46998","inReplyTo":"xmqqvaix8o6k.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-10-30T06:18:41Z","receivedAt":"2017-10-30T06:18:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Mon, Oct 30, 2017 at 1:38 AM, Junio C Hamano wrote:\n> I've queued this from Dscho; please take it into consideration when\n> you reroll.\n\nYeah, I was planning to add something like that, though in Dscho's\nfirst email the patch was adding:\n\n+modules += Git/Packet\n\nand now it's adding:\n\n> +modules += Git/SVN/Packet\n\nI wonder where the \"SVN\" directory comes from as in\nhttps://github.com/dscho/git/commits/cc/git-packet-pm my commit\n(https://github.com/dscho/git/commit/46032e5f55f6e87d22f9a3c952b5d48a5f100a86)\nstill creates perl/Git/Packet.pm and not perl/Git/SVN/Packet.pm\n\nThanks both anyway,\nChristian.\n"},{"id":"331347","messageId":"alpine.DEB.2.21.1.1710301336270.6482@virtualbox","threadId":"46998","inReplyTo":"CAP8UFD3qjmSrCJ1OE6x=sitbm7LqRB3aSm42aV4h=gAG853PhA@mail.gmail.com","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-30T12:37:29Z","receivedAt":"2017-10-30T12:38:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian & Junio,\n\nOn Mon, 30 Oct 2017, Christian Couder wrote:\n\n> On Mon, Oct 30, 2017 at 1:38 AM, Junio C Hamano wrote:\n> > I've queued this from Dscho; please take it into consideration when\n> > you reroll.\n> \n> Yeah, I was planning to add something like that, though in Dscho's\n> first email the patch was adding:\n> \n> +modules += Git/Packet\n> \n> and now it's adding:\n> \n> > +modules += Git/SVN/Packet\n\nBah. I should have paid more attention. The original Git/Packet is\ncorrect, of course. My fixup! commit is bogus.\n\nSorry for the confusion,\nDscho\n"},{"id":"331400","messageId":"20171030180858.e6kp5uwac3kkoaae@sigill.intra.peff.net","threadId":"46998","inReplyTo":"8F505EA8D16F4A0F890AE9DEEB91434B@PhilipOakley","subject":"Re: [PATCH 0/6] Create Git/Packet.pm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-30T18:08:58Z","receivedAt":"2017-10-30T18:09:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 23, 2017 at 01:26:41PM +0100, Philip Oakley wrote:\n\n> > Totally offtopic, but is it only me who finds these \"section\n> > headers\" in cover letters from some people irritating and/or\n> > jarring?\n> \n> Personally I find that, for significant patch series, that clearly breaking\n> out these distinct sections is of advantage. At this stage (the very first\n> patch 0/n) there is no specific conversation, so the subject line is a short\n> 'hello' to the topic, and then the contributor is (it is to be hoped)\n> setting out their proposal in a clear manner.\n> \n> So I do like these headings for larger series, though there is some\n> judgement to be made as to when the subject line alone is sufficient.\n\nI can live with fancily-formatted cover letters. BUT. I would say if\nyour cover letter is getting quite long, you might consider whether some\nof its content ought to be going elsewhere (either into commit messages\nthemselves, or into a design document or other place inside the repo).\n\n> As a separate follow on, one thing that does annoy me is that in subsequent\n> versions of the various patch series, folk tend to drop all explanation of\n> why the series is of any relevance, leaving just the 'changed since last\n> time' part. This means that new readers who try and pick up / review /\n> contribute to a series later on in its development are not told the purpose.\n> When the list is active it can, accidentally, do a disservice to the\n> potential contributors who may feel that only core contributors are able to\n> contribute.\n\nI actually have the opposite opinion. I find it annoying to have to wade\nthrough the same unchanged content for each round just to find the\nlittle snippet of \"here's what's changed\".\n\nI don't mind following a link to the previous iteration to read the\nback-story if I wasn't involved (it's a good idea to do that anyway to\nsee what previous reviews have already discussed).\n\nI do often just post my \"v2\" as a follow-up and assume people can find\nthe original by following the thread backwards. But I imagine that not\neverybody can do so. It's probably a good practice to at least put a\nlink to the prior version (and also to v1 for the original motivation)\nif you're not going to repeat the cover letter in full.\n\n-Peff\n"},{"id":"331817","messageId":"CAP8UFD0MppGwD5iXNjs8y+qxpFGc2NbYE9gcqe2pmJWAt6CZfg@mail.gmail.com","threadId":"46998","inReplyTo":"xmqqefpwdkf9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/6] t0021/rot13-filter: add capability functions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-11-04T08:38:58Z","receivedAt":"2017-11-04T08:39:07Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Oct 22, 2017 at 3:46 AM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> Add functions to help read and write capabilities.\n>> These functions will be reused in following patches.\n>\n> One more thing that is more noteworthy (read: do not forget to\n> describe it in the proposed log message) is that the original used\n> to require capabilities to come in a fixed order.\n>\n> The new code allows these capabilities to be declared in any order,\n\nYeah and I think it is good.\n\n> it even allows duplicates (intended? shouldn't we be flagging it as\n> an error?),\n\nI think allowing duplicates is ok, as we allow duplicates in many\ncases already, like duplicate command line options.\nOr perhaps we should just warn?\n\n> the helper can require a set of capabilities we do want\n> to see and fail if the remote doesn't declare any one of them\n> (good).\n\nYeah.\n\n> It does not check if the remote declares any capability we do not\n> know about (intended? the original noticed this situation and error\n> out---shouldn't the more generalized helper that is meant to be\n> reusable allow us to do so, too?).\n\nI think that it is ok in general to just ignore capabilities we don't\nknow (as long as we don't advertise them). The protocol should work\nusing only the capabilities that both ends support.\n\nNow if we just talk about testing, we might sometimes want to check\nthat one end sends only some specific capabilities. So in this case it\ncould be good if we could error out. On the other hand, if we test how\nGit behaves when we advertise some specific capabilities, it might not\nbe a good idea to error out as it would break tests when Git learns a\nnew capability and advertise it.\n\nIn the specific case of rot13-filter.pl I think we are more in the\nlater case than in the former.\n\nSo I think it is ok to wait until we would really want to check that\none end sends only some specific capabilities, before we improve the\nPacket.pm module to make it support that.\n\n> Side note: my answer to the last question is \"it is OK and\n> even desirable to ignore the fact that they claim to support\n> a capability we do not know about\", but I may be mistaken.\n\nYeah I agree.\n\n> The reasoning and the conclusion that is consistent with\n> what the code does should be described in any case.\n\nOk I will document all the above in the commit message.\n\n>> +sub packet_read_capabilities {\n>> + my @cap;\n>> + while (1) {\n>> + my ( $res, $buf ) = packet_bin_read();\n>> + return ( $res, @cap ) if ( $res != 0 );\n>\n> The original had the same \"'list eq list' does not do what you may\n> think it does\" issue. This one corrects it, which is good.\n>\n> I am not sure if ($res != 0) is correct though. What should happen\n> when you get an unexpected EOF at this point? The original would\n> have died; this ignores and continues.\n\nWell if there is an unexpected EOF, then packet_bin_read() returns\n(-1, \"\"), so packet_read_capabilities() returns (-1, @cap) where @cap\ncontains the capabilities already received. Then\npacket_read_and_check_capabilities() checks that we received all the\ncapabilities we expect and dies if that is not the case. If we did\nreceive all the capabilities, then\npacket_read_and_check_capabilities() still returns -1, so the caller\nmay check that and die.\n\nBut yeah we could also just die in packet_read_capabilities() if $res\nis -1. I will make this change.\n\n>> + unless ( $buf =~ s/\\n$// ) {\n>> + die \"A non-binary line MUST be terminated by an LF.\\n\"\n>> + . \"Received: '$buf'\";\n>> + }\n>\n> It may make sense to extract this in a small helper and call it from\n> here and from packet_txt_read().\n\nOk, I have done this in my current version, that I plan to send soon.\n\n>> + die \"bad capability buf: '$buf'\" unless ( $buf =~ s/capability=// );\n>\n> This may merely be a style thing, but I somehow find statement\n> modifiers hard to follow, unless its condition is almost always\n> true. If you follow the logic in a loop and see \"die\" at the\n> beginning, a normal thing to expect is that there were conditionals\n> that said \"continue\" (eh, 'next' or 'redo') to catch the normal case\n> and the control would reach \"die\" only under exceptional error\n> cases, but hiding a rare error condition after 'unless' statement\n> modifier breaks that expectation.\n\nOk, I will use:\n\nunless ( $buf =~ s/capability=// ) {\n die \"bad capability buf: '$buf'\" ;\n}\n\n>> + push @cap, $buf;\n>> + }\n>> +}\n>> +\n>> +sub packet_read_and_check_capabilities {\n>> + my @local_caps = @_;\n>> + my @remote_res_caps = packet_read_capabilities();\n>> + my $res = shift @remote_res_caps;\n>> + my %remote_caps = map { $_ => 1 } @remote_res_caps;\n>\n> FYI:\n>\n> my ($res, @remote_caps) = packet_read_capabilities();\n> my %remote_caps = map { $_ => 1 } @remote_caps;\n>\n> may be more conventional way.\n\nYeah I will use what you suggest.\n\n>> + foreach (@local_caps) {\n>> + die \"'$_' capability not available\" unless (exists($remote_caps{$_}));\n>> + }\n>\n> It is good that we can now accept capabilities in any order and\n> still enforce all the required capabilities are supported by the\n> other side. It deserves a mention in the proposed log message.\n\nOk, will add one.\n\n>> + return $res;\n>> +}\n>> +\n>> +sub packet_write_capabilities {\n>> + packet_txt_write( \"capability=\" . $_ ) foreach (@_);\n>> + packet_flush();\n>> +}\n>> +\n>> print $debug \"START\\n\";\n>> $debug->flush();\n>>\n>> packet_initialize(\"git-filter\", 2);\n>>\n>> -( packet_txt_read() eq ( 0, \"capability=clean\" ) ) || die \"bad capability\";\n>> -( packet_txt_read() eq ( 0, \"capability=smudge\" ) ) || die \"bad capability\";\n>> -( packet_txt_read() eq ( 0, \"capability=delay\" ) ) || die \"bad capability\";\n>> -( packet_bin_read() eq ( 1, \"\" ) ) || die \"bad capability end\";\n>> +packet_read_and_check_capabilities(\"clean\", \"smudge\", \"delay\");\n>> +packet_write_capabilities(@capabilities);\n>\n> Neither the original nor the rewrite ensures that @capabilities we\n> ask to the other side to activate is a subset of what the other side\n> actually declared.\n>\n> Fixing this is a bit more involved than \"refactor what we have\", but\n> probably is part of \"refactor so that we can reuse in other\n> situations\". You'd want to return the list of caps received from\n> packet_read_and_check_capabilities() and make sure @capabilities is\n> a subset of that before writing them out to the other side to\n> request.\n\nOk, I will add that too.\n\nThanks.\n"},{"id":"331837","messageId":"xmqqwp35lbx3.fsf@gitster.mtv.corp.google.com","threadId":"46998","inReplyTo":"CAP8UFD0MppGwD5iXNjs8y+qxpFGc2NbYE9gcqe2pmJWAt6CZfg@mail.gmail.com","subject":"Re: [PATCH 5/6] t0021/rot13-filter: add capability functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-05T02:03:20Z","receivedAt":"2017-11-05T02:03:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>>> + my ( $res, $buf ) = packet_bin_read();\n>>> + return ( $res, @cap ) if ( $res != 0 );\n>>\n>> The original had the same \"'list eq list' does not do what you may\n>> think it does\" issue. This one corrects it, which is good.\n>>\n>> I am not sure if ($res != 0) is correct though. What should happen\n>> when you get an unexpected EOF at this point? The original would\n>> have died; this ignores and continues.\n>\n> Well if there is an unexpected EOF, then packet_bin_read() returns\n> (-1, \"\"), so packet_read_capabilities() returns (-1, @cap) where @cap\n> contains the capabilities already received. Then\n> packet_read_and_check_capabilities() checks that we received all the\n> capabilities we expect and dies if that is not the case. If we did\n> receive all the capabilities, then\n> packet_read_and_check_capabilities() still returns -1, so the caller\n> may check that and die.\n\nIn other words, it happens, by accident, to stop before going very\nfar. I think we'd be better off making it an explicitly checked\nerror.\n\n> But yeah we could also just die in packet_read_capabilities() if $res\n> is -1. I will make this change.\n\nGood.\n\nThanks.\n"},{"id":"331866","messageId":"CAP8UFD2VxrhhJhiZLPFz6JDGVjuUdyAHTB8c++eNeoxVMAhhyw@mail.gmail.com","threadId":"46998","inReplyTo":"xmqq4lqsf17n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-11-05T12:50:38Z","receivedAt":"2017-11-05T12:50:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Oct 22, 2017 at 2:58 AM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> To make it possible in a following commit to move packet\n>> reading and writing functions into a Packet.pm module,\n>> let's refactor these functions, so they don't handle\n>> printing debug output and exiting.\n>>\n>> Signed-off-by: Christian Couder \n>> ---\n>> t/t0021/rot13-filter.pl | 12 ++++++++----\n>> 1 file changed, 8 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\n>> index ad685d92f8..e4495a52f3 100644\n>> --- a/t/t0021/rot13-filter.pl\n>> +++ b/t/t0021/rot13-filter.pl\n>> @@ -60,8 +60,7 @@ sub packet_bin_read {\n>> my $bytes_read = read STDIN, $buffer, 4;\n>> if ( $bytes_read == 0 ) {\n>> # EOF - Git stopped talking to us!\n>> - print $debug \"STOP\\n\";\n>> - exit();\n>> + return ( -1, \"\" );\n>> }\n>> elsif ( $bytes_read != 4 ) {\n>> die \"invalid packet: '$buffer'\";\n>> @@ -85,7 +84,7 @@ sub packet_bin_read {\n>>\n>> sub packet_txt_read {\n>> my ( $res, $buf ) = packet_bin_read();\n>> - unless ( $buf eq '' or $buf =~ s/\\n$// ) {\n>> + unless ( $res == -1 or $buf eq '' or $buf =~ s/\\n$// ) {\n>> die \"A non-binary line MUST be terminated by an LF.\";\n>> }\n>> return ( $res, $buf );\n>> @@ -131,7 +130,12 @@ print $debug \"init handshake complete\\n\";\n>> $debug->flush();\n>>\n>> while (1) {\n>> - my ( $command ) = packet_txt_read() =~ /^command=(.+)$/;\n>> + my ( $res, $command ) = packet_txt_read();\n>> + if ( $res == -1 ) {\n>> + print $debug \"STOP\\n\";\n>> + exit();\n>> + }\n>> + $command =~ s/^command=//;\n>> print $debug \"IN: $command\";\n>> $debug->flush();\n>\n> This was not an issue in the old code which died upon unexpected EOF\n> inside the lowest-level helper packet_bin_read(), but now you have\n> one call to packet_bin_read() and many calls to packet_txt_read()\n> whose return value is not checked for this new condition you are\n> allowing packet_bin_read() to return. This step taken alone is a\n> regression---let's see how the remainder of the series updates the\n> callers to compensate.\n\nYeah, in the new version I will send really soon now, I have made a\nnumber of changes to check the return value for the EOF condition.\n\n> I initially thought that it may be more Perl-ish to return undef or\n> string instead of returning a 2-element list, but this code needs to\n> distinguish three conditions (i.e. a normal string that is 0 or more\n> bytes long, a flush, and an EOF), so that is not sufficient. Perl\n> experts on the list still may be able to suggest a better way than\n> the current one to do so, but that is outside the scope of this\n> refactoring.\n\nYeah I can't think of a better way either.\n\nThanks.\n"}]}