Commit 20254fc7 authored by dpranke@chromium.org's avatar dpranke@chromium.org

Revert r79002 - bug processing reviewer lists

TBR=maruel@chromium.org

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@79005 0039d316-1c4b-4281-b951-d872f2087c98
parent 0c20c2fc
......@@ -290,7 +290,9 @@ class ChangeInfo(object):
self.name = name
self.issue = int(issue)
self.patchset = int(patchset)
self._change_desc = None
self._description = None
self._subject = None
self._reviewers = None
self._set_description(description)
if files is None:
files = []
......@@ -304,21 +306,42 @@ class ChangeInfo(object):
self.rietveld = GetCodeReviewSetting('CODE_REVIEW_SERVER')
def _get_description(self):
return self._change_desc.description
return self._description
def _set_description(self, description):
self._change_desc = presubmit_support.ChangeDescription(
description=description)
# TODO(dpranke): Cloned from git_cl.py. These should be shared.
if not description:
self._description = description
return
parsed_lines = []
reviewers_re = re.compile(REVIEWERS_REGEX)
reviewers = ''
subject = ''
for l in description.splitlines():
if not subject:
subject = l
matched_reviewers = reviewers_re.match(l)
if matched_reviewers:
reviewers = matched_reviewers.group(1).split(',')
parsed_lines.append(l)
if len(subject) > 100:
subject = subject[:97] + '...'
self._subject = subject
self._reviewers = reviewers
self._description = '\n'.join(parsed_lines)
description = property(_get_description, _set_description)
@property
def reviewers(self):
return self._change_desc.reviewers
return self._reviewers
@property
def subject(self):
return self._change_desc.subject
return self._subject
def NeedsUpload(self):
return self.needs_upload
......@@ -355,7 +378,7 @@ class ChangeInfo(object):
'patchset': self.patchset,
'needs_upload': self.NeedsUpload(),
'files': self.GetFiles(),
'description': self._change_desc.description,
'description': self.description,
'rietveld': self.rietveld,
}, sort_keys=True, indent=2)
gclient_utils.FileWrite(GetChangelistInfoFile(self.name), data)
......@@ -716,6 +739,20 @@ def ListFiles(show_unknown_files):
return 0
def GetEditor():
editor = os.environ.get("SVN_EDITOR")
if not editor:
editor = os.environ.get("EDITOR")
if not editor:
if sys.platform.startswith("win"):
editor = "notepad"
else:
editor = "vi"
return editor
def GenerateDiff(files, root=None):
return SVN.GenerateDiff(files, root=root)
......@@ -1061,38 +1098,48 @@ def CMDchange(args):
affected_files = [x for x in other_files if file_re.match(x[0])]
unaffected_files = [x for x in other_files if not file_re.match(x[0])]
reviewers = change_info.reviewers
if not reviewers:
if not change_info.reviewers:
files_for_review = affected_files[:]
files_for_review.extend(change_info.GetFiles())
reviewers = suggest_reviewers(change_info, files_for_review)
suggested_reviewers = suggest_reviewers(change_info, files_for_review)
if suggested_reviewers:
reviewers_re = re.compile(REVIEWERS_REGEX)
if not any(reviewers_re.match(l) for l in description.splitlines()):
description += '\n\nR=' + ','.join(suggested_reviewers)
description = description.rstrip() + '\n'
separator1 = ("\n---All lines above this line become the description.\n"
"---Repository Root: " + change_info.GetLocalRoot() + "\n"
"---Paths in this changelist (" + change_info.name + "):\n")
separator2 = "\n\n---Paths modified but not in any changelist:\n\n"
footer = (separator1 + '\n' +
'\n'.join([f[0] + f[1] for f in change_info.GetFiles()]))
text = (description + separator1 + '\n' +
'\n'.join([f[0] + f[1] for f in change_info.GetFiles()]))
if change_info.Exists():
footer += (separator2 +
'\n'.join([f[0] + f[1] for f in affected_files]) + '\n')
text += (separator2 +
'\n'.join([f[0] + f[1] for f in affected_files]) + '\n')
else:
footer += ('\n'.join([f[0] + f[1] for f in affected_files]) + '\n' +
separator2)
footer += '\n'.join([f[0] + f[1] for f in unaffected_files]) + '\n'
change_desc = presubmit_support.ChangeDescription(description=description,
reviewers=reviewers)
text += ('\n'.join([f[0] + f[1] for f in affected_files]) + '\n' +
separator2)
text += '\n'.join([f[0] + f[1] for f in unaffected_files]) + '\n'
# These next few lines are equivalent to change_desc.UserUpdate(). We
# call them individually to avoid passing a lot of state back and forth.
original_description = change_desc.description
handle, filename = tempfile.mkstemp(text=True)
os.write(handle, text)
os.close(handle)
result = change_desc.EditableDescription() + footer
if not silent:
result = change_desc.editor(result)
# Open up the default editor in the system to get the CL description.
try:
if not silent:
cmd = '%s %s' % (GetEditor(), filename)
if sys.platform == 'win32' and os.environ.get('TERM') == 'msys':
# Msysgit requires the usage of 'env' to be present.
cmd = 'env ' + cmd
# shell=True to allow the shell to handle all forms of quotes in $EDITOR.
subprocess.check_call(cmd, shell=True)
result = gclient_utils.FileRead(filename, 'r')
finally:
os.remove(filename)
if not result:
return 0
......@@ -1104,8 +1151,8 @@ def CMDchange(args):
# Update the CL description if it has changed.
new_description = split_result[0]
cl_files_text = split_result[1]
change_desc.Parse(new_description)
if change_desc.description != original_description or override_description:
if new_description != description or override_description:
change_info.description = new_description
change_info.needs_upload = True
new_cl_files = []
......@@ -1119,7 +1166,7 @@ def CMDchange(args):
new_cl_files.append((status, filename))
if (not len(change_info.GetFiles()) and not change_info.issue and
not len(change_desc.description) and not new_cl_files):
not len(new_description) and not new_cl_files):
ErrorExit("Empty changelist not saved")
change_info._files = new_cl_files
......
......@@ -12,7 +12,6 @@ import re
import stat
import subprocess
import sys
import tempfile
import threading
import time
import xml.dom.minidom
......@@ -711,37 +710,3 @@ class ExecutionQueue(object):
work_queue.ready_cond.notifyAll()
finally:
work_queue.ready_cond.release()
def GetEditor():
editor = os.environ.get("SVN_EDITOR")
if not editor:
editor = os.environ.get("EDITOR")
if not editor:
if sys.platform.startswith("win"):
editor = "notepad"
else:
editor = "vi"
return editor
def UserEdit(text):
"""Open an editor, edit the text, and return the result."""
(file_handle, filename) = tempfile.mkstemp()
fileobj = os.fdopen(file_handle, 'w')
fileobj.write(text)
fileobj.close()
# Open up the default editor in the system to get the CL description.
try:
cmd = '%s %s' % (GetEditor(), filename)
if sys.platform == 'win32' and os.environ.get('TERM') == 'msys':
# Msysgit requires the usage of 'env' to be present.
cmd = 'env ' + cmd
# shell=True to allow the shell to handle all forms of quotes in $EDITOR.
subprocess.check_call(cmd, shell=True)
return FileRead(filename, 'r')
finally:
os.remove(filename)
......@@ -9,6 +9,7 @@ import os
import re
import subprocess
import sys
import tempfile
import textwrap
import urlparse
import urllib2
......@@ -20,17 +21,17 @@ except ImportError:
# TODO(dpranke): don't use relative import.
import upload # pylint: disable=W0403
# TODO(dpranke): move this file up a directory so we don't need this.
depot_tools_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
sys.path.append(depot_tools_path)
import breakpad # pylint: disable=W0611
import presubmit_support
import scm
import watchlists
try:
# TODO(dpranke): We wrap this in a try block for a limited form of
# backwards-compatibility with older versions of git-cl that weren't
# dependent on depot_tools. This version should still work outside of
# depot_tools as long as --bypass-hooks is used. We should remove this
# once this has baked for a while and things seem safe.
depot_tools_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
sys.path.append(depot_tools_path)
import breakpad # pylint: disable=W0611
except ImportError:
pass
DEFAULT_SERVER = 'http://codereview.appspot.com'
POSTUPSTREAM_HOOK_PATTERN = '.git/hooks/post-cl-%s'
......@@ -332,7 +333,6 @@ class Changelist(object):
self.description = None
self.has_patchset = False
self.patchset = None
self.tbr = False
def GetBranch(self):
"""Returns the short branch name, e.g. 'master'."""
......@@ -535,6 +535,53 @@ def GetCodereviewSettingsInteractively():
# svn-based hackery.
class ChangeDescription(object):
"""Contains a parsed form of the change description."""
def __init__(self, subject, log_desc, reviewers):
self.subject = subject
self.log_desc = log_desc
self.reviewers = reviewers
self.description = self.log_desc
def Update(self):
initial_text = """# Enter a description of the change.
# This will displayed on the codereview site.
# The first line will also be used as the subject of the review.
"""
initial_text += self.description
if 'R=' not in self.description and self.reviewers:
initial_text += '\nR=' + self.reviewers
if 'BUG=' not in self.description:
initial_text += '\nBUG='
if 'TEST=' not in self.description:
initial_text += '\nTEST='
self._ParseDescription(UserEditedLog(initial_text))
def _ParseDescription(self, description):
if not description:
self.description = description
return
parsed_lines = []
reviewers_regexp = re.compile('\s*R=(.+)')
reviewers = ''
subject = ''
for l in description.splitlines():
if not subject:
subject = l
matched_reviewers = reviewers_regexp.match(l)
if matched_reviewers:
reviewers = matched_reviewers.group(1)
parsed_lines.append(l)
self.description = '\n'.join(parsed_lines) + '\n'
self.subject = subject
self.reviewers = reviewers
def IsEmpty(self):
return not self.description
def FindCodereviewSettingsFile(filename='codereview.settings'):
"""Finds the given file starting in the cwd and going up.
......@@ -684,6 +731,36 @@ def CreateDescriptionFromLog(args):
return RunGit(['log', '--pretty=format:%s\n\n%b'] + log_args)
def UserEditedLog(starting_text):
"""Given some starting text, let the user edit it and return the result."""
editor = os.getenv('EDITOR', 'vi')
(file_handle, filename) = tempfile.mkstemp()
fileobj = os.fdopen(file_handle, 'w')
fileobj.write(starting_text)
fileobj.close()
# Open up the default editor in the system to get the CL description.
try:
cmd = '%s %s' % (editor, filename)
if sys.platform == 'win32' and os.environ.get('TERM') == 'msys':
# Msysgit requires the usage of 'env' to be present.
cmd = 'env ' + cmd
# shell=True to allow the shell to handle all forms of quotes in $EDITOR.
subprocess.check_call(cmd, shell=True)
fileobj = open(filename)
text = fileobj.read()
fileobj.close()
finally:
os.remove(filename)
if not text:
return
stripcomment_re = re.compile(r'^#.*$', re.MULTILINE)
return stripcomment_re.sub('', text).strip()
def ConvertToInteger(inputval):
"""Convert a string to integer, but returns either an int or None."""
try:
......@@ -692,20 +769,12 @@ def ConvertToInteger(inputval):
return None
class GitChangeDescription(presubmit_support.ChangeDescription):
def UserEdit(self):
header = (
"# Enter a description of the change.\n"
"# This will displayed on the codereview site.\n"
"# The first line will also be used as the subject of the review.\n"
"\n")
edited_text = self.editor(header + self.EditableDescription())
stripcomment_re = re.compile(r'^#.*$', re.MULTILINE)
self.Parse(stripcomment_re.sub('', edited_text).strip())
def RunHook(committing, upstream_branch, rietveld_server, tbr, may_prompt):
"""Calls sys.exit() if the hook fails; returns a HookResults otherwise."""
import presubmit_support
import scm
import watchlists
root = RunCommand(['git', 'rev-parse', '--show-cdup']).strip()
if not root:
root = '.'
......@@ -774,13 +843,13 @@ def CMDpresubmit(parser, args):
if options.upload:
print '*** Presubmit checks for UPLOAD would report: ***'
RunHook(committing=False, upstream_branch=base_branch,
rietveld_server=cl.GetRietveldServer(), tbr=cl.tbr,
rietveld_server=cl.GetRietveldServer(), tbr=False,
may_prompt=False)
return 0
else:
print '*** Presubmit checks for DCOMMIT would report: ***'
RunHook(committing=True, upstream_branch=base_branch,
rietveld_server=cl.GetRietveldServer, tbr=cl.tbr,
rietveld_server=cl.GetRietveldServer, tbr=False,
may_prompt=False)
return 0
......@@ -824,10 +893,10 @@ def CMDupload(parser, args):
if not options.bypass_hooks and not options.force:
hook_results = RunHook(committing=False, upstream_branch=base_branch,
rietveld_server=cl.GetRietveldServer(), tbr=cl.tbr,
rietveld_server=cl.GetRietveldServer(), tbr=False,
may_prompt=True)
if not options.reviewers and hook_results.reviewers:
options.reviewers = ','.join(hook_results.reviewers)
options.reviewers = hook_results.reviewers
# --no-ext-diff is broken in some versions of Git, so try to work around
......@@ -861,10 +930,10 @@ def CMDupload(parser, args):
"Adding patch to that issue." % cl.GetIssue())
else:
log_desc = CreateDescriptionFromLog(args)
change_desc = GitChangeDescription(subject=options.message,
description=log_desc, reviewers=options.reviewers, tbr=cl.tbr)
if not options.from_logs and (not options.force):
change_desc.UserEdit()
change_desc = ChangeDescription(options.message, log_desc,
options.reviewers)
if not options.from_logs:
change_desc.Update()
if change_desc.IsEmpty():
print "Description is empty; aborting."
......@@ -975,7 +1044,7 @@ def SendUpstream(parser, args, cmd):
if not options.bypass_hooks and not options.force:
RunHook(committing=True, upstream_branch=base_branch,
rietveld_server=cl.GetRietveldServer(), tbr=(cl.tbr or options.tbr),
rietveld_server=cl.GetRietveldServer(), tbr=options.tbr,
may_prompt=True)
if cmd == 'dcommit':
......@@ -1014,15 +1083,17 @@ def SendUpstream(parser, args, cmd):
# create a template description. Eitherway, give the user a chance to edit
# it to fill in the TBR= field.
if cl.GetIssue():
change_desc = GitChangeDescription(description=cl.GetDescription())
description = cl.GetDescription()
# TODO(dpranke): Update to use ChangeDescription object.
if not description:
log_desc = CreateDescriptionFromLog(args)
change_desc = GitChangeDescription(description=log_desc, tbr=True)
description = """# Enter a description of the change.
# This will be used as the change log for the commit.
"""
description += CreateDescriptionFromLog(args)
if not options.force:
change_desc.UserEdit()
description = change_desc.description
description = UserEditedLog(description + '\nTBR=')
if not description:
print "Description empty; aborting."
......
......@@ -812,96 +812,6 @@ class GitChange(Change):
self.scm = 'git'
class ChangeDescription(object):
"""Contains a parsed form of the change description."""
MAX_SUBJECT_LENGTH = 100
def __init__(self, subject=None, description=None, reviewers=None, tbr=False,
editor=None):
self.subject = (subject or '').strip()
self.description = (description or '').strip()
self.reviewers = reviewers or []
self.tbr = tbr
self.editor = editor or gclient_utils.UserEdit
if self.description:
if not self.description.startswith(self.subject):
self.description = self.subject + '\n\n' + self.description
elif self.subject:
self.description = self.subject
self.Parse(self.EditableDescription())
def EditableDescription(self):
text = self.description.strip()
if text:
text += '\n'
tbr_present = False
r_present = False
bug_present = False
test_present = False
for l in text.splitlines():
l = l.strip()
r_present = r_present or l.startswith('R=')
tbr_present = tbr_present or l.startswith('TBR=')
if text and not (r_present or tbr_present):
text += '\n'
if not tbr_present and not r_present:
if self.tbr:
text += 'TBR=' + ','.join(self.reviewers) + '\n'
else:
text += 'R=' + ','.join(self.reviewers) + '\n'
if not bug_present:
text += 'BUG=\n'
if not test_present:
text += 'TEST=\n'
return text
def UserEdit(self):
"""Allows the user to update the description.
Uses the editor callback passed to the constructor."""
self.Parse(self.editor(self.EditableDescription()))
def Parse(self, text):
"""Parse the text returned from UserEdit() and update our state."""
parsed_lines = []
reviewers_regexp = re.compile('\s*(TBR|R)=(.+)')
reviewers = []
subject = ''
tbr = False
for l in text.splitlines():
l = l.strip()
# Throw away empty BUG=, TEST=, and R= lines. We leave in TBR= lines
# to indicate that this change was meant to be "unreviewed".
if l in ('BUG=', 'TEST=', 'R='):
continue
if not subject:
subject = l
matched_reviewers = reviewers_regexp.match(l)
if matched_reviewers:
tbr = (matched_reviewers.group(1) == 'TBR')
reviewers.extend(matched_reviewers.group(2).split(','))
parsed_lines.append(l)
if len(subject) > self.MAX_SUBJECT_LENGTH:
subject = subject[:self.MAX_SUBJECT_LENGTH - 3] + '...'
self.description = '\n'.join(parsed_lines).strip()
self.subject = subject
self.reviewers = reviewers
self.tbr = tbr
def IsEmpty(self):
return not self.description
def ListRelevantPresubmitFiles(files, root):
"""Finds all presubmit files that apply to a given set of source files.
......
......@@ -84,7 +84,7 @@ class GclUnittest(GclTestsBase):
'ErrorExit', 'FILES_CACHE', 'FilterFlag', 'GenUsage',
'GenerateChangeName', 'GenerateDiff', 'GetCLs', 'GetCacheDir',
'GetCachedFile', 'GetChangelistInfoFile', 'GetChangesDir',
'GetCodeReviewSetting', 'GetFilesNotInCL', 'GetInfoDir',
'GetCodeReviewSetting', 'GetEditor', 'GetFilesNotInCL', 'GetInfoDir',
'GetModifiedFiles', 'GetRepositoryRoot', 'ListFiles',
'LoadChangelistInfoForMultiple', 'MISSING_TEST_MSG',
'OptionallyDoPresubmitChecks', 'REPOSITORY_ROOT', 'REVIEWERS_REGEX',
......
......@@ -28,13 +28,13 @@ class GclientUtilsUnittest(GclientUtilBase):
'CheckCall', 'CheckCallError', 'CheckCallAndFilter',
'CheckCallAndFilterAndHeader', 'Error', 'ExecutionQueue', 'FileRead',
'FileWrite', 'FindFileUpwards', 'FindGclientRoot',
'GetGClientRootAndEntries', 'GetEditor', 'GetNamedNodeText',
'MakeFileAutoFlush', 'GetNodeNamedAttributeText', 'MakeFileAnnotated',
'PathDifference', 'ParseXML', 'Popen',
'GetGClientRootAndEntries', 'GetNamedNodeText', 'MakeFileAutoFlush',
'GetNodeNamedAttributeText', 'MakeFileAnnotated', 'PathDifference',
'ParseXML', 'Popen',
'PrintableObject', 'RemoveDirectory', 'SoftClone', 'SplitUrlRevision',
'SyntaxErrorToError', 'UserEdit', 'WorkItem',
'SyntaxErrorToError', 'WorkItem',
'errno', 'hack_subprocess', 'logging', 'os', 'Queue', 're', 'rmtree',
'stat', 'subprocess', 'sys', 'tempfile', 'threading', 'time', 'xml',
'stat', 'subprocess', 'sys','threading', 'time', 'xml',
]
# If this test fails, you should add the relevant test.
self.compareMembers(gclient_utils, members)
......
......@@ -9,7 +9,6 @@
# pylint: disable=E1101,E1103,W0212,W0403
import StringIO
import unittest
# Fixes include path.
from super_mox import mox, SuperMoxTestBase
......@@ -136,8 +135,8 @@ class PresubmitUnittest(PresubmitTestsBase):
def testMembersChanged(self):
self.mox.ReplayAll()
members = [
'AffectedFile', 'Change', 'ChangeDescription', 'DoGetTrySlaves',
'DoPresubmitChecks', 'GetTrySlavesExecuter', 'GitAffectedFile',
'AffectedFile', 'Change', 'DoGetTrySlaves', 'DoPresubmitChecks',
'GetTrySlavesExecuter', 'GitAffectedFile',
'GitChange', 'InputApi', 'ListRelevantPresubmitFiles', 'Main',
'NotImplementedException', 'OutputApi', 'ParseFiles',
'PresubmitExecuter', 'PresubmitOutput', 'ScanSubDirs',
......@@ -1972,81 +1971,6 @@ mac|success|blew
uncovered_files=set(), host_url='https://localhost')
def change_desc(editor=None, **kwargs):
if editor is None:
editor = lambda x: x
return presubmit.ChangeDescription(editor=editor, **kwargs)
class ChangeDescriptionTests(unittest.TestCase):
def setUp(self):
self.editor_input = None
self.editor_output = None
def tearDown(self):
self.editor_input = None
self.editor_output = None
def editor(self, text):
if self.editor_input:
self.assertTrue(self.editor_input in text)
if self.editor_output is not None:
return self.editor_output
return text
def test_empty(self):
desc = change_desc()
self.assertTrue(desc.IsEmpty())
desc.UserEdit()
self.assertTrue(desc.IsEmpty())
def test_basic(self):
desc = change_desc(subject='foo', description='desc',
reviewers=['joe@example.com'])
desc.UserEdit()
self.assertFalse(desc.IsEmpty())
self.assertEqual(desc.subject, 'foo')
self.assertEquals(desc.description,
'foo\n'
'\n'
'desc\n'
'\n'
'R=joe@example.com')
self.assertEquals(desc.reviewers, ['joe@example.com'])
self.assertFalse(desc.tbr)
def test_subject_only(self):
self.editor_input = 'foo\n\nR=\nBUG=\nTEST=\n'
desc = change_desc(subject='foo', editor=self.editor)
desc.UserEdit()
self.assertEquals(desc.description, 'foo')
def test_tbr_with_reviewer(self):
self.editor_input = 'TBR=\nBUG=\nTEST=\n'
self.editor_output = 'foo\n\nTBR=joe@example.com'
desc = change_desc(tbr=True, editor=self.editor)
self.assertFalse(desc.tbr)
self.assertEquals(desc.reviewers, [])
desc.UserEdit()
self.assertTrue(desc.tbr)
self.assertEquals(desc.reviewers, ['joe@example.com'])
self.assertEquals(desc.description,
'foo\n'
'\n'
'TBR=joe@example.com')
def test_tbr_without_reviewer(self):
desc = change_desc(subject='foo', tbr=True)
desc.UserEdit()
self.assertEquals(desc.description, 'foo\n\nTBR=')
def test_really_long_subject(self):
subject = 'foo' * 40
desc = change_desc(subject=subject)
self.assertEquals(desc.description, subject)
self.assertEquals(desc.subject, subject[:97] + '...')
if __name__ == '__main__':
import unittest
unittest.main()
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment