Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions
On Sun, Oct 22, 2017 at 2:58 AM, Junio C Hamanowrote: > Christian Couder 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 >> --- >> 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.
Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions
Christian Couderwrites: > 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 > --- > 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 1/6] t0021/rot13-filter: refactor packet reading functions
On Thu, Oct 19, 2017 at 5:30 AM, Christian Couderwrote: > 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 > --- It looks good to me, though I am no perl expert by far.
[PATCH 1/6] t0021/rot13-filter: refactor packet reading functions
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--- 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