From: Jeff King <peff@peff.net>
To: Simon Ruderich <simon@ruderich.org>
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
"René Scharfe" <l.s.r@web.de>, "Git List" <git@vger.kernel.org>,
"Ralf Thielow" <ralf.thielow@gmail.com>
Subject: Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())
Date: Fri, 17 Nov 2017 17:33:46 -0500 [thread overview]
Message-ID: <20171117223345.s3ihubgda3qdb2j6@sigill.intra.peff.net> (raw)
In-Reply-To: <20171106161315.dmftp6ktk6bu7cah@ruderich.org>
On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote:
> 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.
> 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.
> 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.
> 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.
> 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.
next prev parent reply other threads:[~2017-11-17 22:33 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-31 9:54 [PATCH 1/2] sequencer: factor out rewrite_file() René Scharfe
2017-10-31 9:58 ` [PATCH 2/2] sequencer: use O_TRUNC to truncate files René Scharfe
2017-10-31 16:34 ` Kevin Daudt
2017-11-01 15:34 ` Johannes Schindelin
2017-10-31 16:33 ` [PATCH 1/2] sequencer: factor out rewrite_file() Kevin Daudt
2017-11-01 6:06 ` Kevin Daudt
2017-11-01 11:10 ` Simon Ruderich
2017-11-01 13:00 ` René Scharfe
2017-11-01 14:44 ` [PATCH 1/2] wrapper.c: consistently quote filenames in error messages Simon Ruderich
2017-11-02 4:40 ` Junio C Hamano
2017-11-02 5:16 ` Junio C Hamano
2017-11-02 10:20 ` Simon Ruderich
2017-11-03 1:47 ` Junio C Hamano
2017-11-01 14:45 ` [PATCH 2/2] sequencer.c: check return value of close() in rewrite_file() Simon Ruderich
2017-11-01 17:09 ` René Scharfe
2017-11-01 15:33 ` [PATCH 1/2] sequencer: factor out rewrite_file() Johannes Schindelin
2017-11-01 19:47 ` Jeff King
2017-11-01 21:46 ` Johannes Schindelin
2017-11-01 22:16 ` Jeff King
2017-11-03 10:32 ` Simon Ruderich
2017-11-03 13:44 ` Junio C Hamano
2017-11-03 19:13 ` Jeff King
2017-11-04 9:05 ` René Scharfe
2017-11-04 9:35 ` Jeff King
2017-11-04 18:36 ` Simon Ruderich
2017-11-05 2:07 ` Jeff King
2017-11-06 16:13 ` Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) Simon Ruderich
2017-11-16 10:36 ` Simon Ruderich
2017-11-17 22:33 ` Jeff King [this message]
2017-11-18 9:01 ` Johannes Sixt
2017-12-24 14:54 ` Jeff King
2017-12-24 15:45 ` Randall S. Becker
2017-12-25 10:28 ` Johannes Sixt
2017-11-03 14:46 ` [PATCH 1/2] sequencer: factor out rewrite_file() Johannes Schindelin
2017-11-03 18:57 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171117223345.s3ihubgda3qdb2j6@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
--cc=ralf.thielow@gmail.com \
--cc=simon@ruderich.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).