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

From: Jeff King <peff@peff.net>
Date: Nov 4, 2017, 09:35 UTC
Message-ID: <20171104093537.glucmbljnjlw3htq@sigill.intra.peff.net>
In-Reply-To: <b074d6fa-8778-ea0d-d53b-7cc35bc4264a@web.de>

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

Show 15 quoted lines

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