2026 11 09 tjr authors reviewers

apis/phabricator.py

@@ -183,15 +183,16 @@ def set_reviewer(self, phab_revision, phab_username):
 
         phid = result['response']['data'][0]['phid']
 
-        cmd = "echo " + quote_echo_string("""{"transactions": [{"type":"reviewers.set", "value":["%s"]}], "objectIdentifier": "%s"}""" % (phid, phab_revision))
+        # reviewers.add, not reviewers.set: anyone already reviewing should stay a reviewer
+        cmd = "echo " + quote_echo_string("""{"transactions": [{"type":"reviewers.add", "value":["%s"]}], "objectIdentifier": "%s"}""" % (phid, phab_revision))
         cmd += " | arc call-conduit --conduit-uri=%s differential.revision.edit --""" % self.url
         ret = self.run(cmd, shell=True)
         try:
             result = json.loads(ret.stdout.decode())
         except Exception:
             raise Exception("Could not decode response as JSON: %s" % ret.stdout.decode())
         if result['error']:
-            raise Exception("Got an error from phabricator when trying to set reviewers to %s (%s) for %s: %s" % (phab_username, phid, phab_revision, result))
+            raise Exception("Got an error from phabricator when trying to add %s (%s) as a reviewer of %s: %s" % (phab_username, phid, phab_revision, result))
 
     @logEntryExit
     @retry

components/bugzilla.py

