{"thread":{"id":"62221","subject":"[RFC PATCH 0/1] Typed errors","startedAt":"2024-09-30T22:04:01Z","lastAt":"2024-10-21T12:47:02Z","messageCount":21,"participants":["brian m. carlson","Junio C Hamano","Oswald Buddenhagen","Emily Shaffer","Phillip Wood","Eric Sunshine","phillip.wood123@gmail.com","Richard Kerry","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"503779","messageId":"20240930220352.2461975-1-sandals@crustytoothpaste.net","threadId":"62221","inReplyTo":null,"subject":"[RFC PATCH 0/1] Typed errors","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-09-30T22:03:51Z","receivedAt":"2024-09-30T22:04:01Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We've discussed, as part of the libification effors and at the most\nrecent Git Merge conference, the possibility of typed errors in Git.\nThis would be useful because it allows users of our library to\nunderstand why an error occurred and what to do about it in addition to\nknowing just that one occurred.\n\nIn addition, as someone who has done a lot of scripting of Git in my\nprofessional life, I've noticed we have a plethora of different messages\nfor the same problem.  As a consequence, it's very difficult to\nautomatically map all errors where an object is absent into a distinct\nerror type, which makes automation hard.  It would be easier to work\nwith Git as a scripting aid if all error of the same type produced a\nuniform message that could be easily parsed.\n\nI had indicated that I have a design for this, which I've improved with\nfeedback from the contributor summit.  This is, therefore, an RFC that\ndoesn't wire things up in any way, but is suitable for discussion about\nthe approach and design. It may be that we eventually adopt this\napproach, or we may discard it in favour of another, and either way,\nthat's fine.\n\nI will say that the fact that we have a pointer here is useful, but it\nmeans that freeing memory is necessary.  That's inconvenient, but if we\nchoose to adopt Rust on a larger scale, it should be much easier to\nhandle by simply using a `Drop` implementation to free the memory.\n\nbrian m. carlson (1):\n  Add a type for errors\n\n Makefile |   1 +\n error.c  |  43 ++++++++++++++\n error.h  | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 212 insertions(+)\n create mode 100644 error.c\n create mode 100644 error.h\n\n"},{"id":"503780","messageId":"20240930220352.2461975-2-sandals@crustytoothpaste.net","threadId":"62221","inReplyTo":"20240930220352.2461975-1-sandals@crustytoothpaste.net","subject":"[RFC PATCH 1/1] Add a type for errors","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-09-30T22:03:52Z","receivedAt":"2024-09-30T22:04:01Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"There is work underway to move some of the Git code out into a reusable\nlibrary.  In such a case, it's not very desirable to have the library\ncode write random errors to standard error, since this is an antipattern\nwhich annoys terminal users.\n\nInstead, we will expect callers of our library function to return\nerrors.  The reusability of our library will be substantially improved\nif we return typed errors so that users can easily determine what kind\nof error might have occurred and react differently based on different\ncontexts.  For example, if we are looking up an object in a partial\nclone and it's not found, we might decide to download it, but we might\nreturn an error to the user if the problem is instead that the revision\nspecified is not syntactically valid.\n\nTo help the libification process and make our code more generally\nmaintainable, add an error type.  This consists of on 64-bit integer,\nwhich contains bit flags and a 32-bit code, and a pointer, which depends\non the code.  It is designed to be passed and returned by value, not\npointer, and it is possible to do so in two registers on 64-bit systems.\nSimilar functionality works well for error types in Rust and for the\nstandard library's lldiv_t, so this should not pose a problem.\n\nProvide the ability to specify either an errno value or a git error code\nas the code.  This allows us to use this type generically when handling\nerrno values such as processing files, as well as express a rich set of\npossible error codes specific to Git.  We pick an unsigned 32-bit code\nbecause Windows can use the full set of 32 bits in its error values,\neven though most Unix systems use only a small set of codes which\ntraditionally start at 1.  32 bits for Git errors also allows us plenty\nof space to expand as we see fit.\n\nAllow multiple errors to be provided and wrapped in a single object,\nwhich is useful in many situations, and add helpers to determine if any\nerror in the set matches a particular code.\n\nAdditionally, provide error formatting functions that produce a suitable\nlocalized string for ease of use.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n Makefile |   1 +\n error.c  |  43 ++++++++++++++\n error.h  | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 212 insertions(+)\n create mode 100644 error.c\n create mode 100644 error.h\n\ndiff --git a/Makefile b/Makefile\nindex 7344a7f725..5d9bf992e9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1013,6 +1013,7 @@ LIB_OBJS += dir.o\n LIB_OBJS += editor.o\n LIB_OBJS += entry.o\n LIB_OBJS += environment.o\n+LIB_OBJS += error.o\n LIB_OBJS += ewah/bitmap.o\n LIB_OBJS += ewah/ewah_bitmap.o\n LIB_OBJS += ewah/ewah_io.o\ndiff --git a/error.c b/error.c\nnew file mode 100644\nindex 0000000000..713bc42187\n--- /dev/null\n+++ b/error.c\n@@ -0,0 +1,43 @@\n+#include \"git-compat-util.h\"\n+#include \"gettext.h\"\n+#include \"error.h\"\n+#include \"hex.h\"\n+#include \"strbuf.h\"\n+\n+const char *git_error_string(struct git_error err)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tif (!git_error_strbuf(&buf, err))\n+\t\treturn NULL;\n+\treturn strbuf_detach(&buf, NULL);\n+}\n+\n+const char *git_error_strbuf(struct strbuf *buf, struct git_error err)\n+{\n+\tif (GIT_ERROR_SUCCESS(err)) {\n+\t\treturn NULL;\n+\t} else if (GIT_ERROR_ERRNO(err) != -1) {\n+\t\treturn xstrdup(strerror(GIT_ERROR_ERRNO(err)));\n+\t} else {\n+\t\tstruct git_error_multiple *me = err.meta;\n+\t\tswitch (GIT_ERROR_GITERR(err)) {\n+\t\tcase GIT_ERR_OBJECT_NOT_FOUND:\n+\t\t\tif (err.meta)\n+\t\t\t\tstrbuf_addf(buf, _(\"object not found: %s\"), oid_to_hex(err.meta));\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(buf, _(\"object not found\"));\n+\t\tcase GIT_ERR_NULL_OID:\n+\t\t\tif (err.meta)\n+\t\t\t\tstrbuf_addf(buf, _(\"null object ID not allowed in this context: %s\"), (char *)err.meta);\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(buf, _(\"null object ID not allowed\"));\n+\t\tcase GIT_ERR_MULTIPLE:\n+\t\t\tstrbuf_addf(buf, _(\"multiple errors:\\n\"));\n+\t\t\tfor (size_t i = 0; i < me->count; i++) {\n+\t\t\t\tgit_error_strbuf(buf, me->errs[i]);\n+\t\t\t\tstrbuf_addstr(buf, \"\\n\");\n+\t\t\t}\n+\t\t}\n+\t\treturn buf->buf;\n+\t}\n+}\ndiff --git a/error.h b/error.h\nnew file mode 100644\nindex 0000000000..485cca99e0\n--- /dev/null\n+++ b/error.h\n@@ -0,0 +1,168 @@\n+#ifndef ERROR_H\n+#define ERROR_H\n+\n+#include \"git-compat-util.h\"\n+\n+/* Set if this value is initialized. */\n+#define GIT_ERROR_BIT_INIT (1ULL << 63)\n+/* Set if the code is an errno code, clear if it's a git error code. */\n+#define GIT_ERROR_BIT_ERRNO (1ULL << 62)\n+/*\n+ * Set if the memory in meta should be freed; otherwise, it's statically\n+ * allocated.\n+ */\n+#define GIT_ERROR_BIT_ALLOC (1ULL << 61)\n+/*\n+ * Set if the memory in meta is a C string; otherwise, it's a metadata struct.\n+ */\n+#define GIT_ERROR_BIT_MSG (1ULL << 60)\n+\n+#define GIT_ERROR_BIT_MASK (0xf << 60)\n+\n+#define GIT_ERROR_OK (git_error_ok())\n+\n+#define GIT_ERROR_SUCCESS(e) (((e).code == GIT_ERROR_BIT_INIT))\n+#define GIT_ERROR_SUCCESS_CONSUME(e) ((git_error_free(&(e)).code == GIT_ERROR_BIT_INIT)\n+\n+#define GIT_ERROR_ERRNO(e) (((e).code & GIT_ERROR_BIT_ERRNO) ? ((e).code & 0xffffffff) : -1)\n+#define GIT_ERROR_GITERR(e) (((e).code & GIT_ERROR_BIT_ERRNO) ? -1 : ((e).code & 0xffffffff))\n+\n+/*\n+ * A value representing an error in Git.\n+ */\n+struct git_error {\n+\tuint64_t code;\n+\tvoid *meta;\n+};\n+\n+struct git_error_multiple {\n+\tstruct git_error *errs;\n+\tsize_t count;\n+};\n+\n+enum git_error_code {\n+\t/* The operation was a success. */\n+\tGIT_ERR_SUCCESS = 0,\n+\t/* An object ID was provided, but it was not found.\n+\t *\n+\t * meta will be NULL or a pointer to struct object ID.\n+\t */\n+\tGIT_ERR_OBJECT_NOT_FOUND = 1,\n+\t/*\n+\t * An object ID was provided, but it is all zeros and that is not\n+\t * allowed.\n+\t *\n+\t * meta will be NULL or a message explaining the context.\n+\t */\n+\tGIT_ERR_NULL_OID = 2,\n+\t/*\n+\t * Multiple errors occurred.\n+\t *\n+\t * meta must be a pointer to struct git_error_multiple.\n+\t */\n+\tGIT_ERR_MULTIPLE = 3,\n+};\n+\n+const char *git_error_string(struct git_error err);\n+const char *git_error_strbuf(struct strbuf *buf, struct git_error err);\n+\n+/*\n+ * A successful error status.\n+ */\n+static inline struct git_error git_error_ok(void) {\n+\tstruct git_error e =  {\n+\t\t.code = 0 | GIT_ERROR_BIT_INIT,\n+\t\t.meta = NULL,\n+\t};\n+\treturn e;\n+}\n+\n+static inline struct git_error git_error_new_errno(uint32_t errnoval, const char *msg, int to_free) {\n+\tstruct git_error e =  {\n+\t\t.code = errnoval | GIT_ERROR_BIT_INIT | GIT_ERROR_BIT_ERRNO |\n+\t\t\tGIT_ERROR_BIT_MSG | (to_free ? GIT_ERROR_BIT_ALLOC : 0),\n+\t\t.meta = (void *)msg,\n+\t};\n+\treturn e;\n+}\n+\n+static inline struct git_error git_error_new_git(uint32_t gitcode, const char *msg, int to_free) {\n+\tstruct git_error e =  {\n+\t\t.code = gitcode | GIT_ERROR_BIT_INIT |\n+\t\t\tGIT_ERROR_BIT_MSG | (to_free ? GIT_ERROR_BIT_ALLOC : 0),\n+\t\t.meta = (void *)msg,\n+\t};\n+\treturn e;\n+}\n+\n+static inline struct git_error git_error_new_simple(uint32_t gitcode) {\n+\tstruct git_error e =  {\n+\t\t.code = gitcode | GIT_ERROR_BIT_INIT,\n+\t\t.meta = NULL,\n+\t};\n+\treturn e;\n+}\n+\n+static inline struct git_error git_error_new_multiple(struct git_error *errs, size_t count)\n+{\n+\tstruct git_error_multiple *me = xmalloc(sizeof(*me));\n+\tstruct git_error e =  {\n+\t\t.code = GIT_ERR_MULTIPLE | GIT_ERROR_BIT_INIT | GIT_ERROR_BIT_ALLOC,\n+\t\t.meta = me,\n+\t};\n+\tme->errs = errs;\n+\tme->count = count;\n+\treturn e;\n+}\n+\n+/*\n+ * If this is a git error and the code matches the given code, or if this is a\n+ * multiple error and any of the contained errors are a git error whose code\n+ * matches, returns a pointer to that error.  If there is no match, returns\n+ * NULL.\n+ */\n+static inline struct git_error *git_error_is_git(struct git_error *e, int code) {\n+\tint64_t giterr = GIT_ERROR_GITERR(*e);\n+\tif (giterr == code) {\n+\t\treturn e;\n+\t} else if (giterr == GIT_ERR_MULTIPLE) {\n+\t\tstruct git_error_multiple *me = e->meta;\n+\t\tfor (size_t i = 0; i < me->count; i++)\n+\t\t\treturn git_error_is_git(me->errs + i, code);\n+\t\treturn NULL;\n+\t} else {\n+\t\treturn NULL;\n+\t}\n+}\n+\n+/*\n+ * If this is an errno error and the code matches the given code, or if this is\n+ * a multiple error and any of the contained errors are an errno error whose\n+ * code matches, returns a pointer to that error.  Otherwise, returns NULL.\n+ */\n+static inline struct git_error *git_error_is_errno(struct git_error *e, int code) {\n+\tint64_t giterr = GIT_ERROR_GITERR(*e);\n+\tif (GIT_ERROR_ERRNO(*e) == code) {\n+\t\treturn e;\n+\t} else if (giterr == GIT_ERR_MULTIPLE) {\n+\t\tstruct git_error_multiple *me = e->meta;\n+\t\tfor (size_t i = 0; i < me->count; i++)\n+\t\t\treturn git_error_is_errno(me->errs + i, code);\n+\t\treturn NULL;\n+\t} else {\n+\t\treturn NULL;\n+\t}\n+}\n+\n+/* Frees and deinitializes this error structure. */\n+static inline uint64_t git_error_free(struct git_error *e)\n+{\n+\tuint64_t code = e->code;\n+\tif (e->code & GIT_ERROR_BIT_ALLOC)\n+\t\tfree(e->meta);\n+\te->code = 0;\n+\te->meta = NULL;\n+\treturn code;\n+}\n+\n+#endif\n"},{"id":"503791","messageId":"xmqqv7yc3goa.fsf@gitster.g","threadId":"62221","inReplyTo":"20240930220352.2461975-2-sandals@crustytoothpaste.net","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-30T22:44:37Z","receivedAt":"2024-09-30T22:44:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> ....  It is designed to be passed and returned by value, not\n> pointer, and it is possible to do so in two registers on 64-bit systems.\n> Similar functionality works well for error types in Rust and for the\n> standard library's lldiv_t, so this should not pose a problem.\n\nIt should not, in the sense that \"any reasonable platform should be\nable to pass two 64-bit word in a structure by value\", but isn't it\noptimizing for a wrong (i.e. have error) case?  In the case where\nthere is no error, a \"negative return is error, zero is success\",\nwith a pointer to \"more detailed error info, in case the call\nresulted in an error\", would let us take branch based on a zero-ness\ncheck on an integer in a machine-natural word, without even looking\nat these two words in the struct.\n\n\n\n"},{"id":"503792","messageId":"Zvs11sEe_6c_L8DA@tapette.crustytoothpaste.net","threadId":"62221","inReplyTo":"xmqqv7yc3goa.fsf@gitster.g","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-09-30T23:35:50Z","receivedAt":"2024-09-30T23:35:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-09-30 at 22:44:37, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > ....  It is designed to be passed and returned by value, not\n> > pointer, and it is possible to do so in two registers on 64-bit systems.\n> > Similar functionality works well for error types in Rust and for the\n> > standard library's lldiv_t, so this should not pose a problem.\n> \n> It should not, in the sense that \"any reasonable platform should be\n> able to pass two 64-bit word in a structure by value\", but isn't it\n> optimizing for a wrong (i.e. have error) case?  In the case where\n> there is no error, a \"negative return is error, zero is success\",\n> with a pointer to \"more detailed error info, in case the call\n> resulted in an error\", would let us take branch based on a zero-ness\n> check on an integer in a machine-natural word, without even looking\n> at these two words in the struct.\n\nWe can adjust the first word so that it's always zero on success, in\nwhich case, because it's returned in two registers, the processor will\nbe able to branch on a zero-ness check on one of those registers.  (We\ncan even optimize the check by looking at the low 32 bits, which will do\nthe same for 32-bit machines as well.)  The performance benefit will be\nthe same, and I should note that Rust does this kind of thing without a\nproblem.\n\nWe did discuss passing an error accumulating argument to all functions\nat the contributor summit, but it didn't seem that was the way folks\nwanted to go: people seemed to prefer a return value.\n\nI'm totally open to other proposals here and not particularly tied to\nthis one, but I told Emily that I'd send a proposal to the list, and so\nhere it is.  If we like this with changes or prefer something different,\nthat's fine with me.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"503844","messageId":"ZvwVZs-HLy0RiP3E@ugly","threadId":"62221","inReplyTo":"20240930220352.2461975-2-sandals@crustytoothpaste.net","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2024-10-01T15:29:42Z","receivedAt":"2024-10-01T15:29:44Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Mon, Sep 30, 2024 at 10:03:52PM +0000, brian m. carlson wrote:\n>+\t\tcase GIT_ERR_MULTIPLE:\n>+\t\t\tstrbuf_addf(buf, _(\"multiple errors:\\n\"));\n>+\t\t\tfor (size_t i = 0; i < me->count; i++) {\n>+\t\t\t\tgit_error_strbuf(buf, me->errs[i]);\n>+\t\t\t\tstrbuf_addstr(buf, \"\\n\");\n>+\t\t\t}\n>+\t\t}\n\nah. i was wondering how you'd address this (\"this\" being non-fatal\nerrors piling up), as i'm facing the same problem in a project of mine.\n\nthe problem is that one can either impose a very rigid formatting (as\nyou do here), or provide formatting options, which can get arbitrarily\ncomplex. so i wonder whether one shouldn't pursue a hybrid approach\ninstead: singular return values for low-level functions that either work\nor fail fatally, and error callbacks for higher-level functionality?\n\nbtw, the switch misses breaks.\n\n"},{"id":"503871","messageId":"CAJoAoZm2RJnu52Pb-vr0VWS_GQkL5n=DZs7KU50EZyuEYhawYw@mail.gmail.com","threadId":"62221","inReplyTo":"20240930220352.2461975-2-sandals@crustytoothpaste.net","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Emily Shaffer","fromEmail":"nasamuffin@google.com","sentAt":"2024-10-01T20:31:15Z","receivedAt":"2024-10-01T20:31:30Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Sep 30, 2024 at 3:04 PM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n>\n> There is work underway to move some of the Git code out into a reusable\n> library.  In such a case, it's not very desirable to have the library\n> code write random errors to standard error, since this is an antipattern\n> which annoys terminal users.\n>\n> Instead, we will expect callers of our library function to return\n> errors.  The reusability of our library will be substantially improved\n> if we return typed errors so that users can easily determine what kind\n> of error might have occurred and react differently based on different\n> contexts.  For example, if we are looking up an object in a partial\n> clone and it's not found, we might decide to download it, but we might\n> return an error to the user if the problem is instead that the revision\n> specified is not syntactically valid.\n>\n> To help the libification process and make our code more generally\n> maintainable, add an error type.  This consists of on 64-bit integer,\n> which contains bit flags and a 32-bit code, and a pointer, which depends\n> on the code.  It is designed to be passed and returned by value, not\n> pointer, and it is possible to do so in two registers on 64-bit systems.\n> Similar functionality works well for error types in Rust and for the\n> standard library's lldiv_t, so this should not pose a problem.\n>\n> Provide the ability to specify either an errno value or a git error code\n> as the code.  This allows us to use this type generically when handling\n> errno values such as processing files, as well as express a rich set of\n> possible error codes specific to Git.  We pick an unsigned 32-bit code\n> because Windows can use the full set of 32 bits in its error values,\n> even though most Unix systems use only a small set of codes which\n> traditionally start at 1.  32 bits for Git errors also allows us plenty\n> of space to expand as we see fit.\n>\n> Allow multiple errors to be provided and wrapped in a single object,\n> which is useful in many situations, and add helpers to determine if any\n> error in the set matches a particular code.\n>\n> Additionally, provide error formatting functions that produce a suitable\n> localized string for ease of use.\n>\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n\nLeft a few comments below about portability and readability, but\ngeneral thoughts: of course this is quite limited, but I think it will\nactually be enough in most cases. It also seems like if we need more\nlater, we can use this same code-OR-with-pointer thing to point to\nsomething more nuanced than `void* meta`. I'd be in favor of applying\nthis model to somewhere with clear error codes and relatively few\ninvocations (maybe to the internals of something like argparse or\nrun-command, without initially modifying the public interface, for\nexample?) and seeing where we run into friction. I have a little bit\nof concern about the idea of centralizing all the `meta` parsing and\nerror string generation, but it seems like we might be able to adopt a\nmodel more similar to the one we use for `git_config()` callbacks and\ndo more context-aware parsing instead to keep `git_error_strbuf` from\ngetting too huge.\n\nThanks for sending this. I'm looking forward to hearing others' opinions.\n - Emily\n\n> ---\n>  Makefile |   1 +\n>  error.c  |  43 ++++++++++++++\n>  error.h  | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  3 files changed, 212 insertions(+)\n>  create mode 100644 error.c\n>  create mode 100644 error.h\n>\n> diff --git a/Makefile b/Makefile\n> index 7344a7f725..5d9bf992e9 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1013,6 +1013,7 @@ LIB_OBJS += dir.o\n>  LIB_OBJS += editor.o\n>  LIB_OBJS += entry.o\n>  LIB_OBJS += environment.o\n> +LIB_OBJS += error.o\n>  LIB_OBJS += ewah/bitmap.o\n>  LIB_OBJS += ewah/ewah_bitmap.o\n>  LIB_OBJS += ewah/ewah_io.o\n> diff --git a/error.c b/error.c\n> new file mode 100644\n> index 0000000000..713bc42187\n> --- /dev/null\n> +++ b/error.c\n> @@ -0,0 +1,43 @@\n> +#include \"git-compat-util.h\"\n> +#include \"gettext.h\"\n> +#include \"error.h\"\n> +#include \"hex.h\"\n> +#include \"strbuf.h\"\n> +\n> +const char *git_error_string(struct git_error err)\n> +{\n> +       struct strbuf buf = STRBUF_INIT;\n> +       if (!git_error_strbuf(&buf, err))\n> +               return NULL;\n> +       return strbuf_detach(&buf, NULL);\n> +}\n> +\n> +const char *git_error_strbuf(struct strbuf *buf, struct git_error err)\n\nIs the idea that we continue to extend `git_error_strbuf` with more\n`git_error_code` values as we add this method of error handling across\nthe codebase? I'm worried it could get quite unwieldy. Or are you\nsuggesting that we could write `git_error_strbuf_object_store`,\n`git_error_strbuf_hook`, etc to keep the code->string conversion\nlocal? That would let us do custom processing of err.meta depending on\ncontext, too, wouldn't it?\n\n> +{\n> +       if (GIT_ERROR_SUCCESS(err)) {\n> +               return NULL;\n> +       } else if (GIT_ERROR_ERRNO(err) != -1) {\n> +               return xstrdup(strerror(GIT_ERROR_ERRNO(err)));\n> +       } else {\n> +               struct git_error_multiple *me = err.meta;\n> +               switch (GIT_ERROR_GITERR(err)) {\n> +               case GIT_ERR_OBJECT_NOT_FOUND:\n> +                       if (err.meta)\n> +                               strbuf_addf(buf, _(\"object not found: %s\"), oid_to_hex(err.meta));\n> +                       else\n> +                               strbuf_addf(buf, _(\"object not found\"));\n> +               case GIT_ERR_NULL_OID:\n> +                       if (err.meta)\n> +                               strbuf_addf(buf, _(\"null object ID not allowed in this context: %s\"), (char *)err.meta);\n> +                       else\n> +                               strbuf_addf(buf, _(\"null object ID not allowed\"));\n> +               case GIT_ERR_MULTIPLE:\n> +                       strbuf_addf(buf, _(\"multiple errors:\\n\"));\n> +                       for (size_t i = 0; i < me->count; i++) {\n> +                               git_error_strbuf(buf, me->errs[i]);\n> +                               strbuf_addstr(buf, \"\\n\");\n> +                       }\n> +               }\n> +               return buf->buf;\n> +       }\n> +}\n> diff --git a/error.h b/error.h\n> new file mode 100644\n> index 0000000000..485cca99e0\n> --- /dev/null\n> +++ b/error.h\n> @@ -0,0 +1,168 @@\n> +#ifndef ERROR_H\n> +#define ERROR_H\n> +\n> +#include \"git-compat-util.h\"\n> +\n> +/* Set if this value is initialized. */\n> +#define GIT_ERROR_BIT_INIT (1ULL << 63)\n> +/* Set if the code is an errno code, clear if it's a git error code. */\n> +#define GIT_ERROR_BIT_ERRNO (1ULL << 62)\n> +/*\n> + * Set if the memory in meta should be freed; otherwise, it's statically\n> + * allocated.\n> + */\n> +#define GIT_ERROR_BIT_ALLOC (1ULL << 61)\n> +/*\n> + * Set if the memory in meta is a C string; otherwise, it's a metadata struct.\n> + */\n> +#define GIT_ERROR_BIT_MSG (1ULL << 60)\n> +\n> +#define GIT_ERROR_BIT_MASK (0xf << 60)\n> +\n> +#define GIT_ERROR_OK (git_error_ok())\n> +\n> +#define GIT_ERROR_SUCCESS(e) (((e).code == GIT_ERROR_BIT_INIT))\n> +#define GIT_ERROR_SUCCESS_CONSUME(e) ((git_error_free(&(e)).code == GIT_ERROR_BIT_INIT)\n> +\n> +#define GIT_ERROR_ERRNO(e) (((e).code & GIT_ERROR_BIT_ERRNO) ? ((e).code & 0xffffffff) : -1)\n> +#define GIT_ERROR_GITERR(e) (((e).code & GIT_ERROR_BIT_ERRNO) ? -1 : ((e).code & 0xffffffff))\n\nAurgh, too bad we don't get bitfields before C11. :) (Although I am\nnot sure if that helps with your register-level optimization at that\npoint... but it probably helps with readability.)\n\nBut, I do wonder if this gluing-together-two-types-into-one-value\nthing may break based on endianness? (And if we care? I don't think we\nhave any tests running on POWER systems, so maybe this falls under the\numbrella of \"you should give us tests if you want us to not break\nyou\"?)\n\n> +\n> +/*\n> + * A value representing an error in Git.\n> + */\n> +struct git_error {\n> +       uint64_t code;\n> +       void *meta;\n> +};\n> +\n> +struct git_error_multiple {\n> +       struct git_error *errs;\n> +       size_t count;\n> +};\n> +\n> +enum git_error_code {\n> +       /* The operation was a success. */\n> +       GIT_ERR_SUCCESS = 0,\n> +       /* An object ID was provided, but it was not found.\n> +        *\n> +        * meta will be NULL or a pointer to struct object ID.\n> +        */\n> +       GIT_ERR_OBJECT_NOT_FOUND = 1,\n> +       /*\n> +        * An object ID was provided, but it is all zeros and that is not\n> +        * allowed.\n> +        *\n> +        * meta will be NULL or a message explaining the context.\n> +        */\n> +       GIT_ERR_NULL_OID = 2,\n> +       /*\n> +        * Multiple errors occurred.\n> +        *\n> +        * meta must be a pointer to struct git_error_multiple.\n> +        */\n> +       GIT_ERR_MULTIPLE = 3,\n> +};\n> +\n> +const char *git_error_string(struct git_error err);\n> +const char *git_error_strbuf(struct strbuf *buf, struct git_error err);\n> +\n> +/*\n> + * A successful error status.\n> + */\n> +static inline struct git_error git_error_ok(void) {\n> +       struct git_error e =  {\n> +               .code = 0 | GIT_ERROR_BIT_INIT,\n> +               .meta = NULL,\n> +       };\n> +       return e;\n> +}\n> +\n> +static inline struct git_error git_error_new_errno(uint32_t errnoval, const char *msg, int to_free) {\n> +       struct git_error e =  {\n> +               .code = errnoval | GIT_ERROR_BIT_INIT | GIT_ERROR_BIT_ERRNO |\n> +                       GIT_ERROR_BIT_MSG | (to_free ? GIT_ERROR_BIT_ALLOC : 0),\n> +               .meta = (void *)msg,\n> +       };\n> +       return e;\n> +}\n> +\n> +static inline struct git_error git_error_new_git(uint32_t gitcode, const char *msg, int to_free) {\n> +       struct git_error e =  {\n> +               .code = gitcode | GIT_ERROR_BIT_INIT |\n> +                       GIT_ERROR_BIT_MSG | (to_free ? GIT_ERROR_BIT_ALLOC : 0),\n> +               .meta = (void *)msg,\n> +       };\n> +       return e;\n> +}\n> +\n> +static inline struct git_error git_error_new_simple(uint32_t gitcode) {\n> +       struct git_error e =  {\n> +               .code = gitcode | GIT_ERROR_BIT_INIT,\n> +               .meta = NULL,\n> +       };\n> +       return e;\n> +}\n> +\n> +static inline struct git_error git_error_new_multiple(struct git_error *errs, size_t count)\n> +{\n> +       struct git_error_multiple *me = xmalloc(sizeof(*me));\n> +       struct git_error e =  {\n> +               .code = GIT_ERR_MULTIPLE | GIT_ERROR_BIT_INIT | GIT_ERROR_BIT_ALLOC,\n> +               .meta = me,\n> +       };\n> +       me->errs = errs;\n> +       me->count = count;\n> +       return e;\n> +}\n> +\n> +/*\n> + * If this is a git error and the code matches the given code, or if this is a\n> + * multiple error and any of the contained errors are a git error whose code\n> + * matches, returns a pointer to that error.  If there is no match, returns\n> + * NULL.\n> + */\n> +static inline struct git_error *git_error_is_git(struct git_error *e, int code) {\n> +       int64_t giterr = GIT_ERROR_GITERR(*e);\n> +       if (giterr == code) {\n> +               return e;\n> +       } else if (giterr == GIT_ERR_MULTIPLE) {\n> +               struct git_error_multiple *me = e->meta;\n> +               for (size_t i = 0; i < me->count; i++)\n> +                       return git_error_is_git(me->errs + i, code);\n> +               return NULL;\n> +       } else {\n> +               return NULL;\n> +       }\n> +}\n> +\n> +/*\n> + * If this is an errno error and the code matches the given code, or if this is\n> + * a multiple error and any of the contained errors are an errno error whose\n> + * code matches, returns a pointer to that error.  Otherwise, returns NULL.\n> + */\n> +static inline struct git_error *git_error_is_errno(struct git_error *e, int code) {\n> +       int64_t giterr = GIT_ERROR_GITERR(*e);\n> +       if (GIT_ERROR_ERRNO(*e) == code) {\n> +               return e;\n> +       } else if (giterr == GIT_ERR_MULTIPLE) {\n> +               struct git_error_multiple *me = e->meta;\n> +               for (size_t i = 0; i < me->count; i++)\n> +                       return git_error_is_errno(me->errs + i, code);\n> +               return NULL;\n> +       } else {\n> +               return NULL;\n> +       }\n> +}\n> +\n> +/* Frees and deinitializes this error structure. */\n> +static inline uint64_t git_error_free(struct git_error *e)\n> +{\n> +       uint64_t code = e->code;\n> +       if (e->code & GIT_ERROR_BIT_ALLOC)\n> +               free(e->meta);\n> +       e->code = 0;\n> +       e->meta = NULL;\n> +       return code;\n> +}\n> +\n> +#endif\n"},{"id":"503899","messageId":"2d2f14ea-cfdc-4b52-948f-b42c8f6e41de@gmail.com","threadId":"62221","inReplyTo":"20240930220352.2461975-2-sandals@crustytoothpaste.net","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-02T09:54:52Z","receivedAt":"2024-10-02T09:54:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi brian\n\nThanks for working on this.\n\nOn 30/09/2024 23:03, brian m. carlson wrote:\n> There is work underway to move some of the Git code out into a reusable\n> library.  In such a case, it's not very desirable to have the library\n> code write random errors to standard error, since this is an antipattern\n> which annoys terminal users.\n> \n> Instead, we will expect callers of our library function to return\n> errors. \n\nIsn't it the callers that will expect the function to return an error?\n\n> The reusability of our library will be substantially improved\n> if we return typed errors so that users can easily determine what kind\n> of error might have occurred and react differently based on different\n> contexts.  For example, if we are looking up an object in a partial\n> clone and it's not found, we might decide to download it, but we might\n> return an error to the user if the problem is instead that the revision\n> specified is not syntactically valid.\n> \n> To help the libification process and make our code more generally\n> maintainable, add an error type.  This consists of on 64-bit integer,\n> which contains bit flags and a 32-bit code, and a pointer, which depends\n> on the code.  It is designed to be passed and returned by value, not\n> pointer, and it is possible to do so in two registers on 64-bit systems.\n> Similar functionality works well for error types in Rust and for the\n> standard library's lldiv_t, so this should not pose a problem.\n\nPart of the reason it works well in rust is that it supports \ndiscriminated unions with pattern matching and has the \"?\" macro for \nearly returns. In C the code ends up being quite verbose compared to \ntaking a pointer to error struct as a function parameter and returning a \nboolean success/fail flag.\n\n     struct git_error e;\n     struct object_id oid;\n\n     e = repo_get_oid(r, \"HEAD\", &oid);\n     if (!GIT_ERROR_SUCCESS(e))\n         return e;\n\nWith a boolean return we can have\n\n     struct object_id oid;\n\n     if (repo_get_oid(r, \"HEAD\", &oid, e))\n         return -1;\n\nwhere \"e\" is a \"struct git_error*\" passed into the function.\n\n> Provide the ability to specify either an errno value or a git error code\n> as the code.  This allows us to use this type generically when handling\n> errno values such as processing files, as well as express a rich set of\n> possible error codes specific to Git.  We pick an unsigned 32-bit code\n> because Windows can use the full set of 32 bits in its error values,\n> even though most Unix systems use only a small set of codes which\n> traditionally start at 1.  32 bits for Git errors also allows us plenty\n> of space to expand as we see fit.\n\nI think the design of the struct is fine. It does mean we need a global \nlist of error values. GError [1] avoids this by having a \"domain\" field \nthat is an interned string so that error codes only need to be unique \nwithin a given domain. I think that for a self-contained project like \ngit a global list is probably fine - svn does this for example [2].\n\n[1] https://docs.gtk.org/glib/error-reporting.html\n[2] \nhttps://github.com/apache/subversion/blob/be229fd54f5860b3140831671efbfd3f7f6fbb0b/subversion/include/svn_error_codes.h\n\n> Allow multiple errors to be provided and wrapped in a single object,\n> which is useful in many situations, and add helpers to determine if any\n> error in the set matches a particular code.\n\nThe api appears to require the caller know up front how many errors \nthere will be which seems unlikely to be true in practice. I think a \nmore realistic design would allow callers to push errors as they occur \nand grow the array accordingly. For example ref_transaction_prepare() \nwould want to return a list of errors, one for each ref that it was \nunable to lock or which did not match the expected value but it would \nnot know up-front how many errors there were going to be.\n\nIt would be useful to be able to add context to an error as the stack is \nunwound. For example if unpack_trees() detects that it would overwrite \nuntracked files it prints a error listing those files. The exact \nformatting of that message depends on the command being run. That is \ncurrently handled by calling setup_unpack_trees_porcelain() with the \nname of the command before calling unpack_trees(). In a world where \nunpack_trees() returns a error containing the list of files we would \nwant to customize the message by adding some context before converting \nit to a string.\n\n> Additionally, provide error formatting functions that produce a suitable\n> localized string for ease of use.\n\nI share Emily's concern that this function has to know the details of \nhow to format every error. We could mitigate that somewhat using a \nswitch that calls external helper functions that do the actual formatting\n\n     switch (e.code) {\n     case GIT_ERR_OBJECT_NOT_FOUND:\n         format_object_not_found(buf, e); /* lives in another file */\n         break;\n     ...\n\nI know this is an RFC but I couldn't resist one code comment\n\n> +#define GIT_ERROR_SUCCESS(e) (((e).code == GIT_ERROR_BIT_INIT))\n> +#define GIT_ERROR_SUCCESS_CONSUME(e) ((git_error_free(&(e)).code == GIT_ERROR_BIT_INIT)\n\ngit_error_free() returns the code as in integer so we don't need \".code\" \nhere. Also our coding guidelines would suggest git_error_clear() for the \nname of that function.\n\n\nIn conclusion I like the general idea but have concerns about the \nverbosity of returning an error struct. It would be helpful to see some \nexamples of this being used to see how it works in practice.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"503930","messageId":"dc1a70b3-9a0f-4c5e-931d-8b3b91335ec7@gmail.com","threadId":"62221","inReplyTo":"ZvwVZs-HLy0RiP3E@ugly","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-02T14:01:13Z","receivedAt":"2024-10-02T14:01:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Oswald\n\nOn 01/10/2024 16:29, Oswald Buddenhagen wrote:\n> On Mon, Sep 30, 2024 at 10:03:52PM +0000, brian m. carlson wrote:\n>> +        case GIT_ERR_MULTIPLE:\n>> +            strbuf_addf(buf, _(\"multiple errors:\\n\"));\n>> +            for (size_t i = 0; i < me->count; i++) {\n>> +                git_error_strbuf(buf, me->errs[i]);\n>> +                strbuf_addstr(buf, \"\\n\");\n>> +            }\n>> +        }\n> \n> ah. i was wondering how you'd address this (\"this\" being non-fatal\n> errors piling up), as i'm facing the same problem in a project of mine.\n> \n> the problem is that one can either impose a very rigid formatting (as\n> you do here), or provide formatting options, which can get arbitrarily\n> complex. so i wonder whether one shouldn't pursue a hybrid approach\n> instead: singular return values for low-level functions that either work\n> or fail fatally, and error callbacks for higher-level functionality?\n\nThat's a good point - the rigid formatting is easy to implement but is \nnot ideal from the user's point of view. Ideally we'd compose a coherent \nmessage with the data from the various errors but as you say that's not \ntrivial. I wonder if there are any good examples out there that we could \nuse as inspiration. The ones I'm aware of (rust's anyhow and svn's \nsvn_error_compose) go for a rigid approach. Glib's GError punts the \nproblem entirely by forbidding multiple errors.\n\nBest Wishes\n\nPhillip\n\n> btw, the switch misses breaks.\n> \n> \n\n"},{"id":"503979","messageId":"Zv3AZKjcOoAKOzLn@tapette.crustytoothpaste.net","threadId":"62221","inReplyTo":"CAJoAoZm2RJnu52Pb-vr0VWS_GQkL5n=DZs7KU50EZyuEYhawYw@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-10-02T21:51:32Z","receivedAt":"2024-10-02T21:51:40Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-10-01 at 20:31:15, Emily Shaffer wrote:\n> On Mon, Sep 30, 2024 at 3:04 PM brian m. carlson\n> <sandals@crustytoothpaste.net> wrote:\n> > +const char *git_error_strbuf(struct strbuf *buf, struct git_error err)\n> \n> Is the idea that we continue to extend `git_error_strbuf` with more\n> `git_error_code` values as we add this method of error handling across\n> the codebase? I'm worried it could get quite unwieldy. Or are you\n> suggesting that we could write `git_error_strbuf_object_store`,\n> `git_error_strbuf_hook`, etc to keep the code->string conversion\n> local? That would let us do custom processing of err.meta depending on\n> context, too, wouldn't it?\n\nMy intention was to centralize this processing in one place.  Part of\nthe problem we have is that we have many different messages for one\nerror code, so `GIT_ERR_OBJECT_NOT_FOUND` might have \"object not found -\n(HEX)\" or a variety of other messages, which makes parsing errors\ndifficult.  (As I said, we do this at work, and it's quite annoying.)\n\nIf folks want to do thing differently, we can, but we should avoid\nlosing the consistent error message behaviour.\n\n> > +#define GIT_ERROR_SUCCESS(e) (((e).code == GIT_ERROR_BIT_INIT))\n> > +#define GIT_ERROR_SUCCESS_CONSUME(e) ((git_error_free(&(e)).code == GIT_ERROR_BIT_INIT)\n> > +\n> > +#define GIT_ERROR_ERRNO(e) (((e).code & GIT_ERROR_BIT_ERRNO) ? ((e).code & 0xffffffff) : -1)\n> > +#define GIT_ERROR_GITERR(e) (((e).code & GIT_ERROR_BIT_ERRNO) ? -1 : ((e).code & 0xffffffff))\n> \n> Aurgh, too bad we don't get bitfields before C11. :) (Although I am\n> not sure if that helps with your register-level optimization at that\n> point... but it probably helps with readability.)\n> \n> But, I do wonder if this gluing-together-two-types-into-one-value\n> thing may break based on endianness? (And if we care? I don't think we\n> have any tests running on POWER systems, so maybe this falls under the\n> umbrella of \"you should give us tests if you want us to not break\n> you\"?)\n\nI don't think this breaks on big-endian systems because we're always\ntreating it as a `uint64_t` and never converting it into bytes.  My\nfirst laptop was a PowerPC Mac running Linux and I've owned UltraSPARC\nhardware in the past, so I'm pretty confident in avoiding writing\nendianness (and alignment) bugs.  I wouldn't have written it in a way\nthat broke on less common hardware even if we don't have tests for it.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"503981","messageId":"Zv3DehUxEN6SJI1M@tapette.crustytoothpaste.net","threadId":"62221","inReplyTo":"2d2f14ea-cfdc-4b52-948f-b42c8f6e41de@gmail.com","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-10-02T22:04:42Z","receivedAt":"2024-10-02T22:04:44Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-10-02 at 09:54:52, Phillip Wood wrote:\n> On 30/09/2024 23:03, brian m. carlson wrote:\n> > There is work underway to move some of the Git code out into a reusable\n> > library.  In such a case, it's not very desirable to have the library\n> > code write random errors to standard error, since this is an antipattern\n> > which annoys terminal users.\n> > \n> > Instead, we will expect callers of our library function to return\n> > errors.\n> \n> Isn't it the callers that will expect the function to return an error?\n\nYes, that's correct.  I'll fix that.\n\n> > To help the libification process and make our code more generally\n> > maintainable, add an error type.  This consists of on 64-bit integer,\n> > which contains bit flags and a 32-bit code, and a pointer, which depends\n> > on the code.  It is designed to be passed and returned by value, not\n> > pointer, and it is possible to do so in two registers on 64-bit systems.\n> > Similar functionality works well for error types in Rust and for the\n> > standard library's lldiv_t, so this should not pose a problem.\n> \n> Part of the reason it works well in rust is that it supports discriminated\n> unions with pattern matching and has the \"?\" macro for early returns. In C\n> the code ends up being quite verbose compared to taking a pointer to error\n> struct as a function parameter and returning a boolean success/fail flag.\n> \n>     struct git_error e;\n>     struct object_id oid;\n> \n>     e = repo_get_oid(r, \"HEAD\", &oid);\n>     if (!GIT_ERROR_SUCCESS(e))\n>         return e;\n> \n> With a boolean return we can have\n> \n>     struct object_id oid;\n> \n>     if (repo_get_oid(r, \"HEAD\", &oid, e))\n>         return -1;\n> \n> where \"e\" is a \"struct git_error*\" passed into the function.\n\nYes, I agree that this is more verbose than in Rust.  If we were using\nRust (which is a thing I'm working on), then we'd have nicer error\nhandling, but for now we don't.\n\nHowever, Go still uses this kind of error handling, and many people use\nit every day with this limitation, so I don't think it's too awful for\nwhat we're getting.  I won't say that Go is my favourite language and I\ndo prefer the less verbose error handling in Rust, but the fact that\nthis design is widely used means that it's at least a defensible\ndecision.\n\n> I think the design of the struct is fine. It does mean we need a global list\n> of error values. GError [1] avoids this by having a \"domain\" field that is\n> an interned string so that error codes only need to be unique within a given\n> domain. I think that for a self-contained project like git a global list is\n> probably fine - svn does this for example [2].\n\nI think so, too.  libgit2 does the same thing, which seems to have\nworked out okay.\n\n> > Allow multiple errors to be provided and wrapped in a single object,\n> > which is useful in many situations, and add helpers to determine if any\n> > error in the set matches a particular code.\n> \n> The api appears to require the caller know up front how many errors there\n> will be which seems unlikely to be true in practice. I think a more\n> realistic design would allow callers to push errors as they occur and grow\n> the array accordingly. For example ref_transaction_prepare() would want to\n> return a list of errors, one for each ref that it was unable to lock or\n> which did not match the expected value but it would not know up-front how\n> many errors there were going to be.\n\nThat seems reasonable.  We could add helpers to extend the number of\nerrors.\n\n> It would be useful to be able to add context to an error as the stack is\n> unwound. For example if unpack_trees() detects that it would overwrite\n> untracked files it prints a error listing those files. The exact formatting\n> of that message depends on the command being run. That is currently handled\n> by calling setup_unpack_trees_porcelain() with the name of the command\n> before calling unpack_trees(). In a world where unpack_trees() returns a\n> error containing the list of files we would want to customize the message by\n> adding some context before converting it to a string.\n\nI think we could do that with either a particular error type (e.g.,\n`GIT_ERR_WOULD_OVERWRITE`) that contains nested errors and text or a\ngeneric `GIT_ERR_ANNOTATED`.\n\n> > Additionally, provide error formatting functions that produce a suitable\n> > localized string for ease of use.\n> \n> I share Emily's concern that this function has to know the details of how to\n> format every error. We could mitigate that somewhat using a switch that\n> calls external helper functions that do the actual formatting\n> \n>     switch (e.code) {\n>     case GIT_ERR_OBJECT_NOT_FOUND:\n>         format_object_not_found(buf, e); /* lives in another file */\n>         break;\n>     ...\n> \n> I know this is an RFC but I couldn't resist one code comment\n\nI'm fine with that approach.  This is mostly designed to solicit\ncomments about the overall design, and I don't have a strong opinion\nabout how we format.\n\n> > +#define GIT_ERROR_SUCCESS(e) (((e).code == GIT_ERROR_BIT_INIT))\n> > +#define GIT_ERROR_SUCCESS_CONSUME(e) ((git_error_free(&(e)).code == GIT_ERROR_BIT_INIT)\n> \n> git_error_free() returns the code as in integer so we don't need \".code\"\n> here. Also our coding guidelines would suggest git_error_clear() for the\n> name of that function.\n\nGot it.\n\n> In conclusion I like the general idea but have concerns about the verbosity\n> of returning an error struct. It would be helpful to see some examples of\n> this being used to see how it works in practice.\n\nIf I send a v2, I'll try to wire up some code so folks can see some\nexamples.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"503984","messageId":"CAPig+cRkA58wr_Pn-uhVM0zFA7B0hnArXDeumrjnWrgBbG64Yw@mail.gmail.com","threadId":"62221","inReplyTo":"Zv3DehUxEN6SJI1M@tapette.crustytoothpaste.net","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-02T22:16:00Z","receivedAt":"2024-10-02T22:16:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 2, 2024 at 6:04 PM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n> On 2024-10-02 at 09:54:52, Phillip Wood wrote:\n> > Part of the reason it works well in rust is that it supports discriminated\n> > unions with pattern matching and has the \"?\" macro for early returns. In C\n> > the code ends up being quite verbose compared to taking a pointer to error\n> > struct as a function parameter and returning a boolean success/fail flag.\n> >\n> >     struct git_error e;\n> >     struct object_id oid;\n> >\n> >     e = repo_get_oid(r, \"HEAD\", &oid);\n> >     if (!GIT_ERROR_SUCCESS(e))\n> >         return e;\n> >\n> > With a boolean return we can have\n> >\n> >     struct object_id oid;\n> >\n> >     if (repo_get_oid(r, \"HEAD\", &oid, e))\n> >         return -1;\n> >\n> > where \"e\" is a \"struct git_error*\" passed into the function.\n>\n> However, Go still uses this kind of error handling, and many people use\n> it every day with this limitation, so I don't think it's too awful for\n> what we're getting.  I won't say that Go is my favourite language and I\n> do prefer the less verbose error handling in Rust, but the fact that\n> this design is widely used means that it's at least a defensible\n> decision.\n\nI'm not sure I understand your response to Phillip's observation.\nIdiomatic error handling in Go:\n\n    if oid, err := repo_get_oid(r, \"HEAD\"); err != nil {\n        return err;\n    }\n\nseems much closer to Phillip's more succinct example than to the more\nverbose example using GIT_ERROR_SUCCESS().\n"},{"id":"503986","messageId":"Zv3IHd0c4_0NPjOK@tapette.crustytoothpaste.net","threadId":"62221","inReplyTo":"CAPig+cRkA58wr_Pn-uhVM0zFA7B0hnArXDeumrjnWrgBbG64Yw@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-10-02T22:24:29Z","receivedAt":"2024-10-02T22:24:31Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-10-02 at 22:16:00, Eric Sunshine wrote:\n> I'm not sure I understand your response to Phillip's observation.\n> Idiomatic error handling in Go:\n> \n>     if oid, err := repo_get_oid(r, \"HEAD\"); err != nil {\n>         return err;\n>     }\n> \n> seems much closer to Phillip's more succinct example than to the more\n> verbose example using GIT_ERROR_SUCCESS().\n\nI don't think that approach works if you want to use `oid`, since it\nscopes just to the `if` statement (and any relevant `else`).  If `oid`\nwere already defined, then you would need to omit the colon in `:=`, or\nthe compiler would complain about \"no new variables\".\n\nThat's why I usually see this:\n\n  oid, err := repo_get_oid(r, \"HEAD\")\n  if err != nil {\n      return err\n  }\n\nwhich is much more like what Phillip's more verbose example shows.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"504014","messageId":"CAPig+cQjk5_VJ-LNc9Hx3Q4n6H5bgg5cAztWhFpuweG8wFTjMw@mail.gmail.com","threadId":"62221","inReplyTo":"Zv3IHd0c4_0NPjOK@tapette.crustytoothpaste.net","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-03T05:14:11Z","receivedAt":"2024-10-03T05:14:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 2, 2024 at 6:24 PM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n> On 2024-10-02 at 22:16:00, Eric Sunshine wrote:\n> > I'm not sure I understand your response to Phillip's observation.\n> > Idiomatic error handling in Go:\n> >\n> >     if oid, err := repo_get_oid(r, \"HEAD\"); err != nil {\n> >         return err;\n> >     }\n> >\n> > seems much closer to Phillip's more succinct example than to the more\n> > verbose example using GIT_ERROR_SUCCESS().\n>\n> I don't think that approach works if you want to use `oid`, since it\n> scopes just to the `if` statement (and any relevant `else`).  If `oid`\n> were already defined, then you would need to omit the colon in `:=`, or\n> the compiler would complain about \"no new variables\".\n>\n> That's why I usually see this:\n>\n>   oid, err := repo_get_oid(r, \"HEAD\")\n>   if err != nil {\n>       return err\n>   }\n>\n> which is much more like what Phillip's more verbose example shows.\n\nYes, you often see that in Go, as well as:\n\n    var oid oid_type\n    var err error\n    if oid, err = repo_get_oid(r, \"HEAD\"); err != nil {\n        return err;\n    }\n\nwhich is very much in line with Phillip's succinct C example. But\nisn't this all beside the point?\n\nYour proposal uses Rust as a model to justify the API choice in this\nRFC, but Phillip's point was that -- despite being perfectly suitable\nin Rust -- it is _not_ ergonomic in C. Rather than explaining how the\nproposed non-ergonomic API is superior to the ergonomic API in\nPhillip's example, you instead responded by saying that people in some\nother programming language (Go, in this case) have to deal with\nnon-ergonomic error handling on a daily basis, therefore, a\nnon-ergonomic API is good enough for Git's C programmers. But that is\nnot a very convincing argument for choosing a non-ergonomic API for\n*this* project which is written in C, especially considering that a\nmore ergonomic API has been presented (and is already in use in parts\nof the codebase).\n\nThat's why I said in my original response that I didn't understand\nyour response to Phillip. You seem to be using a non-justification\n(\"other programmers suffer, so Git programmers can suffer too\") as a\njustification for a non-ergonomic design.\n"},{"id":"504029","messageId":"xmqqwmipi342.fsf@gitster.g","threadId":"62221","inReplyTo":"CAPig+cQjk5_VJ-LNc9Hx3Q4n6H5bgg5cAztWhFpuweG8wFTjMw@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-03T16:05:17Z","receivedAt":"2024-10-03T16:05:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Your proposal uses Rust as a model to justify the API choice in this\n> RFC, but Phillip's point was that -- despite being perfectly suitable\n> in Rust -- it is _not_ ergonomic in C.\n> ...\n> That's why I said in my original response that I didn't understand\n> your response to Phillip. You seem to be using a non-justification\n> (\"other programmers suffer, so Git programmers can suffer too\") as a\n> justification for a non-ergonomic design.\n\nThe statement may be a bit too harsh, as some may not even realize\nthat they are suffering anymore, after prolonged exposure to these\nidioms, just like C folks consider it is a fact of life that they\nhave to carefully manage their pointers and the memory they point\nat.\n\nI do agree that \"return value with more details in the out parameter\nwhose address is supplied by the caller\" is a convention that is\neasier to grok when written in C.\n\nThanks.\n"},{"id":"504030","messageId":"CAJoAoZnhY0Z7XdNqt8A598jptiNVDJC=4kT5n_n1FCGN5GkXRg@mail.gmail.com","threadId":"62221","inReplyTo":"2d2f14ea-cfdc-4b52-948f-b42c8f6e41de@gmail.com","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Emily Shaffer","fromEmail":"nasamuffin@google.com","sentAt":"2024-10-03T16:17:12Z","receivedAt":"2024-10-03T16:17:27Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Oct 2, 2024 at 2:54 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>\n> Hi brian\n>\n> Thanks for working on this.\n>\n> On 30/09/2024 23:03, brian m. carlson wrote:\n> > There is work underway to move some of the Git code out into a reusable\n> > library.  In such a case, it's not very desirable to have the library\n> > code write random errors to standard error, since this is an antipattern\n> > which annoys terminal users.\n> >\n> > Instead, we will expect callers of our library function to return\n> > errors.\n>\n> Isn't it the callers that will expect the function to return an error?\n>\n> > The reusability of our library will be substantially improved\n> > if we return typed errors so that users can easily determine what kind\n> > of error might have occurred and react differently based on different\n> > contexts.  For example, if we are looking up an object in a partial\n> > clone and it's not found, we might decide to download it, but we might\n> > return an error to the user if the problem is instead that the revision\n> > specified is not syntactically valid.\n> >\n> > To help the libification process and make our code more generally\n> > maintainable, add an error type.  This consists of on 64-bit integer,\n> > which contains bit flags and a 32-bit code, and a pointer, which depends\n> > on the code.  It is designed to be passed and returned by value, not\n> > pointer, and it is possible to do so in two registers on 64-bit systems.\n> > Similar functionality works well for error types in Rust and for the\n> > standard library's lldiv_t, so this should not pose a problem.\n>\n> Part of the reason it works well in rust is that it supports\n> discriminated unions with pattern matching and has the \"?\" macro for\n> early returns. In C the code ends up being quite verbose compared to\n> taking a pointer to error struct as a function parameter and returning a\n> boolean success/fail flag.\n>\n>      struct git_error e;\n>      struct object_id oid;\n>\n>      e = repo_get_oid(r, \"HEAD\", &oid);\n>      if (!GIT_ERROR_SUCCESS(e))\n>          return e;\n>\n> With a boolean return we can have\n>\n>      struct object_id oid;\n>\n>      if (repo_get_oid(r, \"HEAD\", &oid, e))\n>          return -1;\n>\n> where \"e\" is a \"struct git_error*\" passed into the function.\n\nI actually don't find this complaint all that compelling; it's not\nhard to write a shorter macro that can be used inline, so we can do\nthings like:\n\n    ERR_VAR(e);\n    if(ERR(e, repo_get_oid(...))\n      return e;\n\nor even a macro to do the return if desired:\n\n    ERR_VAR(e); // or, i guess we can be not SO lazy and just write\nstruct git_error e here, whatever :) )\n    RETURN_IF_ERR(e, repo_get_oid(...));\n\nFor better or worse, you can do a lot of things in a macro, so I don't\nsee verboseness as much of an issue because I think we can hide a lot\nof it this way.\n\n>\n> > Provide the ability to specify either an errno value or a git error code\n> > as the code.  This allows us to use this type generically when handling\n> > errno values such as processing files, as well as express a rich set of\n> > possible error codes specific to Git.  We pick an unsigned 32-bit code\n> > because Windows can use the full set of 32 bits in its error values,\n> > even though most Unix systems use only a small set of codes which\n> > traditionally start at 1.  32 bits for Git errors also allows us plenty\n> > of space to expand as we see fit.\n>\n> I think the design of the struct is fine. It does mean we need a global\n> list of error values. GError [1] avoids this by having a \"domain\" field\n> that is an interned string so that error codes only need to be unique\n> within a given domain. I think that for a self-contained project like\n> git a global list is probably fine - svn does this for example [2].\n>\n> [1] https://docs.gtk.org/glib/error-reporting.html\n> [2]\n> https://github.com/apache/subversion/blob/be229fd54f5860b3140831671efbfd3f7f6fbb0b/subversion/include/svn_error_codes.h\n>\n> > Allow multiple errors to be provided and wrapped in a single object,\n> > which is useful in many situations, and add helpers to determine if any\n> > error in the set matches a particular code.\n>\n> The api appears to require the caller know up front how many errors\n> there will be which seems unlikely to be true in practice. I think a\n> more realistic design would allow callers to push errors as they occur\n> and grow the array accordingly. For example ref_transaction_prepare()\n> would want to return a list of errors, one for each ref that it was\n> unable to lock or which did not match the expected value but it would\n> not know up-front how many errors there were going to be.\n>\n> It would be useful to be able to add context to an error as the stack is\n> unwound. For example if unpack_trees() detects that it would overwrite\n> untracked files it prints a error listing those files. The exact\n> formatting of that message depends on the command being run. That is\n> currently handled by calling setup_unpack_trees_porcelain() with the\n> name of the command before calling unpack_trees(). In a world where\n> unpack_trees() returns a error containing the list of files we would\n> want to customize the message by adding some context before converting\n> it to a string.\n>\n> > Additionally, provide error formatting functions that produce a suitable\n> > localized string for ease of use.\n>\n> I share Emily's concern that this function has to know the details of\n> how to format every error. We could mitigate that somewhat using a\n> switch that calls external helper functions that do the actual formatting\n>\n>      switch (e.code) {\n>      case GIT_ERR_OBJECT_NOT_FOUND:\n>          format_object_not_found(buf, e); /* lives in another file */\n>          break;\n>      ...\n>\n> I know this is an RFC but I couldn't resist one code comment\n>\n> > +#define GIT_ERROR_SUCCESS(e) (((e).code == GIT_ERROR_BIT_INIT))\n> > +#define GIT_ERROR_SUCCESS_CONSUME(e) ((git_error_free(&(e)).code == GIT_ERROR_BIT_INIT)\n>\n> git_error_free() returns the code as in integer so we don't need \".code\"\n> here. Also our coding guidelines would suggest git_error_clear() for the\n> name of that function.\n>\n>\n> In conclusion I like the general idea but have concerns about the\n> verbosity of returning an error struct. It would be helpful to see some\n> examples of this being used to see how it works in practice.\n>\n> Best Wishes\n>\n> Phillip\n>\n"},{"id":"504059","messageId":"CAPig+cSta8arLqa+v1rVwMdd7w0q4RxNzrooCzKAma88cR25hA@mail.gmail.com","threadId":"62221","inReplyTo":"xmqqwmipi342.fsf@gitster.g","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-03T22:27:29Z","receivedAt":"2024-10-03T22:27:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 3, 2024 at 12:05 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > That's why I said in my original response that I didn't understand\n> > your response to Phillip. You seem to be using a non-justification\n> > (\"other programmers suffer, so Git programmers can suffer too\") as a\n> > justification for a non-ergonomic design.\n>\n> The statement may be a bit too harsh [...]\n\nYes, sorry if that came across as harsh, brian. The \"Git programmers\ncan suffer too\" was my (clearly) poor tongue-in-cheek attempt to\nsoften the earlier part of the email in case it sounded harsh.\n"},{"id":"504064","messageId":"Zv8zvogCJ9EZDBQC@tapette.crustytoothpaste.net","threadId":"62221","inReplyTo":"CAPig+cSta8arLqa+v1rVwMdd7w0q4RxNzrooCzKAma88cR25hA@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-10-04T00:15:58Z","receivedAt":"2024-10-04T00:16:06Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-10-03 at 22:27:29, Eric Sunshine wrote:\n> On Thu, Oct 3, 2024 at 12:05 PM Junio C Hamano <gitster@pobox.com> wrote:\n> > Eric Sunshine <sunshine@sunshineco.com> writes:\n> > > That's why I said in my original response that I didn't understand\n> > > your response to Phillip. You seem to be using a non-justification\n> > > (\"other programmers suffer, so Git programmers can suffer too\") as a\n> > > justification for a non-ergonomic design.\n> >\n> > The statement may be a bit too harsh [...]\n> \n> Yes, sorry if that came across as harsh, brian. The \"Git programmers\n> can suffer too\" was my (clearly) poor tongue-in-cheek attempt to\n> soften the earlier part of the email in case it sounded harsh.\n\nI'm definitely not offended.  We can certainly disagree on the merits of\nthe proposal; hopefully we'll find some solution that the project\ngenerally approves of.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"504075","messageId":"9731c23c-4383-42c4-83f6-1cf6b25f89d8@gmail.com","threadId":"62221","inReplyTo":"CAJoAoZnhY0Z7XdNqt8A598jptiNVDJC=4kT5n_n1FCGN5GkXRg@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-04T09:00:24Z","receivedAt":"2024-10-04T09:00:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Emily\n\nOn 03/10/2024 17:17, Emily Shaffer wrote:\n> On Wed, Oct 2, 2024 at 2:54 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>>\n>> Part of the reason it works well in rust is that it supports\n>> discriminated unions with pattern matching and has the \"?\" macro for\n>> early returns. In C the code ends up being quite verbose compared to\n>> taking a pointer to error struct as a function parameter and returning a\n>> boolean success/fail flag.\n>>\n>>       struct git_error e;\n>>       struct object_id oid;\n>>\n>>       e = repo_get_oid(r, \"HEAD\", &oid);\n>>       if (!GIT_ERROR_SUCCESS(e))\n>>           return e;\n>>\n>> With a boolean return we can have\n>>\n>>       struct object_id oid;\n>>\n>>       if (repo_get_oid(r, \"HEAD\", &oid, e))\n>>           return -1;\n>>\n>> where \"e\" is a \"struct git_error*\" passed into the function.\n> \n> I actually don't find this complaint all that compelling; it's not\n> hard to write a shorter macro that can be used inline, so we can do\n> things like:\n> \n>      ERR_VAR(e);\n>      if(ERR(e, repo_get_oid(...))\n>        return e;\n\nRight, but what's the advantage over passing the error struct as a \nfunction parameter? It feels like we're adding extra complexity and \nhiding the assignment to \"e\" to work around the inconvenience of \nreturning a struct rather than a flag. Is there some other advantage to \nreturning a struct that makes this worthwhile?\n\nBest Wishes\n\nPhillip\n\n> or even a macro to do the return if desired:\n> \n>      ERR_VAR(e); // or, i guess we can be not SO lazy and just write\n> struct git_error e here, whatever :) )\n>      RETURN_IF_ERR(e, repo_get_oid(...));\n> \n> For better or worse, you can do a lot of things in a macro, so I don't\n> see verboseness as much of an issue because I think we can hide a lot\n> of it this way.\n> \n>>\n>>> Provide the ability to specify either an errno value or a git error code\n>>> as the code.  This allows us to use this type generically when handling\n>>> errno values such as processing files, as well as express a rich set of\n>>> possible error codes specific to Git.  We pick an unsigned 32-bit code\n>>> because Windows can use the full set of 32 bits in its error values,\n>>> even though most Unix systems use only a small set of codes which\n>>> traditionally start at 1.  32 bits for Git errors also allows us plenty\n>>> of space to expand as we see fit.\n>>\n>> I think the design of the struct is fine. It does mean we need a global\n>> list of error values. GError [1] avoids this by having a \"domain\" field\n>> that is an interned string so that error codes only need to be unique\n>> within a given domain. I think that for a self-contained project like\n>> git a global list is probably fine - svn does this for example [2].\n>>\n>> [1] https://docs.gtk.org/glib/error-reporting.html\n>> [2]\n>> https://github.com/apache/subversion/blob/be229fd54f5860b3140831671efbfd3f7f6fbb0b/subversion/include/svn_error_codes.h\n>>\n>>> Allow multiple errors to be provided and wrapped in a single object,\n>>> which is useful in many situations, and add helpers to determine if any\n>>> error in the set matches a particular code.\n>>\n>> The api appears to require the caller know up front how many errors\n>> there will be which seems unlikely to be true in practice. I think a\n>> more realistic design would allow callers to push errors as they occur\n>> and grow the array accordingly. For example ref_transaction_prepare()\n>> would want to return a list of errors, one for each ref that it was\n>> unable to lock or which did not match the expected value but it would\n>> not know up-front how many errors there were going to be.\n>>\n>> It would be useful to be able to add context to an error as the stack is\n>> unwound. For example if unpack_trees() detects that it would overwrite\n>> untracked files it prints a error listing those files. The exact\n>> formatting of that message depends on the command being run. That is\n>> currently handled by calling setup_unpack_trees_porcelain() with the\n>> name of the command before calling unpack_trees(). In a world where\n>> unpack_trees() returns a error containing the list of files we would\n>> want to customize the message by adding some context before converting\n>> it to a string.\n>>\n>>> Additionally, provide error formatting functions that produce a suitable\n>>> localized string for ease of use.\n>>\n>> I share Emily's concern that this function has to know the details of\n>> how to format every error. We could mitigate that somewhat using a\n>> switch that calls external helper functions that do the actual formatting\n>>\n>>       switch (e.code) {\n>>       case GIT_ERR_OBJECT_NOT_FOUND:\n>>           format_object_not_found(buf, e); /* lives in another file */\n>>           break;\n>>       ...\n>>\n>> I know this is an RFC but I couldn't resist one code comment\n>>\n>>> +#define GIT_ERROR_SUCCESS(e) (((e).code == GIT_ERROR_BIT_INIT))\n>>> +#define GIT_ERROR_SUCCESS_CONSUME(e) ((git_error_free(&(e)).code == GIT_ERROR_BIT_INIT)\n>>\n>> git_error_free() returns the code as in integer so we don't need \".code\"\n>> here. Also our coding guidelines would suggest git_error_clear() for the\n>> name of that function.\n>>\n>>\n>> In conclusion I like the general idea but have concerns about the\n>> verbosity of returning an error struct. It would be helpful to see some\n>> examples of this being used to see how it works in practice.\n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n\n"},{"id":"504076","messageId":"c37e4dd0-9ad0-4d96-9d0e-ee13d64eb7a0@gmail.com","threadId":"62221","inReplyTo":"Zv3DehUxEN6SJI1M@tapette.crustytoothpaste.net","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-04T09:00:49Z","receivedAt":"2024-10-04T09:00:52Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi brian\n\nOn 02/10/2024 23:04, brian m. carlson wrote:\n> On 2024-10-02 at 09:54:52, Phillip Wood wrote:\n>> On 30/09/2024 23:03, brian m. carlson wrote:\n>> Part of the reason it works well in rust is that it supports discriminated\n>> unions with pattern matching and has the \"?\" macro for early returns. In C\n>> the code ends up being quite verbose compared to taking a pointer to error\n>> struct as a function parameter and returning a boolean success/fail flag.\n>>\n>>      struct git_error e;\n>>      struct object_id oid;\n>>\n>>      e = repo_get_oid(r, \"HEAD\", &oid);\n>>      if (!GIT_ERROR_SUCCESS(e))\n>>          return e;\n>>\n>> With a boolean return we can have\n>>\n>>      struct object_id oid;\n>>\n>>      if (repo_get_oid(r, \"HEAD\", &oid, e))\n>>          return -1;\n>>\n>> where \"e\" is a \"struct git_error*\" passed into the function.\n> \n> Yes, I agree that this is more verbose than in Rust.  If we were using\n> Rust (which is a thing I'm working on), then we'd have nicer error\n> handling, but for now we don't.\n> \n> However, Go still uses this kind of error handling, and many people use\n> it every day with this limitation, so I don't think it's too awful for\n> what we're getting.\n\nI feel like I'm missing something - what is the advantage of returning \nan error struct rather than passing it as a parameter that makes the \ninconvenience of handling the return value worth while?\n\n>> In conclusion I like the general idea but have concerns about the verbosity\n>> of returning an error struct. It would be helpful to see some examples of\n>> this being used to see how it works in practice.\n> \n> If I send a v2, I'll try to wire up some code so folks can see some\n> examples.\n\nI think that would be really helpful. A couple of examples in the cover \nletter showing how you'd convert some function from our codebase would \ngive us an idea of what using this api would look like in practice.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"504085","messageId":"PA4PR07MB7406087827EE13D3716C82649E722@PA4PR07MB7406.eurprd07.prod.outlook.com","threadId":"62221","inReplyTo":"c37e4dd0-9ad0-4d96-9d0e-ee13d64eb7a0@gmail.com","subject":"RE: [RFC PATCH 1/1] Add a type for errors","fromName":"Richard Kerry","fromEmail":"richard.kerry@eviden.com","sentAt":"2024-10-04T12:13:19Z","receivedAt":"2024-10-04T12:18:33Z","isPatch":true,"sender":{"key":"richard.kerry@eviden.com","avatar":null},"body":"\nThis issue reminded me of a system I used for quite a long time, quite a long time ago.\nDetails of errors were handled using an \"Auxiliary Stack\".\nThe 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.\n\nIf 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.\nAlthough 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).\n\nThis 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).\nThere is no need for a special return type.  Integer or some sort of Boolean would suffice. \nThere 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.\nThere is no need for a common table of error code numbers agreed by all parts of the system.\nThere is no need for a system of registered messages, or an Event Log (eg Windows mc, registry, RegisterEventSource etc).\n\n\nRegards,\nRichard.\n\n\n"},{"id":"505675","messageId":"ZxZNObRiWvxTk6OG@pks.im","threadId":"62221","inReplyTo":"Zvs11sEe_6c_L8DA@tapette.crustytoothpaste.net","subject":"Re: [RFC PATCH 1/1] Add a type for errors","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-21T12:46:54Z","receivedAt":"2024-10-21T12:47:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 30, 2024 at 11:35:50PM +0000, brian m. carlson wrote:\n> On 2024-09-30 at 22:44:37, Junio C Hamano wrote:\n> > \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> > \n> > > ....  It is designed to be passed and returned by value, not\n> > > pointer, and it is possible to do so in two registers on 64-bit systems.\n> > > Similar functionality works well for error types in Rust and for the\n> > > standard library's lldiv_t, so this should not pose a problem.\n> > \n> > It should not, in the sense that \"any reasonable platform should be\n> > able to pass two 64-bit word in a structure by value\", but isn't it\n> > optimizing for a wrong (i.e. have error) case?  In the case where\n> > there is no error, a \"negative return is error, zero is success\",\n> > with a pointer to \"more detailed error info, in case the call\n> > resulted in an error\", would let us take branch based on a zero-ness\n> > check on an integer in a machine-natural word, without even looking\n> > at these two words in the struct.\n> \n> We can adjust the first word so that it's always zero on success, in\n> which case, because it's returned in two registers, the processor will\n> be able to branch on a zero-ness check on one of those registers.  (We\n> can even optimize the check by looking at the low 32 bits, which will do\n> the same for 32-bit machines as well.)  The performance benefit will be\n> the same, and I should note that Rust does this kind of thing without a\n> problem.\n\nI was wondering the same here, also because having to write\n`git_error_ok()` is a bit unwieldy. One of my thoughts in this context\nwas to not be shy of allocating the error structures such that we don't\nhave to pass by value, but instead pass by pointer. It also gives us a\nbit more flexibility with the error structure itself, as we don't have\nto optimize for size anymore. Or at least not to the extent as we'd have\nto do with the current proposal.\n\nThe obvious problem of course is if we're running out of memory. But I\nthink we can easily special-case this and return a statically-allocated\nerror specific to that situation, where `git_error_free()` would know\nnot to free it.\n\nPatrick\n"}]}