Merge branch 'master' into pip609-v2-sbi

docs/html/reference/pip_install.rst

@@ -384,46 +384,52 @@ where ``setup.py`` is not in the root of project, the "subdirectory" component
 is used. The value of the "subdirectory" component should be a path starting
 from the root of the project to where ``setup.py`` is located.
 
-So if your repository layout is:
+If your repository layout is::
 
-    - pkg_dir/
+   pkg_dir
+   ├── setup.py  # setup.py for package "pkg"
+   └── some_module.py
+   other_dir
+   └── some_file
+   some_other_file
 
-      - setup.py  # setup.py for package ``pkg``
-      - some_module.py
-    - other_dir/
+Then, to install from this repository, the syntax would be::
 
-      - some_file
-    - some_other_file
-
-You'll need to use ``pip install -e "vcs+protocol://repo_url/#egg=pkg&subdirectory=pkg_dir"``.
+   $ pip install -e "vcs+protocol://repo_url/#egg=pkg&subdirectory=pkg_dir"
 
 
 Git
 ^^^
 
 pip currently supports cloning over ``git``, ``git+http``, ``git+https``,
-``git+ssh``, ``git+git`` and ``git+file``:
+``git+ssh``, ``git+git`` and ``git+file``.
+
+.. warning::
+
+    Note that the use of ``git``, ``git+git``, and ``git+http`` is discouraged.
+    The former two use `the Git Protocol`_, which lacks authentication, and HTTP is
+    insecure due to lack of TLS based encryption.
 
 Here are the supported forms::
 
-    [-e] git://git.example.com/MyProject#egg=MyProject
     [-e] git+http://git.example.com/MyProject#egg=MyProject
     [-e] git+https://git.example.com/MyProject#egg=MyProject
     [-e] git+ssh://git.example.com/MyProject#egg=MyProject
-    [-e] git+git://git.example.com/MyProject#egg=MyProject
     [-e] git+file:///home/user/projects/MyProject#egg=MyProject
 
 Passing a branch name, a commit hash, a tag name or a git ref is possible like so::
 
-    [-e] git://git.example.com/MyProject.git@master#egg=MyProject
-    [-e] git://git.example.com/[email protected]#egg=MyProject
-    [-e] git://git.example.com/MyProject.git@da39a3ee5e6b4b0d3255bfef95601890afd80709#egg=MyProject
-    [-e] git://git.example.com/MyProject.git@refs/pull/123/head#egg=MyProject
+    [-e] git+https://git.example.com/MyProject.git@master#egg=MyProject
+    [-e] git+https://git.example.com/[email protected]#egg=MyProject
+    [-e] git+https://git.example.com/MyProject.git@da39a3ee5e6b4b0d3255bfef95601890afd80709#egg=MyProject
+    [-e] git+https://git.example.com/MyProject.git@refs/pull/123/head#egg=MyProject
 
 When passing a commit hash, specifying a full hash is preferable to a partial
 hash because a full hash allows pip to operate more efficiently (e.g. by
 making fewer network calls).
 
+.. _`the Git Protocol`: https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols
+
 Mercurial
 ^^^^^^^^^
 

news/1983.doc

@@ -0,0 +1,2 @@
+Emphasize that VCS URLs using git, git+git and git+http are insecure due to
+lack of authentication and encryption

news/7402.bugfix

@@ -0,0 +1 @@
+Reject VCS URLs with an empty revision.

news/7699.bugfix

@@ -0,0 +1,2 @@
+Use better mechanism for handling temporary files, when recording metadata
+about installed files (RECORD) and the installer (INSTALLER).

src/pip/_internal/operations/install/wheel.py

@@ -28,6 +28,7 @@
 from pip._internal.exceptions import InstallationError
 from pip._internal.locations import get_major_minor_version
 from pip._internal.models.direct_url import DIRECT_URL_METADATA_NAME, DirectUrl
+from pip._internal.utils.filesystem import adjacent_tmp_file, replace
 from pip._internal.utils.misc import captured_stdout, ensure_dir, hash_file
 from pip._internal.utils.temp_dir import TempDirectory
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
@@ -37,7 +38,7 @@
 if MYPY_CHECK_RUNNING:
     from email.message import Message
     from typing import (
-        Dict, List, Optional, Sequence, Tuple, IO, Text, Any,
+        Dict, List, Optional, Sequence, Tuple, Any,
         Iterable, Callable, Set,
     )
 
@@ -65,15 +66,15 @@ def rehash(path, blocksize=1 << 20):
     return (digest, str(length))  # type: ignore
 
 
