Repository: copier-org/copier
Pull request #1587 (2024-04-16 14:36:04)

fix(updating): yield merge conflict when both template and project add same file
I've stumbled over some suboptimal updating UX when _both_ the template and the project add the _same_ new file where the update algorithm would delete file content added by the project but not present in the template. This PR aims to improve the UX by yielding merge conflicts instead which seems semantically more correct than simply deleting previous project content.

The problem originates from the diff between the fresh project based on the old template and the actual project which includes a "new file mode" patch for any file added by the actual project, which gets applied to the same new file added to the project as part of rendering the new template on top of the project:

https://github.com/copier-org/copier/blob/813c90335276320a17c964a2cd4d4e91d89d8662/copier/main.py#L896-L906

My suggested solution is to identify all files that are added by _both_ the project _and_ the new template and create an empty file in the fresh project based on the old template before extracting the diff, so the patch does not use the "new file mode".

I've added two test cases that – of course – test the new behavior but also exemplify two (simplified) situations that I was facing:

1. Simple (see `test_update_with_new_file_in_template_and_project`) – Both the template and the project add the file `.gitlab-ci.yml[.jinja]`. In this case, the content of `.gitlab-ci.yml` after updating would have been this:

    ```yaml
    tests:
      stage: test
      script:
        - ./test.sh --slow
    ```

    With the change of this PR:

    ```yaml
    tests:
      stage: test
      script:
    <<<<<<< before updating
        - ./test.sh
    pages:
      stage: deploy
      script:
        - ./deploy.sh
    =======
        - ./test.sh --slow
    >>>>>>> after updating
    ```

1. Complex (see `test_update_with_new_file_in_template_and_project_via_migration`) – The project adds content to `.gitlab-ci.yml` on top of what the first version of the template provides. In a new version of the template, `.gitlab-ci.yml.jinja` is moved to `.gitlab/ci/main.yml.jinja` and `.gitlab-ci.yml.jinja` now includes the generated `.gitlab/ci/main.yml`. To retain the project's changes/additions to `.gitlab-ci.yml`, I created a pre-update migration task that copies `.gitlab-ci.yml` (containing those changes/additions)  to `.gitlab/ci/main.yml` and stages it, then Copier applies the new template's version of that file (which was also moved there, but Git doesn't recognize it as status `R` but as `A`). (In case somebody has a better idea on how to do this, I'd be curious. This is the best what I could think of.) In this case, the content of `.gitlab/ci/main.yml` after updating would have been this:

    ```yaml
    tests:
      stage: test
      script:
        - ./test.sh --slow
    ```

    With the change of this PR:

    ```yaml
    tests:
      stage: test
      script:
    <<<<<<< before updating
        - ./test.sh
    pages:
      stage: deploy
      script:
        - ./deploy.sh
    =======
        - ./test.sh --slow
    >>>>>>> after updating
    ```

