Skip to content

Add support for diffing against root commit #408

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

Merged
merged 12 commits into from
Apr 19, 2016
Merged

Add support for diffing against root commit #408

merged 12 commits into from
Apr 19, 2016

Conversation

nvie
Copy link
Contributor

@nvie nvie commented Apr 12, 2016

This adds support for diff'ing against the initial commit (or any commit without parents, really). This is something that has classically been a bit hard to do since GitPython does not offer special support for this. (See #364, or http://stackoverflow.com/questions/33916648/get-the-diff-details-of-first-commit-in-gitpython)

I've opted to add this as another special case for the first argument to the Commit.diff() call, right next to the currently allowed None, the Index class, the commit SHA, etc. This plays nice since you'd never use this in combination with those existing options.

It enables:

initial_commit.diff(NULL_TREE)

…which will produce a diff much like "git show <initial_commit>" would. Of course, it's possible to use a raw repo.git.diff_tree() call for this, but that would return a raw string, not a nice DiffIndex object. The workaround suggesting to use the magic "empty tree sha" also does not work, as the diff output is then in the "reverse direction".

Let me know what you think of this! Test cases included.

@Byron Byron added this to the v1.0.3 - Fixes milestone Apr 14, 2016
return s
elif isinstance(s, six.binary_type):
if PRE_PY27:
return s.decode(defenc) # we're screwed
Copy link
Member

Choose a reason for hiding this comment

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

As I have never had too many contributors, there is no guide yet. However, would you consider removing this comment , possibly in favour of explaining why python prior to py2.6 is a problem ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right here, this is inappropriate. Fixed in aae2a73.

@@ -8,6 +8,7 @@
# flake8: noqa

import sys
import six
Copy link
Member

Choose a reason for hiding this comment

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

I think I looked very carefully, but couldn't find a new dependency. Is six available in Py2.7 and onwards by default ?

@Byron
Copy link
Member

Byron commented Apr 14, 2016

@nvie This is a nice one ! Thank so much for re-viving GitPython with your contributions.

As you can see, I have left a few notes on the code itself, and am looking forward to what you think !
Besides I very much like the blame_incremental feature you added, thank you !

Something you could consider is to add new features to the the changes.rst, that way you assure they are not forgotten when I at some point make a release. Speaking of which: If you for some reason should need one, please let me know. As the process is cumbersome, I am very much avoiding it unless there is a major bugfix to be distributed.

Last but definitely not least: Using the six library is totally fine, yet I hope it doesn't do anything in the background that could affect GitPython's overall performance. There is quite a vivid memory of me using the performance tests to get python 3 support without slowing things down to a crawl - at the end of this process just a few tweaks were needed and I was quite happy to not have introduced another dependency.

@nvie
Copy link
Contributor Author

nvie commented Apr 14, 2016

(I've reverted my mistake and force-pushed now to be a clean change again. Sorry for mixing this one up with the other feature.)

@nvie
Copy link
Contributor Author

nvie commented Apr 14, 2016

Something you could consider is to add new features to the the changes.rst, that way you assure they are not forgotten when I at some point make a release.

Will do!

Speaking of which: If you for some reason should need one, please let me know. As the process is cumbersome, I am very much avoiding it unless there is a major bugfix to be distributed.

I can understand. Once everything is merged, I'll point our version to the latest master and make sure everything is stable. Then we can roll an update.

Last but definitely not least: Using the six library is totally fine, yet I hope it doesn't do anything in the background that could affect GitPython's overall performance. There is quite a vivid memory of me using the performance tests to get python 3 support without slowing things down to a crawl - at the end of this process just a few tweaks were needed and I was quite happy to not have introduced another dependency.

I'm using six here mostly to get a convenient import target for things like string_type, binary_type, and range. I don't think six itself is a slowdown (it only provides a stable import target for things that have been moved around between Python versions AFAIK), but perhaps I'm missing something.

I'll address your feedback on this branch now.

@nvie
Copy link
Contributor Author

nvie commented Apr 14, 2016

I've rebased this work against the latest master and added commits to address your feedback. Let me know what you think!

@nvie
Copy link
Contributor Author

nvie commented Apr 14, 2016

OK, I think this is done now, test cases fixed, and dependency on six dropped.

return s.decode(defenc) # we're screwed
# Python 2.6 does not support the `errors` argument, so we cannot
# control the replacement of unsafe chars in it.
return s.decode(defenc)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, @Byron, do you still want to keep supporting Python 2.6? This message is echoed when you run the Python 2.6 tests already:

py26 installed: DEPRECATION: Python 2.6 is no longer supported by the Python core team, please upgrade your Python. A future version of pip will drop support for Python 2.6

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for letting me know - under these circumstances it will be fine to drop support !

Copy link
Member

Choose a reason for hiding this comment

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

I just removed py2.6 support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheers! 👍

Copy link
Member

Choose a reason for hiding this comment

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

BTW: I was about to merge a few PRs, but work is ...catching up earlier than expected. Will try again tonight, and if these delays become unbearable to you, I'd love to make you a collaborator. Just let me know what you think.
Cheers :) !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I've been there. Maintaining OSS libraries while also getting work done is a hard balance to strike. On the plus side, patching/maintaining this project is important for our team now, and we're committed to only invest in it more. So if you'd like to have a contributor on board, I'd be more than happy to help out there!

@nvie nvie mentioned this pull request Apr 14, 2016
@nvie nvie merged commit 4adafc5 into gitpython-developers:master Apr 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants