Re: Changes in function read_pipe

Carlos Rica [off-list ref] writes:

Some people talked recently about renaming the function read_pipe
(sha1_file.c) to the better name read_fd.
Here I discuss other possible changes to the current version:

1. It now requires to allocate memory for the buffer before calling it,
you cannot pass it a pointer set to NULL or not initializated at all.

It should be a trivial and easy fix to add "initial allocation"
at the beginning of the function.  While I suspect that the
optimal initial allocation would be different for different
callers, probably we would not care that much.

2. The function doesn't terminate the data with NUL, and if you
need that, you must to realloc before adding the '\0', because
the function only returns the size of the data, not the buffer size.

Among the existing callers, index_fd() does not need NUL
termination, but we are almost always allocating more than
needed when the final size is unknown, so giving an extra NUL
(of course not including that in the final result size) would
not hurt.  Other callers are 'stripspace' and 'mktag', and I
would imagine 'tag' and 'commit' when they are reading the
message from the standard input would benefit from the implicit
NUL termination.  I'd say go for it.

3. When function fails in reading (xread returns < 0), buffer is not
freed.

I do not think this is a problem.  The current callers seem to
do something like this:

	if (read_pipe(fd, ...)) {
        	free(buf);
                return error(...);
	}

but not freeing, and reporting how many you read so far, would
allow readers to work on results partially read (not to be
confused with the EAGAIN which is already handled with xread()).