From 56ee228caf34fcc21b7c3180135e595c4b492070 Mon Sep 17 00:00:00 2001 From: Joanna Wang Date: Fri, 8 Sep 2023 15:43:32 +0000 Subject: [PATCH] Reland "Remove unnecessary notify=None from git cl upload." This reverts commit cea35a3f771ad3c22a35be25b88aa3163a9457cc. Reason for revert: b/298657697 is unrelated to this Original change's description: > Revert "Remove unnecessary notify=None from git cl upload." > > This reverts commit 7688e784503525b598a18991e190038381c333cf. > > Reason for revert: afaict there is a gerrit notifications bug b/297928626 unrelated to this change but i'm reverting this anyway and relanding when the gerrit bug is resolved. > > Original change's description: > > Remove unnecessary notify=None from git cl upload. > > > > This is a NOOP change for all cases except for "Publish comments on push" which gets fixed with this CL. > > Our hosts have the notify on each patchset turned off. > > > > Bug: 1472724 > > Change-Id: I3672c383b1e4ca1f6243c9b9d2c906473f5037d9 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4797981 > > Reviewed-by: Gavin Mak > > Commit-Queue: Joanna Wang > > Bug: 1472724 > Change-Id: I3de1a25fe84a21a2adb8b4bbddb460dd45530418 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4817767 > Reviewed-by: Gavin Mak > Commit-Queue: Joanna Wang Bug: 1472724 Change-Id: I63cdb99111ae89eb6edd5172d827f84b314c015e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4852217 Commit-Queue: Joanna Wang Reviewed-by: Gavin Mak --- git_cl.py | 2 -- tests/git_cl_test.py | 55 +++++++++++++++++++------------------------- 2 files changed, 24 insertions(+), 33 deletions(-) diff --git a/git_cl.py b/git_cl.py index 932b89f2e..c558bac09 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1870,8 +1870,6 @@ class Changelist(object): refspec_opts.append('notify=ALL') elif (not self.GetIssue() and options.squash and not dogfood_path): refspec_opts.append('wip') - else: - refspec_opts.append('notify=NONE') # TODO(tandrii): options.message should be posted as a comment if # --send-mail or --send-email is set on non-initial upload as Rietveld diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 230c09743..1a146bd44 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -904,17 +904,13 @@ class TestGitCl(unittest.TestCase): ] metrics_arguments = [] - + ref_suffix_list = [] if notify: - ref_suffix = '%ready,notify=ALL' + ref_suffix_list.append('ready,notify=ALL') metrics_arguments += ['ready', 'notify=ALL'] - else: - if not issue and squash: - ref_suffix = '%wip' - metrics_arguments.append('wip') - else: - ref_suffix = '%notify=NONE' - metrics_arguments.append('notify=NONE') + elif not issue and squash: + ref_suffix_list.append('wip') + metrics_arguments.append('wip') # If issue is given, then description is fetched from Gerrit instead. if not title: @@ -928,22 +924,23 @@ class TestGitCl(unittest.TestCase): ] title = 'User input' if title: - ref_suffix += ',m=' + gerrit_util.PercentEncodeForGitRef(title) + ref_suffix_list.append('m=' + + gerrit_util.PercentEncodeForGitRef(title)) metrics_arguments.append('m') for k, v in sorted((labels or {}).items()): - ref_suffix += ',l=%s+%d' % (k, v) + ref_suffix_list.append('l=%s+%d' % (k, v)) metrics_arguments.append('l=%s+%d' % (k, v)) if short_hostname == 'chromium': # All reviewers and ccs get into ref_suffix. for r in sorted(reviewers): - ref_suffix += ',r=%s' % r + ref_suffix_list.append('r=%s' % r) metrics_arguments.append('r') if issue is None: cc += ['test-more-cc@chromium.org', 'joe@example.com'] for c in sorted(cc): - ref_suffix += ',cc=%s' % c + ref_suffix_list.append('cc=%s' % c) metrics_arguments.append('cc') reviewers, cc = [], [] else: @@ -960,17 +957,20 @@ class TestGitCl(unittest.TestCase): })] for r in sorted(reviewers): if r != 'bad-account-or-email': - ref_suffix += ',r=%s' % r + ref_suffix_list.append('r=%s' % r) metrics_arguments.append('r') reviewers.remove(r) if issue is None: cc += ['joe@example.com'] for c in sorted(cc): - ref_suffix += ',cc=%s' % c + ref_suffix_list.append('cc=%s' % c) metrics_arguments.append('cc') if c in cc: cc.remove(c) + ref_suffix = '' + if ref_suffix_list: + ref_suffix = '%' + ','.join(ref_suffix_list) calls += [ ( ('time.time', ), @@ -1586,12 +1586,9 @@ class TestGitCl(unittest.TestCase): # Asserts mockCherryPickCommit.assert_called_once_with(options, upstream_gerrit_commit) - expected_refspec = ( - 'commit-to-push:refs/for/refs/heads/main%notify=NONE,' - 'm=honk_stonk,topic=circus,hashtag=cow') - expected_refspec_opts = [ - 'notify=NONE', 'm=honk_stonk', 'topic=circus', 'hashtag=cow' - ] + expected_refspec = ('commit-to-push:refs/for/refs/heads/main%' + 'm=honk_stonk,topic=circus,hashtag=cow') + expected_refspec_opts = ['m=honk_stonk', 'topic=circus', 'hashtag=cow'] mockRunGitPush.assert_called_once_with(expected_refspec, expected_refspec_opts, mock.ANY, options.push_options) @@ -1685,10 +1682,9 @@ class TestGitCl(unittest.TestCase): end_commit=None) ]) - expected_refspec = ( - 'commit-to-push:refs/for/refs/heads/main%notify=NONE,' - 'topic=circus,hashtag=cow') - expected_refspec_opts = ['notify=NONE', 'topic=circus', 'hashtag=cow'] + expected_refspec = ('commit-to-push:refs/for/refs/heads/main%' + 'topic=circus,hashtag=cow') + expected_refspec_opts = ['topic=circus', 'hashtag=cow'] mockRunGitPush.assert_called_once_with(expected_refspec, expected_refspec_opts, mock.ANY, options.push_options) @@ -1770,12 +1766,9 @@ class TestGitCl(unittest.TestCase): options, 'external-commit', 'external-commit', end_commit=None) ]) - expected_refspec = ( - 'commit-to-push:refs/for/refs/heads/main%notify=NONE,' - 'm=honk_stonk,topic=circus,hashtag=cow') - expected_refspec_opts = [ - 'notify=NONE', 'm=honk_stonk', 'topic=circus', 'hashtag=cow' - ] + expected_refspec = ('commit-to-push:refs/for/refs/heads/main%' + 'm=honk_stonk,topic=circus,hashtag=cow') + expected_refspec_opts = ['m=honk_stonk', 'topic=circus', 'hashtag=cow'] mockRunGitPush.assert_called_once_with(expected_refspec, expected_refspec_opts, mock.ANY, options.push_options)