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

[Simon Ruderich <simon@ruderich.org>]

On Fri, Nov 03, 2017 at 03:13:10PM -0400, Jeff King wrote:
> I think we've been gravitating towards error strbufs, which would make
> it something like:

I like this approach to store the error in a separate variable
and let the caller handle it. This provides proper error messages
and is cleaner than printing the error on the error site (what
error_errno does).

However I wouldn't use strbuf directly and instead add a new
struct error which provides a small set of helper functions.
Using a separate type also makes it clear to the reader that is
not a normal string and is more extendable in the future.

> I'm not excited that the amount of error-handling code is now double the
> amount of code that actually does something useful. Maybe this function
> simply isn't large/complex enough to merit flexible error handling, and
> we should simply go with René's original near-duplicate.

A separate struct (and helper functions) would help in this case
and could look like this, which is almost equal (in code size) to
the original solution using error_errno:

    int write_file_buf_gently2(const char *path, const char *buf, size_t len, struct error *err)
    {
            int rc = 0;
            int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
            if (fd < 0)
                    return error_addf_errno(err, _("could not open '%s' for writing"), path);
            if (write_in_full(fd, buf, len) < 0)
                    rc = error_addf_errno(err, _("could not write to '%s'"), path);
            if (close(fd) && !rc)
                    rc = error_addf_errno(err, _("could not close '%s'"), path);
            return rc;
    }

(I didn't touch write_in_full here, but it could also take the
err and then the code would get a little shorter, however would
lose the "path" information, but see below.)

And in the caller:

    void write_file_buf(const char *path, const char *buf, size_t len)
    {
            struct error err = ERROR_INIT;
            if (write_file_buf_gently2(path, buf, len, &err) < 0)
                    error_die(&err);
    }

For now struct error just contains the strbuf, but one could add
the call location (by using a macro for error_addf_errno) or the
original errno or more information in the future.

error_addf_errno() could also prepend the error the buffer so
that the caller can add more information if necessary and we get
something like: "failed to write file 'foo': write failed: errno
text" in the write_file_buf case (the first error string is from
write_file_buf_gently2, the second from write_in_full). However
I'm not sure how well this works with translations.

We could also store the error condition in the error struct and
don't use the return value to indicate and error like this:

    void write_file_buf(const char *path, const char *buf, size_t len)
    {
            struct error err = ERROR_INIT;
            write_file_buf_gently2(path, buf, len, &err);
            if (err.error)
                    error_die(&err);
    }

> OTOH, if we went all-in on flexible error handling contexts, you could
> imagine this function becoming:
>
>   void write_file_buf(const char *path, const char *buf, size_t len,
>                       struct error_context *err)
>   {
> 	int fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> 	if (fd < 0)
> 		return -1;
> 	if (write_in_full(fd, buf, len, err) < 0)
> 		return -1;
> 	if (xclose(fd, err) < 0)
> 		return -1;
> 	return 0;
>   }

This looks interesting as well, but it misses the feature of
custom error messages which is really useful.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9