Re: [PATCH] Rename read_pipe() with read_fd() and make its buffer nul-terminated.

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 18, 2007, 07:49 UTC
Message-ID: <7vps2q6tjf.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <469DBC8A.6090704@gmail.com>

Carlos Rica <jasampler@gmail.com> writes:

Show 13 quoted lines

> The new name is closer to the purpose of the function.
>
> The other change just makes things easier for callers needing a
> NUL-terminated buffer.
>
> Since the function returns only the memory written with data,
> almost always allocating more space than needed because final
> size is unknown, an extra NUL terminating the buffer is harmless.
> It is not included in the returned size, so the function
> remains working as before.
>
> Also, now the function allows the buffer passed to be NULL at first,
> and alloc_nr is now used for growing the buffer, instead size=*2.

Some people may say function name change should be a separate patch by itself, but I'd let it pass for now...

Show 17 quoted lines

> diff --git a/sha1_file.c b/sha1_file.c
> index 1efd9ae..563ec07 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2304,27 +2304,38 @@ int has_sha1_file(const unsigned char *sha1)
>   *
>   * returns 0 if anything went fine and -1 otherwise
>   *
> + * The buffer is always NUL-terminated, not including it in returned size.
> + *
>   * NOTE: both buf and size may change, but even when -1 is returned
>   * you still have to free() it yourself.
>   */
> -int read_pipe(int fd, char** return_buf, unsigned long* return_size)
> +int read_fd(int fd, char** return_buf, unsigned long* return_size)
>  {
>  	char* buf = *return_buf;

Our code seems to prefer pointer declaration to be spelled as "type **var", not "type** var".

Show 9 quoted lines

>  	unsigned long size = *return_size;
>  	ssize_t iret;
>  	unsigned long off = 0;
>
> +	if (!buf || size <= 1) {
> +		size = alloc_nr(size);
> +		buf = xrealloc(buf, size);
> +	}
> +

Hmph. The reason this is not "!size" is because you would want more than one. As your plan is to use this mostly for the -F option of "tag/commit", I suspect using a bit larger default, such as 80 (just a line), or probably 1k (most log messages would fit in such a buffer), would be more practical.

Show 16 quoted lines

>  	do {
>  		iret = xread(fd, buf + off, size - off);
>  		if (iret > 0) {
>  			off += iret;
>  			if (off == size) {
> -				size *= 2;
> +				size = alloc_nr(size);
>  				buf = xrealloc(buf, size);
>  			}
>  		}
>  	} while (iret > 0);
>
> +	if (off == size)
> +		buf = xrealloc(buf, size + 1);
> +	buf[off] = '\0';
> +

I wonder if doing xread(... (size-1) - off) in the loop would ensure (off <= size-1) here. You also would need to update the realloc condition inside loop if you do so.