Merge pull request #584 from akaihola/relative-paths

Fix handling of relative paths and running from non-Git-root

CHANGES.rst

@@ -21,6 +21,7 @@ Fixed
   ``--stdout`` option. This makes the option compatible with the
   `new Black extension for VSCode`__.
 - Badge links in the README on GitHub.
+- Handling of relative paths and running from elsewhere than repository root.
 
 __ https://github.com/microsoft/vscode-black-formatter
 

src/darker/__main__.py

@@ -516,10 +516,14 @@ def main(  # pylint: disable=too-many-locals,too-many-branches,too-many-statemen
     if args.skip_magic_trailing_comma is not None:
         black_config["skip_magic_trailing_comma"] = args.skip_magic_trailing_comma
 
-    paths, root = resolve_paths(args.stdin_filename, args.src)
+    paths, common_root = resolve_paths(args.stdin_filename, args.src)
+    # `common_root` is now the common root of given paths,
+    # not necessarily the repository root.
+    # `paths` are the unmodified paths from `--stdin-filename` or `SRC`,
+    # so either relative to the current working directory or absolute paths.
 
     revrange = RevisionRange.parse_with_common_ancestor(
-        args.revision, root, args.stdin_filename is not None
+        args.revision, common_root, args.stdin_filename is not None
     )
     output_mode = OutputMode.from_args(args)
     write_modified_files = not args.check and output_mode == OutputMode.NOTHING
@@ -539,7 +543,7 @@ def main(  # pylint: disable=too-many-locals,too-many-branches,too-many-statemen
             )
 
     if revrange.rev2 != STDIN:
-        missing = get_missing_at_revision(paths, revrange.rev2, root)
+        missing = get_missing_at_revision(paths, revrange.rev2, common_root)
         if missing:
             missing_reprs = " ".join(repr(str(path)) for path in missing)
             rev2_repr = (
@@ -550,9 +554,9 @@ def main(  # pylint: disable=too-many-locals,too-many-branches,too-many-statemen
                 f"Error: Path(s) {missing_reprs} do not exist in {rev2_repr}",
             )
 
-    # These paths are relative to `root`:
-    files_to_process = filter_python_files(paths, root, {})
-    files_to_blacken = filter_python_files(paths, root, black_config)
+    # These paths are relative to `common_root`:
+    files_to_process = filter_python_files(paths, common_root, {})
+    files_to_blacken = filter_python_files(paths, common_root, black_config)
     # Now decide which files to reformat (Black & isort). Note that this doesn't apply
     # to linting.
     if output_mode == OutputMode.CONTENT or revrange.rev2 == STDIN:
@@ -563,11 +567,11 @@ def main(  # pylint: disable=too-many-locals,too-many-branches,too-many-statemen
         black_exclude = set()
     else:
         # In other modes, only reformat files which have been modified.
-        if git_is_repository(root):
+        if git_is_repository(common_root):
             # Get the modified files only.
-            repo_root = find_project_root((str(root),))
+            repo_root = find_project_root((str(common_root),))
             changed_files = {
-                (repo_root / file).relative_to(root)
+                (repo_root / file).relative_to(common_root)
                 for file in git_get_modified_python_files(paths, revrange, repo_root)
             }
             # Filter out changed files that are not supposed to be processed
@@ -584,7 +588,7 @@ def main(  # pylint: disable=too-many-locals,too-many-branches,too-many-statemen
     formatting_failures_on_modified_lines = False
     for path, old, new in sorted(
         format_edited_parts(
-            root,
+            common_root,
             changed_files_to_reformat,
             Exclusions(
                 black=black_exclude,
@@ -602,16 +606,16 @@ def main(  # pylint: disable=too-many-locals,too-many-branches,too-many-statemen
         #     there were any changes to the original
         formatting_failures_on_modified_lines = True
         if output_mode == OutputMode.DIFF:
-            print_diff(path, old, new, root, use_color)
+            print_diff(path, old, new, common_root, use_color)
         elif output_mode == OutputMode.CONTENT:
             print_source(new, use_color)
         if write_modified_files:
             modify_file(path, new)
     linter_failures_on_modified_lines = run_linters(
         args.lint,
-        root,
+        common_root,
         # paths to lint are not limited to modified files or just Python files:
-        {p.resolve().relative_to(root) for p in paths},
+        {p.resolve().relative_to(common_root) for p in paths},
         revrange,
         use_color,
     )

src/darker/git.py

@@ -76,25 +76,33 @@ def should_reformat_file(path: Path) -> bool:
     return path.exists() and get_path_in_repo(path).suffix == ".py"
 
 
-def _git_exists_in_revision(path: Path, rev2: str, cwd: Path) -> bool:
+def _git_exists_in_revision(path: Path, rev2: str, git_cwd: Path) -> bool:
     """Return ``True`` if the given path exists in the given Git revision
 
-    :param path: The path of the file or directory to check
+    :param path: The path of the file or directory to check, either relative to current
+                 working directory or absolute
     :param rev2: The Git revision to look at
-    :param cwd: The Git repository root
+    :param git_cwd: The working directory to use when invoking Git. This has to be
+                    either the root of the working tree, or another directory inside it.
     :return: ``True`` if the file or directory exists at the revision, or ``False`` if
              it doesn't.
 
     """
-    if (cwd / path).resolve() == cwd.resolve():
-        return True
+    while not git_cwd.exists():
+        # The working directory for running Git doesn't exist. Walk up the directory
+        # tree until we find an existing directory. This is necessary because `git
+        # cat-file` doesn't work if the current working directory doesn't exist.
+        git_cwd = git_cwd.parent
+    relative_path = (Path.cwd() / path).relative_to(git_cwd.resolve())
     # Surprise: On Windows, `git cat-file` doesn't work with backslash directory
     # separators in paths. We need to use Posix paths and forward slashes instead.
-    cmd = ["git", "cat-file", "-e", f"{rev2}:{path.as_posix()}"]
-    logger.debug("[%s]$ %s", cwd, " ".join(cmd))
+    # Surprise #2: `git cat-file` assumes paths are relative to the repository root.
+    # We need to prepend `./` to paths relative to the working directory.
+    cmd = ["git", "cat-file", "-e", f"{rev2}:./{relative_path.as_posix()}"]
+    logger.debug("[%s]$ %s", git_cwd, " ".join(cmd))
     result = run(  # nosec
         cmd,
-        cwd=str(cwd),
+        cwd=str(git_cwd),
         check=False,
         stderr=DEVNULL,
         env=make_git_env(),
@@ -108,9 +116,10 @@ def get_missing_at_revision(paths: Iterable[Path], rev2: str, cwd: Path) -> Set[
     In case of ``WORKTREE``, just check if the files exist on the filesystem instead of
     asking Git.
 
-    :param paths: Paths to check
+    :param paths: Paths to check, relative to the current working directory or absolute
     :param rev2: The Git revision to look at, or ``WORKTREE`` for the working tree
-    :param cwd: The Git repository root
+    :param cwd: The working directory to use when invoking Git. This has to be either
+                the root of the working tree, or another directory inside it.
     :return: The set of file or directory paths which are missing in the revision
 
     """
@@ -120,15 +129,15 @@ def get_missing_at_revision(paths: Iterable[Path], rev2: str, cwd: Path) -> Set[
 
 
 def _git_diff_name_only(
-    rev1: str, rev2: str, relative_paths: Iterable[Path], cwd: Path
+    rev1: str, rev2: str, relative_paths: Iterable[Path], repo_root: Path
 ) -> Set[Path]:
     """Collect names of changed files between commits from Git
 
     :param rev1: The old commit to compare to
     :param rev2: The new commit to compare, or the string ``":WORKTREE:"`` to compare
                  current working tree to ``rev1``
-    :param relative_paths: Relative paths to the files to compare
-    :param cwd: The Git repository root
+    :param relative_paths: Relative paths from repository root to the files to compare
+    :param repo_root: The Git repository root
     :return: Relative paths of changed files
 
     """
@@ -143,7 +152,7 @@ def _git_diff_name_only(
     ]
     if rev2 != WORKTREE:
         diff_cmd.insert(diff_cmd.index("--"), rev2)
-    lines = git_check_output_lines(diff_cmd, cwd)
+    lines = git_check_output_lines(diff_cmd, repo_root)
     return {Path(line) for line in lines}
 
 
@@ -153,7 +162,7 @@ def _git_ls_files_others(relative_paths: Iterable[Path], cwd: Path) -> Set[Path]
     This will return those files in ``relative_paths`` which are untracked and not
     excluded by ``.gitignore`` or other Git's exclusion mechanisms.
 
-    :param relative_paths: Relative paths to the files to consider
+    :param relative_paths: Relative paths from repository root to the files to consider
     :param cwd: The Git repository root
     :return: Relative paths of untracked files
 
@@ -170,23 +179,26 @@ def _git_ls_files_others(relative_paths: Iterable[Path], cwd: Path) -> Set[Path]
 
 
 def git_get_modified_python_files(
-    paths: Iterable[Path], revrange: RevisionRange, cwd: Path
+    paths: Iterable[Path], revrange: RevisionRange, repo_root: Path
 ) -> Set[Path]:
     """Ask Git for modified and untracked ``*.py`` files
 
     - ``git diff --name-only --relative <rev> -- <path(s)>``
     - ``git ls-files --others --exclude-standard -- <path(s)>``
 
-    :param paths: Relative paths to the files to diff
+    :param paths: Files to diff, either relative to the current working dir or absolute
     :param revrange: Git revision range to compare
-    :param cwd: The Git repository root
+    :param repo_root: The Git repository root
     :return: File names relative to the Git repository root
 
     """
-    changed_paths = _git_diff_name_only(revrange.rev1, revrange.rev2, paths, cwd)
+    repo_paths = [path.resolve().relative_to(repo_root) for path in paths]
+    changed_paths = _git_diff_name_only(
+        revrange.rev1, revrange.rev2, repo_paths, repo_root
+    )
     if revrange.rev2 == WORKTREE:
-        changed_paths.update(_git_ls_files_others(paths, cwd))
-    return {path for path in changed_paths if should_reformat_file(cwd / path)}
+        changed_paths.update(_git_ls_files_others(repo_paths, repo_root))
+    return {path for path in changed_paths if should_reformat_file(repo_root / path)}
 
 
 def _revision_vs_lines(

src/darker/tests/test_git.py

@@ -69,7 +69,7 @@ def test_git_exists_in_revision_git_call(retval, expect):
         result = git._git_exists_in_revision(Path("path.py"), "rev2", Path("."))
 
     run.assert_called_once_with(
-        ["git", "cat-file", "-e", "rev2:path.py"],
+        ["git", "cat-file", "-e", "rev2:./path.py"],
         cwd=".",
         check=False,
         stderr=DEVNULL,
@@ -79,54 +79,125 @@ def test_git_exists_in_revision_git_call(retval, expect):
 
 
 @pytest.mark.kwparametrize(
-    dict(rev2="{add}", path="dir/a.py", expect=True),
-    dict(rev2="{add}", path="dir/b.py", expect=True),
-    dict(rev2="{add}", path="dir/", expect=True),
-    dict(rev2="{add}", path="dir", expect=True),
-    dict(rev2="{del_a}", path="dir/a.py", expect=False),
-    dict(rev2="{del_a}", path="dir/b.py", expect=True),
-    dict(rev2="{del_a}", path="dir/", expect=True),
-    dict(rev2="{del_a}", path="dir", expect=True),
-    dict(rev2="HEAD", path="dir/a.py", expect=False),
-    dict(rev2="HEAD", path="dir/b.py", expect=False),
-    dict(rev2="HEAD", path="dir/", expect=False),
-    dict(rev2="HEAD", path="dir", expect=False),
+    dict(cwd=".", rev2="{add}", path="x/dir/a.py", expect=True),
+    dict(cwd=".", rev2="{add}", path="x/dir/sub/b.py", expect=True),
+    dict(cwd=".", rev2="{add}", path="x/dir/", expect=True),
+    dict(cwd=".", rev2="{add}", path="x/dir", expect=True),
+    dict(cwd=".", rev2="{add}", path="x/dir/sub", expect=True),
+    dict(cwd=".", rev2="{del_a}", path="x/dir/a.py", expect=False),
+    dict(cwd=".", rev2="{del_a}", path="x/dir/sub/b.py", expect=True),
+    dict(cwd=".", rev2="{del_a}", path="x/dir/", expect=True),
+    dict(cwd=".", rev2="{del_a}", path="x/dir", expect=True),
+    dict(cwd=".", rev2="{del_a}", path="x/dir/sub", expect=True),
+    dict(cwd=".", rev2="HEAD", path="x/dir/a.py", expect=False),
+    dict(cwd=".", rev2="HEAD", path="x/dir/sub/b.py", expect=False),
+    dict(cwd=".", rev2="HEAD", path="x/dir/", expect=False),
+    dict(cwd=".", rev2="HEAD", path="x/dir", expect=False),
+    dict(cwd=".", rev2="HEAD", path="x/dir/sub", expect=False),
+    dict(cwd="x", rev2="{add}", path="dir/a.py", expect=True),
+    dict(cwd="x", rev2="{add}", path="dir/sub/b.py", expect=True),
+    dict(cwd="x", rev2="{add}", path="dir/", expect=True),
+    dict(cwd="x", rev2="{add}", path="dir", expect=True),
+    dict(cwd="x", rev2="{add}", path="dir/sub", expect=True),
+    dict(cwd="x", rev2="{del_a}", path="dir/a.py", expect=False),
+    dict(cwd="x", rev2="{del_a}", path="dir/sub/b.py", expect=True),
+    dict(cwd="x", rev2="{del_a}", path="dir/", expect=True),
+    dict(cwd="x", rev2="{del_a}", path="dir", expect=True),
+    dict(cwd="x", rev2="{del_a}", path="dir/sub", expect=True),
+    dict(cwd="x", rev2="HEAD", path="dir/a.py", expect=False),
+    dict(cwd="x", rev2="HEAD", path="dir/sub/b.py", expect=False),
+    dict(cwd="x", rev2="HEAD", path="dir/", expect=False),
+    dict(cwd="x", rev2="HEAD", path="dir", expect=False),
+    dict(cwd="x", rev2="HEAD", path="dir/sub", expect=False),
 )
-def test_git_exists_in_revision(git_repo, rev2, path, expect):
+def test_git_exists_in_revision(git_repo, monkeypatch, cwd, rev2, path, expect):
     """``_get_exists_in_revision()`` detects file/dir existence correctly"""
-    git_repo.add({"dir/a.py": "", "dir/b.py": ""}, commit="Add dir/*.py")
+    git_repo.add(
+        {"x/README": "", "x/dir/a.py": "", "x/dir/sub/b.py": ""},
+        commit="Add x/dir/*.py",
+    )
     add = git_repo.get_hash()
-    git_repo.add({"dir/a.py": None}, commit="Delete dir/a.py")
+    git_repo.add({"x/dir/a.py": None}, commit="Delete x/dir/a.py")
     del_a = git_repo.get_hash()
-    git_repo.add({"dir/b.py": None}, commit="Delete dir/b.py")
+    git_repo.add({"x/dir/sub/b.py": None}, commit="Delete x/dir/b.py")
+    monkeypatch.chdir(cwd)
 
     result = git._git_exists_in_revision(
-        Path(path), rev2.format(add=add, del_a=del_a), git_repo.root
+        Path(path), rev2.format(add=add, del_a=del_a), git_repo.root / "x/dir/sub"
     )
 
     assert result == expect
 
 
 @pytest.mark.kwparametrize(
-    dict(rev2="{add}", expect=set()),
-    dict(rev2="{del_a}", expect={Path("dir/a.py")}),
-    dict(rev2="HEAD", expect={Path("dir"), Path("dir/a.py"), Path("dir/b.py")}),
+    dict(
+        paths={"x/dir", "x/dir/a.py", "x/dir/sub", "x/dir/sub/b.py"},
+        rev2="{add}",
+        expect=set(),
+    ),
+    dict(
+        paths={"x/dir", "x/dir/a.py", "x/dir/sub", "x/dir/sub/b.py"},
+        rev2="{del_a}",
+        expect={"x/dir/a.py"},
+    ),
+    dict(
+        paths={"x/dir", "x/dir/a.py", "x/dir/sub", "x/dir/sub/b.py"},
+        rev2="HEAD",
+        expect={"x/dir", "x/dir/a.py", "x/dir/sub", "x/dir/sub/b.py"},
+    ),
+    dict(
+        paths={"x/dir", "x/dir/a.py", "x/dir/sub", "x/dir/sub/b.py"},
+        rev2=":WORKTREE:",
+        expect={"x/dir", "x/dir/a.py", "x/dir/sub", "x/dir/sub/b.py"},
+    ),
+    dict(
+        paths={"dir", "dir/a.py", "dir/sub", "dir/sub/b.py"},
+        cwd="x",
+        rev2="{add}",
+        expect=set(),
+    ),
+    dict(
+        paths={"dir", "dir/a.py", "dir/sub", "dir/sub/b.py"},
+        cwd="x",
+        rev2="{del_a}",
+        expect={"dir/a.py"},
+    ),
+    dict(
+        paths={"dir", "dir/a.py", "dir/sub", "dir/sub/b.py"},
+        cwd="x",
+        rev2="HEAD",
+        expect={"dir", "dir/a.py", "dir/sub", "dir/sub/b.py"},
+    ),
+    dict(
+        paths={"dir", "dir/a.py", "dir/sub", "dir/sub/b.py"},
+        cwd="x",
+        rev2=":WORKTREE:",
+        expect={"dir", "dir/a.py", "dir/sub", "dir/sub/b.py"},
+    ),
+    cwd=".",
+    git_cwd=".",
 )
-def test_get_missing_at_revision(git_repo, rev2, expect):
+def test_get_missing_at_revision(
+    git_repo, monkeypatch, paths, cwd, git_cwd, rev2, expect
+):
     """``get_missing_at_revision()`` returns missing files/directories correctly"""
-    git_repo.add({"dir/a.py": "", "dir/b.py": ""}, commit="Add dir/*.py")
+    git_repo.add(
+        {"x/README": "", "x/dir/a.py": "", "x/dir/sub/b.py": ""},
+        commit="Add x/dir/**/*.py",
+    )
     add = git_repo.get_hash()
-    git_repo.add({"dir/a.py": None}, commit="Delete dir/a.py")
+    git_repo.add({"x/dir/a.py": None}, commit="Delete x/dir/a.py")
     del_a = git_repo.get_hash()
-    git_repo.add({"dir/b.py": None}, commit="Delete dir/b.py")
+    git_repo.add({"x/dir/sub/b.py": None}, commit="Delete x/dir/sub/b.py")
+    monkeypatch.chdir(git_repo.root / cwd)
 
     result = git.get_missing_at_revision(
-        {Path("dir"), Path("dir/a.py"), Path("dir/b.py")},
+        {Path(p) for p in paths},
         rev2.format(add=add, del_a=del_a),
-        git_repo.root,
+        git_repo.root / git_cwd,
     )
 
-    assert result == expect
+    assert result == {Path(p) for p in expect}
 
 
 def test_get_missing_at_revision_worktree(git_repo):
@@ -224,7 +295,7 @@ def test_git_get_modified_python_files(git_repo, modify_paths, paths, expect):
     revrange = RevisionRange("HEAD", ":WORKTREE:")
 
     result = git.git_get_modified_python_files(
-        {root / p for p in paths}, revrange, cwd=root
+        {root / p for p in paths}, revrange, repo_root=root
     )
 
     assert result == {Path(p) for p in expect}