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@xxxxxxxxx>
---
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)
