Improve comment propagation

Reimplemented comment propagation code to make it more robust and more
capable of dealing with non-trivial situations.  Known issues fixed or
improved by this is:

* Comments against lines in files modified by a later merge or rebase in
  the review were not propagate forward properly to commits after the
  merge or rebase.  Issues raised in such circumstances would never be
  marked as addressed by later commits.

* Crash when attempting to comment lines in a version of a file that was
  introduced into the review by a merge commit or rebase but where the
  file is not included in the (equivalent) merge commit's diff because
  there were no (suspected) conflicts in the file.

* Reopening an issue against lines that are themselves modified by a
  later commit.  In that case, the issue is now left as "addressed" only
  with a new commit as the "addressed by" commit.  It's possible to
  reopen the issue again from there, eventually reaching either an open
  issue or an issue that is marked as addressed by the commit that
  actually addressed it.

* Support for writing comments via /showfile even if the file in question
  has been changed in the review.  This was previously not supported
  simply because the old comment propagation code was slightly difficult
  to reuse.

Notable changes:

* Introduced a new utility class Propagation used whenever comments are
  propagated between file versions (when initially created, when new
  commits are added to the review and when reopening an issue.)

* Dropped the column 'commit' from the table commentchainlines.  It was
  confusing since it was not included in the primary key of the table,
  and the columns that make up the primary key could have identical
  values for several different commits.  The value in the 'commit' column
  was thus just a commit (the first) where the comment existed against
  the specified lines.  It wasn't used a lot, and where it was, it was
  either wrong or inaccurate.

* Added columns 'from_addressed_by' and 'to_addressed_by' to the table
  commentchainchanges to properly record the changes to the column
  'addressed_by' in the table 'commentchains' when reopening an issue
  that ends up being addressed again by a different commit.  Previously,
  when reopening an issue, the 'addressed_by' column was always set to
  NULL so changes to it needn't be recorded explicitly.

changeset/html.py

@@ -220,7 +220,7 @@ def render(db, target, user, changeset, review=None, review_mode=None, context_l
 
         for file in changeset.files:
             if file.hasChanges() and not file.wasRemoved():
-                comment_chains = review_comment.loadCommentChains(db, review, user, file, changeset, local_comments_only=local_comments_only)
+                comment_chains = review_comment.loadCommentChains(db, review, user, file=file, changeset=changeset, local_comments_only=local_comments_only)
                 if comment_chains:
                     comment_chains_per_file[file.path] = comment_chains
 

dbschema.comments.sql

@@ -71,7 +71,9 @@ CREATE TABLE commentchainchanges
     from_state commentchainstate,
     to_state commentchainstate,
     from_last_commit INTEGER REFERENCES commits,
-    to_last_commit INTEGER REFERENCES commits );
+    to_last_commit INTEGER REFERENCES commits,
+    from_addressed_by INTEGER REFERENCES commits,
+    to_addressed_by INTEGER REFERENCES commits );
 CREATE INDEX commentchainchanges_review_uid_state ON commentchainchanges (review, uid, state);
 CREATE INDEX commentchainchanges_batch ON commentchainchanges(batch);
 CREATE INDEX commentchainchanges_chain ON commentchainchanges(chain);
@@ -86,7 +88,6 @@ CREATE TABLE commentchainlines
     uid INTEGER REFERENCES users,
     time TIMESTAMP NOT NULL DEFAULT now(),
     state commentchainlinesstate NOT NULL DEFAULT 'draft',
-    commit INTEGER REFERENCES commits,
     sha1 CHAR(40) NOT NULL,
     first_line INTEGER NOT NULL,
     last_line INTEGER NOT NULL,

dbutils/review.py

@@ -36,16 +36,19 @@ def countDraftItems(db, user, review):
     cursor.execute("SELECT count(*) FROM commentchains, comments WHERE commentchains.review=%s AND comments.chain=commentchains.id AND comments.uid=%s AND comments.state='draft'", [review.id, user.id])
     comments = cursor.fetchone()[0]
 
