From 76020f8df47cac007ccf49e706b16cd157916617 Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 15 Apr 2024 19:43:37 +0300 Subject: [PATCH 1/9] Rename `root` to `common_root` in main for clarity --- src/darker/__main__.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/darker/__main__.py b/src/darker/__main__.py index 937343124..09c71220f 100644 --- a/src/darker/__main__.py +++ b/src/darker/__main__.py @@ -516,10 +516,10 @@ 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) 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 +539,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 +550,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 +563,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 +584,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 +602,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, ) From dfa0105525a63ebd89407cecfbc88a74361d057f Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 15 Apr 2024 19:43:56 +0300 Subject: [PATCH 2/9] Add clarifying comment in `main()` --- src/darker/__main__.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/darker/__main__.py b/src/darker/__main__.py index 09c71220f..f9c77d467 100644 --- a/src/darker/__main__.py +++ b/src/darker/__main__.py @@ -517,6 +517,10 @@ def main( # pylint: disable=too-many-locals,too-many-branches,too-many-statemen black_config["skip_magic_trailing_comma"] = args.skip_magic_trailing_comma 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, common_root, args.stdin_filename is not None From 4e676698ded304a3a6834b5db17cbfa3b37bb308 Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 15 Apr 2024 19:45:03 +0300 Subject: [PATCH 3/9] Fix path handling in `_git_exists_in_revision` Now Git calls work correctly even if the common root directory in which to run Git doesn't exist anymore. --- src/darker/git.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/darker/git.py b/src/darker/git.py index fff2eb1d6..c5039860b 100644 --- a/src/darker/git.py +++ b/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(), From 4daedb2dddfc47801260d87d507b89d9ed83dc95 Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 15 Apr 2024 19:46:22 +0300 Subject: [PATCH 4/9] Clarify `get_missing_at_revision` docstring --- src/darker/git.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/darker/git.py b/src/darker/git.py index c5039860b..a1a908c39 100644 --- a/src/darker/git.py +++ b/src/darker/git.py @@ -116,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 """ From 7543f050f87615c62540360cbf04404033ebd407 Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 15 Apr 2024 19:46:46 +0300 Subject: [PATCH 5/9] `cwd=` -> `repo_root=` in `_git_diff_name_only()` --- src/darker/git.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/darker/git.py b/src/darker/git.py index a1a908c39..985a2b0f7 100644 --- a/src/darker/git.py +++ b/src/darker/git.py @@ -129,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 """ @@ -152,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} From cc19c6e67aeb086a11b2f74ccddf844db01ea863 Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 15 Apr 2024 19:47:28 +0300 Subject: [PATCH 6/9] `_git_ls_files_others` docstring clarification --- src/darker/git.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/darker/git.py b/src/darker/git.py index 985a2b0f7..b3b213d30 100644 --- a/src/darker/git.py +++ b/src/darker/git.py @@ -162,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 From 1a87f5ca9fc4074fac50c1049c9dfbee5f17f5b4 Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 15 Apr 2024 19:47:52 +0300 Subject: [PATCH 7/9] Fix `_git_ls_files_others()` - rename `cwd=` to `repo_root=` for clarity - transform paths: relative to cwd -> relative to `repo_root` --- src/darker/git.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/darker/git.py b/src/darker/git.py index b3b213d30..e89ca8636 100644 --- a/src/darker/git.py +++ b/src/darker/git.py @@ -179,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 -- `` - ``git ls-files --others --exclude-standard -- `` - :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( From b0a2674763dd7eabf342418db4119dd82f02a0a7 Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 15 Apr 2024 19:48:50 +0300 Subject: [PATCH 8/9] Test cases for running from non-Git-root Add such test cases to tests for - `_git_exists_in_revision()` - `get_missing_at_revision()` --- src/darker/tests/test_git.py | 129 +++++++++++++++++++++++++++-------- 1 file changed, 100 insertions(+), 29 deletions(-) diff --git a/src/darker/tests/test_git.py b/src/darker/tests/test_git.py index 8601faff3..8b65ed855 100644 --- a/src/darker/tests/test_git.py +++ b/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} From 8e8deb2c0a42da370cf18aaa63bc69a5842d5742 Mon Sep 17 00:00:00 2001 From: Antti Kaihola <13725+akaihola@users.noreply.github.com> Date: Mon, 15 Apr 2024 20:16:51 +0300 Subject: [PATCH 9/9] Update the change log --- CHANGES.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.rst b/CHANGES.rst index 92c84fffa..74ed0f39d 100644 --- a/CHANGES.rst +++ b/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