--- a/review/api.py Tue Jun 29 20:00:12 2010 -0400
+++ b/review/api.py Tue Jun 29 20:49:32 2010 -0400
@@ -61,7 +61,7 @@
class SignoffExists(Exception):
- """Raised when trying to signoff twice without forcing."""
+ """Raised when trying to signoff twice."""
pass
class FileNotInChangeset(Exception):
@@ -325,6 +325,24 @@
old.style = style if style is not None else old.style
old._rename(self.ui, self.repo, old.identifier)
+ def edit_signoff(self, identifier, message=None, opinion=None, style=None):
+ olds = self.get_items(identifier)
+
+ if len(olds) == 0:
+ raise UnknownIdentifier
+ elif len(olds) > 1:
+ raise AmbiguousIdentifier
+
+ old = olds[0]
+ if old.itemtype != 'signoff':
+ raise WrongEditItemType()
+
+ old.hgdate = util.makedate()
+ old.opinion = opinion if opinion is not None else old.opinion
+ old.message = message if message is not None else old.message
+ old.style = style if style is not None else old.style
+ old._rename(self.ui, self.repo, old.identifier)
+
class ReviewChangeset(object):
"""The review data about one changeset in the target repository.
@@ -408,24 +426,22 @@
return self.signoffs_for_user(self.ui.username())
- def add_signoff(self, message, opinion='', force=False, style=''):
+ def add_signoff(self, message, opinion='', style=''):
"""Add (and commit) a signoff for the given revision.
The opinion argument should be 'yes', 'no', or ''.
- If a signoff from the user already exists, a SignoffExists exception
- will be raised unless the force argument is used.
+ If a signoff from the user already exists a SignoffExists exception
+ will be raised.
"""
existing = self.signoffs_for_current_user()
if existing:
- if not force:
- raise SignoffExists
- existing[0]._delete(self.ui, self.repo)
+ raise SignoffExists
signoff = ReviewSignoff(self.ui.username(), util.makedate(),
- self.node, opinion, message, style)
+ self.node, opinion, message, style)
signoff._commit(self.ui, self.repo)
def add_comment(self, message, filename='', lines=[], style=''):
--- a/review/cli.py Tue Jun 29 20:00:12 2010 -0400
+++ b/review/cli.py Tue Jun 29 20:49:32 2010 -0400
@@ -52,7 +52,20 @@
fnames[0], rd.target[comment.node].rev()))
def _edit_signoff(rd, signoff, **opts):
- raise util.Abort('unimplemented, sorry')
+ mdown = opts.pop('mdown')
+ message = opts.pop('message').strip()
+
+ yes, no = opts.pop('yes'), opts.pop('no')
+ if yes and no:
+ raise util.Abort(messages.SIGNOFF_OPINION_CONFLICT)
+ opinion = 'yes' if yes else ('no' if no else signoff.opinion)
+
+ if not message:
+ message = signoff.message
+
+ style = 'markdown' if mdown or signoff.style == 'markdown' else ''
+
+ rd.edit_signoff(signoff.identifier, message, opinion, style)
def _web_command(ui, repo, **opts):
@@ -120,7 +133,6 @@
def _signoff_command(ui, repo, **opts):
message = opts.pop('message').strip()
- force = opts.pop('force')
mdown = opts.pop('mdown')
rd = _get_datastore(ui, repo)
rcset = rd[opts.pop('rev')]
@@ -130,7 +142,7 @@
raise util.Abort(messages.SIGNOFF_OPINION_CONFLICT)
opinion = 'yes' if yes else ('no' if no else '')
- if rcset.signoffs_for_current_user() and not force:
+ if rcset.signoffs_for_current_user():
raise util.Abort(messages.SIGNOFF_EXISTS)
if not message:
@@ -141,7 +153,7 @@
style = mdown and 'markdown' or ''
- rcset.add_signoff(message=message, opinion=opinion, force=force, style=style)
+ rcset.add_signoff(message=message, opinion=opinion, style=style)
def _check_command(ui, repo, **opts):
rd = _get_datastore(ui, repo)
--- a/review/helps.py Tue Jun 29 20:00:12 2010 -0400
+++ b/review/helps.py Tue Jun 29 20:49:32 2010 -0400
@@ -79,7 +79,7 @@
"""
SIGNOFF = r"""
-USAGE: hg review --signoff [-m MESSAGE] [--mdown] [--yes | --no] [-r REV] [--force]
+USAGE: hg review --signoff [-m MESSAGE] [--mdown] [--yes | --no] [-r REV]
If no revision is given the current parent of the working directory will be
used.
@@ -89,9 +89,6 @@
individual project to decide exactly what that means. If neither option is
given the signoff will be marked as "neutral".
-If you've already signed off on a changeset you can use ``--force`` to replace
-your previous signoff with a new one.
-
If ``--mdown`` is used the signoff message text will be interpreted as Markdown.
Examples::
@@ -99,7 +96,6 @@
hg review --signoff -m 'I do not work on this part of the code.'
hg review --signoff --yes -m 'Thanks, this change looks good.'
hg review --signoff --no -m 'This would break backwards compatibility!'
- hg review --signoff --yes --force -m 'Nevermind, this is fine.'
"""
--- a/review/messages.py Tue Jun 29 20:00:12 2010 -0400
+++ b/review/messages.py Tue Jun 29 20:49:32 2010 -0400
@@ -76,7 +76,7 @@
"""
SIGNOFF_EXISTS = """\
-you have already signed off on this changeset (use -f to replace)!
+you have already signed off on this changeset (use "hg review --edit" to modify)!
"""
SIGNOFF_EDITOR = """\
--- a/review/templates/pieces/forms/signoff.html Tue Jun 29 20:00:12 2010 -0400
+++ b/review/templates/pieces/forms/signoff.html Tue Jun 29 20:49:32 2010 -0400
@@ -26,6 +26,7 @@
</div>
{% if cu_signoff %}
+ <input type="hidden" value="{{ cu_signoff.identifier }}" name="current"/>
<a class="submit button" href="#"><span>Change Signoff</span></a>
{% else %}
<a class="submit button" href="#"><span>Add Signoff</span></a>
--- a/review/tests/test_check.py Tue Jun 29 20:00:12 2010 -0400
+++ b/review/tests/test_check.py Tue Jun 29 20:49:32 2010 -0400
@@ -1,45 +1,32 @@
from nose import with_setup
from util import setup_reviewed_sandbox, teardown_sandbox, review, should_fail_with
+from util import get_identifiers
from .. import messages
@with_setup(setup_reviewed_sandbox, teardown_sandbox)
def test_check_empty():
- review(check=True)
-
- output = review(check=True, verbose=True)
- assert messages.CHECK_SUCCESS in output
+ def t(rev):
+ output = review(check=True, rev=rev)
+ assert not output
- review(signoff=True, no=True, message='.')
- output = review(check=True, verbose=True)
- assert messages.CHECK_SUCCESS in output
+ output = review(check=True, verbose=True, rev=rev)
+ assert messages.CHECK_SUCCESS in output
- review(signoff=True, yes=False, message='.', force=True)
- output = review(check=True, verbose=True)
- assert messages.CHECK_SUCCESS in output
-
- review(comment=True, message='.')
- output = review(check=True, verbose=True)
- assert messages.CHECK_SUCCESS in output
+ review(signoff=True, no=True, message='.', rev=rev)
+ output = review(check=True, verbose=True, rev=rev)
+ assert messages.CHECK_SUCCESS in output
-@with_setup(setup_reviewed_sandbox, teardown_sandbox)
-def test_check_empty_non_tip():
- review(rev='0', check=True)
-
- output = review(rev='0', check=True, verbose=True)
- assert messages.CHECK_SUCCESS in output
+ i = get_identifiers(rev)[0]
+ review(edit=i, yes=True, rev=rev)
+ output = review(check=True, verbose=True, rev=rev)
+ assert messages.CHECK_SUCCESS in output
- review(signoff=False, message='.')
- output = review(rev='0', check=True, verbose=True)
- assert messages.CHECK_SUCCESS in output
-
- review(signoff=True, message='.')
- output = review(rev='0', check=True, verbose=True)
- assert messages.CHECK_SUCCESS in output
-
- review(comment=True, message='.')
- output = review(rev='0', check=True, verbose=True)
- assert messages.CHECK_SUCCESS in output
+ review(comment=True, message='.', rev=rev)
+ output = review(check=True, verbose=True, rev=rev)
+ assert messages.CHECK_SUCCESS in output
+ t('.')
+ t('0')
@with_setup(setup_reviewed_sandbox, teardown_sandbox)
def test_check_no_nos():
@@ -49,7 +36,8 @@
review(signoff=True, no=True, message='.')
should_fail_with(messages.CHECK_HAS_NOS, check=True, verbose=True, no_nos=True)
- review(signoff=True, yes=True, message='.', force=True)
+ i = get_identifiers()[0]
+ review(edit=i, yes=True)
output = review(check=True, verbose=True, no_nos=True)
assert messages.CHECK_SUCCESS in output
@@ -74,7 +62,8 @@
output = review(check=True, verbose=True, seen=True)
assert messages.CHECK_SUCCESS in output
- review(signoff=True, no=True, message='.', force=True)
+ i = get_identifiers()[0]
+ review(edit=i, no=True)
output = review(check=True, verbose=True, seen=True)
assert messages.CHECK_SUCCESS in output
@@ -91,7 +80,8 @@
should_fail_with(messages.CHECK_HAS_NOS, check=True, verbose=True, no_nos=True, seen=True)
should_fail_with(messages.CHECK_HAS_NOS, check=True, verbose=True, no_nos=True, seen=True, yeses='0')
- review(signoff=True, yes=True, message='.', force=True)
+ i = get_identifiers()[0]
+ review(edit=i, yes=True)
output = review(check=True, verbose=True, no_nos=True, seen=True, yeses='0')
assert messages.CHECK_SUCCESS in output
--- a/review/tests/test_edit.py Tue Jun 29 20:00:12 2010 -0400
+++ b/review/tests/test_edit.py Tue Jun 29 20:49:32 2010 -0400
@@ -26,6 +26,7 @@
should_fail_with(messages.AMBIGUOUS_ID % i, edit=i)
+
@with_setup(setup_reviewed_sandbox, teardown_sandbox)
def test_touch_comment():
def t(rev):
@@ -131,3 +132,70 @@
check_comment_exists_on_line(2, files=['always_changing'], rev=rev)
t('1')
t('0')
+
+
+@with_setup(setup_reviewed_sandbox, teardown_sandbox)
+def test_touch_signoff():
+ def t(rev):
+ review(rev=rev, signoff=True, message='test', yes=True)
+ i = get_identifiers(rev)[0]
+
+ # This sucks, but we need to do it to support testing "touch" edits.
+ time.sleep(1.1)
+
+ output = review(edit=i)
+ assert not output
+ output = review(rev=rev, verbose=True)
+ assert '(%s)' % i not in output
+ assert len(get_identifiers(rev)) == 1
+ assert messages.REVIEW_LOG_SIGNOFF_LINE % 'test' in output
+ assert 'yes' in output
+ t('1')
+ t('0')
+
+@with_setup(setup_reviewed_sandbox, teardown_sandbox)
+def test_edit_signoff_message():
+ def t(rev):
+ review(rev=rev, signoff=True, message='test')
+ i = get_identifiers(rev)[0]
+
+ output = review(edit=i, message='edited')
+ assert not output
+ output = review(rev=rev, verbose=True)
+ assert '(%s)' % i not in output
+ assert len(get_identifiers(rev)) == 1
+ assert messages.REVIEW_LOG_SIGNOFF_LINE % 'test' not in output
+ assert messages.REVIEW_LOG_SIGNOFF_LINE % 'edited' in output
+ t('.')
+ t('0')
+
+@with_setup(setup_reviewed_sandbox, teardown_sandbox)
+def test_edit_signoff_opinion():
+ def t(rev):
+ review(rev=rev, signoff=True, message='test')
+
+ output = review(rev=rev, verbose=True)
+ assert 'as yes' not in output
+ assert 'as neutral' in output
+ assert 'as no' not in output
+
+ i = get_identifiers(rev)[0]
+ output = review(edit=i, yes=True)
+ assert not output
+
+ output = review(rev=rev, verbose=True)
+ assert 'as yes' in output
+ assert 'as neutral' not in output
+ assert 'as no' not in output
+
+ i = get_identifiers(rev)[0]
+ output = review(edit=i, no=True)
+ assert not output
+
+ output = review(rev=rev, verbose=True)
+ assert 'as yes' not in output
+ assert 'as neutral' not in output
+ assert 'as no' in output
+ t('.')
+ t('0')
+
--- a/review/tests/test_signoff.py Tue Jun 29 20:00:12 2010 -0400
+++ b/review/tests/test_signoff.py Tue Jun 29 20:49:32 2010 -0400
@@ -71,8 +71,6 @@
should_fail_with(messages.SIGNOFF_EXISTS, signoff=True, message='Test signoff two.')
- review(signoff=True, message='Test signoff two.', force=True)
-
output = review()
assert messages.REVIEW_LOG_SIGNOFFS % (1, 0, 0, 1) in output
--- a/review/tests/util.py Tue Jun 29 20:00:12 2010 -0400
+++ b/review/tests/util.py Tue Jun 29 20:49:32 2010 -0400
@@ -132,12 +132,12 @@
a1, a2 = (messages.REVIEW_LOG_COMMENT_AUTHOR % '|').split('|')
-s1, s2 = (messages.REVIEW_LOG_SIGNOFF_AUTHOR % ('|', 'neutral')).split('|')
+s1, s2, s3 = (messages.REVIEW_LOG_SIGNOFF_AUTHOR % ('|', '|')).split('|')
def get_identifiers(rev='.', files=[]):
return [l.split(' ')[-1].strip('()\n')
for l in review(rev=rev, verbose=True, args=files).splitlines()
- if (a1 in l and a2 in l) or (s1 in l and s2 in l)]
+ if (a1 in l and a2 in l) or (s1 in l and s2 in l and s3 in l)]
COMMENT_LINE_ERROR = '''\
--- a/review/web.py Tue Jun 29 20:00:12 2010 -0400
+++ b/review/web.py Tue Jun 29 20:49:32 2010 -0400
@@ -104,7 +104,7 @@
def _handle_signoff(revhash):
- signoff = request.form.get('signoff', None)
+ signoff = request.form['signoff']
if signoff not in ['yes', 'no', 'neutral']:
abort(400)
@@ -115,8 +115,12 @@
body = request.form.get('new-signoff-body', '')
style = 'markdown' if request.form.get('signoff-markdown') else ''
- rcset = g.datastore[revhash]
- rcset.add_signoff(body, signoff, force=True, style=style)
+ current = request.form.get('current')
+ if current:
+ g.datastore.edit_signoff(current, body, signoff, style=style)
+ else:
+ rcset = g.datastore[revhash]
+ rcset.add_signoff(body, signoff, style=style)
return redirect("%s/changeset/%s/" % (app.site_root, revhash))