[PATCH 0/6] Create Git/Packet.pm

[PATCH 0/6] Create Git/Packet.pm

[Christian Couder]

Goal
~~~~

Packet related functions in Perl can be useful to write new filters or
to debug or test existing filters. So instead of having them in
t0021/rot13-filter.pl, let's extract them into a new Git/Packet.pm
module.

Links
~~~~~

This patch series has been extracted from previous "Add initial
experimental external ODB support" patch series.

Version 1, 2, 3, 4, 5 and 6 of this previous series are on the mailing
list here:

https://public-inbox.org/git/20160613085546.11784-1-chriscool@tuxfamily.org/
https://public-inbox.org/git/20160628181933.24620-1-chriscool@tuxfamily.org/
https://public-inbox.org/git/20161130210420.15982-1-chriscool@tuxfamily.org/
https://public-inbox.org/git/20170620075523.26961-1-chriscool@tuxfamily.org/
https://public-inbox.org/git/20170803091926.1755-1-chriscool@tuxfamily.org/
https://public-inbox.org/git/20170916080731.13925-1-chriscool@tuxfamily.org/

They are also available in the following branches:

https://github.com/chriscool/git/commits/gl-external-odb12
https://github.com/chriscool/git/commits/gl-external-odb22
https://github.com/chriscool/git/commits/gl-external-odb61
https://github.com/chriscool/git/commits/gl-external-odb239
https://github.com/chriscool/git/commits/gl-external-odb373
https://github.com/chriscool/git/commits/gl-external-odb411


Christian Couder (6):
  t0021/rot13-filter: refactor packet reading functions
  t0021/rot13-filter: improve 'if .. elsif .. else' style
  t0021/rot13-filter: improve error message
  t0021/rot13-filter: add packet_initialize()
  t0021/rot13-filter: add capability functions
  Add Git/Packet.pm from parts of t0021/rot13-filter.pl

 perl/Git/Packet.pm      | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
 t/t0021/rot13-filter.pl | 110 +++++++++-----------------------------------
 2 files changed, 140 insertions(+), 88 deletions(-)
 create mode 100644 perl/Git/Packet.pm

-- 
2.15.0.rc1.106.g7e97f58a41

[PATCH 1/6] t0021/rot13-filter: refactor packet reading functions

[Christian Couder]

