Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

From: Jeff King <peff@peff.net>
Date: Nov 17, 2017, 22:33 UTC
Message-ID: <20171117223345.s3ihubgda3qdb2j6@sigill.intra.peff.net>
In-Reply-To: <20171106161315.dmftp6ktk6bu7cah@ruderich.org>

On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote:

Show 9 quoted lines

> On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:
> > 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.
> 
> In contrast to error_context I'd like to keep all exiting
> behavior (die, ignore, etc.) in the hand of the caller and not
> use any callbacks as that makes the control flow much harder to
> follow.

Yeah, I have mixed feelings on that. I think it does make the control flow less clear. At the same time, what I found was that handlers like die/ignore/warn were the thing that gave the most reduction in complexity in the callers.

Show 6 quoted lines

> Regarding the API, should it be allowed to pass NULL as error
> pointer to request no additional error handling or should the
> error functions panic on NULL? Allowing NULL makes partial
> conversions possible (e.g. for write_in_full) where old callers
> just pass NULL and check the return values and converted callers
> can use the error struct.

I think it's probably better to be explicit, and pass some "noop" error handling struct. We'll have to be adding parameters to functions to handle this anyway, so I don't think there's much opportunity for having NULL as a fallback for partial conversions.

Show 5 quoted lines

> How should translations get handled? Appending ": %s" for
> strerror(errno) might be problematic. Same goes for "outer
> message: inner message" where the helper function just inserts ":
> " between the messages. Is _("%s: %s") (with appropriate
> translator comments) enough to handle these cases?

I don't have a real opinion, not having done much translation myself. I will say that the existing die_errno(), error_errno(), etc just use "%s: %s", without even allowing for translation (see fmt_with_err in usage.c). I'm sure that probably sucks for RTL languages, but I think it would be fine to punt on it for now.

Show 8 quoted lines

> Suggestions how to name the struct and the corresponding
> functions? My initial idea was struct error and to use error_ as
> prefix, but I'm not sure if struct error is too broad and may
> introduce conflicts with system headers. Also error_ is a little
> long and could be shorted to just err_ but I don't know if that's
> clear enough. The error_ prefix doesn't conflict with many git
> functions, but there are some in usage.c (error_errno, error,
> error_routine).

In my experiments[1] I called the types error_*, and then generally used "err" as a local variable when necessary. Variants on that seem fine to me, but yeah, you have to avoid conflicting with error(). We _could_ rename that, but it would be a pretty invasive patch.

Show 5 quoted lines

> And as general question, is this approach to error handling
> something we should pursue or are there objections? If there's
> consensus that this might be a good idea I'll look into
> converting some parts of the git code (maybe refs.c) to see how
> it pans out.

I dunno. I kind of like the idea, but if the only error context is one that adds to strbufs, I don't know that it's buying us much over the status quo (which is passing around strbufs). It's a little more explicit, I guess.

Other list regulars besides me seem mostly quiet on the subject.

-Peff

[1] This is the jk/error-context-wip branch of https://github.com/peff/git.
    I can't remember if I mentioned that before.