Re: [RFC PATCH 1/1] Add a type for errors

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Oct 2, 2024, 21:51 UTC
Message-ID: <Zv3AZKjcOoAKOzLn@tapette.crustytoothpaste.net>
In-Reply-To: <CAJoAoZm2RJnu52Pb-vr0VWS_GQkL5n=DZs7KU50EZyuEYhawYw@mail.gmail.com>

On 2024-10-01 at 20:31:15, Emily Shaffer wrote:

Show 11 quoted lines

> On Mon, Sep 30, 2024 at 3:04 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > +const char *git_error_strbuf(struct strbuf *buf, struct git_error err)
> 
> Is the idea that we continue to extend `git_error_strbuf` with more
> `git_error_code` values as we add this method of error handling across
> the codebase? I'm worried it could get quite unwieldy. Or are you
> suggesting that we could write `git_error_strbuf_object_store`,
> `git_error_strbuf_hook`, etc to keep the code->string conversion
> local? That would let us do custom processing of err.meta depending on
> context, too, wouldn't it?

My intention was to centralize this processing in one place. Part of the problem we have is that we have many different messages for one error code, so `GIT_ERR_OBJECT_NOT_FOUND` might have "object not found - (HEX)" or a variety of other messages, which makes parsing errors difficult. (As I said, we do this at work, and it's quite annoying.)

If folks want to do thing differently, we can, but we should avoid losing the consistent error message behaviour.

Show 15 quoted lines

> > +#define GIT_ERROR_SUCCESS(e) (((e).code == GIT_ERROR_BIT_INIT))
> > +#define GIT_ERROR_SUCCESS_CONSUME(e) ((git_error_free(&(e)).code == GIT_ERROR_BIT_INIT)
> > +
> > +#define GIT_ERROR_ERRNO(e) (((e).code & GIT_ERROR_BIT_ERRNO) ? ((e).code & 0xffffffff) : -1)
> > +#define GIT_ERROR_GITERR(e) (((e).code & GIT_ERROR_BIT_ERRNO) ? -1 : ((e).code & 0xffffffff))
> 
> Aurgh, too bad we don't get bitfields before C11. :) (Although I am
> not sure if that helps with your register-level optimization at that
> point... but it probably helps with readability.)
> 
> But, I do wonder if this gluing-together-two-types-into-one-value
> thing may break based on endianness? (And if we care? I don't think we
> have any tests running on POWER systems, so maybe this falls under the
> umbrella of "you should give us tests if you want us to not break
> you"?)

I don't think this breaks on big-endian systems because we're always treating it as a `uint64_t` and never converting it into bytes. My first laptop was a PowerPC Mac running Linux and I've owned UltraSPARC hardware in the past, so I'm pretty confident in avoiding writing endianness (and alignment) bugs. I wouldn't have written it in a way that broke on less common hardware even if we don't have tests for it.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA