From 98f0a756f993424b94201cbad42b89af772d0329 Mon Sep 17 00:00:00 2001 From: Thierry Date: Sun, 27 Mar 2022 17:49:54 +0200 Subject: [PATCH 1/6] Improve conflict resolution Co-authored-by: Oleh Prypin --- copier/main.py | 134 ++++++++++++++++++++----------------- docs/updating.md | 25 +++---- tests/test_output.py | 1 - tests/test_subdirectory.py | 7 +- tests/test_updatediff.py | 28 +++++--- 5 files changed, 105 insertions(+), 90 deletions(-) diff --git a/copier/main.py b/copier/main.py index ea0c180fc..46650e647 100644 --- a/copier/main.py +++ b/copier/main.py @@ -1,5 +1,6 @@ """Main functions and classes, used to generate or update projects.""" +import os import platform import subprocess import sys @@ -8,15 +9,14 @@ from functools import partial from itertools import chain from pathlib import Path -from shutil import rmtree +from shutil import copyfile, copytree, rmtree from typing import Callable, Iterable, List, Mapping, Optional, Sequence from unicodedata import normalize import pathspec from jinja2.loaders import FileSystemLoader from jinja2.sandbox import SandboxedEnvironment -from plumbum import ProcessExecutionError, colors -from plumbum.cli.terminal import ask +from plumbum import colors from plumbum.cmd import git from plumbum.machines import local from pydantic.dataclasses import dataclass @@ -218,10 +218,7 @@ def _path_matcher(self, patterns: Iterable[str]) -> Callable[[Path], bool]: return spec.match_file def _solve_render_conflict(self, dst_relpath: Path): - """Properly solve render conflicts. - - It can ask the user if running in interactive mode. - """ + """Properly solve render conflicts.""" assert not dst_relpath.is_absolute() printf( "conflict", @@ -239,16 +236,8 @@ def _solve_render_conflict(self, dst_relpath: Path): file_=sys.stderr, ) return False - if self.overwrite or dst_relpath == self.answers_relpath: - printf( - "overwrite", - dst_relpath, - style=Style.WARNING, - quiet=self.quiet, - file_=sys.stderr, - ) - return True - return bool(ask(f" Overwrite {dst_relpath}?", default=True)) + + return True def _render_allowed( self, @@ -657,26 +646,31 @@ def run_update(self) -> None: print( f"Updating to template version {self.template.version}", file=sys.stderr ) + # Copy old template into a temporary destination - with TemporaryDirectory(prefix=f"{__name__}.update_diff.") as dst_temp: - old_worker = replace( + with TemporaryDirectory( + prefix=f"{__name__}.update_diff.reference." + ) as reference_dst_temp, TemporaryDirectory( + prefix=f"{__name__}.update_diff.original." + ) as original_dst_temp, TemporaryDirectory( + prefix=f"{__name__}.update_diff.merge." + ) as merge_dst_temp: + # Copy reference to be used as base by merge-file + copytree(self.dst_path, reference_dst_temp, dirs_exist_ok=True) + + # Compute modification from the original template to be used as other by merge-file + original_worker = replace( self, - dst_path=dst_temp, + dst_path=original_dst_temp, data=self.subproject.last_answers, defaults=True, quiet=True, src_path=self.subproject.template.url, vcs_ref=self.subproject.template.commit, ) - old_worker.run_copy() - # Extract diff between temporary destination and real destination - with local.cwd(dst_temp): - subproject_top = git( - "-C", - self.subproject.local_abspath.absolute(), - "rev-parse", - "--show-toplevel", - ).strip() + original_worker.run_copy() + # Apply pre-commit hooks if provided by .git + with local.cwd(original_dst_temp): git("init", retcode=None) git("add", ".") git("config", "user.name", "Copier") @@ -686,37 +680,57 @@ def run_update(self) -> None: git("commit", "--allow-empty", "-am", "dumb commit 2") git("config", "--unset", "user.name") git("config", "--unset", "user.email") - git("remote", "add", "real_dst", "file://" + subproject_top) - git("fetch", "--depth=1", "real_dst", "HEAD") - diff_cmd = git["diff-tree", "--unified=1", "HEAD...FETCH_HEAD"] - try: - diff = diff_cmd("--inter-hunk-context=-1") - except ProcessExecutionError: - print( - colors.warn - | "Make sure Git >= 2.24 is installed to improve updates.", - file=sys.stderr, - ) - diff = diff_cmd("--inter-hunk-context=0") - # Run pre-migration tasks - self._execute_tasks( - self.template.migration_tasks("before", self.subproject.template) - ) - # Clear last answers cache to load possible answers migration - with suppress(AttributeError): - del self.answers - with suppress(AttributeError): - del self.subproject.last_answers - # Do a normal update in final destination - self.run_copy() - # Try to apply cached diff into final destination - with local.cwd(self.subproject.local_abspath): - apply_cmd = git["apply", "--reject", "--exclude", self.answers_relpath] - for skip_pattern in chain( - self.skip_if_exists, self.template.skip_if_exists - ): - apply_cmd = apply_cmd["--exclude", skip_pattern] - (apply_cmd << diff)(retcode=None) + + # Run pre-migration tasks + self._execute_tasks( + self.template.migration_tasks("before", self.subproject.template) + ) + # Clear last answers cache to load possible answers migration + with suppress(AttributeError): + del self.answers + with suppress(AttributeError): + del self.subproject.last_answers + # Do a normal update in final destination + self.run_copy() + + # Extract the list of files to merge + participating_files: set[Path] = set() + for src_dir in (original_dst_temp, reference_dst_temp): + for root, dirs, files in os.walk(src_dir, topdown=True): + if root == src_dir and ".git" in dirs: + dirs.remove(".git") + root = Path(root).relative_to(src_dir) + participating_files.update(Path(root, f) for f in files) + + # Merging files + for basename in sorted(participating_files): + subfile_names = [] + for subfile_kind, src_dir in [ + ("modified", reference_dst_temp), + ("old upstream", original_dst_temp), + ("new upstream", self.dst_path), + ]: + path = Path(src_dir, basename) + if path.is_file(): + copyfile(path, Path(merge_dst_temp, subfile_kind)) + else: + subfile_kind = os.devnull + subfile_names.append(subfile_kind) + + with local.cwd(merge_dst_temp): + output = git("merge-file", "-p", *subfile_names, retcode=None) + + dest_path = Path(self.dst_path, basename) + # Remove the file if it was already removed in the project + if not output and "modified" not in subfile_names: + try: + dest_path.unlink() + except FileNotFoundError: + pass + else: + dest_path.parent.mkdir(parents=True, exist_ok=True) + dest_path.write_text(output) + # Run post-migration tasks self._execute_tasks( self.template.migration_tasks("after", self.subproject.template) diff --git a/docs/updating.md b/docs/updating.md index b94e7ab12..7df9b14c6 100644 --- a/docs/updating.md +++ b/docs/updating.md @@ -22,25 +22,18 @@ other git ref you want. When updating, Copier will do its best to respect your project evolution by using the answers you provided when copied last time. However, sometimes it's impossible for -Copier to know what to do with a diff code hunk. In those cases, you will find `*.rej` -files that contain the unresolved diffs. _You should review those manually_ before -committing. - -You probably don't want `*.rej` files in your git history, but if you add them to -`.gitignore`, some important changes could pass unnoticed to you. That's why the -recommended way to deal with them is to _not_ add them to add a -[pre-commit](https://pre-commit.com/) (or equivalent) hook that forbids them, just like -this: +Copier to know what to do with a diff code hunk. In those cases, each conflicted files +will contain the unresolved diffs. _Those conflicts must be reviewed before committing_. +That's why we recommand to add them to add a [pre-commit](https://pre-commit.com/) (or +equivalent) hook that detects them, just like this: ```yaml title=".pre-commit-config.yaml" repos: - - repo: local - hooks: - - id: forbidden-files - name: forbidden files - entry: found copier update rejection files; review them and remove them - language: fail - files: "\\.rej$" + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v4.0.1 + hooks: + - id: check-merge-conflict + args: [--assume-in-merge] ``` ## Never change the answers file manually diff --git a/tests/test_output.py b/tests/test_output.py index be1578279..a9b2e42fb 100644 --- a/tests/test_output.py +++ b/tests/test_output.py @@ -25,7 +25,6 @@ def test_output_force(capsys, tmp_path): render(tmp_path, quiet=False, defaults=True, overwrite=True) _, err = capsys.readouterr() assert re.search(r"conflict[^\s]* config\.py", err) - assert re.search(r"overwrite[^\s]* config\.py", err) assert re.search(r"identical[^\s]* pyproject\.toml", err) assert re.search(r"identical[^\s]* doc[/\\]images[/\\]nslogo\.gif", err) diff --git a/tests/test_subdirectory.py b/tests/test_subdirectory.py index 24a538923..dd82cfdcd 100644 --- a/tests/test_subdirectory.py +++ b/tests/test_subdirectory.py @@ -212,10 +212,13 @@ def test_new_version_uses_subdirectory(tmp_path_factory): copier.copy(dst_path=project_path, defaults=True, overwrite=True) assert "_commit: v2" in (project_path / ".copier-answers.yml").read_text() - # Assert that the README still exists, and was force updated to "upstream version 2" + # Assert that the README still exists with conflicts to solve assert (project_path / "README.md").exists() with (project_path / "README.md").open() as fd: - assert fd.read() == "upstream version 2\n" + assert fd.read() == ( + "<<<<<<< modified\ndownstream version 1\n" + "=======\nupstream version 2\n>>>>>>> new upstream\n" + ) # Also assert the subdirectory itself was not rendered assert not (project_path / "subdir").exists() diff --git a/tests/test_updatediff.py b/tests/test_updatediff.py index 3dacbc40a..e1e8edfde 100644 --- a/tests/test_updatediff.py +++ b/tests/test_updatediff.py @@ -5,7 +5,7 @@ import pytest import yaml -from plumbum import local +from plumbum import ProcessExecutionError, local from plumbum.cmd import git from copier import Worker, copy @@ -305,17 +305,15 @@ def test_commit_hooks_respected(tmp_path_factory): """, ".pre-commit-config.yaml": r""" repos: + - repo: https://github.com/pre-commit/pre-commit-hooks + rev: v4.0.1 + hooks: + - id: check-merge-conflict + args: [--assume-in-merge] - repo: https://github.com/pre-commit/mirrors-prettier rev: v2.0.4 hooks: - id: prettier - - repo: local - hooks: - - id: forbidden-files - name: forbidden files - entry: found forbidden files; remove them - language: fail - files: "\\.rej$" """, "life.yml.tmpl": """ # Following code should be reformatted by pre-commit after copying @@ -401,7 +399,7 @@ def test_commit_hooks_respected(tmp_path_factory): with local.cwd(dst2): # Subproject re-updates just to change some values copy(data={"what": "study"}, defaults=True, overwrite=True) - git("commit", "-am", "re-updated to change values after evolving") + # Subproject evolution was respected up to sane possibilities. # In an ideal world, this file would be exactly the same as what's written # a few lines above, just changing "grog" for "study". However, that's nearly @@ -416,14 +414,22 @@ def test_commit_hooks_respected(tmp_path_factory): """\ Line 1: hello world Line 2: grow up + <<<<<<< modified + Line 2.5: make friends + Line 3: grog + ======= Line 3: study + >>>>>>> new upstream Line 4: grow old Line 4.5: no more work Line 5: bye bye world """ ) - # This time a .rej file is unavoidable - assert Path(f"{life}.rej").is_file() + + # Assert the conflict is detected by precommit + with pytest.raises(ProcessExecutionError) as execution_error: + git("commit", "-am", "re-updated to change values after evolving") + assert 'Merge conflict string "<<<<<<< "' in execution_error.value.stderr def test_skip_update(tmp_path_factory): From c36cfc9477c83725a741d087230766a3c7fadfe1 Mon Sep 17 00:00:00 2001 From: Thierry Date: Mon, 28 Mar 2022 18:09:16 +0200 Subject: [PATCH 2/6] Fixing debian and python 3.7 tests issues --- copier/main.py | 21 ++++++++++++++++++--- docs/updating.md | 6 +++--- tests/test_updatediff.py | 1 + 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/copier/main.py b/copier/main.py index 46650e647..40be276c5 100644 --- a/copier/main.py +++ b/copier/main.py @@ -6,11 +6,12 @@ import sys from contextlib import suppress from dataclasses import asdict, field, replace +from distutils.dir_util import copy_tree from functools import partial from itertools import chain from pathlib import Path -from shutil import copyfile, copytree, rmtree -from typing import Callable, Iterable, List, Mapping, Optional, Sequence +from shutil import copyfile, rmtree +from typing import Callable, Iterable, List, Mapping, Optional, Sequence, Set from unicodedata import normalize import pathspec @@ -40,9 +41,23 @@ # HACK https://github.com/python/mypy/issues/8520#issuecomment-772081075 if sys.version_info >= (3, 8): from functools import cached_property + else: from backports.cached_property import cached_property +# Backport of `shutil.copytree` for python 3.7 to accept `dirs_exist_ok` argument +if sys.version_info <= (3, 8): + from shutil import copytree +else: + from distutils.dir_util import copy_tree + + def copytree(src: Path, dst: Path, dirs_exist_ok: bool = False): + """Backport of `shutil.copytree` with `dirs_exist_ok` argument. + + Can be remove once python 3.7 dropped. + """ + copy_tree(str(src), str(dst)) + @dataclass class Worker: @@ -694,7 +709,7 @@ def run_update(self) -> None: self.run_copy() # Extract the list of files to merge - participating_files: set[Path] = set() + participating_files: Set[Path] = set() for src_dir in (original_dst_temp, reference_dst_temp): for root, dirs, files in os.walk(src_dir, topdown=True): if root == src_dir and ".git" in dirs: diff --git a/docs/updating.md b/docs/updating.md index 7df9b14c6..a48ebedc4 100644 --- a/docs/updating.md +++ b/docs/updating.md @@ -30,9 +30,9 @@ equivalent) hook that detects them, just like this: ```yaml title=".pre-commit-config.yaml" repos: - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v4.0.1 - hooks: - - id: check-merge-conflict + rev: v4.0.1 + hooks: + - id: check-merge-conflict args: [--assume-in-merge] ``` diff --git a/tests/test_updatediff.py b/tests/test_updatediff.py index e1e8edfde..bc2544274 100644 --- a/tests/test_updatediff.py +++ b/tests/test_updatediff.py @@ -220,6 +220,7 @@ def test_updatediff(tmp_path_factory): _src_path: {bundle} author_name: Guybrush project_name: to become a pirate\n + >>>>>>> new upstream """ ) assert readme.read_text() == dedent( From 62b96dc0beb63ac47a8e05d501bd8de0eb9e6f9d Mon Sep 17 00:00:00 2001 From: Thierry Date: Mon, 28 Mar 2022 19:38:23 +0200 Subject: [PATCH 3/6] Fixing wrong test_updatediff --- tests/test_updatediff.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_updatediff.py b/tests/test_updatediff.py index bc2544274..e1e8edfde 100644 --- a/tests/test_updatediff.py +++ b/tests/test_updatediff.py @@ -220,7 +220,6 @@ def test_updatediff(tmp_path_factory): _src_path: {bundle} author_name: Guybrush project_name: to become a pirate\n - >>>>>>> new upstream """ ) assert readme.read_text() == dedent( From 158e8c9bd9f84024b395ef78c9c2385be859d11f Mon Sep 17 00:00:00 2001 From: Thierry Date: Mon, 28 Mar 2022 19:46:33 +0200 Subject: [PATCH 4/6] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6af602bf3..cd346157f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,8 @@ Summary: - Changed `--force` to be the same as `--defaults --overwrite`. - Copied files will reflect permissions on the same files in the template. - Copier now uses `git clone --filter=blob:none` when cloning, to be faster. +- Improved `copier update` conficts resolution by replacing the `.rej` files by merge + markers put in the destination file directly. ### Deprecated From f56e8c6bfd8df2fef81c5709ef0b6267be5c5c64 Mon Sep 17 00:00:00 2001 From: Thierry Date: Mon, 28 Mar 2022 19:48:05 +0200 Subject: [PATCH 5/6] Fixing precommit. --- copier/main.py | 1 - 1 file changed, 1 deletion(-) diff --git a/copier/main.py b/copier/main.py index 40be276c5..6cb949e27 100644 --- a/copier/main.py +++ b/copier/main.py @@ -6,7 +6,6 @@ import sys from contextlib import suppress from dataclasses import asdict, field, replace -from distutils.dir_util import copy_tree from functools import partial from itertools import chain from pathlib import Path From 5b1a8175a93fe7431b59218003af25495adff70c Mon Sep 17 00:00:00 2001 From: Thierry Guillemot Date: Tue, 29 Mar 2022 17:26:00 +0200 Subject: [PATCH 6/6] Update main.py Fixing mistake with python version --- copier/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/copier/main.py b/copier/main.py index 6cb949e27..41f2597a7 100644 --- a/copier/main.py +++ b/copier/main.py @@ -45,7 +45,7 @@ from backports.cached_property import cached_property # Backport of `shutil.copytree` for python 3.7 to accept `dirs_exist_ok` argument -if sys.version_info <= (3, 8): +if sys.version_info >= (3, 8): from shutil import copytree else: from distutils.dir_util import copy_tree