Re: [PATCH v6 1/8] pkt-line: add packet_read_line_gently()

From: Ben Peart <peartben@gmail.com>
Date: Apr 24, 2017, 20:33 UTC
Message-ID: <d6e8a63b-e95d-4194-5ad0-d68f557be083@gmail.com>
In-Reply-To: <xmqqy3uqzb90.fsf@gitster.mtv.corp.google.com>

On 4/24/2017 12:21 AM, Junio C Hamano wrote:

Show 23 quoted lines

> Ben Peart <peartben@gmail.com> writes:
>
>> Add packet_read_line_gently() to enable reading a line without dying on
>> EOF.
>>
>> Signed-off-by: Ben Peart <benpeart@microsoft.com>
>> ---
>>   pkt-line.c | 14 +++++++++++++-
>>   pkt-line.h | 10 ++++++++++
>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/pkt-line.c b/pkt-line.c
>> index d4b6bfe076..bfdb177b34 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -315,7 +315,7 @@ static char *packet_read_line_generic(int fd,
>>   			      PACKET_READ_CHOMP_NEWLINE);
>>   	if (dst_len)
>>   		*dst_len = len;
>> -	return len ? packet_buffer : NULL;
>> +	return (len > 0) ? packet_buffer : NULL;
>>   }
> The log does not seem to explain what this change is about.

This change was made as the result of a request in feedback from the previous version of the patch series which pointed out that the call to packet_read can return -1 in some error cases and to keep the code more consistent with packet_read_line_gently below.

If len < 0 then the old code would have incorrectly returned a pointer to a buffer with garbage while the new code correctly returns NULL (fixes potential bug) if len == 0 then the code will return NULL before and after this change (no change in behavior) if len > 0 then the code will return packet_buffer before and after this change (no change in behavior)

Show 20 quoted lines

>
> Is this supposed to be a preliminary bugfix where this helper used
> to return a non-NULL buffer when underlying packet_read() signaled
> an error by returning a negative len?  If so, this should probably
> be a separate patch early in the series.
>
> Should len==0 be considered an error?  Especially given that
> PACKET_READ_CHOMP_NEWLINE is in use, I would expect that len==0
> should be treated similarly to positive length, i.e. the otherside
> gave us an empty line.
>
>>   char *packet_read_line(int fd, int *len_p)
>> @@ -323,6 +323,18 @@ char *packet_read_line(int fd, int *len_p)
>>   	return packet_read_line_generic(fd, NULL, NULL, len_p);
>>   }
>>   
>> +int packet_read_line_gently(int fd, int *dst_len, char** dst_line)
> ERROR: "foo** bar" should be "foo **bar"
> #29: FILE: pkt-line.c:326:
> +int packet_read_line_gently(int fd, int *dst_len, char** dst_line)

Sorry, missed that somehow. I'll move the space before the ** in the next version of the patch series.

Show 10 quoted lines

>
>> +{
>> +	int len = packet_read(fd, NULL, NULL,
>> +			      packet_buffer, sizeof(packet_buffer),
>> +			      PACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);
>> +	if (dst_len)
>> +		*dst_len = len;
>> +	if (dst_line)
>> +		*dst_line = (len > 0) ? packet_buffer : NULL;
> I have the same doubt as above for len == 0 case.

packet_read() returns -1 when PACKET_READ_GENTLE_ON_EOF is passed and it hits truncated output from the remote process. This occurs when the remote process exits unexpectedly which is the exact case I was fixing with this new function. This requires testing len for this error condition so that it can correctly handle this error case, otherwise it would incorrectly return an invalid buffer. Since this is a new function, there isn't any before/after behavior comparisons but it is consistent with the similar packet_read_line() function.