René Scharfe <l....@web.de> writes:

> Reduce code duplication by factoring out a function that reads an entire
> file into a strbuf, or reports errors on stderr if something goes wrong.
>
> Signed-off-by: Rene Scharfe <l....@web.de>
> ---
> The difference to using strbuf_read_file() is more detailed error
> messages for open(2) failures.  But I don't know if we need them -- or
> under which circumstances reading todo files could fail anyway.  When
> doing multiple rebases in parallel perhaps?
>
>  sequencer.c | 74 
> +++++++++++++++++++++++--------------------------------------
>  1 file changed, 28 insertions(+), 46 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e9baaf59bd..e34334f0ef 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1869,22 +1869,31 @@ static int count_commands(struct todo_list *todo_list)
>       return count;
>  }
>  
> +static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)
> +{
> +     int fd;
> +     ssize_t len;
> +
> +     fd = open(path, O_RDONLY);
> +     if (fd < 0)
> +             return error_errno(_("could not open '%s'"), path);
> +     len = strbuf_read(sb, fd, 0);
> +     close(fd);
> +     if (len < 0)
> +             return error(_("could not read '%s'."), path);
> +     return len;
> +}
> +

This looks like a good granularity of a unit of independent work.
The original we see below looks like it was written with scissors
and glue ;-)

It appears to me that no topic in flight introduce more instances
that need to be converted with a quick trial merge to 'pu', so I'll
queue this forked at the tip of 'master'.

Thanks.

>  static int read_populate_todo(struct todo_list *todo_list,
>                       struct replay_opts *opts)
>  {
>       struct stat st;
>       const char *todo_file = get_todo_path(opts);
> -     int fd, res;
> +     int res;
>  
>       strbuf_reset(&todo_list->buf);
> -     fd = open(todo_file, O_RDONLY);
> -     if (fd < 0)
> -             return error_errno(_("could not open '%s'"), todo_file);
> -     if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
> -             close(fd);
> -             return error(_("could not read '%s'."), todo_file);
> -     }
> -     close(fd);
> +     if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
> +             return -1;
>  
>       res = stat(todo_file, &st);
>       if (res)
> @@ -3151,20 +3160,13 @@ int check_todo_list(void)
>       struct strbuf todo_file = STRBUF_INIT;
>       struct todo_list todo_list = TODO_LIST_INIT;
>       struct strbuf missing = STRBUF_INIT;
> -     int advise_to_edit_todo = 0, res = 0, fd, i;
> +     int advise_to_edit_todo = 0, res = 0, i;
>  
>       strbuf_addstr(&todo_file, rebase_path_todo());
> -     fd = open(todo_file.buf, O_RDONLY);
> -     if (fd < 0) {
> -             res = error_errno(_("could not open '%s'"), todo_file.buf);
> -             goto leave_check;
> -     }
> -     if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> -             close(fd);
> -             res = error(_("could not read '%s'."), todo_file.buf);
> +     if (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {
> +             res = -1;
>               goto leave_check;
>       }
> -     close(fd);
>       advise_to_edit_todo = res =
>               parse_insn_buffer(todo_list.buf.buf, &todo_list);
>  
> @@ -3180,17 +3182,10 @@ int check_todo_list(void)
>  
>       todo_list_release(&todo_list);
>       strbuf_addstr(&todo_file, ".backup");
> -     fd = open(todo_file.buf, O_RDONLY);
> -     if (fd < 0) {
> -             res = error_errno(_("could not open '%s'"), todo_file.buf);
> -             goto leave_check;
> -     }
> -     if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> -             close(fd);
> -             res = error(_("could not read '%s'."), todo_file.buf);
> +     if (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {
> +             res = -1;
>               goto leave_check;
>       }
> -     close(fd);
>       strbuf_release(&todo_file);
>       res = !!parse_insn_buffer(todo_list.buf.buf, &todo_list);
>  
> @@ -3271,15 +3266,8 @@ int skip_unnecessary_picks(void)
>       }
>       strbuf_release(&buf);
>  
> -     fd = open(todo_file, O_RDONLY);
> -     if (fd < 0) {
> -             return error_errno(_("could not open '%s'"), todo_file);
> -     }
> -     if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> -             close(fd);
> -             return error(_("could not read '%s'."), todo_file);
> -     }
> -     close(fd);
> +     if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)
> +             return -1;
>       if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
>               todo_list_release(&todo_list);
>               return -1;
> @@ -3370,17 +3358,11 @@ int rearrange_squash(void)
>       const char *todo_file = rebase_path_todo();
>       struct todo_list todo_list = TODO_LIST_INIT;
>       struct hashmap subject2item;
> -     int res = 0, rearranged = 0, *next, *tail, fd, i;
> +     int res = 0, rearranged = 0, *next, *tail, i;
>       char **subjects;
>  
> -     fd = open(todo_file, O_RDONLY);
> -     if (fd < 0)
> -             return error_errno(_("could not open '%s'"), todo_file);
> -     if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> -             close(fd);
> -             return error(_("could not read '%s'."), todo_file);
> -     }
> -     close(fd);
> +     if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)
> +             return -1;
>       if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
>               todo_list_release(&todo_list);
>               return -1;

Reply via email to