Skip to content

C
Chromium_Depot_tools
Commit c48fb842 authored by Edward Lesmes's avatar Edward Lesmes Committed by LUCI CQ
Browse files
Options

Revert "git-cl: Refactor CMDUploadChange" 

This reverts commit 9f29465e.

Reason for revert:
line 2302, in CMDUploadChange
    change_id = change_ids[0]
IndexError: list index out of range

Original change's description:
> git-cl: Refactor CMDUploadChange
> 
> Changes:
> - Add _GetDescriptionForUpload and _GetTitleForUpload.
> - Add ensure_change_id to ChangeDescription, that ensures the
>   description has a particular Change-Id.
> - If more than a Change-Id was entered on new issue upload, remove
>   all, and add a new one.
> 
> Change-Id: I0eae474eb07ea3036973b18571f72a80da425b21
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2101811
> Reviewed-by: Josip Sokcevic <sokcevic@google.com>
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

TBR=ehmaldonado@chromium.org,apolito@google.com,infra-scoped@luci-project-accounts.iam.gserviceaccount.com,sokcevic@google.com

Change-Id: Ibb76539066c103ec951fa8d02684bc10f84549bf
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2103531Reviewed-by: 's avatarEdward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
parent 9f29465e
Hide whitespace changes
Inline Side-by-side
Showing with 103 additions and 98 deletions
git_cl.py
View file @ c48fb842
......@@ -1447,62 +1447,6 @@ class Changelist(object):
p = subprocess2.Popen(['vpython', PRESUBMIT_SUPPORT] + args)
p.wait()
def _GetDescriptionForUpload(self, options, git_diff_args, files):
# Get description message for upload.
if self.GetIssue():
description = self.FetchDescription()
elif options.message:
description = options.message
else:
description = _create_description_from_log(git_diff_args)
if options.title and options.squash:
description = options.title + '\n\n' + message
# Extract bug number from branch name.
bug = options.bug
fixed = options.fixed
match = re.match(r'(?P<type>bug|fix(?:e[sd])?)[_-]?(?P<bugnum>\d+)',
self.GetBranch())
if not bug and not fixed and match:
if match.group('type') == 'bug':
bug = match.group('bugnum')
else:
fixed = match.group('bugnum')
change_description = ChangeDescription(description, bug, fixed)
# Set the reviewer list now so that presubmit checks can access it.
if options.reviewers or options.tbrs or options.add_owners_to:
change_description.update_reviewers(
options.reviewers, options.tbrs, options.add_owners_to, files,
self.GetAuthor())
return change_description
def _GetTitleForUpload(self, options):
# When not squashing, just return options.title.
if not options.squash:
return options.title
# On first upload, patchset title is always this string, while options.title
# gets converted to first line of message.
if not self.GetIssue():
return 'Initial upload'
# When uploading subsequent patchsets, options.message is taken as the title
# if options.title is not provided.
if options.title:
return options.title
if options.message:
return options.message.strip()
# Use the subject of the last commit as title by default.
title = RunGit(
['show', '-s', '--format=%s', 'HEAD']).strip()
if options.force:
return title
return ask_for_data('Title for patchset [%s]: ' % title) or title
def CMDUpload(self, options, git_diff_args, orig_args):
"""Uploads a change to codereview."""
custom_cl_base = None
......@@ -1522,13 +1466,30 @@ class Changelist(object):
self.EnsureAuthenticated(force=options.force)
self.EnsureCanUploadPatchset(force=options.force)
# Get description message for upload.
if self.GetIssue():
description = self.FetchDescription()
elif options.message:
description = options.message
else:
description = _create_description_from_log(git_diff_args)
if options.title and options.squash:
description = options.title + '\n\n' + message
# Apply watchlists on upload.
watchlist = watchlists.Watchlists(settings.GetRoot())
files = self.GetAffectedFiles(base_branch)
if not options.bypass_watchlists:
self.ExtendCC(watchlist.GetWatchersForPaths(files))
change_desc = self._GetDescriptionForUpload(options, git_diff_args, files)
if options.reviewers or options.tbrs or options.add_owners_to:
# Set the reviewer list now so that presubmit checks can access it.
change_description = ChangeDescription(description)
change_description.update_reviewers(
options.reviewers, options.tbrs, options.add_owners_to, files,
self.GetAuthor())
description = change_description.description
if not options.bypass_hooks:
hook_results = self.RunHook(
committing=False,
......@@ -1536,20 +1497,19 @@ class Changelist(object):
verbose=options.verbose,
parallel=options.parallel,
upstream=base_branch,
description=change_desc.description,
description=description,
all_files=False)
self.ExtendCC(hook_results['more_cc'])
print_stats(git_diff_args)
ret = self.CMDUploadChange(
options, git_diff_args, custom_cl_base, change_desc)
options, git_diff_args, custom_cl_base, description)
if not ret:
self._GitSetBranchConfigValue(
'last-upload-hash', RunGit(['rev-parse', 'HEAD']).strip())
# Run post upload hooks, if specified.
if settings.GetRunPostUploadHook():
self.RunPostUploadHook(
options.verbose, base_branch, change_desc.description)
self.RunPostUploadHook(options.verbose, base_branch, description)
# Upload all dependencies if specified.
if options.dependencies:
......@@ -2277,7 +2237,7 @@ class Changelist(object):
return push_stdout
def CMDUploadChange(
self, options, git_diff_args, custom_cl_base, change_desc):
self, options, git_diff_args, custom_cl_base, message):
"""Upload the current branch to Gerrit."""
if options.squash is None:
# Load default for user, repo, squash=true, in this order.
......@@ -2285,41 +2245,101 @@ class Changelist(object):
remote, remote_branch = self.GetRemoteBranch()
branch = GetTargetRef(remote, remote_branch, options.target_branch)
# This may be None; default fallback value is determined in logic below.
title = options.title
# Extract bug number from branch name.
bug = options.bug
fixed = options.fixed
match = re.match(r'(?P<type>bug|fix(?:e[sd])?)[_-]?(?P<bugnum>\d+)',
self.GetBranch())
if not bug and not fixed and match:
if match.group('type') == 'bug':
bug = match.group('bugnum')
else:
fixed = match.group('bugnum')
if options.squash:
self._GerritCommitMsgHookCheck(offer_removal=not options.force)
if self.GetIssue():
if not title:
if options.message:
# When uploading a subsequent patchset, -m|--message is taken
# as the patchset title if --title was not provided.
title = options.message.strip()
else:
default_title = RunGit(
['show', '-s', '--format=%s', 'HEAD']).strip()
if options.force:
title = default_title
else:
title = ask_for_data(
'Title for patchset [%s]: ' % default_title) or default_title
# User requested to change description
if options.edit_description:
change_desc = ChangeDescription(message, bug=bug, fixed=fixed)
change_desc.prompt()
message = change_desc.description
change_id = self._GetChangeDetail()['change_id']
change_desc.ensure_change_id(change_id)
# Make sure that the Change-Id in the description matches the one
# fetched from Gerrit.
footer_change_ids = git_footers.get_footer_change_id(message)
if footer_change_ids != [change_id]:
if footer_change_ids:
# Remove any existing Change-Id footers since they don't match the
# expected change_id footer.
message = git_footers.remove_footer(message, 'Change-Id')
# Add the expected Change-Id footer.
message = git_footers.add_footer_change_id(message, change_id)
print('WARNING: Change-Id has been set to Change-Id fetched from '
'Gerrit. Use `git cl issue 0` if you want to clear it and '
'set a new one.')
change_desc = ChangeDescription(message, bug=bug, fixed=fixed)
else: # if not self.GetIssue()
change_desc = ChangeDescription(message, bug=bug, fixed=fixed)
if not options.force:
change_desc.prompt()
# On first upload, patchset title is always this string, while
# --title flag gets converted to first line of message.
title = 'Initial upload'
if not change_desc.description:
DieWithError("Description is empty. Aborting...")
change_ids = git_footers.get_footer_change_id(change_desc.description)
if len(change_ids) > 1:
DieWithError('too many Change-Id footers, at most 1 allowed.')
if not change_ids:
# Generate the Change-Id automatically.
change_desc.set_description(git_footers.add_footer_change_id(
change_desc.description,
GenerateGerritChangeId(change_desc.description)))
change_ids = git_footers.get_footer_change_id(change_desc.description)
assert len(change_ids) == 1
change_id = change_ids[0]
if len(change_ids) != 1:
change_id = GenerateGerritChangeId(change_desc.description)
change_desc.ensure_change_id(change_id)
if options.preserve_tryjobs:
change_desc.set_preserve_tryjobs()
remote, upstream_branch = self.FetchUpstreamTuple(self.GetBranch())
parent = self._ComputeParent(
remote, upstream_branch, custom_cl_base, options.force, change_desc)
parent = self._ComputeParent(remote, upstream_branch, custom_cl_base,
options.force, change_desc)
tree = RunGit(['rev-parse', 'HEAD:']).strip()
with gclient_utils.temporary_file() as desc_tempfile:
gclient_utils.FileWrite(desc_tempfile, change_desc.description)
ref_to_push = RunGit(
['commit-tree', tree, '-p', parent, '-F', desc_tempfile]).strip()
else: # if not options.squash
change_desc = ChangeDescription(message)
if not change_desc.description:
DieWithError("Description is empty. Aborting...")
if not git_footers.get_footer_change_id(change_desc.description):
DownloadGerritHook(False)
change_desc.set_description(
self._AddChangeIdToCommitMessage(
change_desc.description, git_diff_args))
self._AddChangeIdToCommitMessage(message, git_diff_args))
ref_to_push = 'HEAD'
# For no-squash mode, we assume the remote called "origin" is the one we
# want. It is not worthwhile to support different workflows for
......@@ -2327,6 +2347,7 @@ class Changelist(object):
parent = 'origin/%s' % branch
change_id = git_footers.get_footer_change_id(change_desc.description)[0]
assert change_desc
SaveDescriptionBackup(change_desc)
commits = RunGitSilent(['rev-list', '%s..%s' % (parent,
ref_to_push)]).splitlines()
......@@ -2378,7 +2399,6 @@ class Changelist(object):
# TODO(tandrii): options.message should be posted as a comment
# if --send-mail is set on non-initial upload as Rietveld used to do it.
title = self._GetTitleForUpload(options)
if title:
# Punctuation and whitespace in |title| must be percent-encoded.
refspec_opts.append('m=' + gerrit_util.PercentEncodeForGitRef(title))
......@@ -2653,21 +2673,6 @@ class ChangeDescription(object):
lines.pop(-1)
self._description_lines = lines
def ensure_change_id(self, change_id):
description = self.description
footer_change_ids = git_footers.get_footer_change_id(description)
# Make sure that the Change-Id in the description matches the given one.
if footer_change_ids != [change_id]:
if footer_change_ids:
# Remove any existing Change-Id footers since they don't match the
# expected change_id footer.
description = git_footers.remove_footer(description, 'Change-Id')
print('WARNING: Change-Id has been set to %s. Use `git cl issue 0` '
'if you want to set a new one.')
# Add the expected Change-Id footer.
description = git_footers.add_footer_change_id(description, change_id)
self.set_description(description)
def update_reviewers(
self, reviewers, tbrs, add_owners_to, affected_files, author_email):
"""Rewrites the R=/TBR= line(s) as a single line each.
......
tests/git_cl_test.py
View file @ c48fb842
......@@ -868,6 +868,17 @@ class TestGitCl(unittest.TestCase):
self.mockGit.config['gerrit.override-squash-uploads'] = (
'true' if squash_mode == 'override_squash' else 'false')
# If issue is given, then description is fetched from Gerrit instead.
if issue is None:
if squash:
title = 'Initial_upload'
else:
if not title:
calls += [
((['git', 'show', '-s', '--format=%s', 'HEAD'],), ''),
(('ask_for_data', 'Title for patchset []: '), 'User input'),
]
title = 'User_input'
if not git_footers.get_footer_change_id(description) and not squash:
calls += [
(('DownloadGerritHook', False), ''),
......@@ -934,17 +945,6 @@ class TestGitCl(unittest.TestCase):
ref_suffix = '%notify=NONE'
metrics_arguments.append('notify=NONE')
# If issue is given, then description is fetched from Gerrit instead.
if issue is None:
if squash:
title = 'Initial_upload'
else:
if not title:
calls += [
((['git', 'show', '-s', '--format=%s', 'HEAD'],), ''),
(('ask_for_data', 'Title for patchset []: '), 'User input'),
]
title = 'User_input'
if title:
ref_suffix += ',m=' + title
metrics_arguments.append('m')
......@@ -1297,7 +1297,7 @@ class TestGitCl(unittest.TestCase):
self._run_gerrit_upload_test(
['-r', 'foo@example.com', '--send-mail'],
'desc ✔\n\nBUG=\n\nChange-Id: I123456789',
reviewers=['foo@example.com'],
['foo@example.com'],
squash=False,
squash_mode='override_nosquash',
notify=True,
......
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