From 7d5cc1b424487447006a7cad9bee4c272b1e15e3 Mon Sep 17 00:00:00 2001 From: "jam@chromium.org" Date: Thu, 11 Apr 2013 03:14:26 +0000 Subject: [PATCH] Revert 193525 I get the following when doing "gcl upload" Traceback (most recent call last): File "f:\src\depot_tools\gcl.py", line 1459, in sys.exit(main(sys.argv[1:])) File "f:\src\depot_tools\gcl.py", line 1437, in main return command(argv[1:]) File "f:\src\depot_tools\gcl.py", line 1070, in CMDchange description = change_info.GetIssueDescription() AttributeError: 'ChangeInfo' object has no attribute 'GetIssueDescription' Sending crash report ... args: ['f:\\src\\depot_tools\\gcl.py', 'change', 'w4f4'] cwd: f:\src\chrome3\src exception: 'ChangeInfo' object has no attribute 'GetIssueDesc host: JABDELMALEK3-W.ad.corp.google.com stack: File "f:\src\depot_tools\gcl.py", line 1459, in user: jabdelmalek version: 2.6.2 (r262:71600, Apr 21 2009, 15:05:37) [MSC v.1 A stack trace has been sent to the maintainers. "Make gcl use git_cl.py code for consistency in th..." > Make gcl use git_cl.py code for consistency in the CL description formatting. > > This way, git_cl.py's ChangeDescription becomes the canonical CL description > handler. > > R=dpranke@chromium.org > BUG= > > Review URL: https://chromiumcodereview.appspot.com/13800018 TBR=maruel@chromium.org Review URL: https://codereview.chromium.org/14016004 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@193555 0039d316-1c4b-4281-b951-d872f2087c98 --- gcl.py | 71 +++++++++++++++++++++++++++---------------- tests/gcl_unittest.py | 36 +++++++++------------- 2 files changed, 59 insertions(+), 48 deletions(-) diff --git a/gcl.py b/gcl.py index f9761e74e..ce6edabab 100755 --- a/gcl.py +++ b/gcl.py @@ -24,7 +24,6 @@ import breakpad # pylint: disable=W0611 import fix_encoding import gclient_utils -import git_cl import presubmit_support import rietveld from scm import SVN @@ -60,6 +59,8 @@ FILES_CACHE = {} DEFAULT_LINT_REGEX = r"(.*\.cpp|.*\.cc|.*\.h)" DEFAULT_LINT_IGNORE_REGEX = r"$^" +REVIEWERS_REGEX = r'\s*R=(.+)' + def CheckHomeForFile(filename): """Checks the users home dir for the existence of the given file. Returns the path to the file if it's there, or None if it is not. @@ -272,12 +273,15 @@ class ChangeInfo(object): def __init__(self, name, issue, patchset, description, files, local_root, rietveld_url, needs_upload): - # Defer the description processing to git_cl.ChangeDescription. - self._desc = git_cl.ChangeDescription(description) self.name = name self.issue = int(issue) self.patchset = int(patchset) - self._files = files or [] + self._description = None + self._reviewers = None + self._set_description(description) + if files is None: + files = [] + self._files = files self.patch = None self._local_root = local_root self.needs_upload = needs_upload @@ -285,19 +289,31 @@ class ChangeInfo(object): rietveld_url or GetCodeReviewSetting('CODE_REVIEW_SERVER')) self._rpc_server = None - @property - def description(self): - return self._desc.description + def _get_description(self): + return self._description - def force_description(self, new_description): - self._desc = git_cl.ChangeDescription(new_description) - self.needs_upload = True + def _set_description(self, description): + # TODO(dpranke): Cloned from git_cl.py. These should be shared. + if not description: + self._description = description + return - def append_footer(self, line): - self._desc.append_footer(line) + parsed_lines = [] + reviewers_re = re.compile(REVIEWERS_REGEX) + reviewers = '' + for l in description.splitlines(): + matched_reviewers = reviewers_re.match(l) + if matched_reviewers: + reviewers = matched_reviewers.group(1).split(',') + parsed_lines.append(l) + self._reviewers = reviewers + self._description = '\n'.join(parsed_lines) - def get_reviewers(self): - return self._desc.get_reviewers() + description = property(_get_description, _set_description) + + @property + def reviewers(self): + return self._reviewers def NeedsUpload(self): return self.needs_upload @@ -372,12 +388,10 @@ class ChangeInfo(object): ctype, body = upload.EncodeMultipartFormData(data, []) self.SendToRietveld('/%d/description' % self.issue, payload=body, content_type=ctype) - self.needs_upload = False - def UpdateDescriptionFromIssue(self): - """Updates self.description with the issue description from Rietveld.""" - self._desc = git_cl.ChangeDescription( - self.SendToRietveld('/%d/description' % self.issue)) + def GetIssueDescription(self): + """Returns the issue description from Rietveld.""" + return self.SendToRietveld('/%d/description' % self.issue) def AddComment(self, comment): """Adds a comment for an issue on Rietveld. @@ -837,7 +851,7 @@ def CMDupload(change_info, args): upload_arg = ["upload.py", "-y"] upload_arg.append("--server=%s" % change_info.rietveld) - reviewers = change_info.get_reviewers() or output.reviewers + reviewers = change_info.reviewers or output.reviewers if (reviewers and not any(arg.startswith('-r') or arg.startswith('--reviewer') for arg in args)): @@ -989,17 +1003,17 @@ def CMDcommit(change_info, args): commit_cmd = ["svn", "commit"] if change_info.issue: # Get the latest description from Rietveld. - change_info.UpdateDescriptionFromIssue() + change_info.description = change_info.GetIssueDescription() - commit_desc = git_cl.ChangeDescription(change_info.description) + commit_message = change_info.description.replace('\r\n', '\n') if change_info.issue: server = change_info.rietveld if not server.startswith("http://") and not server.startswith("https://"): server = "http://" + server - commit_desc.append_footer('Review URL: %s/%d' % (server, change_info.issue)) + commit_message += ('\nReview URL: %s/%d' % (server, change_info.issue)) handle, commit_filename = tempfile.mkstemp(text=True) - os.write(handle, commit_desc.description) + os.write(handle, commit_message) os.close(handle) try: handle, targets_filename = tempfile.mkstemp(text=True) @@ -1025,10 +1039,11 @@ def CMDcommit(change_info, args): revision = re.compile(".*?\nCommitted revision (\d+)", re.DOTALL).match(output).group(1) viewvc_url = GetCodeReviewSetting('VIEW_VC') + change_info.description += '\n' if viewvc_url and revision: - change_info.append_footer('Committed: ' + viewvc_url + revision) + change_info.description += "\nCommitted: " + viewvc_url + revision elif revision: - change_info.append_footer('Committed: ' + revision) + change_info.description += "\nCommitted: " + revision change_info.CloseIssue() props = change_info.RpcServer().get_issue_properties( change_info.issue, False) @@ -1123,7 +1138,8 @@ def CMDchange(args): new_description = split_result[0] cl_files_text = split_result[1] if new_description != description or override_description: - change_info.force_description(new_description) + change_info.description = new_description + change_info.needs_upload = True new_cl_files = [] for line in cl_files_text.splitlines(): @@ -1152,6 +1168,7 @@ def CMDchange(args): # Update the Rietveld issue. if change_info.issue and change_info.NeedsUpload(): change_info.UpdateRietveldDescription() + change_info.needs_upload = False change_info.Save() return 0 diff --git a/tests/gcl_unittest.py b/tests/gcl_unittest.py index cbde6a473..ddaab3a82 100755 --- a/tests/gcl_unittest.py +++ b/tests/gcl_unittest.py @@ -53,6 +53,7 @@ class GclTestsBase(SuperMoxTestBase): change_info.GetLocalRoot = lambda : 'proout' change_info.patch = None change_info.rietveld = 'https://my_server' + change_info.reviewers = None change_info._closed = False change_info._deleted = False change_info._comments_added = [] @@ -101,13 +102,13 @@ class GclUnittest(GclTestsBase): 'GetCodeReviewSetting', 'GetFilesNotInCL', 'GetInfoDir', 'GetModifiedFiles', 'GetRepositoryRoot', 'ListFiles', 'LoadChangelistInfoForMultiple', 'MISSING_TEST_MSG', - 'OptionallyDoPresubmitChecks', 'REPOSITORY_ROOT', + 'OptionallyDoPresubmitChecks', 'REPOSITORY_ROOT', 'REVIEWERS_REGEX', 'RunShell', 'RunShellWithReturnCode', 'SVN', 'TryChange', 'UnknownFiles', 'Warn', 'attrs', 'breakpad', 'defer_attributes', 'fix_encoding', - 'gclient_utils', 'git_cl', 'json', 'main', 'need_change', - 'need_change_and_args', 'no_args', 'optparse', 'os', - 'presubmit_support', 'random', 're', 'rietveld', + 'gclient_utils', 'json', 'main', 'need_change', 'need_change_and_args', + 'no_args', 'optparse', 'os', 'presubmit_support', 'random', 're', + 'rietveld', 'string', 'subprocess2', 'sys', 'tempfile', 'time', 'upload', 'urllib2', ] @@ -192,15 +193,13 @@ class ChangeInfoUnittest(GclTestsBase): self.mox.ReplayAll() members = [ 'AddComment', 'CloseIssue', 'Delete', 'Exists', 'GetFiles', - 'GetFileNames', 'GetLocalRoot', - 'Load', + 'GetFileNames', 'GetLocalRoot', 'GetIssueDescription', 'Load', 'MissingTests', 'NeedsUpload', 'PrimeLint', 'RpcServer', 'Save', 'SendToRietveld', 'SEPARATOR', - 'UpdateDescriptionFromIssue', 'UpdateRietveldDescription', - 'append_footer', - 'description', 'force_description', 'get_reviewers', 'issue', 'name', - 'needs_upload', 'patch', 'patchset', 'rietveld', + 'UpdateRietveldDescription', + 'description', 'issue', 'name', + 'needs_upload', 'patch', 'patchset', 'reviewers', 'rietveld', ] # If this test fails, you should add the relevant test. self.compareMembers( @@ -323,7 +322,6 @@ class CMDuploadUnittest(GclTestsBase): gcl.os.getcwd().AndReturn('somewhere') change_info.GetFiles().AndReturn(change_info.files) gcl.os.chdir('proout') - change_info.get_reviewers().AndReturn('foo@bar.com') change_info.GetFileNames().AndReturn(files) gcl.GenerateDiff(files) gcl.upload.RealMain(['upload.py', '-y', '--server=https://my_server', @@ -458,13 +456,13 @@ class CMDuploadUnittest(GclTestsBase): change_info.files = [('A', 'aa'), ('M', 'bb')] change_info.patch = None change_info.rietveld = 'https://my_server' + change_info.reviewers = ['georges@example.com'] files = [item[1] for item in change_info.files] output = presubmit_support.PresubmitOutput() gcl.DoPresubmitChecks(change_info, False, True).AndReturn(output) #gcl.GetCodeReviewSetting('CODE_REVIEW_SERVER').AndReturn('my_server') gcl.os.getcwd().AndReturn('somewhere') change_info.GetFiles().AndReturn(change_info.files) - change_info.get_reviewers().AndReturn(['georges@example.com']) change_info.GetFileNames().AndReturn(files) change_info.GetLocalRoot().AndReturn('proout') gcl.os.chdir('proout') @@ -502,7 +500,6 @@ class CMDuploadUnittest(GclTestsBase): '--reviewers=foo@example.com,bar@example.com', '--issue=1', '--title= '], change_info.patch).AndReturn(("1", "2")) - change_info.get_reviewers().AndReturn(['foo@example.com,bar@example.com']) change_info.Save() change_info.PrimeLint() gcl.os.chdir('somewhere') @@ -575,8 +572,7 @@ class CMDCommitUnittest(GclTestsBase): change_info = self.mockLoad() self.mockPresubmit(change_info, fail=False) self.mockCommit( - change_info, 'deescription\n\nReview URL: https://my_server/1', '') - change_info.UpdateDescriptionFromIssue() + change_info, 'deescription\nReview URL: https://my_server/1', '') self.mox.ReplayAll() retval = gcl.CMDcommit(['naame']) @@ -591,17 +587,15 @@ class CMDCommitUnittest(GclTestsBase): change_info = self.mockLoad() self.mockPresubmit(change_info, fail=False) self.mockCommit( - change_info, - 'deescription\n\nReview URL: https://my_server/1', + change_info, 'deescription\nReview URL: https://my_server/1', '\nCommitted revision 12345') - change_info.UpdateDescriptionFromIssue() - change_info.append_footer('Committed: http://view/12345') + self.mox.ReplayAll() retval = gcl.CMDcommit(['naame']) self.assertEquals(retval, 0) - # This is because append_footer is mocked. - self.assertEquals(change_info.description, 'deescription') + self.assertEquals(change_info.description, + 'deescription\n\nCommitted: http://view/12345') # pylint: disable=W0212 self.assertTrue(change_info._deleted) self.assertTrue(change_info._closed)