{"thread":{"id":"47274","subject":"[PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()","startedAt":"2017-11-21T16:09:55Z","lastAt":"2017-11-22T07:14:38Z","messageCount":11,"participants":["Christian Couder","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"333104","messageId":"20171121160939.22962-1-chriscool@tuxfamily.org","threadId":"47274","inReplyTo":null,"subject":"[PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-11-21T16:09:38Z","receivedAt":"2017-11-21T16:09:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"The function calls itself \"required\", but it does not die when it\nsees an unexpected EOF.\nLet's rename it to \"packet_key_val_read()\".\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n\nThese 2 patches are a late follow up from:\n\nhttps://public-inbox.org/git/CAP8UFD2vk4jV7jEBx3Axd-dhfcsGSJVFFt+pumdT1j8GD_oM_w@mail.gmail.com/\n\n perl/Git/Packet.pm      | 5 +++--\n t/t0021/rot13-filter.pl | 4 ++--\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm\nindex 255b28c098..82da0cf0db 100644\n--- a/perl/Git/Packet.pm\n+++ b/perl/Git/Packet.pm\n@@ -17,7 +17,7 @@ our @EXPORT = qw(\n \t\t\tpacket_compare_lists\n \t\t\tpacket_bin_read\n \t\t\tpacket_txt_read\n-\t\t\tpacket_required_key_val_read\n+\t\t\tpacket_key_val_read\n \t\t\tpacket_bin_write\n \t\t\tpacket_txt_write\n \t\t\tpacket_flush\n@@ -83,7 +83,8 @@ sub packet_txt_read {\n \treturn ( $res, $buf );\n }\n \n-sub packet_required_key_val_read {\n+# Read a text line and check that it is in the form \"key=value\"\n+sub packet_key_val_read {\n \tmy ( $key ) = @_;\n \tmy ( $res, $buf ) = packet_txt_read();\n \tunless ( $res == -1 or ( $buf =~ s/^$key=// and $buf ne '' ) ) {\ndiff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl\nindex 6fd7fa476b..f1678851de 100644\n--- a/t/t0021/rot13-filter.pl\n+++ b/t/t0021/rot13-filter.pl\n@@ -70,7 +70,7 @@ print $debug \"init handshake complete\\n\";\n $debug->flush();\n \n while (1) {\n-\tmy ( $res, $command ) = packet_required_key_val_read(\"command\");\n+\tmy ( $res, $command ) = packet_key_val_read(\"command\");\n \tif ( $res == -1 ) {\n \t\tprint $debug \"STOP\\n\";\n \t\texit();\n@@ -106,7 +106,7 @@ while (1) {\n \t\tpacket_txt_write(\"status=success\");\n \t\tpacket_flush();\n \t} else {\n-\t\tmy ( $res, $pathname ) = packet_required_key_val_read(\"pathname\");\n+\t\tmy ( $res, $pathname ) = packet_key_val_read(\"pathname\");\n \t\tif ( $res == -1 ) {\n \t\t\tdie \"unexpected EOF while expecting pathname\";\n \t\t}\n-- \n2.15.0.318.g4f69657937\n\n"},{"id":"333105","messageId":"20171121160939.22962-2-chriscool@tuxfamily.org","threadId":"47274","inReplyTo":"20171121160939.22962-1-chriscool@tuxfamily.org","subject":"[PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless'","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-11-21T16:09:39Z","receivedAt":"2017-11-21T16:09:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"The code is more understandable with 'if' instead of 'unless'.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n perl/Git/Packet.pm | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm\nindex 82da0cf0db..1682403ffc 100644\n--- a/perl/Git/Packet.pm\n+++ b/perl/Git/Packet.pm\n@@ -68,16 +68,16 @@ sub packet_bin_read {\n \n sub remove_final_lf_or_die {\n \tmy $buf = shift;\n-\tunless ( $buf =~ s/\\n$// ) {\n-\t\tdie \"A non-binary line MUST be terminated by an LF.\\n\"\n-\t\t    . \"Received: '$buf'\";\n+\tif ( $buf =~ s/\\n$// ) {\n+\t\treturn $buf;\n \t}\n-\treturn $buf;\n+\tdie \"A non-binary line MUST be terminated by an LF.\\n\"\n+\t    . \"Received: '$buf'\";\n }\n \n sub packet_txt_read {\n \tmy ( $res, $buf ) = packet_bin_read();\n-\tunless ( $res == -1 or $buf eq '' ) {\n+\tif ( $res != -1 and $buf ne '' ) {\n \t\t$buf = remove_final_lf_or_die($buf);\n \t}\n \treturn ( $res, $buf );\n@@ -87,10 +87,10 @@ sub packet_txt_read {\n sub packet_key_val_read {\n \tmy ( $key ) = @_;\n \tmy ( $res, $buf ) = packet_txt_read();\n-\tunless ( $res == -1 or ( $buf =~ s/^$key=// and $buf ne '' ) ) {\n-\t\tdie \"bad $key: '$buf'\";\n+\tif ( $res == -1 or ( $buf =~ s/^$key=// and $buf ne '' ) ) {\n+\t\treturn ( $res, $buf );\n \t}\n-\treturn ( $res, $buf );\n+\tdie \"bad $key: '$buf'\";\n }\n \n sub packet_bin_write {\n-- \n2.15.0.318.g4f69657937\n\n"},{"id":"333120","messageId":"20171121191900.GD3429@aiede.mtv.corp.google.com","threadId":"47274","inReplyTo":"20171121160939.22962-1-chriscool@tuxfamily.org","subject":"Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-21T19:19:00Z","receivedAt":"2017-11-21T19:19:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nChristian Couder wrote:\n\n> The function calls itself \"required\", but it does not die when it\n> sees an unexpected EOF.\n> Let's rename it to \"packet_key_val_read()\".\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n\nnit: please wrap lines to a consistent width, to make the message\neasier to read.  In the above, it looks like the line break is\nintentional --- is it meant to be two paragraphs (i.e. is it missing\nanother newline)?\n\noptional, just noticed while I'm nitpicking: the description 'rename\npacket_required_key_val_read' doesn't tell why the function is being\nrenamed.  Maybe something like\n\n\tGit::Packet: clarify that packet_required_key_val_read allows EOF\n\nwould do the trick.\n\n[...]\n> +++ b/perl/Git/Packet.pm\n> @@ -83,7 +83,8 @@ sub packet_txt_read {\n>  \treturn ( $res, $buf );\n>  }\n>  \n> -sub packet_required_key_val_read {\n> +# Read a text line and check that it is in the form \"key=value\"\n> +sub packet_key_val_read {\n\nThis comment doesn't tell me how to use the function.  How do I detect\nwhether it successfully read a line?  What do the return values\nrepresent?  What happens if the line it read doesn't match the key?\n\nThanks,\nJonathan\n"},{"id":"333122","messageId":"20171121192440.GE3429@aiede.mtv.corp.google.com","threadId":"47274","inReplyTo":"20171121160939.22962-2-chriscool@tuxfamily.org","subject":"Re: [PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-21T19:24:40Z","receivedAt":"2017-11-21T19:24:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nChristian Couder wrote:\n\n> The code is more understandable with 'if' instead of 'unless'.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  perl/Git/Packet.pm | 16 ++++++++--------\n>  1 file changed, 8 insertions(+), 8 deletions(-)\n\nI'm agnostic about that.  In some ways it seems like an odd change,\nsince it is changing from\n\n\tif (exceptional case) {\n\t\tdie \"explanation\";\n\t}\n\tcommon case;\n\nto\n\n\tif (!exceptional case) {\n\t\tcommon case;\n\t}\n\tdie \"explanation\";\n\nwhen we usually prefer the former (getting the exceptional case over\nwith early so the reader can concentrate on the common case).  In\nperl, it might be even more idiomatic to do\n\n\tdie \"explanation\" if exceptional case;\n\tcommon case;\n\nbut Documentation/CodingGuidelines appears to recommend not going that\nfar. :)\n\n[...]\n> --- a/perl/Git/Packet.pm\n> +++ b/perl/Git/Packet.pm\n> @@ -68,16 +68,16 @@ sub packet_bin_read {\n>  \n>  sub remove_final_lf_or_die {\n>  \tmy $buf = shift;\n> -\tunless ( $buf =~ s/\\n$// ) {\n\nFor readability, I find this whitespace within parens more distracting.\n\nDocumentation/CodingGuidelines covers that for C programs but not for\nPerl.  Do we have a preferred style either way about it?\n\nThanks,\nJonathan\n"},{"id":"333232","messageId":"xmqq60a3vv63.fsf@gitster.mtv.corp.google.com","threadId":"47274","inReplyTo":"20171121191900.GD3429@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T03:39:32Z","receivedAt":"2017-11-22T03:39:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> nit: please wrap lines to a consistent width, to make the message\n> easier to read.  In the above, it looks like the line break is\n> intentional --- is it meant to be two paragraphs (i.e. is it missing\n> another newline)?\n\nI'd think so; will add a missing LF while queuing..\n\n> optional, just noticed while I'm nitpicking: the description 'rename\n> packet_required_key_val_read' doesn't tell why the function is being\n> renamed.  Maybe something like\n>\n> \tGit::Packet: clarify that packet_required_key_val_read allows EOF\n>\n> would do the trick.\n\nSounds good. \n\n>> +# Read a text line and check that it is in the form \"key=value\"\n>> +sub packet_key_val_read {\n>\n> This comment doesn't tell me how to use the function.  How do I detect\n> whether it successfully read a line?  What do the return values\n> represent?  What happens if the line it read doesn't match the key?\n\nWould this work for both of you?\n\n# Read a text packet, expecting that it is in the form \"key=value\" for\n# the given $key.  An EOF does not trigger any error and is reported\n# back to the caller (like packet_txt_read() does).  Die if the \"key\"\n# part of \"key=value\" does not match the given $key, or the value part\n# is empty.\n"},{"id":"333233","messageId":"xmqq1skrvurt.fsf@gitster.mtv.corp.google.com","threadId":"47274","inReplyTo":"20171121192440.GE3429@aiede.mtv.corp.google.com","subject":"Re: [PATCH 2/2] Git/Packet.pm: use 'if' instead of 'unless'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T03:48:06Z","receivedAt":"2017-11-22T03:48:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> [...]\n>> --- a/perl/Git/Packet.pm\n>> +++ b/perl/Git/Packet.pm\n>> @@ -68,16 +68,16 @@ sub packet_bin_read {\n>>  \n>>  sub remove_final_lf_or_die {\n>>  \tmy $buf = shift;\n>> -\tunless ( $buf =~ s/\\n$// ) {\n>\n> For readability, I find this whitespace within parens more distracting.\n\nI personally find them distracting, too.  This file seems full of\nthem so it is OK to be consistent with the existing practice in this\nstep, leaving the eventual clean-up (if we agree that it is a good\nidea) to a later step that does it for the entire file contents.\n\nThanks.\n"},{"id":"333234","messageId":"xmqqzi7fug6x.fsf@gitster.mtv.corp.google.com","threadId":"47274","inReplyTo":"20171121160939.22962-1-chriscool@tuxfamily.org","subject":"Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T03:48:22Z","receivedAt":"2017-11-22T03:48:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> The function calls itself \"required\", but it does not die when it\n> sees an unexpected EOF.\n> Let's rename it to \"packet_key_val_read()\".\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>\n> These 2 patches are a late follow up from:\n>\n> https://public-inbox.org/git/CAP8UFD2vk4jV7jEBx3Axd-dhfcsGSJVFFt+pumdT1j8GD_oM_w@mail.gmail.com/\n\nThanks for tying the loose end.\n\n"},{"id":"333243","messageId":"20171122051014.GA7625@aiede.mtv.corp.google.com","threadId":"47274","inReplyTo":"xmqq60a3vv63.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-22T05:10:14Z","receivedAt":"2017-11-22T05:10:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>> Christian Couder wrote:\n\n>>> +# Read a text line and check that it is in the form \"key=value\"\n>>> +sub packet_key_val_read {\n>>\n>> This comment doesn't tell me how to use the function.  How do I detect\n>> whether it successfully read a line?  What do the return values\n>> represent?  What happens if the line it read doesn't match the key?\n>\n> Would this work for both of you?\n>\n> # Read a text packet, expecting that it is in the form \"key=value\" for\n> # the given $key.  An EOF does not trigger any error and is reported\n> # back to the caller (like packet_txt_read() does).  Die if the \"key\"\n> # part of \"key=value\" does not match the given $key, or the value part\n> # is empty.\n\nYes, thank you.\n\nJonathan\n"},{"id":"333250","messageId":"xmqqfu96u8s4.fsf@gitster.mtv.corp.google.com","threadId":"47274","inReplyTo":"20171122051014.GA7625@aiede.mtv.corp.google.com","subject":"Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T06:28:27Z","receivedAt":"2017-11-22T06:28:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>>> This comment doesn't tell me how to use the function.  How do I detect\n>>> whether it successfully read a line?  What do the return values\n>>> represent?  What happens if the line it read doesn't match the key?\n>>\n>> Would this work for both of you?\n>>\n>> # Read a text packet, expecting that it is in the form \"key=value\" for\n>> # the given $key.  An EOF does not trigger any error and is reported\n>> # back to the caller (like packet_txt_read() does).  Die if the \"key\"\n>> # part of \"key=value\" does not match the given $key, or the value part\n>> # is empty.\n>\n> Yes, thank you.\n\nHeh.  I actually was expecting a different response: \"that describes\nwhat the reader can easily read out of the implementation and is\nuseless\", though.\n"},{"id":"333251","messageId":"20171122070708.GD7625@aiede.mtv.corp.google.com","threadId":"47274","inReplyTo":"xmqqfu96u8s4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-22T07:07:08Z","receivedAt":"2017-11-22T07:07:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>> Junio C Hamano wrote:\n>>> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>>>> This comment doesn't tell me how to use the function.  How do I detect\n>>>> whether it successfully read a line?  What do the return values\n>>>> represent?  What happens if the line it read doesn't match the key?\n>>>\n>>> Would this work for both of you?\n>>>\n>>> # Read a text packet, expecting that it is in the form \"key=value\" for\n>>> # the given $key.  An EOF does not trigger any error and is reported\n>>> # back to the caller (like packet_txt_read() does).  Die if the \"key\"\n>>> # part of \"key=value\" does not match the given $key, or the value part\n>>> # is empty.\n>>\n>> Yes, thank you.\n>\n> Heh.  I actually was expecting a different response: \"that describes\n> what the reader can easily read out of the implementation and is\n> useless\", though.\n\nThe main context that I'm missing and that this function comment\ndoesn't answer is what this function is for.  When would I want to\nread a line and exit if it doesn't match \"key\" but not exit if I hit\nEOF?  It seems very strange.\n\nThe function comment does successfully capture the strangeness,\nthough, and that already helps.  When looking at the implementation, I\nhad a bit of a double-take and wondered what I was missing.  This doc\ncomment says \"you weren't missing anything --- that is actually the\ncontract that this function intends to fulfill\".\n\nThanks,\nJonathan\n"},{"id":"333252","messageId":"CAP8UFD3sL4etgtbHFbGChF36ggARsr5961F5CZuZhSZY_z_1-Q@mail.gmail.com","threadId":"47274","inReplyTo":"xmqqfu96u8s4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] Git/Packet.pm: rename packet_required_key_val_read()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-11-22T07:14:30Z","receivedAt":"2017-11-22T07:14:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Nov 22, 2017 at 7:28 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>>>> This comment doesn't tell me how to use the function.  How do I detect\n>>>> whether it successfully read a line?  What do the return values\n>>>> represent?  What happens if the line it read doesn't match the key?\n>>>\n>>> Would this work for both of you?\n>>>\n>>> # Read a text packet, expecting that it is in the form \"key=value\" for\n>>> # the given $key.  An EOF does not trigger any error and is reported\n>>> # back to the caller (like packet_txt_read() does).  Die if the \"key\"\n>>> # part of \"key=value\" does not match the given $key, or the value part\n>>> # is empty.\n>>\n>> Yes, thank you.\n>\n> Heh.  I actually was expecting a different response: \"that describes\n> what the reader can easily read out of the implementation and is\n> useless\", though.\n\nI was going to resend without the comment after Jonathan's first\nemail, but I am ok with either your improved comment or without any\ncomment. Thanks.\n"}]}