To make it possible in a following commit to move packet
reading and writing functions into a Packet.pm module,
let's refactor these functions, so they don't handle
printing debug output and exiting.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t0021/rot13-filter.pl | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index ad685d92f8..e4495a52f3 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -60,8 +60,7 @@ sub packet_bin_read {
 	my $bytes_read = read STDIN, $buffer, 4;
 	if ( $bytes_read == 0 ) {
 		# EOF - Git stopped talking to us!
-		print $debug "STOP\n";
-		exit();
+		return ( -1, "" );
 	}
 	elsif ( $bytes_read != 4 ) {
 		die "invalid packet: '$buffer'";
@@ -85,7 +84,7 @@ sub packet_bin_read {
 
 sub packet_txt_read {
 	my ( $res, $buf ) = packet_bin_read();
-	unless ( $buf eq '' or $buf =~ s/\n$// ) {
+	unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
 		die "A non-binary line MUST be terminated by an LF.";
 	}
 	return ( $res, $buf );
@@ -131,7 +130,12 @@ print $debug "init handshake complete\n";
 $debug->flush();
 
 while (1) {
-	my ( $command ) = packet_txt_read() =~ /^command=(.+)$/;
+	my ( $res, $command ) = packet_txt_read();
+	if ( $res == -1 ) {
+		print $debug "STOP\n";
+		exit();
+	}
+	$command =~ s/^command=//;
 	print $debug "IN: $command";
 	$debug->flush();
 
-- 
2.15.0.rc1.106.g7e97f58a41

[PATCH 2/6] t0021/rot13-filter: improve 'if .. elsif .. else' style

[Christian Couder]

Before further refactoring the "t0021/rot13-filter.pl" script,
let's modernize the style of its 'if .. elsif .. else' clauses
to improve its readability by making it more similar to our
other perl scripts.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t0021/rot13-filter.pl | 39 +++++++++++++--------------------------
 1 file changed, 13 insertions(+), 26 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index e4495a52f3..82882392ae 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -61,23 +61,20 @@ sub packet_bin_read {
 	if ( $bytes_read == 0 ) {
 		# EOF - Git stopped talking to us!
 		return ( -1, "" );
-	}
-	elsif ( $bytes_read != 4 ) {
+	} elsif ( $bytes_read != 4 ) {
 		die "invalid packet: '$buffer'";
 	}
 	my $pkt_size = hex($buffer);
 	if ( $pkt_size == 0 ) {
 		return ( 1, "" );
-	}
-	elsif ( $pkt_size > 4 ) {
+	} elsif ( $pkt_size > 4 ) {
 		my $content_size = $pkt_size - 4;
 		$bytes_read = read STDIN, $buffer, $content_size;
 		if ( $bytes_read != $content_size ) {
 			die "invalid packet ($content_size bytes expected; $bytes_read bytes read)";
 		}
 		return ( 0, $buffer );
-	}
-	else {
+	} else {
 		die "invalid packet size: $pkt_size";
 	}
 }
@@ -165,8 +162,7 @@ while (1) {
 		$debug->flush();
 		packet_txt_write("status=success");
 		packet_flush();
-	}
-	else {
+	} else {
 		my ( $pathname ) = packet_txt_read() =~ /^pathname=(.+)$/;
 		print $debug " $pathname";
 		$debug->flush();
@@ -205,17 +201,13 @@ while (1) {
 		my $output;
 		if ( exists $DELAY{$pathname} and exists $DELAY{$pathname}{"output"} ) {
 			$output = $DELAY{$pathname}{"output"}
-		}
-		elsif ( $pathname eq "error.r" or $pathname eq "abort.r" ) {
+		} elsif ( $pathname eq "error.r" or $pathname eq "abort.r" ) {
 			$output = "";
-		}
-		elsif ( $command eq "clean" and grep( /^clean$/, @capabilities ) ) {
+		} elsif ( $command eq "clean" and grep( /^clean$/, @capabilities ) ) {
 			$output = rot13($input);
-		}
-		elsif ( $command eq "smudge" and grep( /^smudge$/, @capabilities ) ) {
+		} elsif ( $command eq "smudge" and grep( /^smudge$/, @capabilities ) ) {
 			$output = rot13($input);
-		}
-		else {
+		} else {
 			die "bad command '$command'";
 		}
 
@@ -224,25 +216,21 @@ while (1) {
 			$debug->flush();
 			packet_txt_write("status=error");
 			packet_flush();
-		}
-		elsif ( $pathname eq "abort.r" ) {
+		} elsif ( $pathname eq "abort.r" ) {
 			print $debug "[ABORT]\n";
 			$debug->flush();
 			packet_txt_write("status=abort");
 			packet_flush();
-		}
-		elsif ( $command eq "smudge" and
+		} elsif ( $command eq "smudge" and
 			exists $DELAY{$pathname} and
-			$DELAY{$pathname}{"requested"} == 1
-		) {
+			$DELAY{$pathname}{"requested"} == 1 ) {
 			print $debug "[DELAYED]\n";
 			$debug->flush();
 			packet_txt_write("status=delayed");
 			packet_flush();
 			$DELAY{$pathname}{"requested"} = 2;
 			$DELAY{$pathname}{"output"} = $output;
-		}
-		else {
+		} else {
 			packet_txt_write("status=success");
 			packet_flush();
 
@@ -262,8 +250,7 @@ while (1) {
 				print $debug ".";
 				if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {
 					$output = substr( $output, $MAX_PACKET_CONTENT_SIZE );
-				}
-				else {
+				} else {
 					$output = "";
 				}
 			}
-- 
2.15.0.rc1.106.g7e97f58a41

[PATCH 3/6] t0021/rot13-filter: improve error message

[Christian Couder]

If there is no new line at the end of something it receives,
the packet_txt_read() function die()s, but it's difficult to
debug without much context.

Let's give a bit more information when that happens.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t0021/rot13-filter.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 82882392ae..3b3da8a03d 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -82,7 +82,8 @@ sub packet_bin_read {
 sub packet_txt_read {
 	my ( $res, $buf ) = packet_bin_read();
 	unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
-		die "A non-binary line MUST be terminated by an LF.";
+		die "A non-binary line MUST be terminated by an LF.\n"
+		    . "Received: '$buf'";
 	}
 	return ( $res, $buf );
 }
-- 
2.15.0.rc1.106.g7e97f58a41

[PATCH 4/6] t0021/rot13-filter: add packet_initialize()

[Christian Couder]

Let's refactor the code to initialize communication into its own
packet_initialize() function, so that we can reuse this
functionality in following patches.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t0021/rot13-filter.pl | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 3b3da8a03d..278fc6f534 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -104,16 +104,22 @@ sub packet_flush {
 	STDOUT->flush();
 }
 
+sub packet_initialize {
+	my ($name, $version) = @_;
+
+	( packet_txt_read() eq ( 0, $name . "-client" ) )       || die "bad initialize";
+	( packet_txt_read() eq ( 0, "version=" . $version ) )   || die "bad version";
+	( packet_bin_read() eq ( 1, "" ) )                      || die "bad version end";
+
+	packet_txt_write( $name . "-server" );
+	packet_txt_write( "version=" . $version );
+	packet_flush();
+}
+
 print $debug "START\n";
 $debug->flush();
 
-( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
-( packet_txt_read() eq ( 0, "version=2" ) )         || die "bad version";
-( packet_bin_read() eq ( 1, "" ) )                  || die "bad version end";
-
-packet_txt_write("git-filter-server");
-packet_txt_write("version=2");
-packet_flush();
+packet_initialize("git-filter", 2);
 
 ( packet_txt_read() eq ( 0, "capability=clean" ) )  || die "bad capability";
 ( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
-- 
2.15.0.rc1.106.g7e97f58a41

[PATCH 5/6] t0021/rot13-filter: add capability functions

[Christian Couder]

Add functions to help read and write capabilities.
These functions will be reused in following patches.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t0021/rot13-filter.pl | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 278fc6f534..ba18b207c6 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -116,20 +116,44 @@ sub packet_initialize {
 	packet_flush();
 }
 
+sub packet_read_capabilities {
+	my @cap;
+	while (1) {
+		my ( $res, $buf ) = packet_bin_read();
+		return ( $res, @cap ) if ( $res != 0 );
+		unless ( $buf =~ s/\n$// ) {
+			die "A non-binary line MUST be terminated by an LF.\n"
+			    . "Received: '$buf'";
+		}
+		die "bad capability buf: '$buf'" unless ( $buf =~ s/capability=// );
+		push @cap, $buf;
+	}
+}
+
+sub packet_read_and_check_capabilities {
+	my @local_caps = @_;
+	my @remote_res_caps = packet_read_capabilities();
+	my $res = shift @remote_res_caps;
+	my %remote_caps = map { $_ => 1 } @remote_res_caps;
+	foreach (@local_caps) {
+        	die "'$_' capability not available" unless (exists($remote_caps{$_}));
+	}
+	return $res;
+}
+
+sub packet_write_capabilities {
+	packet_txt_write( "capability=" . $_ ) foreach (@_);
+	packet_flush();
+}
+
 print $debug "START\n";
 $debug->flush();
 
 packet_initialize("git-filter", 2);
 
-( packet_txt_read() eq ( 0, "capability=clean" ) )  || die "bad capability";
-( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
-( packet_txt_read() eq ( 0, "capability=delay" ) )  || die "bad capability";
-( packet_bin_read() eq ( 1, "" ) )                  || die "bad capability end";
+packet_read_and_check_capabilities("clean", "smudge", "delay");
+packet_write_capabilities(@capabilities);
 
-foreach (@capabilities) {
-	packet_txt_write( "capability=" . $_ );
-}
-packet_flush();
 print $debug "init handshake complete\n";
 $debug->flush();
 
-- 
2.15.0.rc1.106.g7e97f58a41

[PATCH 6/6] Add Git/Packet.pm from parts of t0021/rot13-filter.pl

[Christian Couder]

And while at it let's simplify t0021/rot13-filter.pl by
using Git/Packet.pm.

This will make it possible to reuse packet related
functions in other test scripts.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 perl/Git/Packet.pm      | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
 t/t0021/rot13-filter.pl |  94 ++------------------------------------
 2 files changed, 121 insertions(+), 91 deletions(-)
 create mode 100644 perl/Git/Packet.pm

diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
new file mode 100644
index 0000000000..b1e67477a0
--- /dev/null
+++ b/perl/Git/Packet.pm
@@ -0,0 +1,118 @@
+package Git::Packet;
+use 5.008;
+use strict;
+use warnings;
+BEGIN {
+	require Exporter;
+	if ($] < 5.008003) {
+		*import = \&Exporter::import;
+	} else {
+		# Exporter 5.57 which supports this invocation was
+		# released with perl 5.8.3
+		Exporter->import('import');
+	}
+}
+
+our @EXPORT = qw(
+			packet_bin_read
+			packet_txt_read
+			packet_bin_write
+			packet_txt_write
+			packet_flush
+			packet_initialize
+			packet_read_capabilities
+			packet_write_capabilities
+			packet_read_and_check_capabilities
+		);
+our @EXPORT_OK = @EXPORT;
+
+sub packet_bin_read {
+	my $buffer;
+	my $bytes_read = read STDIN, $buffer, 4;
+	if ( $bytes_read == 0 ) {
+		# EOF - Git stopped talking to us!
+		return ( -1, "" );
+	} elsif ( $bytes_read != 4 ) {
+		die "invalid packet: '$buffer'";
+	}
+	my $pkt_size = hex($buffer);
+	if ( $pkt_size == 0 ) {
+		return ( 1, "" );
+	} elsif ( $pkt_size > 4 ) {
+		my $content_size = $pkt_size - 4;
+		$bytes_read = read STDIN, $buffer, $content_size;
+		if ( $bytes_read != $content_size ) {
+			die "invalid packet ($content_size bytes expected; $bytes_read bytes read)";
+		}
+		return ( 0, $buffer );
+	} else {
+		die "invalid packet size: $pkt_size";
+	}
+}
+
+sub packet_txt_read {
+	my ( $res, $buf ) = packet_bin_read();
+	unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
+		die "A non-binary line MUST be terminated by an LF.\n"
+		    . "Received: '$buf'";
+	}
+	return ( $res, $buf );
+}
+
+sub packet_bin_write {
+	my $buf = shift;
+	print STDOUT sprintf( "%04x", length($buf) + 4 );
+	print STDOUT $buf;
+	STDOUT->flush();
+}
+
+sub packet_txt_write {
+	packet_bin_write( $_[0] . "\n" );
+}
+
+sub packet_flush {
+	print STDOUT sprintf( "%04x", 0 );
+	STDOUT->flush();
+}
+
+sub packet_initialize {
+	my ($name, $version) = @_;
+
+	( packet_txt_read() eq ( 0, $name . "-client" ) )	|| die "bad initialize";
+	( packet_txt_read() eq ( 0, "version=" . $version ) )	|| die "bad version";
+	( packet_bin_read() eq ( 1, "" ) )			|| die "bad version end";
+
+	packet_txt_write( $name . "-server" );
+	packet_txt_write( "version=" . $version );
+	packet_flush();
+}
+
+sub packet_read_capabilities {
+	my @cap;
+	while (1) {
+		my ( $res, $buf ) = packet_bin_read();
+		return ( $res, @cap ) if ( $res != 0 );
+		unless ( $buf =~ s/\n$// ) {
+			die "A non-binary line MUST be terminated by an LF.\n"
+			    . "Received: '$buf'";
+		}
+		die "bad capability buf: '$buf'" unless ( $buf =~ s/capability=// );
+		push @cap, $buf;
+	}
+}
+
+sub packet_read_and_check_capabilities {
+	my @local_caps = @_;
+	my @remote_res_caps = packet_read_capabilities();
+	my $res = shift @remote_res_caps;
+	my %remote_caps = map { $_ => 1 } @remote_res_caps;
+	foreach (@local_caps) {
+		die "'$_' capability not available" unless (exists($remote_caps{$_}));
+	}
+	return $res;
+}
+
+sub packet_write_capabilities {
+	packet_txt_write( "capability=" . $_ ) foreach (@_);
+	packet_flush();
+}
diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index ba18b207c6..2e8ad4d496 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -30,9 +30,12 @@
 #     to the "list_available_blobs" response.
 #
 
+use 5.008;
+use lib (split(/:/, $ENV{GITPERLLIB}));
 use strict;
 use warnings;
 use IO::File;
+use Git::Packet;
 
 my $MAX_PACKET_CONTENT_SIZE = 65516;
 my $log_file                = shift @ARGV;
@@ -55,97 +58,6 @@ sub rot13 {
 	return $str;
 }
 
-sub packet_bin_read {
-	my $buffer;
-	my $bytes_read = read STDIN, $buffer, 4;
-	if ( $bytes_read == 0 ) {
-		# EOF - Git stopped talking to us!
-		return ( -1, "" );
-	} elsif ( $bytes_read != 4 ) {
-		die "invalid packet: '$buffer'";
-	}
-	my $pkt_size = hex($buffer);
-	if ( $pkt_size == 0 ) {
-		return ( 1, "" );
-	} elsif ( $pkt_size > 4 ) {
-		my $content_size = $pkt_size - 4;
-		$bytes_read = read STDIN, $buffer, $content_size;
-		if ( $bytes_read != $content_size ) {
-			die "invalid packet ($content_size bytes expected; $bytes_read bytes read)";
-		}
-		return ( 0, $buffer );
-	} else {
-		die "invalid packet size: $pkt_size";
-	}
-}
-
-sub packet_txt_read {
-	my ( $res, $buf ) = packet_bin_read();
-	unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
-		die "A non-binary line MUST be terminated by an LF.\n"
-		    . "Received: '$buf'";
-	}
-	return ( $res, $buf );
-}
-
-sub packet_bin_write {
-	my $buf = shift;
-	print STDOUT sprintf( "%04x", length($buf) + 4 );
-	print STDOUT $buf;
-	STDOUT->flush();
-}
-
-sub packet_txt_write {
-	packet_bin_write( $_[0] . "\n" );
-}
-
-sub packet_flush {
-	print STDOUT sprintf( "%04x", 0 );
-	STDOUT->flush();
-}
-
-sub packet_initialize {
-	my ($name, $version) = @_;
-
-	( packet_txt_read() eq ( 0, $name . "-client" ) )       || die "bad initialize";
-	( packet_txt_read() eq ( 0, "version=" . $version ) )   || die "bad version";
-	( packet_bin_read() eq ( 1, "" ) )                      || die "bad version end";
-
-	packet_txt_write( $name . "-server" );
-	packet_txt_write( "version=" . $version );
-	packet_flush();
-}
-
-sub packet_read_capabilities {
-	my @cap;
-	while (1) {
-		my ( $res, $buf ) = packet_bin_read();
-		return ( $res, @cap ) if ( $res != 0 );
-		unless ( $buf =~ s/\n$// ) {
-			die "A non-binary line MUST be terminated by an LF.\n"
-			    . "Received: '$buf'";
-		}
-		die "bad capability buf: '$buf'" unless ( $buf =~ s/capability=// );
-		push @cap, $buf;
-	}
-}
-
-sub packet_read_and_check_capabilities {
-	my @local_caps = @_;
-	my @remote_res_caps = packet_read_capabilities();
-	my $res = shift @remote_res_caps;
-	my %remote_caps = map { $_ => 1 } @remote_res_caps;
-	foreach (@local_caps) {
-        	die "'$_' capability not available" unless (exists($remote_caps{$_}));
-	}
-	return $res;
-}
-
-sub packet_write_capabilities {
-	packet_txt_write( "capability=" . $_ ) foreach (@_);
-	packet_flush();
-}
-
 print $debug "START\n";
 $debug->flush();
 
-- 
2.15.0.rc1.106.g7e97f58a41

Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions

[Stefan Beller]

On Thu, Oct 19, 2017 at 5:30 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> To make it possible in a following commit to move packet
> reading and writing functions into a Packet.pm module,
> let's refactor these functions, so they don't handle
> printing debug output and exiting.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---

It looks good to me, though I am no perl expert by far.

Re: [PATCH 6/6] Add Git/Packet.pm from parts of t0021/rot13-filter.pl

[Stefan Beller]

On Thu, Oct 19, 2017 at 5:30 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> And while at it let's simplify t0021/rot13-filter.pl by
> using Git/Packet.pm.
>
> This will make it possible to reuse packet related
> functions in other test scripts.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  perl/Git/Packet.pm      | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
>  t/t0021/rot13-filter.pl |  94 ++------------------------------------
>  2 files changed, 121 insertions(+), 91 deletions(-)
>  create mode 100644 perl/Git/Packet.pm
>
> diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
> new file mode 100644
> index 0000000000..b1e67477a0
> --- /dev/null
> +++ b/perl/Git/Packet.pm
> @@ -0,0 +1,118 @@
> +package Git::Packet;
> +use 5.008;

+Avar, who knows more about perl versioning.

As we move it from our tests to the official part of the code
we'd want to keep aligned to the rest of the perl parts
of the code base?

This whole series looks good to me as a perl bystander.

Thanks,
Stefan

Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> To make it possible in a following commit to move packet
> reading and writing functions into a Packet.pm module,
> let's refactor these functions, so they don't handle
> printing debug output and exiting.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t0021/rot13-filter.pl | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> index ad685d92f8..e4495a52f3 100644
> --- a/t/t0021/rot13-filter.pl
> +++ b/t/t0021/rot13-filter.pl
> @@ -60,8 +60,7 @@ sub packet_bin_read {
>  	my $bytes_read = read STDIN, $buffer, 4;
>  	if ( $bytes_read == 0 ) {
>  		# EOF - Git stopped talking to us!
> -		print $debug "STOP\n";
> -		exit();
> +		return ( -1, "" );
>  	}
>  	elsif ( $bytes_read != 4 ) {
>  		die "invalid packet: '$buffer'";
> @@ -85,7 +84,7 @@ sub packet_bin_read {
>  
>  sub packet_txt_read {
>  	my ( $res, $buf ) = packet_bin_read();
> -	unless ( $buf eq '' or $buf =~ s/\n$// ) {
> +	unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
>  		die "A non-binary line MUST be terminated by an LF.";
>  	}
>  	return ( $res, $buf );
> @@ -131,7 +130,12 @@ print $debug "init handshake complete\n";
>  $debug->flush();
>  
>  while (1) {
> -	my ( $command ) = packet_txt_read() =~ /^command=(.+)$/;
> +	my ( $res, $command ) = packet_txt_read();
> +	if ( $res == -1 ) {
> +		print $debug "STOP\n";
> +		exit();
> +	}
> +	$command =~ s/^command=//;
>  	print $debug "IN: $command";
>  	$debug->flush();

This was not an issue in the old code which died upon unexpected EOF
inside the lowest-level helper packet_bin_read(), but now you have
one call to packet_bin_read() and many calls to packet_txt_read()
whose return value is not checked for this new condition you are
allowing packet_bin_read() to return.  This step taken alone is a
regression---let's see how the remainder of the series updates the
callers to compensate.

I initially thought that it may be more Perl-ish to return undef or
string instead of returning a 2-element list, but this code needs to
distinguish three conditions (i.e. a normal string that is 0 or more
bytes long, a flush, and an EOF), so that is not sufficient.  Perl
experts on the list still may be able to suggest a better way than
the current one to do so, but that is outside the scope of this
refactoring.

Thanks for starting to work on this.

Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> +sub packet_initialize {
> +	my ($name, $version) = @_;
> +
> +	( packet_txt_read() eq ( 0, $name . "-client" ) )       || die "bad initialize";
> +	( packet_txt_read() eq ( 0, "version=" . $version ) )   || die "bad version";
> +	( packet_bin_read() eq ( 1, "" ) )                      || die "bad version end";

This is not a new issue and it is a bit surprising that nobody
noticed when edcc8581 ("convert: add filter.<driver>.process
option", 2016-10-16) was reviewed, but the above is quite broken.
packet_txt_read() returns a 2-element list, and on the right hand
side of "eq" also has a list with (two, elements), but this takes
the last element of the list on each side, and compares them.  The
elading 0/1 we see above do not matter, which means we do not require
to see a flush at the end of the version---a simple empty string or
an EOF would do, which is definitely not what we want.

	#!/usr/bin/perl
	sub p {
		my ($res, $buf) = @_;
		return ($res, $buf);
	}
	if (p(0, "") eq (-1, 2, 3, "")) { print "ok\n"; }

It is fine to leave the original code broken at this step while we
purely move the lines around, and hopefully this will be corrected
in a later step in the series (I am responding as I read on, so I do
not yet know at which patch that happens in this series).

Thanks.

Re: [PATCH 5/6] t0021/rot13-filter: add capability functions

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Add functions to help read and write capabilities.
> These functions will be reused in following patches.

One more thing that is more noteworthy (read: do not forget to
describe it in the proposed log message) is that the original used
to require capabilities to come in a fixed order.

The new code allows these capabilities to be declared in any order,
it even allows duplicates (intended? shouldn't we be flagging it as
an error?), the helper can require a set of capabilities we do want
to see and fail if the remote doesn't declare any one of them
(good).

It does not check if the remote declares any capability we do not
know about (intended? the original noticed this situation and error
out---shouldn't the more generalized helper that is meant to be
reusable allow us to do so, too?).

	Side note: my answer to the last question is "it is OK and
	even desirable to ignore the fact that they claim to support
	a capability we do not know about", but I may be mistaken.
	The reasoning and the conclusion that is consistent with
	what the code does should be described in any case.

> +sub packet_read_capabilities {
> +	my @cap;
> +	while (1) {
> +		my ( $res, $buf ) = packet_bin_read();
> +		return ( $res, @cap ) if ( $res != 0 );

The original had the same "'list eq list' does not do what you may
think it does" issue.  This one corrects it, which is good.

I am not sure if ($res != 0) is correct though.  What should happen
when you get an unexpected EOF at this point?  The original would
have died; this ignores and continues.

> +		unless ( $buf =~ s/\n$// ) {
> +			die "A non-binary line MUST be terminated by an LF.\n"
> +			    . "Received: '$buf'";
> +		}

It may make sense to extract this in a small helper and call it from
here and from packet_txt_read().

> +		die "bad capability buf: '$buf'" unless ( $buf =~ s/capability=// );

This may merely be a style thing, but I somehow find statement
modifiers hard to follow, unless its condition is almost always
true.  If you follow the logic in a loop and see "die" at the
beginning, a normal thing to expect is that there were conditionals
that said "continue" (eh, 'next' or 'redo') to catch the normal case
and the control would reach "die" only under exceptional error
cases, but hiding a rare error condition after 'unless' statement
modifier breaks that expectation.

> +		push @cap, $buf;
> +	}
> +}
> +
> +sub packet_read_and_check_capabilities {
> +	my @local_caps = @_;
> +	my @remote_res_caps = packet_read_capabilities();
> +	my $res = shift @remote_res_caps;
> +	my %remote_caps = map { $_ => 1 } @remote_res_caps;

FYI:

	my ($res, @remote_caps) = packet_read_capabilities();
	my %remote_caps = map { $_ => 1 } @remote_caps;

may be more conventional way.

> +	foreach (@local_caps) {
> +        	die "'$_' capability not available" unless (exists($remote_caps{$_}));
> +	}

It is good that we can now accept capabilities in any order and
still enforce all the required capabilities are supported by the
other side.  It deserves a mention in the proposed log message.

> +	return $res;
> +}
> +
> +sub packet_write_capabilities {
> +	packet_txt_write( "capability=" . $_ ) foreach (@_);
> +	packet_flush();
> +}
> +
>  print $debug "START\n";
>  $debug->flush();
>  
>  packet_initialize("git-filter", 2);
>  
> -( packet_txt_read() eq ( 0, "capability=clean" ) )  || die "bad capability";
> -( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
> -( packet_txt_read() eq ( 0, "capability=delay" ) )  || die "bad capability";
> -( packet_bin_read() eq ( 1, "" ) )                  || die "bad capability end";
> +packet_read_and_check_capabilities("clean", "smudge", "delay");
> +packet_write_capabilities(@capabilities);

Neither the original nor the rewrite ensures that @capabilities we
ask to the other side to activate is a subset of what the other side
actually declared.

Fixing this is a bit more involved than "refactor what we have", but
probably is part of "refactor so that we can reuse in other
situations".  You'd want to return the list of caps received from
packet_read_and_check_capabilities() and make sure @capabilities is
a subset of that before writing them out to the other side to
request.

Thanks.

Re: [PATCH 0/6] Create Git/Packet.pm

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Goal
> ~~~~

Totally offtopic, but is it only me who finds these "section
headers" in cover letters from some people irritating and/or
jarring?  It perhaps is because I view the cover letter more as a
part of conversation, not a presentation.  And when you walk up to
somebody and start a conversation, you do not declare section
headers ;-)  

Saying "I want to be able to do these things in the future, and here
is to prepare for that future" at the beginning nevertheless is a
good thing.  It gives us readers an overall vision we can agree to
(or be against, or offer alternatives) and sets expectations on what
the series would do and where it stops and leaves the remainder to
follow-up work.

> Packet related functions in Perl can be useful to write new filters or
> to debug or test existing filters. So instead of having them in
> t0021/rot13-filter.pl, let's extract them into a new Git/Packet.pm
> module.

I left some comments on individual patches to point out places that
may need improvements.  I agree with the overall direction.

Thanks for starting this topic.

Re: [PATCH 0/6] Create Git/Packet.pm

[Philip Oakley]

Hi Junio,

From: "Junio C Hamano" <gitster@pobox.com>
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Goal
>> ~~~~
>
> Totally offtopic, but is it only me who finds these "section
> headers" in cover letters from some people irritating and/or
> jarring?

Personally I find that, for significant patch series, that clearly breaking 
out these distinct sections is of advantage. At this stage (the very first 
patch 0/n) there is no specific conversation, so the subject line is a short 
'hello' to the topic, and then the contributor is (it is to be hoped) 
setting out their proposal in a clear manner.

So I do like these headings for larger series, though there is some 
judgement to be made as to when the subject line alone is sufficient.

As a separate follow on, one thing that does annoy me is that in subsequent 
versions of the various patch series, folk tend to drop all explanation of 
why the series is of any relevance, leaving just the 'changed since last 
time' part. This means that new readers who try and pick up / review / 
contribute to a series later on in its development are not told the purpose. 
When the list is active it can, accidentally, do a disservice to the 
potential contributors who may feel that only core contributors are able to 
contribute.

Whether this series needed a Goal heading is separate discussion ;-)

> ..          It perhaps is because I view the cover letter more as a
> part of conversation, not a presentation.  And when you walk up to
> somebody and start a conversation, you do not declare section
> headers ;-)
>
> Saying "I want to be able to do these things in the future, and here
> is to prepare for that future" at the beginning nevertheless is a
> good thing.  It gives us readers an overall vision we can agree to
> (or be against, or offer alternatives) and sets expectations on what
> the series would do and where it stops and leaves the remainder to
> follow-up work.
>
>> Packet related functions in Perl can be useful to write new filters or
>> to debug or test existing filters. So instead of having them in
>> t0021/rot13-filter.pl, let's extract them into a new Git/Packet.pm
>> module.
>
> I left some comments on individual patches to point out places that
> may need improvements.  I agree with the overall direction.
>
> Thanks for starting this topic.
--
Philip 

Re: [PATCH 0/6] Create Git/Packet.pm

[Johannes Schindelin]

Hi Christian,

On Thu, 19 Oct 2017, Christian Couder wrote:

>   Add Git/Packet.pm from parts of t0021/rot13-filter.pl
> 
>  perl/Git/Packet.pm      | 118 ++++++++++++++++++++++++++++++++++++++++++++++++

This change, together with forcing t0021/rot13-filter.pl to use
Git/Packet.pm, breaks the test suite on Windows:

	https://travis-ci.org/git/git/jobs/292461846

There are actually two problems, one of which is fixed by

-- snip --
diff --git a/perl/Makefile b/perl/Makefile
index 15d96fcc7a5..f657de20e39 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -30,6 +30,7 @@ instdir_SQ = $(subst ','\'',$(prefix)/lib)
 modules += Git
 modules += Git/I18N
 modules += Git/IndexInfo
+modules += Git/Packet
 modules += Git/SVN
 modules += Git/SVN/Memoize/YAML
 modules += Git/SVN/Fetcher
-- snap --

You will want to pick this up in the next iteration. (You simply did not
notice because you did not build with NO_PERL_MAKEMAKER.)

However, that *still* does not fix the test for me: note how in the
verbose output recorded in Travis (see the link above), Perl fails to find
the Perl modules and then says:

Can'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 [..]

Note that the correct blib path starts with `C:\BuildAgent\_work` and
the line

	use lib (split(/:/, $ENV{GITPERLLIB}));

splits off the drive letter from the rest of the path. Obviously, this
fails to Do The Right Thing, and simply points to Yet Another Portability
Problem with Git's reliance on Unix scripting.

But why is it a Windows path in the first place? We take pains at using
only Unix-style paths in Git's test suite after all.

Well, this one is easy. We call git.exe, which is a pure Win32 executable
(i.e. it *wants* Windows paths, even in the environment passed to it) and
git.exe in turn calls Perl to interpret rot13-filter.pl. So on the way to
git.exe, GITPERLLIB is converted by the MSYS2 runtime into a Windows-style
path list. And then it is not converted back when we call Perl.

As a workaround, I used a trick to exclude GITPERLLIB from being converted
by MSYS2: setting the environment variable MSYS2_ENV_CONV_EXCL=GITPERLLIB
"fixed" the test for me (with above patch thrown in). I also set the test
in the Visual Studio Team Services build definition that runs those tests
triggered by Travis.

If your patch makes it into Git's `master`, we may have to hardcode that
MSYS2_ENV_CONV_EXCL=GITPERLLIB (or augment any existing
MSYS2_ENV_CONV_EXCL), so that other Windows developers do not have to
stumble over the same thing and then spend 3 hours to debug it.

Ciao,
Dscho

Re: [PATCH 0/6] Create Git/Packet.pm

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Note that the correct blib path starts with `C:\BuildAgent\_work` and
> the line
>
> 	use lib (split(/:/, $ENV{GITPERLLIB}));
>
> splits off the drive letter from the rest of the path. Obviously, this
> fails to Do The Right Thing, and simply points to Yet Another Portability
> Problem with Git's reliance on Unix scripting.

In our C code, we have "#define PATH_SEP ';'", and encourage our
code to be careful and use it.  Is there something similar for Perl
scripts, I wonder.

I notice that t/{t0202,t9000,t9700}/test.pl share the same
split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
the non-platform convention to accomodate the use of split(/:/)
certainly is a workaround, but it does feel dirty.

It is hard to imagine that we were the first people who wants to
split the value of a variable into a list, where the value is a list
of paths, concatenated into a single string with a delimiter that
may be platform specific.  I wonder if we are going against a best
practice established in the Perl world, simply because we don't know
about it (i.e. basically, it would say "don't split at a colon
because not all world is Unix; use $this_module instead", similar to
"don't split at a slash, use File::Spec instead to extract path
components").

Re: [PATCH 0/6] Create Git/Packet.pm

[Jacob Keller]

On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
>> the line
>>
>>       use lib (split(/:/, $ENV{GITPERLLIB}));
>>
>> splits off the drive letter from the rest of the path. Obviously, this
>> fails to Do The Right Thing, and simply points to Yet Another Portability
>> Problem with Git's reliance on Unix scripting.
>
> In our C code, we have "#define PATH_SEP ';'", and encourage our
> code to be careful and use it.  Is there something similar for Perl
> scripts, I wonder.
>
> I notice that t/{t0202,t9000,t9700}/test.pl share the same
> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
> the non-platform convention to accomodate the use of split(/:/)
> certainly is a workaround, but it does feel dirty.
>
> It is hard to imagine that we were the first people who wants to
> split the value of a variable into a list, where the value is a list
> of paths, concatenated into a single string with a delimiter that
> may be platform specific.  I wonder if we are going against a best
> practice established in the Perl world, simply because we don't know
> about it (i.e. basically, it would say "don't split at a colon
> because not all world is Unix; use $this_module instead", similar to
> "don't split at a slash, use File::Spec instead to extract path
> components").
>

I thought there was a way to do this in File::Spec, but that's only
for splitting regular paths, and not for splitting a list of paths
separated by ":" or ";"

We probably should find a better solution to allow this to work with
windows style paths...? I know that python provides os.pathsep, but I
haven't seen an equivalent for perl yet.

The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
maybe we should be using this?

Thanks,
Jake

[1] https://perldoc.perl.org/Env.html
[2] https://perldoc.perl.org/Config.html

Re: [PATCH 0/6] Create Git/Packet.pm

[Bryan Turner]

On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
>>> the line
>>>
>>>       use lib (split(/:/, $ENV{GITPERLLIB}));
>>>
>>> splits off the drive letter from the rest of the path. Obviously, this
>>> fails to Do The Right Thing, and simply points to Yet Another Portability
>>> Problem with Git's reliance on Unix scripting.
>>
>> In our C code, we have "#define PATH_SEP ';'", and encourage our
>> code to be careful and use it.  Is there something similar for Perl
>> scripts, I wonder.
>>
>> I notice that t/{t0202,t9000,t9700}/test.pl share the same
>> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
>> the non-platform convention to accomodate the use of split(/:/)
>> certainly is a workaround, but it does feel dirty.
>>
>> It is hard to imagine that we were the first people who wants to
>> split the value of a variable into a list, where the value is a list
>> of paths, concatenated into a single string with a delimiter that
>> may be platform specific.  I wonder if we are going against a best
>> practice established in the Perl world, simply because we don't know
>> about it (i.e. basically, it would say "don't split at a colon
>> because not all world is Unix; use $this_module instead", similar to
>> "don't split at a slash, use File::Spec instead to extract path
>> components").
>>
>
> I thought there was a way to do this in File::Spec, but that's only
> for splitting regular paths, and not for splitting a list of paths
> separated by ":" or ";"
>
> We probably should find a better solution to allow this to work with
> windows style paths...? I know that python provides os.pathsep, but I
> haven't seen an equivalent for perl yet.
>
> The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
> maybe we should be using this?
>
> Thanks,
> Jake
>
> [1] https://perldoc.perl.org/Env.html
> [2] https://perldoc.perl.org/Config.html

Re: [PATCH 0/6] Create Git/Packet.pm

[Bryan Turner]

On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
>>> the line
>>>
>>>       use lib (split(/:/, $ENV{GITPERLLIB}));
>>>
>>> splits off the drive letter from the rest of the path. Obviously, this
>>> fails to Do The Right Thing, and simply points to Yet Another Portability
>>> Problem with Git's reliance on Unix scripting.
>>
>> In our C code, we have "#define PATH_SEP ';'", and encourage our
>> code to be careful and use it.  Is there something similar for Perl
>> scripts, I wonder.
>>
>> I notice that t/{t0202,t9000,t9700}/test.pl share the same
>> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
>> the non-platform convention to accomodate the use of split(/:/)
>> certainly is a workaround, but it does feel dirty.
>>
>> It is hard to imagine that we were the first people who wants to
>> split the value of a variable into a list, where the value is a list
>> of paths, concatenated into a single string with a delimiter that
>> may be platform specific.  I wonder if we are going against a best
>> practice established in the Perl world, simply because we don't know
>> about it (i.e. basically, it would say "don't split at a colon
>> because not all world is Unix; use $this_module instead", similar to
>> "don't split at a slash, use File::Spec instead to extract path
>> components").
>>
>
> I thought there was a way to do this in File::Spec, but that's only
> for splitting regular paths, and not for splitting a list of paths
> separated by ":" or ";"
>
> We probably should find a better solution to allow this to work with
> windows style paths...? I know that python provides os.pathsep, but I
> haven't seen an equivalent for perl yet.
>
> The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
> maybe we should be using this?

I was testing this recently on the Perl included with Git for Windows
and it returns : for the path separator even though it's on Windows,
so I don't think that would work. The Perl in Git for Windows seems to
want UNIX-style inputs (something Dscho seemed to allude to in his
response earlier.). I'm not sure why it's that way, but he probably
knows.

Bryan

(Pardon my previous blank message; Gmail fail.)

>
> Thanks,
> Jake
>
> [1] https://perldoc.perl.org/Env.html
> [2] https://perldoc.perl.org/Config.html

Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> It is fine to leave the original code broken at this step while we
> purely move the lines around, and hopefully this will be corrected
> in a later step in the series (I am responding as I read on, so I do
> not yet know at which patch that happens in this series).

Actually, I think you'd probably be better off if you fixed these
broken comparisions that does (@list1 eq @list2) very early in the
series, perhaps as [PATCH 0.8/6].  

I am sure Perl experts among us on the list can come up with a
cleaner and better implementation of compare_lists sub I am adding
here, but in the meantime, here is what I would start with if I were
working on this topic.

Thanks.

 t/t0021/rot13-filter.pl | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index ad685d92f8..9bf5a756af 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -107,21 +107,42 @@ sub packet_flush {
 	STDOUT->flush();
 }
 
+sub compare_lists {
+	my ($expect, @result) = @_;
+	my $ix;
+	if (scalar @$expect != scalar @result) {
+		return undef;
+	}
+	for ($ix = 0; $ix < $#result; $ix++) {
+		if ($expect->[$ix] ne $result[$ix]) {
+			return undef;
+		}
+	}
+	return 1;
+}
+
 print $debug "START\n";
 $debug->flush();
 
-( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
-( packet_txt_read() eq ( 0, "version=2" ) )         || die "bad version";
-( packet_bin_read() eq ( 1, "" ) )                  || die "bad version end";
+compare_lists([0, "git-filter-client"], packet_txt_read()) ||
+	die "bad initialize";
+compare_lists([0, "version=2"], packet_txt_read()) ||
+	die "bad version";
+compare_lists([1, ""], packet_bin_read()) ||
+	die "bad version end";
 
 packet_txt_write("git-filter-server");
 packet_txt_write("version=2");
 packet_flush();
 
-( packet_txt_read() eq ( 0, "capability=clean" ) )  || die "bad capability";
-( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
-( packet_txt_read() eq ( 0, "capability=delay" ) )  || die "bad capability";
-( packet_bin_read() eq ( 1, "" ) )                  || die "bad capability end";
+compare_lists([0, "capability=clean"], packet_txt_read()) ||
+	die "bad capability";
+compare_lists([0, "capability=smudge"], packet_txt_read()) ||
+	die "bad capability";
+compare_lists([0, "capability=delay"], packet_txt_read()) ||
+	die "bad capability";
+compare_lists([1, ""], packet_bin_read()) ||
+	die "bad capability end";
 
 foreach (@capabilities) {
 	packet_txt_write( "capability=" . $_ );

Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()

[Christian Couder]

On Fri, Oct 27, 2017 at 4:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> It is fine to leave the original code broken at this step while we
>> purely move the lines around, and hopefully this will be corrected
>> in a later step in the series (I am responding as I read on, so I do
>> not yet know at which patch that happens in this series).
>
> Actually, I think you'd probably be better off if you fixed these
> broken comparisions that does (@list1 eq @list2) very early in the
> series, perhaps as [PATCH 0.8/6].

Yeah, it's better to fix the comparisons first.

> I am sure Perl experts among us on the list can come up with a
> cleaner and better implementation of compare_lists sub I am adding
> here, but in the meantime, here is what I would start with if I were
> working on this topic.

Thanks for the patch,
Christian.

Re: [PATCH 0/6] Create Git/Packet.pm

[Johannes Schindelin]

Hi Junio,

On Thu, 26 Oct 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Note that the correct blib path starts with `C:\BuildAgent\_work` and
> > the line
> >
> > 	use lib (split(/:/, $ENV{GITPERLLIB}));
> >
> > splits off the drive letter from the rest of the path. Obviously, this
> > fails to Do The Right Thing, and simply points to Yet Another Portability
> > Problem with Git's reliance on Unix scripting.
> 
> In our C code, we have "#define PATH_SEP ';'", and encourage our
> code to be careful and use it.  Is there something similar for Perl
> scripts, I wonder.
> 
> I notice that t/{t0202,t9000,t9700}/test.pl share the same
> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
> the non-platform convention to accomodate the use of split(/:/)
> certainly is a workaround, but it does feel dirty.

It is not only dirty, it *still* does not work with that workaround: Note
that GITPERLLIB is *also* set in the bin-wrappers/*...

And even then, it does not work: somewhere along the way, the path is
converted to a Windows path with backslashes, and for some reason MSYS2
Perl fails to handle that.

The best workaround I found in Git's source code (yes, it took me 2h to
investigate this littly problem, but at least I got an in-depth
understanding of the issue) was to pass BLIB_DIR instead, and not perform
a split but generate the two paths to include in Perl instead. Of course,
that would break out-of-tree usage of GITPERLLIB...

That's why I settled on the out-of-tree workaround that Windows
contributors will have to somehow figure out (as if it was not hard enough
already to contribute to Git for Windows...).

> It is hard to imagine that we were the first people who wants to
> split the value of a variable into a list, where the value is a list
> of paths, concatenated into a single string with a delimiter that
> may be platform specific.

No, the test suite has plenty of use cases for that. It usually works.

The problem is that t0021 contains very complex code that goes back and
forth between the C layer and the scripted layer. At one stage, the
pseudo-Unix paths are converted to Windows paths, with drive prefix and
backslashes, separated by semicolons. And somewhere along the lines, this
cannot be converted back.

I *think* that it happens when the bin-wrapper for git.exe is executed
from inside Git itself, or some such.

> I wonder if we are going against a best practice established in the Perl
> world, simply because we don't know about it (i.e. basically, it would
> say "don't split at a colon because not all world is Unix; use
> $this_module instead", similar to "don't split at a slash, use
> File::Spec instead to extract path components").

We go against best practices by having crucial parts of Git implemented as
Perl scripts. But you knew that ;-)

Ciao,
Dscho

Re: [PATCH 0/6] Create Git/Packet.pm

[Johannes Schindelin]

Hi Bryan,

On Thu, 26 Oct 2017, Bryan Turner wrote:

> On Thu, Oct 26, 2017 at 2:07 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> > On Wed, Oct 25, 2017 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >>
> >>> Note that the correct blib path starts with `C:\BuildAgent\_work` and
> >>> the line
> >>>
> >>>       use lib (split(/:/, $ENV{GITPERLLIB}));
> >>>
> >>> splits off the drive letter from the rest of the path. Obviously, this
> >>> fails to Do The Right Thing, and simply points to Yet Another Portability
> >>> Problem with Git's reliance on Unix scripting.
> >>
> >> In our C code, we have "#define PATH_SEP ';'", and encourage our
> >> code to be careful and use it.  Is there something similar for Perl
> >> scripts, I wonder.
> >>
> >> I notice that t/{t0202,t9000,t9700}/test.pl share the same
> >> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
> >> the non-platform convention to accomodate the use of split(/:/)
> >> certainly is a workaround, but it does feel dirty.
> >>
> >> It is hard to imagine that we were the first people who wants to
> >> split the value of a variable into a list, where the value is a list
> >> of paths, concatenated into a single string with a delimiter that
> >> may be platform specific.  I wonder if we are going against a best
> >> practice established in the Perl world, simply because we don't know
> >> about it (i.e. basically, it would say "don't split at a colon
> >> because not all world is Unix; use $this_module instead", similar to
> >> "don't split at a slash, use File::Spec instead to extract path
> >> components").
> >>
> >
> > I thought there was a way to do this in File::Spec, but that's only
> > for splitting regular paths, and not for splitting a list of paths
> > separated by ":" or ";"
> >
> > We probably should find a better solution to allow this to work with
> > windows style paths...? I know that python provides os.pathsep, but I
> > haven't seen an equivalent for perl yet.
> >
> > The Env[1] core modules suggests using $Config::Config{path_sep}[2]..
> > maybe we should be using this?
> 
> I was testing this recently on the Perl included with Git for Windows
> and it returns : for the path separator even though it's on Windows,
> so I don't think that would work. The Perl in Git for Windows seems to
> want UNIX-style inputs (something Dscho seemed to allude to in his
> response earlier.). I'm not sure why it's that way, but he probably
> knows.

MSYS2 Perl is essentially Cygwin's Perl ported over to MSYS2. And Cygwin
tries to keep everything as pseudo Unix as possible, to make it easier to
port software (if you think Git's source code is the only source code
woefully unprepared for semicolons as path separators, you just need to
buy me a few beers to hear plenty of war stories).

Ciao,
Dscho

Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()

[Lars Schneider]

> On 27 Oct 2017, at 04:57, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> It is fine to leave the original code broken at this step while we
>> purely move the lines around, and hopefully this will be corrected
>> in a later step in the series (I am responding as I read on, so I do
>> not yet know at which patch that happens in this series).
> 
> Actually, I think you'd probably be better off if you fixed these
> broken comparisions that does (@list1 eq @list2) very early in the
> series, perhaps as [PATCH 0.8/6].  
> 
> I am sure Perl experts among us on the list can come up with a
> cleaner and better implementation of compare_lists sub I am adding
> here, but in the meantime, here is what I would start with if I were
> working on this topic.
> 

Thanks for spotting this! I had some trouble applying the patch in
this email. I got an error about "corrupt patch at line 54"

BTW: I am using this little snippet to apply patches from the mailing:

    PATCH=$(curl -L --silent https://public-inbox.org/git/xmqqr2tpcn6g.fsf@gitster.mtv.corp.google.com/raw); 
    ((printf '%s' "$PATCH" | git am -3) || (git am --abort; printf '%s' "$PATCH" | git apply)) && 
    echo && echo "Patch successfully applied"

Does this look sensible to you?

I applied the patch "manually" and the result looks good to me.
The patch looks good to me, too, but I am by no means a Perl
expert.

Thanks,
Lars




> Thanks.
> 
> t/t0021/rot13-filter.pl | 35 ++++++++++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> index ad685d92f8..9bf5a756af 100644
> --- a/t/t0021/rot13-filter.pl
> +++ b/t/t0021/rot13-filter.pl
> @@ -107,21 +107,42 @@ sub packet_flush {
> 	STDOUT->flush();
> }
> 
> +sub compare_lists {
> +	my ($expect, @result) = @_;
> +	my $ix;
> +	if (scalar @$expect != scalar @result) {
> +		return undef;
> +	}
> +	for ($ix = 0; $ix < $#result; $ix++) {
> +		if ($expect->[$ix] ne $result[$ix]) {
> +			return undef;
> +		}
> +	}
> +	return 1;
> +}
> +
> print $debug "START\n";
> $debug->flush();
> 
> -( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
> -( packet_txt_read() eq ( 0, "version=2" ) )         || die "bad version";
> -( packet_bin_read() eq ( 1, "" ) )                  || die "bad version end";
> +compare_lists([0, "git-filter-client"], packet_txt_read()) ||
> +	die "bad initialize";
> +compare_lists([0, "version=2"], packet_txt_read()) ||
> +	die "bad version";
> +compare_lists([1, ""], packet_bin_read()) ||
> +	die "bad version end";
> 
> packet_txt_write("git-filter-server");
> packet_txt_write("version=2");
> packet_flush();
> 
> -( packet_txt_read() eq ( 0, "capability=clean" ) )  || die "bad capability";
> -( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
> -( packet_txt_read() eq ( 0, "capability=delay" ) )  || die "bad capability";
> -( packet_bin_read() eq ( 1, "" ) )                  || die "bad capability end";
> +compare_lists([0, "capability=clean"], packet_txt_read()) ||
> +	die "bad capability";
> +compare_lists([0, "capability=smudge"], packet_txt_read()) ||
> +	die "bad capability";
> +compare_lists([0, "capability=delay"], packet_txt_read()) ||
> +	die "bad capability";
> +compare_lists([1, ""], packet_bin_read()) ||
> +	die "bad capability end";
> 
> foreach (@capabilities) {
> 	packet_txt_write( "capability=" . $_ );

Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> BTW: I am using this little snippet to apply patches from the mailing:
>
>     PATCH=$(curl -L --silent https://public-inbox.org/git/xmqqr2tpcn6g.fsf@gitster.mtv.corp.google.com/raw); 
>     ((printf '%s' "$PATCH" | git am -3) || (git am --abort; printf '%s' "$PATCH" | git apply)) && 
>     echo && echo "Patch successfully applied"
>
> Does this look sensible to you?

Sensible?  I have no answer.  I wouldn't trust printf well enough to
stuff large value in a shell variable and feeding it myself in the
first place, but I would not be surprised if you did

	VAR=$(command) && printf '%s' "$VAR" >output

and ended up with a file with an incomplete line at the end.

Re: [PATCH 0/6] Create Git/Packet.pm

[Junio C Hamano]

I've queued this from Dscho; please take it into consideration when
you reroll.

Thanks.

-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sun, 29 Oct 2017 16:17:06 +0100
Subject: [PATCH] fixup! Git/Packet.pm: extract parts of t0021/rot13-filter.pl
 for reuse

The patch introducing Git/Packet.pm forgot the NO_PERL_MAKEMAKER part.
Breaking the test suite on Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 perl/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/perl/Makefile b/perl/Makefile
index 15d96fcc7a..4a74a493e6 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -37,6 +37,7 @@ modules += Git/SVN/Editor
 modules += Git/SVN/GlobSpec
 modules += Git/SVN/Log
 modules += Git/SVN/Migration
+modules += Git/SVN/Packet
 modules += Git/SVN/Prompt
 modules += Git/SVN/Ra
 modules += Git/SVN/Utils
-- 
2.15.0-rc2-267-g7d3ed0014a

Re: [PATCH 0/6] Create Git/Packet.pm

[Christian Couder]

Hi,

On Mon, Oct 30, 2017 at 1:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I've queued this from Dscho; please take it into consideration when
> you reroll.

Yeah, I was planning to add something like that, though in Dscho's
first email the patch was adding:

+modules += Git/Packet

and now it's adding:

> +modules += Git/SVN/Packet

I wonder where the "SVN" directory comes from as in
https://github.com/dscho/git/commits/cc/git-packet-pm my commit
(https://github.com/dscho/git/commit/46032e5f55f6e87d22f9a3c952b5d48a5f100a86)
still creates perl/Git/Packet.pm and not perl/Git/SVN/Packet.pm

Thanks both anyway,
Christian.

Re: [PATCH 0/6] Create Git/Packet.pm

[Johannes Schindelin]

Hi Christian & Junio,

On Mon, 30 Oct 2017, Christian Couder wrote:

> On Mon, Oct 30, 2017 at 1:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > I've queued this from Dscho; please take it into consideration when
> > you reroll.
> 
> Yeah, I was planning to add something like that, though in Dscho's
> first email the patch was adding:
> 
> +modules += Git/Packet
> 
> and now it's adding:
> 
> > +modules += Git/SVN/Packet

Bah. I should have paid more attention. The original Git/Packet is
correct, of course. My fixup! commit is bogus.

Sorry for the confusion,
Dscho

Re: [PATCH 0/6] Create Git/Packet.pm

[Jeff King]

On Mon, Oct 23, 2017 at 01:26:41PM +0100, Philip Oakley wrote:

> > Totally offtopic, but is it only me who finds these "section
> > headers" in cover letters from some people irritating and/or
> > jarring?
> 
> Personally I find that, for significant patch series, that clearly breaking
> out these distinct sections is of advantage. At this stage (the very first
> patch 0/n) there is no specific conversation, so the subject line is a short
> 'hello' to the topic, and then the contributor is (it is to be hoped)
> setting out their proposal in a clear manner.
> 
> So I do like these headings for larger series, though there is some
> judgement to be made as to when the subject line alone is sufficient.

I can live with fancily-formatted cover letters. BUT. I would say if
your cover letter is getting quite long, you might consider whether some
of its content ought to be going elsewhere (either into commit messages
themselves, or into a design document or other place inside the repo).

> As a separate follow on, one thing that does annoy me is that in subsequent
> versions of the various patch series, folk tend to drop all explanation of
> why the series is of any relevance, leaving just the 'changed since last
> time' part. This means that new readers who try and pick up / review /
> contribute to a series later on in its development are not told the purpose.
> When the list is active it can, accidentally, do a disservice to the
> potential contributors who may feel that only core contributors are able to
> contribute.

I actually have the opposite opinion. I find it annoying to have to wade
through the same unchanged content for each round just to find the
little snippet of "here's what's changed".

I don't mind following a link to the previous iteration to read the
back-story if I wasn't involved (it's a good idea to do that anyway to
see what previous reviews have already discussed).

I do often just post my "v2" as a follow-up and assume people can find
the original by following the thread backwards. But I imagine that not
everybody can do so. It's probably a good practice to at least put a
link to the prior version (and also to v1 for the original motivation)
if you're not going to repeat the cover letter in full.

-Peff

Re: [PATCH 5/6] t0021/rot13-filter: add capability functions

[Christian Couder]

On Sun, Oct 22, 2017 at 3:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Add functions to help read and write capabilities.
>> These functions will be reused in following patches.
>
> One more thing that is more noteworthy (read: do not forget to
> describe it in the proposed log message) is that the original used
> to require capabilities to come in a fixed order.
>
> The new code allows these capabilities to be declared in any order,

Yeah and I think it is good.

> it even allows duplicates (intended? shouldn't we be flagging it as
> an error?),

I think allowing duplicates is ok, as we allow duplicates in many
cases already, like duplicate command line options.
Or perhaps we should just warn?

> the helper can require a set of capabilities we do want
> to see and fail if the remote doesn't declare any one of them
> (good).

Yeah.

> It does not check if the remote declares any capability we do not
> know about (intended? the original noticed this situation and error
> out---shouldn't the more generalized helper that is meant to be
> reusable allow us to do so, too?).

I think that it is ok in general to just ignore capabilities we don't
know (as long as we don't advertise them). The protocol should work
using only the capabilities that both ends support.

Now if we just talk about testing, we might sometimes want to check
that one end sends only some specific capabilities. So in this case it
could be good if we could error out. On the other hand, if we test how
Git behaves when we advertise some specific capabilities, it might not
be a good idea to error out as it would break tests when Git learns a
new capability and advertise it.

In the specific case of rot13-filter.pl I think we are more in the
later case than in the former.

So I think it is ok to wait until we would really want to check that
one end sends only some specific capabilities, before we improve the
Packet.pm module to make it support that.

>         Side note: my answer to the last question is "it is OK and
>         even desirable to ignore the fact that they claim to support
>         a capability we do not know about", but I may be mistaken.

Yeah I agree.

>         The reasoning and the conclusion that is consistent with
>         what the code does should be described in any case.

Ok I will document all the above in the commit message.

>> +sub packet_read_capabilities {
>> +     my @cap;
>> +     while (1) {
>> +             my ( $res, $buf ) = packet_bin_read();
>> +             return ( $res, @cap ) if ( $res != 0 );
>
> The original had the same "'list eq list' does not do what you may
> think it does" issue.  This one corrects it, which is good.
>
> I am not sure if ($res != 0) is correct though.  What should happen
> when you get an unexpected EOF at this point?  The original would
> have died; this ignores and continues.

Well if there is an unexpected EOF, then packet_bin_read() returns
(-1, ""), so packet_read_capabilities() returns (-1, @cap) where @cap
contains the capabilities already received. Then
packet_read_and_check_capabilities() checks that we received all the
capabilities we expect and dies if that is not the case. If we did
receive all the capabilities, then
packet_read_and_check_capabilities() still returns -1, so the caller
may check that and die.

But yeah we could also just die in packet_read_capabilities() if $res
is -1. I will make this change.

>> +             unless ( $buf =~ s/\n$// ) {
>> +                     die "A non-binary line MUST be terminated by an LF.\n"
>> +                         . "Received: '$buf'";
>> +             }
>
> It may make sense to extract this in a small helper and call it from
> here and from packet_txt_read().

Ok, I have done this in my current version, that I plan to send soon.

>> +             die "bad capability buf: '$buf'" unless ( $buf =~ s/capability=// );
>
> This may merely be a style thing, but I somehow find statement
> modifiers hard to follow, unless its condition is almost always
> true.  If you follow the logic in a loop and see "die" at the
> beginning, a normal thing to expect is that there were conditionals
> that said "continue" (eh, 'next' or 'redo') to catch the normal case
> and the control would reach "die" only under exceptional error
> cases, but hiding a rare error condition after 'unless' statement
> modifier breaks that expectation.

Ok, I will use:

unless ( $buf =~ s/capability=// ) {
        die "bad capability buf: '$buf'" ;
}

>> +             push @cap, $buf;
>> +     }
>> +}
>> +
>> +sub packet_read_and_check_capabilities {
>> +     my @local_caps = @_;
>> +     my @remote_res_caps = packet_read_capabilities();
>> +     my $res = shift @remote_res_caps;
>> +     my %remote_caps = map { $_ => 1 } @remote_res_caps;
>
> FYI:
>
>         my ($res, @remote_caps) = packet_read_capabilities();
>         my %remote_caps = map { $_ => 1 } @remote_caps;
>
> may be more conventional way.

Yeah I will use what you suggest.

>> +     foreach (@local_caps) {
>> +             die "'$_' capability not available" unless (exists($remote_caps{$_}));
>> +     }
>
> It is good that we can now accept capabilities in any order and
> still enforce all the required capabilities are supported by the
> other side.  It deserves a mention in the proposed log message.

Ok, will add one.

>> +     return $res;
>> +}
>> +
>> +sub packet_write_capabilities {
>> +     packet_txt_write( "capability=" . $_ ) foreach (@_);
>> +     packet_flush();
>> +}
>> +
>>  print $debug "START\n";
>>  $debug->flush();
>>
>>  packet_initialize("git-filter", 2);
>>
>> -( packet_txt_read() eq ( 0, "capability=clean" ) )  || die "bad capability";
>> -( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
>> -( packet_txt_read() eq ( 0, "capability=delay" ) )  || die "bad capability";
>> -( packet_bin_read() eq ( 1, "" ) )                  || die "bad capability end";
>> +packet_read_and_check_capabilities("clean", "smudge", "delay");
>> +packet_write_capabilities(@capabilities);
>
> Neither the original nor the rewrite ensures that @capabilities we
> ask to the other side to activate is a subset of what the other side
> actually declared.
>
> Fixing this is a bit more involved than "refactor what we have", but
> probably is part of "refactor so that we can reuse in other
> situations".  You'd want to return the list of caps received from
> packet_read_and_check_capabilities() and make sure @capabilities is
> a subset of that before writing them out to the other side to
> request.

Ok, I will add that too.

Thanks.

Re: [PATCH 5/6] t0021/rot13-filter: add capability functions

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>>> +             my ( $res, $buf ) = packet_bin_read();
>>> +             return ( $res, @cap ) if ( $res != 0 );
>>
>> The original had the same "'list eq list' does not do what you may
>> think it does" issue.  This one corrects it, which is good.
>>
>> I am not sure if ($res != 0) is correct though.  What should happen
>> when you get an unexpected EOF at this point?  The original would
>> have died; this ignores and continues.
>
> Well if there is an unexpected EOF, then packet_bin_read() returns
> (-1, ""), so packet_read_capabilities() returns (-1, @cap) where @cap
> contains the capabilities already received. Then
> packet_read_and_check_capabilities() checks that we received all the
> capabilities we expect and dies if that is not the case. If we did
> receive all the capabilities, then
> packet_read_and_check_capabilities() still returns -1, so the caller
> may check that and die.

In other words, it happens, by accident, to stop before going very
far.  I think we'd be better off making it an explicitly checked
error.

> But yeah we could also just die in packet_read_capabilities() if $res
> is -1. I will make this change.

Good.

Thanks.

Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions

[Christian Couder]

On Sun, Oct 22, 2017 at 2:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> To make it possible in a following commit to move packet
>> reading and writing functions into a Packet.pm module,
>> let's refactor these functions, so they don't handle
>> printing debug output and exiting.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>  t/t0021/rot13-filter.pl | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
>> index ad685d92f8..e4495a52f3 100644
>> --- a/t/t0021/rot13-filter.pl
>> +++ b/t/t0021/rot13-filter.pl
>> @@ -60,8 +60,7 @@ sub packet_bin_read {
>>       my $bytes_read = read STDIN, $buffer, 4;
>>       if ( $bytes_read == 0 ) {
>>               # EOF - Git stopped talking to us!
>> -             print $debug "STOP\n";
>> -             exit();
>> +             return ( -1, "" );
>>       }
>>       elsif ( $bytes_read != 4 ) {
>>               die "invalid packet: '$buffer'";
>> @@ -85,7 +84,7 @@ sub packet_bin_read {
>>
>>  sub packet_txt_read {
>>       my ( $res, $buf ) = packet_bin_read();
>> -     unless ( $buf eq '' or $buf =~ s/\n$// ) {
>> +     unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
>>               die "A non-binary line MUST be terminated by an LF.";
>>       }
>>       return ( $res, $buf );
>> @@ -131,7 +130,12 @@ print $debug "init handshake complete\n";
>>  $debug->flush();
>>
>>  while (1) {
>> -     my ( $command ) = packet_txt_read() =~ /^command=(.+)$/;
>> +     my ( $res, $command ) = packet_txt_read();
>> +     if ( $res == -1 ) {
>> +             print $debug "STOP\n";
>> +             exit();
>> +     }
>> +     $command =~ s/^command=//;
>>       print $debug "IN: $command";
>>       $debug->flush();
>
> This was not an issue in the old code which died upon unexpected EOF
> inside the lowest-level helper packet_bin_read(), but now you have
> one call to packet_bin_read() and many calls to packet_txt_read()
> whose return value is not checked for this new condition you are
> allowing packet_bin_read() to return.  This step taken alone is a
> regression---let's see how the remainder of the series updates the
> callers to compensate.

Yeah, in the new version I will send really soon now, I have made a
number of changes to check the return value for the EOF condition.

> I initially thought that it may be more Perl-ish to return undef or
> string instead of returning a 2-element list, but this code needs to
> distinguish three conditions (i.e. a normal string that is 0 or more
> bytes long, a flush, and an EOF), so that is not sufficient.  Perl
> experts on the list still may be able to suggest a better way than
> the current one to do so, but that is outside the scope of this
> refactoring.

Yeah I can't think of a better way either.

Thanks.