-def open_for_csv(name, mode):
-    # type: (str, Text) -> IO[Any]
-    if sys.version_info[0] < 3:
-        nl = {}  # type: Dict[str, Any]
-        bin = 'b'
+def csv_io_kwargs(mode):
+    # type: (str) -> Dict[str, Any]
+    """Return keyword arguments to properly open a CSV file
+    in the given mode.
+    """
+    if sys.version_info.major < 3:
+        return {'mode': '{}b'.format(mode)}
     else:
-        nl = {'newline': ''}  # type: Dict[str, Any]
-        bin = ''
-    return open(name, mode + bin, **nl)
+        return {'mode': mode, 'newline': ''}
 
 
 def fix_script(path):
@@ -565,12 +566,11 @@ def is_entrypoint_wrapper(name):
             logger.warning(msg)
 
     # Record pip as the installer
-    installer = os.path.join(dest_info_dir, 'INSTALLER')
-    temp_installer = os.path.join(dest_info_dir, 'INSTALLER.pip')
-    with open(temp_installer, 'wb') as installer_file:
+    installer_path = os.path.join(dest_info_dir, 'INSTALLER')
+    with adjacent_tmp_file(installer_path) as installer_file:
         installer_file.write(b'pip\n')
-    shutil.move(temp_installer, installer)
-    generated.append(installer)
+    replace(installer_file.name, installer_path)
+    generated.append(installer_path)
 
     # Record the PEP 610 direct URL reference
     if direct_url is not None:
@@ -584,20 +584,18 @@ def is_entrypoint_wrapper(name):
         generated.append(direct_url_path)
 
     # Record details of all files installed
