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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2017, 03:39 UTC
Message-ID: <xmqq60a3vv63.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171121191900.GD3429@aiede.mtv.corp.google.com>

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.