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 static Diff.parse_diff #774

Merged
merged 2 commits into from
Mar 5, 2018
Merged

Add static Diff.parse_diff #774

merged 2 commits into from
Mar 5, 2018

Conversation

brandonio21
Copy link
Contributor

I think this PR is a little less straight forward than #772

I think the user should be able to create a Diff object from a unified diff without needing a repository, as in git_diff_from_buffer. The existing pygit2 API, however, requires that a Repository be provided when creating a Diff object.

This PR relaxes that requirement and allows NULL to be provided for the Repository when creating a Diff object. This allows Diffs to be created without repos.

If this is a good idea, let's merge this in! If not, let's have a discussion. I'm also open to the idea of having an optional kwarg repository= so that we can reuse this code in Repository.parse_diff

@jdavid
Copy link
Member

jdavid commented Mar 4, 2018

Diff_merge would need to be updated.

Does it have any sense for the diff object to be attached to the repository? I only found Diff_merge and it doesn't look to do anything useful with the repo. So I would just remove the repo from the Diff type (then Repository.parse_diff would go away as well).

@brandonio21
Copy link
Contributor Author

To me, it doesn't make much sense. (However, libgit2's git_diff struct does indeed have a pointer to a repo).

I think removing the repo from the Diff type is a solid idea, but seems like a very breaking change.

@jdavid
Copy link
Member

jdavid commented Mar 5, 2018

We don't expose the repo, today diff.repo raises attribute error, so we would not break user code.

But we may still need the reference to the repo to keep the libgit2's repo alive.

@jdavid
Copy link
Member

jdavid commented Mar 5, 2018

Well, I think better keep it the way it is, just to be sure the reference is alive.

But then, why to have 2 functions that do essentially the same? Could we just have Diff.parse_diff and remove Repository.parse_diff ?

@brandonio21
Copy link
Contributor Author

Oh! I was not aware of the AttributeError. In that case, I am happy to remove Repository.parse_diff. An update to this PR incoming...

@jdavid jdavid merged commit 2d4c1f5 into libgit2:master Mar 5, 2018
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.

2 participants