Attach comments to files

Since each revision now has files, associate comments with the
file relation rather than revisions so that the path is not stored
twice in the database.

Also correct a problem where comments could be left on empty files
or lines that are not in a file in the unified view.

The SQLAlchemy constraints API seems to have changed between
0.9.9 and 1.0.4.  Support both behaviors to be user-friendly even
though we now specify 1.0.4.

Change-Id: If6593d279a432ea8a48f4bd74a157e4978e69eaa
This commit is contained in:
James E. Blair 2015-04-19 14:34:48 -04:00
parent a61c5fdf67
commit c0cabc82f6
10 changed files with 277 additions and 117 deletions

View File

@ -0,0 +1,64 @@
"""attach comments to files
Revision ID: 254ac5fc3941
Revises: 50344aecd1c2
Create Date: 2015-04-13 15:52:07.104397
"""
# revision identifiers, used by Alembic.
revision = '254ac5fc3941'
down_revision = '50344aecd1c2'
import sys
import warnings
from alembic import op
import sqlalchemy as sa
from gertty.dbsupport import sqlite_alter_columns, sqlite_drop_columns
def upgrade():
with warnings.catch_warnings():
warnings.simplefilter("ignore")
op.add_column('comment', sa.Column('file_key', sa.Integer()))
sqlite_alter_columns('comment', [
sa.Column('file_key', sa.Integer(), sa.ForeignKey('file.key'))
])
update_query = sa.text('update comment set file_key=:file_key where key=:key')
file_query = sa.text('select f.key from file f where f.revision_key=:revision_key and f.path=:path')
file_insert_query = sa.text('insert into file (key, revision_key, path, old_path, status, inserted, deleted) '
' values (NULL, :revision_key, :path, NULL, NULL, NULL, NULL)')
conn = op.get_bind()
countres = conn.execute('select count(*) from comment')
comments = countres.fetchone()[0]
comment_res = conn.execute('select p.name, c.number, c.status, r.key, r.number, m.file, m.key '
'from project p, change c, revision r, comment m '
'where m.revision_key=r.key and r.change_key=c.key and '
'c.project_key=p.key order by p.name')
count = 0
for (pname, cnumber, cstatus, rkey, rnumber, mfile, mkey) in comment_res.fetchall():
count += 1
sys.stdout.write('Comment %s / %s\r' % (count, comments))
sys.stdout.flush()
file_res = conn.execute(file_query, revision_key=rkey, path=mfile)
file_key = file_res.fetchone()
if not file_key:
conn.execute(file_insert_query, revision_key=rkey, path=mfile)
file_res = conn.execute(file_query, revision_key=rkey, path=mfile)
file_key = file_res.fetchone()
fkey = file_key[0]
file_res = conn.execute(update_query, file_key=fkey, key=mkey)
sqlite_drop_columns('comment', ['revision_key', 'file'])
print
def downgrade():
pass

View File

