Skip to content

Refactors credentials callback on remote to make pushes authenticate#438

Closed
eldavido wants to merge 1 commit intolibgit2:masterfrom
eldavido:bugfix/no-credentials-on-push
Closed

Refactors credentials callback on remote to make pushes authenticate#438
eldavido wants to merge 1 commit intolibgit2:masterfrom
eldavido:bugfix/no-credentials-on-push

Conversation

@eldavido
Copy link
Copy Markdown

@eldavido eldavido commented Oct 18, 2014

#437

There's a single test break in this:

======================================================================
FAIL: test_remote_refcount (test.test_remote.RepositoryTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/dalbrecht/pygit2/test/test_remote.py", line 189, in test_remote_refcount
    self.assertEqual(start, end)
AssertionError: 2 != 3

----------------------------------------------------------------------
Ran 228 tests in 9.560s

FAILED (failures=1)

It looks like the patch is causing the refcount to get thrown. If someone could take a look at why this is happening, we can get this merged and pushes should work with credentials.

@carlosmn
Copy link
Copy Markdown
Member

carlosmn commented Oct 18, 2014

The reason is that you removed a self._self_handle = None assignment, which is necessary to break the cyclic reference which python cannot break itself. That is what's keeping #431 from getting merged, which attempts to solve this problem as well.

@jdavid
Copy link
Copy Markdown
Member

jdavid commented Nov 3, 2014

So issue #437 and PRs #431 and #438 are about the same problem? Would be nice to get this fixed for the upcoming release.

@carlosmn
Copy link
Copy Markdown
Member

carlosmn commented Nov 3, 2014

Yeah, I was hoping @DrWong would get back to us on the handle issue, but I can squash the fix in and push that to master if you'd like. We can still have a refactoring after the pushing bit is fixed.

@jdavid
Copy link
Copy Markdown
Member

jdavid commented Nov 3, 2014

@carlosmn that would be cool

@jdavid
Copy link
Copy Markdown
Member

jdavid commented Nov 3, 2014

So I understand this is fixed.. closing

@jdavid jdavid closed this Nov 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@eldavido @carlosmn @jdavid