Skip to content
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

Add support for git_graph_descendant_of #816

Merged
merged 5 commits into from
Sep 15, 2018
Merged

Conversation

chadrik
Copy link
Contributor

@chadrik chadrik commented Sep 6, 2018

Fixes issue #815

Let me know if this should be added to the docs, and if so, which section.

Added as Repository.descendant_of
@chadrik
Copy link
Contributor Author

chadrik commented Sep 6, 2018

Also, do you think it's worth adding a feature like this:

branches = repo.branches.with_commit(oid)
for branch in branches:
    print(branch)
print('master' in branches)

I would achieve this with a new __init__ argument to the Branches class that would constrain the results of __getitem__ and __iter__ to branches that contained the passed commit id.

@jdavid
Copy link
Member

jdavid commented Sep 7, 2018

Yes please add to the docs, just in docs/repository.rst

@jdavid
Copy link
Member

jdavid commented Sep 7, 2018

Yes that sounds good, but would not be better branches.contains(...)? to mimic git branch --contains

@chadrik
Copy link
Contributor Author

chadrik commented Sep 7, 2018

I added the docs. Let me know if you had something more in mind.

Yes that sounds good, but would not be better branches.contains(...)? to mimic git branch --contains

That was my original thought, but I felt that branches.contains() feels like a method that should return a boolean (do the branches contain something, yes or no?), whereas branches.with_commit() seems to better indicate that it will return more branches. Also, it makes it more clear that it has something to do with commits. Ultimately, it's your call, though. Let me know what you think.

@@ -1159,14 +1160,23 @@ def get(self, key):

def __iter__(self):
for branch_name in self._repository.listall_branches(self._flag):
yield branch_name
if self._valid(self[branch_name]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather call self._valid(branch_name) to avoid a lookup in the case that self._commit is None, which is probably the most common case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but I don't want to incur the lookup in the case of __getitem__ where we already have a branch reference object. Check my latest commit for what I think is a good compromise.

@jdavid
Copy link
Member

jdavid commented Sep 7, 2018

Sounds good, true with_commit is more intuitive.

Could you add a test for with_commit? See the review as well.

Thanks!

@chadrik
Copy link
Contributor Author

chadrik commented Sep 7, 2018

Could you add a test for with_commit? See the review as well.

Yep, just wanted to get sign off on the general design first.

@chadrik
Copy link
Contributor Author

chadrik commented Sep 14, 2018

Updated! Let me know what you think.

@jdavid jdavid merged commit 2b852c7 into libgit2:master Sep 15, 2018
@jdavid
Copy link
Member

jdavid commented Sep 15, 2018

Merged, thanks!

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Dec 1, 2018
0.27.2 (2018-09-16)
-------------------------

- Add support for Python 3.7
  `#809 <https://github.com/libgit2/pygit2/issues/809>`_

- New ``Object.short_id``
  `#799 <https://github.com/libgit2/pygit2/issues/799>`_
  `#806 <https://github.com/libgit2/pygit2/pull/806>`_
  `#807 <https://github.com/libgit2/pygit2/pull/807>`_

- New ``Repository.descendant_of`` and ``Repository.branches.with_commit``
  `#815 <https://github.com/libgit2/pygit2/issues/815>`_
  `#816 <https://github.com/libgit2/pygit2/pull/816>`_

- Fix repository initialization in ``clone_repository(...)``
  `#818 <https://github.com/libgit2/pygit2/issues/818>`_

- Fix several warnings and errors, commits
  `cd896ddc <https://github.com/libgit2/pygit2/commit/cd896ddc>`_
  and
  `dfa536a3 <https://github.com/libgit2/pygit2/commit/dfa536a3>`_

- Documentation fixes and improvements
  `#805 <https://github.com/libgit2/pygit2/pull/805>`_
  `#808 <https://github.com/libgit2/pygit2/pull/808>`_


0.27.1 (2018-06-02)
-------------------------

Breaking changes:

- Now ``discover_repository`` returns ``None`` if repository not found, instead
  of raising ``KeyError``
  `#531 <https://github.com/libgit2/pygit2/issues/531>`_

Other changes:

- New ``DiffLine.raw_content``
  `#610 <https://github.com/libgit2/pygit2/issues/610>`_

- Fix tests failing in some cases
  `#795 <https://github.com/libgit2/pygit2/issues/795>`_

- Automatize wheels upload to pypi
  `#563 <https://github.com/libgit2/pygit2/issues/563>`_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants