# [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read() 11 messages from 2017-11-21 to 2017-11-22. Participants: Christian Couder, Jonathan Nieder, Junio C Hamano. Thread: https://gitlist.dev/t/47274 ## Christian Couder, 2017-11-21 16:09 Subject: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read() Message-ID: <20171121160939.22962-1-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20171121160939.22962-1-chriscool%40tuxfamily.org ``` 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 --- 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 ``` ## Christian Couder, 2017-11-21 16:09 Subject: [PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless' Message-ID: <20171121160939.22962-2-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20171121160939.22962-2-chriscool%40tuxfamily.org In-Reply-To: <20171121160939.22962-1-chriscool@tuxfamily.org> ``` The code is more understandable with 'if' instead of 'unless'. Signed-off-by: Christian Couder --- 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 ``` ## Jonathan Nieder, 2017-11-21 19:19 Subject: Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read() Message-ID: <20171121191900.GD3429@aiede.mtv.corp.google.com> URL: https://gitlist.dev/e/20171121191900.GD3429%40aiede.mtv.corp.google.com In-Reply-To: <20171121160939.22962-1-chriscool@tuxfamily.org> ``` 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 > --- 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 ``` ## Jonathan Nieder, 2017-11-21 19:24 Subject: Re: [PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless' Message-ID: <20171121192440.GE3429@aiede.mtv.corp.google.com> URL: https://gitlist.dev/e/20171121192440.GE3429%40aiede.mtv.corp.google.com In-Reply-To: <20171121160939.22962-2-chriscool@tuxfamily.org> ``` Hi, Christian Couder wrote: > The code is more understandable with 'if' instead of 'unless'. > > Signed-off-by: Christian Couder > --- > 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 ``` ## Junio C Hamano, 2017-11-22 03:39 Subject: Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read() Message-ID: URL: https://gitlist.dev/e/xmqq60a3vv63.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20171121191900.GD3429@aiede.mtv.corp.google.com> ``` Jonathan Nieder 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. ``` ## Junio C Hamano, 2017-11-22 03:48 Subject: Re: [PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless' Message-ID: URL: https://gitlist.dev/e/xmqq1skrvurt.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20171121192440.GE3429@aiede.mtv.corp.google.com> ``` Jonathan Nieder 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. ``` ## Junio C Hamano, 2017-11-22 03:48 Subject: Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read() Message-ID: URL: https://gitlist.dev/e/xmqqzi7fug6x.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20171121160939.22962-1-chriscool@tuxfamily.org> ``` Christian Couder 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 > --- > > 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. ``` ## Jonathan Nieder, 2017-11-22 05:10 Subject: Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read() Message-ID: <20171122051014.GA7625@aiede.mtv.corp.google.com> URL: https://gitlist.dev/e/20171122051014.GA7625%40aiede.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano wrote: > Jonathan Nieder 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 ``` ## Junio C Hamano, 2017-11-22 06:28 Subject: Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read() Message-ID: URL: https://gitlist.dev/e/xmqqfu96u8s4.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20171122051014.GA7625@aiede.mtv.corp.google.com> ``` Jonathan Nieder 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. ``` ## Jonathan Nieder, 2017-11-22 07:07 Subject: Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read() Message-ID: <20171122070708.GD7625@aiede.mtv.corp.google.com> URL: https://gitlist.dev/e/20171122070708.GD7625%40aiede.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano wrote: > Jonathan Nieder writes: >> Junio C Hamano wrote: >>> Jonathan Nieder 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 ``` ## Christian Couder, 2017-11-22 07:14 Subject: Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read() Message-ID: URL: https://gitlist.dev/e/CAP8UFD3sL4etgtbHFbGChF36ggARsr5961F5CZuZhSZY_z_1-Q%40mail.gmail.com In-Reply-To: ``` On Wed, Nov 22, 2017 at 7:28 AM, Junio C Hamano wrote: > Jonathan Nieder 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. ```