-    cursor.execute("""SELECT count(*) FROM commentchains, commentchainchanges
+    cursor.execute("""SELECT DISTINCT commentchains.id
+                        FROM commentchains
+                        JOIN commentchainchanges ON (commentchainchanges.chain=commentchains.id)
                        WHERE commentchains.review=%s
-                         AND commentchains.state=commentchainchanges.from_state
-                         AND commentchainchanges.chain=commentchains.id
                          AND commentchainchanges.uid=%s
                          AND commentchainchanges.state='draft'
-                         AND (commentchainchanges.from_state='addressed' OR commentchainchanges.from_state='closed')
-                         AND commentchainchanges.to_state='open'""",
+                         AND ((commentchains.state=commentchainchanges.from_state
+                           AND commentchainchanges.from_state IN ('addressed', 'closed')
+                           AND commentchainchanges.to_state='open')
+                          OR (commentchainchanges.from_addressed_by IS NOT NULL
+                           AND commentchainchanges.to_addressed_by IS NOT NULL))""",
                    [review.id, user.id])
-    reopened = cursor.fetchone()[0]
+    reopened = len(cursor.fetchall())
 
     cursor.execute("""SELECT count(*) FROM commentchains, commentchainchanges
                        WHERE commentchains.review=%s
@@ -121,6 +124,53 @@ def __str__(self):
             if issues: return "%s and %s" % (progress, issues)
             else: return progress
 
+class ReviewRebase(object):
+    def __init__(self, review, old_head, new_head, old_upstream, new_upstream, user):
+        self.review = review
+        self.old_head = old_head
+        self.new_head = new_head
+        self.old_upstream = old_upstream
+        self.new_upstream = new_upstream
+        self.user = user
+
+class ReviewRebases(list):
+    def __init__(self, db, review):
+        import gitutils
+        from dbutils import User
+
+        self.__old_head_map = {}
+        self.__new_head_map = {}
+
+        cursor = db.cursor()
+        cursor.execute("""SELECT old_head, new_head, old_upstream, new_upstream, uid
+                            FROM reviewrebases
+                           WHERE review=%s
+                             AND new_head IS NOT NULL""",
+                       (review.id,))
+
+        for old_head_id, new_head_id, old_upstream_id, new_upstream_id, user_id in cursor:
+            old_head = gitutils.Commit.fromId(db, review.repository, old_head_id)
+            new_head = gitutils.Commit.fromId(db, review.repository, new_head_id)
+
+            if old_upstream_id is not None and new_upstream_id is not None:
+                old_upstream = gitutils.Commit.fromId(db, review.repository, old_upstream_id)
+                new_upstream = gitutils.Commit.fromId(db, review.repository, new_upstream_id)
+            else:
+                old_upstream = new_upstream = None
+
+            user = User.fromId(db, user_id)
+            rebase = ReviewRebase(review, old_head, new_head, old_upstream, new_upstream, user)
+
+            self.append(rebase)
+            self.__old_head_map[old_head] = rebase
+            self.__new_head_map[new_head] = rebase
+
+    def fromOldHead(self, commit):
+        return self.__old_head_map.get(commit)
+
+    def fromNewHead(self, commit):
+        return self.__new_head_map.get(commit)
+
 class Review(object):
     def __init__(self, review_id, owners, review_type, branch, state, serial, summary, description, applyfilters, applyparentfilters):
         self.id = review_id
@@ -185,7 +235,29 @@ def getReviewState(self, db):
 
         return ReviewState(self, self.accepted(db), pending, reviewed, issues)
 
-    def containsCommit(self, db, commit):
+    def getReviewRebases(self, db):
+        return ReviewRebases(db, self)
+
+    def getCommitSet(self, db):
+        import gitutils
+        import log.commitset
+
+        cursor = db.cursor()
+        cursor.execute("""SELECT DISTINCT commits.id, commits.sha1
+                            FROM commits
+                            JOIN changesets ON (changesets.child=commits.id)
+                            JOIN reviewchangesets ON (reviewchangesets.changeset=changesets.id)
+                           WHERE reviewchangesets.review=%s""",
+                       (self.id,))
+
+        commits = []
+
+        for commit_id, commit_sha1 in cursor:
+            commits.append(gitutils.Commit.fromSHA1(db, self.repository, commit_sha1, commit_id))
+
+        return log.commitset.CommitSet(commits)
+
+    def containsCommit(self, db, commit, include_head_and_tails=False):
         import gitutils
 
         commit_id = None
@@ -196,8 +268,10 @@ def containsCommit(self, db, commit):
             else: commit_sha1 = commit.sha1
         elif isinstance(commit, str):
             commit_sha1 = self.repository.revparse(commit)
+            commit = None
         elif isinstance(commit, int):
             commit_id = commit
+            commit = None
         else:
             raise TypeError
 
@@ -219,7 +293,26 @@ def containsCommit(self, db, commit):
                                  AND commits.sha1=%s""",
                            (self.id, commit_sha1))
 
