-
-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normalize error handling for Repository_lookup_branch #700
Conversation
The first commit should be reverted so we don't break backwards compatibility, then the second commit will need to be updated. Maybe do a new single commit to make the PR easier to read. Iterating over a dict returns the keys not the values, we should do the same. While we implement the mapping interface, it doesn't need to be identical to a dict. For instance the fact that |
Sorry, I posted some comments to the fist commit, then saw you have addressed them in the second commit, so I have removed those comments. |
Maybe the implementation can be simplified if replacing If you can make a single commit it will be easier to review. Thanks! |
Fixed the last comment, squashed the commits into one, and also copied the branch test cases & converted them to the new API. |
branch = self._repository.lookup_branch(name, GIT_BRANCH_LOCAL) | ||
|
||
if branch is None and self._flag & GIT_BRANCH_REMOTE: | ||
branch = self._repository.lookup_branch(name, GIT_BRANCH_REMOTE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be reduced to one line:
branch = self._repository.lookup_branch(name, self._flag)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work. lookup_branch
can take GIT_BRANCH_REMOTE
or nothing.
See:
https://github.com/libgit2/libgit2/blob/master/src/branch.c#L353
https://github.com/libgit2/libgit2/blob/master/src/branch.c#L31
So as far as I can tell, this is needed.
pygit2/repository.py
Outdated
self._repository = repository | ||
self._flag = flag | ||
|
||
if flag == GIT_BRANCH_LOCAL | GIT_BRANCH_REMOTE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can replace GIT_BRANCH_LOCAL | GIT_BRANCH_REMOTE
by GIT_BRANCH_ALL
, though first this constant needs to be added to pygit2.
Good work, it is almost done 😃 Could you update the documentation too? There should be no mention of the old API in the documentation. Thanks! |
Sure. Should I document in the code as well? Any guidelines for that? |
I think the code is clear enough. |
I couldn't get the docs to build yet (also, the |
Merged, thanks! |
0.26.0 (2017-07-06) ------------------------- - Update to libgit2 v0.26 `#713 <https://github.com/libgit2/pygit2/pull/713>`_ - Drop support for Python 3.2, add support for cffi 1.10 `#706 <https://github.com/libgit2/pygit2/pull/706>`_ `#694 <https://github.com/libgit2/pygit2/issues/694>`_ - New ``Repository.revert_commit(...)`` `#711 <https://github.com/libgit2/pygit2/pull/711>`_ `#710 <https://github.com/libgit2/pygit2/issues/710>`_ - New ``Branch.is_checked_out()`` `#696 <https://github.com/libgit2/pygit2/pull/696>`_ - Various fixes `#706 <https://github.com/libgit2/pygit2/pull/706>`_ `#707 <https://github.com/libgit2/pygit2/pull/707>`_ `#708 <https://github.com/libgit2/pygit2/pull/708>`_ 0.25.1 (2017-04-25) ------------------------- - Add suport for Python 3.6 - New support for stash: repository methods ``stash``, ``stash_apply``, ``stash_drop`` and ``stash_pop`` `#695 <https://github.com/libgit2/pygit2/pull/695>`_ - Improved support for submodules: new repository methods ``init_submodules`` and ``update_submodules`` `#692 <https://github.com/libgit2/pygit2/pull/692>`_ - New friendlier API for branches & references: ``Repository.branches`` and ``Repository.references`` `#700 <https://github.com/libgit2/pygit2/pull/700>`_ `#701 <https://github.com/libgit2/pygit2/pull/701>`_ - New support for custom backends `#690 <https://github.com/libgit2/pygit2/pull/690>`_ - Fix ``init_repository`` crash on None input `#688 <https://github.com/libgit2/pygit2/issues/688>`_ `#697 <https://github.com/libgit2/pygit2/pull/697>`_ - Fix checkout with an orphan master branch `#669 <https://github.com/libgit2/pygit2/issues/669>`_ `#685 <https://github.com/libgit2/pygit2/pull/685>`_ - Better error messages for opening repositories `#645 <https://github.com/libgit2/pygit2/issues/645>`_ `#698 <https://github.com/libgit2/pygit2/pull/698>`_
Closes #681
I'm not sure that there isn't other code that needs to be changed around there.