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

Add warning for git version parse failures #10446

Closed
wants to merge 1 commit into from
Closed

Add warning for git version parse failures #10446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/10361.enhancement.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a warning upon failing to match (via regex) the current system's git version.
1 change: 1 addition & 0 deletions src/pip/_internal/vcs/git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ def get_git_version(self) -> Tuple[int, ...]:
version = self.run_command(["version"], show_stdout=False, stdout_only=True)
match = GIT_VERSION_REGEX.match(version)
if not match:
logger.warning("Unable to parse '%s'.", version)
Copy link
Member

@uranusjr uranusjr Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It may also be worth while to explain where we got that unparsable string, i.e.

Suggested change
logger.warning("Unable to parse '%s'.", version)
logger.warning("Unable to parse 'git version' output: %r.", version)

Copy link
Author

@HausCloud HausCloud Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean something like the below?
"""
Equivalent of running 'git --version'
See run_command in parent class
"""

Copy link
Member

Choose a reason for hiding this comment

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

I don’t quite understand what you meant. What I was trying to express is, this Unable to parse message will be seen by the user in the console, and when that happens the user may not fully understand the context of the message (why does the string fail to parse? Where does that string come from?) Therefore, I recommended adding git version output to the message, so the user understands the string is the output to git version and react accordingly (and yes, git --version gives the exact same output, but we are using git version right now so that should be used instead).

Copy link
Author

@HausCloud HausCloud Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ah okay. I misunderstood, my apologies.

EDIT: What I can I do now to get this moving along?

return ()
return tuple(int(c) for c in match.groups())

Expand Down
19 changes: 19 additions & 0 deletions tests/unit/test_vcs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,25 @@ def test_git_is_commit_id_equal(mock_get_revision, rev_name, result):
assert Git.is_commit_id_equal("/path", rev_name) is result


@pytest.mark.parametrize(
"version_out,result",
(
("git version -2.25.1", ()),
("git version 2.a.1", ()),
("git ver. 2.25.1", ()),
),
)
uranusjr marked this conversation as resolved.
Show resolved Hide resolved
@patch("pip._internal.vcs.versioncontrol.VersionControl.run_command")
def test_git_parse_fail_warning(mock_run_command, caplog, version_out, result):
"""
Test Git.get_git_version().
"""
mock_run_command.return_value = version_out
git_tuple = Git().get_git_version()
assert git_tuple == result
assert caplog.messages == ["Unable to parse '%s'." % version_out]
uranusjr marked this conversation as resolved.
Show resolved Hide resolved


# The non-SVN backends all use the same get_netloc_and_auth(), so only test
# Git as a representative.
@pytest.mark.parametrize(
Expand Down