Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(68)

Unified Diff: git_cl.py

Issue 23072039: Fix R= line rewriter to handle TBRs well. (Closed) Base URL: https://chromium.googlesource.com/chromium/tools/depot_tools.git@master
Patch Set: More comments. Created 7 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | tests/git_cl_test.py » ('j') | tests/git_cl_test.py » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index dc351a2123a9f1bb29c2c7cf95806e7b4dcf7d1e..d0a27faa5af3aa5b93bf25a69650e0b8401102eb 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -849,80 +849,111 @@ def GetCodereviewSettingsInteractively():
class ChangeDescription(object):
"""Contains a parsed form of the change description."""
R_LINE = r'^[ \t]*(TBR|R)[ \t]*=[ \t]*(.*?)[ \t]*$'
+ BUG_LINE = r'^[ \t]*(BUG)[ \t]*=[ \t]*(.*?)[ \t]*$'
def __init__(self, description):
- self._description = (description or '').strip()
+ self._description_lines = (description or '').strip().splitlines()
- @property
- def description(self):
- return self._description
+ @property # www.logilab.org/ticket/89786
+ def description(self): # pylint: disable=E0202
+ return '\n'.join(self._description_lines)
+
+ @description.setter
+ def description(self, desc): # pylint: disable=E0202
M-A Ruel 2013/08/30 15:52:36 I must say I'm not a fan of keeping a setter here,
agable 2013/09/11 21:08:05 Yeah, you're right that the setter is confusing. I
+ if isinstance(desc, basestring):
+ lines = desc.strip().splitlines()
M-A Ruel 2013/08/30 15:52:36 .strip() is not necessary because of lines 867-870
agable 2013/09/11 21:08:05 Done.
+ else:
+ lines = [line.strip() for line in desc]
M-A Ruel 2013/08/30 15:52:36 That's wrong, you are left-aligning everything.
agable 2013/09/11 21:08:05 Done.
+ while lines and not lines[0]:
+ lines = lines[1:]
M-A Ruel 2013/08/30 15:52:36 lines.pop(0) will just the same faster.
agable 2013/09/11 21:08:05 Done.
+ while lines and not lines[-1]:
+ lines = lines.pop()
M-A Ruel 2013/08/30 15:52:36 That's wrong since you are keeping the last line;
agable 2013/09/11 21:08:05 Done.
+ self._description_lines = lines
def update_reviewers(self, reviewers):
- """Rewrites the R=/TBR= line(s) as a single line."""
+ """Rewrites the R=/TBR= line(s) as a single line each."""
assert isinstance(reviewers, list), reviewers
if not reviewers:
return
- regexp = re.compile(self.R_LINE, re.MULTILINE)
- matches = list(regexp.finditer(self._description))
- is_tbr = any(m.group(1) == 'TBR' for m in matches)
- if len(matches) > 1:
- # Erase all except the first one.
- for i in xrange(len(matches) - 1, 0, -1):
- self._description = (
- self._description[:matches[i].start()] +
- self._description[matches[i].end():])
-
- if is_tbr:
- new_r_line = 'TBR=' + ', '.join(reviewers)
- else:
- new_r_line = 'R=' + ', '.join(reviewers)
- if matches:
- self._description = (
- self._description[:matches[0].start()] + new_r_line +
- self._description[matches[0].end():]).strip()
+ # Get the set of R= and TBR= lines and remove them from the desciption.
+ regexp = re.compile(self.R_LINE)
+ matches = [regexp.match(line) for line in self._description_lines]
+ new_desc = [self._description_lines[i] for i in range(len(matches))
+ if not matches[i]]
+ self.description = new_desc
+
+ # Construct new unified R= and TBR= lines.
+ r_names = []
+ tbr_names = []
+ for match in matches:
+ if match is None:
+ continue
+ is_tbr = match.group(1) == 'TBR'
+ people = cleanup_list([match.group(2).strip()])
+ if is_tbr:
+ tbr_names.extend(people)
+ else:
+ r_names.extend(people)
+ r_final_names = reviewers[:]
+ for name in r_names:
+ if name not in r_final_names:
+ r_final_names.append(name)
+ new_r_line = 'R=' + ', '.join(r_final_names) if r_final_names else None
+ new_tbr_line = 'TBR=' + ', '.join(tbr_names) if tbr_names else None
+
+ # Put the new lines in the desciption where the old first R= line was.
+ line_loc = next((i for i, match in enumerate(matches) if match), -1)
+ if line_loc != -1 and line_loc < len(self._description_lines):
+ if new_tbr_line:
+ self._description_lines.insert(line_loc, new_tbr_line)
+ if new_r_line:
+ self._description_lines.insert(line_loc, new_r_line)
else:
- self.append_footer(new_r_line)
+ if new_r_line:
+ self.append_footer(new_r_line)
+ if new_tbr_line:
+ self.append_footer(new_tbr_line)
def prompt(self):
"""Asks the user to update the description."""
- self._description = (
- '# Enter a description of the change.\n'
- '# This will be displayed on the codereview site.\n'
- '# The first line will also be used as the subject of the review.\n'
+ self.description = [
+ '# Enter a description of the change.',
+ '# This will be displayed on the codereview site.',
+ '# The first line will also be used as the subject of the review.',
'#--------------------This line is 72 characters long'
- '--------------------\n'
- ) + self._description
+ '--------------------',
+ ] + self._description_lines
- if '\nBUG=' not in self._description:
+ regexp = re.compile(self.BUG_LINE)
+ if not any((regexp.match(line) for line in self._description_lines)):
self.append_footer('BUG=')
- content = gclient_utils.RunEditor(self._description, True,
+ content = gclient_utils.RunEditor(self.description, True,
git_editor=settings.GetGitEditor())
if not content:
DieWithError('Running editor failed')
+ lines = content.splitlines()
# Strip off comments.
- content = re.compile(r'^#.*$', re.MULTILINE).sub('', content).strip()
- if not content:
+ clean_lines = [line.strip() for line in lines
+ if not re.match(r'^#.*$', line)]
+ if not clean_lines:
DieWithError('No CL description, aborting')
- self._description = content
+ self.description = clean_lines
def append_footer(self, line):
- # Adds a LF if the last line did not have 'FOO=BAR' or if the new one isn't.
- if self._description:
- if '\n' not in self._description:
- self._description += '\n'
- else:
- last_line = self._description.rsplit('\n', 1)[1]
- if (not presubmit_support.Change.TAG_LINE_RE.match(last_line) or
- not presubmit_support.Change.TAG_LINE_RE.match(line)):
- self._description += '\n'
- self._description += '\n' + line
+ if self._description_lines:
+ # Add an empty line if either the last line or the new line isn't a tag.
+ last_line = self._description_lines[-1]
+ if (not presubmit_support.Change.TAG_LINE_RE.match(last_line) or
+ not presubmit_support.Change.TAG_LINE_RE.match(line)):
+ self._description_lines.append('')
+ self._description_lines.append(line)
def get_reviewers(self):
"""Retrieves the list of reviewers."""
- regexp = re.compile(self.R_LINE, re.MULTILINE)
- reviewers = [i.group(2).strip() for i in regexp.finditer(self._description)]
+ matches = [re.match(self.R_LINE, line) for line in self._description_lines]
+ reviewers = [match.group(2).strip() for match in matches if match]
return cleanup_list(reviewers)
« no previous file with comments | « no previous file | tests/git_cl_test.py » ('j') | tests/git_cl_test.py » ('J')

Powered by Google App Engine
This is Rietveld 408576698