Jelajahi Sumber

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 <gavinmak@google.com>
> Commit-Queue: Joanna Wang <jojwang@chromium.org>

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 <gavinmak@google.com>
Commit-Queue: Joanna Wang <jojwang@chromium.org>
Joanna Wang 2 tahun lalu
induk
melakukan
cea35a3f77
2 mengubah file dengan 26 tambahan dan 20 penghapusan
  1. 2 0
      git_cl.py
  2. 24 20
      tests/git_cl_test.py

+ 2 - 0
git_cl.py

@@ -1571,6 +1571,8 @@ class Changelist(object):
       refspec_opts.append('notify=ALL')
       refspec_opts.append('notify=ALL')
     elif (not self.GetIssue() and options.squash and not dogfood_path):
     elif (not self.GetIssue() and options.squash and not dogfood_path):
       refspec_opts.append('wip')
       refspec_opts.append('wip')
+    else:
+      refspec_opts.append('notify=NONE')
 
 
     # TODO(tandrii): options.message should be posted as a comment if
     # TODO(tandrii): options.message should be posted as a comment if
     # --send-mail or --send-email is set on non-initial upload as Rietveld used
     # --send-mail or --send-email is set on non-initial upload as Rietveld used

+ 24 - 20
tests/git_cl_test.py

@@ -861,13 +861,16 @@ class TestGitCl(unittest.TestCase):
 
 
     metrics_arguments = []
     metrics_arguments = []
 
 
-    ref_suffix_list = []
     if notify:
     if notify:
-      ref_suffix_list.append('ready,notify=ALL')
+      ref_suffix = '%ready,notify=ALL'
       metrics_arguments += ['ready', 'notify=ALL']
       metrics_arguments += ['ready', 'notify=ALL']
-    elif not issue and squash:
-      ref_suffix_list.append('wip')
-      metrics_arguments.append('wip')
+    else:
+      if not issue and squash:
+        ref_suffix = '%wip'
+        metrics_arguments.append('wip')
+      else:
+        ref_suffix = '%notify=NONE'
+        metrics_arguments.append('notify=NONE')
 
 
     # If issue is given, then description is fetched from Gerrit instead.
     # If issue is given, then description is fetched from Gerrit instead.
     if not title:
     if not title:
@@ -881,22 +884,22 @@ class TestGitCl(unittest.TestCase):
         ]
         ]
         title = 'User input'
         title = 'User input'
     if title:
     if title:
-      ref_suffix_list.append('m=' + gerrit_util.PercentEncodeForGitRef(title))
+      ref_suffix += ',m=' + gerrit_util.PercentEncodeForGitRef(title)
       metrics_arguments.append('m')
       metrics_arguments.append('m')
 
 
     for k, v in sorted((labels or {}).items()):
     for k, v in sorted((labels or {}).items()):
-      ref_suffix_list.append('l=%s+%d' % (k, v))
+      ref_suffix += ',l=%s+%d' % (k, v)
       metrics_arguments.append('l=%s+%d' % (k, v))
       metrics_arguments.append('l=%s+%d' % (k, v))
 
 
     if short_hostname == 'chromium':
     if short_hostname == 'chromium':
       # All reviewers and ccs get into ref_suffix.
       # All reviewers and ccs get into ref_suffix.
       for r in sorted(reviewers):
       for r in sorted(reviewers):
-        ref_suffix_list.append('r=' + r)
+        ref_suffix += ',r=%s' % r
         metrics_arguments.append('r')
         metrics_arguments.append('r')
       if issue is None:
       if issue is None:
         cc += ['test-more-cc@chromium.org', 'joe@example.com']
         cc += ['test-more-cc@chromium.org', 'joe@example.com']
       for c in sorted(cc):
       for c in sorted(cc):
-        ref_suffix_list.append('cc=' + c)
+        ref_suffix += ',cc=%s' % c
         metrics_arguments.append('cc')
         metrics_arguments.append('cc')
       reviewers, cc = [], []
       reviewers, cc = [], []
     else:
     else:
@@ -912,20 +915,17 @@ class TestGitCl(unittest.TestCase):
       ]
       ]
       for r in sorted(reviewers):
       for r in sorted(reviewers):
         if r != 'bad-account-or-email':
         if r != 'bad-account-or-email':
