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/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 edb07401..179a23b6 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), @@ -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)