[v3,03/19] sequencer: use file strbuf for read_oneliner()
diff mbox series

Message ID 7c37777f07191c8ac1d26300f9465b90758550b2.1584782450.git.liu.denton@gmail.com
State New
Headers show
Series
  • merge: learn --autostash
Related show

Commit Message

Denton Liu March 21, 2020, 9:21 a.m. UTC
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.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 sequencer.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

Comments

Phillip Wood April 2, 2020, 1:34 p.m. UTC | #1
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)
>

Patch
diff mbox series

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)