-          ref_suffix_list.append('r=' + r)
+          ref_suffix  += ',r=%s' % r
           metrics_arguments.append('r')
           metrics_arguments.append('r')
           reviewers.remove(r)
           reviewers.remove(r)
       if issue is None:
       if issue is None:
         cc += ['joe@example.com']
         cc += ['joe@example.com']
       for c in sorted(cc):
       for c in sorted(cc):
-        ref_suffix_list.append('cc=' + c)
+        ref_suffix += ',cc=%s' % c
         metrics_arguments.append('cc')
         metrics_arguments.append('cc')
         if c in cc:
         if c in cc:
           cc.remove(c)
           cc.remove(c)
 
 
-    ref_suffix = ''
-    if ref_suffix_list:
-      ref_suffix = '%' + ','.join(ref_suffix_list)
     calls += [
     calls += [
         (
         (
             ('time.time', ),
             ('time.time', ),
@@ -1564,9 +1564,11 @@ class TestGitCl(unittest.TestCase):
     # Asserts
     # Asserts
     mockCherryPickCommit.assert_called_once_with(options,
     mockCherryPickCommit.assert_called_once_with(options,
                                                  upstream_gerrit_commit)
                                                  upstream_gerrit_commit)
-    expected_refspec = ('commit-to-push:refs/for/refs/heads/main%'
+    expected_refspec = ('commit-to-push:refs/for/refs/heads/main%notify=NONE,'
                         'm=honk_stonk,topic=circus,hashtag=cow')
                         'm=honk_stonk,topic=circus,hashtag=cow')
-    expected_refspec_opts = ['m=honk_stonk', 'topic=circus', 'hashtag=cow']
+    expected_refspec_opts = [
+        'notify=NONE', 'm=honk_stonk', 'topic=circus', 'hashtag=cow'
+    ]
     mockRunGitPush.assert_called_once_with(expected_refspec,
     mockRunGitPush.assert_called_once_with(expected_refspec,
                                            expected_refspec_opts, mock.ANY,
                                            expected_refspec_opts, mock.ANY,
                                            options.push_options)
                                            options.push_options)
@@ -1654,9 +1656,9 @@ class TestGitCl(unittest.TestCase):
                   end_commit=None)
                   end_commit=None)
     ])
     ])
 
 
-    expected_refspec = ('commit-to-push:refs/for/refs/heads/main%'
+    expected_refspec = ('commit-to-push:refs/for/refs/heads/main%notify=NONE,'
                         'topic=circus,hashtag=cow')
                         'topic=circus,hashtag=cow')
-    expected_refspec_opts = ['topic=circus', 'hashtag=cow']
+    expected_refspec_opts = ['notify=NONE', 'topic=circus', 'hashtag=cow']
     mockRunGitPush.assert_called_once_with(expected_refspec,
     mockRunGitPush.assert_called_once_with(expected_refspec,
                                            expected_refspec_opts, mock.ANY,
                                            expected_refspec_opts, mock.ANY,
                                            options.push_options)
                                            options.push_options)
@@ -1736,9 +1738,11 @@ class TestGitCl(unittest.TestCase):
             options, 'external-commit', 'external-commit', end_commit=None)
             options, 'external-commit', 'external-commit', end_commit=None)
     ])
     ])
 
 
-    expected_refspec = ('commit-to-push:refs/for/refs/heads/main%'
+    expected_refspec = ('commit-to-push:refs/for/refs/heads/main%notify=NONE,'
                         'm=honk_stonk,topic=circus,hashtag=cow')
                         'm=honk_stonk,topic=circus,hashtag=cow')
-    expected_refspec_opts = ['m=honk_stonk', 'topic=circus', 'hashtag=cow']
+    expected_refspec_opts = [
+        'notify=NONE', 'm=honk_stonk', 'topic=circus', 'hashtag=cow'
+    ]
     mockRunGitPush.assert_called_once_with(expected_refspec,
     mockRunGitPush.assert_called_once_with(expected_refspec,
                                            expected_refspec_opts, mock.ANY,
                                            expected_refspec_opts, mock.ANY,
                                            options.push_options)
                                            options.push_options)