From 0b9491952aad54e40edeb8c584b6220758aef5b7 Mon Sep 17 00:00:00 2001 From: Tom Saunders Date: Thu, 3 Sep 2020 22:38:37 +0000 Subject: [PATCH 1/5] Add test to show differing behaviour between DIFF and COLOR_DIFF --- tests/test_black.py | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/tests/test_black.py b/tests/test_black.py index bc80c8fca8b..364eccaeca7 100644 --- a/tests/test_black.py +++ b/tests/test_black.py @@ -1395,9 +1395,29 @@ def test_no_cache_when_writeback_diff(self) -> None: src = (workspace / "test.py").resolve() with src.open("w") as fobj: fobj.write("print('hello')") - self.invokeBlack([str(src), "--diff"]) - cache_file = black.get_cache_file(mode) - self.assertFalse(cache_file.exists()) + with patch("black.read_cache") as read_cache, patch( + "black.write_cache" + ) as write_cache: + self.invokeBlack([str(src), "--diff"]) + cache_file = black.get_cache_file(mode) + self.assertFalse(cache_file.exists()) + write_cache.assert_not_called() + read_cache.assert_not_called() + + def test_no_cache_when_writeback_color_diff(self) -> None: + mode = DEFAULT_MODE + with cache_dir() as workspace: + src = (workspace / "test.py").resolve() + with src.open("w") as fobj: + fobj.write("print('hello')") + with patch("black.read_cache") as read_cache, patch( + "black.write_cache" + ) as write_cache: + self.invokeBlack([str(src), "--diff", "--color"]) + cache_file = black.get_cache_file(mode) + self.assertFalse(cache_file.exists()) + write_cache.assert_not_called() + read_cache.assert_not_called() def test_no_cache_when_stdin(self) -> None: mode = DEFAULT_MODE From a983bdaf40cad81cceb2e4e46d9a148d08222fa9 Mon Sep 17 00:00:00 2001 From: Tom Saunders Date: Fri, 4 Sep 2020 17:26:37 +0000 Subject: [PATCH 2/5] Demonstrate that diff with colour doesn't lock for output. This isn't _quite_ true - this demonstrates that we never get the manager used to provide the lock, but without that we're clearly not getting the lock. --- tests/test_black.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/test_black.py b/tests/test_black.py index 364eccaeca7..90857d3f8b7 100644 --- a/tests/test_black.py +++ b/tests/test_black.py @@ -1,4 +1,5 @@ #!/usr/bin/env python3 +import multiprocessing import asyncio import logging from concurrent.futures import ThreadPoolExecutor @@ -1419,6 +1420,32 @@ def test_no_cache_when_writeback_color_diff(self) -> None: write_cache.assert_not_called() read_cache.assert_not_called() + @event_loop() + def test_output_locking_when_writeback_diff(self) -> None: + with cache_dir() as workspace: + for tag in range(0, 4): + src = (workspace / f"test{tag}.py").resolve() + with src.open("w") as fobj: + fobj.write("print('hello')") + with patch("black.Manager", wraps=multiprocessing.Manager) as mgr: + self.invokeBlack(["--diff", str(workspace)], exit_code=123) + # this isn't quite doing what we want, but if it _isn't_ + # called then we cannot be using the lock it provides + mgr.assert_called() + + @event_loop() + def test_output_locking_when_writeback_color_diff(self) -> None: + with cache_dir() as workspace: + for tag in range(0, 4): + src = (workspace / f"test{tag}.py").resolve() + with src.open("w") as fobj: + fobj.write("print('hello')") + with patch("black.Manager", wraps=multiprocessing.Manager) as mgr: + self.invokeBlack(["--diff", "--color", str(workspace)], exit_code=123) + # this isn't quite doing what we want, but if it _isn't_ + # called then we cannot be using the lock it provides + mgr.assert_called() + def test_no_cache_when_stdin(self) -> None: mode = DEFAULT_MODE with cache_dir(): From 5a5934e646a56c6c4c543ed373e7d56b77539346 Mon Sep 17 00:00:00 2001 From: Tom Saunders Date: Thu, 3 Sep 2020 22:41:02 +0000 Subject: [PATCH 3/5] Check both WriteBack.DIFF and .COLORDIFF when outputting. It seems that when .COLOR_DIFF is used we don't use the locks in a manner consistent with how .DIFF is using them. --- src/black/__init__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/black/__init__.py b/src/black/__init__.py index 64a18655905..f7e7603321a 100644 --- a/src/black/__init__.py +++ b/src/black/__init__.py @@ -661,7 +661,7 @@ def reformat_one( changed = Changed.YES else: cache: Cache = {} - if write_back != WriteBack.DIFF: + if write_back not in (WriteBack.DIFF, WriteBack.COLOR_DIFF): cache = read_cache(mode) res_src = src.resolve() if res_src in cache and cache[res_src] == get_cache_info(res_src): @@ -735,7 +735,7 @@ async def schedule_formatting( :func:`format_file_in_place`. """ cache: Cache = {} - if write_back != WriteBack.DIFF: + if write_back not in (WriteBack.DIFF, WriteBack.COLOR_DIFF): cache = read_cache(mode) sources, cached = filter_cached(cache, sources) for src in sorted(cached): @@ -746,7 +746,7 @@ async def schedule_formatting( cancelled = [] sources_to_cache = [] lock = None - if write_back == WriteBack.DIFF: + if write_back in (WriteBack.DIFF, WriteBack.COLOR_DIFF): # For diff output, we need locks to ensure we don't interleave output # from different processes. manager = Manager() From 8f70e9526485d114b152f7a16cb652abbbefb8e0 Mon Sep 17 00:00:00 2001 From: Tom Saunders Date: Fri, 4 Sep 2020 18:10:32 +0000 Subject: [PATCH 4/5] Modify test exit codes to stop failing in CI. As modified they fail local to me, but I suspect that's a me problem :) --- tests/test_black.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_black.py b/tests/test_black.py index 90857d3f8b7..edcf7208b46 100644 --- a/tests/test_black.py +++ b/tests/test_black.py @@ -1428,7 +1428,7 @@ def test_output_locking_when_writeback_diff(self) -> None: with src.open("w") as fobj: fobj.write("print('hello')") with patch("black.Manager", wraps=multiprocessing.Manager) as mgr: - self.invokeBlack(["--diff", str(workspace)], exit_code=123) + self.invokeBlack(["--diff", str(workspace)], exit_code=0) # this isn't quite doing what we want, but if it _isn't_ # called then we cannot be using the lock it provides mgr.assert_called() @@ -1441,7 +1441,7 @@ def test_output_locking_when_writeback_color_diff(self) -> None: with src.open("w") as fobj: fobj.write("print('hello')") with patch("black.Manager", wraps=multiprocessing.Manager) as mgr: - self.invokeBlack(["--diff", "--color", str(workspace)], exit_code=123) + self.invokeBlack(["--diff", "--color", str(workspace)], exit_code=0) # this isn't quite doing what we want, but if it _isn't_ # called then we cannot be using the lock it provides mgr.assert_called() From 405ec2a5994047b1fb0bfaa68d9c46517b6d48eb Mon Sep 17 00:00:00 2001 From: Tom Saunders Date: Fri, 4 Sep 2020 20:38:06 +0000 Subject: [PATCH 5/5] Add changelog entry for .COLORDIFF handling --- CHANGES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 7e356f1f29c..52c8016a257 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -12,6 +12,8 @@ - fixed a crash when PWD=/ on POSIX (#1631) +- Prevent coloured diff output being interleaved with multiple files (#1673) + ### 20.8b1 #### _Packaging_