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

[Jeff King <peff@peff.net>]

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:
> > 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.

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