@@ -33,7 +33,7 @@ def UPDATE_DETAILS_WITHOUT_COMMITS(library, commit_stats, error):
 """ % (library.repo_url, error.strip().replace("\n", "\n> "), commit_stats)
 
     @staticmethod
-    def UPDATE_DETAILS(num_commits, num_new_commits, commit_stats, commit_details):
+    def UPDATE_DETAILS(num_commits, num_new_commits, commit_stats, commit_details, author_summary=""):
         new_commit_str = ""
         if num_new_commits != num_commits:
             if num_new_commits == 0:
@@ -48,9 +48,9 @@ def UPDATE_DETAILS(num_commits, num_new_commits, commit_stats, commit_details):
 
 ----------------------------------------
 %s
-
 %s
-""" % (num_commits, new_commit_str, ", and then the commit information" if commit_details else "", commit_stats, commit_details)
+%s
+""" % (num_commits, new_commit_str, ", and then the commit information" if commit_details else "", commit_stats, ("\n" + author_summary + "\n") if author_summary else "", commit_details)
 
     @staticmethod
     def EXAMINE_COMMITS_SUMMARY(library, new_commits):

components/commandrunner.py

@@ -55,8 +55,9 @@ def _run(args, shell, clean_return, errorlog=do_nothing, infolog=do_nothing, deb
         exception = e
     else:
         ran_to_completion = True
-        stdout = ret.stdout.decode()
-        stderr = ret.stderr.decode()
+        # Only for the log: callers get the raw output, which may not be UTF-8
+        stdout = ret.stdout.decode(errors="replace")
+        stderr = ret.stderr.decode(errors="replace")
     finally:
         if stdin_handle:
             stdin_handle.close()

components/scmprovider.py

@@ -4,6 +4,7 @@
 
 import os
 import copy
+import collections
 import shutil
 import tempfile
 import functools
@@ -56,6 +57,7 @@ def __init__(self, pretty_line):
         # populate_details. Used to build a compare URL whose range includes
         # this commit.
         self.parent_revision = None
+        self.signed = None
         self.populated = False
 
     def populate_details(self, repo, run):
@@ -84,6 +86,12 @@ def populate_details(self, repo, run):
         self.summary = run(["git", "log", "--pretty=%s", "-1", self.revision]).stdout.decode()
         self.author = run(["git", "log", "--pretty=%an <%ae>", "-1", self.revision]).stdout.decode()
         self.description = run(["git", "log", "--pretty=%b", "-1", self.revision]).stdout.decode()
+        # A signed commit (with GPG or SSH) has a gpgsig header. We can't verify the
+        # signature without the signer's key, only tell whether there is one.
+        # cat-file prints the commit as stored, which may not be UTF-8 (unlike git log,
+        # which converts it), but the header names we look for are ASCII.
+        headers = run(["git", "cat-file", "commit", self.revision]).stdout.decode("utf-8", "replace").split("\n\n", 1)[0]
+        self.signed = any(line.startswith(("gpgsig ", "gpgsig-sha256 ")) for line in headers.split("\n"))
         self.revision_link = repo_and_commit_to_url(repo, self.revision)
         self.populated = True
 
@@ -299,6 +307,33 @@ def _print_differing_commit_lists(self, list_a, list_a_name, list_b, list_b_name
 
     # =================================================================
 
+    @staticmethod
+    def build_author_summary(list_of_commits, max_authors=20):
+        """
+        A list of each author's number of commits, most first, and whether they were signed.
+        """
+        counts = collections.defaultdict(lambda: [0, 0])
+        for c in list_of_commits:
+            counts[c.author.strip()][0 if c.signed else 1] += 1
+        if not counts:
+            return ""
+
+        # We can only tell a commit has a signature, not check it
+        def signatures(signed, unsigned):
+            if signed and unsigned:
+                return "%d signed but unverified, %d unsigned" % (signed, unsigned)
+            state = "signed but unverified" if signed else "unsigned"
+            return state if signed + unsigned == 1 else "all " + state
+
+        authors = sorted(counts.items(), key=lambda a: (-sum(a[1]), a[0]))
+        lines = ["Commits by author:"]
+        for author, (signed, unsigned) in authors[:max_authors]:
+            lines.append("  %4d  %s (%s)" % (signed + unsigned, author, signatures(signed, unsigned)))
+        if len(authors) > max_authors:
+            rest = authors[max_authors:]
+            lines.append("  ... and %d more authors, with %d commits" % (len(rest), sum(sum(a[1]) for a in rest)))
+        return "\n".join(lines)
+
     def _commit_block(self, c, verbosity):
         # Render a single commit at the given verbosity:
         #   1: revision, author and the revision link

tasktypes/vendoring.py

@@ -187,10 +187,12 @@ def _process_new_job(self, library, task, new_version, timestamp, most_recent_jo
         try:
             all_upstream_commits, unseen_upstream_commits = self.scmProvider.check_for_update(library, task, new_version, most_recent_job.version if most_recent_job else None)
             if library.should_show_commit_details:
-                commit_chunks = self.scmProvider.build_bug_description(all_upstream_commits, 65534 - len(commit_stats) - 220, library.repo_url, options=task.options, new_commits=unseen_upstream_commits, compare_range=(library.revision, new_version))
+                author_summary = self.scmProvider.build_author_summary(all_upstream_commits)
+                commit_chunks = self.scmProvider.build_bug_description(all_upstream_commits, 65534 - len(commit_stats) - len(author_summary) - 220, library.repo_url, options=task.options, new_commits=unseen_upstream_commits, compare_range=(library.revision, new_version))
             else:
+                author_summary = ""
                 commit_chunks = [""]
-            bug_description = CommentTemplates.UPDATE_DETAILS(len(all_upstream_commits), len(unseen_upstream_commits), commit_stats, commit_chunks[0])
+            bug_description = CommentTemplates.UPDATE_DETAILS(len(all_upstream_commits), len(unseen_upstream_commits), commit_stats, commit_chunks[0], author_summary)
         except Exception as e:
             # Listing the upstream commits means cloning the upstream repo, which
             # fails if its host is down. Still file the bug: the job already exists,

tests/build_bug_description.py

@@ -11,6 +11,7 @@
 sys.path.append("..")
 
 from components.scmprovider import SCMProvider, Commit
+from components.utilities import Struct
 
 GITHUB_REPO = "https://github.com/org/repo"
 
@@ -143,6 +144,44 @@ def test_update_details_new_commit_counts(self):
         self.assertIn("covers 3 commits, all of which I've filed a bug on before.", CommentTemplates.UPDATE_DETAILS(3, 0, "", ""))
         self.assertNotIn("top 0", CommentTemplates.UPDATE_DETAILS(3, 0, "", ""))
 
+    def test_signed_commits_are_detected(self):
+        def cat_file(headers):
+            outputs = {"cat-file": headers + "\n\nThe message\n", "rev-parse": "p0", "diff": "", "log": ""}
+
+            def run(cmd):
+                return Struct(stdout=next(v for k, v in outputs.items() if k in cmd).encode())
+            c = Commit("abc1234|2020-01-01 00:00:00 +0000|2020-01-02 00:00:00 +0000")
+            c.populate_details(GITHUB_REPO, run)
+            return c.signed
+
+        self.assertTrue(cat_file("tree t\nauthor a <a@example.com> 1 +0000\ngpgsig -----BEGIN PGP SIGNATURE-----\n wsBc\n -----END PGP SIGNATURE-----"))
+        self.assertTrue(cat_file("tree t\ngpgsig -----BEGIN SSH SIGNATURE-----\n U1NI\n -----END SSH SIGNATURE-----"))
+        self.assertFalse(cat_file("tree t\nauthor a <a@example.com> 1 +0000\ncommitter a <a@example.com> 1 +0000"))
+
+    def test_author_summary(self):
+        def commit(author, signed):
+            c = make_commit("c" + str(len(author)) + ("1" if signed else "0"), "p")
+            c.author = author + "\n"
+            c.signed = signed
+            return c
+        commits = [commit("A Dev <a@example.com>", True)] * 3 + [commit("B Dev <b@example.com>", False)] + \
+            [commit("C Dev <c@example.com>", True), commit("C Dev <c@example.com>", False)]
+        self.assertEqual(SCMProvider.build_author_summary(commits),
+                         "Commits by author:\n"
+                         "     3  A Dev <a@example.com> (all signed but unverified)\n"
+                         "     2  C Dev <c@example.com> (1 signed but unverified, 1 unsigned)\n"
+                         "     1  B Dev <b@example.com> (unsigned)")
+        self.assertEqual(SCMProvider.build_author_summary(commits, max_authors=1).split("\n")[-1],
+                         "  ... and 2 more authors, with 3 commits")
+        self.assertEqual(SCMProvider.build_author_summary([]), "")
+
+    def test_update_details_author_summary_follows_the_stats(self):
+        from components.bugzilla import CommentTemplates
+        details = CommentTemplates.UPDATE_DETAILS(3, 3, "STATS", "COMMITS", "AUTHORS")
+        self.assertLess(details.index("STATS"), details.index("AUTHORS"))
+        self.assertLess(details.index("AUTHORS"), details.index("COMMITS"))
+        self.assertNotIn("\n\n\n", CommentTemplates.UPDATE_DETAILS(3, 3, "STATS", "COMMITS"))
+
     def test_chained_no_compare_url_for_unsupported_host(self):
         commits = self._commits()
         result = self.scm.build_bug_description(commits, 200, "https://example.com/repo", options={"verbose-diff": True})

tests/functionality_utilities.py

@@ -123,7 +123,7 @@ def patch(cmd):
         ("arc diff --verbatim", command_callbacks.get('phab_submit', default_phab_submit)),
         ("arcanist diff --verbatim", command_callbacks.get('phab_submit', default_phab_submit)),
         (echo_str("echo {\"constraints\""), lambda: CONDUIT_USERNAME_SEARCH_OUTPUT),  # This is also used for differential.revision.search, that's alright
-        (echo_str("echo {\"transactions\": [{\"type\":\"reviewers.set\""), lambda: CONDUIT_EDIT_OUTPUT),
+        (echo_str("echo {\"transactions\": [{\"type\":\"reviewers.add\""), lambda: CONDUIT_EDIT_OUTPUT),
         (echo_str("echo {\"transactions\": [{\"type\":\"abandon\""), command_callbacks.get('abandon', AssertFalse)),
         (echo_str("echo {\"transactions\": [{\"type\":\"bugzilla.bug-id\""), lambda: CONDUIT_EDIT_OUTPUT),
         (echo_str("echo {\"transactions\": [{\"type\":\"parents.add\""), lambda: CONDUIT_EDIT_OUTPUT),
@@ -137,6 +137,7 @@ def patch(cmd):
         ("git log --pretty=%s", lambda: "Roll SPIRV-Tools from a61d07a72763 to 1cda495274bb (1 revision)"),
         ("git log --pretty=%an", lambda: "Tom Ritter"),
         ("git log --pretty=%b", lambda: GIT_COMMIT_BODY),
+        ("git cat-file commit", lambda: GIT_CAT_FILE_SIGNED_COMMIT),
     ])
 
 
@@ -264,6 +265,19 @@ def TRY_OUTPUT(revision, include_auto_line=True):
 Q	src/tests/gl_tests/StateChangeTest5.cpp
 """
 
