Commits

Patrick Mézard committed 69941b9

hg: store changelist metadata in changesets

This simplifies the implementation as we do not have to link codereview
metadata with changesets anymore. Also, changesets published on codereview
can be exchanged between clones by regular means, preserving the metadata.

The drawback is clpush and clemail now always require selected changesets
to be non-public to be able to rewrite their metadata.

  • Participants
  • Parent commits 62c474e

Comments (0)

Files changed (3)

File lib/codereview/codereview.py

 	l.sort()
 	return l
 
-def Intersect(l1, l2):
-	return [l for l in l1 if l in l2]
-
 #######################################################################
 # RE: UNICODE STRING HANDLING
 #
 		self.private = False
 		self.lgtm = []
 
-	def DiskText(self):
-		cl = self
-		s = ""
-		if cl.private:
-			s += "Private: " + str(self.private) + "\n"
-		s += "Mailed: " + str(self.mailed) + "\n"
-		s += "Description:\n"
-		s += Indent(cl.desc, "\t")
-		s += "Files:\n"
-		for f in cl.files:
-			s += "\t" + f + "\n"
-		typecheck(s, str)
-		return s
-
 	def Flush(self, ui, repo):
 		if self.name == "new":
 			self.Upload(ui, repo, gofmt_just_warn=True, creating=True)
-		dir = CodeReviewDir(ui, repo)
-		path = dir + '/cl.' + self.name
-		f = open(path+'!', "w")
-		f.write(self.DiskText())
-		f.close()
-		if sys.platform == "win32" and os.path.isfile(path):
-			os.remove(path)
-		os.rename(path+'!', path)
 		if self.web:
 			EditDesc(self.name, desc=self.desc,
 				reviewers=JoinComma(self.reviewer), cc=JoinComma(self.cc),
 				private=self.private)
 
-	def Delete(self, ui, repo):
-		dir = CodeReviewDir(ui, repo)
-		os.unlink(dir + "/cl." + self.name)
-
 	def Subject(self):
 		typecheck(self.desc, str)
 		s = self.desc.split('\n')[0]
 	typecheck(name, str)
 	return re.match("^[0-9]+$", name)
 
-def ParseCL(text, name):
-	typecheck(text, str)
-	typecheck(name, str)
-	sname = None
-	lineno = 0
-	sections = {
-		'Author': '',
-		'Description': '',
-		'Files': '',
-		'URL': '',
-		'Reviewer': '',
-		'CC': '',
-		'Mailed': '',
-		'Private': '',
-	}
-	for line in text.split('\n'):
-		lineno += 1
-		line = line.rstrip()
-		if line != '' and line[0] == '#':
+def stripclmeta(desc):
+	"""Returns desc stripped from its changelist metadata."""
+	kept = []
+	for line in desc.split('\n'):
+		if line.startswith('CLMETA-'):
 			continue
-		if line == '' or line[0] == ' ' or line[0] == '\t':
-			if sname == None and line != '':
-				return None, lineno, 'text outside section'
-			if sname != None:
-				sections[sname] += line + '\n'
+		kept.append(line)
+	return '\n'.join(kept).rstrip() + '\n'
+
+def addclmeta(cl, desc):
+	"""Returns the input description augmented with metadata from a changelist."""
+	desc = stripclmeta(desc)
+
+	lines = ['URL: %s' % cl.url]
+	if cl.mailed:
+		lines.append('MAILED: true')
+	if cl.private:
+		lines.append('PRIVATE: true')
+	lines = [('CLMETA-' + l + '\n') for l in lines]
+	meta = ''.join(lines)
+	return desc + '\n' + meta
+
+def parseclmeta(desc):
+	"""Creates a CL instance from a changeset description metadata, or None."""
+	meta = {}
+	desclines = []
+	for line in desc.split('\n'):
+		if not line.startswith('CLMETA-'):
+			desclines.append(line)
 			continue
-		p = line.find(':')
-		if p >= 0:
-			s, val = line[:p].strip(), line[p+1:].strip()
-			if s in sections:
-				sname = s
-				if val != '':
-					sections[sname] += val + '\n'
-				continue
-		return None, lineno, 'malformed section header'
+		line = line[len('CLMETA-'):]
+		if ':' not in line:
+			continue
+		key, value = line.split(':', 1)	
+		if key and value:
+			meta[key.strip()] = value.strip()
 
