codereview: new commands
* clpatch
* download
* submit, on behalf of clpatch
stir hgpatch to fix a few bugs
R=r
CC=go-dev
http://go/go-review/1016051
diff --git a/lib/codereview/codereview.py b/lib/codereview/codereview.py
index 0e71a69..6fc26dd 100644
--- a/lib/codereview/codereview.py
+++ b/lib/codereview/codereview.py
@@ -16,28 +16,34 @@
'''Mercurial interface to codereview.appspot.com.
-To configure it, set the following options in
+To configure, set the following options in
your repository's .hg/hgrc file.
- [extensions]
- codereview = path/to/codereview.py
+ [extensions]
+ codereview = path/to/codereview.py
- [codereview]
+ [codereview]
server = codereview.appspot.com
The server should be running Rietveld; see http://code.google.com/p/rietveld/.
+
+In addition to the new commands, this extension introduces
+the file pattern syntax @nnnnnn, where nnnnnn is a change list
+number, to mean the files included in that change list, which
+must be associated with the current client.
+
+For example, if change 123456 contains the files x.go and y.go,
+"hg diff @123456" is equivalent to"hg diff x.go y.go".
'''
# TODO(rsc):
# fix utf-8 upload bug
-# look for and clear submitted CLs during sync / add "adopt" command?
-# creating an issue prints the URL twice
-# better documentation
from mercurial import cmdutil, commands, hg, util, error, match
from mercurial.node import nullrev, hex, nullid, short
import os, re
import stat
+import subprocess
import threading
from HTMLParser import HTMLParser
from xml.etree import ElementTree as ET
@@ -83,10 +89,13 @@
self.url = ''
self.local = False
self.web = False
+ self.original_author = None # None means current user
def DiskText(self):
cl = self
s = ""
+ if cl.original_author:
+ s += "Author: " + cl.original_author + "\n\n"
s += "Description:\n"
s += Indent(cl.desc, "\t")
s += "Files:\n"
@@ -98,6 +107,8 @@
cl = self
s = _change_prolog
s += "\n"
+ if cl.original_author:
+ s += "Author: " + cl.original_author + "\n"
if cl.url != '':
s += 'URL: ' + cl.url + ' # cannot edit\n\n'
s += "Reviewer: " + JoinComma(cl.reviewer) + "\n"
@@ -109,10 +120,11 @@
else:
s += Indent(cl.desc, "\t")
s += "\n"
- s += "Files:\n"
- for f in cl.files:
- s += "\t" + f + "\n"
- s += "\n"
+ if cl.local or cl.name == "new":
+ s += "Files:\n"
+ for f in cl.files:
+ s += "\t" + f + "\n"
+ s += "\n"
return s
def PendingText(self):
@@ -120,6 +132,8 @@
s = cl.name + ":" + "\n"
s += Indent(cl.desc, "\t")
s += "\n"
+ if cl.original_author:
+ s += "\tAuthor: " + cl.original_author + "\n"
s += "\tReviewer: " + JoinComma(cl.reviewer) + "\n"
s += "\tCC: " + JoinComma(cl.cc) + "\n"
s += "\tFiles:\n"
@@ -136,7 +150,7 @@
f.write(self.DiskText())
f.close()
os.rename(path+'!', path)
- if self.web:
+ if self.web and not self.original_author:
EditDesc(self.name, desc=self.desc,
reviewers=JoinComma(self.reviewer), cc=JoinComma(self.cc))
@@ -211,6 +225,7 @@
sname = None
lineno = 0
sections = {
+ 'Author': '',
'Description': '',
'Files': '',
'URL': '',
@@ -242,6 +257,8 @@
sections[k] = StripCommon(sections[k]).rstrip()
cl = CL(name)
+ if sections['Author']:
+ cl.original_author = sections['Author']
cl.desc = sections['Description']
for line in sections['Files'].split('\n'):
i = line.find('#')
@@ -303,7 +320,7 @@
try:
f = GetSettings(name)
except:
- return None, "cannot load CL data from code review server: "+ExceptionDetail()
+ return None, "cannot load CL %s from code review server: %s" % (name, ExceptionDetail())
if 'reviewers' not in f:
return None, "malformed response loading CL data from code review server"
cl.reviewer = SplitCommaSpace(f['reviewers'])
@@ -515,6 +532,8 @@
if opts.get('message'):
return None, "cannot use -m with existing CL"
cl, err = LoadCL(ui, repo, pats[0], web=True)
+ if err != "":
+ return None, err
else:
cl = CL("new")
cl.local = True
@@ -610,6 +629,14 @@
In the absence of options, the change command opens the
change list for editing in the default editor.
+
+ Deleting a change with the -d or -D flag does not affect
+ the contents of the files listed in that change. To revert
+ the files listed in a change, use
+
+ hg revert @123456
+
+ before running hg change -d 123456.
"""
dirty = {}
@@ -631,15 +658,23 @@
taken = TakenFiles(ui, repo)
files = Sub(files, taken)
- if opts["delete"]:
+ if opts["delete"] or opts["deletelocal"]:
+ if opts["delete"] and opts["deletelocal"]:
+ return "cannot use -d and -D together"
+ flag = "-d"
+ if opts["deletelocal"]:
+ flag = "-D"
if name == "new":
- return "cannot use -d with file patterns"
+ return "cannot use "+flag+" with file patterns"
if opts["stdin"] or opts["stdout"]:
- return "cannot use -d with -i or -o"
+ return "cannot use "+flag+" with -i or -o"
if not cl.local:
return "cannot change non-local CL " + name
- PostMessage(cl.name, "*** Abandoned ***", send_mail="checked")
- EditDesc(cl.name, closed="checked")
+ if opts["delete"]:
+ if cl.original_author:
+ return "original author must delete CL; hg change -D will remove locally"
+ PostMessage(cl.name, "*** Abandoned ***", send_mail="checked")
+ EditDesc(cl.name, closed="checked")
cl.Delete(ui, repo)
return
@@ -689,6 +724,55 @@
"""
MySend(None)
+def clpatch(ui, repo, clname, **opts):
+ """import a patch from the code review server
+
+ Imports a patch from the code review server into the local client.
+ If the local client has already modified any of the files that the
+ patch modifies, this command will refuse to apply the patch.
+
+ Submitting an imported patch will keep the original author's
+ name as the Author: line but add your own name to a Committer: line.
+ """
+ cl, patch, err = DownloadCL(ui, repo, clname)
+ argv = ["hgpatch"]
+ if opts["no_incoming"]:
+ argv += ["--checksync=false"]
+ if err != "":
+ return err
+ try:
+ cmd = subprocess.Popen(argv, shell=False, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=None, close_fds=True)
+ except:
+ return "hgpatch: " + ExceptionDetail()
+ if os.fork() == 0:
+ cmd.stdin.write(patch)
+ os._exit(0)
+ cmd.stdin.close()
+ out = cmd.stdout.read()
+ if cmd.wait() != 0:
+ return "hgpatch failed"
+ cl.local = True
+ cl.files = out.strip().split()
+ files = ChangedFiles(ui, repo, [], opts)
+ extra = Sub(cl.files, files)
+ if extra:
+ ui.warn("warning: these files were listed in the patch but not changed:\n\t" + "\n\t".join(extra) + "\n")
+ cl.Flush(ui, repo)
+ ui.write(cl.PendingText() + "\n")
+
+def download(ui, repo, clname, **opts):
+ """download a change from the code review server
+
+ Download prints a description of the given change list
+ followed by its diff, downloaded from the code review server.
+ """
+ cl, patch, err = DownloadCL(ui, repo, clname)
+ if err != "":
+ return err
+ ui.write(cl.EditorText() + "\n")
+ ui.write(patch + "\n")
+ return
+
def file(ui, repo, clname, pat, *pats, **opts):
"""assign files to or remove files from a change list
@@ -784,7 +868,10 @@
if not cl.reviewer:
return "no reviewers listed in CL"
cl.Upload(ui, repo)
- pmsg = "Hello " + JoinComma(cl.reviewer) + ",\n"
+ pmsg = "Hello " + JoinComma(cl.reviewer)
+ if cl.cc:
+ pmsg += " (cc: %s)" % (', '.join(cl.cc),)
+ pmsg += ",\n"
pmsg += "\n"
pmsg += "I'd like you to review the following change.\n"
PostMessage(cl.name, pmsg, send_mail="checked", subject=cl.Subject())
@@ -819,18 +906,35 @@
cmdutil.match = ReplacementForCmdutilMatch
RietveldSetup(ui, repo)
-def CheckContributor(ui, repo):
- user = ui.config("ui", "username")
+def CheckContributor(ui, repo, user=None):
if not user:
- raise util.Abort("[ui] username is not configured in .hgrc")
+ user = ui.config("ui", "username")
+ if not user:
+ raise util.Abort("[ui] username is not configured in .hgrc")
+ userline = FindContributor(ui, repo, user, warn=False)
+ if not userline:
+ raise util.Abort("cannot find %s in CONTRIBUTORS" % (user,))
+ return userline
+
+def FindContributor(ui, repo, user, warn=True):
try:
f = open(repo.root + '/CONTRIBUTORS', 'r')
except:
raise util.Abort("cannot open %s: %s" % (repo.root+'/CONTRIBUTORS', ExceptionDetail()))
for line in f.readlines():
- if line.rstrip() == user.rstrip():
- return
- raise util.Abort("cannot find %s in CONTRIBUTORS" % (user,))
+ line = line.rstrip()
+ if line.startswith('#'):
+ continue
+ if line == user:
+ return line
+ match = re.match(r"(.*) <(.*)>", line)
+ if not match:
+ continue
+ if match.group(2) == user:
+ return line
+ if warn:
+ ui.warn("warning: cannot find %s in CONTRIBUTORS\n" % (user,))
+ return None
def submit(ui, repo, *pats, **opts):
"""submit change to remote repository
@@ -838,7 +942,6 @@
Submits change to remote repository.
Bails out if the local repository is not in sync with the remote one.
"""
- CheckContributor(ui, repo)
repo.ui.quiet = True
if not opts["no_incoming"] and Incoming(ui, repo, opts):
return "local repository out of date; must sync before submit"
@@ -847,6 +950,11 @@
if err != "":
return err
+ user = None
+ if cl.original_author:
+ user = cl.original_author
+ userline = CheckContributor(ui, repo, user)
+
about = ""
if cl.reviewer:
about += "R=" + JoinComma([CutDomain(s) for s in cl.reviewer]) + "\n"
@@ -864,16 +972,30 @@
return "cannot submit non-local CL"
# upload, to sync current patch and also get change number if CL is new.
- cl.Upload(ui, repo)
+ if not cl.original_author:
+ cl.Upload(ui, repo)
about += "%s%s\n" % (server_url_base, cl.name)
+ if cl.original_author:
+ about += "\nCommitter: " + CheckContributor(ui, repo, None) + "\n"
+
# submit changes locally
date = opts.get('date')
if date:
opts['date'] = util.parsedate(date)
opts['message'] = cl.desc.rstrip() + "\n\n" + about
+
+ if opts['dryrun']:
+ print "NOT SUBMITTING:"
+ print "User: ", userline
+ print "Message:"
+ print Indent(opts['message'], "\t")
+ print "Files:"
+ print Indent('\n'.join(cl.files), "\t")
+ return "dry run; not submitted"
+
m = match.exact(repo.root, repo.getcwd(), cl.files)
- node = repo.commit(opts['message'], opts.get('user'), opts.get('date'), m)
+ node = repo.commit(opts['message'], userline, opts.get('date'), m)
if not node:
return "nothing changed"
@@ -906,7 +1028,8 @@
print >>sys.stderr, "URL: ", url
pmsg = "*** Submitted as " + changeURL + " ***\n\n" + opts['message']
PostMessage(cl.name, pmsg, send_mail="checked")
- EditDesc(cl.name, closed="checked")
+ if not cl.original_author:
+ EditDesc(cl.name, closed="checked")
cl.Delete(ui, repo)
def sync(ui, repo, **opts):
@@ -1002,10 +1125,18 @@
change,
[
('d', 'delete', None, 'delete existing change list'),
+ ('D', 'deletelocal', None, 'delete locally, but do not change CL on server'),
('i', 'stdin', None, 'read change list from standard input'),
('o', 'stdout', None, 'print change list to standard output'),
],
- "[-i] [-o] change# or FILE ..."
+ "[-d | -D] [-i] [-o] change# or FILE ..."
+ ),
+ "^clpatch": (
+ clpatch,
+ [
+ ('', 'no_incoming', None, 'disable check for incoming changes'),
+ ],
+ "change#"
),
# Would prefer to call this codereview-login, but then
# hg help codereview prints the help for this command
@@ -1020,6 +1151,11 @@
[],
"",
),
+ "^download": (
+ download,
+ [],
+ "change#"
+ ),
"^file": (
file,
[
@@ -1049,6 +1185,7 @@
submit,
review_opts + [
('', 'no_incoming', None, 'disable initial incoming check (for testing)'),
+ ('n', 'dryrun', None, 'make change only locally (for testing)'),
] + commands.walkopts + commands.commitopts + commands.commitopts2,
"[-r reviewer] [--cc cc] [change# | file ...]"
),
@@ -1139,6 +1276,57 @@
return True
return False
+def DownloadCL(ui, repo, clname):
+ cl, err = LoadCL(ui, repo, clname)
+ if err != "":
+ return None, None, "error loading CL %s: %s" % (clname, ExceptionDetail())
+
+ # Grab RSS feed to learn about CL
+ feed = XMLGet(ui, "/rss/issue/" + clname)
+ if feed is None:
+ return None, None, "cannot download CL"
+
+ # Find most recent diff
+ diff = None
+ prefix = 'http://' + server + '/'
+ for link in feed.findall("{http://www.w3.org/2005/Atom}entry/{http://www.w3.org/2005/Atom}link"):
+ if link.get('rel') != 'alternate':
+ continue
+ text = link.get('href')
+ if not text.startswith(prefix) or not text.endswith('.diff'):
+ continue
+ diff = text[len(prefix)-1:]
+ if diff is None:
+ return None, None, "CL has no diff"
+ diffdata = MySend(diff, force_auth=False)
+
+ # Find author - first entry will be author who created CL.
+ nick = None
+ for author in feed.findall("{http://www.w3.org/2005/Atom}entry/{http://www.w3.org/2005/Atom}author/{http://www.w3.org/2005/Atom}name"):
+ nick = author.findtext("", None).strip()
+ break
+ if not nick:
+ return None, None, "CL has no author"
+
+ # The author is just a nickname: get the real email address.
+ try:
+ data = MySend("/user_popup/" + nick, force_auth=False)
+ except:
+ return None, None, "error looking up %s: %s" % (nick, ExceptionDetail())
+ match = re.match(r"<b>(.*) \((.*)\)</b>", data)
+ if not match or match.group(2) != nick:
+ return None, None, "error looking up %s: cannot parse result" % (nick,)
+ email = match.group(1)
+
+ # Temporary hack until we move to the public code review server.
+ email = re.sub("@google.com$", "@golang.org", email)
+
+ # Print warning if email is not in CONTRIBUTORS file.
+ FindContributor(ui, repo, email)
+ cl.original_author = email
+
+ return cl, diffdata, ""
+
# Like upload.py Send but only authenticates when the
# redirect is to www.google.com/accounts. This keeps
# unnecessary redirects from happening during testing.
@@ -1210,10 +1398,22 @@
f.map[k] = v.replace("\r\n", "\n");
return f.map
+# Fetch the settings for the CL, like reviewer and CC list, by
+# scraping the Rietveld editing forms.
def GetSettings(issue):
- f = GetForm("/" + issue + "/edit")
+ # The /issue/edit page has everything but only the
+ # CL owner is allowed to fetch it (and submit it).
+ f = None
+ try:
+ f = GetForm("/" + issue + "/edit")
+ except:
+ pass
if not f or 'reviewers' not in f:
+ # Maybe we're not the CL owner. Fall back to the
+ # /publish page, which has the reviewer and CC lists,
+ # and then fetch the description separately.
f = GetForm("/" + issue + "/publish")
+ f['description'] = MySend("/"+issue+"/description", force_auth=False)
return f
def CreateIssue(subject, desc):
