Remove manual --tbr support and convert it into automatic TBR= detection.

It result in less code, forces uploading the change for eventual review and most
importantly enables the commit queue to correctly handle this.

BUG=
TEST=

Review URL: http://codereview.chromium.org/7253015

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@91575 0039d316-1c4b-4281-b951-d872f2087c98

gcl.py

@@ -1234,7 +1234,6 @@ def DoPresubmitChecks(change_info, committing, may_prompt):
       input_stream=sys.stdin,
       default_presubmit=root_presubmit,
       may_prompt=may_prompt,
-      tbr=False,
       rietveld_obj=change_info.RpcServer())
   if not output.should_continue() and may_prompt:
     # TODO(dpranke): move into DoPresubmitChecks(), unify cmd line args.

git_cl.py

@@ -510,8 +510,7 @@ or verify this branch is set up to track another (via the --track argument to
       self.SetPatchset(0)
     self.has_issue = False
 
-  def RunHook(self, committing, upstream_branch, tbr, may_prompt, verbose,
-      author):
+  def RunHook(self, committing, upstream_branch, may_prompt, verbose, author):
     """Calls sys.exit() if the hook fails; returns a HookResults otherwise."""
     root = RunCommand(['git', 'rev-parse', '--show-cdup']).strip() or '.'
     absroot = os.path.abspath(root)
@@ -552,7 +551,7 @@ or verify this branch is set up to track another (via the --track argument to
     try:
       output = presubmit_support.DoPresubmitChecks(change, committing,
           verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin,
-          default_presubmit=None, may_prompt=may_prompt, tbr=tbr,
+          default_presubmit=None, may_prompt=may_prompt,
           rietveld_obj=self.RpcServer())
     except presubmit_support.PresubmitFailure, e:
       DieWithError(
@@ -897,7 +896,7 @@ def CMDpresubmit(parser, args):
     base_branch = cl.GetUpstreamBranch()
 
   cl.RunHook(committing=not options.upload, upstream_branch=base_branch,
-             tbr=False, may_prompt=False, verbose=options.verbose,
+             may_prompt=False, verbose=options.verbose,
              author=None)
   return 0
 
@@ -943,7 +942,7 @@ def CMDupload(parser, args):
 
   if not options.bypass_hooks and not options.force:
     hook_results = cl.RunHook(committing=False, upstream_branch=base_branch,
-                              tbr=False, may_prompt=True,
+                              may_prompt=True,
                               verbose=options.verbose,
                               author=None)
     if not options.reviewers and hook_results.reviewers:
@@ -1058,9 +1057,6 @@ def SendUpstream(parser, args, cmd):
                     help="external contributor for patch (appended to " +
                          "description and used as author for git). Should be " +
                          "formatted as 'First Last <email@example.com>'")
-  parser.add_option('--tbr', action='store_true', dest='tbr',
-                    help="short for 'to be reviewed', commit branch " +
-                         "even without uploading for review")
   (options, args) = parser.parse_args(args)
   cl = Changelist()
 
@@ -1100,7 +1096,7 @@ def SendUpstream(parser, args, cmd):
 
   if not options.bypass_hooks and not options.force:
     cl.RunHook(committing=True, upstream_branch=base_branch,
-               tbr=options.tbr, may_prompt=True, verbose=options.verbose,
+               may_prompt=True, verbose=options.verbose,
                author=options.contributor)
 
     if cmd == 'dcommit':
@@ -1115,45 +1111,16 @@ def SendUpstream(parser, args, cmd):
                'use "git cl dcommit -f" to commit on a closed tree.')
 
   description = options.message
-  if not options.tbr:
-    # It is important to have these checks early.  Not only for user
-    # convenience, but also because the cl object then caches the correct values
-    # of these fields even as we're juggling branches for setting up the commit.
-    if not cl.GetIssue():
-      print 'Current issue unknown -- has this branch been uploaded?'
-      print 'Use --tbr to commit without review.'
-      return 1
+  if not description and cl.GetIssue():
+    description = cl.GetDescription()
 
-    if not description:
-      description = cl.GetDescription()
-
-    if not description:
-      print 'No description set.'
-      print 'Visit %s/edit to set it.' % (cl.GetIssueURL())
-      return 1
+  if not description:
+    print 'No description set.'
+    print 'Visit %s/edit to set it.' % (cl.GetIssueURL())
+    return 1
 
+  if cl.GetIssue():
     description += "\n\nReview URL: %s" % cl.GetIssueURL()
-  else:
-    if not description:
-      # Submitting TBR.  See if there's already a description in Rietveld, else
-      # create a template description. Eitherway, give the user a chance to edit
-      # it to fill in the TBR= field.
-      if cl.GetIssue():
-        description = cl.GetDescription()
-
-      # TODO(dpranke): Update to use ChangeDescription object.
-      if not description:
-        description = """# Enter a description of the change.
-# This will be used as the change log for the commit.
-
-"""
-        description += CreateDescriptionFromLog(args)
-
-      description = UserEditedLog(description + '\nTBR=')
-
-    if not description:
-      print "Description empty; aborting."
-      return 1
 
   if options.contributor:
     if not re.match('^.*\s<\S+@\S+>$', options.contributor):

presubmit_canned_checks.py

@@ -63,6 +63,16 @@ def CheckChangeHasDescription(input_api, output_api):
       return [output_api.PresubmitNotifyResult('Add a description.')]
   return []
 
+
+def CheckChangeWasUploaded(input_api, output_api):
+  """Checks that the issue was uploaded before committing."""
+  if (input_api.is_committing and
+      (not input_api.change.issue or not input_api.change.patchset)):
+    return [output_api.PresubmitError(
+      'Issue wasn\'t uploaded. Please upload first.')]
+  return []
+
+
 ### Content checks
 
 def CheckDoNotSubmitInFiles(input_api, output_api):
@@ -929,5 +939,8 @@ def PanProjectChecks(input_api, output_api,
     snapshot("checking license")
     results.extend(input_api.canned_checks.CheckLicense(
         input_api, output_api, license_header, source_file_filter=sources))
+    snapshot("checking was uploaded")
+    results.extend(input_api.canned_checks.CheckChangeWasUploaded(
+        input_api, output_api))
   snapshot("done")
   return results

presubmit_support.py

@@ -217,7 +217,7 @@ class InputApi(object):
       r"(|.*[\\\/])\.svn[\\\/].*",
   )
 
-  def __init__(self, change, presubmit_path, is_committing, tbr,
+  def __init__(self, change, presubmit_path, is_committing,
       rietveld_obj, verbose):
     """Builds an InputApi object.
 
@@ -225,14 +225,12 @@ class InputApi(object):
       change: A presubmit.Change object.
       presubmit_path: The path to the presubmit script being processed.
       is_committing: True if the change is about to be committed.
-      tbr: True if '--tbr' was passed to skip any reviewer/owner checks
       rietveld_obj: rietveld.Rietveld client object
     """
     # Version number of the presubmit_support script.
     self.version = [int(x) for x in __version__.split('.')]
     self.change = change
     self.is_committing = is_committing
-    self.tbr = tbr
     self.rietveld = rietveld_obj
     # TBD
     self.host_url = 'http://codereview.chromium.org'
@@ -417,6 +415,11 @@ class InputApi(object):
       raise IOError('Access outside the repository root is denied.')
     return gclient_utils.FileRead(file_item, mode)
 
+  @property
+  def tbr(self):
+    """Returns if a change is TBR'ed."""
+    return 'TBR' in self.change.tags
+
 
 class AffectedFile(object):
   """Representation of a file in a change."""
@@ -957,17 +960,15 @@ def DoGetTrySlaves(changed_files,
 
 
 class PresubmitExecuter(object):
-  def __init__(self, change, committing, tbr, rietveld_obj, verbose):
+  def __init__(self, change, committing, rietveld_obj, verbose):
     """
     Args:
       change: The Change object.
       committing: True if 'gcl commit' is running, False if 'gcl upload' is.
-      tbr: True if '--tbr' was passed to skip any reviewer/owner checks
       rietveld_obj: rietveld.Rietveld client object.
     """
     self.change = change
     self.committing = committing
-    self.tbr = tbr
     self.rietveld = rietveld_obj
     self.verbose = verbose
 
@@ -989,7 +990,7 @@ class PresubmitExecuter(object):
 
     # Load the presubmit script into context.
     input_api = InputApi(self.change, presubmit_path, self.committing,
-                         self.tbr, self.rietveld, self.verbose)
+                         self.rietveld, self.verbose)
     context = {}
     try:
       exec script_text in context
@@ -1031,7 +1032,6 @@ def DoPresubmitChecks(change,
                       input_stream,
                       default_presubmit,
                       may_prompt,
-                      tbr,
                       rietveld_obj):
   """Runs all presubmit checks that apply to the files in the change.
 
@@ -1050,7 +1050,6 @@ def DoPresubmitChecks(change,
     input_stream: A stream to read input from the user.
     default_presubmit: A default presubmit script to execute in any case.
     may_prompt: Enable (y/n) questions on warning or error.
-    tbr: was --tbr specified to skip any reviewer/owner checks?
     rietveld_obj: rietveld.Rietveld object.
 
   Warning:
@@ -1078,7 +1077,7 @@ def DoPresubmitChecks(change,
     if not presubmit_files and verbose:
       output.write("Warning, no presubmit.py found.\n")
     results = []
-    executer = PresubmitExecuter(change, committing, tbr, rietveld_obj, verbose)
+    executer = PresubmitExecuter(change, committing, rietveld_obj, verbose)
     if default_presubmit:
       if verbose:
         output.write("Running default presubmit script.\n")
@@ -1242,7 +1241,6 @@ def Main(argv):
         sys.stdin,
         options.default_presubmit,
         options.may_prompt,
-        False,
         rietveld_obj)
     return not results.should_continue()
   except PresubmitFailure, e:

tests/post-dcommit-hook-test.sh

@@ -20,10 +20,11 @@ _EOF
   git config rietveld.server localhost:1
   git checkout -q --track -b work
   echo "some work done" >> test
-  git add test; git commit -q -m "work"
+  git add test; git commit -q -m "work \
+TBR=foo"
 
   test_expect_success "dcommitted code" \
-      "$GIT_CL dcommit -f --tbr --bypass-hooks -m 'dcommit'"
+      "$GIT_CL dcommit -f --bypass-hooks -m 'dcommit'"
 
   test_expect_success "post-cl-dcommit hook executed" \
       "git symbolic-ref HEAD | grep -q COMMITTED"

tests/presubmit_unittest.py

@@ -368,7 +368,7 @@ class PresubmitUnittest(PresubmitTestsBase):
         0,
         0,
         None)
-    executer = presubmit.PresubmitExecuter(change, False, False, None, False)
+    executer = presubmit.PresubmitExecuter(change, False, None, False)
     self.failIf(executer.ExecPresubmitScript('', fake_presubmit))
     # No error if no on-upload entry point
     self.failIf(executer.ExecPresubmitScript(
@@ -377,7 +377,7 @@ class PresubmitUnittest(PresubmitTestsBase):
       fake_presubmit
     ))
 
-    executer = presubmit.PresubmitExecuter(change, True, False, None, False)
+    executer = presubmit.PresubmitExecuter(change, True, None, False)
     # No error if no on-commit entry point
     self.failIf(executer.ExecPresubmitScript(
       ('def CheckChangeOnUpload(input_api, output_api):\n'
@@ -447,7 +447,7 @@ class PresubmitUnittest(PresubmitTestsBase):
         0,
         None)
     output = presubmit.DoPresubmitChecks(
-        change, False, True, None, input_buf, None, False, False, None)
+        change, False, True, None, input_buf, None, False, None)
     self.failIf(output.should_continue())
     self.assertEqual(output.getvalue().count('!!'), 2)
     self.assertEqual(output.getvalue().count(
@@ -487,13 +487,13 @@ class PresubmitUnittest(PresubmitTestsBase):
         0,
         None)
     output = presubmit.DoPresubmitChecks(
-        change, False, True, None, input_buf, None, True, False, None)
+        change, False, True, None, input_buf, None, True, None)
     self.failIf(output.should_continue())
     self.assertEqual(output.getvalue().count('??'), 2)
 
     input_buf = StringIO.StringIO('y\n')  # say yes to the warning
     output = presubmit.DoPresubmitChecks(
-        change, False, True, None, input_buf, None, True, False, None)
+        change, False, True, None, input_buf, None, True, None)
     self.failUnless(output.should_continue())
     self.assertEquals(output.getvalue().count('??'), 2)
     self.assertEqual(output.getvalue().count(
@@ -532,7 +532,7 @@ class PresubmitUnittest(PresubmitTestsBase):
         0,
         None)
     output = presubmit.DoPresubmitChecks(change, False, True, None, None,
-        None, False, False, None)
+        None, False, None)
     self.assertEqual(output.getvalue().count('??'), 2)
     self.assertEqual(output.getvalue().count('XX!!XX'), 2)
     self.assertEqual(output.getvalue().count('(y/N)'), 0)
@@ -575,8 +575,7 @@ def CheckChangeOnCommit(input_api, output_api):
         0,
         None)
     output = presubmit.DoPresubmitChecks(
-        change, False, True, None, input_buf, DEFAULT_SCRIPT, False, False,
-        None)
+        change, False, True, None, input_buf, DEFAULT_SCRIPT, False, None)
     self.failIf(output.should_continue())
     text = ('Running presubmit upload checks ...\n'
             'Warning, no presubmit.py found.\n'
@@ -656,8 +655,7 @@ def CheckChangeOnCommit(input_api, output_api):
         0,
         None)
     self.failUnless(presubmit.DoPresubmitChecks(
-        change, False, True, output, input_buf, DEFAULT_SCRIPT, False, False,
-        None))
+        change, False, True, output, input_buf, DEFAULT_SCRIPT, False, None))
     self.assertEquals(output.getvalue(),
                       ('Running presubmit upload checks ...\n'
                        'Warning, no presubmit.py found.\n'
@@ -755,7 +753,7 @@ def CheckChangeOnCommit(input_api, output_api):
     presubmit.DoPresubmitChecks(mox.IgnoreArg(), False, False,
                                 mox.IgnoreArg(),
                                 mox.IgnoreArg(),
-                                None, False, False, None).AndReturn(output)
+                                None, False, None).AndReturn(output)
     self.mox.ReplayAll()
 
     self.assertEquals(
@@ -804,7 +802,7 @@ class InputApiUnittest(PresubmitTestsBase):
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(
-        presubmit.InputApi(self.fake_change, './.', False, False, None, False),
+        presubmit.InputApi(self.fake_change, './.', False, None, False),
         members)
 
   def testDepotToLocalPath(self):
@@ -814,11 +812,11 @@ class InputApiUnittest(PresubmitTestsBase):
     self.mox.ReplayAll()
 
     path = presubmit.InputApi(
-        self.fake_change, './p', False, False, None, False).DepotToLocalPath(
+        self.fake_change, './p', False, None, False).DepotToLocalPath(
             'svn://foo/smurf')
     self.failUnless(path == 'prout')
     path = presubmit.InputApi(
-        self.fake_change, './p', False, False, None, False).DepotToLocalPath(
+        self.fake_change, './p', False, None, False).DepotToLocalPath(
             'svn:/foo/notfound/burp')
     self.failUnless(path == None)
 
@@ -827,11 +825,11 @@ class InputApiUnittest(PresubmitTestsBase):
     presubmit.scm.SVN.CaptureInfo('notfound-food').AndReturn({})
     self.mox.ReplayAll()
     path = presubmit.InputApi(
-        self.fake_change, './p', False, False, None, False).LocalToDepotPath(
+        self.fake_change, './p', False, None, False).LocalToDepotPath(
             'smurf')
     self.assertEqual(path, 'svn://foo')
     path = presubmit.InputApi(
-        self.fake_change, './p', False, False, None, False).LocalToDepotPath(
+        self.fake_change, './p', False, None, False).LocalToDepotPath(
             'notfound-food')
     self.assertEquals(path, None)
 
@@ -840,7 +838,7 @@ class InputApiUnittest(PresubmitTestsBase):
     api = presubmit.InputApi(
         self.fake_change,
         presubmit_path='foo/path/PRESUBMIT.py',
-        is_committing=False, tbr=False, rietveld_obj=None, verbose=False)
+        is_committing=False, rietveld_obj=None, verbose=False)
     self.assertEquals(api.PresubmitLocalPath(), 'foo/path')
     self.assertEquals(api.change, self.fake_change)
     self.assertEquals(api.host_url, 'http://codereview.chromium.org')
@@ -905,7 +903,7 @@ class InputApiUnittest(PresubmitTestsBase):
     input_api = presubmit.InputApi(
         change,
         join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'),
-        False, False, None, False)
+        False, None, False)
     # Doesn't filter much
     got_files = input_api.AffectedFiles()
     self.assertEquals(len(got_files), 7)
@@ -991,8 +989,7 @@ class InputApiUnittest(PresubmitTestsBase):
       ),
     ]
     input_api = presubmit.InputApi(
-        self.fake_change, './PRESUBMIT.py', False,
-        False, None, False)
+        self.fake_change, './PRESUBMIT.py', False, None, False)
     self.mox.ReplayAll()
 
     self.assertEqual(len(input_api.DEFAULT_WHITE_LIST), 21)
@@ -1023,7 +1020,7 @@ class InputApiUnittest(PresubmitTestsBase):
     input_api = presubmit.InputApi(
         change,
         presubmit.os.path.join(self.fake_root_dir, 'PRESUBMIT.py'),
-        False, False, None, False)
+        False, None, False)
     got_files = input_api.AffectedSourceFiles(FilterSourceFile)
     self.assertEquals(len(got_files), 2)
     self.assertEquals(got_files[0].LocalPath(), 'eeaee')
@@ -1043,7 +1040,7 @@ class InputApiUnittest(PresubmitTestsBase):
     change = presubmit.SvnChange(
         'mychange', '', self.fake_root_dir, files, 0, 0, None)
     input_api = presubmit.InputApi(
-        change, './PRESUBMIT.py', False, False, None, False)
+        change, './PRESUBMIT.py', False, None, False)
     # Sample usage of overiding the default white and black lists.
     got_files = input_api.AffectedSourceFiles(
         lambda x: input_api.FilterSourceFile(x, white_list, black_list))
@@ -1084,7 +1081,7 @@ class InputApiUnittest(PresubmitTestsBase):
     presubmit_path = join(self.fake_root_dir, 'isdir', 'PRESUBMIT.py')
     api = presubmit.InputApi(
         change=change, presubmit_path=presubmit_path,
-        is_committing=True, tbr=False, rietveld_obj=None, verbose=False)
+        is_committing=True, rietveld_obj=None, verbose=False)
     paths_from_api = api.AbsoluteLocalPaths(include_dirs=True)
     self.assertEqual(len(paths_from_api), 2)
     for absolute_paths in [paths_from_change, paths_from_api]:
@@ -1102,7 +1099,7 @@ class InputApiUnittest(PresubmitTestsBase):
     api = presubmit.InputApi(
         change,
         presubmit.os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'), True,
-        False, None, False)
+        None, False)
     api.AffectedTextFiles(include_deletes=False)
 
   def testReadFileStringDenied(self):
@@ -1112,7 +1109,7 @@ class InputApiUnittest(PresubmitTestsBase):
         'foo', 'foo', self.fake_root_dir, [('M', 'AA')], 0, 0, None)
     input_api = presubmit.InputApi(
         change, presubmit.os.path.join(self.fake_root_dir, '/p'), False,
-        False, None, False)
+        None, False)
     self.assertRaises(IOError, input_api.ReadFile, 'boo', 'x')
 
   def testReadFileStringAccepted(self):
@@ -1124,7 +1121,7 @@ class InputApiUnittest(PresubmitTestsBase):
         'foo', 'foo', self.fake_root_dir, [('M', 'AA')], 0, 0, None)
     input_api = presubmit.InputApi(
         change, presubmit.os.path.join(self.fake_root_dir, '/p'), False,
-        False, None, False)
+        None, False)
     input_api.ReadFile(path, 'x')
 
   def testReadFileAffectedFileDenied(self):
@@ -1135,7 +1132,7 @@ class InputApiUnittest(PresubmitTestsBase):
         'foo', 'foo', self.fake_root_dir, [('M', 'AA')], 0, 0, None)
     input_api = presubmit.InputApi(
         change, presubmit.os.path.join(self.fake_root_dir, '/p'), False,
-        False, None, False)
+        None, False)
     self.assertRaises(IOError, input_api.ReadFile, fileobj, 'x')
 
   def testReadFileAffectedFileAccepted(self):
@@ -1148,7 +1145,7 @@ class InputApiUnittest(PresubmitTestsBase):
         'foo', 'foo', self.fake_root_dir, [('M', 'AA')], 0, 0, None)
     input_api = presubmit.InputApi(
         change, presubmit.os.path.join(self.fake_root_dir, '/p'), False,
-        False, None, False)
+        None, False)
     input_api.ReadFile(fileobj, 'x')
 
 
@@ -1379,6 +1376,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
       'CheckChangeHasTestField',
       'CheckChangeLintsClean',
       'CheckChangeSvnEolStyle',
+      'CheckChangeWasUploaded',
       'CheckDoNotSubmit',
       'CheckDoNotSubmitInDescription', 'CheckDoNotSubmitInFiles',
       'CheckLongLines', 'CheckTreeIsOpen', 'PanProjectChecks',

tests/tbr.sh

@@ -1,39 +0,0 @@
-#!/bin/bash
-
-# Tests the "tbr" functionality, which lets you submit without uploading
-# first.
-
-set -e
-
-. ./test-lib.sh
-
-setup_initsvn
-setup_gitsvn
-
-(
-  set -e
-  cd git-svn
-
-  # We need a server set up, but we don't use it.
-  git config rietveld.server localhost:1
-
-  echo "some work done" >> test
-  git add test; git commit -q -m "work"
-
-  test_expect_success "git-cl dcommit tbr without an issue" \
-    "$GIT_CL dcommit -f --tbr -m 'foo-quux'"
-
-  git svn -q rebase >/dev/null 2>&1
-  test_expect_success "dcommitted code has proper description" \
-      "git show | grep -q 'foo-quux'"
-
-  test_expect_success "upstream svn has our commit" \
-      "svn log $REPO_URL 2>/dev/null | grep -q 'foo-quux'"
-)
-SUCCESS=$?
-
-cleanup
-
-if [ $SUCCESS == 0 ]; then
-  echo PASS
-fi