Save blame info to lint messages

Test Plan:
Applied the patch.
Looked at blame and plain blame of SVN and Git file.
Ran the lint saver.
Looked at lint messages list.
/diffusion/lint/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D5218

resources/sql/patches/20130304.lintauthor.sql

@@ -0,0 +1,3 @@
+ALTER TABLE `{$NAMESPACE}_repository`.`repository_lintmessage`
+  ADD authorPHID varchar(64) COLLATE utf8_bin AFTER line,
+  ADD INDEX key_author (authorPHID);

scripts/repository/save_lint.php

@@ -39,6 +39,10 @@ $args = id(new PhutilArgumentParser($argv))
       'default' => 256,
       'help' => "Number of paths passed to `arc` at once.",
     ),
+    array(
+      'name' => 'blame',
+      'help' => "Assign lint errors to authors who last modified the line.",
+    ),
   ));
 
 echo "Saving lint errors to database...\n";
@@ -48,6 +52,7 @@ $count = id(new DiffusionLintSaveRunner())
   ->setArc($args->getArg('arc'))
   ->setSeverity($args->getArg('severity'))
   ->setChunkSize($args->getArg('chunk-size'))
+  ->setNeedsBlame($args->getArg('blame'))
   ->run('.');
 
 echo "\nProcessed {$count} files.\n";

src/applications/diffusion/DiffusionLintSaveRunner.php

@@ -5,6 +5,7 @@ final class DiffusionLintSaveRunner {
   private $severity = ArcanistLintSeverity::SEVERITY_ADVICE;
   private $all = false;
   private $chunkSize = 256;
+  private $needsBlame = false;
 
   private $svnRoot;
   private $lintCommit;
@@ -12,6 +13,7 @@ final class DiffusionLintSaveRunner {
   private $conn;
   private $deletes = array();
   private $inserts = array();
+  private $blame = array();
 
 
   public function setArc($path) {
@@ -34,6 +36,11 @@ final class DiffusionLintSaveRunner {
     return $this;
   }
 
+  public function setNeedsBlame($boolean) {
+    $this->needsBlame = $boolean;
+    return $this;
+  }
+
 
   public function run($dir) {
     $working_copy = ArcanistWorkingCopyIdentity::newFromPath($dir);
@@ -98,8 +105,6 @@ final class DiffusionLintSaveRunner {
       $all_files = $api->getAllFiles();
     }
 
-    $this->deletes = array();
-    $this->inserts = array();
     $count = 0;
 
     $files = array();
@@ -123,9 +128,16 @@ final class DiffusionLintSaveRunner {
 
     $this->runArcLint($files);
     $this->saveLintMessages();
-    $this->branch->setLintCommit($api->getUnderlyingWorkingCopyRevision());
+
+    $this->lintCommit = $api->getUnderlyingWorkingCopyRevision();
+    $this->branch->setLintCommit($this->lintCommit);
     $this->branch->save();
 
+    if ($this->blame) {
+      $this->blameAuthors();
+      $this->blame = array();
+    }
+
     return $count;
   }
 
@@ -156,22 +168,26 @@ final class DiffusionLintSaveRunner {
           }
 
           foreach ($messages as $message) {
+            $line = idx($message, 'line', 0);
+
             $this->inserts[] = qsprintf(
               $this->conn,
               '(%d, %s, %d, %s, %s, %s, %s)',
               $this->branch->getID(),
               $this->svnRoot.'/'.$path,
-              idx($message, 'line', 0),
+              $line,
               idx($message, 'code', ''),
               idx($message, 'severity', ''),
               idx($message, 'name', ''),
               idx($message, 'description', ''));
+
+            if ($line && $this->needsBlame) {
+              $this->blame[$path][$line] = true;
+            }
           }
 
           if (count($this->deletes) >= 1024 || count($this->inserts) >= 256) {
-            $this->saveLintMessages($this->branch);
-            $this->deletes = array();
-            $this->inserts = array();
+            $this->saveLintMessages();
           }
         }
       }
@@ -205,6 +221,67 @@ final class DiffusionLintSaveRunner {
     }
 
     $this->conn->saveTransaction();
+
+    $this->deletes = array();
+    $this->inserts = array();
+  }
+
+
+  private function blameAuthors() {
+    $repository = id(new PhabricatorRepository())->load(
+      $this->branch->getRepositoryID());
+
+    $queries = array();
+    $futures = array();
+    foreach ($this->blame as $path => $lines) {
+      $drequest = DiffusionRequest::newFromDictionary(array(
+        'repository' => $repository,
+        'branch' => $this->branch->getName(),
+        'path' => $path,
+        'commit' => $this->lintCommit,
+      ));
+      $query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest)
+        ->setNeedsBlame(true);
+      $queries[$path] = $query;
+      $futures[$path] = $query->getFileContentFuture();
+    }
+
+    $authors = array();
+
+    foreach (Futures($futures)->limit(8) as $path => $future) {
+      $queries[$path]->loadFileContentFromFuture($future);
+      list(, $rev_list, $blame_dict) = $queries[$path]->getBlameData();
+      foreach (array_keys($this->blame[$path]) as $line) {
+        $commit_identifier = $rev_list[$line - 1];
+        $author = idx($blame_dict[$commit_identifier], 'authorPHID');
+        if ($author) {
+          $authors[$author][$path][] = $line;
+        }
+      }
+    }
+
+    if ($authors) {
+      $this->conn->openTransaction();
+
+      foreach ($authors as $author => $paths) {
+        $where = array();
+        foreach ($paths as $path => $lines) {
+          $where[] = qsprintf(
+            $this->conn,
+            '(path = %s AND line IN (%Ld))',
+            $this->svnRoot.'/'.$path,
+            $lines);
+        }
+        queryfx(
+          $this->conn,
+          'UPDATE %T SET authorPHID = %s WHERE %Q',
+          PhabricatorRepository::TABLE_LINTMESSAGE,
+          $author,
+          implode(' OR ', $where));
+      }
+
+      $this->conn->saveTransaction();
+    }
   }
 
 }