-	for k in sections:
-		sections[k] = StripCommon(sections[k]).rstrip()
+	desc = '\n'.join(desclines) + '\n'
+	if not desc.strip():
+		raise hg_util.Abort('changeset description contains only CLMETA lines')
+	if not meta:
+		return None
+	if 'URL' not in meta:
+		raise hg_util.Abort('changelist URL not found in description metadata')
+	url = meta['URL']
+	m = re.search('^\S*/(\d+)$', url, re.I)
+	if not m:
+		raise hg_util.Abort('cannot extract changelist name from URL')
+	cl = CL(m.group(1))
+	cl.desc = desc
+	cl.url = url
+	cl.private = meta.get('PRIVATE', 'false').lower() == 'true'
+	cl.mailed = meta.get('MAILED', 'false').lower() == 'true'
+	return cl
 
-	cl = CL(name)
-	cl.desc = sections['Description']
-	for line in sections['Files'].split('\n'):
-		i = line.find('#')
-		if i >= 0:
-			line = line[0:i].rstrip()
-		line = line.strip()
-		if line == '':
-			continue
-		cl.files.append(line)
-	cl.reviewer = SplitCommaSpace(sections['Reviewer'])
-	cl.cc = SplitCommaSpace(sections['CC'])
-	cl.url = sections['URL']
-	if sections['Mailed'] != 'False':
-		# Odd default, but avoids spurious mailings when
-		# reading old CLs that do not have a Mailed: line.
-		# CLs created with this update will always have 
-		# Mailed: False on disk.
-		cl.mailed = True
-	if sections['Private'] in ('True', 'true', 'Yes', 'yes'):
-		cl.private = True
-	if cl.desc == '<enter description here>':
-		cl.desc = ''
-	return cl, 0, ''
+def clfromctx(ctx):
+	"""If ctx contains changelist metadata, parse them and return a CL instance,
+	returns None otherwise.
+	"""
+	cl = parseclmeta(ctx.description())
+	if cl is None:
+		return cl
+	cl.files = list(ctx.files())
+	return cl
+
+def mergecls(local, remote, private=None):
+	"""Merge local cl into remote one, return updated remote."""
+	remote.desc = local.desc
+	remote.files = list(local.files)
+	remote.mailed = local.mailed
+	if private is not None:
+		remote.private = bool(private)
+	return remote
 
 def SplitCommaSpace(s):
 	typecheck(s, str)
 		s += ": " + arg
 	return s
 
-def IsLocalCL(ui, repo, name):
-	return GoodCLName(name) and os.access(CodeReviewDir(ui, repo) + "/cl." + name, 0)
-
 # Load CL from disk and/or the web.
-def LoadCL(ui, repo, name, web=True):
+def LoadCL(ui, repo, name):
 	typecheck(name, str)
 	set_status("loading CL " + name)
 	if not GoodCLName(name):
 		return None, "invalid CL name"
-	dir = CodeReviewDir(ui, repo)
-	path = dir + "cl." + name
-	if os.access(path, 0):
-		ff = open(path)
-		text = ff.read()
-		ff.close()
-		cl, lineno, err = ParseCL(text, name)
-		if err != "":
-			return None, "malformed CL data: "+err
-		cl.local = True
-	else:
-		cl = CL(name)
-	if web:
-		set_status("getting issue metadata from web")
-		d = JSONGet(ui, "/api/" + name + "?messages=true")
-		set_status(None)
-		if d is None:
-			return None, "cannot load CL %s from server" % (name,)
-		if 'owner_email' not in d or 'issue' not in d or str(d['issue']) != name:
-			return None, "malformed response loading CL data from code review server"
-		cl.dict = d
-		cl.reviewer = d.get('reviewers', [])
-		cl.cc = d.get('cc', [])
-		cl.desc = d.get('description', "")
-		cl.url = server_url_base + name
-		cl.web = True
-		cl.private = d.get('private', False) != False
-		cl.lgtm = []
-		for m in d.get('messages', []):
-			if m.get('approval', False) == True or m.get('disapproval', False) == True:
-				who = re.sub('@.*', '', m.get('sender', ''))
-				text = re.sub("\n(.|\n)*", '', m.get('text', ''))
-				cl.lgtm.append((who, text, m.get('approval', False)))
+	cl = CL(name)
+
+	set_status("getting issue metadata from web")
+	d = JSONGet(ui, "/api/" + name + "?messages=true")
+	set_status(None)
+	if d is None:
+		return None, "cannot load CL %s from server" % (cl.name,)
+	if 'owner_email' not in d or 'issue' not in d or str(d['issue']) != cl.name:
+		return None, "malformed response loading CL data from code review server"
+	cl.dict = d
+	cl.reviewer = d.get('reviewers', [])
+	cl.cc = d.get('cc', [])
+	cl.desc = d.get('description', "")
+	cl.url = server_url_base + name
+	cl.web = True
+	cl.private = d.get('private', False) != False
+	cl.lgtm = []
+	for m in d.get('messages', []):
+		if m.get('approval', False) == True or m.get('disapproval', False) == True:
+			who = re.sub('@.*', '', m.get('sender', ''))
+			text = re.sub("\n(.|\n)*", '', m.get('text', ''))
+			cl.lgtm.append((who, text, m.get('approval', False)))
 
 	set_status("loaded CL " + name)
 	return cl, ''
 
