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

[Jeff King <peff@peff.net>]

On Sat, Nov 04, 2017 at 10:05:43AM +0100, René Scharfe wrote:

> >> How about *not* printing the error at the place where you notice the
> >> error, and instead return an error code to the caller to be noticed
> >> which dies with an error message?
> > 
> > That ends up giving less-specific errors.
> 
> Not necessarily.  Function could return different codes for different
> errors, e.g. -1 for an open(2) error and -2 for a write(2) error, and
> the caller could use that to select the message to show.
> 
> Basically all of the messages in wrapper.c consist of some text mixed
> with the affected path path and a strerror(3) string, so they're
> compatible in that way.  A single function (get_path_error_format()?)
> could thus be used and callers would be able to combine its result with
> die(), error(), or warning().

I think we've had this discussion before, a while ago. Yes, returning an
integer error code is nice because you don't have pass in an extra
parameter. But I think there are two pitfalls:

  1. Integers may not be descriptive enough to cover all cases, which is
     how we ended up with the strbuf-passing strategy in the ref code.
     Certainly you could add an integer for every possible bespoke
     message, but then I'm not sure it's buying that much over having
     the function simply fill in a strbuf.

  2. For complex functions there may be multiple errors that need to
     stack. I think the refs code has cases like this, where a syscall
     fails, which causes a fundamental ref operation to fail, which
     causes a higher-level operation to fail. It's only the caller of
     the higher-level operation that knows how to report the error.

Certainly an integer error code would work for _this_ function, but I'd
rather see us grow towards consistent error handling.

-Peff