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

[phillip.wood123@gmail.com]

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