From 5e00be8c5f4d680fbe51c93e9918a30a59d05ecf Mon Sep 17 00:00:00 2001 From: Tom Ritter Date: Mon, 9 Nov 2026 20:03:19 +0000 Subject: [PATCH 1/2] Add the maintainer as a reviewer instead of replacing the reviewers. Fixes #419 Setting the reviewer with reviewers.set removed anyone who was already reviewing the revision, such as someone added while the try run was going. --- apis/phabricator.py | 5 +++-- tests/functionality_utilities.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/apis/phabricator.py b/apis/phabricator.py index 8921f3eb..93e1e50b 100644 --- a/apis/phabricator.py +++ b/apis/phabricator.py @@ -183,7 +183,8 @@ 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: @@ -191,7 +192,7 @@ def set_reviewer(self, phab_revision, phab_username): 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 diff --git a/tests/functionality_utilities.py b/tests/functionality_utilities.py index edb07401..fea4c0f2 100644 --- a/tests/functionality_utilities.py +++ b/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), From 79a30e389d4fc3127e2ca99e3657e55cba48d961 Mon Sep 17 00:00:00 2001 From: Tom Ritter Date: Mon, 9 Nov 2026 20:03:19 +0000 Subject: [PATCH 2/2] Summarize the commits by author, and whether they were signed, in the bug description. Fixes #362 After the diff statistics, list each author's number of commits, most first, and whether their commits were signed (with GPG or SSH). Updatebot can't verify the signatures without the signers' keys, only tell whether there is one. --- components/bugzilla.py | 6 ++--- components/commandrunner.py | 5 ++-- components/scmprovider.py | 35 ++++++++++++++++++++++++++++ tasktypes/vendoring.py | 6 +++-- tests/build_bug_description.py | 39 ++++++++++++++++++++++++++++++++ tests/functionality_utilities.py | 14 ++++++++++++ tests/mock_commandprovider.py | 2 +- tests/scm.py | 29 +++++++++++++++++++++++- 8 files changed, 127 insertions(+), 9 deletions(-) diff --git a/components/bugzilla.py b/components/bugzilla.py index 5c81d693..9b9144bf 100644 --- a/components/bugzilla.py +++ b/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): diff --git a/components/commandrunner.py b/components/commandrunner.py index d7110142..d0e9c26a 100644 --- a/components/commandrunner.py +++ b/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() diff --git a/components/scmprovider.py b/components/scmprovider.py index efedc4ee..f6ab5372 100644 --- a/components/scmprovider.py +++ b/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 diff --git a/tasktypes/vendoring.py b/tasktypes/vendoring.py index 1135ef6e..90c9f57e 100644 --- a/tasktypes/vendoring.py +++ b/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, diff --git a/tests/build_bug_description.py b/tests/build_bug_description.py index 841809fc..f34a1a68 100644 --- a/tests/build_bug_description.py +++ b/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 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 1 +0000\ncommitter a 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 ", True)] * 3 + [commit("B Dev ", False)] + \ + [commit("C Dev ", True), commit("C Dev ", False)] + self.assertEqual(SCMProvider.build_author_summary(commits), + "Commits by author:\n" + " 3 A Dev (all signed but unverified)\n" + " 2 C Dev (1 signed but unverified, 1 unsigned)\n" + " 1 B Dev (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}) diff --git a/tests/functionality_utilities.py b/tests/functionality_utilities.py index fea4c0f2..179a23b6 100644 --- a/tests/functionality_utilities.py +++ b/tests/functionality_utilities.py @@ -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 1612908604 -0500 +committer GitHub 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. diff --git a/tests/mock_commandprovider.py b/tests/mock_commandprovider.py index 9d06f3ce..e646b818 100644 --- a/tests/mock_commandprovider.py +++ b/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}) diff --git a/tests/scm.py b/tests/scm.py index cf2c22d1..cd0e8cf7 100755 --- a/tests/scm.py +++ b/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)