@ -95,12 +95,11 @@ message_table = Table(
comment_table = Table(
'comment', metadata,
Column('key', Integer, primary_key=True),
Column('revision_key', Integer, ForeignKey("revision.key"), index=True),
Column('file_key', Integer, ForeignKey("file.key"), index=True),
Column('account_key', Integer, ForeignKey("account.key"), index=True),
Column('id', String(255), index=True), #, unique=True, nullable=False),
Column('in_reply_to', String(255)),
Column('created', DateTime, index=True, nullable=False),
Column('file', Text, nullable=False),
Column('parent', Boolean, nullable=False),
Column('line', Integer),
Column('message', Text, nullable=False),
@ -347,15 +346,6 @@ class Revision(object):
session.flush()
return m
def createComment(self, *args, **kw):
session = Session.object_session(self)
args = [self] + list(args)
c = Comment(*args, **kw)
self.comments.append(c)
session.add(c)
session.flush()
return c
def createPendingCherryPick(self, *args, **kw):
session = Session.object_session(self)
args = [self] + list(args)
@ -372,8 +362,17 @@ class Revision(object):
self.files.append(f)
session.add(f)
session.flush()
if hasattr(self, '_file_cache'):
self._file_cache[f.path] = f
return f
def getFile(self, path):
if not hasattr(self, '_file_cache'):
self._file_cache = {}
for f in self.files:
self._file_cache[f.path] = f
return self._file_cache.get(path, None)
def getPendingMessage(self):
for m in self.messages:
if m.pending:
@ -410,13 +409,12 @@ class Message(object):
return author_name
class Comment(object):
def __init__(self, revision, id, author, in_reply_to, created, file, parent, line, message, draft=False):
self.revision_key = revision.key
def __init__(self, file, id, author, in_reply_to, created, parent, line, message, draft=False):
self.file_key = file.key
self.account_key = author.key
self.id = id
self.in_reply_to = in_reply_to
self.created = created
self.file = file
self.parent = parent
self.line = line
self.message = message
@ -489,7 +487,22 @@ class File(object):
break
post = ''.join(post)
mid = '{%s => %s}' % (self.old_path[start:0-end+1], self.path[start:0-end+1])
return pre + mid + post
if pre and post:
mid = '{%s => %s}' % (self.old_path[start:0-end+1],
self.path[start:0-end+1])
return pre + mid + post
else:
return '%s => %s' % (self.old_path, self.path)
def createComment(self, *args, **kw):
session = Session.object_session(self)
args = [self] + list(args)
c = Comment(*args, **kw)
self.comments.append(c)
session.add(c)
session.flush()
return c
mapper(Account, account_table)
mapper(Project, project_table, properties=dict(
@ -535,20 +548,22 @@ mapper(Change, change_table, properties=dict(
))
mapper(Revision, revision_table, properties=dict(
messages=relationship(Message, backref='revision'),
comments=relationship(Comment, backref='revision',
order_by=(comment_table.c.line,
comment_table.c.created)),
draft_comments=relationship(Comment,
primaryjoin=and_(revision_table.c.key==comment_table.c.revision_key,
comment_table.c.draft==True),
order_by=(comment_table.c.line,
comment_table.c.created)),
files=relationship(File, backref='revision'),
pending_cherry_picks=relationship(PendingCherryPick, backref='revision'),
))
mapper(Message, message_table, properties=dict(
author=relationship(Account)))
mapper(File, file_table)
mapper(File, file_table, properties=dict(
comments=relationship(Comment, backref='file',
order_by=(comment_table.c.line,
comment_table.c.created)),
draft_comments=relationship(Comment,
primaryjoin=and_(file_table.c.key==comment_table.c.file_key,
comment_table.c.draft==True),
order_by=(comment_table.c.line,
comment_table.c.created)),
))
mapper(Comment, comment_table, properties=dict(
author=relationship(Account)))
mapper(Label, label_table)
@ -748,6 +763,12 @@ class DatabaseSession(object):
except sqlalchemy.orm.exc.NoResultFound:
return None
def getFile(self, key):
try:
return self.session().query(File).filter_by(key=key).one()
except sqlalchemy.orm.exc.NoResultFound:
return None
def getComment(self, key):
try:
return self.session().query(Comment).filter_by(key=key).one()

View File

@ -133,6 +133,22 @@ def sqlite_drop_columns(table_name, drop_columns):
new_columns.append(col_copy)
for key in meta.tables[table_name].foreign_keys:
# If this is a single column constraint for a dropped column,
# don't copy it.
if isinstance(key.constraint.columns, sqlalchemy.sql.base.ColumnCollection):
# This is needed for SQLAlchemy >= 1.0.4
columns = [c.name for c in key.constraint.columns]
else:
# This is needed for SQLAlchemy <= 0.9.9. This is
# backwards compat code just in case someone updates
# Gertty without updating SQLAlchemy. This is simple
# enough to check and will hopefully avoid leaving the
# user's db in an inconsistent state. Remove this after
# Gertty 1.2.0.
columns = key.constraint.columns
if (len(columns)==1 and columns[0] in drop_columns):
continue
# Otherwise, recreate the constraint.
constraint = key.constraint
con_copy = constraint.copy()
new_columns.append(con_copy)

View File

@ -170,6 +170,8 @@ class DiffFile(object):
def __init__(self):
self.newname = 'Unknown File'
self.oldname = 'Unknown File'
self.old_empty = False
self.new_empty = False
self.chunks = []
self.current_chunk = None
self.old_lineno = 0
@ -389,8 +391,10 @@ class Repo(object):
# f.newname = diff_context.b_path
if diff_context.new_file:
f.oldname = 'Empty file'
f.old_empty = True
if diff_context.deleted_file:
f.newname = 'Empty file'
f.new_empty = True
files.append(f)
if diff_context.rename_from:
f.oldname = diff_context.rename_from
@ -468,6 +472,16 @@ class Repo(object):
continue
if not last_line:
raise Exception("Unhandled line: %s" % line)
if not diff_context.diff:
# There is no diff, possibly because this is simply a
# rename. Include context lines so that comments may
# appear.
newc = repo.commit(new)
blob = newc.tree[f.newname]
f.old_lineno = 1
f.new_lineno = 1
for line in blob.data_stream.read().splitlines():
f.addContextLine(line)
f.finalize()
return files
@ -479,7 +493,10 @@ class Repo(object):
f.new_lineno = 1
repo = git.Repo(self.path)
newc = repo.commit(new)
blob = newc.tree[path]
try:
blob = newc.tree[path]
except KeyError:
return None
for line in blob.data_stream.read().splitlines():
f.addContextLine(line)
f.finalize()

View File

@ -298,11 +298,13 @@ def SearchParser():
def p_file_term(p):
'''file_term : OP_FILE string'''
if p[2].startswith('^'):
p[0] = or_(func.matches(p[2], gertty.db.file_table.c.path),
func.matches(p[2], gertty.db.file_table.c.old_path))
p[0] = and_(or_(func.matches(p[2], gertty.db.file_table.c.path),
func.matches(p[2], gertty.db.file_table.c.old_path)),
gertty.db.file_table.c.status is not None)
else:
p[0] = or_(gertty.db.file_table.c.path == p[2],
gertty.db.file_table.c.old_path == p[2])
p[0] = and_(or_(gertty.db.file_table.c.path == p[2],
gertty.db.file_table.c.old_path == p[2]),
gertty.db.file_table.c.status is not None)
def p_status_term(p):
'''status_term : OP_STATUS string'''

View File

@ -618,16 +618,21 @@ class SyncChangeTask(Task):
parent_commits.add(revision.parent)
result.updateRelatedChanges(session, change)
filemap = {}
f = revision.getFile('/COMMIT_MSG')
if f is None:
f = revision.createFile('/COMMIT_MSG', None,
None, None, None)
for remote_path, remote_file in remote_revision['files'].items():
if remote_file.get('binary'):
inserted = deleted = None
else:
inserted = remote_file.get('lines_inserted', 0)
deleted = remote_file.get('lines_deleted', 0)
f = revision.createFile(remote_path, remote_file.get('status', 'M'),
remote_file.get('old_path'), inserted, deleted)
filemap[remote_path] = f
f = revision.getFile(remote_path)
if f is None:
if remote_file.get('binary'):
inserted = deleted = None
else:
inserted = remote_file.get('lines_inserted', 0)
deleted = remote_file.get('lines_deleted', 0)
f = revision.createFile(remote_path, remote_file.get('status', 'M'),
remote_file.get('old_path'),
inserted, deleted)
remote_comments_data = remote_revision['_gertty_remote_comments_data']
for remote_file, remote_comments in remote_comments_data.items():
@ -643,11 +648,12 @@ class SyncChangeTask(Task):
parent = False
if remote_comment.get('side', '') == 'PARENT':
parent = True
comment = revision.createComment(remote_comment['id'], account,
remote_comment.get('in_reply_to'),
created,
remote_file, parent, remote_comment.get('line'),
remote_comment['message'])
fileobj = revision.getFile(remote_file)
comment = fileobj.createComment(remote_comment['id'], account,
remote_comment.get('in_reply_to'),
created,
parent, remote_comment.get('line'),
remote_comment['message'])
self.log.info("Created new comment %s for revision %s in local DB.",
comment.key, revision.key)
else:
@ -1124,21 +1130,20 @@ class UploadReviewTask(Task):
for approval in change.draft_approvals:
data['labels'][approval.category] = approval.value
session.delete(approval)
if revision.draft_comments:
data['comments'] = {}
last_file = None
comment_list = []
for comment in revision.draft_comments:
if comment.file != last_file:
last_file = comment.file
comment_list = []
data['comments'][comment.file] = comment_list
d = dict(line=comment.line,
message=comment.message)
if comment.parent:
d['side'] = 'PARENT'
comment_list.append(d)
session.delete(comment)
comments = {}
for file in revision.files:
if file.draft_comments:
comment_list = []
for comment in file.draft_comments:
d = dict(line=comment.line,
message=comment.message)
if comment.parent:
d['side'] = 'PARENT'
comment_list.append(d)
session.delete(comment)
comments[file.path] = comment_list
if comments:
data['comments'] = comments
session.delete(message)
# Inside db session for rollback
sync.post('changes/%s/revisions/%s/review' % (change.id, revision.commit),

View File

@ -237,6 +237,8 @@ class RevisionRow(urwid.WidgetWrap):
total_added = 0
total_removed = 0
for rfile in revision.files:
if rfile.status is None:
continue
added = rfile.inserted or 0
removed = rfile.deleted or 0
total_added += added
@ -279,13 +281,13 @@ class RevisionRow(urwid.WidgetWrap):
def update(self, revision):
line = [('revision-name', 'Patch Set %s ' % revision.number),
('revision-commit', revision.commit)]
num_drafts = len(revision.draft_comments)
num_drafts = sum([len(f.draft_comments) for f in revision.files])
if num_drafts:
pending_message = revision.getPendingMessage()
if not pending_message:
line.append(('revision-drafts', ' (%s draft%s)' % (
num_drafts, num_drafts>1 and 's' or '')))
num_comments = len(revision.comments) - num_drafts
num_comments = sum([len(f.comments) for f in revision.files]) - num_drafts
if num_comments:
line.append(('revision-comments', ' (%s inline comment%s)' % (
num_comments, num_comments>1 and 's' or '')))

View File

@ -84,17 +84,16 @@ class PatchsetDialog(urwid.WidgetWrap):
return old, new
class LineContext(object):
def __init__(self, old_revision_key, new_revision_key,
old_revision_num, new_revision_num,
old_fn, new_fn, old_ln, new_ln):
self.old_revision_key = old_revision_key
self.new_revision_key = new_revision_key
self.old_revision_num = old_revision_num
self.new_revision_num = new_revision_num
def __init__(self, old_file_key, new_file_key,
old_fn, new_fn, old_ln, new_ln,
header=False):
self.old_file_key = old_file_key
self.new_file_key = new_file_key
self.old_fn = old_fn
self.new_fn = new_fn
self.old_ln = old_ln
self.new_ln = new_ln
self.header = header
class BaseDiffCommentEdit(urwid.Columns):
pass
@ -172,20 +171,33 @@ class BaseDiffView(urwid.WidgetWrap):
del self._w.contents[:]
with self.app.db.getSession() as session:
new_revision = session.getRevision(self.new_revision_key)
old_comments = []
new_comments = []
self.old_file_keys = {}
self.new_file_keys = {}
if self.old_revision_key is not None:
old_revision = session.getRevision(self.old_revision_key)
self.old_revision_num = old_revision.number
old_str = 'patchset %s' % self.old_revision_num
self.base_commit = old_revision.commit
old_comments = old_revision.comments
for f in old_revision.files:
old_comments += f.comments
self.old_file_keys[f.path] = f.key
show_old_commit = True
else:
old_revision = None
self.old_revision_num = None
old_str = 'base'
self.base_commit = new_revision.parent
old_comments = []
show_old_commit = False
# The old files are the same as the new files since we
# are diffing from base -> change, however, we should
# use the old file names for file lookup.
for f in new_revision.files:
if f.old_path:
self.old_file_keys[f.old_path] = f.key
else:
self.old_file_keys[f.path] = f.key
self.title = u'Diff of %s change %s from %s to patchset %s' % (
new_revision.change.project.name,
new_revision.change.number,
@ -194,19 +206,25 @@ class BaseDiffView(urwid.WidgetWrap):
self.change_key = new_revision.change.key
self.project_name = new_revision.change.project.name
self.commit = new_revision.commit
for f in new_revision.files:
new_comments += f.comments
self.new_file_keys[f.path] = f.key
comment_lists = {}
comment_filenames = set()
for comment in new_revision.comments:
for comment in new_comments:
path = comment.file.path
if comment.parent:
if old_revision: # we're not looking at the base
continue
key = 'old'
if comment.file.old_path:
path = comment.file.old_path
else:
key = 'new'
if comment.draft:
key += 'draft'
key += '-' + str(comment.line)
key += '-' + str(comment.file)
key += '-' + path
comment_list = comment_lists.get(key, [])
if comment.draft:
message = comment.message
@ -215,15 +233,16 @@ class BaseDiffView(urwid.WidgetWrap):
('comment', u': '+comment.message)]
comment_list.append((comment.key, message))
comment_lists[key] = comment_list
comment_filenames.add(comment.file)
comment_filenames.add(path)
for comment in old_comments:
if comment.parent:
continue
path = comment.file.path
key = 'old'
if comment.draft:
key += 'draft'
key += '-' + str(comment.line)
key += '-' + str(comment.file)
key += '-' + path
comment_list = comment_lists.get(key, [])
if comment.draft:
message = comment.message
@ -232,7 +251,7 @@ class BaseDiffView(urwid.WidgetWrap):
('comment', u': '+comment.message)]
comment_list.append((comment.key, message))
comment_lists[key] = comment_list
comment_filenames.add(comment.file)
comment_filenames.add(path)
repo = self.app.getRepo(self.project_name)
self._w.contents.append((self.app.header, ('pack', 1)))
self.file_reminder = self.makeFileReminder()
@ -250,7 +269,10 @@ class BaseDiffView(urwid.WidgetWrap):
# that contain the full text.
for filename in comment_filenames:
diff = repo.getFile(self.base_commit, self.commit, filename)
diffs.append(diff)
if diff:
diffs.append(diff)
else:
self.log.debug("Unable to find file %s in commit %s" % (filename, self.commit))
for i, diff in enumerate(diffs):
if i > 0:
lines.append(urwid.Text(''))
@ -297,6 +319,11 @@ class BaseDiffView(urwid.WidgetWrap):
oldnew = gitrepo.OLD
else:
oldnew = gitrepo.NEW
file_diffs = self.file_diffs[oldnew]
if path not in file_diffs:
self.log.error("Unable to display comment: %s" % key)
del comment_lists[key]
continue
diff = self.file_diffs[oldnew][path]
for chunk in diff.chunks:
if (chunk.range[oldnew][gitrepo.START] <= lineno and
@ -341,6 +368,21 @@ class BaseDiffView(urwid.WidgetWrap):
else:
chunk.button.update()
def makeContext(self, diff, old_ln, new_ln, header=False):
old_key = None
new_key = None
if not diff.old_empty:
if diff.oldname in self.old_file_keys:
old_key = self.old_file_keys[diff.oldname]
elif diff.newname in self.old_file_keys:
old_key = self.old_file_keys[diff.newname]
if not diff.new_empty:
new_key = self.new_file_keys[diff.newname]
return LineContext(
old_key, new_key,
diff.oldname, diff.newname,
old_ln, new_ln, header)
def makeLines(self, diff, lines_to_add, comment_lists):
raise NotImplementedError
@ -431,28 +473,25 @@ class BaseDiffView(urwid.WidgetWrap):
session.delete(comment)
def saveComment(self, context, text, new=True):
if (not new) and (not context.old_revision_num):
if (not new) and (not self.old_revision_num):
parent = True
revision_key = context.new_revision_key
else:
parent = False
if new:
revision_key = context.new_revision_key
else:
revision_key = context.old_revision_key
if new:
line_num = context.new_ln
filename = context.new_fn
file_key = context.new_file_key
else:
line_num = context.old_ln
filename = context.old_fn
file_key = context.old_file_key
if file_key is None:
raise Exception("Comment is not associated with a file")
with self.app.db.getSession() as session:
revision = session.getRevision(revision_key)
fileojb = session.getFile(file_key)
account = session.getAccountByUsername(self.app.config.username)
comment = revision.createComment(None, account, None,
datetime.datetime.utcnow(),
filename, parent,
line_num, text, draft=True)
comment = fileojb.createComment(None, account, None,
datetime.datetime.utcnow(),
parent,
line_num, text, draft=True)
key = comment.key
return key

View File

@ -17,7 +17,6 @@ import urwid
from gertty import keymap
from gertty.view.diff import BaseDiffComment, BaseDiffCommentEdit, BaseDiffLine
from gertty.view.diff import BaseFileHeader, BaseFileReminder, BaseDiffView
from gertty.view.diff import LineContext
LN_COL_WIDTH = 5
@ -32,20 +31,32 @@ class SideDiffCommentEdit(BaseDiffCommentEdit):
self.old = urwid.Edit(edit_text=old, multiline=True)
self.new = urwid.Edit(edit_text=new, multiline=True)
self.contents.append((urwid.Text(u''), ('given', LN_COL_WIDTH, False)))
self.contents.append((urwid.AttrMap(self.old, 'draft-comment'), ('weight', 1, False)))
if context.old_ln is not None or context.header:
self.contents.append((urwid.AttrMap(self.old, 'draft-comment'), ('weight', 1, False)))
else:
self.contents.append((urwid.Text(u''), ('weight', 1, False)))
self.contents.append((urwid.Text(u''), ('given', LN_COL_WIDTH, False)))
self.contents.append((urwid.AttrMap(self.new, 'draft-comment'), ('weight', 1, False)))
self.focus_position = 3
if context.new_ln is not None or context.header:
self.contents.append((urwid.AttrMap(self.new, 'draft-comment'), ('weight', 1, False)))
else:
self.contents.append((urwid.Text(u''), ('weight', 1, False)))
if context.new_ln is not None or context.header:
self.focus_position = 3
else:
self.focus_position = 1
def keypress(self, size, key):
r = super(SideDiffCommentEdit, self).keypress(size, key)
commands = self.app.config.keymap.getCommands(r)
if ((keymap.NEXT_SELECTABLE in commands) or
(keymap.PREV_SELECTABLE in commands)):
if self.focus_position == 3:
self.focus_position = 1
else:
self.focus_position = 3
if ((self.context.old_ln is not None and
self.context.new_ln is not None) or
self.context.header):
if self.focus_position == 3:
self.focus_position = 1
else:
self.focus_position = 3
return None
return r
@ -117,11 +128,7 @@ class SideDiffView(BaseDiffView):
def makeLines(self, diff, lines_to_add, comment_lists):
lines = []
for old, new in lines_to_add:
context = LineContext(
self.old_revision_key, self.new_revision_key,
self.old_revision_num, self.new_revision_num,
diff.oldname, diff.newname,
old[0], new[0])
context = self.makeContext(diff, old[0], new[0])
lines.append(SideDiffLine(self.app, context, old, new,
callback=self.onSelect))
# see if there are any comments for this line
@ -159,11 +166,7 @@ class SideDiffView(BaseDiffView):
return SideFileReminder()
def makeFileHeader(self, diff, comment_lists):
context = LineContext(
self.old_revision_key, self.new_revision_key,
self.old_revision_num, self.new_revision_num,
diff.oldname, diff.newname,
None, None)
context = self.makeContext(diff, None, None, header=True)
lines = []
lines.append(SideFileHeader(self.app, context, diff.oldname, diff.newname,
callback=self.onSelect))

View File

@ -18,7 +18,6 @@ import urwid
from gertty import gitrepo
from gertty.view.diff import BaseDiffCommentEdit, BaseDiffComment, BaseDiffLine
from gertty.view.diff import BaseFileHeader, BaseFileReminder, BaseDiffView
from gertty.view.diff import LineContext
LN_COL_WIDTH = 5
@ -121,11 +120,7 @@ class UnifiedDiffView(BaseDiffView):
def makeLines(self, diff, lines_to_add, comment_lists):
lines = []
for old, new in lines_to_add:
context = LineContext(
self.old_revision_key, self.new_revision_key,
self.old_revision_num, self.new_revision_num,
diff.oldname, diff.newname,
old[0], new[0])
context = self.makeContext(diff, old[0], new[0])
if context.old_ln is not None:
lines.append(UnifiedDiffLine(self.app, context, gitrepo.OLD, old, new,
callback=self.onSelect))
@ -169,11 +164,7 @@ class UnifiedDiffView(BaseDiffView):
return UnifiedFileReminder()
def makeFileHeader(self, diff, comment_lists):
context = LineContext(
self.old_revision_key, self.new_revision_key,
self.old_revision_num, self.new_revision_num,
diff.oldname, diff.newname,
None, None)
context = self.makeContext(diff, None, None, header=True)
lines = []
lines.append(UnifiedFileHeader(self.app, context, gitrepo.OLD,
diff.oldname, diff.newname,