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

[Phillip Wood <phillip.wood123@gmail.com>]

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)

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@gmail.com>
> ---
>   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;
>