Merge pull request #6013 from TaymKal/fix/conventions-renames

fix(conventions): check only changed lines in renamed files

tools/check_conventions.py

@@ -76,17 +76,23 @@ class Check:
         self.blobs = {}
         self.tree = set(self.modes)
         self.dirs = {str(parent) for path in self.tree for parent in PurePosixPath(path).parents}
-        statuses = git("diff", "--name-status", "--no-renames", "-z", self.base, head).split("\0")
-        for status, path in zip(statuses[::2], statuses[1::2]):
-            self.status[path] = status
-        for line in git("diff", "--numstat", "--no-renames", "-z", self.base, head).split("\0"):
-            if not line:
-                continue
+        diff = ("--find-renames", "-l0", self.base, head)
+        statuses = iter(git("diff", "--name-status", "-z", *diff).split("\0")[:-1])
+        for status in statuses:
+            path = next(statuses)
+            if status.startswith("R"):
+                path = next(statuses)
+            self.status[path] = status[0]
+        statistics = iter(git("diff", "--numstat", "-z", *diff).split("\0")[:-1])
+        for line in statistics:
             added, deleted, path = line.split("\t", 2)
+            if not path:
+                next(statistics)
+                path = next(statistics)
             self.numstat[path] = (added, deleted)
         paths = iter([path for path, status in self.status.items() for _ in range(2 if status == "T" else 1)])
         path, number = None, None
-        for line in git("diff", "-U0", "--no-color", "--no-renames", self.base, head).split("\n"):
+        for line in git("diff", "-U0", "--no-color", *diff).split("\n"):
             if line.startswith("diff --git "):
                 path, number = next(paths), None
             elif line.startswith("@@"):
@@ -206,11 +212,15 @@ class Check:
             suffix = PurePosixPath(path).suffix.lower()
             if self.status.get(path) != "D" and (added == "-" or suffix in IMAGES or not utf8(self.blob(path))):
                 self.report("binary", path, 0)
-            if self.status.get(path) == "A" and allowed_notes(path) is False:
+            if self.status.get(path) in ("A", "R") and allowed_notes(path) is False:
                 self.report("notes-file", path, 0)
 
     def docs(self):
-        for path, lines in self.files(lambda p: p.suffix == ".md"):
+        files = dict(self.files(lambda p: p.suffix == ".md"))
+        for path, status in self.status.items():
+            if status == "R" and not path.startswith("3rdparty/") and PurePosixPath(path).suffix == ".md":
+                files[path] = list(enumerate(self.show(path).split("\n"), 1))
+        for path, lines in files.items():
             fenced = fenced_lines(self.show(path))
             for number, text in lines:
                 name = debt_name(text) if is_debt(path) and text.startswith("- ") else None

tools/tests/test_check_conventions_comments.py

@@ -498,6 +498,80 @@ class RepositoryChecksTests(unittest.TestCase):
         check.code()
         self.assertIn(("comment", "core/example.cpp", 3, ""), check.findings)
 
+    def test_unchanged_move_preserves_existing_comments_and_extensions(self):
+        source = self.root / "core/tests/Existing.cpp"
+        source.parent.mkdir(parents=True)
+        source.write_text("// existing explanation\n__attribute__((noinline)) int Existing() { return 1; }\n", encoding="utf-8")
+        self.commit()
+        destination = self.root / "core/tests/group/Café example.cpp"
+        destination.parent.mkdir()
+        source.rename(destination)
+        (self.root / "a.bin").write_bytes(b"binary\x00data")
+        (self.root / "README.md").unlink()
+        self.commit()
+        check = Check("HEAD~1", "HEAD")
+        check.code()
+        check.repository()
+        name = destination.relative_to(self.root).as_posix()
+        self.assertEqual(check.status[name], "R")
+        self.assertEqual(check.numstat[name], ("0", "0"))
+        self.assertNotIn(name, check.added)
+        self.assertEqual(check.findings, [("binary", "a.bin", 0, "")])
+
+    def test_edited_move_checks_new_lines_at_the_destination(self):
+        source = self.root / "core/tests/Existing.cpp"
+        source.parent.mkdir(parents=True)
+        original = "// existing explanation\n__attribute__((noinline)) int Existing() { return 1; }\n"
+        original += "".join(f"int Existing{index}() {{ return {index}; }}\n" for index in range(12))
+        source.write_text(original, encoding="utf-8")
+        self.commit()
+        destination = self.root / "core/tests/group/Edited.cpp"
+        destination.parent.mkdir()
+        source.rename(destination)
+        destination.write_text(original + "// new explanation\n__declspec(noinline) int Added() { return 2; }\n", encoding="utf-8")
+        self.commit()
+        check = Check("HEAD~1", "HEAD")
+        check.code()
+        name = destination.relative_to(self.root).as_posix()
+        self.assertEqual(check.status[name], "R")
+        self.assertEqual(check.numstat[name], ("2", "0"))
+        self.assertEqual(check.findings, [("comment", name, 15, ""), ("extension", name, 16, "__declspec")])
+
+    def test_copied_code_is_still_checked_as_an_addition(self):
+        source = self.root / "core/tests/Existing.cpp"
+        source.parent.mkdir(parents=True)
+        source.write_text("// existing explanation\n__attribute__((noinline)) int Existing() { return 1; }\n", encoding="utf-8")
+        self.commit()
+        destination = self.root / "core/tests/Copy.cpp"
+        destination.write_bytes(source.read_bytes())
+        self.commit()
+        check = Check("HEAD~1", "HEAD")
+        check.code()
+        name = destination.relative_to(self.root).as_posix()
+        self.assertEqual(check.status[name], "A")
+        self.assertEqual(check.findings, [("comment", name, 1, ""), ("extension", name, 2, "__attribute__")])
+
+    def test_moving_a_document_to_a_forbidden_path_is_rejected(self):
+        (self.root / "README.md").rename(self.root / "investigation.md")
+        self.commit()
+        check = Check("HEAD~1", "HEAD")
+        check.repository()
+        self.assertEqual(check.findings, [("notes-file", "investigation.md", 0, "")])
+
+    def test_moving_a_document_rechecks_relative_links(self):
+        source = self.root / "docs/dev/guide.md"
+        source.parent.mkdir(parents=True)
+        source.write_text("[reference](reference.md)\n", encoding="utf-8")
+        (source.parent / "reference.md").write_text("Reference\n", encoding="utf-8")
+        self.commit()
+        destination = self.root / "docs/dev/group/guide.md"
+        destination.parent.mkdir()
+        source.rename(destination)
+        self.commit()
+        check = Check("HEAD~1", "HEAD")
+        check.docs()
+        self.assertEqual(check.findings, [("doc-link", "docs/dev/group/guide.md", 1, "reference.md")])
+
     def test_unicode_raw_literals_keep_git_patch_and_grep_records_intact(self):
         literals = 'R"text(foo\u2028+tail\u2029+tail)text"'
         fixtures = (