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

[Phillip Wood <phillip.wood123@gmail.com>]

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