Replaced exception handling logic

[omniproc]

As discussed in #830 and #996 this is a initial implementation of a new exception handling mechanism. The goals so far have been:

• Don't break backwards compatibility
• Wrap C-Calls that return an exit code more elegant in a way that a propper Python exception is rised if the exit code is not OK
• If no propper exit code was found raise a generic GitException and try to populate it with as much information as available.
• If a native Python exception occurres during that C-call, raise that exception and do not hide it

As suggested / discussed in #830 it was implemented using mutliple inheritance.
Currently the tree looks like this Exception -> GitError (for backward compatibility) -> BaseGitException (for multi inheritance support and new exception handling logic) -> GitException (as actual class to derive from) -> GitXxxError (Exceptions matching GIT_E* exit codes from libgit2).

The new code is in errors.py - the other classes have only been touched to implement it. A single unittest was changed to be more specific when detecting the returned exception, thought that change should not break anything.

There are multiple ways to use it.

@GitException.check_result
def foo(val):
    if val == 99:
        raise Exception("Native Python exception thrown inside foo().")
    return val 

foo(1)      # OK
foo(0)      # OK
foo(-3)     # Raises GitNotFoundError because GIT_ENOTFOUND == -3
foo(-16)    # Raises GitException because GIT_EAUTH == -16 and GitAuthError has not yet been explicitly implemented as standalone Exception
foo(99)     # Raises Exception with text "Native Python exception thrown inside foo().". Don't wrap/hide/manipulate Python exception but just re-raise them!

# If we want to raise a more specific exception in case an error occures we can do so:
@GitNotFoundError.check_result
def bar(val):
    if val == 99:
        raise Exception("Native Python exception thrown inside foo().")
    return val 

bar(1)      # OK
bar(0)      # OK
bar(-3)     # Raises GitNotFoundError because we explicitly told to raise GitNotFoundError for all error codes
bar(-16)    # Raises GitNotFoundError because we explicitly told to raise GitNotFoundError for all error codes
bar(99)     # Raises Exception with text "Native Python exception thrown inside foo().". Don't wrap/hide/manipulate actual Python exception but just re-raise them!

# Also we have control over the message that should be returned to the user
@GitException.check_result(message="Something bad happend at baz().")
def baz(val):
    if val == 99:
        raise Exception("Native Python exception thrown inside foo().")
    return val 

baz(1)      # OK
baz(0)      # OK
baz(-3)     # Raises GitNotFoundError because GIT_ENOTFOUND == -3. Exception message will be "Something bad happend at baz()."
baz(-16)    # Raises GitException because GIT_EAUTH == -16 and GitAuthError has not yet been explicitly implemented as standalone Exception. Exception message will be "Something bad happend at baz()."
baz(99)     # Raises Exception with text "Native Python exception thrown inside foo().". Don't wrap/hide/manipulate actual Python exception but just re-raise them!


# If we don't want to wrap the hole function - thought a redesign in such a way that C-calls are wrapped into individual Python functions would make sense - we can use old style wrapping without decorators.
# This is usefull for a quick replacement of the current exception logic. For example:
GitException.check_result(baz, message="Something bad happend at baz().")(1)

[jdavid]

Let's see an exemple. Today we have:

err = C.git_clone(crepo, to_bytes(url), to_bytes(path), opts)
check_error(err)

The proposal is to write:

GitException.check_result(C.git_clone)(crepo, to_bytes(url), to_bytes(path), opts)

Today's code already transforms the error code to a Python exception, and it's in my opinion easier to read.

The thing the new code does is to catch and re-raise an exception from C.git_clone. But C.git_clone cannot raise any exception, only return an error code. If C.git_clone could raise an exception, it would work already with today's code.

What happens, as seen in issue #996, is that a callback written in Python may raise an exception. Then the callback has to store that exception somewhere (_stored_exception), and return GIT_EUSER.

For the record, from libgit2 sources:

        /**
         * GIT_EUSER is a special error that is never generated by libgit2
         * code.  You can return it from a callback (e.g to stop an iteration)
         * to know that it was generated by the callback and not by libgit2.
         */
        GIT_EUSER      = -7,

Then the code that has called C.git_clone must check _stored_exception and raise an exception. It would be nice to have a more general solution for this, I understand this is one of the stated goals of this PR:

• If a native Python exception occurres during that C-call, raise that exception and do not hide it

But the PR doesn't address this actually. The code handling _stored_exception is still there.

It looks to me that this PR is actually trying to resolve issue #830, not #996. Am I right?

[omniproc]

It looks to me that this PR is actually trying to resolve issue #830, not #996. Am I right?

Yes, absolutely. Maybe I should have been more clear about that. When looking into the problem as discussed in #996 I realized that before we can handle that it would make sense to first have a more generic exception handling implemented as suggested in #830. The thing that this PR add's, I hope, is to implement it and thus provides us a single place where all errors - Python and C type - can be handled.

Because of this we also get a single place where special error codes like GIT_EUSER can be handled differently. For now it'll just convert it into a Python exception. But this could easily be modified in such a way that it does store and - then later, when the generic GitError occures, rises the actual Exception it stored before. Essentially the logic there can be the same as before: get any exception that occures. If it's a C error, store it and return it's value. The code higher up in the stack, from my understanding, will then raise a generic GitError. At that point the implementation could be changed so it don't simply rise the GitError but rise the stored Exception.
The difference is that now that logic, when needed, can be very different for different error types and is still kept in a central place where you'd expect it: in the error module that's supposed to handle all the errors the way we want it.

I have not yet integrated any logic that would specially handle GIT_EUSER but that would be the next step to fix #998. I just wanted to better understand what's happening there before I apply the changes, so I opend the todos in #830.
If needed I can supply a working example how the handling of such special errors could work with this PR but I'll need some time to fully understand the call stack and think about the different ways that could be handled.

[jdavid]

Okay. The callback code was contributed long time ago by someone else, I myself didn't know exactly how it worked. So I've reviewed it and done some commits. It's all internal refactoring, no external changes (not intended at least).

Now it should be much easier to read and maintain, at least now I understand how it works. You can give a look at the new pygit2/callbacks.py file, which now holds all code related to callbacks, it has some documentation strings and comments that should make it easier.

I don't think there's any dependency between issues #830 and #996. There're many calls to check_error but only a handful of callbacks, GIT_EUSER and GIT_PASSTHROUGH are only of interest in the context of callbacks.

Next I'll work on the unit tests concerning #996 (not completely sure, but I think that issue is already fixed).