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

[Christian Couder]

On Thu, Aug 3, 2017 at 9:11 PM, Junio C Hamano  wrote:

>> diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
>> new file mode 100644
>> index 00..aaffecbe2a
>> --- /dev/null
>> +++ b/perl/Git/Packet.pm
>> @@ -0,0 +1,71 @@
>> +package Git::Packet;
>> +use 5.008;
>> +use strict;
>> +use warnings;
>> +BEGIN {
>> + require Exporter;
>> + if ($] < 5.008003) {
>> + *import = \::import;
>> + } else {
>> + # Exporter 5.57 which supports this invocation was
>> + # released with perl 5.8.3
>> + Exporter->import('import');
>> + }
>> +}
>
> This is merely me being curious, but do we want this boilerplate,
> which we do not use in perl/Git.pm but we do in perl/Git/I18N.pm?

I don't know. I copied it as I thought that we wanted to support Perl
versions starting from 5.8.0, but I am ok to remove it or to leave it
depending on what the Perl experts think (CCing AEvar) and what we
decide.

>> +our @EXPORT = qw(
>> + packet_bin_read
>> + packet_txt_read
>> + packet_bin_write
>> + packet_txt_write
>> + packet_flush
>> + );
>> +our @EXPORT_OK = @EXPORT;
>
> We can see that you made sure that the only thing 05/40 needs to do
> is to use this package and remove the definition of these subs,
> without having to touch any caller by first updating the original
> implementation in 03/40 and then exporting these names in 04/40.
> Knowing that the preparation is nicely done already, it is a bit
> irritating to see that 05/40 is a separate patch, as we need to
> switch between the patches to see if there is any difference between
> the original implementation of the subs, and the replacement
> implemented in here.  It would have been nicer to have changes in
> 04/40 and 05/40 in a single patch.

Ok, I have squashed 04/40 and 05/40 together in my current version of
this series.

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

[Junio C Hamano]

Christian Couder  writes:

> This will make it possible to reuse packet reading and writing
> functions in other test scripts.
>
> Signed-off-by: Christian Couder 
> ---
>  perl/Git/Packet.pm | 71 
> ++
>  1 file changed, 71 insertions(+)
>  create mode 100644 perl/Git/Packet.pm
>
> diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
> new file mode 100644
> index 00..aaffecbe2a
> --- /dev/null
> +++ b/perl/Git/Packet.pm
> @@ -0,0 +1,71 @@
> +package Git::Packet;
> +use 5.008;
> +use strict;
> +use warnings;
> +BEGIN {
> + require Exporter;
> + if ($] < 5.008003) {
> + *import = \::import;
> + } else {
> + # Exporter 5.57 which supports this invocation was
> + # released with perl 5.8.3
> + Exporter->import('import');
> + }
> +}

This is merely me being curious, but do we want this boilerplate,
which we do not use in perl/Git.pm but we do in perl/Git/I18N.pm?

> +our @EXPORT = qw(
> + packet_bin_read
> + packet_txt_read
> + packet_bin_write
> + packet_txt_write
> + packet_flush
> + );
> +our @EXPORT_OK = @EXPORT;

We can see that you made sure that the only thing 05/40 needs to do
is to use this package and remove the definition of these subs,
without having to touch any caller by first updating the original
implementation in 03/40 and then exporting these names in 04/40.
Knowing that the preparation is nicely done already, it is a bit
irritating to see that 05/40 is a separate patch, as we need to
switch between the patches to see if there is any difference between
the original implementation of the subs, and the replacement
implemented in here.  It would have been nicer to have changes in
04/40 and 05/40 in a single patch.

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

[Christian Couder]

This will make it possible to reuse packet reading and writing
functions in other test scripts.

Signed-off-by: Christian Couder 
---
 perl/Git/Packet.pm | 71 ++
 1 file changed, 71 insertions(+)
 create mode 100644 perl/Git/Packet.pm

diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
new file mode 100644
index 00..aaffecbe2a
--- /dev/null
+++ b/perl/Git/Packet.pm
@@ -0,0 +1,71 @@
+package Git::Packet;
+use 5.008;
+use strict;
+use warnings;
+BEGIN {
+   require Exporter;
+   if ($] < 5.008003) {
+   *import = \::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
+   );
+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 || $buf =~ s/\n$// ) {
+   die "A non-binary line MUST be terminated by an LF.";
+   }
+   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();
+}
-- 
2.14.0.rc1.52.gf02fb0ddac.dirty