[RFC PATCH 0/1] Typed errors

[RFC PATCH 0/1] Typed errors

[brian m. carlson]

We've discussed, as part of the libification effors and at the most
recent Git Merge conference, the possibility of typed errors in Git.
This would be useful because it allows users of our library to
understand why an error occurred and what to do about it in addition to
knowing just that one occurred.

In addition, as someone who has done a lot of scripting of Git in my
professional life, I've noticed we have a plethora of different messages
for the same problem.  As a consequence, it's very difficult to
automatically map all errors where an object is absent into a distinct
error type, which makes automation hard.  It would be easier to work
with Git as a scripting aid if all error of the same type produced a
uniform message that could be easily parsed.

I had indicated that I have a design for this, which I've improved with
feedback from the contributor summit.  This is, therefore, an RFC that
doesn't wire things up in any way, but is suitable for discussion about
the approach and design. It may be that we eventually adopt this
approach, or we may discard it in favour of another, and either way,
that's fine.

I will say that the fact that we have a pointer here is useful, but it
means that freeing memory is necessary.  That's inconvenient, but if we
choose to adopt Rust on a larger scale, it should be much easier to
handle by simply using a `Drop` implementation to free the memory.

brian m. carlson (1):
  Add a type for errors

 Makefile |   1 +
 error.c  |  43 ++++++++++++++
 error.h  | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 212 insertions(+)
 create mode 100644 error.c
 create mode 100644 error.h

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

[brian m. carlson]

There is work underway to move some of the Git code out into a reusable
library.  In such a case, it's not very desirable to have the library
code write random errors to standard error, since this is an antipattern
which annoys terminal users.

Instead, we will expect callers of our library function to return
errors.  The reusability of our library will be substantially improved
if we return typed errors so that users can easily determine what kind
of error might have occurred and react differently based on different
contexts.  For example, if we are looking up an object in a partial
clone and it's not found, we might decide to download it, but we might
return an error to the user if the problem is instead that the revision
specified is not syntactically valid.

To help the libification process and make our code more generally
maintainable, add an error type.  This consists of on 64-bit integer,
which contains bit flags and a 32-bit code, and a pointer, which depends
on the code.  It is designed to be passed and returned by value, not
pointer, and it is possible to do so in two registers on 64-bit systems.
Similar functionality works well for error types in Rust and for the
standard library's lldiv_t, so this should not pose a problem.

Provide the ability to specify either an errno value or a git error code
as the code.  This allows us to use this type generically when handling
errno values such as processing files, as well as express a rich set of
possible error codes specific to Git.  We pick an unsigned 32-bit code
because Windows can use the full set of 32 bits in its error values,
even though most Unix systems use only a small set of codes which
traditionally start at 1.  32 bits for Git errors also allows us plenty
of space to expand as we see fit.

Allow multiple errors to be provided and wrapped in a single object,
which is useful in many situations, and add helpers to determine if any
error in the set matches a particular code.

Additionally, provide error formatting functions that produce a suitable
localized string for ease of use.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Makefile |   1 +
 error.c  |  43 ++++++++++++++
 error.h  | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 212 insertions(+)
 create mode 100644 error.c
 create mode 100644 error.h

diff --git a/Makefile b/Makefile
index 7344a7f725..5d9bf992e9 100644
--- a/Makefile
+++ b/Makefile
@@ -1013,6 +1013,7 @@ LIB_OBJS += dir.o
 LIB_OBJS += editor.o
 LIB_OBJS += entry.o
 LIB_OBJS += environment.o
+LIB_OBJS += error.o
 LIB_OBJS += ewah/bitmap.o
 LIB_OBJS += ewah/ewah_bitmap.o
 LIB_OBJS += ewah/ewah_io.o
