Credentials callback not set when pushing

[eldavido]

@carlosmn did some hard debugging on this.

The story starts off with me writing some python code that uses libgit2. I'm creating a wrapper module around a git repo to use for append-only document storage, and I have a method in the repo that gets the "origin" remote:

    def _get_origin(self):
        origin = self.git_repo.remotes[0]
        if origin.name != 'origin':
            raise Exception('Expecting only origin remote in manifest repo')

        def authentication_cb(url, username, allowed):
            print('URL: ' % url)
            print('Username: ' % username)
            print('Allowed: ' % allowed)

            return UserPass('eldavido', '<redacted>')

        origin.credentials = authentication_cb
        origin.push_url = origin.url
        origin.add_push('refs/heads/*:refs/heads/*')
        origin.save()

        return origin

After dozens of tries, I kept getting the following error when attempting a push operation: _pygit2.GitError: authentication required but no callback set.

After scratching my head and puzzling around with different ways to set callbacks, I jumped in to remote.py in pygit2, and discovered some code to set up the C callbacks in fetch:

        # Get the default callbacks first
        defaultcallbacks = ffi.new('git_remote_callbacks *')
        err = C.git_remote_init_callbacks(defaultcallbacks, 1)
        check_error(err)

        # Build custom callback structure
        callbacks = ffi.new('git_remote_callbacks *')
        callbacks.version = 1
        callbacks.sideband_progress = self._sideband_progress_cb
        callbacks.transfer_progress = self._transfer_progress_cb
        callbacks.update_tips = self._update_tips_cb
        callbacks.credentials = self._credentials_cb

I saw nothing equivalent that gets called via the push code path.

Looking further, I ended up debugging into libgit2 directly and checking the remote's callbacks structure that gets passed via FFI to libgit2, and found that the credentials callback on it was null.

Was this deliberate, or just an oversight? If it was just an oversight, I could take a crack at refactoring this over the weekend by extracting the callback setters into a common method in remote.py, and ensuring that method gets called on all push/fetch operations in the file. Just wanted to check in before I blow half a weekend day on this...

[eldavido]

OK yeah I have a patch for this, I'll open a PR on it shortly. Didn't take as long as I'd thought. I think there were a lot of authors in the file who had conflicting ideas about how the memory retain/release pattern should work for this module; I moved the callback initialization into the constructor and everything seems to work fine.

Also, in general, I realized it's a good idea to wrap things that call user-supplied callbacks inside exception traps, a la

def library_callback_handler(args):
    try:
        UserCallback(args)
    except Exception as e:
        self._stored_exception = e

as exceptions don't propagate back through this stack trace properly:

• User defined callback (Python) - exception here => Bad Things
• pygit2 callback wrapper, which calls user-defined callback
• libgit2, called via FFI into C, which calls back into pygit2 callback wrapper
• pygit2, which calls libgit2 via FFI
• User code, calls pygit2

[jdavid]

So I understand this is fixed.. closing