project: Use dicts to keep track of copyfiles and linkfiles

This avoids copying/linking the same file/link multiple times if a
copyfile/linkfile element with the same values has been specifed
multiple times. This can happen when including a common manifest that
uses an extend-project element that has a copyfile/linkfile element.

This uses dicts rather than sets to store the copyfiles and linkfiles to
make sure the order they are specified in the manifest is maintained.
For Python 3.7+, maintaining the order that keys are added to dicts is
guaranteed, and for Python 3.6 it happened to be true.

The _CopyFile class and the _LinkFile class are changed to inherit from
NamedTuple to be able to store them in dicts.

Change-Id: I9f5a80298b875251a81c5fe7d353e262d104fae4
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/525322
Reviewed-by: Mike Frysinger <vapier@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>
Tested-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
Commit-Queue: Peter Kjellerstedt <peter.kjellerstedt@axis.com>

4 files changed, 131 insertions, 40 deletions

diff --git a/docs/manifest-format.md b/docs/manifest-format.md
index d1a11cc9..42e1ab18 100644
--- a/docs/manifest-format.md
+++ b/docs/manifest-format.md
@@ -453,10 +453,14 @@ Intermediate paths must not be symlinks either.
 
 Parent directories of "dest" will be automatically created if missing.
 
+The files are copied in the order they are specified in the manifests.
+If multiple elements specify the same source and destination, they will
+only be applied as one, based on the first occurence. Files are copied
+before any links specified via linkfile elements are created.
+
 ### Element linkfile
 
-It's just like copyfile and runs at the same time as copyfile but
-instead of copying it creates a symlink.
+It's just like copyfile, but instead of copying it creates a symlink.
 
 The symlink is created at "dest" (relative to the top of the tree) and
 points to the path specified by "src" which is a path in the project.
@@ -466,6 +470,11 @@ Parent directories of "dest" will be automatically created if missing.
 The symlink target may be a file or directory, but it may not point outside
 of the repo client.
 
+The links are created in the order they are specified in the manifests.
+If multiple elements specify the same source and destination, they will
+only be applied as one, based on the first occurence. Links are created
+after any files specified via copyfile elements are copied.
+
 ### Element remove-project
 
 Deletes a project from the internal manifest table, possibly
diff --git a/man/repo-manifest.1 b/man/repo-manifest.1
index df3943ce..1a97ff7d 100644
--- a/man/repo-manifest.1
+++ b/man/repo-manifest.1
@@ -521,10 +521,14 @@ Intermediate paths must not be symlinks either.
 .PP
 Parent directories of "dest" will be automatically created if missing.
 .PP
+The files are copied in the order they are specified in the manifests. If
+multiple elements specify the same source and destination, they will only be
+applied as one, based on the first occurence. Files are copied before any links
+specified via linkfile elements are created.
+.PP
 Element linkfile
 .PP
-It's just like copyfile and runs at the same time as copyfile but instead of
-copying it creates a symlink.
+It's just like copyfile, but instead of copying it creates a symlink.
 .PP
 The symlink is created at "dest" (relative to the top of the tree) and points to
 the path specified by "src" which is a path in the project.
@@ -534,6 +538,11 @@ Parent directories of "dest" will be automatically created if missing.
 The symlink target may be a file or directory, but it may not point outside of
 the repo client.
 .PP
+The links are created in the order they are specified in the manifests. If
+multiple elements specify the same source and destination, they will only be
+applied as one, based on the first occurence. Links are created after any files
+specified via copyfile elements are copied.
+.PP
 Element remove\-project
 .PP
 Deletes a project from the internal manifest table, possibly allowing a
diff --git a/project.py b/project.py
index 8e430175..41606442 100644
--- a/project.py
+++ b/project.py
@@ -390,22 +390,17 @@ def _SafeExpandPath(base, subpath, skipfinal=False):
     return path
 
 
-class _CopyFile:
+class _CopyFile(NamedTuple):
     """Container for <copyfile> manifest element."""
 