diff --git a/error.c b/error.c
new file mode 100644
index 0000000000..713bc42187
--- /dev/null
+++ b/error.c
@@ -0,0 +1,43 @@
+#include "git-compat-util.h"
+#include "gettext.h"
+#include "error.h"
+#include "hex.h"
+#include "strbuf.h"
+
+const char *git_error_string(struct git_error err)
+{
+	struct strbuf buf = STRBUF_INIT;
+	if (!git_error_strbuf(&buf, err))
+		return NULL;
+	return strbuf_detach(&buf, NULL);
+}
+
+const char *git_error_strbuf(struct strbuf *buf, struct git_error err)
+{
+	if (GIT_ERROR_SUCCESS(err)) {
+		return NULL;
+	} else if (GIT_ERROR_ERRNO(err) != -1) {
+		return xstrdup(strerror(GIT_ERROR_ERRNO(err)));
+	} else {
+		struct git_error_multiple *me = err.meta;
+		switch (GIT_ERROR_GITERR(err)) {
+		case GIT_ERR_OBJECT_NOT_FOUND:
+			if (err.meta)
+				strbuf_addf(buf, _("object not found: %s"), oid_to_hex(err.meta));
+			else
+				strbuf_addf(buf, _("object not found"));
+		case GIT_ERR_NULL_OID:
+			if (err.meta)
+				strbuf_addf(buf, _("null object ID not allowed in this context: %s"), (char *)err.meta);
+			else
+				strbuf_addf(buf, _("null object ID not allowed"));
+		case GIT_ERR_MULTIPLE:
+			strbuf_addf(buf, _("multiple errors:\n"));
+			for (size_t i = 0; i < me->count; i++) {
+				git_error_strbuf(buf, me->errs[i]);
+				strbuf_addstr(buf, "\n");
+			}
+		}
+		return buf->buf;
+	}
+}
diff --git a/error.h b/error.h
new file mode 100644
index 0000000000..485cca99e0
--- /dev/null
+++ b/error.h
@@ -0,0 +1,168 @@
+#ifndef ERROR_H
+#define ERROR_H
+
+#include "git-compat-util.h"
+
+/* Set if this value is initialized. */
+#define GIT_ERROR_BIT_INIT (1ULL << 63)
+/* Set if the code is an errno code, clear if it's a git error code. */
+#define GIT_ERROR_BIT_ERRNO (1ULL << 62)
+/*
+ * Set if the memory in meta should be freed; otherwise, it's statically
+ * allocated.
+ */
+#define GIT_ERROR_BIT_ALLOC (1ULL << 61)
+/*
+ * Set if the memory in meta is a C string; otherwise, it's a metadata struct.
+ */
+#define GIT_ERROR_BIT_MSG (1ULL << 60)
+
+#define GIT_ERROR_BIT_MASK (0xf << 60)
+
+#define GIT_ERROR_OK (git_error_ok())
+
+#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))
+
+/*
+ * A value representing an error in Git.
+ */
+struct git_error {
+	uint64_t code;
+	void *meta;
+};
+
+struct git_error_multiple {
+	struct git_error *errs;
+	size_t count;
+};
+
+enum git_error_code {
+	/* The operation was a success. */
+	GIT_ERR_SUCCESS = 0,
+	/* An object ID was provided, but it was not found.
+	 *
+	 * meta will be NULL or a pointer to struct object ID.
+	 */
+	GIT_ERR_OBJECT_NOT_FOUND = 1,
+	/*
+	 * An object ID was provided, but it is all zeros and that is not
+	 * allowed.
+	 *
+	 * meta will be NULL or a message explaining the context.
+	 */
+	GIT_ERR_NULL_OID = 2,
+	/*
+	 * Multiple errors occurred.
+	 *
+	 * meta must be a pointer to struct git_error_multiple.
+	 */
+	GIT_ERR_MULTIPLE = 3,
+};
+
+const char *git_error_string(struct git_error err);
+const char *git_error_strbuf(struct strbuf *buf, struct git_error err);
+
+/*
+ * A successful error status.
+ */
+static inline struct git_error git_error_ok(void) {
+	struct git_error e =  {
+		.code = 0 | GIT_ERROR_BIT_INIT,
+		.meta = NULL,
+	};
+	return e;
+}
+
+static inline struct git_error git_error_new_errno(uint32_t errnoval, const char *msg, int to_free) {
+	struct git_error e =  {
+		.code = errnoval | GIT_ERROR_BIT_INIT | GIT_ERROR_BIT_ERRNO |
+			GIT_ERROR_BIT_MSG | (to_free ? GIT_ERROR_BIT_ALLOC : 0),
+		.meta = (void *)msg,
+	};
+	return e;
+}
+
+static inline struct git_error git_error_new_git(uint32_t gitcode, const char *msg, int to_free) {
+	struct git_error e =  {
+		.code = gitcode | GIT_ERROR_BIT_INIT |
+			GIT_ERROR_BIT_MSG | (to_free ? GIT_ERROR_BIT_ALLOC : 0),
+		.meta = (void *)msg,
+	};
+	return e;
+}
+
+static inline struct git_error git_error_new_simple(uint32_t gitcode) {
+	struct git_error e =  {
+		.code = gitcode | GIT_ERROR_BIT_INIT,
+		.meta = NULL,
+	};
+	return e;
+}
+
+static inline struct git_error git_error_new_multiple(struct git_error *errs, size_t count)
+{
+	struct git_error_multiple *me = xmalloc(sizeof(*me));
+	struct git_error e =  {
+		.code = GIT_ERR_MULTIPLE | GIT_ERROR_BIT_INIT | GIT_ERROR_BIT_ALLOC,
+		.meta = me,
+	};
+	me->errs = errs;
+	me->count = count;
+	return e;
+}
+
+/*
+ * If this is a git error and the code matches the given code, or if this is a
+ * multiple error and any of the contained errors are a git error whose code
+ * matches, returns a pointer to that error.  If there is no match, returns
+ * NULL.
+ */
+static inline struct git_error *git_error_is_git(struct git_error *e, int code) {
+	int64_t giterr = GIT_ERROR_GITERR(*e);
+	if (giterr == code) {
+		return e;
+	} else if (giterr == GIT_ERR_MULTIPLE) {
+		struct git_error_multiple *me = e->meta;
+		for (size_t i = 0; i < me->count; i++)
+			return git_error_is_git(me->errs + i, code);
+		return NULL;
+	} else {
+		return NULL;
+	}
+}
+
+/*
+ * If this is an errno error and the code matches the given code, or if this is
+ * a multiple error and any of the contained errors are an errno error whose
+ * code matches, returns a pointer to that error.  Otherwise, returns NULL.
+ */
+static inline struct git_error *git_error_is_errno(struct git_error *e, int code) {
+	int64_t giterr = GIT_ERROR_GITERR(*e);
+	if (GIT_ERROR_ERRNO(*e) == code) {
+		return e;
+	} else if (giterr == GIT_ERR_MULTIPLE) {
+		struct git_error_multiple *me = e->meta;
+		for (size_t i = 0; i < me->count; i++)
+			return git_error_is_errno(me->errs + i, code);
+		return NULL;
+	} else {
+		return NULL;
+	}
+}
+
+/* Frees and deinitializes this error structure. */
+static inline uint64_t git_error_free(struct git_error *e)
+{
+	uint64_t code = e->code;
+	if (e->code & GIT_ERROR_BIT_ALLOC)
+		free(e->meta);
+	e->code = 0;
+	e->meta = NULL;
+	return code;
+}
+
+#endif

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> ....  It is designed to be passed and returned by value, not
> pointer, and it is possible to do so in two registers on 64-bit systems.
> Similar functionality works well for error types in Rust and for the
> standard library's lldiv_t, so this should not pose a problem.

It should not, in the sense that "any reasonable platform should be
able to pass two 64-bit word in a structure by value", but isn't it
optimizing for a wrong (i.e. have error) case?  In the case where
there is no error, a "negative return is error, zero is success",
with a pointer to "more detailed error info, in case the call
resulted in an error", would let us take branch based on a zero-ness
check on an integer in a machine-natural word, without even looking
at these two words in the struct.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

On 2024-09-30 at 22:44:37, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > ....  It is designed to be passed and returned by value, not
> > pointer, and it is possible to do so in two registers on 64-bit systems.
> > Similar functionality works well for error types in Rust and for the
> > standard library's lldiv_t, so this should not pose a problem.
> 
> It should not, in the sense that "any reasonable platform should be
> able to pass two 64-bit word in a structure by value", but isn't it
> optimizing for a wrong (i.e. have error) case?  In the case where
> there is no error, a "negative return is error, zero is success",
> with a pointer to "more detailed error info, in case the call
> resulted in an error", would let us take branch based on a zero-ness
> check on an integer in a machine-natural word, without even looking
> at these two words in the struct.

We can adjust the first word so that it's always zero on success, in
which case, because it's returned in two registers, the processor will
be able to branch on a zero-ness check on one of those registers.  (We
can even optimize the check by looking at the low 32 bits, which will do
the same for 32-bit machines as well.)  The performance benefit will be
the same, and I should note that Rust does this kind of thing without a
problem.

We did discuss passing an error accumulating argument to all functions
at the contributor summit, but it didn't seem that was the way folks
wanted to go: people seemed to prefer a return value.

I'm totally open to other proposals here and not particularly tied to
this one, but I told Emily that I'd send a proposal to the list, and so
here it is.  If we like this with changes or prefer something different,
that's fine with me.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Oswald Buddenhagen]

On Mon, Sep 30, 2024 at 10:03:52PM +0000, brian m. carlson wrote:
>+		case GIT_ERR_MULTIPLE:
>+			strbuf_addf(buf, _("multiple errors:\n"));
>+			for (size_t i = 0; i < me->count; i++) {
>+				git_error_strbuf(buf, me->errs[i]);
>+				strbuf_addstr(buf, "\n");
>+			}
>+		}

ah. i was wondering how you'd address this ("this" being non-fatal
errors piling up), as i'm facing the same problem in a project of mine.

the problem is that one can either impose a very rigid formatting (as
you do here), or provide formatting options, which can get arbitrarily
complex. so i wonder whether one shouldn't pursue a hybrid approach
instead: singular return values for low-level functions that either work
or fail fatally, and error callbacks for higher-level functionality?

btw, the switch misses breaks.

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

[Emily Shaffer]