-        return cursor.fetchone() is not None
+        if cursor.fetchone() is not None:
+            return True
+
+        if include_head_and_tails:
+            head_and_tails = set([self.branch.head])
+
+            commitset = self.getCommitSet(db)
+
+            if commitset:
+                head_and_tails |= commitset.getTails()
+
+            if commit_sha1 is None:
+                if commit is None:
+                    commit = gitutils.Commit.fromId(db, self.repository, commit_id)
+                commit_sha1 = commit.sha1
+
+            if commit_sha1 in head_and_tails:
+                return True
+
+        return False
 
     def getJS(self):
         return "var review = critic.review = { id: %d, branch: { id: %d, name: %r }, owners: [ %s ], serial: %d };" % (self.id, self.branch.id, self.branch.name, ", ".join(owner.getJSConstructor() for owner in self.owners), self.serial)

gitutils.py

@@ -629,12 +629,16 @@ def isAncestorOf(self, other):
 
         return self.repository.mergebase([self.sha1, other_sha1]) == self.sha1
 
-    def getFileSHA1(self, path):
+    def getFileEntry(self, path):
         try:
-            tree = Tree.fromPath(self, os.path.dirname(path))
-            return tree[os.path.basename(path)].sha1
-        except KeyError:
+            tree = Tree.fromPath(self, "/" + os.path.dirname(path).lstrip("/"))
+        except GitCommandError:
             return None
+        return tree.get(os.path.basename(path))
+
+    def getFileSHA1(self, path):
+        entry = self.getFileEntry(path)
+        return entry.sha1 if entry else None
 
 RE_LSTREE_LINE = re.compile("^([0-9]{6}) (blob|tree|commit) ([0-9a-f]{40})  *([0-9]+|-)\t(.*)$")
 

installation/migrations/dbschema.altertable.commentchainchanges.addressed_by.py

@@ -0,0 +1,54 @@
+# -*- mode: python; encoding: utf-8 -*-
+#
+# Copyright 2012 Jens Lindström, Opera Software ASA
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License.  You may obtain a copy of
+# the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations under
+# the License.
+
+import sys
+import psycopg2
+import json
+import argparse
+import os
+
+parser = argparse.ArgumentParser()
+parser.add_argument("--uid", type=int)
+parser.add_argument("--gid", type=int)
+
+arguments = parser.parse_args()
+
+os.setgid(arguments.gid)
+os.setuid(arguments.uid)
+
+data = json.load(sys.stdin)
+
+db = psycopg2.connect(database="critic")
+cursor = db.cursor()
+
+try:
+    # Make sure the columns don't already exist.
+    cursor.execute("SELECT from_addressed_by, to_addressed_by FROM commentchainchanges")
+
+    # Above statement should have thrown a psycopg2.ProgrammingError, but it
+    # didn't, so just exit.
+    sys.exit(0)
+except psycopg2.ProgrammingError:
+    db.rollback()
+except:
+    raise
+
+cursor.execute("""ALTER TABLE commentchainchanges
+                          ADD from_addressed_by INTEGER REFERENCES commits,
+                          ADD to_addressed_by INTEGER REFERENCES commits""")
+
+db.commit()
+db.close()

installation/migrations/dbschema.altertable.commentchainlines.drop.commit.py

