On Thu, May 26, 2011 at 01:53:09PM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > The problem is that most callers are not careful enough to repeatedly
> > call read_in_full and find out that there might have been an error in
> > the previous result. They see a read shorter than what they asked, and
> > assume it was EOF.
>
> I can buy that argument, but then shouldn't we change the "careful"
> callers to treat any short-read from read_in_full() as an error?
I don't think so. A short-read could still be EOF, and you can
distinguish between the two. Before, if you asked for n bytes, you would
get back an 'r' that was one of:
r < 0: error on first read
r < n: short read via EOF, or error on subsequent read
r == n: OK, got n bytes
With my patch, you get:
r < 0: error on any read
r < n: short read via EOF
r == n: OK, got n bytes
So any negative return is an error, and less than n now _always_ means a
short read. So your "careful" callers will now get the error
automatically. If you want to update any callers, it would be ones like:
if (read_in_full(fd, buf, len) != len))
die("unable to read %d bytes", len);
which are not _wrong_, but could be more specific in doing:
ssize_t r = read_in_full(fd, buf, len);
if (r < 0)
die_errno("unable to read");
else if (r < len)
die("short read");
But that is just a quality-of-error-message issue, not a correctness
issue.
> diff --git a/combine-diff.c b/combine-diff.c
> index be67cfc..176231e 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -845,11 +845,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
> result = xmalloc(len + 1);
>
> done = read_in_full(fd, result, len);
> - if (done < 0)
> + if (done != len)
> die_errno("read error '%s'", elem->path);
> - else if (done < len)
> - die("early EOF '%s'", elem->path);
> -
This is backwards. We now _can_ tell the two apart, so more callers
could be like this.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html