Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Emily Shaffer <nasamuffin@google.com>
Cc: "brian m. carlson" <sandals@crustytoothpaste.net>,
	Git List <git@vger.kernel.org>, Jonathan Nieder <jrn@google.com>,
	Jose Lopes <jabolopes@google.com>,
	Aleksandr Mikhailov <avmikhailov@google.com>
Subject: Re: Proposal/Discussion: Turning parts of Git into libraries
Date: Fri, 24 Feb 2023 16:41:31 -0500	[thread overview]
Message-ID: <Y/kvC9+VVo80npe3@coredump.intra.peff.net> (raw)
In-Reply-To: <CAJoAoZknYizS4peYgR4Zy5KUMEpFUbj5eREZoC_K5vUDXnAhng@mail.gmail.com>

On Fri, Feb 24, 2023 at 12:31:16PM -0800, Emily Shaffer wrote:

> > One thing that strategy doesn't help with, though, that your
> > libification might want: it's not very machine-readable. The error
> > reporters would still fundamentally be working with strings. So a
> > libified process can know "OK, writing this ref failed, and I have some
> > error messages in a buffer". But the calling code can't know specifics
> > like "it failed because we tried to open file 'foo' and it got EPERM".
> > We _could_ design an error context that stores individual errno values
> > or codes in a list, but having each caller report those specific errors
> > is a much bigger job (and ongoing maintenance burden as we catalogue and
> > give an identifier to each error).
> 
> Is there a reason not to use this kind of struct and provide
> library-specific error code enums, though, I wonder? You're right that
> parsing the error string is really bad for the caller, for anything
> besides just logging it. But it seems somewhat reasonable to expect
> that any call from config library returning an integer error code is
> referring to enum config_errors...

Right, you could definitely layer the two approaches by storing the
enums in the struct. And that might be a good thing to do in the long
run. But I think it's a much harder change, as it implies assigning
those codes (and developing a taxonomy of errors). But that can also be
done incrementally, especially if it's done on top of human-readable
strings.

I.e., I can imagine a world where low-level code reports an error like:

   int read_foo(const char *path, struct error_context *err)
   {
           fd = open(path, ...);
           if (fd < 0)
                   return report_errno(&err, ERR_FOO_OPEN, "unable to open foo file '%s'", path);
           ...read and parse...
           if (some_unexpected_format)
                   return report(&err, ERR_FOO_FORMAT, "unable to parse foo file '%s'", path);
   }

and then the caller has many options:

  - pass in a context that just dumps the human readable errors to
    stderr

  - collect the error strings in a buffer to report by some other
    mechanism

  - check err.type to act on ERR_FOO_OPEN, etc, including errno (which
    I'd imagine report_errno() to record)

The details above are just a sketch, of course. Rather than FOO_OPEN,
callers may actually want to think of things more like a stack of
errors that would mirror the callstack. You could imagine something
like:

  type=ERR_REF_WRITE
  type=ERR_GET_LOCK
  type=ERR_OPEN, errno=EPERM

I do think it's possible to over-engineer this to the point of
absurdity, and end up creating a lot of work. Which is why I'd probably
start with uniformly trying to use an error context struct with strings,
after which adding on fancier features gets easier (and again, is
possibly something that can be done incrementally as various subsystems
support it).

-Peff

  reply	other threads:[~2023-02-24 21:41 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-17 21:12 Proposal/Discussion: Turning parts of Git into libraries Emily Shaffer
2023-02-17 21:21 ` brian m. carlson
2023-02-17 21:38   ` Emily Shaffer
2023-02-17 22:41     ` brian m. carlson
2023-02-17 22:49       ` Emily Shaffer
2023-02-22 19:34         ` Jeff King
2023-02-24 20:31           ` Emily Shaffer
2023-02-24 21:41             ` Jeff King [this message]
2023-02-24 22:59             ` Junio C Hamano
2023-02-17 22:04   ` rsbecker
2023-02-17 22:48     ` brian m. carlson
2023-02-17 22:57 ` Junio C Hamano
2023-02-18  1:59   ` demerphq
2023-02-18 10:36     ` Phillip Wood
2023-03-23 23:22       ` Felipe Contreras
2023-03-23 23:30         ` rsbecker
2023-03-23 23:34           ` Felipe Contreras
2023-03-23 23:42             ` rsbecker
2023-03-23 23:55               ` Felipe Contreras
2023-03-24 19:27                 ` rsbecker
2023-03-24 21:21                   ` Felipe Contreras
2023-03-24 22:06                     ` rsbecker
2023-03-24 22:29                       ` Felipe Contreras
2023-02-21 21:42   ` Emily Shaffer
2023-02-22  0:22     ` Junio C Hamano
2023-02-18  4:05 ` Elijah Newren
2023-02-21 22:06   ` Emily Shaffer
2023-02-22  8:23     ` Elijah Newren
2023-02-22 19:25     ` Jeff King
2023-02-21 19:09 ` Taylor Blau
2023-02-21 22:27   ` Emily Shaffer
2023-02-22  1:44 ` Victoria Dye
2023-02-25  1:48   ` Jonathan Tan
2023-02-22 14:55 ` Derrick Stolee
2023-02-24 21:06   ` Emily Shaffer
2023-03-23 23:37 ` Felipe Contreras
2023-03-23 23:44   ` rsbecker

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y/kvC9+VVo80npe3@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avmikhailov@google.com \
    --cc=git@vger.kernel.org \
    --cc=jabolopes@google.com \
    --cc=jrn@google.com \
    --cc=nasamuffin@google.com \
    --cc=sandals@crustytoothpaste.net \
    /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.
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).