diff --git a/gcl.py b/gcl.py index bd815382a..4a7a5afd9 100755 --- a/gcl.py +++ b/gcl.py @@ -285,7 +285,6 @@ class ChangeInfo(object): self.issue = int(issue) self.patchset = int(patchset) self._description = None - self._subject = None self._reviewers = None self._set_description(description) if files is None: @@ -310,19 +309,11 @@ class ChangeInfo(object): 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) @@ -332,10 +323,6 @@ class ChangeInfo(object): def reviewers(self): return self._reviewers - @property - def subject(self): - return self._subject - def NeedsUpload(self): return self.needs_upload @@ -859,18 +846,14 @@ def CMDupload(change_info, args): desc_file = None try: - if change_info.issue: # Uploading a new patchset. - found_message = False - for arg in args: - if arg.startswith("--message") or arg.startswith("-m"): - found_message = True - break - - if not found_message: - upload_arg.append("--message=''") - + if change_info.issue: + # Uploading a new patchset. upload_arg.append("--issue=%d" % change_info.issue) - else: # First time we upload. + + if not any(i.startswith('--message') or i.startswith('-m') for i in args): + upload_arg.append('--title= ') + else: + # First time we upload. handle, desc_file = tempfile.mkstemp(text=True) os.write(handle, change_info.description) os.close(handle) @@ -888,9 +871,7 @@ def CMDupload(change_info, args): cc_list = ','.join(filter(None, [cc_list] + watchers)) if cc_list: upload_arg.append("--cc=" + cc_list) - upload_arg.append("--description_file=%s" % desc_file) - if change_info.subject: - upload_arg.append("--message=" + change_info.subject) + upload_arg.append("--file=%s" % desc_file) if GetCodeReviewSetting("PRIVATE") == "True": upload_arg.append("--private") diff --git a/git_cl.py b/git_cl.py index 72ad8f589..463b6c800 100755 --- a/git_cl.py +++ b/git_cl.py @@ -626,47 +626,43 @@ def GetCodereviewSettingsInteractively(): class ChangeDescription(object): """Contains a parsed form of the change description.""" - def __init__(self, subject, log_desc, reviewers): - self.subject = subject + def __init__(self, log_desc, reviewers): self.log_desc = log_desc self.reviewers = reviewers self.description = self.log_desc - def Update(self): - initial_text = """# Enter a description of the change. + def Prompt(self): + content = """# 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 + content += self.description if ('\nR=' not in self.description and '\nTBR=' not in self.description and self.reviewers): - initial_text += '\nR=' + self.reviewers + content += '\nR=' + self.reviewers if '\nBUG=' not in self.description: - initial_text += '\nBUG=' + content += '\nBUG=' if '\nTEST=' not in self.description: - initial_text += '\nTEST=' - initial_text = initial_text.rstrip('\n') + '\n' - content = gclient_utils.RunEditor(initial_text, True) + content += '\nTEST=' + content = content.rstrip('\n') + '\n' + content = gclient_utils.RunEditor(content, True) if not content: DieWithError('Running editor failed') content = re.compile(r'^#.*$', re.MULTILINE).sub('', content).strip() - if not content: + if not content.strip(): DieWithError('No CL description, aborting') - self.ParseDescription(content) + self.description = content - def ParseDescription(self, description): + def ParseDescription(self): """Updates the list of reviewers and subject from the description.""" - if not description: - self.description = description - return - - self.description = description.strip('\n') + '\n' - self.subject = description.split('\n', 1)[0] + self.description = self.description.strip('\n') + '\n' # Retrieves all reviewer lines regexp = re.compile(r'^\s*(TBR|R)=(.+)$', re.MULTILINE) - self.reviewers = ','.join( + reviewers = ','.join( i.group(2).strip() for i in regexp.finditer(self.description)) + if reviewers: + self.reviewers = reviewers def IsEmpty(self): return not self.description @@ -890,12 +886,11 @@ def GerritUpload(options, args, cl): if options.target_branch: branch = options.target_branch - log_desc = CreateDescriptionFromLog(args) + log_desc = options.message or CreateDescriptionFromLog(args) if options.reviewers: log_desc += '\nR=' + options.reviewers - change_desc = ChangeDescription(options.message, log_desc, - options.reviewers) - change_desc.ParseDescription(log_desc) + change_desc = ChangeDescription(log_desc, options.reviewers) + change_desc.ParseDescription() if change_desc.IsEmpty(): print "Description is empty; aborting." return 1 @@ -928,9 +923,6 @@ def RietveldUpload(options, args, cl): upload_args.extend(['--server', cl.GetRietveldServer()]) if options.emulate_svn_auto_props: upload_args.append('--emulate_svn_auto_props') - if options.from_logs and not options.message: - print 'Must set message for subject line if using desc_from_logs' - return 1 change_desc = None @@ -941,18 +933,17 @@ def RietveldUpload(options, args, cl): print ("This branch is associated with issue %s. " "Adding patch to that issue." % cl.GetIssue()) else: - log_desc = CreateDescriptionFromLog(args) - change_desc = ChangeDescription(options.message, log_desc, - options.reviewers) - if not options.from_logs: - change_desc.Update() + message = options.message or CreateDescriptionFromLog(args) + change_desc = ChangeDescription(message, options.reviewers) + if not options.force: + change_desc.Prompt() + change_desc.ParseDescription() if change_desc.IsEmpty(): print "Description is empty; aborting." return 1 - upload_args.extend(['--message', change_desc.subject]) - upload_args.extend(['--description', change_desc.description]) + upload_args.extend(['--message', change_desc.description]) if change_desc.reviewers: upload_args.extend(['--reviewers', change_desc.reviewers]) if options.send_mail: diff --git a/tests/gcl_unittest.py b/tests/gcl_unittest.py index 4ea69b446..bd6e5b984 100755 --- a/tests/gcl_unittest.py +++ b/tests/gcl_unittest.py @@ -187,7 +187,6 @@ class ChangeInfoUnittest(GclTestsBase): 'UpdateRietveldDescription', 'description', 'issue', 'name', 'needs_upload', 'patch', 'patchset', 'reviewers', 'rietveld', - 'subject', ] # If this test fails, you should add the relevant test. self.compareMembers( @@ -314,7 +313,7 @@ class CMDuploadUnittest(GclTestsBase): gcl.GenerateDiff(files) gcl.upload.RealMain(['upload.py', '-y', '--server=https://my_server', '-r', 'georges@example.com', - '--message=\'\'', '--issue=1'], + '--issue=1', '--title= '], change_info.patch).AndReturn(("1", "2")) change_info.GetLocalRoot().AndReturn('proout') @@ -358,7 +357,7 @@ class CMDuploadUnittest(GclTestsBase): gcl.GenerateDiff(change_info.GetFileNames()) gcl.upload.RealMain( [ 'upload.py', '-y', '--server=https://my_server', '--server=a', - '--description_file=descfile', '--message=deescription'], + '--file=descfile'], change_info.patch).AndReturn(("1", "2")) gcl.os.remove('descfile') change_info.SendToRietveld("/lint/issue%s_%s" % ('1', '2'), timeout=1) @@ -398,9 +397,9 @@ class CMDuploadUnittest(GclTestsBase): gcl.os.getcwd().AndReturn('somewhere') gcl.os.chdir(change_info.GetLocalRoot()) gcl.GenerateDiff(change_info.GetFileNames()) - gcl.upload.RealMain(['upload.py', '-y', '--server=https://my_server', - "--description_file=descfile", - "--message=deescription"], change_info.patch).AndReturn(("1", "2")) + gcl.upload.RealMain( + ['upload.py', '-y', '--server=https://my_server', "--file=descfile" ], + change_info.patch).AndReturn(("1", "2")) gcl.os.remove('descfile') change_info.SendToRietveld("/lint/issue%s_%s" % ('1', '2'), timeout=1) gcl.os.chdir('somewhere') @@ -457,7 +456,7 @@ class CMDuploadUnittest(GclTestsBase): gcl.GenerateDiff(files) gcl.upload.RealMain(['upload.py', '-y', '--server=https://my_server', '--reviewers=georges@example.com', - '--message=\'\'', '--issue=1'], + '--issue=1', '--title= '], change_info.patch).AndReturn(("1", "2")) change_info.Save() change_info.PrimeLint() @@ -486,7 +485,7 @@ class CMDuploadUnittest(GclTestsBase): gcl.GenerateDiff(change_info.GetFileNames()) gcl.upload.RealMain(['upload.py', '-y', '--server=https://my_server', '--reviewers=foo@example.com,bar@example.com', - '--message=\'\'', '--issue=1'], + '--issue=1', '--title= '], change_info.patch).AndReturn(("1", "2")) change_info.Save() change_info.PrimeLint() diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index ef6fa19e6..5f3394f80 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -210,12 +210,10 @@ class TestGitCl(TestCase): @staticmethod def _cmd_line(description, args): """Returns the upload command line passed to upload.RealMain().""" - msg = description.split('\n', 1)[0] return [ 'upload', '--assume_yes', '--server', 'https://codereview.example.com', - '--message', msg, - '--description', description + '--message', description ] + args + [ '--cc', 'joe@example.com', 'master...' diff --git a/third_party/upload.py b/third_party/upload.py index e8fee3b0a..d5e1fb2ee 100755 --- a/third_party/upload.py +++ b/third_party/upload.py @@ -1,4 +1,5 @@ #!/usr/bin/env python +# coding: utf-8 # # Copyright 2007 Google Inc. # @@ -215,7 +216,7 @@ class AbstractRpcServer(object): def _CreateRequest(self, url, data=None): """Creates a new urllib request.""" logging.debug("Creating request for: '%s' with payload:\n%s", url, data) - req = urllib2.Request(url, data=data) + req = urllib2.Request(url, data=data, headers={"Accept": "text/plain"}) if self.host_override: req.add_header("Host", self.host_override) for key, value in self.extra_headers.iteritems(): @@ -399,14 +400,13 @@ class AbstractRpcServer(object): raise elif e.code == 401 or e.code == 302: self._Authenticate() -## elif e.code >= 500 and e.code < 600: -## # Server Error - try again. -## continue elif e.code == 301: # Handle permanent redirect manually. url = e.info()["location"] url_loc = urlparse.urlparse(url) self.host = '%s://%s' % (url_loc[0], url_loc[1]) + elif e.code >= 500: + ErrorExit(e.read()) else: raise finally: @@ -532,14 +532,13 @@ group.add_option("--account_type", action="store", dest="account_type", "valid choices are 'GOOGLE' and 'HOSTED').")) # Issue group = parser.add_option_group("Issue options") -group.add_option("-d", "--description", action="store", dest="description", - metavar="DESCRIPTION", default=None, - help="Optional description when creating an issue.") -group.add_option("-f", "--description_file", action="store", - dest="description_file", metavar="DESCRIPTION_FILE", +group.add_option("-t", "--title", action="store", dest="title", + help="New issue subject or new patch set title") +group.add_option("-m", "--message", action="store", dest="message", default=None, - help="Optional path of a file that contains " - "the description when creating an issue.") + help="New issue description or new patch set message") +group.add_option("-F", "--file", action="store", dest="file", + default=None, help="Read the message above from file.") group.add_option("-r", "--reviewers", action="store", dest="reviewers", metavar="REVIEWERS", default=None, help="Add reviewers (comma separated email addresses).") @@ -551,10 +550,6 @@ group.add_option("--private", action="store_true", dest="private", help="Make the issue restricted to reviewers and those CCed") # Upload options group = parser.add_option_group("Patch options") -group.add_option("-m", "--message", action="store", dest="message", - metavar="MESSAGE", default=None, - help="A message to identify the patch. " - "Will prompt if omitted.") group.add_option("-i", "--issue", type="int", action="store", metavar="ISSUE", default=None, help="Issue number to which to add. Defaults to new issue.") @@ -785,7 +780,7 @@ class VersionControlSystem(object): options: Command line options. """ self.options = options - + def GetGUID(self): """Return string to distinguish the repository from others, for example to query all opened review issues for it""" @@ -945,7 +940,7 @@ class SubversionVCS(VersionControlSystem): # Result is cached to not guess it over and over again in GetBaseFile(). required = self.options.download_base or self.options.revision is not None self.svn_base = self._GuessBase(required) - + def GetGUID(self): return self._GetInfo("Repository UUID") @@ -980,7 +975,7 @@ class SubversionVCS(VersionControlSystem): if required: ErrorExit("Can't find URL in output from svn info") return None - + def _GetInfo(self, key): """Parses 'svn info' for current dir. Returns value for key or None""" for line in RunShell(["svn", "info"]).splitlines(): @@ -1223,7 +1218,7 @@ class GitVCS(VersionControlSystem): self.hashes = {} # Map of new filename -> old filename for renames. self.renames = {} - + def GetGUID(self): revlist = RunShell("git rev-list --parents HEAD".split()).splitlines() # M-A: Return the 1st root hash, there could be multiple when a @@ -1310,7 +1305,7 @@ class GitVCS(VersionControlSystem): # then the diff of everything except deleted files with rename and copy # support enabled. cmd = [ - "git", "diff", "--no-ext-diff", "--full-index", "--ignore-submodules" + "git", "diff", "--no-color", "--no-ext-diff", "--full-index", "--ignore-submodules" ] diff = RunShell(cmd + ["--diff-filter=D"] + extra_args, env=env, silent_ok=True) @@ -1464,11 +1459,13 @@ class MercurialVCS(VersionControlSystem): def _GetRelPath(self, filename): """Get relative path of a file according to the current directory, given its logical path in the repo.""" - assert filename.startswith(self.subdir), (filename, self.subdir) - return filename[len(self.subdir):].lstrip(r"\/") + absname = os.path.join(self.repo_dir, filename) + return os.path.relpath(absname) def GenerateDiff(self, extra_args): - cmd = ["hg", "diff", "--git", "-r", self.base_rev] + extra_args + cmd = [ + "hg", "diff", "--color", "never", "--git", "-r", self.base_rev + ] + extra_args data = RunShell(cmd, silent_ok=True) svndiff = [] filecount = 0 @@ -1494,7 +1491,8 @@ class MercurialVCS(VersionControlSystem): def GetUnknownFiles(self): """Return a list of files unknown to the VCS.""" args = [] - status = RunShell(["hg", "status", "--rev", self.base_rev, "-u", "."], + status = RunShell( + ["hg", "status", "--color", "never", "--rev", self.base_rev, "-u", "."], silent_ok=True) unknown_files = [] for line in status.splitlines(): @@ -1504,15 +1502,16 @@ class MercurialVCS(VersionControlSystem): return unknown_files def GetBaseFile(self, filename): - # "hg status" and "hg cat" both take a path relative to the current subdir - # rather than to the repo root, but "hg diff" has given us the full path - # to the repo root. + # "hg status" and "hg cat" both take a path relative to the current subdir, + # but "hg diff" has given us the path relative to the repo root. base_content = "" new_content = None is_binary = False oldrelpath = relpath = self._GetRelPath(filename) # "hg status -C" returns two lines for moved/copied files, one otherwise - out = RunShell(["hg", "status", "-C", "--rev", self.base_rev, relpath]) + out = RunShell( + [ "hg", "status", "--color", "never", "-C", "--rev", self.base_rev, + relpath]) out = out.splitlines() # HACK: strip error message about missing file/directory if it isn't in # the working copy @@ -1575,15 +1574,15 @@ class PerforceVCS(VersionControlSystem): ConfirmLogin() - if not options.message: + if not options.title: description = self.RunPerforceCommand(["describe", self.p4_changelist], marshal_output=True) if description and "desc" in description: # Rietveld doesn't support multi-line descriptions - raw_message = description["desc"].strip() - lines = raw_message.splitlines() + raw_title = description["desc"].strip() + lines = raw_title.splitlines() if len(lines): - options.message = lines[0] + options.title = lines[0] def GetGUID(self): """For now we don't know how to get repository ID for Perforce""" @@ -1974,10 +1973,12 @@ def GuessVCSName(options): if res != None: return res - # Subversion has a .svn in all working directories. - if os.path.isdir('.svn'): - logging.info("Guessed VCS = Subversion") - return (VCS_SUBVERSION, None) + # Subversion from 1.7 has a single centralized .svn folder + # ( see http://subversion.apache.org/docs/release-notes/1.7.html#wc-ng ) + # That's why we use 'svn info' instead of checking for .svn dir + res = RunDetectCommand(VCS_SUBVERSION, ["svn", "info"]) + if res != None: + return res # Git has a command to test if you're in a git tree. # Try running it, but don't die if we don't have git installed. @@ -2219,20 +2220,13 @@ def RealMain(argv, data=None): files = vcs.GetBaseFiles(data) if verbosity >= 1: print "Upload server:", options.server, "(change with -s/--server)" - if options.issue: - prompt = "Message describing this patch set: " - else: - prompt = "New issue subject: " - message = options.message or raw_input(prompt).strip() - if not message: - ErrorExit("A non-empty message is required") rpc_server = GetRpcServer(options.server, options.email, options.host, options.save_cookies, options.account_type) - form_fields = [("subject", message)] - + form_fields = [] + repo_guid = vcs.GetGUID() if repo_guid: form_fields.append(("repo_guid", repo_guid)) @@ -2256,15 +2250,40 @@ def RealMain(argv, data=None): for cc in options.cc.split(','): CheckReviewer(cc) form_fields.append(("cc", options.cc)) - description = options.description - if options.description_file: - if options.description: - ErrorExit("Can't specify description and description_file") - file = open(options.description_file, 'r') - description = file.read() + + # Process --message, --title and --file. + message = options.message or "" + title = options.title or "" + if options.file: + if options.message: + ErrorExit("Can't specify both message and message file options") + file = open(options.file, 'r') + message = file.read() file.close() - if description: - form_fields.append(("description", description)) + if options.issue: + prompt = "Title describing this patch set: " + else: + prompt = "New issue subject: " + title = ( + title or message.split('\n', 1)[0].strip() or raw_input(prompt).strip()) + if not title and not options.issue: + ErrorExit("A non-empty title is required for a new issue") + # For existing issues, it's fine to give a patchset an empty name. Rietveld + # doesn't accept that so use a whitespace. + title = title or " " + if len(title) > 100: + title = title[:99] + '…' + if title and not options.issue: + message = message or title + + form_fields.append(("subject", title)) + if message: + if not options.issue: + form_fields.append(("description", message)) + else: + # TODO: [ ] Use //publish to add a comment. + pass + # Send a hash of all the base file so the server can determine if a copy # already exists in an earlier patchset. base_hashes = "" @@ -2282,10 +2301,6 @@ def RealMain(argv, data=None): form_fields.append(("private", "1")) if options.send_patch: options.send_mail = True - # If we're uploading base files, don't send the email before the uploads, so - # that it contains the file status. - if options.send_mail and options.download_base: - form_fields.append(("send_mail", "1")) if not options.download_base: form_fields.append(("content_upload", "1")) if len(data) > MAX_UPLOAD_SIZE: @@ -2320,11 +2335,15 @@ def RealMain(argv, data=None): if not options.download_base: vcs.UploadBaseFiles(issue, rpc_server, patches, patchset, options, files) - if options.send_mail: - payload = "" - if options.send_patch: - payload=urllib.urlencode({"attach_patch": "yes"}) - rpc_server.Send("/" + issue + "/mail", payload=payload) + + payload = {} # payload for final request + if options.send_mail: + payload["send_mail"] = "yes" + if options.send_patch: + payload["attach_patch"] = "yes" + payload = urllib.urlencode(payload) + rpc_server.Send("/" + issue + "/upload_complete/" + (patchset or ""), + payload=payload) return issue, patchset