On Mon, Sep 30, 2024 at 3:04 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> There is work underway to move some of the Git code out into a reusable
> library.  In such a case, it's not very desirable to have the library
> code write random errors to standard error, since this is an antipattern
> which annoys terminal users.
>
> Instead, we will expect callers of our library function to return
> errors.  The reusability of our library will be substantially improved
> if we return typed errors so that users can easily determine what kind
> of error might have occurred and react differently based on different
> contexts.  For example, if we are looking up an object in a partial
> clone and it's not found, we might decide to download it, but we might
> return an error to the user if the problem is instead that the revision
> specified is not syntactically valid.
>
> To help the libification process and make our code more generally
> maintainable, add an error type.  This consists of on 64-bit integer,
> which contains bit flags and a 32-bit code, and a pointer, which depends
> on the code.  It is designed to be passed and returned by value, not
> pointer, and it is possible to do so in two registers on 64-bit systems.
> Similar functionality works well for error types in Rust and for the
> standard library's lldiv_t, so this should not pose a problem.
>
> Provide the ability to specify either an errno value or a git error code
> as the code.  This allows us to use this type generically when handling
> errno values such as processing files, as well as express a rich set of
> possible error codes specific to Git.  We pick an unsigned 32-bit code
> because Windows can use the full set of 32 bits in its error values,
> even though most Unix systems use only a small set of codes which
> traditionally start at 1.  32 bits for Git errors also allows us plenty
> of space to expand as we see fit.
>
> Allow multiple errors to be provided and wrapped in a single object,
> which is useful in many situations, and add helpers to determine if any
> error in the set matches a particular code.
>
> Additionally, provide error formatting functions that produce a suitable
> localized string for ease of use.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>

Left a few comments below about portability and readability, but
general thoughts: of course this is quite limited, but I think it will
actually be enough in most cases. It also seems like if we need more
later, we can use this same code-OR-with-pointer thing to point to
something more nuanced than `void* meta`. I'd be in favor of applying
this model to somewhere with clear error codes and relatively few
invocations (maybe to the internals of something like argparse or
run-command, without initially modifying the public interface, for
example?) and seeing where we run into friction. I have a little bit
of concern about the idea of centralizing all the `meta` parsing and
error string generation, but it seems like we might be able to adopt a
model more similar to the one we use for `git_config()` callbacks and
do more context-aware parsing instead to keep `git_error_strbuf` from
getting too huge.

Thanks for sending this. I'm looking forward to hearing others' opinions.
 - Emily

> ---
>  Makefile |   1 +
>  error.c  |  43 ++++++++++++++
>  error.h  | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 212 insertions(+)
>  create mode 100644 error.c
>  create mode 100644 error.h
>
> diff --git a/Makefile b/Makefile
> index 7344a7f725..5d9bf992e9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1013,6 +1013,7 @@ LIB_OBJS += dir.o
>  LIB_OBJS += editor.o
>  LIB_OBJS += entry.o
>  LIB_OBJS += environment.o
> +LIB_OBJS += error.o
>  LIB_OBJS += ewah/bitmap.o
>  LIB_OBJS += ewah/ewah_bitmap.o
>  LIB_OBJS += ewah/ewah_io.o
> diff --git a/error.c b/error.c
> new file mode 100644
> index 0000000000..713bc42187
> --- /dev/null
> +++ b/error.c
> @@ -0,0 +1,43 @@
> +#include "git-compat-util.h"
> +#include "gettext.h"
> +#include "error.h"
> +#include "hex.h"
> +#include "strbuf.h"
> +
> +const char *git_error_string(struct git_error err)
> +{
> +       struct strbuf buf = STRBUF_INIT;
> +       if (!git_error_strbuf(&buf, err))
> +               return NULL;
> +       return strbuf_detach(&buf, NULL);
> +}
> +
> +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?

> +{
> +       if (GIT_ERROR_SUCCESS(err)) {
> +               return NULL;
> +       } else if (GIT_ERROR_ERRNO(err) != -1) {
> +               return xstrdup(strerror(GIT_ERROR_ERRNO(err)));
> +       } else {
> +               struct git_error_multiple *me = err.meta;
> +               switch (GIT_ERROR_GITERR(err)) {
> +               case GIT_ERR_OBJECT_NOT_FOUND:
> +                       if (err.meta)
> +                               strbuf_addf(buf, _("object not found: %s"), oid_to_hex(err.meta));
> +                       else
> +                               strbuf_addf(buf, _("object not found"));
> +               case GIT_ERR_NULL_OID:
> +                       if (err.meta)
> +                               strbuf_addf(buf, _("null object ID not allowed in this context: %s"), (char *)err.meta);
> +                       else
> +                               strbuf_addf(buf, _("null object ID not allowed"));
> +               case GIT_ERR_MULTIPLE:
> +                       strbuf_addf(buf, _("multiple errors:\n"));
> +                       for (size_t i = 0; i < me->count; i++) {
> +                               git_error_strbuf(buf, me->errs[i]);
> +                               strbuf_addstr(buf, "\n");
> +                       }
> +               }
> +               return buf->buf;
> +       }
> +}
> diff --git a/error.h b/error.h
> new file mode 100644
> index 0000000000..485cca99e0
> --- /dev/null
> +++ b/error.h
> @@ -0,0 +1,168 @@
> +#ifndef ERROR_H
> +#define ERROR_H
> +
> +#include "git-compat-util.h"
> +
> +/* Set if this value is initialized. */
> +#define GIT_ERROR_BIT_INIT (1ULL << 63)
> +/* Set if the code is an errno code, clear if it's a git error code. */
> +#define GIT_ERROR_BIT_ERRNO (1ULL << 62)
> +/*
> + * Set if the memory in meta should be freed; otherwise, it's statically
> + * allocated.
> + */
> +#define GIT_ERROR_BIT_ALLOC (1ULL << 61)
> +/*
> + * Set if the memory in meta is a C string; otherwise, it's a metadata struct.
> + */
> +#define GIT_ERROR_BIT_MSG (1ULL << 60)
> +
> +#define GIT_ERROR_BIT_MASK (0xf << 60)
> +
> +#define GIT_ERROR_OK (git_error_ok())
> +
> +#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"?)

