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

On Fri, Nov 03, 2017 at 03:13:10PM -0400, Jeff King wrote:

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.

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

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

Regards Simon