WDYT?

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/copier-org/copier/pull/1587?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org) Report
Attention: Patch coverage is `96.10390%` with `3 lines` in your changes are missing coverage. Please review.
> Project coverage is 96.73%. Comparing base [(`813c903`)](https://app.codecov.io/gh/copier-org/copier/commit/813c90335276320a17c964a2cd4d4e91d89d8662?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org) to head [(`d4627e4`)](https://app.codecov.io/gh/copier-org/copier/pull/1587?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org).

> :exclamation: Current head d4627e4 differs from pull request most recent head 5bf7e8a. Consider uploading reports for the commit 5bf7e8a to get more accurate results

| [Files](https://app.codecov.io/gh/copier-org/copier/pull/1587?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org) | Patch % | Lines |
|---|---|---|
| [copier/main.py](https://app.codecov.io/gh/copier-org/copier/pull/1587?src=pr&el=tree&filepath=copier%2Fmain.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org#diff-Y29waWVyL21haW4ucHk=) | 89.28% | [3 Missing :warning: ](https://app.codecov.io/gh/copier-org/copier/pull/1587?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #1587      +/-   ##
==========================================
- Coverage   97.29%   96.73%   -0.56%     
==========================================
  Files          48       48              
  Lines        4621     4687      +66     
==========================================
+ Hits         4496     4534      +38     
- Misses        125      153      +28     
```

| [Flag](https://app.codecov.io/gh/copier-org/copier/pull/1587/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org) | Coverage Δ | |
|---|---|---|
| [unittests](https://app.codecov.io/gh/copier-org/copier/pull/1587/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org) | `96.73% <96.10%> (-0.56%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org#carryforward-flags-in-the-pull-request-comment) to find out more.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/copier-org/copier/pull/1587?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=copier-org).

sisp: I don't understand why Codecov thinks there's such a high coverage drop. :thinking:
sisp: @pawamoy Good idea, thanks! I've force-pushed the amended commit.

Patch:
diff --git a/copier/main.py b/copier/main.py
index 5e148d0..277f01d 100644
--- a/copier/main.py
+++ b/copier/main.py
@@ -11,7 +11,7 @@ from filecmp import dircmp
 from functools import cached_property, partial
 from itertools import chain
 from pathlib import Path
-from shutil import rmtree
+from shutil import copytree, ignore_patterns, rmtree
 from tempfile import TemporaryDirectory
 from types import TracebackType
 from typing import (
@@ -853,8 +853,12 @@ class Worker:
         subproject_subdir = self.subproject.local_abspath.relative_to(subproject_top)
 
         with TemporaryDirectory(
-            prefix=f"{__name__}.old_copy."
-        ) as old_copy, TemporaryDirectory(prefix=f"{__name__}.new_copy.") as new_copy:
+            prefix=f"{__name__}.old_copy.",
+        ) as old_copy, TemporaryDirectory(
+            prefix=f"{__name__}.new_copy.",
+        ) as new_copy, TemporaryDirectory(
+            prefix=f"{__name__}.dst_copy.",
+        ) as dst_copy:
             # Copy old template into a temporary destination
             with replace(
                 self,
@@ -866,27 +870,23 @@ class Worker:
                 vcs_ref=self.subproject.template.commit,  # type: ignore[union-attr]
             ) as old_worker:
                 old_worker.run_copy()
-            # Extract diff between temporary destination and real destination
-            with local.cwd(old_copy):
-                self._git_initialize_repo()
-                git("remote", "add", "real_dst", "file://" + str(subproject_top))
-                git("fetch", "--depth=1", "real_dst", "HEAD")
-                diff_cmd = git[
-                    "diff-tree", f"--unified={self.context_lines}", "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)  # type: ignore[arg-type]
             )
+            # Create a copy of the real destination after applying migrations
+            # but before performing any further update for extracting the diff
+            # between the temporary destination of the old template and the
+            # real destination later.
+            with local.cwd(dst_copy):
+                copytree(
+                    subproject_top,
+                    ".",
+                    symlinks=True,
+                    ignore=ignore_patterns("/.git"),
+                    dirs_exist_ok=True,
+                )
+                self._git_initialize_repo()
             # Clear last answers cache to load possible answers migration, if skip_answered flag is not set
             if self.skip_answered is False:
                 self.answers = AnswersMap()
@@ -913,6 +913,49 @@ class Worker:
                 src_path=self.subproject.template.url,  # type: ignore[union-attr]
             ) as new_worker:
                 new_worker.run_copy()
+            with local.cwd(new_copy):
+                self._git_initialize_repo()
+            # Extract diff between temporary destination and (copy from above)
+            # real destination with some special handling of newly added files
+            # in both the poject and the template.
+            with local.cwd(old_copy):
+                self._git_initialize_repo()
+                git("remote", "add", "dst_copy", "file://" + str(dst_copy))
+                git("fetch", "--depth=1", "dst_copy", "HEAD:dst_copy")
+                git("remote", "add", "new_copy", "file://" + str(new_copy))
+                git("fetch", "--depth=1", "new_copy", "HEAD:new_copy")
+                # Create an empty file in the temporary destination when the
+                # same file was added in *both* the project and the temporary
+                # destination of the new template. With this minor change, the
+                # diff between the temporary destination and the real
+                # destination for such files will use the "update file mode"
+                # instead of the "new file mode" which avoids deleting the file
+                # content previously added in the project.
+                diff_added_cmd = git[
+                    "diff-tree", "-r", "--diff-filter=A", "--name-only"
+                ]
+                for filename in (
+                    set(diff_added_cmd("HEAD...dst_copy").splitlines())
+                ) & set(diff_added_cmd("HEAD...new_copy").splitlines()):
+                    f = Path(filename)
+                    f.parent.mkdir(parents=True, exist_ok=True)
+                    f.touch(Path(dst_copy, filename).stat().st_mode)
+                    git("add", filename)
+                self._git_commit("add new empty files")
+                # Extract diff between temporary destination and real
+                # destination
+                diff_cmd = git[
+                    "diff-tree", f"--unified={self.context_lines}", "HEAD...dst_copy"
+                ]
+                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")
             compared = dircmp(old_copy, new_copy)
             # Try to apply cached diff into final destination
             with local.cwd(subproject_top):
@@ -998,6 +1041,10 @@ class Worker:
         git = get_git()
         git("init", retcode=None)
         git("add", ".")
+        self._git_commit()
+
+    def _git_commit(self, message: str = "dumb commit") -> None:
+        git = get_git()
         git("config", "user.name", "Copier")
         git("config", "user.email", "copier@copier")
         # 1st commit could fail if any pre-commit hook reformats code
@@ -1006,7 +1053,7 @@ class Worker:
             "commit",
             "--allow-empty",
             "-am",
-            "dumb commit 1",
+            f"{message} 1",
             "--no-gpg-sign",
             retcode=None,
         )
@@ -1014,7 +1061,7 @@ class Worker:
             "commit",
             "--allow-empty",
             "-am",
-            "dumb commit 2",
+            f"{message} 2",
             "--no-gpg-sign",
             "--no-verify",
         )


Test patch:
diff --git a/tests/test_updatediff.py b/tests/test_updatediff.py
index 263ae9f..81a2019 100644
--- a/tests/test_updatediff.py
+++ b/tests/test_updatediff.py
@@ -1076,3 +1076,220 @@ def test_conflicted_files_are_marked_unmerged(
             line.startswith("UU") and normalize_git_path(line[3:]) == filename
             for line in lines
         )
+
+
+def test_update_with_new_file_in_template_and_project(
+    tmp_path_factory: pytest.TempPathFactory,
+) -> None:
+    src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
+
+    with local.cwd(src):
+        build_file_tree(
+            {
+                "{{ _copier_conf.answers_file }}.jinja": (
+                    "{{ _copier_answers|to_yaml }}"
+                ),
+            }
+        )
+        git_init("v1")
+        git("tag", "v1")
+
+    run_copy(str(src), dst, defaults=True, overwrite=True)
+    assert "_commit: v1" in (dst / ".copier-answers.yml").read_text()
+
+    with local.cwd(dst):
+        git_init("v1")
+        Path(".gitlab-ci.yml").write_text(
+            dedent(
+                """\
+                tests:
+                    stage: test
+                    script:
+                        - ./test.sh
+
+                pages:
+                    stage: deploy
+                    script:
+                        - ./deploy.sh
+                """
+            )
+        )
+        git("add", ".")
+        git("commit", "-m", "v2")
+
+    with local.cwd(src):
+        Path(".gitlab-ci.yml.jinja").write_text(
+            dedent(
+                """\
+                tests:
+                    stage: test
+                    script:
+                        - ./test.sh --slow
+                """
+            )
+        )
+        git("add", ".")
+        git("commit", "-m", "v2")
+        git("tag", "v2")
+
+    run_update(dst_path=dst, defaults=True, overwrite=True, conflict="inline")
+    assert "_commit: v2" in (dst / ".copier-answers.yml").read_text()
+    assert (dst / ".gitlab-ci.yml").read_text() == dedent(
+        """\
+        tests:
+            stage: test
+            script:
+        <<<<<<< before updating
+                - ./test.sh
+
+        pages:
+            stage: deploy
+            script:
+                - ./deploy.sh
+        =======
+                - ./test.sh --slow
+        >>>>>>> after updating
+        """
+    )
+
+
+def test_update_with_new_file_in_template_and_project_via_migration(
+    tmp_path_factory: pytest.TempPathFactory,
+) -> None:
+    """Merge conflicts are yielded when both template and project add same file.
+
+    The project adds content to `.gitlab-ci.yml` on top of what template v1 provides.
+    In a template v2, `.gitlab-ci.yml.jinja` is moved to `.gitlab/ci/main.yml.jinja`
+    and `.gitlab-ci.yml.jinja` now includes the generated `.gitlab/ci/main.yml`. To
+    retain the project's changes/additions to `.gitlab-ci.yml`, a pre-update migration
+    task copies `.gitlab-ci.yml` (containing those changes/additions) to
+    `.gitlab/ci/main.yml` and stages it, then Copier applies template v2's version of
+    that file (which was also moved there, but Git doesn't recognize it as status `R`
+    but as `A`).
+    """
+    src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
+
+    with local.cwd(src):
+        build_file_tree(
+            {
+                "{{ _copier_conf.answers_file }}.jinja": (
+                    "{{ _copier_answers|to_yaml }}"
+                ),
+                ".gitlab-ci.yml.jinja": (
+                    """\
+                    tests:
+                        stage: test
+                        script:
+                            - ./test.sh
+                    """
+                ),
+            }
+        )
+        git_init("v1")
+        git("tag", "v1")
+
+    run_copy(str(src), dst, defaults=True, overwrite=True)
+    assert "_commit: v1" in (dst / ".copier-answers.yml").read_text()
+    assert (dst / ".gitlab-ci.yml").exists()
+
+    with local.cwd(dst):
+        with Path(".gitlab-ci.yml").open(mode="at") as f:
+            f.write(
+                dedent(
+                    """\
+
+                    pages:
+                        stage: deploy
+                        script:
+                            - ./deploy.sh
+                    """
+                )
+            )
+        git_init("v1")
+
+    with local.cwd(src):
+        old_file = Path(".gitlab-ci.yml.jinja")
+        new_file = Path(".gitlab", "ci", "main.yml.jinja")
+        new_file.parent.mkdir(parents=True)
+        # Move `.gitlab-ci.yml.jinja` to `.gitlab/ci/main.yml.jinja`
+        git("mv", old_file, new_file)
+        # Make a small modification in `.gitlab/ci/main.yml.jinja`
+        new_file.write_text(new_file.read_text().replace("test.sh", "test.sh --slow"))
+        # Include `.gitlab/ci/main.yml.jinja` in `.gitlab-ci.yml.jinja`
+        old_file.write_text(
+            dedent(
+                """\
+                include:
+                    - local: .gitlab/ci/main.yml
+                """
+            )
+        )
+        # Add a pre-migration that copies `.gitlab-ci.yml` to
+        # `.gitlab/ci/main.yml` and stages it, so that the user changes made in
+        # the project are retained after moving the file.
+        build_file_tree(
+            {
+                "copier.yml": (
+                    """\
+                    _migrations:
+                    -   version: v2
+                        before:
+                        -    "{{ _copier_python }} {{ _copier_conf.src_path / 'migrate.py' }}"
+                    """
+                ),
+                "migrate.py": (
+                    """\
+                    from pathlib import Path
+                    from plumbum.cmd import git
+
+                    f = Path(".gitlab", "ci", "main.yml")
+                    f.parent.mkdir(parents=True)
+                    f.write_text(Path(".gitlab-ci.yml").read_text())
+                    git("add", f)
+                    """
+                ),
+            }
+        )
+        git("add", ".")
+        git("commit", "-m", "v2")
+        git("tag", "v2")
+
+    run_update(
+        dst_path=dst, defaults=True, overwrite=True, conflict="inline", unsafe=True
+    )
+    assert "_commit: v2" in (dst / ".copier-answers.yml").read_text()
+    assert (dst / ".gitlab-ci.yml").read_text() == dedent(
+        """\
+        <<<<<<< before updating
+        tests:
+            stage: test
+            script:
+                - ./test.sh
+
+        pages:
+            stage: deploy
+            script:
+                - ./deploy.sh
+        =======
+        include:
+            - local: .gitlab/ci/main.yml
+        >>>>>>> after updating
+        """
+    )
+    assert (dst / ".gitlab" / "ci" / "main.yml").read_text() == dedent(
+        """\
+        tests:
+            stage: test
+            script:
+        <<<<<<< before updating
+                - ./test.sh
+
+        pages:
+            stage: deploy
+            script:
+                - ./deploy.sh
+        =======
+                - ./test.sh --slow
+        >>>>>>> after updating
+        """
+    )