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@xxxxxx>
> ---
> 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.