-    def __init__(self, git_worktree, src, topdir, dest):
-        """Register a <copyfile> request.
-
-        Args:
-            git_worktree: Absolute path to the git project checkout.
-            src: Relative path under |git_worktree| of file to read.
-            topdir: Absolute path to the top of the repo client checkout.
-            dest: Relative path under |topdir| of file to write.
-        """
-        self.git_worktree = git_worktree
-        self.topdir = topdir
-        self.src = src
-        self.dest = dest
+    # Absolute path to the git project checkout.
+    git_worktree: str
+    # Relative path under |git_worktree| of file to read.
+    src: str
+    # Absolute path to the top of the repo client checkout.
+    topdir: str
+    # Relative path under |topdir| of file to write.
+    dest: str
 
     def _Copy(self):
         src = _SafeExpandPath(self.git_worktree, self.src)
@@ -439,22 +434,17 @@ class _CopyFile:
                 logger.error("error: Cannot copy file %s to %s", src, dest)
 
 
-class _LinkFile:
+class _LinkFile(NamedTuple):
     """Container for <linkfile> manifest element."""
 
-    def __init__(self, git_worktree, src, topdir, dest):
-        """Register a <linkfile> request.
-
-        Args:
-            git_worktree: Absolute path to the git project checkout.
-            src: Target of symlink relative to path under |git_worktree|.
-            topdir: Absolute path to the top of the repo client checkout.
-            dest: Relative path under |topdir| of symlink to create.
-        """
-        self.git_worktree = git_worktree
-        self.topdir = topdir
-        self.src = src
-        self.dest = dest
+    # Absolute path to the git project checkout.
+    git_worktree: str
+    # Target of symlink relative to path under |git_worktree|.
+    src: str
+    # Absolute path to the top of the repo client checkout.
+    topdir: str
+    # Relative path under |topdir| of symlink to create.
+    dest: str
 
     def __linkIt(self, relSrc, absDest):
         # Link file if it does not exist or is out of date.
@@ -633,8 +623,9 @@ class Project:
         self.subprojects = []
 
         self.snapshots = {}
-        self.copyfiles = []
-        self.linkfiles = []
+        # Use dicts to dedupe while maintaining declared order.
+        self.copyfiles = {}
+        self.linkfiles = {}
         self.annotations = []
         self.dest_branch = dest_branch
 
@@ -1794,7 +1785,7 @@ class Project:
         Paths should have basic validation run on them before being queued.
         Further checking will be handled when the actual copy happens.
         """
-        self.copyfiles.append(_CopyFile(self.worktree, src, topdir, dest))
+        self.copyfiles[_CopyFile(self.worktree, src, topdir, dest)] = True
 
     def AddLinkFile(self, src, dest, topdir):
         """Mark |dest| to create a symlink (relative to |topdir|) pointing to
@@ -1805,7 +1796,7 @@ class Project:
         Paths should have basic validation run on them before being queued.
         Further checking will be handled when the actual link happens.
         """
-        self.linkfiles.append(_LinkFile(self.worktree, src, topdir, dest))
+        self.linkfiles[_LinkFile(self.worktree, src, topdir, dest)] = True
 
     def AddAnnotation(self, name, value, keep):
         self.annotations.append(Annotation(name, value, keep))
diff --git a/tests/test_manifest_xml.py b/tests/test_manifest_xml.py
index d4bf76a9..f5991515 100644
--- a/tests/test_manifest_xml.py
+++ b/tests/test_manifest_xml.py
@@ -1254,8 +1254,8 @@ class ExtendProjectElementTests(ManifestParseTestCase):
 </manifest>
 """
         )
-        self.assertEqual(manifest.projects[0].copyfiles[0].src, "foo")
-        self.assertEqual(manifest.projects[0].copyfiles[0].dest, "bar")
+        self.assertEqual(list(manifest.projects[0].copyfiles)[0].src, "foo")
+        self.assertEqual(list(manifest.projects[0].copyfiles)[0].dest, "bar")
         self.assertEqual(
             sort_attributes(manifest.ToXml().toxml()),
             '<?xml version="1.0" ?><manifest>'
@@ -1267,6 +1267,47 @@ class ExtendProjectElementTests(ManifestParseTestCase):
             "</manifest>",
         )
 
