From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 02 Apr 2020 13:41:27 GMT
Subject: Re: [PATCH v3 07/19] rebase: use read_oneliner()
Message-ID: <51bbff1e-ccca-5d13-24f1-942dcfe6174d@gmail.com>
In-Reply-To: <d07a50c5b22128263aaa8eef94546fe8bc8a9ec8.1584782450.git.liu.denton@gmail.com>

Hi Denton

On 21/03/2020 09:21, Denton Liu wrote:
> Since in sequencer.c, read_one() basically duplicates the functionality
> of read_oneliner(), reduce code duplication by replacing read_one() with
> read_oneliner().
> 
> This was done with the following Coccinelle script
> 
> 	@@
> 	expression a, b;
> 	@@
> 	- read_one(a, b)
> 	+ !read_oneliner(b, a, READ_ONELINER_WARN_NON_EXISTENCE)
> 
> and long lines were manually broken up.

It's great to see read_one() going away

Thanks

Phillip

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>   builtin/rebase.c | 37 +++++++++++++++++--------------------
>   1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index bff53d5d16..1146463099 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -586,15 +586,6 @@ static const char *state_dir_path(const char *filename, struct rebase_options *o
>   	return path.buf;
>   }
>   
> -/* Read one file, then strip line endings */
> -static int read_one(const char *path, struct strbuf *buf)
> -{
> -	if (strbuf_read_file(buf, path, 0) < 0)
> -		return error_errno(_("could not read '%s'"), path);
> -	strbuf_trim_trailing_newline(buf);
> -	return 0;
> -}
> -
>   /* Initialize the rebase options from the state directory. */
>   static int read_basic_state(struct rebase_options *opts)
>   {
> @@ -602,8 +593,10 @@ static int read_basic_state(struct rebase_options *opts)
>   	struct strbuf buf = STRBUF_INIT;
>   	struct object_id oid;
>   
> -	if (read_one(state_dir_path("head-name", opts), &head_name) ||
> -	    read_one(state_dir_path("onto", opts), &buf))
> +	if (!read_oneliner(&head_name, state_dir_path("head-name", opts),
> +			   READ_ONELINER_WARN_NON_EXISTENCE) ||
> +	    !read_oneliner(&buf, state_dir_path("onto", opts),
> +			   READ_ONELINER_WARN_NON_EXISTENCE))
>   		return -1;
>   	opts->head_name = starts_with(head_name.buf, "refs/") ?
>   		xstrdup(head_name.buf) : NULL;
> @@ -619,9 +612,11 @@ static int read_basic_state(struct rebase_options *opts)
>   	 */
>   	strbuf_reset(&buf);
>   	if (file_exists(state_dir_path("orig-head", opts))) {
> -		if (read_one(state_dir_path("orig-head", opts), &buf))
> +		if (!read_oneliner(&buf, state_dir_path("orig-head", opts),
> +				   READ_ONELINER_WARN_NON_EXISTENCE))
>   			return -1;
> -	} else if (read_one(state_dir_path("head", opts), &buf))
> +	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
> +				  READ_ONELINER_WARN_NON_EXISTENCE))
>   		return -1;
>   	if (get_oid(buf.buf, &opts->orig_head))
>   		return error(_("invalid orig-head: '%s'"), buf.buf);
> @@ -641,8 +636,8 @@ static int read_basic_state(struct rebase_options *opts)
>   
>   	if (file_exists(state_dir_path("allow_rerere_autoupdate", opts))) {
>   		strbuf_reset(&buf);
> -		if (read_one(state_dir_path("allow_rerere_autoupdate", opts),
> -			    &buf))
> +		if (!read_oneliner(&buf, state_dir_path("allow_rerere_autoupdate", opts),
> +				   READ_ONELINER_WARN_NON_EXISTENCE))
>   			return -1;
>   		if (!strcmp(buf.buf, "--rerere-autoupdate"))
>   			opts->allow_rerere_autoupdate = RERERE_AUTOUPDATE;
> @@ -655,8 +650,8 @@ static int read_basic_state(struct rebase_options *opts)
>   
>   	if (file_exists(state_dir_path("gpg_sign_opt", opts))) {
>   		strbuf_reset(&buf);
> -		if (read_one(state_dir_path("gpg_sign_opt", opts),
> -			    &buf))
> +		if (!read_oneliner(&buf, state_dir_path("gpg_sign_opt", opts),
> +				   READ_ONELINER_WARN_NON_EXISTENCE))
>   			return -1;
>   		free(opts->gpg_sign_opt);
>   		opts->gpg_sign_opt = xstrdup(buf.buf);
> @@ -664,7 +659,8 @@ static int read_basic_state(struct rebase_options *opts)
>   
>   	if (file_exists(state_dir_path("strategy", opts))) {
>   		strbuf_reset(&buf);
> -		if (read_one(state_dir_path("strategy", opts), &buf))
> +		if (!read_oneliner(&buf, state_dir_path("strategy", opts),
> +				   READ_ONELINER_WARN_NON_EXISTENCE))
>   			return -1;
>   		free(opts->strategy);
>   		opts->strategy = xstrdup(buf.buf);
> @@ -672,7 +668,8 @@ static int read_basic_state(struct rebase_options *opts)
>   
>   	if (file_exists(state_dir_path("strategy_opts", opts))) {
>   		strbuf_reset(&buf);
> -		if (read_one(state_dir_path("strategy_opts", opts), &buf))
> +		if (!read_oneliner(&buf, state_dir_path("strategy_opts", opts),
> +				   READ_ONELINER_WARN_NON_EXISTENCE))
>   			return -1;
>   		free(opts->strategy_opts);
>   		opts->strategy_opts = xstrdup(buf.buf);
> @@ -724,7 +721,7 @@ static int apply_autostash(struct rebase_options *opts)
>   	if (!file_exists(path))
>   		return 0;
>   
> -	if (read_one(path, &autostash))
> +	if (!read_oneliner(&autostash, path, READ_ONELINER_WARN_NON_EXISTENCE))
>   		return error(_("Could not read '%s'"), path);
>   	/* Ensure that the hash is not mistaken for a number */
>   	strbuf_addstr(&autostash, "^0");
>