-# Find repository root.  On error, ui.warn and return None
-def RepoDir(ui, repo):
-	url = repo.url();
-	if not url.startswith('file:'):
-		ui.warn("repository %s is not in local file system\n" % (url,))
-		return None
-	url = url[5:]
-	if url.endswith('/'):
-		url = url[:-1]
-	typecheck(url, str)
-	return url
-
-# Find (or make) code review directory.  On error, ui.warn and return None
-def CodeReviewDir(ui, repo):
-	dir = RepoDir(ui, repo)
-	if dir == None:
-		return None
-	dir += '/.hg/codereview/'
-	if not os.path.isdir(dir):
-		try:
-			os.mkdir(dir, 0700)
-		except:
-			ui.warn('cannot mkdir %s: %s\n' % (dir, ExceptionDetail()))
-			return None
-	typecheck(dir, str)
-	return dir
-
 # Turn leading tabs into spaces, so that the common white space
 # prefix doesn't get confused when people's editors write out 
 # some lines with spaces, some with tabs.  Only a heuristic
 def CommandLineCL(ui, repo, clname, opts, op="verb", defaultcc=None):
 	if not GoodCLName(clname):
 		return None, "invalid changelist name: " + clname
-	cl, err = LoadCL(ui, repo, clname, web=True)
+	cl, err = LoadCL(ui, repo, clname)
 	if err != "":
 		return None, err
 	if opts.get('reviewer'):
 
 def DownloadCL(ui, repo, clname):
 	set_status("downloading CL " + clname)
-	cl, err = LoadCL(ui, repo, clname, web=True)
+	cl, err = LoadCL(ui, repo, clname)
 	if err != "":
 		return None, None, None, "error loading CL %s: %s" % (clname, err)
 
 	finally:
 		lockmod.release(lock, wlock)
 
