Re: [PATCH v3 03/19] sequencer: use file strbuf for read_oneliner()
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Apr 2, 2020, 13:34 UTC
- 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:
Show 10 quoted lines
> 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
Show 51 quoted lines
>
> 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)
>