-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Address issue #4675: fix Git.check_version() #4676
Merged
xavfernandez
merged 8 commits into
pypa:master
from
cjerdonek:issue-4675-fix-check-version
Oct 5, 2017
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
4c97794
Add a failing test case to test_git_check_version().
cjerdonek 3e7ce53
Fix test expectations (the tests now fail).
cjerdonek 3e4f808
Fix check_version() (the tests now pass).
cjerdonek d1b9441
Change check_version() to accept a commit hash / id.
cjerdonek d646aac
Choose a better name, and other clean-ups.
cjerdonek b0d1160
Add news entry.
cjerdonek da30b70
Address @pradyunsg's review comment.
cjerdonek cf23b06
Add a note to the pip-install docs.
cjerdonek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Fix an issue where ``pip install -e`` on a Git url would fail to update if | ||
a branch or tag name is specified that happens to match the prefix of the | ||
current ``HEAD`` commit hash. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the fact that
2b466712a067a171e8d5a0fe6210e004cfe103a8
won't be equal to2b466712a
.Would
self.get_revision(dest) == self.run_command(['rev-parse',name], show_stdout=False, cwd=location
work ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking some more I guess the point of this PR is that
2b466712a
could also be the name of a branch so my remark is moot.If one wants to avoid an useless update, one should specify the whole commit hash. An additionnal note in https://pip.pypa.io/en/stable/reference/pip_install/#git might be welcome :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the thing you're checking is user-supplied, and requiring people to provide a full hash is pretty user-unfriendly. We should match git's behaviour (which is to allow prefixes as long as they don't match a branch or tag, as I understand it).
Disclaimer: I've never used this feature, and don't anticipate a need to. So if people who actually use it are happy with requiring a full hash, then their opinions should override mine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Replying to @xavfernandez] Right. For example, if you look at the original docstring, you'll see that the function isn't ever supposed to return True for branches or tags. A more realistic example might be an integer tag like "10". An accidental SHA prefix match could cause the tag not to be installed.
Specifying a SHA prefix should still work to install a SHA. It just won't incorrectly "shadow" a branch or tag, which is what this PR addresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pfmoore Providing a partial SHA still works. The user isn't required to provide a full hash. This PR just addresses an optimization code path. The optimization was implemented not quite correctly. It was implemented so that if a user specified a tag that accidentally matches the beginning of a SHA, the tag wouldn't get installed (because the optimization bypasses fetching new tags).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pfmoore specifying a short hash will still work but pip will update the source in this case instead of assuming equality.
Cf https://gist.github.com/xavfernandez/47ac526e0c6bd0ada872deba2c3c2c05 for the difference between pip 9.0.1 and this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to do those examples, @xavfernandez.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, if @pfmoore agrees I'd say the only thing missing before merging would be a small note in https://github.com/pypa/pip/blob/master/docs/reference/pip_install.rst#git if it's not too much to ask :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, would be happy to do that. I'll see if I can do that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying, yes I'm happy for this to go in with the note as requested.