From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 02 Apr 2020 13:34:57 GMT
Subject: Re: [PATCH v3 03/19] sequencer: use file strbuf for read_oneliner()
Message-ID: <0b237d3c-5b2d-1714-9243-8f32621a332d@gmail.com>
In-Reply-To: <7c37777f07191c8ac1d26300f9465b90758550b2.1584782450.git.liu.denton@gmail.com>

Hi Denton

On 21/03/2020 09:21, Denton Liu wrote:
> In the original read_oneliner logic, we duplicated the logic for
> strbuf_trim_trailing_newline() with one exception: instead of preventing
> buffer accesses below index 0, it would prevent buffer accesses below
> index `orig_len`. Although this is correct, it isn't worth having the
> duplicated logic around.
> 
> Add a second strbuf to which files are read and run
> strbuf_trim_trailing_newline() directly on this strbuf then concatenate
> this strbuf with the argument strbuf at the end of the function. The
> function's external behaviour is unchanged.

read_oneliner() is typically called with an strbuf that is used multiple 
times with the caller resetting it in-between calls. This saves us from 
having to allocate a new buffer for each call. The change here negates 
that as we end up allocating a new buffer each time and then copying the 
result. I'd be surprised if any of the callers actually wanted to append 
to the buffer, I suspect they all call strbuf_reset() before passing the 
buffer to read_oneliner(). If that is the case maybe we should think 
about adding the call to strbuf_reset() inside read_oneliner() and 
documenting it as resetting the buffer before reading. Then we can just 
use strbuf_trim() on the 'real' buffer.

Best Wishes

Phillip

> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   sequencer.c | 24 +++++++++++++-----------
>   1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index e528225e78..c49fe76fe6 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -431,26 +431,28 @@ static int write_message(const void *buf, size_t len, const char *filename,
>   static int read_oneliner(struct strbuf *buf,
>   	const char *path, int skip_if_empty)
>   {
> -	int orig_len = buf->len;
> +	int ret = 0;
> +	struct strbuf file_buf = STRBUF_INIT;
>   
>   	if (!file_exists(path))
>   		return 0;
>   
> -	if (strbuf_read_file(buf, path, 0) < 0) {
> +	if (strbuf_read_file(&file_buf, path, 0) < 0) {
>   		warning_errno(_("could not read '%s'"), path);
> -		return 0;
> +		goto done;
>   	}
>   
> -	if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
> -		if (--buf->len > orig_len && buf->buf[buf->len - 1] == '\r')
> -			--buf->len;
> -		buf->buf[buf->len] = '\0';
> -	}
> +	strbuf_trim_trailing_newline(&file_buf);
>   
> -	if (skip_if_empty && buf->len == orig_len)
> -		return 0;
> +	if (skip_if_empty && !file_buf.len)
> +		goto done;
>   
> -	return 1;
> +	strbuf_addbuf(buf, &file_buf);
> +	ret = 1;
> +
> +done:
> +	strbuf_release(&file_buf);
> +	return ret;
>   }
>   
>   static struct tree *empty_tree(struct repository *r)
>