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
2024-09-20 01:08:50 +02:00 · 2011-09-04 14:39:52 -07:00 · 2011-09-04 14:39:52 -07:00 · 8f3b342287
commit 8f3b342287
parent 8b06d7d1c6
8 changed files with 174 additions and 78 deletions
--- 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 =
+                "<strong>NOTE:</strong> ".
+                "You can manually retry this task by running this script:".
+                "<pre>".
+                  "phabricator/\$ ./scripts/repository/parse_one_commit.php ".
+                  "r".
+                  phutil_escape_html($repository->getCallsign()).
+                  phutil_escape_html($commit->getCommitIdentifier()).
+                "</pre>";
+            }
+          }
+        }
+        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(
--- 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');


--- 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!");
      }
--- 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');


--- 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(
-      '<div class="diffusion-commit-view">'.
-        '<div class="diffusion-commit-dateline">'.
-          'r'.$callsign.$commit->getCommitIdentifier().
-          ' &middot; '.
-          phabricator_datetime($commit->getEpoch(), $user).
-        '</div>'.
-        '<h1>Revision Detail</h1>'.
-        '<div class="diffusion-commit-details">'.
-          $property_table.
-          '<hr />'.
-          '<div class="diffusion-commit-message phabricator-remarkup">'.
-            $engine->markupText($commit_data->getCommitMessage()).
+      $property_table = $this->renderPropertyTable($commit, $commit_data);
+
+      $detail_panel->appendChild(
+        '<div class="diffusion-commit-view">'.
+          '<div class="diffusion-commit-dateline">'.
+            'r'.$callsign.$commit->getCommitIdentifier().
+            ' &middot; '.
+            phabricator_datetime($commit->getEpoch(), $user).
          '</div>'.
-        '</div>'.
-      '</div>');
+          '<h1>Revision Detail</h1>'.
+          '<div class="diffusion-commit-details">'.
+            $property_table.
+            '<hr />'.
+            '<div class="diffusion-commit-message phabricator-remarkup">'.
+              $engine->markupText($commit_data->getCommitMessage()).
+            '</div>'.
+          '</div>'.
+        '</div>');

-    $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 =
-          '<div class="differential-primary-pane">'.
-            $change_list->render().
-          '</div>';
-      } else {
-        $change_list =
-          '<div style="margin: 2em; color: #666; padding: 1em;
-            background: #eee;">'.
-            '(no changes blah blah)'.
-          '</div>';
+      $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 =
+        '<div class="differential-primary-pane">'.
+          $change_list->render().
+        '</div>';
+
      $content[] = $change_list;
    }

--- 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() {
    // <private>
--- 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);
--- 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, '/');