+# The headers of a signed commit, from `git cat-file commit`
+GIT_CAT_FILE_SIGNED_COMMIT = """tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
+parent abcdef1234567890abcdef1234567890abcdef12
+author Tom Ritter <tom@ritter.vg> 1612908604 -0500
+committer GitHub <noreply@github.com> 1612908604 -0500
+gpgsig -----BEGIN PGP SIGNATURE-----
+ wsBcBAABCAAQBQJgIu88CRBK7hj4Ov3rIwAAdHIIAJgf
+ =abcd
+ -----END PGP SIGNATURE-----
+
+Roll SPIRV-Tools from a61d07a72763 to 1cda495274bb (1 revision)
+"""
+
 GIT_COMMIT_BODY = """
 If glBufferSubData results in a new vk::BufferHelper allocation,
 VertexArrayVk::mCurrentElementArrayBuffer needs to be updated.

tests/mock_commandprovider.py

@@ -78,6 +78,6 @@ def check_returncode():
                 raise subprocess.CalledProcessError(returncode, argument_string)
             pass
         return Struct(**{'stdout':
-                         Struct(**{'decode': lambda: stdout}),
+                         Struct(**{'decode': lambda *args, **kwargs: stdout}),
                          'returncode': returncode,
                          'check_returncode': check_returncode})

tests/scm.py

@@ -14,7 +14,7 @@
 
 sys.path.append(".")
 sys.path.append("..")
-from components.scmprovider import SCMProvider
+from components.scmprovider import SCMProvider, Commit
 from components.logging import SimpleLogger, log
 from components.commandprovider import CommandProvider
 from components.utilities import Struct
@@ -319,5 +319,32 @@ def run(args, **kwargs):
         self.assertEqual(attempts, [[], []], "The retry should start from an empty directory")
 
 
+class TestNonUTF8Commit(unittest.TestCase):
+    def testISO88591Commit(self):
+        # git cat-file prints a commit as stored, so one in another encoding isn't UTF-8
+        runner = CommandProvider({})
+        runner.update_config({'LoggingProvider': SimpleLogger({'local': True})})
+        previous_dir = os.getcwd()
+        with tempfile.TemporaryDirectory() as repo:
+            os.chdir(repo)
+            try:
+                git = ["git", "-c", "user.name=Test", "-c", "user.email=test@example.com", "-c", "commit.gpgsign=false"]
+                subprocess.run(["git", "init", "-q"], check=True)
+                subprocess.run(git + ["commit", "-q", "--allow-empty", "-m", "First"], check=True)
+                with open("message", "wb") as f:
+                    f.write("Ajout d'une fonctionnalit\u00e9 \u00e0 la biblioth\u00e8que".encode("iso-8859-1"))
+                subprocess.run(git + ["-c", "i18n.commitEncoding=ISO-8859-1", "commit", "-q", "--allow-empty", "-F", "message"], check=True)
+                head = subprocess.run(["git", "rev-parse", "HEAD"], check=True, capture_output=True).stdout.decode().strip()
+                stored = subprocess.run(["git", "cat-file", "commit", head], check=True, capture_output=True).stdout
+                with self.assertRaises(UnicodeDecodeError, msg="The commit should be stored in ISO-8859-1"):
+                    stored.decode()
+
+                commit = Commit(head + "|2020-01-01 00:00:00 +0000|2020-01-02 00:00:00 +0000")
+                commit.populate_details("https://github.com/org/repo", runner.run)
+                self.assertFalse(commit.signed)
+            finally:
+                os.chdir(previous_dir)
+
+
 if __name__ == '__main__':
     unittest.main(verbosity=0)