From 8f3b342287bccbac1ce5b65b66f23792a0d2251c Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 4 Sep 2011 14:39:52 -0700 Subject: [PATCH] Improve several Diffusion UI error states Summary: Give users better errors and UI: - For subpath SVN repositories, default the path to the subdirectory, not to "/". This makes the home screen useful and things generally less confusing. - For unparsed commits, show a more descriptive error message without the "blah blah" silliness. - For paths outside of the subpath parse tree, short circuit into an appropriate error message. - For foreign SVN stub commits (see D892), show an explicit message. Test Plan: Looked at unparsed commits, subpath repositories, foreign stub commits, and paths outside of the subpath parse tree. Received sensible error messages. Reviewers: jungejason, nh, tuomaspelkonen, aran Reviewed By: jungejason CC: aran, jungejason Differential Revision: 894 --- .../PhabricatorWorkerTaskDetailController.php | 42 ++++- .../controller/workertaskdetail/__init__.php | 4 + .../browse/DiffusionBrowseController.php | 10 ++ .../diffusion/controller/browse/__init__.php | 1 + .../commit/DiffusionCommitController.php | 167 ++++++++++-------- .../browse/base/DiffusionBrowseQuery.php | 11 +- .../browse/svn/DiffusionSvnBrowseQuery.php | 9 + .../request/svn/DiffusionSvnRequest.php | 8 + 8 files changed, 174 insertions(+), 78 deletions(-) diff --git a/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php b/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php index 03cbfdcadc..d4acf74840 100644 --- a/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php +++ b/src/applications/daemon/controller/workertaskdetail/PhabricatorWorkerTaskDetailController.php @@ -46,6 +46,35 @@ class PhabricatorWorkerTaskDetailController $data = id(new PhabricatorWorkerTaskData())->loadOneWhere( 'id = %d', $task->getDataID()); + + $extra = null; + switch ($task->getTaskClass()) { + case 'PhabricatorRepositorySvnCommitChangeParserWorker': + case 'PhabricatorRepositoryGitCommitChangeParserWorker': + $commit_id = idx($data->getData(), 'commitID'); + if ($commit_id) { + $commit = id(new PhabricatorRepositoryCommit())->load($commit_id); + if ($commit) { + $repository = id(new PhabricatorRepository())->load( + $commit->getRepositoryID()); + if ($repository) { + $extra = + "NOTE: ". + "You can manually retry this task by running this script:". + "
".
+                  "phabricator/\$ ./scripts/repository/parse_one_commit.php ".
+                  "r".
+                  phutil_escape_html($repository->getCallsign()).
+                  phutil_escape_html($commit->getCommitIdentifier()).
+                "
"; + } + } + } + break; + default: + break; + } + if ($data) { $data = json_encode($data->getData()); } @@ -75,14 +104,23 @@ class PhabricatorWorkerTaskDetailController ->appendChild( id(new AphrontFormTextAreaControl()) ->setLabel('Data') - ->setValue($data)) + ->setValue($data)); + + if ($extra) { + $form->appendChild( + id(new AphrontFormMarkupControl()) + ->setLabel('More') + ->setValue($extra)); + } + + $form ->appendChild( id(new AphrontFormSubmitControl()) ->addCancelButton('/daemon/')); $panel = new AphrontPanelView(); $panel->setHeader('Task Detail'); - $panel->setWidth(AphrontPanelView::WIDTH_FORM); + $panel->setWidth(AphrontPanelView::WIDTH_WIDE); $panel->appendChild($form); return $this->buildStandardPageResponse( diff --git a/src/applications/daemon/controller/workertaskdetail/__init__.php b/src/applications/daemon/controller/workertaskdetail/__init__.php index 13e9017739..66839c4cf7 100644 --- a/src/applications/daemon/controller/workertaskdetail/__init__.php +++ b/src/applications/daemon/controller/workertaskdetail/__init__.php @@ -7,15 +7,19 @@ phutil_require_module('phabricator', 'applications/daemon/controller/base'); +phutil_require_module('phabricator', 'applications/repository/storage/commit'); +phutil_require_module('phabricator', 'applications/repository/storage/repository'); phutil_require_module('phabricator', 'infrastructure/daemon/workers/storage/task'); phutil_require_module('phabricator', 'infrastructure/daemon/workers/storage/taskdata'); phutil_require_module('phabricator', 'view/form/base'); +phutil_require_module('phabricator', 'view/form/control/markup'); phutil_require_module('phabricator', 'view/form/control/static'); phutil_require_module('phabricator', 'view/form/control/submit'); phutil_require_module('phabricator', 'view/form/control/textarea'); phutil_require_module('phabricator', 'view/form/error'); phutil_require_module('phabricator', 'view/layout/panel'); +phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/diffusion/controller/browse/DiffusionBrowseController.php b/src/applications/diffusion/controller/browse/DiffusionBrowseController.php index f69d18da9e..f67b9cfaf2 100644 --- a/src/applications/diffusion/controller/browse/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/browse/DiffusionBrowseController.php @@ -75,6 +75,16 @@ class DiffusionBrowseController extends DiffusionController { $controller->setDiffusionRequest($drequest); return $this->delegateToController($controller); break; + case DiffusionBrowseQuery::REASON_IS_UNTRACKED_PARENT: + $subdir = $drequest->getRepository()->getDetail('svn-subpath'); + $title = 'Directory Not Tracked'; + $body = + "This repository is configured to track only one subdirectory ". + "of the entire repository ('".phutil_escape_html($subdir)."'), ". + "but you aren't looking at something in that subdirectory, so no ". + "information is available."; + $severity = AphrontErrorView::SEVERITY_WARNING; + break; default: throw new Exception("Unknown failure reason!"); } diff --git a/src/applications/diffusion/controller/browse/__init__.php b/src/applications/diffusion/controller/browse/__init__.php index 20a7e93855..2c07414577 100644 --- a/src/applications/diffusion/controller/browse/__init__.php +++ b/src/applications/diffusion/controller/browse/__init__.php @@ -14,6 +14,7 @@ phutil_require_module('phabricator', 'applications/phid/handle/data'); phutil_require_module('phabricator', 'view/form/error'); phutil_require_module('phabricator', 'view/layout/panel'); +phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/diffusion/controller/commit/DiffusionCommitController.php b/src/applications/diffusion/controller/commit/DiffusionCommitController.php index 4b7e80178c..1d8566656d 100644 --- a/src/applications/diffusion/controller/commit/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/commit/DiffusionCommitController.php @@ -44,31 +44,47 @@ class DiffusionCommitController extends DiffusionController { $commit_data = $drequest->loadCommitData(); - $engine = PhabricatorMarkupEngine::newDifferentialMarkupEngine(); + $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub'); + if ($is_foreign) { + $subpath = $commit_data->getCommitDetail('svn-subpath'); - require_celerity_resource('diffusion-commit-view-css'); - require_celerity_resource('phabricator-remarkup-css'); + $error_panel = new AphrontErrorView(); + $error_panel->setWidth(AphrontErrorView::WIDTH_WIDE); + $error_panel->setTitle('Commit Not Tracked'); + $error_panel->setSeverity(AphrontErrorView::SEVERITY_WARNING); + $error_panel->appendChild( + "This Diffusion repository is configured to track only one ". + "subdirectory of the entire Subversion repository, and this commit ". + "didn't affect the tracked subdirectory ('". + phutil_escape_html($subpath)."'), so no information is available."); + $content[] = $error_panel; + } else { + $engine = PhabricatorMarkupEngine::newDifferentialMarkupEngine(); - $property_table = $this->renderPropertyTable($commit, $commit_data); + require_celerity_resource('diffusion-commit-view-css'); + require_celerity_resource('phabricator-remarkup-css'); - $detail_panel->appendChild( - '
'. - '
'. - 'r'.$callsign.$commit->getCommitIdentifier(). - ' · '. - phabricator_datetime($commit->getEpoch(), $user). - '
'. - '

Revision Detail

'. - '
'. - $property_table. - '
'. - '
'. - $engine->markupText($commit_data->getCommitMessage()). + $property_table = $this->renderPropertyTable($commit, $commit_data); + + $detail_panel->appendChild( + '
'. + '
'. + 'r'.$callsign.$commit->getCommitIdentifier(). + ' · '. + phabricator_datetime($commit->getEpoch(), $user). '
'. - '
'. - '
'); + '

Revision Detail

'. + '
'. + $property_table. + '
'. + '
'. + $engine->markupText($commit_data->getCommitMessage()). + '
'. + '
'. + '
'); - $content[] = $detail_panel; + $content[] = $detail_panel; + } $change_query = DiffusionPathChangeQuery::newFromDiffusionRequest( $drequest); @@ -103,6 +119,23 @@ class DiffusionCommitController extends DiffusionController { phutil_escape_html($bad_commit['description'])); $content[] = $error_panel; + } else if ($is_foreign) { + // Don't render anything else. + } else if (!count($changes)) { + $no_changes = new AphrontErrorView(); + $no_changes->setWidth(AphrontErrorView::WIDTH_WIDE); + $no_changes->setSeverity(AphrontErrorView::SEVERITY_WARNING); + $no_changes->setTitle('Not Yet Parsed'); + // TODO: This can also happen with weird SVN changes that don't do + // anything (or only alter properties?), although the real no-changes case + // is extremely rare and might be impossible to produce organically. We + // should probably write some kind of "Nothing Happened!" change into the + // DB once we parse these changes so we can distinguish between + // "not parsed yet" and "no changes". + $no_changes->appendChild( + "This commit hasn't been fully parsed yet (or doesn't affect any ". + "paths)."); + $content[] = $no_changes; } else { $change_panel = new AphrontPanelView(); $change_panel->setHeader("Changes (".number_format($count).")"); @@ -130,60 +163,52 @@ class DiffusionCommitController extends DiffusionController { $content[] = $change_panel; - if ($changes) { - $changesets = DiffusionPathChange::convertToDifferentialChangesets( - $changes); + $changesets = DiffusionPathChange::convertToDifferentialChangesets( + $changes); - $vcs = $repository->getVersionControlSystem(); - switch ($vcs) { - case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $vcs_supports_directory_changes = true; - break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: - $vcs_supports_directory_changes = false; - break; - default: - throw new Exception("Unknown VCS."); - } - - $references = array(); - foreach ($changesets as $key => $changeset) { - $file_type = $changeset->getFileType(); - if ($file_type == DifferentialChangeType::FILE_DIRECTORY) { - if (!$vcs_supports_directory_changes) { - unset($changesets[$key]); - continue; - } - } - - $branch = $drequest->getBranchURIComponent( - $drequest->getBranch()); - $filename = $changeset->getFilename(); - $commit = $drequest->getCommit(); - $reference = "{$branch}{$filename};{$commit}"; - $references[$key] = $reference; - } - - $change_list = new DifferentialChangesetListView(); - $change_list->setChangesets($changesets); - $change_list->setRenderingReferences($references); - $change_list->setRenderURI('/diffusion/'.$callsign.'/diff/'); - - // TODO: This is pretty awkward, unify the CSS between Diffusion and - // Differential better. - require_celerity_resource('differential-core-view-css'); - $change_list = - '
'. - $change_list->render(). - '
'; - } else { - $change_list = - '
'. - '(no changes blah blah)'. - '
'; + $vcs = $repository->getVersionControlSystem(); + switch ($vcs) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + $vcs_supports_directory_changes = true; + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + $vcs_supports_directory_changes = false; + break; + default: + throw new Exception("Unknown VCS."); } + $references = array(); + foreach ($changesets as $key => $changeset) { + $file_type = $changeset->getFileType(); + if ($file_type == DifferentialChangeType::FILE_DIRECTORY) { + if (!$vcs_supports_directory_changes) { + unset($changesets[$key]); + continue; + } + } + + $branch = $drequest->getBranchURIComponent( + $drequest->getBranch()); + $filename = $changeset->getFilename(); + $commit = $drequest->getCommit(); + $reference = "{$branch}{$filename};{$commit}"; + $references[$key] = $reference; + } + + $change_list = new DifferentialChangesetListView(); + $change_list->setChangesets($changesets); + $change_list->setRenderingReferences($references); + $change_list->setRenderURI('/diffusion/'.$callsign.'/diff/'); + + // TODO: This is pretty awkward, unify the CSS between Diffusion and + // Differential better. + require_celerity_resource('differential-core-view-css'); + $change_list = + '
'. + $change_list->render(). + '
'; + $content[] = $change_list; } diff --git a/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php b/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php index 02c3283a87..cba77ec2b5 100644 --- a/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php +++ b/src/applications/diffusion/query/browse/base/DiffusionBrowseQuery.php @@ -25,11 +25,12 @@ abstract class DiffusionBrowseQuery { protected $deletedAtCommit; protected $validityOnly; - const REASON_IS_FILE = 'is-file'; - const REASON_IS_DELETED = 'is-deleted'; - const REASON_IS_NONEXISTENT = 'nonexistent'; - const REASON_BAD_COMMIT = 'bad-commit'; - const REASON_IS_EMPTY = 'empty'; + const REASON_IS_FILE = 'is-file'; + const REASON_IS_DELETED = 'is-deleted'; + const REASON_IS_NONEXISTENT = 'nonexistent'; + const REASON_BAD_COMMIT = 'bad-commit'; + const REASON_IS_EMPTY = 'empty'; + const REASON_IS_UNTRACKED_PARENT = 'untracked-parent'; final private function __construct() { // diff --git a/src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php b/src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php index c47952118a..b513b50763 100644 --- a/src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php +++ b/src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php @@ -25,6 +25,15 @@ final class DiffusionSvnBrowseQuery extends DiffusionBrowseQuery { $path = $drequest->getPath(); $commit = $drequest->getCommit(); + $subpath = $repository->getDetail('svn-subpath'); + if ($subpath && strncmp($subpath, $path, strlen($subpath))) { + // If we have a subpath and the path isn't a child of it, it (almost + // certainly) won't exist since we don't track commits which affect + // it. (Even if it exists, return a consistent result.) + $this->reason = self::REASON_IS_UNTRACKED_PARENT; + return array(); + } + $conn_r = $repository->establishConnection('r'); $parent_path = dirname($path); diff --git a/src/applications/diffusion/request/svn/DiffusionSvnRequest.php b/src/applications/diffusion/request/svn/DiffusionSvnRequest.php index 577e36c64e..81c9c68bd1 100644 --- a/src/applications/diffusion/request/svn/DiffusionSvnRequest.php +++ b/src/applications/diffusion/request/svn/DiffusionSvnRequest.php @@ -20,6 +20,14 @@ class DiffusionSvnRequest extends DiffusionRequest { protected function initializeFromAphrontRequestDictionary(array $data) { parent::initializeFromAphrontRequestDictionary($data); + + if ($this->path === null) { + $subpath = $this->repository->getDetail('svn-subpath'); + if ($subpath) { + $this->path = $subpath; + } + } + if (!strncmp($this->path, ':', 1)) { $this->path = substr($this->path, 1); $this->path = ltrim($this->path, '/');