+    def test_extend_project_duplicate_copyfiles(self):
+        root_m = self.manifest_dir / "root.xml"
+        root_m.write_text(
+            """
+<manifest>
+  <remote name="test-remote" fetch="http://localhost" />
+  <default remote="test-remote" revision="refs/heads/main" />
+  <project name="myproject" />
+  <include name="man1.xml" />
+  <include name="man2.xml" />
+</manifest>
+"""
+        )
+        (self.manifest_dir / "man1.xml").write_text(
+            """
+<manifest>
+  <include name="common.xml" />
+</manifest>
+"""
+        )
+        (self.manifest_dir / "man2.xml").write_text(
+            """
+<manifest>
+  <include name="common.xml" />
+</manifest>
+"""
+        )
+        (self.manifest_dir / "common.xml").write_text(
+            """
+<manifest>
+  <extend-project name="myproject">
+    <copyfile dest="bar" src="foo"/>
+  </extend-project>
+</manifest>
+"""
+        )
+        manifest = manifest_xml.XmlManifest(str(self.repodir), str(root_m))
+        self.assertEqual(len(manifest.projects[0].copyfiles), 1)
+        self.assertEqual(list(manifest.projects[0].copyfiles)[0].src, "foo")
+        self.assertEqual(list(manifest.projects[0].copyfiles)[0].dest, "bar")
+
     def test_extend_project_linkfiles(self):
         manifest = self.getXmlManifest(
             """
@@ -1280,8 +1321,8 @@ class ExtendProjectElementTests(ManifestParseTestCase):
 </manifest>
 """
         )
-        self.assertEqual(manifest.projects[0].linkfiles[0].src, "foo")
-        self.assertEqual(manifest.projects[0].linkfiles[0].dest, "bar")
+        self.assertEqual(list(manifest.projects[0].linkfiles)[0].src, "foo")
+        self.assertEqual(list(manifest.projects[0].linkfiles)[0].dest, "bar")
         self.assertEqual(
             sort_attributes(manifest.ToXml().toxml()),
             '<?xml version="1.0" ?><manifest>'
@@ -1293,6 +1334,47 @@ class ExtendProjectElementTests(ManifestParseTestCase):
             "</manifest>",
         )
 
+    def test_extend_project_duplicate_linkfiles(self):
+        root_m = self.manifest_dir / "root.xml"
+        root_m.write_text(
+            """
+<manifest>
+  <remote name="test-remote" fetch="http://localhost" />
+  <default remote="test-remote" revision="refs/heads/main" />
+  <project name="myproject" />
+  <include name="man1.xml" />
+  <include name="man2.xml" />
+</manifest>
+"""
+        )
+        (self.manifest_dir / "man1.xml").write_text(
+            """
+<manifest>
+  <include name="common.xml" />
+</manifest>
+"""
+        )
+        (self.manifest_dir / "man2.xml").write_text(
+            """
+<manifest>
+  <include name="common.xml" />
+</manifest>
+"""
+        )
+        (self.manifest_dir / "common.xml").write_text(
+            """
+<manifest>
+  <extend-project name="myproject">
+    <linkfile dest="bar" src="foo"/>
+  </extend-project>
+</manifest>
+"""
+        )
+        manifest = manifest_xml.XmlManifest(str(self.repodir), str(root_m))
+        self.assertEqual(len(manifest.projects[0].linkfiles), 1)
+        self.assertEqual(list(manifest.projects[0].linkfiles)[0].src, "foo")
+        self.assertEqual(list(manifest.projects[0].linkfiles)[0].dest, "bar")
+
     def test_extend_project_annotations(self):
         manifest = self.getXmlManifest(
             """