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 RemoteCollection.names() iterator #1159

Merged
merged 1 commit into from
Oct 31, 2022
Merged

Conversation

agausmann
Copy link
Contributor

@agausmann agausmann commented Sep 25, 2022

Motivation

This names() iterator provides a big benefit in performance in my use case, where I want to test whether or not a remote exists for a large number of potential remote names (hundreds)

The first code I used looked something like:

try:
    repo.remotes[remote_name]
except KeyError:
    repo.remotes.create(remote_name, remote_url)

However, this performs a lookup for all remote names, which was prohibitively expensive. So I decided to try collecting all the names into a set beforehand:

existing_remotes = set(remote.name for remote in repo.remotes)

for remote_name, remote_url in wanted_remotes:
    if remote_name not in existing_remotes:
        repo.remotes.create(remote_name, remote_url)

However, this still performs a lookup for each remote to create the Remote objects yielded by the iterator.

With RemoteCollection.names(), I can construct the set much more efficiently:

existing_remotes = set(repo.remotes.names())

Prior to this change, the only way to get names of all
remotes via pygit2 was to use the `__iter__` API, like

    (remote.name for remote in repo.remotes)

which internally performs a remote lookup for each
remote. This has suboptimal performance that is
especially noticeable in repositories with large
numbers of remotes.
@jdavid
Copy link
Member

jdavid commented Sep 26, 2022

Thanks, please add a unit test.

@jdavid
Copy link
Member

jdavid commented Sep 26, 2022

For reference I think iteration should yield the names, like references/branches do, but that breaks compatibility.
Also I'm unsure which is best names() or keys(), names is more like git, keys is more like python.

@jdavid jdavid merged commit feda946 into libgit2:master Oct 31, 2022
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