> +
> +/*
> + * A value representing an error in Git.
> + */
> +struct git_error {
> +       uint64_t code;
> +       void *meta;
> +};
> +
> +struct git_error_multiple {
> +       struct git_error *errs;
> +       size_t count;
> +};
> +
> +enum git_error_code {
> +       /* The operation was a success. */
> +       GIT_ERR_SUCCESS = 0,
> +       /* An object ID was provided, but it was not found.
> +        *
> +        * meta will be NULL or a pointer to struct object ID.
> +        */
> +       GIT_ERR_OBJECT_NOT_FOUND = 1,
> +       /*
> +        * An object ID was provided, but it is all zeros and that is not
> +        * allowed.
> +        *
> +        * meta will be NULL or a message explaining the context.
> +        */
> +       GIT_ERR_NULL_OID = 2,
> +       /*
> +        * Multiple errors occurred.
> +        *
> +        * meta must be a pointer to struct git_error_multiple.
> +        */
> +       GIT_ERR_MULTIPLE = 3,
> +};
> +
> +const char *git_error_string(struct git_error err);
> +const char *git_error_strbuf(struct strbuf *buf, struct git_error err);
> +
> +/*
> + * A successful error status.
> + */
> +static inline struct git_error git_error_ok(void) {
> +       struct git_error e =  {
> +               .code = 0 | GIT_ERROR_BIT_INIT,
> +               .meta = NULL,
> +       };
> +       return e;
> +}
> +
> +static inline struct git_error git_error_new_errno(uint32_t errnoval, const char *msg, int to_free) {
> +       struct git_error e =  {
> +               .code = errnoval | GIT_ERROR_BIT_INIT | GIT_ERROR_BIT_ERRNO |
> +                       GIT_ERROR_BIT_MSG | (to_free ? GIT_ERROR_BIT_ALLOC : 0),
> +               .meta = (void *)msg,
> +       };
> +       return e;
> +}
> +
> +static inline struct git_error git_error_new_git(uint32_t gitcode, const char *msg, int to_free) {
> +       struct git_error e =  {
> +               .code = gitcode | GIT_ERROR_BIT_INIT |
> +                       GIT_ERROR_BIT_MSG | (to_free ? GIT_ERROR_BIT_ALLOC : 0),
> +               .meta = (void *)msg,
> +       };
> +       return e;
> +}
> +
> +static inline struct git_error git_error_new_simple(uint32_t gitcode) {
> +       struct git_error e =  {
> +               .code = gitcode | GIT_ERROR_BIT_INIT,
> +               .meta = NULL,
> +       };
> +       return e;
> +}
> +
> +static inline struct git_error git_error_new_multiple(struct git_error *errs, size_t count)
> +{
> +       struct git_error_multiple *me = xmalloc(sizeof(*me));
> +       struct git_error e =  {
> +               .code = GIT_ERR_MULTIPLE | GIT_ERROR_BIT_INIT | GIT_ERROR_BIT_ALLOC,
> +               .meta = me,
> +       };
> +       me->errs = errs;
> +       me->count = count;
> +       return e;
> +}
> +
> +/*
> + * If this is a git error and the code matches the given code, or if this is a
> + * multiple error and any of the contained errors are a git error whose code
> + * matches, returns a pointer to that error.  If there is no match, returns
> + * NULL.
> + */
> +static inline struct git_error *git_error_is_git(struct git_error *e, int code) {
> +       int64_t giterr = GIT_ERROR_GITERR(*e);
> +       if (giterr == code) {
> +               return e;
> +       } else if (giterr == GIT_ERR_MULTIPLE) {
> +               struct git_error_multiple *me = e->meta;
> +               for (size_t i = 0; i < me->count; i++)
> +                       return git_error_is_git(me->errs + i, code);
> +               return NULL;
> +       } else {
> +               return NULL;
> +       }
> +}
> +
> +/*
> + * If this is an errno error and the code matches the given code, or if this is
> + * a multiple error and any of the contained errors are an errno error whose
> + * code matches, returns a pointer to that error.  Otherwise, returns NULL.
> + */
> +static inline struct git_error *git_error_is_errno(struct git_error *e, int code) {
> +       int64_t giterr = GIT_ERROR_GITERR(*e);
> +       if (GIT_ERROR_ERRNO(*e) == code) {
> +               return e;
> +       } else if (giterr == GIT_ERR_MULTIPLE) {
> +               struct git_error_multiple *me = e->meta;
> +               for (size_t i = 0; i < me->count; i++)
> +                       return git_error_is_errno(me->errs + i, code);
> +               return NULL;
> +       } else {
> +               return NULL;
> +       }
> +}
> +
> +/* Frees and deinitializes this error structure. */
> +static inline uint64_t git_error_free(struct git_error *e)
> +{
> +       uint64_t code = e->code;
> +       if (e->code & GIT_ERROR_BIT_ALLOC)
> +               free(e->meta);
> +       e->code = 0;
> +       e->meta = NULL;
> +       return code;
> +}
> +
> +#endif

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

[Phillip Wood]

Hi brian

Thanks for working on this.

On 30/09/2024 23:03, brian m. carlson wrote:
> There is work underway to move some of the Git code out into a reusable
> library.  In such a case, it's not very desirable to have the library
> code write random errors to standard error, since this is an antipattern
> which annoys terminal users.
> 
> Instead, we will expect callers of our library function to return
> errors. 

Isn't it the callers that will expect the function to return an error?

> The reusability of our library will be substantially improved
> if we return typed errors so that users can easily determine what kind
> of error might have occurred and react differently based on different
> contexts.  For example, if we are looking up an object in a partial
> clone and it's not found, we might decide to download it, but we might
> return an error to the user if the problem is instead that the revision
> specified is not syntactically valid.
> 
> To help the libification process and make our code more generally
> maintainable, add an error type.  This consists of on 64-bit integer,
> which contains bit flags and a 32-bit code, and a pointer, which depends
> on the code.  It is designed to be passed and returned by value, not
> pointer, and it is possible to do so in two registers on 64-bit systems.
> Similar functionality works well for error types in Rust and for the
> standard library's lldiv_t, so this should not pose a problem.

Part of the reason it works well in rust is that it supports 
discriminated unions with pattern matching and has the "?" macro for 
early returns. In C the code ends up being quite verbose compared to 
taking a pointer to error struct as a function parameter and returning a 
boolean success/fail flag.

     struct git_error e;
     struct object_id oid;

     e = repo_get_oid(r, "HEAD", &oid);
     if (!GIT_ERROR_SUCCESS(e))
         return e;

With a boolean return we can have

     struct object_id oid;

     if (repo_get_oid(r, "HEAD", &oid, e))
         return -1;

where "e" is a "struct git_error*" passed into the function.

> Provide the ability to specify either an errno value or a git error code
> as the code.  This allows us to use this type generically when handling
> errno values such as processing files, as well as express a rich set of
> possible error codes specific to Git.  We pick an unsigned 32-bit code
> because Windows can use the full set of 32 bits in its error values,
> even though most Unix systems use only a small set of codes which
> traditionally start at 1.  32 bits for Git errors also allows us plenty
> of space to expand as we see fit.

I think the design of the struct is fine. It does mean we need a global 
list of error values. GError [1] avoids this by having a "domain" field 
that is an interned string so that error codes only need to be unique 
within a given domain. I think that for a self-contained project like 
git a global list is probably fine - svn does this for example [2].

[1] https://docs.gtk.org/glib/error-reporting.html
[2] 
https://github.com/apache/subversion/blob/be229fd54f5860b3140831671efbfd3f7f6fbb0b/subversion/include/svn_error_codes.h

> Allow multiple errors to be provided and wrapped in a single object,
> which is useful in many situations, and add helpers to determine if any
> error in the set matches a particular code.

The api appears to require the caller know up front how many errors 
there will be which seems unlikely to be true in practice. I think a 
more realistic design would allow callers to push errors as they occur 
and grow the array accordingly. For example ref_transaction_prepare() 
would want to return a list of errors, one for each ref that it was 
unable to lock or which did not match the expected value but it would 
not know up-front how many errors there were going to be.

It would be useful to be able to add context to an error as the stack is 
unwound. For example if unpack_trees() detects that it would overwrite 
untracked files it prints a error listing those files. The exact 
formatting of that message depends on the command being run. That is 
currently handled by calling setup_unpack_trees_porcelain() with the 
name of the command before calling unpack_trees(). In a world where 
unpack_trees() returns a error containing the list of files we would 
want to customize the message by adding some context before converting 
it to a string.

> Additionally, provide error formatting functions that produce a suitable
> localized string for ease of use.

I share Emily's concern that this function has to know the details of 
how to format every error. We could mitigate that somewhat using a 
switch that calls external helper functions that do the actual formatting

     switch (e.code) {
     case GIT_ERR_OBJECT_NOT_FOUND:
         format_object_not_found(buf, e); /* lives in another file */
         break;
     ...

I know this is an RFC but I couldn't resist one code comment

> +#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)

git_error_free() returns the code as in integer so we don't need ".code" 
here. Also our coding guidelines would suggest git_error_clear() for the 
name of that function.


In conclusion I like the general idea but have concerns about the 
verbosity of returning an error struct. It would be helpful to see some 
examples of this being used to see how it works in practice.

Best Wishes

Phillip

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

[Phillip Wood]

Hi Oswald

On 01/10/2024 16:29, Oswald Buddenhagen wrote:
> On Mon, Sep 30, 2024 at 10:03:52PM +0000, brian m. carlson wrote:
>> +        case GIT_ERR_MULTIPLE:
>> +            strbuf_addf(buf, _("multiple errors:\n"));
>> +            for (size_t i = 0; i < me->count; i++) {
>> +                git_error_strbuf(buf, me->errs[i]);
>> +                strbuf_addstr(buf, "\n");
>> +            }
>> +        }
> 
> ah. i was wondering how you'd address this ("this" being non-fatal
> errors piling up), as i'm facing the same problem in a project of mine.
> 
> the problem is that one can either impose a very rigid formatting (as
> you do here), or provide formatting options, which can get arbitrarily
> complex. so i wonder whether one shouldn't pursue a hybrid approach
> instead: singular return values for low-level functions that either work
> or fail fatally, and error callbacks for higher-level functionality?

That's a good point - the rigid formatting is easy to implement but is 
not ideal from the user's point of view. Ideally we'd compose a coherent 
message with the data from the various errors but as you say that's not 
trivial. I wonder if there are any good examples out there that we could 
use as inspiration. The ones I'm aware of (rust's anyhow and svn's 
svn_error_compose) go for a rigid approach. Glib's GError punts the 
problem entirely by forbidding multiple errors.

Best Wishes

Phillip

> btw, the switch misses breaks.
> 
> 

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2512 bytes --]

On 2024-10-01 at 20:31:15, Emily Shaffer wrote:
> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 5783 bytes --]

On 2024-10-02 at 09:54:52, Phillip Wood wrote:
> On 30/09/2024 23:03, brian m. carlson wrote:
> > There is work underway to move some of the Git code out into a reusable
> > library.  In such a case, it's not very desirable to have the library
> > code write random errors to standard error, since this is an antipattern
> > which annoys terminal users.
> > 
> > Instead, we will expect callers of our library function to return
> > errors.
> 
> Isn't it the callers that will expect the function to return an error?

Yes, that's correct.  I'll fix that.

> > To help the libification process and make our code more generally
> > maintainable, add an error type.  This consists of on 64-bit integer,
> > which contains bit flags and a 32-bit code, and a pointer, which depends
> > on the code.  It is designed to be passed and returned by value, not
> > pointer, and it is possible to do so in two registers on 64-bit systems.
> > Similar functionality works well for error types in Rust and for the
> > standard library's lldiv_t, so this should not pose a problem.
> 
> Part of the reason it works well in rust is that it supports discriminated
> unions with pattern matching and has the "?" macro for early returns. In C
> the code ends up being quite verbose compared to taking a pointer to error
> struct as a function parameter and returning a boolean success/fail flag.
> 
>     struct git_error e;
>     struct object_id oid;
> 
>     e = repo_get_oid(r, "HEAD", &oid);
>     if (!GIT_ERROR_SUCCESS(e))
>         return e;
> 
> With a boolean return we can have
> 
>     struct object_id oid;
> 
>     if (repo_get_oid(r, "HEAD", &oid, e))
>         return -1;
> 
> where "e" is a "struct git_error*" passed into the function.

