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

Repository: make use of peel for diff() #434

Merged
merged 3 commits into from
Oct 10, 2014

Conversation

carlosmn
Copy link
Member

@carlosmn carlosmn commented Oct 7, 2014

Instead of trying to reimplement parts of it, make use of Object.peel()
to get to a Tree, with short-circuit returns for None and Blob which can
be handled differently.

This makes both objects and references peelable via the same interface,
simplifying how to get to the wanted type.
Instead of trying to reimplement parts of it, make use of Object.peel()
and Reference.peel() to get to a Blob or Tree.
@carlosmn
Copy link
Member Author

carlosmn commented Oct 7, 2014

Rebased to work on top of @jdavid 's refactor/fix. The first makes sure we have a "peelable" interface for both references and objects, so we can get to objects regardless of the input.

The second one then makes use of that in order to peel whatever non-string we got, and forcing the string to be a valid revspec, which we then continue trying to peel (so we can still get to "HEAD~"'s tree). I think the current version would not work well if there are annotated tags involved.

@carlosmn
Copy link
Member Author

carlosmn commented Oct 7, 2014

BTW, since it's mildly related, I was also playing around with moving diff() out of Repository. There is are a couple of cases where the repo object is needed (revparse, and diffing to worktree or index). No other case (and this includes once we've run revparse_single()) needs the repository at all, as the diffs actually happen between the objects themselves (and in general there's no need for the objects to belong to the same repo, which makes the need to choose a particular repository in which to call the method rather an arbitrary restriction. We can even run diffs against arbitrary pieces of text (though it's not wrapped yet)

So I was thinking of having pygit2.diff() which takes objects and keep Repository.diff() for the cases where do want to use the index/worktree or run a revparse query, which would then fall back to the general diff, which would build on top of this.

@jdavid
Copy link
Member

jdavid commented Oct 8, 2014

There is already Reference.get_object() that wraps git_reference_peel, we could do with just one wrapper. And for coherence with Object.peel(...) it should be named Reference.peel(...), so Reference.get_object() would be deprecated.

This means the new Reference.peel() without a parameter should behave like Reference.get_object(), and this one should be deprecated.

@carlosmn
Copy link
Member Author

carlosmn commented Oct 8, 2014

Yeah, this makes sense.

@jdavid
Copy link
Member

jdavid commented Oct 8, 2014

I am going to be off-line for the rest of the week, so feel free to merge this.

Let Reference.peel()'s argument be optional, and default to GIT_OBJ_ANY,
which mirrors the behaviour of get_object().

Since we reimplement the get_object() behaviour, change it to call
peel() which makes sure we can keep using the old tests for this aspect.
carlosmn added a commit that referenced this pull request Oct 10, 2014
Repository: make use of peel for diff()
@carlosmn carlosmn merged commit b8efdde into libgit2:master Oct 10, 2014
@carlosmn
Copy link
Member Author

get_object()'s doc now says it's deprecated, so we can remove it in the next large version bump, and meanwhile it just calls peel() with no arguments.

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