From: phillip.wood123@gmail.com Date: Fri, 04 Oct 2024 09:00:24 GMT Subject: Re: [RFC PATCH 1/1] Add a type for errors Message-ID: <9731c23c-4383-42c4-83f6-1cf6b25f89d8@gmail.com> In-Reply-To: Hi Emily On 03/10/2024 17:17, Emily Shaffer wrote: > On Wed, Oct 2, 2024 at 2:54 AM 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. > > 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 >>