Yes, I agree that this is more verbose than in Rust.  If we were using
Rust (which is a thing I'm working on), then we'd have nicer error
handling, but for now we don't.

However, Go still uses this kind of error handling, and many people use
it every day with this limitation, so I don't think it's too awful for
what we're getting.  I won't say that Go is my favourite language and I
do prefer the less verbose error handling in Rust, but the fact that
this design is widely used means that it's at least a defensible
decision.

> I think the design of the struct is fine. It does mean we need a global list
> of error values. GError [1] avoids this by having a "domain" field that is
> an interned string so that error codes only need to be unique within a given
> domain. I think that for a self-contained project like git a global list is
> probably fine - svn does this for example [2].

I think so, too.  libgit2 does the same thing, which seems to have
worked out okay.

> > Allow multiple errors to be provided and wrapped in a single object,
> > which is useful in many situations, and add helpers to determine if any
> > error in the set matches a particular code.
> 
> The api appears to require the caller know up front how many errors there
> will be which seems unlikely to be true in practice. I think a more
> realistic design would allow callers to push errors as they occur and grow
> the array accordingly. For example ref_transaction_prepare() would want to
> return a list of errors, one for each ref that it was unable to lock or
> which did not match the expected value but it would not know up-front how
> many errors there were going to be.

That seems reasonable.  We could add helpers to extend the number of
errors.

> It would be useful to be able to add context to an error as the stack is
> unwound. For example if unpack_trees() detects that it would overwrite
> untracked files it prints a error listing those files. The exact formatting
> of that message depends on the command being run. That is currently handled
> by calling setup_unpack_trees_porcelain() with the name of the command
> before calling unpack_trees(). In a world where unpack_trees() returns a
> error containing the list of files we would want to customize the message by
> adding some context before converting it to a string.

I think we could do that with either a particular error type (e.g.,
`GIT_ERR_WOULD_OVERWRITE`) that contains nested errors and text or a
generic `GIT_ERR_ANNOTATED`.

> > Additionally, provide error formatting functions that produce a suitable
> > localized string for ease of use.
> 
> I share Emily's concern that this function has to know the details of how to
> format every error. We could mitigate that somewhat using a switch that
> calls external helper functions that do the actual formatting
> 
>     switch (e.code) {
>     case GIT_ERR_OBJECT_NOT_FOUND:
>         format_object_not_found(buf, e); /* lives in another file */
>         break;
>     ...
> 
> I know this is an RFC but I couldn't resist one code comment

I'm fine with that approach.  This is mostly designed to solicit
comments about the overall design, and I don't have a strong opinion
about how we format.

> > +#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)
> 
> git_error_free() returns the code as in integer so we don't need ".code"
> here. Also our coding guidelines would suggest git_error_clear() for the
> name of that function.

Got it.

> In conclusion I like the general idea but have concerns about the verbosity
> of returning an error struct. It would be helpful to see some examples of
> this being used to see how it works in practice.

If I send a v2, I'll try to wire up some code so folks can see some
examples.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Eric Sunshine]

On Wed, Oct 2, 2024 at 6:04 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On 2024-10-02 at 09:54:52, Phillip Wood wrote:
> > Part of the reason it works well in rust is that it supports discriminated
> > unions with pattern matching and has the "?" macro for early returns. In C
> > the code ends up being quite verbose compared to taking a pointer to error
> > struct as a function parameter and returning a boolean success/fail flag.
> >
> >     struct git_error e;
> >     struct object_id oid;
> >
> >     e = repo_get_oid(r, "HEAD", &oid);
> >     if (!GIT_ERROR_SUCCESS(e))
> >         return e;
> >
> > With a boolean return we can have
> >
> >     struct object_id oid;
> >
> >     if (repo_get_oid(r, "HEAD", &oid, e))
> >         return -1;
> >
> > where "e" is a "struct git_error*" passed into the function.
>
> However, Go still uses this kind of error handling, and many people use
> it every day with this limitation, so I don't think it's too awful for
> what we're getting.  I won't say that Go is my favourite language and I
> do prefer the less verbose error handling in Rust, but the fact that
> this design is widely used means that it's at least a defensible
> decision.

I'm not sure I understand your response to Phillip's observation.
Idiomatic error handling in Go:

    if oid, err := repo_get_oid(r, "HEAD"); err != nil {
        return err;
    }

seems much closer to Phillip's more succinct example than to the more
verbose example using GIT_ERROR_SUCCESS().

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

On 2024-10-02 at 22:16:00, Eric Sunshine wrote:
> I'm not sure I understand your response to Phillip's observation.
> Idiomatic error handling in Go:
> 
>     if oid, err := repo_get_oid(r, "HEAD"); err != nil {
>         return err;
>     }
> 
> seems much closer to Phillip's more succinct example than to the more
> verbose example using GIT_ERROR_SUCCESS().

I don't think that approach works if you want to use `oid`, since it
scopes just to the `if` statement (and any relevant `else`).  If `oid`
were already defined, then you would need to omit the colon in `:=`, or
the compiler would complain about "no new variables".

That's why I usually see this:

  oid, err := repo_get_oid(r, "HEAD")
  if err != nil {
      return err
  }

which is much more like what Phillip's more verbose example shows.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Eric Sunshine]

On Wed, Oct 2, 2024 at 6:24 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On 2024-10-02 at 22:16:00, Eric Sunshine wrote:
> > I'm not sure I understand your response to Phillip's observation.
> > Idiomatic error handling in Go:
> >
> >     if oid, err := repo_get_oid(r, "HEAD"); err != nil {
> >         return err;
> >     }
> >
> > seems much closer to Phillip's more succinct example than to the more
> > verbose example using GIT_ERROR_SUCCESS().
>
> I don't think that approach works if you want to use `oid`, since it
> scopes just to the `if` statement (and any relevant `else`).  If `oid`
> were already defined, then you would need to omit the colon in `:=`, or
> the compiler would complain about "no new variables".
>
> That's why I usually see this:
>
>   oid, err := repo_get_oid(r, "HEAD")
>   if err != nil {
>       return err
>   }
>
> which is much more like what Phillip's more verbose example shows.

Yes, you often see that in Go, as well as:

    var oid oid_type
    var err error
    if oid, err = repo_get_oid(r, "HEAD"); err != nil {
        return err;
    }

which is very much in line with Phillip's succinct C example. But
isn't this all beside the point?

Your proposal uses Rust as a model to justify the API choice in this
RFC, but Phillip's point was that -- despite being perfectly suitable
in Rust -- it is _not_ ergonomic in C. Rather than explaining how the
proposed non-ergonomic API is superior to the ergonomic API in
Phillip's example, you instead responded by saying that people in some
other programming language (Go, in this case) have to deal with
non-ergonomic error handling on a daily basis, therefore, a
non-ergonomic API is good enough for Git's C programmers. But that is
not a very convincing argument for choosing a non-ergonomic API for
*this* project which is written in C, especially considering that a
more ergonomic API has been presented (and is already in use in parts
of the codebase).

That's why I said in my original response that I didn't understand
your response to Phillip. You seem to be using a non-justification
("other programmers suffer, so Git programmers can suffer too") as a
justification for a non-ergonomic design.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Your proposal uses Rust as a model to justify the API choice in this
> RFC, but Phillip's point was that -- despite being perfectly suitable
> in Rust -- it is _not_ ergonomic in C.
> ...
> That's why I said in my original response that I didn't understand
> your response to Phillip. You seem to be using a non-justification
> ("other programmers suffer, so Git programmers can suffer too") as a
> justification for a non-ergonomic design.

The statement may be a bit too harsh, as some may not even realize
that they are suffering anymore, after prolonged exposure to these
idioms, just like C folks consider it is a fact of life that they
have to carefully manage their pointers and the memory they point
at.

I do agree that "return value with more details in the out parameter
whose address is supplied by the caller" is a convention that is
easier to grok when written in C.

Thanks.

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

[Emily Shaffer]

On Wed, Oct 2, 2024 at 2:54 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi brian
>
> Thanks for working on this.
>
> On 30/09/2024 23:03, brian m. carlson wrote:
> > There is work underway to move some of the Git code out into a reusable
> > library.  In such a case, it's not very desirable to have the library
> > code write random errors to standard error, since this is an antipattern
> > which annoys terminal users.
> >
> > Instead, we will expect callers of our library function to return
> > errors.
>
> Isn't it the callers that will expect the function to return an error?
>
> > The reusability of our library will be substantially improved
> > if we return typed errors so that users can easily determine what kind
> > of error might have occurred and react differently based on different
> > contexts.  For example, if we are looking up an object in a partial
> > clone and it's not found, we might decide to download it, but we might
> > return an error to the user if the problem is instead that the revision
> > specified is not syntactically valid.
> >
> > To help the libification process and make our code more generally
> > maintainable, add an error type.  This consists of on 64-bit integer,
> > which contains bit flags and a 32-bit code, and a pointer, which depends
> > on the code.  It is designed to be passed and returned by value, not
> > pointer, and it is possible to do so in two registers on 64-bit systems.
> > Similar functionality works well for error types in Rust and for the
> > standard library's lldiv_t, so this should not pose a problem.
>
> Part of the reason it works well in rust is that it supports
> discriminated unions with pattern matching and has the "?" macro for
> early returns. In C the code ends up being quite verbose compared to
> taking a pointer to error struct as a function parameter and returning a
> boolean success/fail flag.
>
>      struct git_error e;
>      struct object_id oid;
>
>      e = repo_get_oid(r, "HEAD", &oid);
>      if (!GIT_ERROR_SUCCESS(e))
>          return e;
>
> With a boolean return we can have
>
>      struct object_id oid;
>
>      if (repo_get_oid(r, "HEAD", &oid, e))
>          return -1;
>
> where "e" is a "struct git_error*" passed into the function.

I actually don't find this complaint all that compelling; it's not
hard to write a shorter macro that can be used inline, so we can do
things like:

    ERR_VAR(e);
    if(ERR(e, repo_get_oid(...))
      return e;

or even a macro to do the return if desired:

    ERR_VAR(e); // or, i guess we can be not SO lazy and just write
struct git_error e here, whatever :) )
    RETURN_IF_ERR(e, repo_get_oid(...));