src/applications/diffusion/controller/DiffusionBrowseFileController.php

@@ -202,11 +202,7 @@ final class DiffusionBrowseFileController extends DiffusionController {
         $rows = array();
         foreach ($text_list as $k => $line) {
           $rev = $rev_list[$k];
-          if (isset($blame_dict[$rev]['handle'])) {
-            $author = $blame_dict[$rev]['handle']->getName();
-          } else {
           $author = $blame_dict[$rev]['author'];
-          }
           $rows[] =
             sprintf("%-10s %-20s %s", substr($rev, 0, 7), $author, $line);
         }
@@ -430,11 +426,13 @@ final class DiffusionBrowseFileController extends DiffusionController {
     DiffusionFileContentQuery $file_query,
     $selected) {
 
+    $handles = array();
     if ($blame_dict) {
       $epoch_list  = ipull(ifilter($blame_dict, 'epoch'), 'epoch');
       $epoch_min   = min($epoch_list);
       $epoch_max   = max($epoch_list);
       $epoch_range = ($epoch_max - $epoch_min) + 1;
+      $handles     = $this->loadViewerHandles(ipull($blame_dict, 'authorPHID'));
     }
 
     $line_arr = array();
@@ -499,8 +497,9 @@ final class DiffusionBrowseFileController extends DiffusionController {
           $display_line['color'] = $color;
           $display_line['commit'] = $rev;
 
-          if (isset($blame['handle'])) {
-            $author_link = $blame['handle']->renderLink();
+          $author_phid = idx($blame, 'authorPHID');
+          if ($author_phid && $handles[$author_phid]) {
+            $author_link = $handles[$author_phid]->renderLink();
           } else {
             $author_link = phutil_tag(
               'span',

src/applications/diffusion/controller/DiffusionLintController.php

@@ -169,18 +169,19 @@ final class DiffusionLintController extends DiffusionController {
     }
 
     if ($owner_phids) {
+      $or = array();
+      $or[] = qsprintf($conn, 'authorPHID IN (%Ls)', $owner_phids);
+
+      $paths = array();
       $packages = id(new PhabricatorOwnersOwner())
         ->loadAllWhere('userPHID IN (%Ls)', $owner_phids);
-      if (!$packages) {
-        return array();
-      }
-
-      $paths = id(new PhabricatorOwnersPath())
-        ->loadAllWhere('packageID IN (%Ld)', array_keys($packages));
-      if (!$paths) {
-        return array();
+      if ($packages) {
+        $paths = id(new PhabricatorOwnersPath())->loadAllWhere(
+          'packageID IN (%Ld)',
+          mpull($packages, 'getPackageID'));
       }
 
+      if ($paths) {
         $repositories = id(new PhabricatorRepository())->loadAllWhere(
           'phid IN (%Ls)',
           array_unique(mpull($paths, 'getRepositoryPHID')));
@@ -190,8 +191,8 @@ final class DiffusionLintController extends DiffusionController {
           'repositoryID IN (%Ld)',
           $repositories);
         $branches = mgroup($branches, 'getRepositoryID');
+      }
 
-      $or = array();
       foreach ($paths as $path) {
         $branch = idx($branches, $repositories[$path->getRepositoryPHID()]);
         if ($branch) {
@@ -207,9 +208,6 @@ final class DiffusionLintController extends DiffusionController {
           }
         }
       }
-      if (!$or) {
-        return array();
-      }
       $where[] = '('.implode(' OR ', $or).')';
     }
 

src/applications/diffusion/controller/DiffusionLintDetailsController.php

@@ -11,6 +11,8 @@ final class DiffusionLintDetailsController extends DiffusionController {
     $messages = $this->loadLintMessages($branch, $limit, $offset);
     $is_dir = (substr('/'.$drequest->getPath(), -1) == '/');
 
+    $authors = $this->loadViewerHandles(ipull($messages, 'authorPHID'));
+
     $rows = array();
     foreach ($messages as $message) {
       $path = hsprintf(
@@ -31,9 +33,15 @@ final class DiffusionLintDetailsController extends DiffusionController {
         )),
         $message['line']);
 
+      $author = $message['authorPHID'];
+      if ($author && $authors[$author]) {
+        $author = $authors[$author]->renderLink();
+      }
+
       $rows[] = array(
         $path,
         $line,
+        $author,
         ArcanistLintSeverity::getStringForSeverity($message['severity']),
         $message['name'],
         $message['description'],
@@ -44,11 +52,12 @@ final class DiffusionLintDetailsController extends DiffusionController {
       ->setHeaders(array(
         'Path',
         'Line',
+        'Author',
         'Severity',
         'Name',
         'Description',
       ))
-      ->setColumnClasses(array('', 'n', '', '', ''))
+      ->setColumnClasses(array('', 'n'))
       ->setColumnVisibility(array($is_dir));
 
     $content = array();

src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php

@@ -11,8 +11,11 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery {
     return parent::newQueryObject(__CLASS__, $request);
   }
 
-  final public function loadFileContent() {
-    $this->fileContent = $this->executeQuery();
+  abstract public function getFileContentFuture();
+  abstract protected function executeQueryFromFuture(Future $future);
+
+  final public function loadFileContentFromFuture(Future $future) {
+    $this->fileContent = $this->executeQueryFromFuture($future);
 
     $repository = $this->getRequest()->getRepository();
     $try_encoding = $repository->getDetail('encoding');
@@ -25,6 +28,14 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery {
     return $this->fileContent;
   }
 
+  final protected function executeQuery() {
+    return $this->loadFileContentFromFuture($this->getFileContentFuture());
+  }
+
+  final public function loadFileContent() {
+    return $this->executeQuery();
+  }
+
   final public function getRawData() {
     return $this->fileContent->getCorpus();
   }
@@ -72,30 +83,22 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery {
           $commit->getEpoch();
       }
 
-      $commits_data = array();
       if ($commits) {
         $commits_data = id(new PhabricatorRepositoryCommitData())->loadAllWhere(
           'commitID IN (%Ls)',
           mpull($commits, 'getID'));
-      }
-
-      $phids = array();
-      foreach ($commits_data as $data) {
-        $phids[] = $data->getCommitDetail('authorPHID');
-      }
-
-      $loader = new PhabricatorObjectHandleData(array_unique($phids));
-      $loader->setViewer($this->viewer);
-      $handles = $loader->loadHandles();
 
         foreach ($commits_data as $data) {
-        if ($data->getCommitDetail('authorPHID')) {
-          $commit_identifier =
-            $commits[$data->getCommitID()]->getCommitIdentifier();
-          $blame_dict[$commit_identifier]['handle'] =
-            $handles[$data->getCommitDetail('authorPHID')];
+          $author_phid = $data->getCommitDetail('authorPHID');
+          if (!$author_phid) {
+            continue;
+          }
+          $commit = $commits[$data->getCommitID()];
+          $commit_identifier = $commit->getCommitIdentifier();
+          $blame_dict[$commit_identifier]['authorPHID'] = $author_phid;
         }
       }
+
    }
 
     return array($text_list, $rev_list, $blame_dict);

src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php

@@ -2,7 +2,7 @@
 
 final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery {
 
-  protected function executeQuery() {
+  public function getFileContentFuture() {
     $drequest = $this->getRequest();
 
     $repository = $drequest->getRepository();
@@ -10,16 +10,20 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery {
     $commit = $drequest->getCommit();
 
     if ($this->getNeedsBlame()) {
-      list($corpus) = $repository->execxLocalCommand(
+      return $repository->getLocalCommandFuture(
         '--no-pager blame -c -l --date=short %s -- %s',
         $commit,
         $path);
     } else {
-      list($corpus) = $repository->execxLocalCommand(
+      return $repository->getLocalCommandFuture(
         'cat-file blob %s:%s',
         $commit,
         $path);
     }
+  }
+
+  protected function executeQueryFromFuture(Future $future) {
+    list($corpus) = $future->resolvex();
 
     $file_content = new DiffusionFileContent();
     $file_content->setCorpus($corpus);
@@ -27,7 +31,6 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery {
     return $file_content;
   }
 
-
   protected function tokenizeLine($line) {
     $m = array();
     // sample lines:

src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php

@@ -3,7 +3,7 @@
 final class DiffusionMercurialFileContentQuery
   extends DiffusionFileContentQuery {
 
-  protected function executeQuery() {
+  public function getFileContentFuture() {
     $drequest = $this->getRequest();
 
     $repository = $drequest->getRepository();
@@ -13,16 +13,20 @@ final class DiffusionMercurialFileContentQuery
     if ($this->getNeedsBlame()) {
       // NOTE: We're using "--number" instead of "--changeset" because there is
       // no way to get "--changeset" to show us the full commit hashes.
-      list($corpus) = $repository->execxLocalCommand(
+      return $repository->getLocalCommandFuture(
         'annotate --user --number --rev %s -- %s',
         $commit,
         $path);
     } else {
-      list($corpus) = $repository->execxLocalCommand(
+      return $repository->getLocalCommandFuture(
         'cat --rev %s -- %s',
         $commit,
         $path);
     }
+  }
+
+  protected function executeQueryFromFuture(Future $future) {
+    list($corpus) = $future->resolvex();
 
     $file_content = new DiffusionFileContent();
     $file_content->setCorpus($corpus);

src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php

@@ -2,7 +2,7 @@
 
 final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery {
 
-  protected function executeQuery() {
+  public function getFileContentFuture() {
     $drequest = $this->getRequest();
 
     $repository = $drequest->getRepository();
@@ -11,13 +11,17 @@ final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery {
 
     $remote_uri = $repository->getRemoteURI();
 
-    try {
-      list($corpus) = $repository->execxRemoteCommand(
+    return $repository->getRemoteCommandFuture(
       '%C %s%s@%s',
       $this->getNeedsBlame() ? 'blame --force' : 'cat',
       $remote_uri,
       phutil_escape_uri($path),
       $commit);
+  }
+
+  protected function executeQueryFromFuture(Future $future) {
+    try {
+      list($corpus) = $future->resolvex();
     } catch (CommandException $ex) {
       $stderr = $ex->getStdErr();
       if (preg_match('/path not found$/', trim($stderr))) {

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

@@ -1161,6 +1161,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
         'type'    => 'sql',
         'name'    => $this->getPatchPath('20131302.maniphestvalue.sql'),
       ),
+      '20130304.lintauthor.sql' => array(
+        'type'    => 'sql',
+        'name'    => $this->getPatchPath('20130304.lintauthor.sql'),
+      ),
     );
   }