| Message ID | 7c37777f07191c8ac1d26300f9465b90758550b2.1584782450.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | merge: learn --autostash | expand |
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) >
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)
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(-)