-    record = os.path.join(dest_info_dir, 'RECORD')
-    temp_record = os.path.join(dest_info_dir, 'RECORD.pip')
-    with open_for_csv(record, 'r') as record_in:
-        with open_for_csv(temp_record, 'w+') as record_out:
-            reader = csv.reader(record_in)
-            outrows = get_csv_rows_for_installed(
-                reader, installed=installed, changed=changed,
-                generated=generated, lib_dir=lib_dir,
-            )
-            writer = csv.writer(record_out)
-            # Sort to simplify testing.
-            for row in sorted_outrows(outrows):
-                writer.writerow(row)
-    shutil.move(temp_record, record)
+    record_path = os.path.join(dest_info_dir, 'RECORD')
+    with open(record_path, **csv_io_kwargs('r')) as record_file:
+        rows = get_csv_rows_for_installed(
+            csv.reader(record_file),
+            installed=installed,
+            changed=changed,
+            generated=generated,
+            lib_dir=lib_dir)
+    with adjacent_tmp_file(record_path, **csv_io_kwargs('w')) as record_file:
+        writer = csv.writer(record_file)
+        writer.writerows(sorted_outrows(rows))  # sort to simplify testing
+    replace(record_file.name, record_path)
 
 
 def install_wheel(

src/pip/_internal/utils/filesystem.py

@@ -17,7 +17,7 @@
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING, cast
 
 if MYPY_CHECK_RUNNING:
-    from typing import BinaryIO, Iterator
+    from typing import Any, BinaryIO, Iterator
 
     class NamedTemporaryFileResult(BinaryIO):
         @property
@@ -85,16 +85,22 @@ def is_socket(path):
 
 
 @contextmanager
-def adjacent_tmp_file(path):
-    # type: (str) -> Iterator[NamedTemporaryFileResult]
-    """Given a path to a file, open a temp file next to it securely and ensure
-    it is written to disk after the context reaches its end.
+def adjacent_tmp_file(path, **kwargs):
+    # type: (str, **Any) -> Iterator[NamedTemporaryFileResult]
+    """Return a file-like object pointing to a tmp file next to path.
+
+    The file is created securely and is ensured to be written to disk
+    after the context reaches its end.
+
+    kwargs will be passed to tempfile.NamedTemporaryFile to control
+    the way the temporary file will be opened.
     """
     with NamedTemporaryFile(
         delete=False,
         dir=os.path.dirname(path),
         prefix=os.path.basename(path),
         suffix='.tmp',
+        **kwargs
     ) as f:
         result = cast('NamedTemporaryFileResult', f)
         try:

src/pip/_internal/vcs/versioncontrol.py

@@ -11,7 +11,7 @@
 from pip._vendor import pkg_resources
 from pip._vendor.six.moves.urllib import parse as urllib_parse
 
-from pip._internal.exceptions import BadCommand
+from pip._internal.exceptions import BadCommand, InstallationError
 from pip._internal.utils.compat import samefile
 from pip._internal.utils.misc import (
     ask_path_exists,
@@ -436,6 +436,12 @@ def get_url_rev_and_auth(cls, url):
         rev = None
         if '@' in path:
             path, rev = path.rsplit('@', 1)
+            if not rev:
+                raise InstallationError(
+                    "The URL {!r} has an empty revision (after @) "
+                    "which is not supported. Include a revision after @ "
+                    "or remove @ from the URL.".format(url)
+                )
         url = urllib_parse.urlunsplit((scheme, netloc, path, query, ''))
         return url, rev, user_pass
 

tests/functional/test_yaml.py

@@ -1,15 +1,16 @@
-"""Tests for the resolver
+"""
+Tests for the resolver
 """
 
 import os
 import re
 
 import pytest
+import yaml
 
 from tests.lib import DATA_DIR, create_basic_wheel_for_package, path_to_url
-from tests.lib.yaml_helpers import generate_yaml_tests, id_func
 
-_conflict_finder_re = re.compile(
+_conflict_finder_pat = re.compile(
     # Conflicting Requirements: \
     # A 1.0.0 requires B == 2.0.0, C 1.0.0 requires B == 1.0.0.
     r"""
@@ -24,7 +25,49 @@
 )
 
 
-def _convert_to_dict(string):
+def generate_yaml_tests(directory):
+    """
+    Generate yaml test cases from the yaml files in the given directory
+    """
+    for yml_file in directory.glob("*/*.yml"):
+        data = yaml.safe_load(yml_file.read_text())
+        assert "cases" in data, "A fixture needs cases to be used in testing"
+
+        # Strip the parts of the directory to only get a name without
+        # extension and resolver directory
+        base_name = str(yml_file)[len(str(directory)) + 1:-4]
+
+        base = data.get("base", {})
+        cases = data["cases"]
+
+        for i, case_template in enumerate(cases):
+            case = base.copy()
+            case.update(case_template)
+
+            case[":name:"] = base_name
+            if len(cases) > 1:
+                case[":name:"] += "-" + str(i)
+
+            if case.pop("skip", False):
+                case = pytest.param(case, marks=pytest.mark.xfail)
+
+            yield case
+
+
+def id_func(param):
+    """
+    Give a nice parameter name to the generated function parameters
+    """
+    if isinstance(param, dict) and ":name:" in param:
+        return param[":name:"]
+
+    retval = str(param)
+    if len(retval) > 25:
+        retval = retval[:20] + "..." + retval[-2:]
+    return retval
+
+
+def convert_to_dict(string):
 
     def stripping_split(my_str, splitwith, count=None):
         if count is None:
@@ -89,7 +132,7 @@ def handle_install_request(script, requirement):
         message = result.stderr.rsplit("\n", 1)[-1]
 
         # XXX: There might be a better way than parsing the message
-        for match in re.finditer(message, _conflict_finder_re):
+        for match in re.finditer(message, _conflict_finder_pat):
             di = match.groupdict()
             retval["conflicting"].append(
                 {
@@ -119,7 +162,7 @@ def test_yaml_based(script, case):
     # XXX: This doesn't work because this isn't making an index of files.
     for package in available:
         if isinstance(package, str):
-            package = _convert_to_dict(package)
+            package = convert_to_dict(package)
 
         assert isinstance(package, dict), "Needs to be a dictionary"
 

tests/unit/test_vcs.py

@@ -5,7 +5,7 @@
 from mock import patch
 from pip._vendor.packaging.version import parse as parse_version
 
-from pip._internal.exceptions import BadCommand
+from pip._internal.exceptions import BadCommand, InstallationError
 from pip._internal.utils.misc import hide_url, hide_value
 from pip._internal.vcs import make_vcs_requirement_url
 from pip._internal.vcs.bazaar import Bazaar
@@ -292,6 +292,21 @@ def test_version_control__get_url_rev_and_auth__missing_plus(url):
     assert 'malformed VCS url' in str(excinfo.value)
 
 
+@pytest.mark.parametrize('url', [
+    # Test a URL with revision part as empty.
+    'git+https://github.com/MyUser/myProject.git@#egg=py_pkg',
+])
+def test_version_control__get_url_rev_and_auth__no_revision(url):
+    """
+    Test passing a URL to VersionControl.get_url_rev_and_auth() with
+    empty revision
+    """
+    with pytest.raises(InstallationError) as excinfo:
+        VersionControl.get_url_rev_and_auth(url)
+
+    assert 'an empty revision (after @)' in str(excinfo.value)
+
+
 @pytest.mark.parametrize('url, expected', [
     # Test http.
     ('bzr+http://bzr.myproject.org/MyProject/trunk/#egg=MyProject',

tests/unit/test_wheel.py

@@ -146,7 +146,7 @@ def call_get_csv_rows_for_installed(tmpdir, text):
     generated = []
     lib_dir = '/lib/dir'
 
-    with wheel.open_for_csv(path, 'r') as f:
+    with open(path, **wheel.csv_io_kwargs('r')) as f:
         reader = csv.reader(f)
         outrows = wheel.get_csv_rows_for_installed(
             reader, installed=installed, changed=changed,