From 1bf68e06a501e01c8d50c5c19b4c390230470a37 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 9 May 2012 17:28:57 -0700 Subject: [PATCH] Improve Diffusion error messages and UI for partially imported repositories Summary: - When you have an un-cloned repository, we currently throw random-looking Git/Hg exception. Instead, throw a useful error. - When you have a cloned but undiscovered repository, we show no commits. This is crazy confusing. Instead, show commits as "importing...". - Fix some warnings and errors for empty path table cases, etc. Test Plan: - Wiped database. - Added Mercurial repo without running daemons. Viewed in Diffusion, got a good exception. - Pulled Mercurial repo without discovering it. Got "Importing...". - Discovered Mercurial repo without parsing it. Got "Importing..." plus date information. - Parsed Mercurial repo, got everything working properly. - Added Git repo without running daemons, did all the stuff above, same results. - This doesn't improve SVN much but that's a trickier case since we don't actually make SVN calls and rely only on the parse state. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T776 Differential Revision: https://secure.phabricator.com/D2439 --- src/__phutil_library_map__.php | 1 + .../setup/DiffusionSetupException.php | 25 +++++++++ .../diffusion/exception/setup/__init__.php | 12 ++++ .../diffusion/query/base/DiffusionQuery.php | 56 +++++++++++++------ .../request/base/DiffusionRequest.php | 12 ++++ .../diffusion/request/base/__init__.php | 1 + .../request/git/DiffusionGitRequest.php | 8 ++- .../diffusion/request/git/__init__.php | 2 + .../mercurial/DiffusionMercurialRequest.php | 8 +++ .../diffusion/request/mercurial/__init__.php | 2 + .../DiffusionHistoryTableView.php | 17 ++++-- .../commit/PhabricatorRepositoryCommit.php | 10 ++++ 12 files changed, 131 insertions(+), 23 deletions(-) create mode 100755 src/applications/diffusion/exception/setup/DiffusionSetupException.php create mode 100644 src/applications/diffusion/exception/setup/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6e8db5a4b7..0446053a30 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -373,6 +373,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryPath' => 'applications/diffusion/data/repositorypath', 'DiffusionRepositoryTag' => 'applications/diffusion/tag', 'DiffusionRequest' => 'applications/diffusion/request/base', + 'DiffusionSetupException' => 'applications/diffusion/exception/setup', 'DiffusionSvnBrowseQuery' => 'applications/diffusion/query/browse/svn', 'DiffusionSvnCommitParentsQuery' => 'applications/diffusion/query/parents/svn', 'DiffusionSvnCommitTagsQuery' => 'applications/diffusion/query/committags/svn', diff --git a/src/applications/diffusion/exception/setup/DiffusionSetupException.php b/src/applications/diffusion/exception/setup/DiffusionSetupException.php new file mode 100755 index 0000000000..d63775d94c --- /dev/null +++ b/src/applications/diffusion/exception/setup/DiffusionSetupException.php @@ -0,0 +1,25 @@ +setRepositoryID($repository->getID()); + $commit_obj->setCommitIdentifier($identifier); + $commit_obj->setIsUnparsed(true); + $commit_obj->makeEphemeral(); + } + $commit_list[$identifier] = $commit_obj; + } + $commits = $commit_list; + + $commit_ids = array_filter(mpull($commits, 'getID')); + if ($commit_ids) { + $commit_data = id(new PhabricatorRepositoryCommitData())->loadAllWhere( + 'commitID in (%Ld)', + $commit_ids); + $commit_data = mpull($commit_data, null, 'getCommitID'); } - $commit_data = id(new PhabricatorRepositoryCommitData())->loadAllWhere( - 'commitID in (%Ld)', - mpull($commits, 'getID')); - $commit_data = mpull($commit_data, null, 'getCommitID'); - foreach ($commits as $commit) { + if (!$commit->getID()) { + continue; + } if (idx($commit_data, $commit->getID())) { $commit->attachCommitData($commit_data[$commit->getID()]); } @@ -123,13 +140,18 @@ abstract class DiffusionQuery { $paths = ipull($paths, 'id', 'path'); $path_id = idx($paths, $path_normal); - $path_changes = queryfx_all( - $conn_r, - 'SELECT * FROM %T WHERE commitID IN (%Ld) AND pathID = %d', - PhabricatorRepository::TABLE_PATHCHANGE, - mpull($commits, 'getID'), - $path_id); - $path_changes = ipull($path_changes, null, 'commitID'); + $commit_ids = array_filter(mpull($commits, 'getID')); + + $path_changes = array(); + if ($path_id && $commit_ids) { + $path_changes = queryfx_all( + $conn_r, + 'SELECT * FROM %T WHERE commitID IN (%Ld) AND pathID = %d', + PhabricatorRepository::TABLE_PATHCHANGE, + $commit_ids, + $path_id); + $path_changes = ipull($path_changes, null, 'commitID'); + } $history = array(); foreach ($identifiers as $identifier) { diff --git a/src/applications/diffusion/request/base/DiffusionRequest.php b/src/applications/diffusion/request/base/DiffusionRequest.php index 5d4d0cafd8..5b418da562 100644 --- a/src/applications/diffusion/request/base/DiffusionRequest.php +++ b/src/applications/diffusion/request/base/DiffusionRequest.php @@ -510,4 +510,16 @@ abstract class DiffusionRequest { return $result; } + protected function raiseCloneException() { + $host = php_uname('n'); + $callsign = $this->getRepository()->getCallsign(); + throw new DiffusionSetupException( + "The working copy for this repository ('{$callsign}') hasn't been ". + "cloned yet on this machine ('{$host}'). Make sure you've started the ". + "Phabricator daemons. If this problem persists for longer than a clone ". + "should take, check the daemon logs (in the Daemon Console) to see if ". + "there were errors cloning the repository. Consult the 'Diffusion User ". + "Guide' in the documentation for help setting up repositories."); + } + } diff --git a/src/applications/diffusion/request/base/__init__.php b/src/applications/diffusion/request/base/__init__.php index 55d28e7536..76f935f32f 100644 --- a/src/applications/diffusion/request/base/__init__.php +++ b/src/applications/diffusion/request/base/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/diffusion/exception/setup'); phutil_require_module('phabricator', 'applications/repository/constants/repositorytype'); phutil_require_module('phabricator', 'applications/repository/storage/commit'); phutil_require_module('phabricator', 'applications/repository/storage/commitdata'); diff --git a/src/applications/diffusion/request/git/DiffusionGitRequest.php b/src/applications/diffusion/request/git/DiffusionGitRequest.php index d9471b0aa2..d59b77b4c8 100644 --- a/src/applications/diffusion/request/git/DiffusionGitRequest.php +++ b/src/applications/diffusion/request/git/DiffusionGitRequest.php @@ -26,13 +26,19 @@ final class DiffusionGitRequest extends DiffusionRequest { } protected function didInitialize() { + $repository = $this->getRepository(); + + if (!Filesystem::pathExists($repository->getLocalPath())) { + $this->raiseCloneException(); + } + if (!$this->commit) { return; } // Expand short commit names and verify - $future = $this->getRepository()->getLocalCommandFuture( + $future = $repository->getLocalCommandFuture( 'cat-file --batch'); $future->write($this->commit); list($stdout) = $future->resolvex(); diff --git a/src/applications/diffusion/request/git/__init__.php b/src/applications/diffusion/request/git/__init__.php index f4eb1320a3..0c20833928 100644 --- a/src/applications/diffusion/request/git/__init__.php +++ b/src/applications/diffusion/request/git/__init__.php @@ -10,5 +10,7 @@ phutil_require_module('phabricator', 'aphront/exception/usage'); phutil_require_module('phabricator', 'applications/diffusion/data/branch'); phutil_require_module('phabricator', 'applications/diffusion/request/base'); +phutil_require_module('phutil', 'filesystem'); + phutil_require_source('DiffusionGitRequest.php'); diff --git a/src/applications/diffusion/request/mercurial/DiffusionMercurialRequest.php b/src/applications/diffusion/request/mercurial/DiffusionMercurialRequest.php index cceaf49073..53b54088f7 100644 --- a/src/applications/diffusion/request/mercurial/DiffusionMercurialRequest.php +++ b/src/applications/diffusion/request/mercurial/DiffusionMercurialRequest.php @@ -26,6 +26,12 @@ final class DiffusionMercurialRequest extends DiffusionRequest { } protected function didInitialize() { + $repository = $this->getRepository(); + + if (!Filesystem::pathExists($repository->getLocalPath())) { + $this->raiseCloneException(); + } + return; } @@ -33,9 +39,11 @@ final class DiffusionMercurialRequest extends DiffusionRequest { if ($this->branch) { return $this->branch; } + if ($this->repository) { return $this->repository->getDetail('default-branch', 'default'); } + throw new Exception("Unable to determine branch!"); } diff --git a/src/applications/diffusion/request/mercurial/__init__.php b/src/applications/diffusion/request/mercurial/__init__.php index 32af21a2de..a049ba735a 100644 --- a/src/applications/diffusion/request/mercurial/__init__.php +++ b/src/applications/diffusion/request/mercurial/__init__.php @@ -8,5 +8,7 @@ phutil_require_module('phabricator', 'applications/diffusion/request/base'); +phutil_require_module('phutil', 'filesystem'); + phutil_require_source('DiffusionMercurialRequest.php'); diff --git a/src/applications/diffusion/view/historytable/DiffusionHistoryTableView.php b/src/applications/diffusion/view/historytable/DiffusionHistoryTableView.php index 34c50d6f3d..d392a14766 100644 --- a/src/applications/diffusion/view/historytable/DiffusionHistoryTableView.php +++ b/src/applications/diffusion/view/historytable/DiffusionHistoryTableView.php @@ -93,6 +93,17 @@ final class DiffusionHistoryTableView extends DiffusionView { $author = phutil_escape_html($history->getAuthorName()); } + $commit = $history->getCommit(); + if ($commit && !$commit->getIsUnparsed() && $data) { + $change = $this->linkChange( + $history->getChangeType(), + $history->getFileType(), + $path = null, + $history->getCommitIdentifier()); + } else { + $change = "Importing\xE2\x80\xA6"; + } + $rows[] = array( $this->linkBrowse( $drequest->getPath(), @@ -103,11 +114,7 @@ final class DiffusionHistoryTableView extends DiffusionView { self::linkCommit( $drequest->getRepository(), $history->getCommitIdentifier()), - $this->linkChange( - $history->getChangeType(), - $history->getFileType(), - null, - $history->getCommitIdentifier()), + $change, $date, $time, $author, diff --git a/src/applications/repository/storage/commit/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/commit/PhabricatorRepositoryCommit.php index 243daa8c9c..3badbe3089 100644 --- a/src/applications/repository/storage/commit/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/commit/PhabricatorRepositoryCommit.php @@ -27,6 +27,16 @@ final class PhabricatorRepositoryCommit extends PhabricatorRepositoryDAO { protected $auditStatus = PhabricatorAuditCommitStatusConstants::NONE; private $commitData; + private $isUnparsed; + + public function setIsUnparsed($is_unparsed) { + $this->isUnparsed = $is_unparsed; + return $this; + } + + public function getIsUnparsed() { + return $this->isUnparsed; + } public function getConfiguration() { return array(