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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 22, 2017, 07:07 UTC
Message-ID: <20171122070708.GD7625@aiede.mtv.corp.google.com>
In-Reply-To: <xmqqfu96u8s4.fsf@gitster.mtv.corp.google.com>

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