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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 22, 2017, 00:58 UTC
Message-ID: <xmqq4lqsf17n.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171019123030.17338-2-chriscool@tuxfamily.org>

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

Show 46 quoted lines

> 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.