RemoteCallbacks exceptions are ignored

[omniproc]

Currently if one implements a custom RemoteCallbacks.get_credentials() and in the process of returning the credentials to the caller throws any Exception, that Exception is ignored and will be replaced by a generic GitError exception, limiting any custom exception handling.

The code that does this seems to be in the remote.py. To be more precise here in the RemoteCallbacks class.

try:
	ccred = get_credentials(credentials, url, username, allowed)
	cred_out[0] = ccred[0]
except Passthrough:
	return C.GIT_PASSTHROUGH
except Exception as e:
	self._stored_exception = e
	return C.GIT_EUSER

It stores the error into _stored_exception but simply returns -7 to the caller.
The calling function then usually will pass that error code to the generic check_error() function of the errors.py module. However, the actual Exception stored in _stored_exception is never processed there. Instead, it simply raises a GitError with the message err -7 (no message provided).

It's unclear how a custom RemoteCallbacks is supposed to perform exception handling when the desired handling is to "bubble the error upwards the stack" where it would be handled and it's unclear why the exception is stored but never re-raised or atleast it's message used to inform the caller about the actual error.

E.g. if someone implements RemoteCallbacks like this:

class MyCallbacks(RemoteCallbacks):
    def credentials(self, url, username_from_url, allowed_types):
        raise Exception("handle me higher up in the stack!")

and calls it like this:

try:
    pygit2.clone_repository("ssh://github.com/libgit2/pygit2", "pygit2.git", callbacks=MyCallbacks())
except GitError ex:
    print(ex)
except Exception as ex:
    print(f"custom exception handled here: {ex}")

One would expect the output to be:

custom exception handled here: handle me higher up in the stack!

While we actually get:

err -7 (no message provided)

[omniproc]

As far as I can tell it seems like there would be a bigger refactoring needed to fix this issue propperly.

So what about a temporary workaround: implement a CustomPygitException Exception that users can throw if they wish to handle those errors on their own and, if CustomPygitException is detected in the check_error() handler just re-raise it.

Something like:

#errors.py
class CustomPygitException(Exception)
'''
Custom exception that will bypass any PyGit2 error handling and bubble up the stack.
Should be extended if custom exceptions are required.
'''
    def __init__(self, message):
        super().__init__(message)

def check_error(err, io=False):
    if err and isinstance(err, CustomPygitException):
        raise err

    if err >= 0:
        return

    ...

If this sounds acceptable let me know and I'll gladly provide the PR.

[jdavid]

Commit cd09686 fixes this for clone_repository, but there're other places where we use credentials callbacks. Feel free to do a PR if you think the approach is good.

I found there was already a similar mechanism for other callbacks in clone_repository, so I followed the same approach.

Thanks!

[omniproc]

Interesting. Well this seems to be all over the classes. I see about 89 instances where check_error() is called. Is there any paricular reason why instead of raising Exceptions - in the most simple form a default Exception with the C.GIT_E* message included - only the error code is passed and the actual exception is then created as generic GitError inside check_error()?
I don't find any special reason for this so I guess that's still a leftover from the first implementation?
Would you mind if I'd redesign and replace that system with native Python Exceptions? Anything that would argue against such a redesign?

89 instances seems doable. I probably could submit a PR by afternoon.

[jdavid]

Not sure to follow. The point of check_error is to transform a libgit2 error code/message to a Python exception. Inside check_error git_error_last is called to get the libgit2 error message.

If I have understood correctly, what you've found is that: when a Python error is raised within a callback, libgit2 will return an error code and the original Python exception will be masked, while it should be re-raised.

The subject of exceptions has been discussed before (though not the callback issue). See issue #830, which is a follow-up of two other issues. For reference I found as well issue #844, which I missed at the time 😕

One important thing is to keep backwards compatibility. For that purpose in issue #830 the proposal is to use multiple inheritance. Backwards compatibility can be broken when it makes sense, but hopefully not too much.

