threads / patch / 47274

patch, 2 partsGit/Packet.pm: rename packet_required_key_val_read()

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

## tl;dr

11 messages between Nov 21, 2017 and Nov 22, 2017. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Christian Couder· Nov 21, 2017, 16:09 UTC · lore

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(-)
Show changes to 2 files +5 −4

perl/Git/Packet.pm, t/t0021/rot13-filter.pl

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
Christian Couder· Nov 21, 2017, 16:09 UTC · re: Christian Couder · lore

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

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(-)
Show changes to perl/Git/Packet.pm +8 −8
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
Jonathan Nieder· Nov 21, 2017, 19:24 UTC · re: Christian Couder · lore

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

Hi,
Christian Couder wrote:
Show 6 quoted lines
> 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. :)

[...]
Show 7 quoted lines
> --- 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

Junio C Hamano· Nov 22, 2017, 03:48 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 10 quoted lines
> [...]
>> --- 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.
Jonathan Nieder· Nov 21, 2017, 19:19 UTC · re: Christian Couder · lore

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

Hi,
Christian Couder wrote:
Show 6 quoted lines
> 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.
[...]
Show 8 quoted lines
> +++ 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

Junio C Hamano· Nov 22, 2017, 03:39 UTC · re: Jonathan Nieder · lore

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

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..
Show 7 quoted lines
> 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.
Show 6 quoted lines
>> +# 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.

Jonathan Nieder· Nov 22, 2017, 05:10 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Christian Couder wrote:
Show 14 quoted lines
>>> +# 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
Junio C Hamano· Nov 22, 2017, 06:28 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
>>> 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.

Jonathan Nieder· Nov 22, 2017, 07:07 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Junio C Hamano wrote:
>>> Jonathan Nieder <jrnieder@gmail.com> writes:
Show 17 quoted lines
>>>> 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

Christian Couder· Nov 22, 2017, 07:14 UTC · re: Junio C Hamano · lore

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

On Wed, Nov 22, 2017 at 7:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> 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.

Junio C Hamano· Nov 22, 2017, 03:48 UTC · re: Christian Couder · lore

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

Christian Couder <christian.couder@gmail.com> writes:
Show 10 quoted lines
> 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.

← back to recent threads