Re: [PATCH v4 04/23] sequencer: reuse strbuf_trim() in read_oneliner()

[Phillip Wood <phillip.wood123@xxxxxxxxx>]

Hi Denton

On 06/04/2020 15:03, Phillip Wood wrote:
> Hi Denton
>
> On 04/04/2020 02:11, 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.
> >
> > Reset `buf` before using it then replace the trimming logic with
> > strbuf_trim().
>
> I should have picked up on this before but this changes the semantics of 
> the function as it strips all whitespace from the start and end of the 
> strbuf. Above you talked about using strbuf_trim_trailing_newline() 
> instead which would not change the semantics of this function. You could 
> test to see if we've read anything and only call 
> strbuf_trim_trailing_newline() in that case without messing with 
> strbuf_reset(). (there is a corner case where if the buffer ends with 
> '\r' when the function is called and it reads a single '\n' then the 
> '\r' would be stripped as well but I think that is unlikely to happen in 
> the wild)

I'd also be fine with dropping this patch and leaving the trimming code 
as is

Best Wishes

Phillip

> Best Wishes
>
> Phillip
>
> > As a cleanup, remove all reset_strbuf()s that happen before
> > read_oneliner() is called.
> >
> > Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
> > ---
> >   sequencer.c | 18 +++++-------------
> >   1 file changed, 5 insertions(+), 13 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index faab0b13e8..09ca68f540 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -420,8 +420,8 @@ static int write_message(const void *buf, size_t 
> > len, const char *filename,
> >   }
> >   /*
> > - * Reads a file that was presumably written by a shell script, i.e. 
> > with an
> > - * end-of-line marker that needs to be stripped.
> > + * Resets a strbuf then reads a file that was presumably written by a 
> > shell
> > + * script, i.e. with an end-of-line marker that needs to be stripped.
> >    *
> >    * Note that only the last end-of-line marker is stripped, 
> > consistent with the
> >    * behavior of "$(cat path)" in a shell script.
> > @@ -431,23 +431,19 @@ 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;
> >       if (!file_exists(path))
> >           return 0;
> > +    strbuf_reset(buf);
> >       if (strbuf_read_file(buf, path, 0) < 0) {
> >           warning_errno(_("could not read '%s'"), path);
> >           return 0;
> >       }
> > -    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(buf);
> > -    if (skip_if_empty && buf->len == orig_len)
> > +    if (skip_if_empty && !buf->len)
> >           return 0;
> >       return 1;
> > @@ -2471,7 +2467,6 @@ void parse_strategy_opts(struct replay_opts 
> > *opts, char *raw_opts)
> >   static void read_strategy_opts(struct replay_opts *opts, struct 
> > strbuf *buf)
> >   {
> > -    strbuf_reset(buf);
> >       if (!read_oneliner(buf, rebase_path_strategy(), 0))
> >           return;
> >       opts->strategy = strbuf_detach(buf, NULL);
> > @@ -2494,7 +2489,6 @@ static int read_populate_opts(struct replay_opts 
> > *opts)
> >                   free(opts->gpg_sign);
> >                   opts->gpg_sign = xstrdup(buf.buf + 2);
> >               }
> > -            strbuf_reset(&buf);
> >           }
> >           if (read_oneliner(&buf, 
> > rebase_path_allow_rerere_autoupdate(), 1)) {
> > @@ -2526,7 +2520,6 @@ static int read_populate_opts(struct replay_opts 
> > *opts)
> >               opts->keep_redundant_commits = 1;
> >           read_strategy_opts(opts, &buf);
> > -        strbuf_reset(&buf);
> >           if (read_oneliner(&opts->current_fixups,
> >                     rebase_path_current_fixups(), 1)) {
> > @@ -4006,7 +3999,6 @@ static int pick_commits(struct repository *r,
> >                   res = error(_("could not read orig-head"));
> >                   goto cleanup_head_ref;
> >               }
> > -            strbuf_reset(&buf);
> >               if (!read_oneliner(&buf, rebase_path_onto(), 0)) {
> >                   res = error(_("could not read 'onto'"));
> >                   goto cleanup_head_ref;