@@ -0,0 +1,47 @@
+# -*- mode: python; encoding: utf-8 -*-
+#
+# Copyright 2012 Jens Lindström, Opera Software ASA
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License.  You may obtain a copy of
+# the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+# License for the specific language governing permissions and limitations under
+# the License.
+
+import sys
+import psycopg2
+import json
+import argparse
+import os
+
+parser = argparse.ArgumentParser()
+parser.add_argument("--uid", type=int)
+parser.add_argument("--gid", type=int)
+
+arguments = parser.parse_args()
+
+os.setgid(arguments.gid)
+os.setuid(arguments.uid)
+
+data = json.load(sys.stdin)
+
+db = psycopg2.connect(database="critic")
+cursor = db.cursor()
+
+try:
+    # Check if the 'commit' column already doesn't exist.
+    cursor.execute("SELECT commit FROM commentchainlines")
+except psycopg2.ProgrammingError:
+    # Seems it doesn't, so just exit.
+    sys.exit(0)
+
+cursor.execute("""ALTER TABLE commentchainlines DROP commit""")
+
+db.commit()
+db.close()

log/commitset.py

@@ -174,10 +174,12 @@ def filtered(self, commits):
         return CommitSet(filtered)
 
     def without(self, commits):
-        """Return a copy of this commit set without 'commit' and all any ancestors of
-'commit' that don't have other descendants in the commit set."""
+        """
+        Return a copy of this commit set without 'commit' and any ancestors of
+        'commit' that don't have other descendants in the commit set.
+        """
 
-        pending = set(commits)
+        pending = set(filter(None, (self.__commits.get(str(commit)) for commit in commits)))
         commits = self.__commits.copy()
         children = self.__children.copy()
 

operation/createcomment.py

@@ -22,14 +22,16 @@
 class ValidateCommentChain(Operation):
     def __init__(self):
         Operation.__init__(self, { "review_id": int,
+                                   "origin": set(["old", "new"]),
+                                   "parent_id": int,
+                                   "child_id": int,
                                    "file_id": int,
-                                   "sha1": str,
                                    "offset": int,
                                    "count": int })
 
-    def process(self, db, user, review_id, file_id, sha1, offset, count):
+    def process(self, db, user, review_id, origin, parent_id, child_id, file_id, offset, count):
         review = dbutils.Review.fromId(db, review_id)
-        verdict, data = validateCommentChain(db, review, file_id, sha1, offset, count)
+        verdict, data = validateCommentChain(db, review, origin, parent_id, child_id, file_id, offset, count)
         return OperationResult(verdict=verdict, **data)
 
 class CreateCommentChain(Operation):

operation/draftchanges.py

@@ -312,6 +312,22 @@ def process(self, db, user, review_id, remark=None):
 
         profiler.check("commentchainchanges reject type changes")
 
+        # Reject all draft comment chain changes where the affected comment chain
+        # addressed_by isn't what it was in when the change was drafted.
+        cursor.execute("""UPDATE commentchainchanges
+                             SET state='rejected',
+                                 time=now()
+                            FROM commentchains
+                           WHERE commentchains.review=%s
+                             AND commentchainchanges.chain=commentchains.id
+                             AND commentchainchanges.uid=%s
+                             AND commentchainchanges.state='draft'
+                             AND commentchainchanges.from_addressed_by IS NOT NULL
+                             AND commentchainchanges.from_addressed_by!=commentchains.addressed_by""",
+                       (review.id, user.id))
+
+        profiler.check("commentchainchanges reject addressed_by changes")
+
         # Then perform the remaining draft comment chain changes by updating the
         # state of the corresponding comment chain.
 
@@ -346,6 +362,19 @@ def process(self, db, user, review_id, remark=None):
 
         profiler.check("commentchains reopen")
 
+        # Perform addressed->addressed changes, i.e. updating 'addressed_by'.
+        cursor.execute("""UPDATE commentchains
+                             SET addressed_by=commentchainchanges.to_addressed_by
+                            FROM commentchainchanges
+                           WHERE commentchains.review=%s
+                             AND commentchainchanges.chain=commentchains.id
+                             AND commentchainchanges.uid=%s
+                             AND commentchainchanges.state='draft'
+                             AND commentchainchanges.to_addressed_by IS NOT NULL""",
+                       (review.id, user.id))
+
+        profiler.check("commentchains reopen (partial)")
+
         # Perform type changes.
         cursor.execute("""UPDATE commentchains
                              SET type=commentchainchanges.to_type