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

On Sat, Nov 04, 2017 at 07:36:43PM +0100, Simon Ruderich wrote:

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

quoted

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.

Yes, I think what you've written here (and below) is quite close to the
error_context patches I linked elsewhere in the thread. In other
words, I think it's a sane approach.

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

I agree it might be nice for the error context to have a positive "there
was an error" flag. It's probably worth making it redundant with the
return code, though, so callers can use whichever style is most
convenient for them.

quoted

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.

Right, I didn't think that example through. The functions after the
open() don't have enough information to make a good message.

-Peff