On 4/24/2017 12:21 AM, Junio C Hamano wrote:
Ben Peart <peartben@xxxxxxxxx> writes:
Add packet_read_line_gently() to enable reading a line without dying on
EOF.
Signed-off-by: Ben Peart <benpeart@xxxxxxxxxxxxx>
---
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)
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.
+{
+ 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.