Re: [PATCH] sequencer: factor out strbuf_read_file_or_whine()

[Jeff King]

On Thu, Feb 22, 2018 at 08:29:25PM +0100, René Scharfe wrote:

> 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 
> ---
> 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?

I'm fine with this patch, but FWIW I think reporting the result of
strbuf_read_file with error_errno() would actually be an improvement.
The errno values are generally sufficient to tell if the problem was in
opening or reading, and then we'd get more information in the case of a
failed read() call.

Thought note...

> 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;
> +}

If we were to use error_errno() in the second conditional here, we
should take care not to clobber errno during the close(). I think
strbuf_read_file() actually has the same problem, which might be worth
fixing.

-Peff

Re: [PATCH] sequencer: factor out strbuf_read_file_or_whine()

[Junio C Hamano]

René Scharfe  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 
> ---
> 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(_list->buf);
> - fd = open(todo_file, O_RDONLY);
> - if (fd < 0)
> - return error_errno(_("could not open '%s'"), todo_file);
> - if (strbuf_read(_list->buf, fd, 0) < 0) {
> - close(fd);
> - return error(_("could not read '%s'."), todo_file);
> - }
> - close(fd);
> + if (strbuf_read_file_or_whine(_list->buf, todo_file) < 0)
> + return -1;
>  
>   res = stat(todo_file, );
>   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(_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(_list.buf, fd, 0) < 0) {
> - close(fd);
> - res = error(_("could not read '%s'."), todo_file.buf);
> + if (strbuf_read_file_or_whine(_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, _list);
>  
> @@ -3180,17 +3182,10 @@ int check_todo_list(void)
>  
>   todo_list_release(_list);
>   strbuf_addstr(_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(_list.buf, fd, 0) < 0) {
> - close(fd);
> - res = error(_("could not read '%s'."), todo_file.buf);
> + if (strbuf_read_file_or_whine(_list.buf, todo_file.buf) < 0) {
> + res = -1;
>   goto leave_check;
>   }
> - close(fd);
>   strbuf_release(_file);
>   res = !!parse_insn_buffer(todo_list.buf.buf, _list);
>  
> @@ -3271,15 +3266,8 @@ int skip_unnecessary_picks(void)
>   }
>   strbuf_release();
>  
> - fd = open(todo_file, O_RDONLY);
> - if (fd < 0) {
> - return error_errno(_("could not open '%s'"), todo_file);
> - }
> - if (strbuf_read(_list.buf, fd, 0) < 0) {
> - close(fd);
> - return error(_("could not read '%s'."), todo_file);
> - }
> - close(fd);
> + if (strbuf_read_file_or_whine(_list.buf, todo_file) < 0)
> + return -1;
>   if (parse_insn_buffer(todo_list.buf.buf, _list) < 0) {
>   todo_list_release(_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;
>  
> -