Could you please be more specific on your proposal? Maybe after looking at the background discussion in issue #830

Thanks!

[jdavid]

And thanks for sponsoring!

[omniproc]

One important thing is to keep backwards compatibility. For that purpose in issue #830 the proposal is to use multiple inheritance.

The PR I'm working on now should be fully backward compatible (actually, the new exception class I propose would inherit from GitError just as you suggest in #830 so any older code should still work).
It should result in cleaner code and propper exception re-raise. Working with cross lang code is never ideal so yes, at some point a translation based on exit codes needs to happen. The question is how can we hide as much of it as possible and still use Python default exception handling in all other cases.
I'm working on this for the last hour and so far I didn't run into any code that would break my PR. We'll see how this goes. I'll comment on #830 as well when I'm done with the PR.

I'm not looking into #844 explicitly for now since this seems to be libgit2 related but I'm confident that the proposed change will not make it any more difficult to fix then it is currently if anyone decides to fix it.

And thanks for sponsoring!

Sure thing! Thanks for the lib! I think most ppl. don't realize how bady we need automated Git for many common devops scenarios.

[omniproc]

Pushed a first version in https://github.com/m451/pygit2/commit/08ab93b114fe3a56200a136c490dc592a52c243e

Didn't have time to test everything yet since it's getting late but I wanted to push it for the unittests to kick in.
There is also a open todo regarding the need of _stored_exceptions in the Remote class. It can probably be replaced with the next logic as well but I'll have to look into it.

[omniproc]

Some tests are still failing with https://github.com/m451/pygit2/commit/420cc9aa1b23c57bd8fb7cf9b47bf03340abdcd8 and I'm not sure why. Will have to digg into the tests. I believe the issue is that the unit tests have not been setup for exception handling yet. This is the list of tests that seem to be failing:

• test_cherrypick_remove_conflicts
• test_iterator
• test_read
• test_write
• test_bad_cred_type
• test_describe_follows_first_branch_only
• test_describe_strategies
• test_add
• test_add_aspath
• test_remove
• test_remove_all
• test_remove_all_aspath
• test_remove_aspath
• test_write
• test_merge_no_fastforward_conflicts
• test_merge_remove_conflicts
• test_remote_collection
• test_remote_create
• test_remote_set_url
• test_conflicts_in_bare_repository
• test_stash

Quite hard for me to debug since I don't have a local build env but I'll see what I can do.

[omniproc]

No errors in the unit tests anymore and only actually needed to change one test to check the resulting error message more specificly for all to work, so it should be backwards compatible with any older code. https://github.com/m451/pygit2/commit/6d45967f4939ee97533c37dbb3c9e744a1cdf40a is a first implementation I came up with.

However: getting rid of that _stored_exceptions logic seems to be not as easy as I would have tought.
What I wasn't aware of is that C.GIT_EUSER seems to be used for some kind of flow control in @ffi.callback. So I'll have to dig into anything that is using that decorator and anything that is returning C.GIT_EUSER to get the full picture. Currently, if I try to simply replace the private callback functions _fill_fetch_options, _fill_push_options, _fill_prune_callbacks, _fill_connect_callbacks exception handling that returns C.GIT_EUSER and stores the actual exception in _stored_exception it will result in a segmentation fault.

Since this is more related to #830 I added a TODO list there and would suggest we continue the discussion there. I'll reference this issue in there as to be tested because since I wasn't able to fully get rid of the _stored_exceptions logic, I yet don't know if my initial approach would fix this issue.

BTW: since I saw Python 3.5 was dropped, I decided to use f-Strings where possible. Hope that's fine.

[jdavid]

The issue reported here was fixed with commit cd09686, now in commit a92a453 I've added a unit test for it. Exceptions within a user defined callback are properly re-raised so they can be handled by the code calling the pygit2 API.

Other places where we use callbacks were already good, they didn't suffer from this problem. There was already a unit test for fetch: test_bad_cred_type.