From 01a20c0480ff4d4cc782b378713efbd4cc9ced4a Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 19 Mar 2011 14:38:50 -0700 Subject: [PATCH] Fix various parsing bugs in Differential. --- resources/sql/patches/008.repoopt.sql | 5 ++++ .../browse/DiffusionBrowseController.php | 24 ++++++++++++------- .../commit/DiffusionCommitController.php | 6 ++++- .../browse/base/DiffusionBrowseQuery.php | 1 + .../base/DiffusionPathChangeQuery.php | 3 ++- .../request/base/DiffusionRequest.php | 4 ++++ .../DiffusionCommitChangeTableView.php | 24 +++++++++++++++---- ...rRepositorySvnCommitChangeParserWorker.php | 24 ++++++++++++++----- 8 files changed, 70 insertions(+), 21 deletions(-) create mode 100644 resources/sql/patches/008.repoopt.sql diff --git a/resources/sql/patches/008.repoopt.sql b/resources/sql/patches/008.repoopt.sql new file mode 100644 index 0000000000..d20b6084b5 --- /dev/null +++ b/resources/sql/patches/008.repoopt.sql @@ -0,0 +1,5 @@ +ALTER TABLE phabricator_repository.repository_filesystem DROP PRIMARY KEY; +ALTER TABLE phabricator_repository.repository_filesystem + DROP KEY repositoryID_2; +ALTER TABLE phabricator_repository.repository_filesystem + ADD PRIMARY KEY (repositoryID, parentID, pathID, svnCommit); diff --git a/src/applications/diffusion/controller/browse/DiffusionBrowseController.php b/src/applications/diffusion/controller/browse/DiffusionBrowseController.php index 44074b0561..e1a1f4bfc8 100644 --- a/src/applications/diffusion/controller/browse/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/browse/DiffusionBrowseController.php @@ -35,6 +35,16 @@ class DiffusionBrowseController extends DiffusionController { if (!$results) { + // TODO: Format all these commits into nice VCS-agnostic links, and + // below. + $commit = $drequest->getCommit(); + $callsign = $drequest->getRepository()->getCallsign(); + if ($commit) { + $commit = "r{$callsign}{$commit}"; + } else { + $commit = 'HEAD'; + } + switch ($browse_query->getReasonForEmptyResultSet()) { case DiffusionBrowseQuery::REASON_IS_NONEXISTENT: $title = 'Path Does Not Exist'; @@ -43,19 +53,15 @@ class DiffusionBrowseController extends DiffusionController { $body = "This path does not exist anywhere."; $severity = AphrontErrorView::SEVERITY_ERROR; break; + case DiffusionBrowseQuery::REASON_IS_EMPTY: + $title = 'Empty Directory'; + $body = "This path was an empty directory at {$commit}.\n"; + $severity = AphrontErrorView::SEVERITY_NOTICE; + break; case DiffusionBrowseQuery::REASON_IS_DELETED: - // TODO: Format all these commits into nice VCS-agnostic links. - $commit = $drequest->getCommit(); $deleted = $browse_query->getDeletedAtCommit(); $existed = $browse_query->getExistedAtCommit(); - $callsign = $drequest->getRepository()->getCallsign(); - - if ($commit) { - $commit = "r{$callsign}{$commit}"; - } else { - $commit = 'HEAD'; - } $deleted = "r{$callsign}{$deleted}"; $existed = "r{$callsign}{$existed}"; diff --git a/src/applications/diffusion/controller/commit/DiffusionCommitController.php b/src/applications/diffusion/controller/commit/DiffusionCommitController.php index 32bb7a6d2f..11a754c9ec 100644 --- a/src/applications/diffusion/controller/commit/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/commit/DiffusionCommitController.php @@ -66,8 +66,12 @@ class DiffusionCommitController extends DiffusionController { $change_table->setDiffusionRequest($drequest); $change_table->setPathChanges($changes); + // TODO: Large number of modified files check. + + $count = number_format(count($changes)); + $change_panel = new AphrontPanelView(); - $change_panel->setHeader('Changes'); + $change_panel->setHeader("Changes ({$count})"); $change_panel->appendChild($change_table); $content[] = $change_panel; diff --git a/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php b/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php index b0b9cfd29b..c570585357 100644 --- a/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php +++ b/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php @@ -28,6 +28,7 @@ abstract class DiffusionBrowseQuery { const REASON_IS_DELETED = 'is-deleted'; const REASON_IS_NONEXISTENT = 'nonexistent'; const REASON_BAD_COMMIT = 'bad-commit'; + const REASON_IS_EMPTY = 'empty'; final private function __construct() { // diff --git a/src/applications/diffusion/query/pathchange/base/DiffusionPathChangeQuery.php b/src/applications/diffusion/query/pathchange/base/DiffusionPathChangeQuery.php index 5cd79d2b27..ca7789b538 100644 --- a/src/applications/diffusion/query/pathchange/base/DiffusionPathChangeQuery.php +++ b/src/applications/diffusion/query/pathchange/base/DiffusionPathChangeQuery.php @@ -53,7 +53,7 @@ class DiffusionPathChangeQuery { FROM %T c LEFT JOIN %T p ON c.pathID = p.id LEFT JOIN %T t on c.targetPathID = t.id - WHERE c.commitID = %d', + WHERE c.commitID = %d AND isDirect = 1', PhabricatorRepository::TABLE_PATHCHANGE, PhabricatorRepository::TABLE_PATH, PhabricatorRepository::TABLE_PATH, @@ -61,6 +61,7 @@ class DiffusionPathChangeQuery { $changes = array(); + $raw_changes = isort($raw_changes, 'pathName'); foreach ($raw_changes as $raw_change) { $type = $raw_change['changeType']; diff --git a/src/applications/diffusion/request/base/DiffusionRequest.php b/src/applications/diffusion/request/base/DiffusionRequest.php index d3758d36fe..064996de1c 100644 --- a/src/applications/diffusion/request/base/DiffusionRequest.php +++ b/src/applications/diffusion/request/base/DiffusionRequest.php @@ -120,6 +120,10 @@ class DiffusionRequest { $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere( 'commitID = %d', $commit->getID()); + if (!$data) { + $data = new PhabricatorRepositoryCommitData(); + $data->setCommitMessage('(This commit has not fully parsed yet.)'); + } $this->repositoryCommitData = $data; } return $this->repositoryCommitData; diff --git a/src/applications/diffusion/view/commitchangetable/DiffusionCommitChangeTableView.php b/src/applications/diffusion/view/commitchangetable/DiffusionCommitChangeTableView.php index 6b8d90c8e8..bfbf911e3a 100644 --- a/src/applications/diffusion/view/commitchangetable/DiffusionCommitChangeTableView.php +++ b/src/applications/diffusion/view/commitchangetable/DiffusionCommitChangeTableView.php @@ -27,19 +27,35 @@ final class DiffusionCommitChangeTableView extends DiffusionView { public function render() { $rows = array(); + + // TODO: Experiment with path stack rendering. + + // TODO: Copy Away and Move Away are rendered junkily still. + foreach ($this->pathChanges as $change) { $change_verb = DifferentialChangeType::getFullNameForChangeType( $change->getChangeType()); + $suffix = null; + if ($change->getFileType() == DifferentialChangeType::FILE_DIRECTORY) { + $suffix = '/'; + } + + $path = $change->getPath(); + $hash = substr(sha1($path), 0, 7); + $rows[] = array( $this->linkHistory($change->getPath()), $this->linkBrowse($change->getPath()), $this->linkChange( - $change->getPath(), + $change->getChangeType(), + $change->getFileType()), + phutil_render_tag( + 'a', array( - 'text' => $change_verb, - )), - phutil_escape_html($change->getPath()), + 'href' => '#'.$hash, + ), + phutil_escape_html($path).$suffix), ); } diff --git a/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php index 5ce4d1ce94..c35f5a4121 100644 --- a/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/svn/PhabricatorRepositorySvnCommitChangeParserWorker.php @@ -203,12 +203,17 @@ class PhabricatorRepositorySvnCommitChangeParserWorker $source_file_type = $this->lookupPathFileType( $repository, - $path, + $copy_from, array( 'rawPath' => $copy_from, 'rawCommit' => $copy_rev, )); + if ($source_file_type == DifferentialChangeType::FILE_DELETED) { + throw new Exception( + "Something is wrong; source of a copy must exist."); + } + if ($source_file_type != DifferentialChangeType::FILE_DIRECTORY) { if (isset($raw_paths[$copy_from])) { break; @@ -243,7 +248,7 @@ class PhabricatorRepositorySvnCommitChangeParserWorker 'rawPath' => $full_to, 'rawTargetPath' => $full_from, 'rawTargetCommit' => $copy_rev, - 'rawDirect' => true, + 'rawDirect' => false, 'changeType' => $type, 'fileType' => $from_file_type, @@ -421,17 +426,24 @@ class PhabricatorRepositorySvnCommitChangeParserWorker foreach ($effects as $effect) { $type = $effect['changeType']; - // Don't write COPY_AWAY to the filesystem table if it isn't a direct - // event. We do write CHILD. if (!$effect['rawDirect']) { if ($type == DifferentialChangeType::TYPE_COPY_AWAY) { + // Don't write COPY_AWAY to the filesystem table if it isn't a direct + // event. + continue; + } + if ($type == DifferentialChangeType::TYPE_CHILD) { + // Don't write CHILD to the filesystem table. Although doing these + // writes has the nice property of letting you see when a directory's + // contents were last changed, it explodes the table tremendously + // and makes Diffusion far slower. continue; } } if ($effect['rawPath'] == '/') { - // Don't bother writing the CHILD events on '/' to the filesystem - // table; in particular, it doesn't have a meaningful parentID. + // Don't write any events on '/' to the filesystem table; in + // particular, it doesn't have a meaningful parentID. continue; }