Reland of Gerrit git cl: implement git cl patch. (patchset #1 id:1 of https://codereview.chromium.org/1848393002/ )
Reason for revert:
nope, the windows breakage is due to apply_issue failing to connect to Rietveld.
Original issue's description:
> Revert of Gerrit git cl: implement git cl patch. (patchset #7 id:120001 of https://codereview.chromium.org/1852593002/ )
>
> Reason for revert:
> just in case.
>
> Original issue's description:
> > Gerrit git cl: implement git cl patch.
> >
> > BUG=579182
> >
> > Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299644
>
> TBR=andybons@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=579182
>
> Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299645
TBR=andybons@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=579182
Review URL: https://codereview.chromium.org/1852803002
git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@299647 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/git_cl.py b/git_cl.py
index b3d1d2e..4c6bb10 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -834,6 +834,43 @@
return None
+class _ParsedIssueNumberArgument(object):
+ def __init__(self, issue=None, patchset=None, hostname=None):
+ self.issue = issue
+ self.patchset = patchset
+ self.hostname = hostname
+
+ @property
+ def valid(self):
+ return self.issue is not None
+
+
+class _RietveldParsedIssueNumberArgument(_ParsedIssueNumberArgument):
+ def __init__(self, *args, **kwargs):
+ self.patch_url = kwargs.pop('patch_url', None)
+ super(_RietveldParsedIssueNumberArgument, self).__init__(*args, **kwargs)
+
+
+def ParseIssueNumberArgument(arg):
+ """Parses the issue argument and returns _ParsedIssueNumberArgument."""
+ fail_result = _ParsedIssueNumberArgument()
+
+ if arg.isdigit():
+ return _ParsedIssueNumberArgument(issue=int(arg))
+ if not arg.startswith('http'):
+ return fail_result
+ url = gclient_utils.UpgradeToHttps(arg)
+ try:
+ parsed_url = urlparse.urlparse(url)
+ except ValueError:
+ return fail_result
+ for cls in _CODEREVIEW_IMPLEMENTATIONS.itervalues():
+ tmp = cls.ParseIssueURL(parsed_url)
+ if tmp is not None:
+ return tmp
+ return fail_result
+
+
class Changelist(object):
"""Changelist works with one changelist in local branch.
@@ -1257,6 +1294,20 @@
('%s\nMaybe your depot_tools is out of date?\n'
'If all fails, contact maruel@') % e)
+ def CMDPatchIssue(self, issue_arg, reject, nocommit, directory):
+ """Fetches and applies the issue patch from codereview to local branch."""
+ if issue_arg.isdigit():
+ parsed_issue_arg = _RietveldParsedIssueNumberArgument(int(issue_arg))
+ else:
+ # Assume url.
+ parsed_issue_arg = self._codereview_impl.ParseIssueURL(
+ urlparse.urlparse(issue_arg))
+ if not parsed_issue_arg or not parsed_issue_arg.valid:
+ DieWithError('Failed to parse issue argument "%s". '
+ 'Must be an issue number or a valid URL.' % issue_arg)
+ return self._codereview_impl.CMDPatchWithParsedIssue(
+ parsed_issue_arg, reject, nocommit, directory)
+
# Forward methods to codereview specific implementation.
def CloseIssue(self):
@@ -1346,6 +1397,26 @@
"""Returns the most recent patchset number from the codereview site."""
raise NotImplementedError()
+ def CMDPatchWithParsedIssue(self, parsed_issue_arg, reject, nocommit,
+ directory):
+ """Fetches and applies the issue.
+
+ Arguments:
+ parsed_issue_arg: instance of _ParsedIssueNumberArgument.
+ reject: if True, reject the failed patch instead of switching to 3-way
+ merge. Rietveld only.
+ nocommit: do not commit the patch, thus leave the tree dirty. Rietveld
+ only.
+ directory: switch to directory before applying the patch. Rietveld only.
+ """
+ raise NotImplementedError()
+
+ @staticmethod
+ def ParseIssueURL(parsed_url):
+ """Parses url and returns instance of _ParsedIssueNumberArgument or None if
+ failed."""
+ raise NotImplementedError()
+
class _RietveldChangelistImpl(_ChangelistCodereviewBase):
def __init__(self, changelist, auth_config=None, rietveld_server=None):
@@ -1518,6 +1589,94 @@
def GetRieveldObjForPresubmit(self):
return self.RpcServer()
+ def CMDPatchWithParsedIssue(self, parsed_issue_arg, reject, nocommit,
+ directory):
+ # TODO(maruel): Use apply_issue.py
+
+ # PatchIssue should never be called with a dirty tree. It is up to the
+ # caller to check this, but just in case we assert here since the
+ # consequences of the caller not checking this could be dire.
+ assert(not git_common.is_dirty_git_tree('apply'))
+ assert(parsed_issue_arg.valid)
+ self._changelist.issue = parsed_issue_arg.issue
+ if parsed_issue_arg.hostname:
+ self._rietveld_server = 'https://%s' % parsed_issue_arg.hostname
+
+ if parsed_issue_arg.patch_url:
+ assert parsed_issue_arg.patchset
+ patchset = parsed_issue_arg.patchset
+ patch_data = urllib2.urlopen(parsed_issue_arg.patch_url).read()
+ else:
+ patchset = parsed_issue_arg.patchset or self.GetMostRecentPatchset()
+ patch_data = self.GetPatchSetDiff(self.GetIssue(), patchset)
+
+ # Switch up to the top-level directory, if necessary, in preparation for
+ # applying the patch.
+ top = settings.GetRelativeRoot()
+ if top:
+ os.chdir(top)
+
+ # Git patches have a/ at the beginning of source paths. We strip that out
+ # with a sed script rather than the -p flag to patch so we can feed either
+ # Git or svn-style patches into the same apply command.
+ # re.sub() should be used but flags=re.MULTILINE is only in python 2.7.
+ try:
+ patch_data = subprocess2.check_output(
+ ['sed', '-e', 's|^--- a/|--- |; s|^+++ b/|+++ |'], stdin=patch_data)
+ except subprocess2.CalledProcessError:
+ DieWithError('Git patch mungling failed.')
+ logging.info(patch_data)
+
+ # We use "git apply" to apply the patch instead of "patch" so that we can
+ # pick up file adds.
+ # The --index flag means: also insert into the index (so we catch adds).
+ cmd = ['git', 'apply', '--index', '-p0']
+ if directory:
+ cmd.extend(('--directory', directory))
+ if reject:
+ cmd.append('--reject')
+ elif IsGitVersionAtLeast('1.7.12'):
+ cmd.append('--3way')
+ try:
+ subprocess2.check_call(cmd, env=GetNoGitPagerEnv(),
+ stdin=patch_data, stdout=subprocess2.VOID)
+ except subprocess2.CalledProcessError:
+ print 'Failed to apply the patch'
+ return 1
+
+ # If we had an issue, commit the current state and register the issue.
+ if not nocommit:
+ RunGit(['commit', '-m', (self.GetDescription() + '\n\n' +
+ 'patch from issue %(i)s at patchset '
+ '%(p)s (http://crrev.com/%(i)s#ps%(p)s)'
+ % {'i': self.GetIssue(), 'p': patchset})])
+ self.SetIssue(self.GetIssue())
+ self.SetPatchset(patchset)
+ print "Committed patch locally."
+ else:
+ print "Patch applied to index."
+ return 0
+
+ @staticmethod
+ def ParseIssueURL(parsed_url):
+ if not parsed_url.scheme or not parsed_url.scheme.startswith('http'):
+ return None
+ # Typical url: https://domain/<issue_number>[/[other]]
+ match = re.match('/(\d+)(/.*)?$', parsed_url.path)
+ if match:
+ return _RietveldParsedIssueNumberArgument(
+ issue=int(match.group(1)),
+ hostname=parsed_url.netloc)
+ # Rietveld patch: https://domain/download/issue<number>_<patchset>.diff
+ match = re.match(r'/download/issue(\d+)_(\d+).diff$', parsed_url.path)
+ if match:
+ return _RietveldParsedIssueNumberArgument(
+ issue=int(match.group(1)),
+ patchset=int(match.group(2)),
+ hostname=parsed_url.netloc,
+ patch_url=gclient_utils.UpgradeToHttps(parsed_url.geturl()))
+ return None
+
class _GerritChangelistImpl(_ChangelistCodereviewBase):
def __init__(self, changelist, auth_config=None):
@@ -1696,6 +1855,63 @@
print('Issue %s has been submitted.' % self.GetIssueURL())
return 0
+ def CMDPatchWithParsedIssue(self, parsed_issue_arg, reject, nocommit,
+ directory):
+ assert not reject
+ assert not nocommit
+ assert not directory
+ assert parsed_issue_arg.valid
+
+ self._changelist.issue = parsed_issue_arg.issue
+
+ if parsed_issue_arg.hostname:
+ self._gerrit_host = parsed_issue_arg.hostname
+ self._gerrit_server = 'https://%s' % self._gerrit_host
+
+ detail = self._GetChangeDetail(['ALL_REVISIONS'])
+
+ if not parsed_issue_arg.patchset:
+ # Use current revision by default.
+ revision_info = detail['revisions'][detail['current_revision']]
+ patchset = int(revision_info['_number'])
+ else:
+ patchset = parsed_issue_arg.patchset
+ for revision_info in detail['revisions'].itervalues():
+ if int(revision_info['_number']) == parsed_issue_arg.patchset:
+ break
+ else:
+ DieWithError('Couldn\'t find patchset %i in issue %i' %
+ (parsed_issue_arg.patchset, self.GetIssue()))
+
+ fetch_info = revision_info['fetch']['http']
+ RunGit(['fetch', fetch_info['url'], fetch_info['ref']])
+ RunGit(['cherry-pick', 'FETCH_HEAD'])
+ self.SetIssue(self.GetIssue())
+ self.SetPatchset(patchset)
+ print('Committed patch for issue %i pathset %i locally' %
+ (self.GetIssue(), self.GetPatchset()))
+ return 0
+
+ @staticmethod
+ def ParseIssueURL(parsed_url):
+ if not parsed_url.scheme or not parsed_url.scheme.startswith('http'):
+ return None
+ # Gerrit's new UI is https://domain/c/<issue_number>[/[patchset]]
+ # But current GWT UI is https://domain/#/c/<issue_number>[/[patchset]]
+ # Short urls like https://domain/<issue_number> can be used, but don't allow
+ # specifying the patchset (you'd 404), but we allow that here.
+ if parsed_url.path == '/':
+ part = parsed_url.fragment
+ else:
+ part = parsed_url.path
+ match = re.match('(/c)?/(\d+)(/(\d+)?/?)?$', part)
+ if match:
+ return _ParsedIssueNumberArgument(
+ issue=int(match.group(2)),
+ patchset=int(match.group(4)) if match.group(4) else None,
+ hostname=parsed_url.netloc)
+ return None
+
_CODEREVIEW_IMPLEMENTATIONS = {
'rietveld': _RietveldChangelistImpl,
@@ -3593,15 +3809,6 @@
return SendUpstream(parser, args, 'land')
-def ParseIssueNum(arg):
- """Parses the issue number from args if present otherwise returns None."""
- if re.match(r'\d+', arg):
- return arg
- if arg.startswith('http'):
- return re.sub(r'.*/(\d+)/?', r'\1', arg)
- return None
-
-
@subcommand.usage('<patch url or issue id or issue url>')
def CMDpatch(parser, args):
"""Patches in a code review."""
@@ -3611,64 +3818,69 @@
help='with -b, clobber any existing branch')
parser.add_option('-d', '--directory', action='store', metavar='DIR',
help='Change to the directory DIR immediately, '
- 'before doing anything else.')
+ 'before doing anything else. Rietveld only.')
parser.add_option('--reject', action='store_true',
help='failed patches spew .rej files rather than '
- 'attempting a 3-way merge')
+ 'attempting a 3-way merge. Rietveld only.')
parser.add_option('-n', '--no-commit', action='store_true', dest='nocommit',
- help="don't commit after patch applies")
+ help='don\'t commit after patch applies. Rietveld only.')
- group = optparse.OptionGroup(parser,
- """Options for continuing work on the current issue uploaded
-from a different clone (e.g. different machine). Must be used independently from
-the other options. No issue number should be specified, and the branch must have
-an issue number associated with it""")
- group.add_option('--reapply', action='store_true',
- dest='reapply',
- help="""Reset the branch and reapply the issue.
-CAUTION: This will undo any local changes in this branch""")
+
+ group = optparse.OptionGroup(
+ parser,
+ 'Options for continuing work on the current issue uploaded from a '
+ 'different clone (e.g. different machine). Must be used independently '
+ 'from the other options. No issue number should be specified, and the '
+ 'branch must have an issue number associated with it')
+ group.add_option('--reapply', action='store_true', dest='reapply',
+ help='Reset the branch and reapply the issue.\n'
+ 'CAUTION: This will undo any local changes in this '
+ 'branch')
group.add_option('--pull', action='store_true', dest='pull',
- help="Performs a pull before reapplying.")
+ help='Performs a pull before reapplying.')
parser.add_option_group(group)
auth.add_auth_options(parser)
(options, args) = parser.parse_args(args)
auth_config = auth.extract_auth_config_from_options(options)
+ cl = Changelist(auth_config=auth_config)
+
issue_arg = None
if options.reapply :
if len(args) > 0:
- parser.error("--reapply implies no additional arguments.")
+ parser.error('--reapply implies no additional arguments.')
- cl = Changelist()
issue_arg = cl.GetIssue()
upstream = cl.GetUpstreamBranch()
if upstream == None:
- parser.error("No upstream branch specified. Cannot reset branch")
+ parser.error('No upstream branch specified. Cannot reset branch')
RunGit(['reset', '--hard', upstream])
if options.pull:
RunGit(['pull'])
else:
if len(args) != 1:
- parser.error("Must specify issue number")
+ parser.error('Must specify issue number or url')
+ issue_arg = args[0]
- issue_arg = ParseIssueNum(args[0])
-
- # The patch URL works because ParseIssueNum won't do any substitution
- # as the re.sub pattern fails to match and just returns it.
- if issue_arg == None:
+ if not issue_arg:
parser.print_help()
return 1
+ if cl.IsGerrit():
+ if options.reject:
+ parser.error('--reject is not supported with Gerrit codereview.')
+ if options.nocommit:
+ parser.error('--nocommit is not supported with Gerrit codereview.')
+ if options.directory:
+ parser.error('--directory is not supported with Gerrit codereview.')
+
# We don't want uncommitted changes mixed up with the patch.
if git_common.is_dirty_git_tree('patch'):
return 1
- # TODO(maruel): Use apply_issue.py
- # TODO(ukai): use gerrit-cherry-pick for gerrit repository?
-
if options.newbranch:
if options.reapply:
parser.error("--reapply excludes any option other than --pull")
@@ -3678,84 +3890,8 @@
RunGit(['checkout', '-b', options.newbranch,
Changelist().GetUpstreamBranch()])
- return PatchIssue(issue_arg, options.reject, options.nocommit,
- options.directory, auth_config)
-
-
-def PatchIssue(issue_arg, reject, nocommit, directory, auth_config):
- # PatchIssue should never be called with a dirty tree. It is up to the
- # caller to check this, but just in case we assert here since the
- # consequences of the caller not checking this could be dire.
- assert(not git_common.is_dirty_git_tree('apply'))
-
- # TODO(tandrii): implement for Gerrit.
- if type(issue_arg) is int or issue_arg.isdigit():
- # Input is an issue id. Figure out the URL.
- issue = int(issue_arg)
- cl = Changelist(issue=issue, codereview='rietveld', auth_config=auth_config)
- patchset = cl.GetMostRecentPatchset()
- patch_data = cl._codereview_impl.GetPatchSetDiff(issue, patchset)
- else:
- # Assume it's a URL to the patch. Default to https.
- issue_url = gclient_utils.UpgradeToHttps(issue_arg)
- match = re.match(r'(.*?)/download/issue(\d+)_(\d+).diff', issue_url)
- if not match:
- DieWithError('Must pass an issue ID or full URL for '
- '\'Download raw patch set\'')
- issue = int(match.group(2))
- cl = Changelist(issue=issue, codereview='rietveld',
- rietveld_server=match.group(1), auth_config=auth_config)
- patchset = int(match.group(3))
- patch_data = urllib2.urlopen(issue_arg).read()
-
- # Switch up to the top-level directory, if necessary, in preparation for
- # applying the patch.
- top = settings.GetRelativeRoot()
- if top:
- os.chdir(top)
-
- # Git patches have a/ at the beginning of source paths. We strip that out
- # with a sed script rather than the -p flag to patch so we can feed either
- # Git or svn-style patches into the same apply command.
- # re.sub() should be used but flags=re.MULTILINE is only in python 2.7.
- try:
- patch_data = subprocess2.check_output(
- ['sed', '-e', 's|^--- a/|--- |; s|^+++ b/|+++ |'], stdin=patch_data)
- except subprocess2.CalledProcessError:
- DieWithError('Git patch mungling failed.')
- logging.info(patch_data)
-
- # We use "git apply" to apply the patch instead of "patch" so that we can
- # pick up file adds.
- # The --index flag means: also insert into the index (so we catch adds).
- cmd = ['git', 'apply', '--index', '-p0']
- if directory:
- cmd.extend(('--directory', directory))
- if reject:
- cmd.append('--reject')
- elif IsGitVersionAtLeast('1.7.12'):
- cmd.append('--3way')
- try:
- subprocess2.check_call(cmd, env=GetNoGitPagerEnv(),
- stdin=patch_data, stdout=subprocess2.VOID)
- except subprocess2.CalledProcessError:
- print 'Failed to apply the patch'
- return 1
-
- # If we had an issue, commit the current state and register the issue.
- if not nocommit:
- RunGit(['commit', '-m', (cl.GetDescription() + '\n\n' +
- 'patch from issue %(i)s at patchset '
- '%(p)s (http://crrev.com/%(i)s#ps%(p)s)'
- % {'i': issue, 'p': patchset})])
- cl = Changelist(codereview='rietveld', auth_config=auth_config,
- rietveld_server=cl.GetCodereviewServer())
- cl.SetIssue(issue)
- cl.SetPatchset(patchset)
- print "Committed patch locally."
- else:
- print "Patch applied to index."
- return 0
+ return cl.CMDPatchIssue(issue_arg, options.reject, options.nocommit,
+ options.directory)
def CMDrebase(parser, args):
@@ -4157,7 +4293,7 @@
parser.error('Unrecognized args: %s' % ' '.join(args))
# Uncommitted (staged and unstaged) changes will be destroyed by
- # "git reset --hard" if there are merging conflicts in PatchIssue().
+ # "git reset --hard" if there are merging conflicts in CMDPatchIssue().
# Staged changes would be committed along with the patch from last
# upload, hence counted toward the "last upload" side in the final
# diff output, and this is not what we want.
@@ -4175,8 +4311,7 @@
# Create a new branch based on the merge-base
RunGit(['checkout', '-q', '-b', TMP_BRANCH, base_branch])
try:
- # Patch in the latest changes from rietveld.
- rtn = PatchIssue(issue, False, False, None, auth_config)
+ rtn = cl.CMDPatchIssue(issue, reject=False, nocommit=False, directory=None)
if rtn != 0:
RunGit(['reset', '--hard'])
return rtn
@@ -4384,16 +4519,18 @@
@subcommand.usage('<codereview url or issue id>')
def CMDcheckout(parser, args):
"""Checks out a branch associated with a given Rietveld issue."""
+ # TODO(tandrii): consider adding this for Gerrit?
_, args = parser.parse_args(args)
if len(args) != 1:
parser.print_help()
return 1
- target_issue = ParseIssueNum(args[0])
- if target_issue == None:
+ issue_arg = ParseIssueNumberArgument(args[0])
+ if issue_arg.valid:
parser.print_help()
return 1
+ target_issue = issue_arg.issue
key_and_issues = [x.split() for x in RunGit(
['config', '--local', '--get-regexp', r'branch\..*\.rietveldissue'])