[PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Christian Couder]

The function calls itself "required", but it does not die when it
sees an unexpected EOF.
Let's rename it to "packet_key_val_read()".

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---

These 2 patches are a late follow up from:

https://public-inbox.org/git/CAP8UFD2vk4jV7jEBx3Axd-dhfcsGSJVFFt+pumdT1j8GD_oM_w@mail.gmail.com/

 perl/Git/Packet.pm      | 5 +++--
 t/t0021/rot13-filter.pl | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
index 255b28c098..82da0cf0db 100644
--- a/perl/Git/Packet.pm
+++ b/perl/Git/Packet.pm
@@ -17,7 +17,7 @@ our @EXPORT = qw(
 			packet_compare_lists
 			packet_bin_read
 			packet_txt_read
-			packet_required_key_val_read
+			packet_key_val_read
 			packet_bin_write
 			packet_txt_write
 			packet_flush
@@ -83,7 +83,8 @@ sub packet_txt_read {
 	return ( $res, $buf );
 }
 
-sub packet_required_key_val_read {
+# Read a text line and check that it is in the form "key=value"
+sub packet_key_val_read {
 	my ( $key ) = @_;
 	my ( $res, $buf ) = packet_txt_read();
 	unless ( $res == -1 or ( $buf =~ s/^$key=// and $buf ne '' ) ) {
diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 6fd7fa476b..f1678851de 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -70,7 +70,7 @@ print $debug "init handshake complete\n";
 $debug->flush();
 
 while (1) {
-	my ( $res, $command ) = packet_required_key_val_read("command");
+	my ( $res, $command ) = packet_key_val_read("command");
 	if ( $res == -1 ) {
 		print $debug "STOP\n";
 		exit();
@@ -106,7 +106,7 @@ while (1) {
 		packet_txt_write("status=success");
 		packet_flush();
 	} else {
-		my ( $res, $pathname ) = packet_required_key_val_read("pathname");
+		my ( $res, $pathname ) = packet_key_val_read("pathname");
 		if ( $res == -1 ) {
 			die "unexpected EOF while expecting pathname";
 		}
-- 
2.15.0.318.g4f69657937

[PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless'

[Christian Couder]

The code is more understandable with 'if' instead of 'unless'.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 perl/Git/Packet.pm | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
index 82da0cf0db..1682403ffc 100644
--- a/perl/Git/Packet.pm
+++ b/perl/Git/Packet.pm
@@ -68,16 +68,16 @@ sub packet_bin_read {
 
 sub remove_final_lf_or_die {
 	my $buf = shift;
-	unless ( $buf =~ s/\n$// ) {
-		die "A non-binary line MUST be terminated by an LF.\n"
-		    . "Received: '$buf'";
+	if ( $buf =~ s/\n$// ) {
+		return $buf;
 	}
-	return $buf;
+	die "A non-binary line MUST be terminated by an LF.\n"
+	    . "Received: '$buf'";
 }
 
 sub packet_txt_read {
 	my ( $res, $buf ) = packet_bin_read();
-	unless ( $res == -1 or $buf eq '' ) {
+	if ( $res != -1 and $buf ne '' ) {
 		$buf = remove_final_lf_or_die($buf);
 	}
 	return ( $res, $buf );
@@ -87,10 +87,10 @@ sub packet_txt_read {
 sub packet_key_val_read {
 	my ( $key ) = @_;
 	my ( $res, $buf ) = packet_txt_read();
-	unless ( $res == -1 or ( $buf =~ s/^$key=// and $buf ne '' ) ) {
-		die "bad $key: '$buf'";
+	if ( $res == -1 or ( $buf =~ s/^$key=// and $buf ne '' ) ) {
+		return ( $res, $buf );
 	}
-	return ( $res, $buf );
+	die "bad $key: '$buf'";
 }
 
 sub packet_bin_write {
-- 
2.15.0.318.g4f69657937

Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Jonathan Nieder]

Hi,

Christian Couder wrote:

> The function calls itself "required", but it does not die when it
> sees an unexpected EOF.
> Let's rename it to "packet_key_val_read()".
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---

nit: please wrap lines to a consistent width, to make the message
easier to read.  In the above, it looks like the line break is
intentional --- is it meant to be two paragraphs (i.e. is it missing
another newline)?

optional, just noticed while I'm nitpicking: the description 'rename
packet_required_key_val_read' doesn't tell why the function is being
renamed.  Maybe something like

	Git::Packet: clarify that packet_required_key_val_read allows EOF

would do the trick.

[...]
> +++ b/perl/Git/Packet.pm
> @@ -83,7 +83,8 @@ sub packet_txt_read {
>  	return ( $res, $buf );
>  }
>  
> -sub packet_required_key_val_read {
> +# Read a text line and check that it is in the form "key=value"
> +sub packet_key_val_read {

This comment doesn't tell me how to use the function.  How do I detect
whether it successfully read a line?  What do the return values
represent?  What happens if the line it read doesn't match the key?

Thanks,
Jonathan

Re: [PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless'

[Jonathan Nieder]

Hi,

Christian Couder wrote:

> The code is more understandable with 'if' instead of 'unless'.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  perl/Git/Packet.pm | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

I'm agnostic about that.  In some ways it seems like an odd change,
since it is changing from

	if (exceptional case) {
		die "explanation";
	}
	common case;

to

	if (!exceptional case) {
		common case;
	}
	die "explanation";

when we usually prefer the former (getting the exceptional case over
with early so the reader can concentrate on the common case).  In
perl, it might be even more idiomatic to do

	die "explanation" if exceptional case;
	common case;

but Documentation/CodingGuidelines appears to recommend not going that
far. :)

[...]
> --- a/perl/Git/Packet.pm
> +++ b/perl/Git/Packet.pm
> @@ -68,16 +68,16 @@ sub packet_bin_read {
>  
>  sub remove_final_lf_or_die {
>  	my $buf = shift;
> -	unless ( $buf =~ s/\n$// ) {

For readability, I find this whitespace within parens more distracting.

Documentation/CodingGuidelines covers that for C programs but not for
Perl.  Do we have a preferred style either way about it?

Thanks,
Jonathan

Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> nit: please wrap lines to a consistent width, to make the message
> easier to read.  In the above, it looks like the line break is
> intentional --- is it meant to be two paragraphs (i.e. is it missing
> another newline)?

I'd think so; will add a missing LF while queuing..

> optional, just noticed while I'm nitpicking: the description 'rename
> packet_required_key_val_read' doesn't tell why the function is being
> renamed.  Maybe something like
>
> 	Git::Packet: clarify that packet_required_key_val_read allows EOF
>
> would do the trick.

Sounds good. 

>> +# Read a text line and check that it is in the form "key=value"
>> +sub packet_key_val_read {
>
> This comment doesn't tell me how to use the function.  How do I detect
> whether it successfully read a line?  What do the return values
> represent?  What happens if the line it read doesn't match the key?

Would this work for both of you?

# Read a text packet, expecting that it is in the form "key=value" for
# the given $key.  An EOF does not trigger any error and is reported
# back to the caller (like packet_txt_read() does).  Die if the "key"
# part of "key=value" does not match the given $key, or the value part
# is empty.

Re: [PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless'

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> [...]
>> --- a/perl/Git/Packet.pm
>> +++ b/perl/Git/Packet.pm
>> @@ -68,16 +68,16 @@ sub packet_bin_read {
>>  
>>  sub remove_final_lf_or_die {
>>  	my $buf = shift;
>> -	unless ( $buf =~ s/\n$// ) {
>
> For readability, I find this whitespace within parens more distracting.

I personally find them distracting, too.  This file seems full of
them so it is OK to be consistent with the existing practice in this
step, leaving the eventual clean-up (if we agree that it is a good
idea) to a later step that does it for the entire file contents.

Thanks.

Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Junio C Hamano]

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

> The function calls itself "required", but it does not die when it
> sees an unexpected EOF.
> Let's rename it to "packet_key_val_read()".
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>
> These 2 patches are a late follow up from:
>
> https://public-inbox.org/git/CAP8UFD2vk4jV7jEBx3Axd-dhfcsGSJVFFt+pumdT1j8GD_oM_w@mail.gmail.com/

Thanks for tying the loose end.

Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Christian Couder wrote:

>>> +# Read a text line and check that it is in the form "key=value"
>>> +sub packet_key_val_read {
>>
>> This comment doesn't tell me how to use the function.  How do I detect
>> whether it successfully read a line?  What do the return values
>> represent?  What happens if the line it read doesn't match the key?
>
> Would this work for both of you?
>
> # Read a text packet, expecting that it is in the form "key=value" for
> # the given $key.  An EOF does not trigger any error and is reported
> # back to the caller (like packet_txt_read() does).  Die if the "key"
> # part of "key=value" does not match the given $key, or the value part
> # is empty.

Yes, thank you.

Jonathan

Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>>> This comment doesn't tell me how to use the function.  How do I detect
>>> whether it successfully read a line?  What do the return values
>>> represent?  What happens if the line it read doesn't match the key?
>>
>> Would this work for both of you?
>>
>> # Read a text packet, expecting that it is in the form "key=value" for
>> # the given $key.  An EOF does not trigger any error and is reported
>> # back to the caller (like packet_txt_read() does).  Die if the "key"
>> # part of "key=value" does not match the given $key, or the value part
>> # is empty.
>
> Yes, thank you.

Heh.  I actually was expecting a different response: "that describes
what the reader can easily read out of the implementation and is
useless", though.

Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Junio C Hamano wrote:
>>> Jonathan Nieder <jrnieder@gmail.com> writes:

>>>> This comment doesn't tell me how to use the function.  How do I detect
>>>> whether it successfully read a line?  What do the return values
>>>> represent?  What happens if the line it read doesn't match the key?
>>>
>>> Would this work for both of you?
>>>
>>> # Read a text packet, expecting that it is in the form "key=value" for
>>> # the given $key.  An EOF does not trigger any error and is reported
>>> # back to the caller (like packet_txt_read() does).  Die if the "key"
>>> # part of "key=value" does not match the given $key, or the value part
>>> # is empty.
>>
>> Yes, thank you.
>
> Heh.  I actually was expecting a different response: "that describes
> what the reader can easily read out of the implementation and is
> useless", though.

The main context that I'm missing and that this function comment
doesn't answer is what this function is for.  When would I want to
read a line and exit if it doesn't match "key" but not exit if I hit
EOF?  It seems very strange.

The function comment does successfully capture the strangeness,
though, and that already helps.  When looking at the implementation, I
had a bit of a double-take and wondered what I was missing.  This doc
comment says "you weren't missing anything --- that is actually the
contract that this function intends to fulfill".

Thanks,
Jonathan

Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()

[Christian Couder]

On Wed, Nov 22, 2017 at 7:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>>> This comment doesn't tell me how to use the function.  How do I detect
>>>> whether it successfully read a line?  What do the return values
>>>> represent?  What happens if the line it read doesn't match the key?
>>>
>>> Would this work for both of you?
>>>
>>> # Read a text packet, expecting that it is in the form "key=value" for
>>> # the given $key.  An EOF does not trigger any error and is reported
>>> # back to the caller (like packet_txt_read() does).  Die if the "key"
>>> # part of "key=value" does not match the given $key, or the value part
>>> # is empty.
>>
>> Yes, thank you.
>
> Heh.  I actually was expecting a different response: "that describes
> what the reader can easily read out of the implementation and is
> useless", though.

I was going to resend without the comment after Jonathan's first
email, but I am ok with either your improved comment or without any
comment. Thanks.