From 0ec9d15571fb274b817e3f0336a78bfd35b75e50 Mon Sep 17 00:00:00 2001 From: Andrii Shyshkalov Date: Thu, 23 Aug 2018 00:22:58 +0000 Subject: [PATCH] git cl: use project~number on Gerrit for better routing when setting reviewers R=ehmaldonado@chromium.org Testing patched my own depot_tools in $PATH and uploaded https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1186072 which set reviewers and ccs as expected. Bug: 876910 Change-Id: I43c0f2284941cf703133bb51132226d4a0472d8e Reviewed-on: https://chromium-review.googlesource.com/1186068 Commit-Queue: Andrii Shyshkalov Reviewed-by: Edward Lesmes --- gerrit_util.py | 11 +++++++++ git_cl.py | 31 ++++++++++++++++++++---- tests/git_cl_test.py | 56 ++++++++++++++++++++++++-------------------- 3 files changed, 68 insertions(+), 30 deletions(-) diff --git a/gerrit_util.py b/gerrit_util.py index d5780adce..610d9b30b 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -900,3 +900,14 @@ def tempdir(): finally: if tdir: gclient_utils.rmtree(tdir) + + +def ChangeIdentifier(project, change_number): + """Returns change identifier "project~number" suitable for |chagne| arg of + this module API. + + Such format is allows for more efficient Gerrit routing of HTTP requests, + comparing to specifying just change_number. + """ + assert int(change_number) + return '%s~%s' % (urllib.quote(project, safe=''), change_number) diff --git a/git_cl.py b/git_cl.py index 43be2313f..1d2904767 100755 --- a/git_cl.py +++ b/git_cl.py @@ -2399,6 +2399,15 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): self._gerrit_server = 'https://%s' % self._gerrit_host return self._gerrit_server + def _GetGerritProject(self, remote_url=None): + """Returns Gerrit project name based on remote git URL.""" + if remote_url is None: + remote_url = self.GetRemoteUrl() + project = urlparse.urlparse(remote_url).path.strip('/') + if project.endswith('.git'): + project = project[:-len('.git')] + return project + @classmethod def IssueConfigKey(cls): return 'gerritissue' @@ -3075,9 +3084,13 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): 'spaces not allowed in refspec: "%s"' % refspec_suffix) refspec = '%s:refs/for/%s%s' % (ref_to_push, branch, refspec_suffix) + # TODO(tandrii): hack to avoid messing with tests while resolving + # https://crbug.com/876910. Instead, remote_url should be cached inside this + # class, just like GetIssue caches issue. + remote_url = self.GetRemoteUrl() try: push_stdout = gclient_utils.CheckCallAndFilter( - ['git', 'push', self.GetRemoteUrl(), refspec], + ['git', 'push', remote_url, refspec], print_stdout=True, # Flush after every line: useful for seeing progress when running as # recipe. @@ -3115,9 +3128,15 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): if change_desc.get_cced(): cc.extend(change_desc.get_cced()) - gerrit_util.AddReviewers( - self._GetGerritHost(), self.GetIssue(), reviewers, cc, - notify=bool(options.send_mail)) + if self.GetIssue(): + # GetIssue() is not set in case of non-squash uploads according to tests. + # TODO(agable): non-squash uploads in git cl should be removed. + gerrit_util.AddReviewers( + self._GetGerritHost(), + gerrit_util.ChangeIdentifier( + self._GetGerritProject(remote_url), self.GetIssue()), + reviewers, cc, + notify=bool(options.send_mail)) if change_desc.get_reviewers(tbr_only=True): labels = self._GetChangeDetail(['LABELS']).get('labels', {}) @@ -3126,7 +3145,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): score = max([int(x) for x in labels['Code-Review']['values'].keys()]) print('Adding self-LGTM (Code-Review +%d) because of TBRs.' % score) gerrit_util.SetReview( - self._GetGerritHost(), self.GetIssue(), + self._GetGerritHost(), + gerrit_util.ChangeIdentifier( + self._GetGerritProject(remote_url), self.GetIssue()), msg='Self-approving for TBR', labels={'Code-Review': score}) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 3bc4495ce..524ec140c 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -1311,25 +1311,25 @@ class TestGitCl(TestCase): 'https://chromium.googlesource.com/yyy/zzz'), ] - calls += [ - ((['git', 'push', - 'https://chromium.googlesource.com/yyy/zzz', - ref_to_push + ':refs/for/refs/heads/master' + ref_suffix],), - ('remote:\n' - 'remote: Processing changes: (\)\n' - 'remote: Processing changes: (|)\n' - 'remote: Processing changes: (/)\n' - 'remote: Processing changes: (-)\n' - 'remote: Processing changes: new: 1 (/)\n' - 'remote: Processing changes: new: 1, done\n' - 'remote:\n' - 'remote: New Changes:\n' - 'remote: https://chromium-review.googlesource.com/#/c/foo/+/123456 ' - 'XXX\n' - 'remote:\n' - 'To https://chromium.googlesource.com/yyy/zzz\n' - ' * [new branch] hhhh -> refs/for/refs/heads/master\n')), - ] + calls.append(( + (['git', 'push', + 'https://chromium.googlesource.com/yyy/zzz', + ref_to_push + ':refs/for/refs/heads/master' + ref_suffix],), + ('remote:\n' + 'remote: Processing changes: (\)\n' + 'remote: Processing changes: (|)\n' + 'remote: Processing changes: (/)\n' + 'remote: Processing changes: (-)\n' + 'remote: Processing changes: new: 1 (/)\n' + 'remote: Processing changes: new: 1, done\n' + 'remote:\n' + 'remote: New Changes:\n' + 'remote: https://chromium-review.googlesource.com/#/c/yyy/zzz/+/123456' + ' XXX\n' + 'remote:\n' + 'To https://chromium.googlesource.com/yyy/zzz\n' + ' * [new branch] hhhh -> refs/for/refs/heads/master\n') + )) if squash: calls += [ ((['git', 'config', 'branch.master.gerritissue', '123456'],), @@ -1341,11 +1341,15 @@ class TestGitCl(TestCase): ] calls += [ ((['git', 'config', 'rietveld.cc'],), ''), - (('AddReviewers', 'chromium-review.googlesource.com', 123456 - if squash else None, sorted(reviewers), - ['joe@example.com', 'chromium-reviews+test-more-cc@chromium.org'] + - cc, notify), ''), ] + if squash: + calls += [ + (('AddReviewers', + 'chromium-review.googlesource.com', 'yyy%2Fzzz~123456', + sorted(reviewers), + ['joe@example.com', 'chromium-reviews+test-more-cc@chromium.org'] + + cc, notify), ''), + ] if tbr: calls += [ (('GetChangeDetail', 'chromium-review.googlesource.com', '123456', @@ -1363,8 +1367,10 @@ class TestGitCl(TestCase): } } }), - (('SetReview', 'chromium-review.googlesource.com', - 123456 if squash else None, 'Self-approving for TBR', + (('SetReview', + 'chromium-review.googlesource.com', + 'yyy%2Fzzz~123456', + 'Self-approving for TBR', {'Code-Review': 2}, None), ''), ] calls += cls._git_post_upload_calls()