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@xxxxxxxxxxxxx> > --- 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