For better or worse, you can do a lot of things in a macro, so I don't
see verboseness as much of an issue because I think we can hide a lot
of it this way.

>
> > Provide the ability to specify either an errno value or a git error code
> > as the code.  This allows us to use this type generically when handling
> > errno values such as processing files, as well as express a rich set of
> > possible error codes specific to Git.  We pick an unsigned 32-bit code
> > because Windows can use the full set of 32 bits in its error values,
> > even though most Unix systems use only a small set of codes which
> > traditionally start at 1.  32 bits for Git errors also allows us plenty
> > of space to expand as we see fit.
>
> I think the design of the struct is fine. It does mean we need a global
> list of error values. GError [1] avoids this by having a "domain" field
> that is an interned string so that error codes only need to be unique
> within a given domain. I think that for a self-contained project like
> git a global list is probably fine - svn does this for example [2].
>
> [1] https://docs.gtk.org/glib/error-reporting.html
> [2]
> https://github.com/apache/subversion/blob/be229fd54f5860b3140831671efbfd3f7f6fbb0b/subversion/include/svn_error_codes.h
>
> > Allow multiple errors to be provided and wrapped in a single object,
> > which is useful in many situations, and add helpers to determine if any
> > error in the set matches a particular code.
>
> The api appears to require the caller know up front how many errors
> there will be which seems unlikely to be true in practice. I think a
> more realistic design would allow callers to push errors as they occur
> and grow the array accordingly. For example ref_transaction_prepare()
> would want to return a list of errors, one for each ref that it was
> unable to lock or which did not match the expected value but it would
> not know up-front how many errors there were going to be.
>
> It would be useful to be able to add context to an error as the stack is
> unwound. For example if unpack_trees() detects that it would overwrite
> untracked files it prints a error listing those files. The exact
> formatting of that message depends on the command being run. That is
> currently handled by calling setup_unpack_trees_porcelain() with the
> name of the command before calling unpack_trees(). In a world where
> unpack_trees() returns a error containing the list of files we would
> want to customize the message by adding some context before converting
> it to a string.
>
> > Additionally, provide error formatting functions that produce a suitable
> > localized string for ease of use.
>
> I share Emily's concern that this function has to know the details of
> how to format every error. We could mitigate that somewhat using a
> switch that calls external helper functions that do the actual formatting
>
>      switch (e.code) {
>      case GIT_ERR_OBJECT_NOT_FOUND:
>          format_object_not_found(buf, e); /* lives in another file */
>          break;
>      ...
>
> I know this is an RFC but I couldn't resist one code comment
>
> > +#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)
>
> git_error_free() returns the code as in integer so we don't need ".code"
> here. Also our coding guidelines would suggest git_error_clear() for the
> name of that function.
>
>
> In conclusion I like the general idea but have concerns about the
> verbosity of returning an error struct. It would be helpful to see some
> examples of this being used to see how it works in practice.
>
> Best Wishes
>
> Phillip
>

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

[Eric Sunshine]

On Thu, Oct 3, 2024 at 12:05 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > That's why I said in my original response that I didn't understand
> > your response to Phillip. You seem to be using a non-justification
> > ("other programmers suffer, so Git programmers can suffer too") as a
> > justification for a non-ergonomic design.
>
> The statement may be a bit too harsh [...]

Yes, sorry if that came across as harsh, brian. The "Git programmers
can suffer too" was my (clearly) poor tongue-in-cheek attempt to
soften the earlier part of the email in case it sounded harsh.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

On 2024-10-03 at 22:27:29, Eric Sunshine wrote:
> On Thu, Oct 3, 2024 at 12:05 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > That's why I said in my original response that I didn't understand
> > > your response to Phillip. You seem to be using a non-justification
> > > ("other programmers suffer, so Git programmers can suffer too") as a
> > > justification for a non-ergonomic design.
> >
> > The statement may be a bit too harsh [...]
> 
> Yes, sorry if that came across as harsh, brian. The "Git programmers
> can suffer too" was my (clearly) poor tongue-in-cheek attempt to
> soften the earlier part of the email in case it sounded harsh.

I'm definitely not offended.  We can certainly disagree on the merits of
the proposal; hopefully we'll find some solution that the project
generally approves of.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[phillip.wood123]

Hi Emily

On 03/10/2024 17:17, Emily Shaffer wrote:
> On Wed, Oct 2, 2024 at 2:54 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Part of the reason it works well in rust is that it supports
>> discriminated unions with pattern matching and has the "?" macro for
>> early returns. In C the code ends up being quite verbose compared to
>> taking a pointer to error struct as a function parameter and returning a
>> boolean success/fail flag.
>>
>>       struct git_error e;
>>       struct object_id oid;
>>
>>       e = repo_get_oid(r, "HEAD", &oid);
>>       if (!GIT_ERROR_SUCCESS(e))
>>           return e;
>>
>> With a boolean return we can have
>>
>>       struct object_id oid;
>>
>>       if (repo_get_oid(r, "HEAD", &oid, e))
>>           return -1;
>>
>> where "e" is a "struct git_error*" passed into the function.
> 
> I actually don't find this complaint all that compelling; it's not
> hard to write a shorter macro that can be used inline, so we can do
> things like:
> 
>      ERR_VAR(e);
>      if(ERR(e, repo_get_oid(...))
>        return e;

Right, but what's the advantage over passing the error struct as a 
function parameter? It feels like we're adding extra complexity and 
hiding the assignment to "e" to work around the inconvenience of 
returning a struct rather than a flag. Is there some other advantage to 
returning a struct that makes this worthwhile?

Best Wishes

Phillip

> or even a macro to do the return if desired:
> 
>      ERR_VAR(e); // or, i guess we can be not SO lazy and just write
> struct git_error e here, whatever :) )
>      RETURN_IF_ERR(e, repo_get_oid(...));
> 
> For better or worse, you can do a lot of things in a macro, so I don't
> see verboseness as much of an issue because I think we can hide a lot
> of it this way.
> 
>>
>>> Provide the ability to specify either an errno value or a git error code
>>> as the code.  This allows us to use this type generically when handling
>>> errno values such as processing files, as well as express a rich set of
>>> possible error codes specific to Git.  We pick an unsigned 32-bit code
>>> because Windows can use the full set of 32 bits in its error values,
>>> even though most Unix systems use only a small set of codes which
>>> traditionally start at 1.  32 bits for Git errors also allows us plenty
>>> of space to expand as we see fit.
>>
>> I think the design of the struct is fine. It does mean we need a global
>> list of error values. GError [1] avoids this by having a "domain" field
>> that is an interned string so that error codes only need to be unique
>> within a given domain. I think that for a self-contained project like
>> git a global list is probably fine - svn does this for example [2].
>>
>> [1] https://docs.gtk.org/glib/error-reporting.html
>> [2]
>> https://github.com/apache/subversion/blob/be229fd54f5860b3140831671efbfd3f7f6fbb0b/subversion/include/svn_error_codes.h
>>
>>> Allow multiple errors to be provided and wrapped in a single object,
>>> which is useful in many situations, and add helpers to determine if any
>>> error in the set matches a particular code.
>>
>> The api appears to require the caller know up front how many errors
>> there will be which seems unlikely to be true in practice. I think a
>> more realistic design would allow callers to push errors as they occur
>> and grow the array accordingly. For example ref_transaction_prepare()
>> would want to return a list of errors, one for each ref that it was
>> unable to lock or which did not match the expected value but it would
>> not know up-front how many errors there were going to be.
>>
>> It would be useful to be able to add context to an error as the stack is
>> unwound. For example if unpack_trees() detects that it would overwrite
>> untracked files it prints a error listing those files. The exact
>> formatting of that message depends on the command being run. That is
>> currently handled by calling setup_unpack_trees_porcelain() with the
>> name of the command before calling unpack_trees(). In a world where
>> unpack_trees() returns a error containing the list of files we would
>> want to customize the message by adding some context before converting
>> it to a string.
>>
>>> Additionally, provide error formatting functions that produce a suitable
>>> localized string for ease of use.
>>
>> I share Emily's concern that this function has to know the details of
>> how to format every error. We could mitigate that somewhat using a
>> switch that calls external helper functions that do the actual formatting
>>
>>       switch (e.code) {
>>       case GIT_ERR_OBJECT_NOT_FOUND:
>>           format_object_not_found(buf, e); /* lives in another file */
>>           break;
>>       ...
>>
>> I know this is an RFC but I couldn't resist one code comment
>>
>>> +#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)
>>
>> git_error_free() returns the code as in integer so we don't need ".code"
>> here. Also our coding guidelines would suggest git_error_clear() for the
>> name of that function.
>>
>>
>> In conclusion I like the general idea but have concerns about the
>> verbosity of returning an error struct. It would be helpful to see some
>> examples of this being used to see how it works in practice.
>>
>> Best Wishes
>>
>> Phillip
>>

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

[phillip.wood123]

Hi brian

On 02/10/2024 23:04, brian m. carlson wrote:
> On 2024-10-02 at 09:54:52, Phillip Wood wrote:
>> On 30/09/2024 23:03, brian m. carlson wrote:
>> Part of the reason it works well in rust is that it supports discriminated
>> unions with pattern matching and has the "?" macro for early returns. In C
>> the code ends up being quite verbose compared to taking a pointer to error
>> struct as a function parameter and returning a boolean success/fail flag.
>>
>>      struct git_error e;
>>      struct object_id oid;
>>
>>      e = repo_get_oid(r, "HEAD", &oid);
>>      if (!GIT_ERROR_SUCCESS(e))
>>          return e;
>>
>> With a boolean return we can have
>>
>>      struct object_id oid;
>>
>>      if (repo_get_oid(r, "HEAD", &oid, e))
>>          return -1;
>>
>> where "e" is a "struct git_error*" passed into the function.
> 
> Yes, I agree that this is more verbose than in Rust.  If we were using
> Rust (which is a thing I'm working on), then we'd have nicer error
> handling, but for now we don't.
> 
> However, Go still uses this kind of error handling, and many people use
> it every day with this limitation, so I don't think it's too awful for
> what we're getting.

I feel like I'm missing something - what is the advantage of returning 
an error struct rather than passing it as a parameter that makes the 
inconvenience of handling the return value worth while?

>> In conclusion I like the general idea but have concerns about the verbosity
>> of returning an error struct. It would be helpful to see some examples of
>> this being used to see how it works in practice.
> 
> If I send a v2, I'll try to wire up some code so folks can see some
> examples.

I think that would be really helpful. A couple of examples in the cover 
letter showing how you'd convert some function from our codebase would 
give us an idea of what using this api would look like in practice.

Best Wishes

Phillip

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

[Richard Kerry]

This issue reminded me of a system I used for quite a long time, quite a long time ago.
Details of errors were handled using an "Auxiliary Stack".
The caller receives a simple result value indicating Ok or error.  If there is an error then the part of the system that instigated the error may create a frame on the aux stack and add relevant data to it.  Then the initiating caller can just unstack any frames and display, or otherwise handle, (or completely ignore) the data therein.

If a function detected a problem somewhere down the call stack it could stack whatever it liked, and then abort.  Higher levels could add more frames if they wanted.
Although I don't now remember the details it could have been that the convention was to stack a formatting sting together with a series of values.  Then at the top level use a formatter function to turn all that into a single actual string to output (This was Pascal so didn't have C-style formatted string handling).

This means that the only common data type required is a standard OK/Fail value (original was in Pascal so had strongly-typed Boolean for this).  (Implementation of aux stack was in assembler, callers were Pascal).
There is no need for a special return type.  Integer or some sort of Boolean would suffice. 
There is no need for a special variable referencing an error status block to be passed into every call.  This implicitly only allows one set of error information to be returned per call - aux tack could have several.
There is no need for a common table of error code numbers agreed by all parts of the system.
There is no need for a system of registered messages, or an Event Log (eg Windows mc, registry, RegisterEventSource etc).


Regards,
Richard.

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

[Patrick Steinhardt]

On Mon, Sep 30, 2024 at 11:35:50PM +0000, brian m. carlson wrote:
> On 2024-09-30 at 22:44:37, Junio C Hamano wrote:
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > 
> > > ....  It is designed to be passed and returned by value, not
> > > pointer, and it is possible to do so in two registers on 64-bit systems.
> > > Similar functionality works well for error types in Rust and for the
> > > standard library's lldiv_t, so this should not pose a problem.
> > 
> > It should not, in the sense that "any reasonable platform should be
> > able to pass two 64-bit word in a structure by value", but isn't it
> > optimizing for a wrong (i.e. have error) case?  In the case where
> > there is no error, a "negative return is error, zero is success",
> > with a pointer to "more detailed error info, in case the call
> > resulted in an error", would let us take branch based on a zero-ness
> > check on an integer in a machine-natural word, without even looking
> > at these two words in the struct.
> 
> We can adjust the first word so that it's always zero on success, in
> which case, because it's returned in two registers, the processor will
> be able to branch on a zero-ness check on one of those registers.  (We
> can even optimize the check by looking at the low 32 bits, which will do
> the same for 32-bit machines as well.)  The performance benefit will be
> the same, and I should note that Rust does this kind of thing without a
> problem.

I was wondering the same here, also because having to write
`git_error_ok()` is a bit unwieldy. One of my thoughts in this context
was to not be shy of allocating the error structures such that we don't
have to pass by value, but instead pass by pointer. It also gives us a
bit more flexibility with the error structure itself, as we don't have
to optimize for size anymore. Or at least not to the extent as we'd have
to do with the current proposal.

The obvious problem of course is if we're running out of memory. But I
think we can easily special-case this and return a statically-allocated
error specific to that situation, where `git_error_free()` would know
not to free it.

Patrick