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.

Sign up for GitHub

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

Jump to bottom

Address issue #4675: fix Git.check_version() #4676

Merged
merged 8 commits into from
Oct 5, 2017
Merged

Address issue #4675: fix Git.check_version() #4676

merged 8 commits into from
Oct 5, 2017

Conversation

cjerdonek
Copy link
Member

This addresses issue #4675.

@cjerdonek cjerdonek mentioned this pull request Aug 14, 2017
Closed
@cjerdonek cjerdonek changed the title Address issue #4675: fix Git.check_version() [WIP] Address issue #4675: fix Git.check_version() Aug 14, 2017
@pradyunsg
Copy link
Member

Is this still a WIP?

@cjerdonek
Copy link
Member Author

Thanks, yes. I noticed something I want to fix before having this considered. I'll remove "WIP" when ready.

@pradyunsg pradyunsg added type: enhancement Improvements to functionality C: vcs pip's interaction with version control systems like git, svn and bzr labels Aug 20, 2017
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 1, 2017
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Oct 3, 2017
@cjerdonek cjerdonek changed the title [WIP] Address issue #4675: fix Git.check_version() Address issue #4675: fix Git.check_version() Oct 3, 2017
@cjerdonek
Copy link
Member Author

Since PR #4707 was merged, I was able to finish working on this. So I removed the "WIP" prefix.

pradyunsg
pradyunsg reviewed Oct 3, 2017
View reviewed changes
tests/unit/test_vcs.py Outdated
assert git.check_version('foo', rev_options) is result
def test_git_does_commit_id_equal(git, rev_name, result):
"""
Test Git.does_commit_id_equal().
Copy link
Member

Choose a reason for hiding this comment

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

🚲shed: is_commit_id_equal

@cjerdonek
Copy link
Member Author

Thanks, @pradyunsg! Updated. :)

xavfernandez
xavfernandez reviewed Oct 4, 2017
View reviewed changes
src/pip/_internal/vcs/git.py
# Then avoid an unnecessary subprocess call.
return False

return self.get_revision(dest) == name
Copy link
Member

@xavfernandez xavfernandez Oct 4, 2017 •
edited
Loading

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 to 2b466712a.

Would self.get_revision(dest) == self.run_command(['rev-parse',name], show_stdout=False, cwd=location work ?

Copy link
Member

@xavfernandez xavfernandez Oct 5, 2017 •
edited
Loading

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 :)

Copy link
Member

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.

Copy link
Member Author

@cjerdonek cjerdonek Oct 5, 2017 •
edited
Loading

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.

Copy link
Member Author

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).

Copy link
Member

@xavfernandez xavfernandez Oct 5, 2017 •
edited
Loading

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.

Copy link
Member Author

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.

Copy link
Member

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 :)

Copy link
Member Author

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.

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 clarifying, yes I'm happy for this to go in with the note as requested.

@cjerdonek
Copy link
Member Author

Okay, I just rebased and added the note to the docs as requested. Let me know if you want the wording to be adjusted at all.

@xavfernandez xavfernandez merged commit 4092707 into pypa:master Oct 5, 2017
@xavfernandez
Copy link
Member

Thanks 👍 ✨

@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xavfernandez xavfernandez xavfernandez left review comments

@pradyunsg pradyunsg pradyunsg approved these changes

@pfmoore pfmoore pfmoore approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation C: vcs pip's interaction with version control systems like git, svn and bzr type: enhancement Improvements to functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cjerdonek @pradyunsg @BrownTruck @xavfernandez @pfmoore @pypa-bot