Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Kevin Daudt <me@ikke.info>]

On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:
> Reduce code duplication by extracting a function for rewriting an
> existing file.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  sequencer.c | 46 +++++++++++++++++-----------------------------
>  1 file changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index f2a10cc4f2..17360eb38a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2665,6 +2665,20 @@ int check_todo_list(void)
>  	return res;
>  }
>  
> +static int rewrite_file(const char *path, const char *buf, size_t len)
> +{
> +	int rc = 0;
> +	int fd = open(path, O_WRONLY);
> +	if (fd < 0)
> +		return error_errno(_("could not open '%s' for writing"), path);
> +	if (write_in_full(fd, buf, len) < 0)
> +		rc = error_errno(_("could not write to '%s'"), path);
> +	if (!rc && ftruncate(fd, len) < 0)
> +		rc = error_errno(_("could not truncate '%s'"), path);
> +	close(fd);
> +	return rc;
> +}
> +
>  /* skip picking commits whose parents are unchanged */
>  int skip_unnecessary_picks(void)
>  {
> @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)
>  		}
>  		close(fd);
>  
> -		fd = open(rebase_path_todo(), O_WRONLY, 0666);
> -		if (fd < 0) {
> -			error_errno(_("could not open '%s' for writing"),
> -				    rebase_path_todo());
> +		if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,
> +				 todo_list.buf.len - offset) < 0) {
>  			todo_list_release(&todo_list);
>  			return -1;
>  		}
> -		if (write_in_full(fd, todo_list.buf.buf + offset,
> -				todo_list.buf.len - offset) < 0) {
> -			error_errno(_("could not write to '%s'"),
> -				    rebase_path_todo());
> -			close(fd);
> -			todo_list_release(&todo_list);

Is this missing on purpose in the new situation?

> -			return -1;
> -		}
> -		if (ftruncate(fd, todo_list.buf.len - offset) < 0) {
> -			error_errno(_("could not truncate '%s'"),
> -				    rebase_path_todo());
> -			todo_list_release(&todo_list);
> -			close(fd);
> -			return -1;
> -		}
> -		close(fd);
>  
>  		todo_list.current = i;
>  		if (is_fixup(peek_command(&todo_list, 0)))
> @@ -2944,15 +2940,7 @@ int rearrange_squash(void)
>  			}
>  		}
>  
> -		fd = open(todo_file, O_WRONLY);
> -		if (fd < 0)
> -			res = error_errno(_("could not open '%s'"), todo_file);
> -		else if (write(fd, buf.buf, buf.len) < 0)
> -			res = error_errno(_("could not write to '%s'"), todo_file);
> -		else if (ftruncate(fd, buf.len) < 0)
> -			res = error_errno(_("could not truncate '%s'"),
> -					   todo_file);
> -		close(fd);
> +		res = rewrite_file(todo_file, buf.buf, buf.len);
>  		strbuf_release(&buf);
>  	}
>  
> -- 
> 2.15.0

Except for that question, it looks good to me (as a beginner), it makes
the code better readable.