From cc73ad68ffed1cb0f52ed8e38b078dcd3fb3c7fb Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Wed, 6 Jul 2011 17:39:26 +0000 Subject: [PATCH] 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 | 1 - git_cl.py | 57 +++++++-------------------------- presubmit_canned_checks.py | 13 ++++++++ presubmit_support.py | 20 ++++++------ tests/post-dcommit-hook-test.sh | 5 +-- tests/presubmit_unittest.py | 54 +++++++++++++++---------------- tests/tbr.sh | 39 ---------------------- 7 files changed, 63 insertions(+), 126 deletions(-) delete mode 100755 tests/tbr.sh diff --git a/gcl.py b/gcl.py index 5d5e4248a..e038d5d73 100755 --- a/gcl.py +++ b/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. diff --git a/git_cl.py b/git_cl.py index 94bf1ea76..e59749f77 100755 --- a/git_cl.py +++ b/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 '") - 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): diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 4f267ca66..a69bacee4 100644 --- a/presubmit_canned_checks.py +++ b/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 diff --git a/presubmit_support.py b/presubmit_support.py index a292bd78d..1a84b59ca 100755 --- a/presubmit_support.py +++ b/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: diff --git a/tests/post-dcommit-hook-test.sh b/tests/post-dcommit-hook-test.sh index cd3ecbe7f..e5d1a17c6 100755 --- a/tests/post-dcommit-hook-test.sh +++ b/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" diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 7f87be418..40887d8aa 100755 --- a/tests/presubmit_unittest.py +++ b/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', diff --git a/tests/tbr.sh b/tests/tbr.sh deleted file mode 100755 index d6caf69fb..000000000 --- a/tests/tbr.sh +++ /dev/null @@ -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