Revision: 5167
Author: [email protected]
Date: Thu Nov 29 12:08:20 2012
Log: Update tools/upload.py from Rietveld's copy.
https://codereview.appspot.com/6842118
codereview.appspot.com now 301 redirects to HTTPS, which is probably
a good thing, but the version of upload.py we're using doesn't like it.
* Update tools/upload.py to the latest version from the Rietveld
repository, since we haven't done so in ages:
<http://code.google.com/p/rietveld/source/browse/upload.py?spec=svnf66cc9030489ea92a2e699a0f1a68a854f1d064c&r=f66cc9030489ea92a2e699a0f1a68a854f1d064c>
* Explicitly specify https: in appspot.py because even the new
version of upload.py appears, from a quick review, to be defaulting to
http:.
* upload.py has also renamed the fields of an issue; change appspot.py
to match. 'Message' is now 'Title' and 'Description' is now 'Message'.
Includes backwards compatibility for old .appspot-change files.
If you are reading this and it is not mangled, then uploading a review
using these changes to the tools worked.
[email protected]
http://code.google.com/p/google-caja/source/detail?r=5167
Modified:
/trunk/tools/appspot.py
/trunk/tools/upload.py
=======================================
--- /trunk/tools/appspot.py Tue Aug 14 06:16:34 2012
+++ /trunk/tools/appspot.py Thu Nov 29 12:08:20 2012
@@ -13,8 +13,8 @@
If you're using git, the filename will be '.appspot-change.$GITBRANCH'.
Flags:
- -m --message : overrides the changelist message, a one line summary.
- -d --description : overrides the changelist's detailed description.
+ -t --title : overrides the changelist title, a one line summary.
+ -m --message : overrides the changelist's detailed description.
-r --reviewer : overrides the email of the assigned reviewer
-c --cc : overrides the CC list.
-p --private : mark the mail as private.
@@ -34,25 +34,28 @@
import tempfile
import upload
+# explicitly specify https, because upload.py doesn't default to it
+server = 'https://codereview.appspot.com'
+
class ChangeList(object):
- def __init__(self, issue=None, description=None, reviewer=None, cc=None,
- message=None, private=None):
+ def __init__(self, issue=None, title=None, message=None, cc=None,
+ reviewer=None, private=None):
"""
A value of None for any parameter means its unspecified and so will not
be set on a merge operation.
"""
assert issue is None or type(issue) in (str, unicode)
- assert description is None or type(description) in (str, unicode)
+ assert title is None or type(title) in (str, unicode)
+ assert message is None or type(message) in (str, unicode)
assert cc is None or type(cc) in (str, unicode)
assert reviewer is None or type(reviewer) in (str, unicode)
- assert message is None or type(message) in (str, unicode)
assert private is None or type(private) is bool
self.issue = issue
- self.description = description
+ self.title = title
+ self.message = message
self.cc = cc
self.reviewer = reviewer
- self.message = message
self.private = private
# TODO(mikesamuel): add a code.google.com bug number to update when
code
# is submitted
@@ -60,12 +63,12 @@
def get_app_spot_uri(self):
"""The URL of a change list."""
if self.issue is not None:
- return 'http://codereview.appspot.com/%d' % int(self.issue)
+ return '%s/%d' % (server, int(self.issue))
return None
def is_unspecified(self):
- return (self.issue is None and self.description is None and self.cc is
None
- and self.reviewer is None and self.message is None
+ return (self.issue is None and self.title is None and self.message is
None
+ and self.cc is None and self.reviewer is None
and self.private is None)
def get_upload_args(self, send_mail=False):
@@ -74,10 +77,13 @@
modify a code review.
"""
args = []
+ args.extend(['--server', server])
if self.issue:
args.extend(['--issue', str(self.issue)])
- if self.description:
- args.extend(['--description', str(self.description)])
+ if self.title:
+ args.extend(['--title', str(self.title)])
+ if self.message:
+ args.extend(['--message', str(self.message)])
if self.private:
# Private issues should not go to the public list.
cc_list = difference_of_address_lists(
@@ -98,8 +104,6 @@
args.extend(['--cc', cc_list])
if self.reviewer:
args.extend(['--reviewer', str(self.reviewer)])
- if self.message:
- args.extend(['--message', str(self.message)])
if self.private:
args.append('--private');
if send_mail:
@@ -131,10 +135,10 @@
def merge_into(self, target):
if self.issue is not None: target.issue = self.issue
- if self.description is not None: target.description = self.description
+ if self.title is not None: target.title = self.title
+ if self.message is not None: target.message = self.message
if self.cc is not None: target.cc = self.cc
if self.reviewer is not None: target.reviewer = self.reviewer
- if self.message is not None: target.message = self.message
if self.private is not None: target.private = self.private
@@ -190,9 +194,9 @@
%(url)s
-### Message:
+### Title:
### One-line summary of the change.
-%(message)s
+%(title)s
### Private:
@@ -201,9 +205,9 @@
%(private)s
-### Description:
+### Message:
### Detailed description of the change.
-%(description)s
+%(message)s
### Reviewer:
@@ -223,10 +227,10 @@
return ('''
-%(message)s
+%(title)s
%(url)s
-%(description)s
+%(message)s
R=%(reviewer)s
@@ -242,8 +246,8 @@
return {
'issue': changelist.issue or '<unassigned>',
'url': changelist.get_app_spot_uri() or '<unassigned>',
+ 'title': changelist.title or '',
'message': changelist.message or '',
- 'description': changelist.description or '',
'reviewer': changelist.reviewer or '',
'cc': changelist.cc or '',
'private': private_str,
@@ -279,10 +283,17 @@
issue = fields.get('Issue')
if issue is not None and not re.search('^\d+$', issue):
issue = None
- description = fields.get('Description')
+
+ # Compatibility with older field names. TODO(kpreid): Remove this after
no one
+ # likely has older files.
+ if 'Description' in fields and not 'Title' in fields:
+ title = fields.get('Message')
+ message = fields.get('Description')
+ else:
+ title = fields.get('Title')
+ message = fields.get('Message')
cc = fields.get('CC')
reviewer = fields.get('Reviewer')
- message = fields.get('Message')
private = fields.get('Private')
if private is not None:
private = private.strip().lower()
@@ -290,8 +301,8 @@
private = None
else:
private = private != 'false'
- return ChangeList(issue=issue, description=description, cc=cc,
- reviewer=reviewer, message=message, private=private)
+ return ChangeList(issue=issue, title=title, message=message, cc=cc,
+ reviewer=reviewer, private=private)
def do_edit(given_cl, current_cl, cl_file_path):
@@ -339,7 +350,7 @@
if current_cl.reviewer is not None:
send_mail = True
# If the user has not created a CL description, show an editor.
- if not current_cl.message or not current_cl.reviewer:
+ if not current_cl.title or not current_cl.reviewer:
do_edit(ChangeList(), current_cl, cl_file_path)
# If the CL does not have an issue number but user specified a reviewer
# send mail since it's the first upload.
@@ -382,8 +393,8 @@
# Map short flag names to long flag names
abbrevs = {
'-i': '--issue',
+ '-t': '--title',
'-m': '--message',
- '-d': '--description',
'-r': '--reviewer',
'-c': '--cc',
'-p': '--private',
@@ -392,8 +403,8 @@
flag_spec = {
'--send_mail': bool,
'--issue': int,
+ '--title': str,
'--message': str,
- '--description': str,
'--reviewer': str,
'--cc': str,
'--private': bool,
@@ -452,8 +463,8 @@
if len(argv) != 0: show_help_and_exit()
given_cl = ChangeList(
issue=params.get('--issue'),
+ title=params.get('--title'),
message=params.get('--message'),
- description=params.get('--description'),
reviewer=params.get('--reviewer'),
cc=params.get('--cc'),
private=params.get('--private'))
=======================================
--- /trunk/tools/upload.py Fri Aug 13 17:09:11 2010
+++ /trunk/tools/upload.py Thu Nov 29 12:08:20 2012
@@ -1,4 +1,5 @@
#!/usr/bin/env python
+# coding: utf-8
#
# Copyright 2007 Google Inc.
#
@@ -16,7 +17,7 @@
"""Tool for uploading diffs from a version control system to the
codereview app.
-Usage summary: upload.py [options] [-- diff_options]
+Usage summary: upload.py [options] [-- diff_options] [path...]
Diff options are passed to the diff command of the underlying system.
@@ -24,6 +25,8 @@
Git
Mercurial
Subversion
+ Perforce
+ CVS
It is important for Git/Mercurial users to specify a tree/node/branch to
diff
against by using the '--rev' option.
@@ -31,9 +34,13 @@
# This code is derived from appcfg.py in the App Engine SDK (open source),
# and from ASPN recipe #146306.
+import ConfigParser
import cookielib
+import errno
+import fnmatch
import getpass
import logging
+import marshal
import mimetypes
import optparse
import os
@@ -51,11 +58,15 @@
except ImportError:
from md5 import md5
-if __name__ == "__main__":
- try:
- import readline
- except ImportError:
- pass
+try:
+ import readline
+except ImportError:
+ pass
+
+try:
+ import keyring
+except ImportError:
+ keyring = None
# The logging verbosity:
# 0: Errors only.
@@ -64,6 +75,15 @@
# 3: Debug logs.
verbosity = 1
+# The account type used for authentication.
+# This line could be changed by the review server (see handler for
+# upload.py).
+AUTH_ACCOUNT_TYPE = "GOOGLE"
+
+# URL of the default review server. As for AUTH_ACCOUNT_TYPE, this line
could be
+# changed by the review server (see handler for upload.py).
+DEFAULT_REVIEW_SERVER = "codereview.appspot.com"
+
# Max size of patch or base file.
MAX_UPLOAD_SIZE = 900 * 1024
@@ -71,8 +91,23 @@
VCS_GIT = "Git"
VCS_MERCURIAL = "Mercurial"
VCS_SUBVERSION = "Subversion"
+VCS_PERFORCE = "Perforce"
+VCS_CVS = "CVS"
VCS_UNKNOWN = "Unknown"
+VCS_ABBREVIATIONS = {
+ VCS_MERCURIAL.lower(): VCS_MERCURIAL,
+ "hg": VCS_MERCURIAL,
+ VCS_SUBVERSION.lower(): VCS_SUBVERSION,
+ "svn": VCS_SUBVERSION,
+ VCS_PERFORCE.lower(): VCS_PERFORCE,
+ "p4": VCS_PERFORCE,
+ VCS_GIT.lower(): VCS_GIT,
+ VCS_CVS.lower(): VCS_CVS,
+}
+
+# The result of parsing Subversion's [auto-props] setting.
+svn_auto_props_map = None
def GetEmail(prompt):
"""Prompts the user for their email address and returns it.
@@ -130,15 +165,23 @@
def __init__(self, url, code, msg, headers, args):
urllib2.HTTPError.__init__(self, url, code, msg, headers, None)
self.args = args
- self.reason = args["Error"]
+ self._reason = args["Error"]
+ self.info = args.get("Info", None)
+ @property
+ def reason(self):
+ # reason is a property on python 2.7 but a member variable on <=2.6.
+ # self.args is modified so it cannot be used as-is so save the value in
+ # self._reason.
+ return self._reason
+
class AbstractRpcServer(object):
"""Provides a common interface for a simple RPC server."""
def __init__(self, host, auth_function, host_override=None,
extra_headers={},
- save_cookies=False):
- """Creates a new HttpRpcServer.
+ save_cookies=False, account_type=AUTH_ACCOUNT_TYPE):
+ """Creates a new AbstractRpcServer.
Args:
host: The host to send requests to.
@@ -150,13 +193,19 @@
save_cookies: If True, save the authentication cookies to local disk.
If False, use an in-memory cookiejar instead. Subclasses must
implement this functionality. Defaults to False.
+ account_type: Account type used for authentication. Defaults to
+ AUTH_ACCOUNT_TYPE.
"""
self.host = host
+ if (not self.host.startswith("http://";) and
+ not self.host.startswith("https://";)):
+ self.host = "http://"; + self.host
self.host_override = host_override
self.auth_function = auth_function
self.authenticated = False
self.extra_headers = extra_headers
self.save_cookies = save_cookies
+ self.account_type = account_type
self.opener = self._GetOpener()
if self.host_override:
logging.info("Server: %s; Host: %s", self.host, self.host_override)
@@ -174,7 +223,7 @@
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():
@@ -195,7 +244,7 @@
Returns:
The authentication token returned by ClientLogin.
"""
- account_type = "GOOGLE"
+ account_type = self.account_type
if self.host.endswith(".google.com"):
# Needed for use inside Google.
account_type = "HOSTED"
@@ -236,7 +285,7 @@
# This is a dummy value to allow us to identify when we're successful.
continue_location = "http://localhost/";
args = {"continue": continue_location, "auth": auth_token}
- req = self._CreateRequest("http://%s/_ah/login?%s"; %
+ req = self._CreateRequest("%s/_ah/login?%s" %
(self.host, urllib.urlencode(args)))
try:
response = self.opener.open(req)
@@ -260,49 +309,57 @@
us to the URL we provided.
If we attempt to access the upload API without first obtaining an
- authentication cookie, it returns a 401 response and directs us to
- authenticate ourselves with ClientLogin.
+ authentication cookie, it returns a 401 response (or a 302) and
+ directs us to authenticate ourselves with ClientLogin.
"""
for i in range(3):
credentials = self.auth_function()
try:
auth_token = self._GetAuthToken(credentials[0], credentials[1])
except ClientLoginError, e:
+ print >>sys.stderr, ''
if e.reason == "BadAuthentication":
- print >>sys.stderr, "Invalid username or password."
- continue
- if e.reason == "CaptchaRequired":
+ if e.info == "InvalidSecondFactor":
+ print >>sys.stderr, (
+ "Use an application-specific password instead "
+ "of your regular account password.\n"
+ "See http://www.google.com/";
+ "support/accounts/bin/answer.py?answer=185833")
+ else:
+ print >>sys.stderr, "Invalid username or password."
+ elif e.reason == "CaptchaRequired":
print >>sys.stderr, (
"Please go to\n"
"https://www.google.com/accounts/DisplayUnlockCaptcha\n";
- "and verify you are a human. Then try again.")
- break
- if e.reason == "NotVerified":
+ "and verify you are a human. Then try again.\n"
+ "If you are using a Google Apps account the URL is:\n"
+ "https://www.google.com/a/yourdomain.com/UnlockCaptcha";)
+ elif e.reason == "NotVerified":
print >>sys.stderr, "Account not verified."
- break
- if e.reason == "TermsNotAgreed":
+ elif e.reason == "TermsNotAgreed":
print >>sys.stderr, "User has not agreed to TOS."
- break
- if e.reason == "AccountDeleted":
+ elif e.reason == "AccountDeleted":
print >>sys.stderr, "The user account has been deleted."
- break
- if e.reason == "AccountDisabled":
+ elif e.reason == "AccountDisabled":
print >>sys.stderr, "The user account has been disabled."
break
- if e.reason == "ServiceDisabled":
+ elif e.reason == "ServiceDisabled":
print >>sys.stderr, ("The user's access to the service has been "
"disabled.")
- break
- if e.reason == "ServiceUnavailable":
+ elif e.reason == "ServiceUnavailable":
print >>sys.stderr, "The service is not available; try again
later."
- break
- raise
+ else:
+ # Unknown error.
+ raise
+ print >>sys.stderr, ''
+ continue
self._GetAuthCookie(auth_token)
return
def Send(self, request_path, payload=None,
content_type="application/octet-stream",
timeout=None,
+ extra_headers=None,
**kwargs):
"""Sends an RPC and returns the response.
@@ -312,6 +369,9 @@
content_type: The Content-Type header to use.
timeout: timeout in seconds; default None i.e. no timeout.
(Note: for large requests on OS X, the timeout doesn't work right.)
+ extra_headers: Dict containing additional HTTP headers that should be
+ included in the request (string header names mapped to their
values),
+ or None to not include any additional headers.
kwargs: Any keyword arguments are converted into query string
parameters.
Returns:
@@ -329,11 +389,14 @@
while True:
tries += 1
args = dict(kwargs)
- url = "http://%s%s"; % (self.host, request_path)
+ url = "%s%s" % (self.host, request_path)
if args:
url += "?" + urllib.urlencode(args)
req = self._CreateRequest(url=url, data=payload)
req.add_header("Content-Type", content_type)
+ if extra_headers:
+ for header, value in extra_headers.items():
+ req.add_header(header, value)
try:
f = self.opener.open(req)
response = f.read()
@@ -342,11 +405,15 @@
except urllib2.HTTPError, e:
if tries > 3:
raise
- elif e.code == 401:
+ 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:
@@ -401,7 +468,39 @@
return opener
-parser = optparse.OptionParser(usage="%prog [options] [-- diff_options]")
+class CondensedHelpFormatter(optparse.IndentedHelpFormatter):
+ """Frees more horizontal space by removing indentation from group
+ options and collapsing arguments between short and long, e.g.
+ '-o ARG, --opt=ARG' to -o --opt ARG"""
+
+ def format_heading(self, heading):
+ return "%s:\n" % heading
+
+ def format_option(self, option):
+ self.dedent()
+ res = optparse.HelpFormatter.format_option(self, option)
+ self.indent()
+ return res
+
+ def format_option_strings(self, option):
+ self.set_long_opt_delimiter(" ")
+ optstr = optparse.HelpFormatter.format_option_strings(self, option)
+ optlist = optstr.split(", ")
+ if len(optlist) > 1:
+ if option.takes_value():
+ # strip METAVAR from all but the last option
+ optlist = [x.split()[0] for x in optlist[:-1]] + optlist[-1:]
+ optstr = " ".join(optlist)
+ return optstr
+
+
+parser = optparse.OptionParser(
+ usage="%prog [options] [-- diff_options] [path...]",
+ add_help_option=False,
+ formatter=CondensedHelpFormatter()
+)
+parser.add_option("-h", "--help", action="store_true",
+ help="Show this help message and exit.")
parser.add_option("-y", "--assume_yes", action="store_true",
dest="assume_yes", default=False,
help="Assume that the answer to yes/no questions
is 'yes'.")
@@ -411,13 +510,15 @@
dest="verbose", help="Print errors only.")
group.add_option("-v", "--verbose", action="store_const", const=2,
dest="verbose", default=1,
- help="Print info level logs (default).")
+ help="Print info level logs.")
group.add_option("--noisy", action="store_const", const=3,
dest="verbose", help="Print all logs.")
+group.add_option("--print_diffs", dest="print_diffs", action="store_true",
+ help="Print full diffs.")
# Review server
group = parser.add_option_group("Review server options")
group.add_option("-s", "--server", action="store", dest="server",
- default="codereview.appspot.com",
+ default=DEFAULT_REVIEW_SERVER,
metavar="SERVER",
help=("The server to upload to. The format is
host[:port]. "
"Defaults to '%default'."))
@@ -430,16 +531,21 @@
group.add_option("--no_cookies", action="store_false",
dest="save_cookies", default=True,
help="Do not save authentication cookies to local disk.")
+group.add_option("--account_type", action="store", dest="account_type",
+ metavar="TYPE", default=AUTH_ACCOUNT_TYPE,
+ choices=["GOOGLE", "HOSTED"],
+ help=("Override the default account type "
+ "(defaults to '%default', "
+ "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).")
@@ -451,63 +557,142 @@
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.")
+group.add_option("--base_url", action="store", dest="base_url",
default=None,
+ help="Base URL path for files (listed as \"Base URL\"
when "
+ "viewing issue). If omitted, will be guessed
automatically "
+ "for SVN repos and left blank for others.")
group.add_option("--download_base", action="store_true",
dest="download_base", default=False,
help="Base files will be downloaded by the server "
"(side-by-side diffs may not work on files with CRs).")
group.add_option("--rev", action="store", dest="revision",
metavar="REV", default=None,
- help="Branch/tree/revision to diff against (used by
DVCS).")
+ help="Base revision/branch/tree to diff against. Use "
+ "rev1:rev2 range to review already committed
changeset.")
group.add_option("--send_mail", action="store_true",
dest="send_mail", default=False,
help="Send notification email to reviewers.")
+group.add_option("-p", "--send_patch", action="store_true",
+ dest="send_patch", default=False,
+ help="Same as --send_mail, but include diff as an "
+ "attachment, and prepend email subject
with 'PATCH:'.")
+group.add_option("--vcs", action="store", dest="vcs",
+ metavar="VCS", default=None,
+ help=("Version control system (optional, usually
upload.py "
+ "already guesses the right VCS)."))
+group.add_option("--emulate_svn_auto_props", action="store_true",
+ dest="emulate_svn_auto_props", default=False,
+ help=("Emulate Subversion's auto properties feature."))
+# Git-specific
+group = parser.add_option_group("Git-specific options")
+group.add_option("--git_similarity", action="store", dest="git_similarity",
+ metavar="SIM", type="int", default=50,
+ help=("Set the minimum similarity index for detecting
renames "
+ "and copies. See `git diff -C`. (default 50)."))
+group.add_option("--git_no_find_copies", action="store_false",
default=True,
+ dest="git_find_copies",
+ help=("Prevents git from looking for copies (default
off)."))
+# Perforce-specific
+group = parser.add_option_group("Perforce-specific options "
+ "(overrides P4 environment variables)")
+group.add_option("--p4_port", action="store", dest="p4_port",
+ metavar="P4_PORT", default=None,
+ help=("Perforce server and port (optional)"))
+group.add_option("--p4_changelist", action="store", dest="p4_changelist",
+ metavar="P4_CHANGELIST", default=None,
+ help=("Perforce changelist id"))
+group.add_option("--p4_client", action="store", dest="p4_client",
+ metavar="P4_CLIENT", default=None,
+ help=("Perforce client/workspace"))
+group.add_option("--p4_user", action="store", dest="p4_user",
+ metavar="P4_USER", default=None,
+ help=("Perforce user"))
-def GetRpcServer(options):
- """Returns an instance of an AbstractRpcServer.
+class KeyringCreds(object):
+ def __init__(self, server, host, email):
+ self.server = server
+ self.host = host
+ self.email = email
+ self.accounts_seen = set()
- Returns:
- A new AbstractRpcServer, on which RPC calls can be made.
- """
+ def GetUserCredentials(self):
+ """Prompts the user for a username and password.
- rpc_server_class = HttpRpcServer
-
- def GetUserCredentials():
- """Prompts the user for a username and password."""
- email = options.email
+ Only use keyring on the initial call. If the keyring contains the wrong
+ password, we want to give the user a chance to enter another one.
+ """
+ # Create a local alias to the email variable to avoid Python's crazy
+ # scoping rules.
+ global keyring
+ email = self.email
if email is None:
- email = GetEmail("Email (login for uploading to %s)" %
options.server)
- password = getpass.getpass("Password for %s: " % email)
+ email = GetEmail("Email (login for uploading to %s)" % self.server)
+ password = None
+ if keyring and not email in self.accounts_seen:
+ try:
+ password = keyring.get_password(self.host, email)
+ except:
+ # Sadly, we have to trap all errors here as
+ # gnomekeyring.IOError inherits from object. :/
+ print "Failed to get password from keyring"
+ keyring = None
+ if password is not None:
+ print "Using password from system keyring."
+ self.accounts_seen.add(email)
+ else:
+ password = getpass.getpass("Password for %s: " % email)
+ if keyring:
+ answer = raw_input("Store password in system
keyring?(y/N) ").strip()
+ if answer == "y":
+ keyring.set_password(self.host, email, password)
+ self.accounts_seen.add(email)
return (email, password)
+
+def GetRpcServer(server, email=None, host_override=None, save_cookies=True,
+ account_type=AUTH_ACCOUNT_TYPE):
+ """Returns an instance of an AbstractRpcServer.
+
+ Args:
+ server: String containing the review server URL.
+ email: String containing user's email address.
+ host_override: If not None, string containing an alternate hostname to
use
+ in the host header.
+ save_cookies: Whether authentication cookies should be saved to disk.
+ account_type: Account type for authentication, either 'GOOGLE'
+ or 'HOSTED'. Defaults to AUTH_ACCOUNT_TYPE.
+
+ Returns:
+ A new HttpRpcServer, on which RPC calls can be made.
+ """
+
# If this is the dev_appserver, use fake authentication.
- host = (options.host or options.server).lower()
- if host == "localhost" or host.startswith("localhost:"):
- email = options.email
+ host = (host_override or server).lower()
+ if re.match(r'(http://)?localhost([:/]|$)', host):
if email is None:
email = "[email protected]"
logging.info("Using debug user %s. Override with --email" % email)
- server = rpc_server_class(
- options.server,
+ server = HttpRpcServer(
+ server,
lambda: (email, "password"),
- host_override=options.host,
+ host_override=host_override,
extra_headers={"Cookie":
'dev_appserver_login="%s:False"' % email},
- save_cookies=options.save_cookies)
+ save_cookies=save_cookies,
+ account_type=account_type)
# Don't try to talk to ClientLogin.
server.authenticated = True
return server
- return rpc_server_class(options.server, GetUserCredentials,
- host_override=options.host,
- save_cookies=options.save_cookies)
+ return HttpRpcServer(server,
+ KeyringCreds(server, host,
email).GetUserCredentials,
+ host_override=host_override,
+ save_cookies=save_cookies,
+ account_type=account_type)
def EncodeMultipartFormData(fields, files):
@@ -530,6 +715,8 @@
lines.append('--' + BOUNDARY)
lines.append('Content-Disposition: form-data; name="%s"' % key)
lines.append('')
+ if isinstance(value, unicode):
+ value = value.encode('utf-8')
lines.append(value)
for (key, filename, value) in files:
lines.append('--' + BOUNDARY)
@@ -537,6 +724,8 @@
(key, filename))
lines.append('Content-Type: %s' % GetContentType(filename))
lines.append('')
+ if isinstance(value, unicode):
+ value = value.encode('utf-8')
lines.append(value)
lines.append('--' + BOUNDARY + '--')
lines.append('')
@@ -553,9 +742,10 @@
# Use a shell for subcommands on Windows to get a PATH search.
use_shell = sys.platform.startswith("win")
-def RunShellWithReturnCode(command, print_output=False,
- universal_newlines=True):
- """Executes a command and returns the output from stdout and the return
code.
+def RunShellWithReturnCodeAndStderr(command, print_output=False,
+ universal_newlines=True,
+ env=os.environ):
+ """Executes a command and returns the output from stdout, stderr and the
return code.
Args:
command: Command to execute.
@@ -564,11 +754,14 @@
universal_newlines: Use universal_newlines flag (default: True).
Returns:
- Tuple (output, return code)
+ Tuple (stdout, stderr, return code)
"""
logging.info("Running %s", command)
+ env = env.copy()
+ env['LC_MESSAGES'] = 'C'
p = subprocess.Popen(command, stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
- shell=use_shell,
universal_newlines=universal_newlines)
+ shell=use_shell,
universal_newlines=universal_newlines,
+ env=env)
if print_output:
output_array = []
while True:
@@ -586,13 +779,20 @@
print >>sys.stderr, errout
p.stdout.close()
p.stderr.close()
- return output, p.returncode
+ return output, errout, p.returncode
+def RunShellWithReturnCode(command, print_output=False,
+ universal_newlines=True,
+ env=os.environ):
+ """Executes a command and returns the output from stdout and the return
code."""
+ out, err, retcode = RunShellWithReturnCodeAndStderr(command,
print_output,
+ universal_newlines, env)
+ return out, retcode
def RunShell(command, silent_ok=False, universal_newlines=True,
- print_output=False):
+ print_output=False, env=os.environ):
data, retcode = RunShellWithReturnCode(command, print_output,
- universal_newlines)
+ universal_newlines, env)
if retcode:
ErrorExit("Got error status from %s:\n%s" % (command, data))
if not silent_ok and not data:
@@ -611,6 +811,17 @@
"""
self.options = options
+ def GetGUID(self):
+ """Return string to distinguish the repository from others, for
example to
+ query all opened review issues for it"""
+ raise NotImplementedError(
+ "abstract method -- subclass %s must override" % self.__class__)
+
+ def PostProcessDiff(self, diff):
+ """Return the diff with any special post processing this VCS needs,
e.g.
+ to include an svn-style "Index:"."""
+ return diff
+
def GenerateDiff(self, args):
"""Return the current diff as a string.
@@ -732,12 +943,11 @@
return False
return mimetype.startswith("image/")
- def IsBinary(self, filename):
- """Returns true if the guessed mimetyped isnt't in text group."""
- mimetype = mimetypes.guess_type(filename)[0]
- if not mimetype:
- return False # e.g. README, "real" binaries usually have an
extension
- return not mimetype.startswith("text/")
+ def IsBinaryData(self, data):
+ """Returns true if data contains a null byte."""
+ # Derived from how Mercurial's heuristic, see
+ # http://selenic.com/hg/file/848a6658069e/mercurial/util.py#l229
+ return bool(data and "\0" in data)
class SubversionVCS(VersionControlSystem):
@@ -756,59 +966,58 @@
# Cache output from "svn list -r REVNO dirname".
# Keys: dirname, Values: 2-tuple (ouput for start rev and end rev).
self.svnls_cache = {}
- # SVN base URL is required to fetch files deleted in an older revision.
+ # Base URL is required to fetch files deleted in an older revision.
# 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")
+
def GuessBase(self, required):
"""Wrapper for _GuessBase."""
return self.svn_base
def _GuessBase(self, required):
- """Returns the SVN base URL.
+ """Returns base URL for current diff.
Args:
required: If true, exits if the url can't be guessed, otherwise None
is
returned.
"""
- info = RunShell(["svn", "info"])
- for line in info.splitlines():
- words = line.split()
- if len(words) == 2 and words[0] == "URL:":
- url = words[1]
+ url = self._GetInfo("URL")
+ if url:
scheme, netloc, path, params, query, fragment =
urlparse.urlparse(url)
- username, netloc = urllib.splituser(netloc)
- if username:
- logging.info("Removed username from base URL")
- if netloc.endswith("svn.python.org"):
- if netloc == "svn.python.org":
- if path.startswith("/projects/"):
- path = path[9:]
- elif netloc != "[email protected]":
- ErrorExit("Unrecognized Python URL: %s" % url)
- base = "http://svn.python.org/view/*checkout*%s/"; % path
- logging.info("Guessed Python base = %s", base)
- elif netloc.endswith("svn.collab.net"):
- if path.startswith("/repos/"):
- path = path[6:]
- base = "http://svn.collab.net/viewvc/*checkout*%s/"; % path
- logging.info("Guessed CollabNet base = %s", base)
+ guess = ""
+ # TODO(anatoli) - repository specific hacks should be handled by
server
+ if netloc == "svn.python.org" and scheme == "svn+ssh":
+ path = "projects" + path
+ scheme = "http"
+ guess = "Python "
elif netloc.endswith(".googlecode.com"):
- path = path + "/"
- base = urlparse.urlunparse(("http", netloc, path, params,
- query, fragment))
- logging.info("Guessed Google Code base = %s", base)
- else:
- path = path + "/"
- base = urlparse.urlunparse((scheme, netloc, path, params,
- query, fragment))
- logging.info("Guessed base = %s", base)
+ scheme = "http"
+ guess = "Google Code "
+ path = path + "/"
+ base = urlparse.urlunparse((scheme, netloc, path, params,
+ query, fragment))
+ logging.info("Guessed %sbase = %s", guess, base)
return base
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():
+ if line.startswith(key + ": "):
+ return line.split(":", 1)[1].strip()
+
+ def _EscapeFilename(self, filename):
+ """Escapes filename for SVN commands."""
+ if "@" in filename and not filename.endswith("@"):
+ filename = "%s@" % filename
+ return filename
+
def GenerateDiff(self, args):
cmd = ["svn", "diff"]
if self.options.revision:
@@ -876,7 +1085,8 @@
def GetStatus(self, filename):
"""Returns the status of a file."""
if not self.options.revision:
- status = RunShell(["svn", "status", "--ignore-externals", filename])
+ status = RunShell(["svn", "status", "--ignore-externals",
+ self._EscapeFilename(filename)])
if not status:
ErrorExit("svn status returned no output for %s" % filename)
status_lines = status.splitlines()
@@ -895,15 +1105,22 @@
else:
dirname, relfilename = os.path.split(filename)
if dirname not in self.svnls_cache:
- cmd = ["svn", "list", "-r", self.rev_start, dirname or "."]
- out, returncode = RunShellWithReturnCode(cmd)
+ cmd = ["svn", "list", "-r", self.rev_start,
+ self._EscapeFilename(dirname) or "."]
+ out, err, returncode = RunShellWithReturnCodeAndStderr(cmd)
if returncode:
- ErrorExit("Failed to get status for %s." % filename)
- old_files = out.splitlines()
+ # Directory might not yet exist at start revison
+ # svn: Unable to find repository location for 'abc' in revision
nnn
+ if re.match('^svn: Unable to find repository location for .+ in
revision \d+', err):
+ old_files = ()
+ else:
+ ErrorExit("Failed to get status for %s:\n%s" % (filename, err))
+ else:
+ old_files = out.splitlines()
args = ["svn", "list"]
if self.rev_end:
args += ["-r", self.rev_end]
- cmd = args + [dirname or "."]
+ cmd = args + [self._EscapeFilename(dirname) or "."]
out, returncode = RunShellWithReturnCode(cmd)
if returncode:
ErrorExit("Failed to run command %s" % cmd)
@@ -929,17 +1146,18 @@
if status[0] == "A" and status[3] != "+":
# We'll need to upload the new content if we're adding a binary file
# since diff's output won't contain it.
- mimetype = RunShell(["svn", "propget", "svn:mime-type", filename],
- silent_ok=True)
+ mimetype = RunShell(["svn", "propget", "svn:mime-type",
+ self._EscapeFilename(filename)], silent_ok=True)
base_content = ""
is_binary = bool(mimetype) and not mimetype.startswith("text/")
- if is_binary and self.IsImage(filename):
+ if is_binary:
new_content = self.ReadFile(filename)
elif (status[0] in ("M", "D", "R") or
(status[0] == "A" and status[3] == "+") or # Copied file.
(status[0] == " " and status[1] == "M")): # Property change.
args = []
if self.options.revision:
+ # filename must not be escaped. We already add an ampersand here.
url = "%s/%s@%s" % (self.svn_base, filename, self.rev_start)
else:
# Don't change filename, it's needed later.
@@ -951,23 +1169,27 @@
# File does not exist in the requested revision.
# Reset mimetype, it contains an error message.
mimetype = ""
+ else:
+ mimetype = mimetype.strip()
get_base = False
- is_binary = bool(mimetype) and not mimetype.startswith("text/")
+ # this test for binary is exactly the test prescribed by the
+ # official SVN docs at
+ # http://subversion.apache.org/faq.html#binary-files
+ is_binary = (bool(mimetype) and
+ not mimetype.startswith("text/") and
+ mimetype not in ("image/x-xbitmap", "image/x-xpixmap"))
if status[0] == " ":
# Empty base content just to force an upload.
base_content = ""
elif is_binary:
- if self.IsImage(filename):
- get_base = True
- if status[0] == "M":
- if not self.rev_end:
- new_content = self.ReadFile(filename)
- else:
- url = "%s/%s@%s" % (self.svn_base, filename, self.rev_end)
- new_content = RunShell(["svn", "cat", url],
- universal_newlines=True,
silent_ok=True)
- else:
- base_content = ""
+ get_base = True
+ if status[0] == "M":
+ if not self.rev_end:
+ new_content = self.ReadFile(filename)
+ else:
+ url = "%s/%s@%s" % (self.svn_base, filename, self.rev_end)
+ new_content = RunShell(["svn", "cat", url],
+ universal_newlines=True, silent_ok=True)
else:
get_base = True
@@ -984,9 +1206,18 @@
universal_newlines=universal_newlines,
silent_ok=True)
else:
- base_content = RunShell(["svn", "cat", filename],
- universal_newlines=universal_newlines,
- silent_ok=True)
+ base_content, ret_code = RunShellWithReturnCode(
+ ["svn", "cat", self._EscapeFilename(filename)],
+ universal_newlines=universal_newlines)
+ if ret_code and status[0] == "R":
+ # It's a replaced file without local history (see issue208).
+ # The base file needs to be fetched from the server.
+ url = "%s/%s" % (self.svn_base, filename)
+ base_content = RunShell(["svn", "cat", url],
+ universal_newlines=universal_newlines,
+ silent_ok=True)
+ elif ret_code:
+ ErrorExit("Got error status from 'svn cat %s'" % filename)
if not is_binary:
args = []
if self.rev_start:
@@ -1007,41 +1238,121 @@
class GitVCS(VersionControlSystem):
"""Implementation of the VersionControlSystem interface for Git."""
- NULL_HASH = "0"*40
-
def __init__(self, options):
super(GitVCS, self).__init__(options)
# Map of filename -> (hash before, hash after) of base file.
- self.base_hashes = {}
+ # Hashes for "no such file" are represented as None.
+ self.hashes = {}
+ # Map of new filename -> old filename for renames.
+ self.renames = {}
- def GenerateDiff(self, extra_args):
- # This is more complicated than svn's GenerateDiff because we must
convert
- # the diff output to include an svn-style "Index:" line as well as
record
- # the hashes of the base files, so we can upload them along with our
diff.
- if self.options.revision:
- extra_args = [self.options.revision] + extra_args
- gitdiff = RunShell(["git", "diff", "--full-index"] + extra_args)
+ 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
+ # subtree is merged. In that case, more analysis would need to
+ # be done to figure out which HEAD is the 'most representative'.
+ for r in revlist:
+ if ' ' not in r:
+ return r
+
+ def PostProcessDiff(self, gitdiff):
+ """Converts the diff output to include an svn-style "Index:" line as
well
+ as record the hashes of the files, so we can upload them along with our
+ diff."""
+ # Special used by git to indicate "no such content".
+ NULL_HASH = "0"*40
+
+ def IsFileNew(filename):
+ return filename in self.hashes and self.hashes[filename][0] is None
+
+ def AddSubversionPropertyChange(filename):
+ """Add svn's property change information into the patch if given
file is
+ new file.
+
+ We use Subversion's auto-props setting to retrieve its property.
+ See http://svnbook.red-bean.com/en/1.1/ch07.html#svn-ch-7-sect-1.3.2
for
+ Subversion's [auto-props] setting.
+ """
+ if self.options.emulate_svn_auto_props and IsFileNew(filename):
+ svnprops = GetSubversionPropertyChanges(filename)
+ if svnprops:
+ svndiff.append("\n" + svnprops + "\n")
+
svndiff = []
filecount = 0
filename = None
for line in gitdiff.splitlines():
- match = re.match(r"diff --git a/(.*) b/.*$", line)
+ match = re.match(r"diff --git a/(.*) b/(.*)$", line)
if match:
+ # Add auto property here for previously seen file.
+ if filename is not None:
+ AddSubversionPropertyChange(filename)
filecount += 1
- filename = match.group(1)
+ # Intentionally use the "after" filename so we can show renames.
+ filename = match.group(2)
svndiff.append("Index: %s\n" % filename)
+ if match.group(1) != match.group(2):
+ self.renames[match.group(2)] = match.group(1)
else:
# The "index" line in a git diff looks like this (long hashes
elided):
# index 82c0d44..b2cee3f 100755
# We want to save the left hash, as that identifies the base file.
match = re.match(r"index (\w+)\.\.(\w+)", line)
if match:
- self.base_hashes[filename] = (match.group(1), match.group(2))
+ before, after = (match.group(1), match.group(2))
+ if before == NULL_HASH:
+ before = None
+ if after == NULL_HASH:
+ after = None
+ self.hashes[filename] = (before, after)
svndiff.append(line + "\n")
if not filecount:
ErrorExit("No valid patches found in output from git diff")
+ # Add auto property for the last seen file.
+ assert filename is not None
+ AddSubversionPropertyChange(filename)
return "".join(svndiff)
+ def GenerateDiff(self, extra_args):
+ extra_args = extra_args[:]
+ if self.options.revision:
+ if ":" in self.options.revision:
+ extra_args = self.options.revision.split(":", 1) + extra_args
+ else:
+ extra_args = [self.options.revision] + extra_args
+
+ # --no-ext-diff is broken in some versions of Git, so try to work
around
+ # this by overriding the environment (but there is still a problem if
the
+ # git config key "diff.external" is used).
+ env = os.environ.copy()
+ if "GIT_EXTERNAL_DIFF" in env:
+ del env["GIT_EXTERNAL_DIFF"]
+ # -M/-C will not print the diff for the deleted file when a file is
renamed.
+ # This is confusing because the original file will not be shown on the
+ # review when a file is renamed. So, get a diff with ONLY deletes, then
+ # append a diff (with rename detection), without deletes.
+ cmd = [
+ "git", "diff", "--no-color", "--no-ext-diff", "--full-index",
+ "--ignore-submodules",
+ ]
+ diff = RunShell(
+ cmd + ["--no-renames", "--diff-filter=D"] + extra_args,
+ env=env, silent_ok=True)
+ if self.options.git_find_copies:
+ similarity_options = ["--find-copies-harder", "-l100000",
+ "-C%s" % self.options.git_similarity ]
+ else:
+ similarity_options = ["-M%s" % self.options.git_similarity ]
+ diff += RunShell(
***The diff for this file has been truncated for email.***