-def extractclname(desc):
-	"""Find a changelist reference in changeset message and return the
-	description without the reference and the changelist name, or a None tuple.
-	"""
-	desc = desc.strip().splitlines()
-	if not desc:
-		return None, None
-	last = desc[-1].strip()
-	m = re.search('^codereview\s+cl:\s+\S+?(\d+)$', last, re.I)
-	if not m:
-		return None, None
-	name = m.group(1)
-	if not GoodCLName(name):
-		raise hg_util.Abort(
-			"CL line found with invalid name, please correct it: %s" % last)
-	desc = '\n'.join(desc[:-1]).strip() + '\n'
-	return desc, name
-
 @hgcommand('clpush',
 	[('n', 'test', None,
 		'print patches and base files which would be sent')],
 
 	This command requires evolve extension.
 
-	If selected changeset description contains a trailing line like:
+	If selected changeset was clpushed before and contains CLMETA- lines in
+	it description, fetch metadata from remote changelist, merge them into
+	the local changelist, then update the remote changelist with local content.
 
-	    Codereview CL: https://codereview.appspot.com/64640043
+	Otherwise, a new changelist is created on codereview server.
 
-	it updates referenced changelist with changeset content. Otherwise, a new
-	changelist is created and the changeset is made obsolete by a copy amended
-	with the changelist URL. This requires submitted changeset not to be public.
+	In both cases, the local changeset cannot be public if changelist metadata
+	is being created or updated.
 	"""
 	if codereview_disabled:
 		raise hg_util.Abort(codereview_disabled)
 	evolve = extensions.find('evolve')
 
 	ctx = scmutil.revsingle(repo, rev)
+	if not ctx.mutable():
+		raise hg_util.Abort(
+			'cannot create or update changelist from a public changeset')
+	olddesc = ctx.description()
+	localcl = clfromctx(ctx)
 	creating = False
-	desc, name = extractclname(ctx.description())
-	if name is not None:
-		cl, err = LoadCL(ui, repo, name, web=True)
+	if localcl is not None:
+		cl, err = LoadCL(ui, repo, localcl.name)
 		if err:
 			raise hg_util.Abort(err)
-		if not cl.local:
-			raise hg_util.Abort("cannot change non-local CL " + name)
+		mergecls(localcl, cl)
 	else:
-		if not ctx.mutable():
-			raise hg_util.Abort("cannot create CL from a public changeset")
+		creating = True
 		cl = CL("new")
-		desc = ctx.description()
-		creating = True
-	cl.desc = desc
-	cl.files = list(ctx.files())
+		cl.desc = ctx.description()
+		cl.files = list(ctx.files())
 
 	test = opts.get("test")
 	if not test:
 	cl.Upload(ui, repo, quiet=creating, test=test, ctx=ctx)
 	if test:
 		return
-	if creating:
-		desc = ctx.description() + ('\n\nCodereview CL: %s\n' % cl.url)
-		reword(ui, repo, ctx, desc)
+
+	newdesc = addclmeta(cl, ctx.description())
+	if newdesc != olddesc:
+		ui.status('amending changeset changelist metadata\n')
+		reword(ui, repo, ctx, newdesc)
 
 @hgcommand('^clmail',
 	[('', 'no-email', None,
 def clmail(ui, repo, rev, **opts):
 	"""mail a change for review
 
+	This command requires evolve extension.
+
 	Uploads a patch to the code review server and then sends mail
 	to the reviewer and CC list asking for a review.
+
+	Since this can change changelist metadata, selected changeset may be
+	rewritten and cannot be public.
 	"""
 	if codereview_disabled:
 		raise hg_util.Abort(codereview_disabled)
+	evolve = extensions.find('evolve')
+
 	ctx = scmutil.revsingle(repo, rev)
-	desc, name = extractclname(ctx.description())
-	if name is None:
+	if not ctx.mutable():
+		raise hg_util.Abort("cannot update and email a public changeset")
+	olddesc = ctx.description()
+
+	localcl = clfromctx(ctx)
+	if localcl is None:
 		raise hg_util.Abort('no changelist associated to specified revision',
 				hint="use 'hg clpush'")
-	cl, err = CommandLineCL(ui, repo, name, opts, op="mail", defaultcc=defaultcc)
+	cl, err = CommandLineCL(ui, repo, localcl.name, opts, op="mail",
+			defaultcc=defaultcc)
 	if err:
 		raise hg_util.Abort(err)
+	mergecls(localcl, cl)
 	cl.Upload(ui, repo, gofmt_just_warn=True, ctx=ctx)
 	if not cl.reviewer:
 		# If no reviewer is listed, assign the review to defaultcc.
 		cl.Flush(ui, repo)
 
 	if not cl.files:
-			raise hg_util.Abort("no changed files, not sending mail")
+		raise hg_util.Abort("no changed files, not sending mail")
 
 	if not opts.get('no_email'):
 		cl.Mail(ui, repo)
 
+	newdesc = addclmeta(cl, ctx.description())
+	if newdesc != olddesc:
+		ui.status('amending changeset changelist metadata\n')
+		reword(ui, repo, ctx, newdesc)
+
 @hgcommand('^clclose',
 	[('', 'delete', None, 'delete changelist'),
 	 ('', 'no-email', None, 'update the changelist but do not email')],
 def clclose(ui, repo, rev, **opts):
 	"""close a change list
 
-	Non-public changesets description is rewritten without the reference to
-	the closed change list.
+	Non-public changesets description is stripped of any changelist metadata.
 	"""
 	if codereview_disabled:
 		raise hg_util.Abort(codereview_disabled)
 	delete = opts.get('delete')
 
 	ctx = scmutil.revsingle(repo, rev)
-	desc, name = extractclname(ctx.description())
-	if name is None:
+	localcl = clfromctx(ctx)
+	if localcl is None:
 		raise hg_util.Abort('no changelist associated to specified revision')
-	cl, err = LoadCL(ui, repo, name, web=True)
+	cl, err = LoadCL(ui, repo, localcl.name)
 	if err:
 		raise hg_util.Abort(err)
-	if not cl.local:
-		raise hg_util.Abort("cannot change non-local CL " + name)
+	mergecls(localcl, cl)
 	PostMessage(ui, cl.name, "*** Abandoned ***", send_mail=cl.mailed and email)
 	# call EditDesc even with --delete for test coverage purpose
 	EditDesc(cl.name, closed=True, private=cl.private)
 	if delete:
 		DeleteCL(cl.name)
-	cl.Delete(ui, repo)
 	if ctx.mutable():
-		reword(ui, repo, ctx, desc)
+		reword(ui, repo, ctx, stripclmeta(ctx.description()))
 	else:
 		ui.warn('skipping removing CL reference from public changeset')
 	if delete:
 		desc = desc[0].strip()
 	else:
 		desc = '(no description)'
-	ui.status('CL %s imported as:\n' % clname)
+	ui.status('changelist %s imported as:\n' % clname)
 	ui.status('  %d:%s %s\n' % (ctx.rev(), str(ctx), desc))
 
 def clidkw(**args):
 	""":clid: String. Codereview changelist identifier or the empty string."""
-	_, name = extractclname(args['ctx'].description())
-	return name or ''
+	cl = clfromctx(args['ctx'])
+	if cl is None:
+		return ''
+	return cl.name
 
 @hgcommand('code-login', [], '')
 def code_login(ui, repo, **opts):

File tests/test-climport.t

 
   $ hg clpush --traceback .
   Issue created. URL: https://codereview.appspot.com/\d+ (re)
+  amending changeset changelist metadata
   $ clid=$(hg log -r . --template '{clid}')
 
 Modify the changeset so we can import it again later
 
   $ hg climport ${clid}
   applying .*CL.*.diff (re)
-  CL \d+ imported as: (re)
+  changelist \d+ imported as: (re)
     6:\S{12} change (re)
 
   $ hg export -g .
   $ hg climport --exact ${clid}
   1 files updated, 0 files merged, 1 files removed, 0 files unresolved
   applying .* (re)
-  CL \d+ imported as: (re)
+  changelist \d+ imported as: (re)
     8:\S{12} change (re)
   $ hg log -r .^
   changeset:   0:ef94c2c26560

File tests/test-clpush.t

 
   $ hg phase --public .
   $ hg clpush .
-  abort: cannot create CL from a public changeset
+  abort: cannot create or update changelist from a public changeset
   [255]
   $ hg phase --draft --force .
 
 
   $ hg clpush .
   Issue created. URL: https://codereview.appspot.com/\d+ (re)
-  $ hg log -r . --template '{desc}\n'
+  amending changeset changelist metadata
+  $ hg parents --template '{desc}\n'
   test
   
-  Codereview CL: https://codereview.appspot.com/\d+ (re)
+  CLMETA-URL: https://codereview.appspot.com/\d+ (re)
 
 Updating a changelist
 
   $ hg amend
   $ hg clpush .
   Issue updated. URL: https://codereview.appspot.com/\d+ (re)
-  $ hg log -r . --template '{desc}\n'
+  amending changeset changelist metadata
+  $ hg parents --template '{desc}\n'
   test
   
-  Codereview CL: https://codereview.appspot.com/\d+ (re)
+  CLMETA-URL: https://codereview.appspot.com/\d+ (re)
 
 Mailing change
 
   $ hg clmail --no-email 2>&1 | grep 'invalid arguments'
   hg clmail: invalid arguments
+  $ hg phase --public .
+  $ hg clmail --no-email .
+  abort: cannot update and email a public changeset
+  [255]
+  $ hg phase --draft --force .
   $ hg clmail --no-email .
   Issue updated. URL: https://codereview.appspot.com/\d+ (re)
+  amending changeset changelist metadata
+  $ hg parents --template '{desc}\n'
+  test
+  
+  CLMETA-URL: https://codereview.appspot.com/\d+ (re)
 
 Test 'clid' keyword on published changeset
 
-  $ hg log -r . --template 'CLID: {clid}\n'
+  $ hg parents --template 'CLID: {clid}\n'
   CLID: \d+ (re)
-  $ clid=$(hg log -r . --template '{clid}')
+  $ clid=$(hg parents --template '{clid}')
 
 Import invalid changelist