diff --git a/src/applications/diffusion/controller/DiffusionCommitEditController.php b/src/applications/diffusion/controller/DiffusionCommitEditController.php --- a/src/applications/diffusion/controller/DiffusionCommitEditController.php +++ b/src/applications/diffusion/controller/DiffusionCommitEditController.php @@ -8,13 +8,13 @@ } public function processRequest() { - $request = $this->getRequest(); $user = $request->getUser(); $drequest = $this->getDiffusionRequest(); $callsign = $drequest->getRepository()->getCallsign(); $repository = $drequest->getRepository(); $commit = $drequest->loadCommit(); + $data = $commit->loadCommitData(); $page_title = pht('Edit Diffusion Commit'); if (!$commit) { @@ -52,7 +52,7 @@ } $tokenizer_id = celerity_generate_unique_node_id(); - $form = id(new AphrontFormView()) + $form = id(new AphrontFormView()) ->setUser($user) ->setAction($request->getRequestURI()->getPath()) ->appendChild( @@ -72,6 +72,42 @@ pht('Create New Project'))) ->setDatasource(new PhabricatorProjectDatasource())); + $reason = $data->getCommitDetail('autocloseReason', false); + if ($reason !== false) { + switch ($reason) { + case PhabricatorRepository::BECAUSE_REPOSITORY_IMPORTING: + $desc = pht('No, Repository Importing'); + break; + case PhabricatorRepository::BECAUSE_AUTOCLOSE_DISABLED: + $desc = pht('No, Autoclose Disabled'); + break; + case PhabricatorRepository::BECAUSE_NOT_ON_AUTOCLOSE_BRANCH: + $desc = pht('No, Not On Autoclose Branch'); + break; + case null: + $desc = pht('Yes'); + break; + default: + $desc = pht('Unknown'); + break; + } + + $doc_href = PhabricatorEnv::getDoclink('Diffusion User Guide: Autoclose'); + $doc_link = phutil_tag( + 'a', + array( + 'href' => $doc_href, + 'target' => '_blank', + ), + pht('Learn More')); + + $form->appendChild( + id(new AphrontFormMarkupControl()) + ->setLabel(pht('Autoclose?')) + ->setValue(array($desc, " \xC2\xB7 ", $doc_link))); + } + + Javelin::initBehavior('project-create', array( 'tokenizerID' => $tokenizer_id, )); @@ -81,12 +117,18 @@ ->addCancelButton('/r'.$callsign.$commit->getCommitIdentifier()); $form->appendChild($submit); + $crumbs = $this->buildCrumbs(array( + 'commit' => true, + )); + $crumbs->addTextCrumb(pht('Edit')); + $form_box = id(new PHUIObjectBoxView()) ->setHeaderText($page_title) ->setForm($form); return $this->buildApplicationPage( array( + $crumbs, $form_box, ), array( diff --git a/src/applications/diffusion/view/DiffusionBranchTableView.php b/src/applications/diffusion/view/DiffusionBranchTableView.php --- a/src/applications/diffusion/view/DiffusionBranchTableView.php +++ b/src/applications/diffusion/view/DiffusionBranchTableView.php @@ -20,6 +20,11 @@ public function render() { $drequest = $this->getDiffusionRequest(); $current_branch = $drequest->getBranch(); + $repository = $drequest->getRepository(); + + Javelin::initBehavior('phabricator-tooltips'); + + $doc_href = PhabricatorEnv::getDoclink('Diffusion User Guide: Autoclose'); $rows = array(); $rowc = array(); @@ -33,6 +38,43 @@ $details = null; } + switch ($repository->shouldSkipAutocloseBranch($branch->getShortName())) { + case PhabricatorRepository::BECAUSE_REPOSITORY_IMPORTING: + $icon = 'fa-times bluegrey'; + $tip = pht('Repository Importing'); + break; + case PhabricatorRepository::BECAUSE_AUTOCLOSE_DISABLED: + $icon = 'fa-times bluegrey'; + $tip = pht('Repository Autoclose Disabled'); + break; + case PhabricatorRepository::BECAUSE_BRANCH_UNTRACKED: + $icon = 'fa-times bluegrey'; + $tip = pht('Branch Untracked'); + break; + case PhabricatorRepository::BECAUSE_BRANCH_NOT_AUTOCLOSE: + $icon = 'fa-times bluegrey'; + $tip = pht('Branch Autoclose Disabled'); + break; + case null: + $icon = 'fa-check bluegrey'; + $tip = pht('Autoclose Enabled'); + break; + default: + $icon = 'fa-question'; + $tip = pht('Status Unknown'); + break; + } + + $status_icon = id(new PHUIIconView()) + ->setIconFont($icon) + ->addSigil('has-tooltip') + ->setHref($doc_href) + ->setMetadata( + array( + 'tip' => $tip, + 'size' => 200, + )); + $rows[] = array( phutil_tag( 'a', @@ -57,6 +99,7 @@ self::linkCommit( $drequest->getRepository(), $branch->getCommitIdentifier()), + $status_icon, $datetime, AphrontTableView::renderSingleDisplayLine($details), ); @@ -73,6 +116,7 @@ pht('History'), pht('Branch'), pht('Head'), + pht(''), pht('Modified'), pht('Details'), )); @@ -82,6 +126,7 @@ 'pri', '', '', + '', 'wide', )); $view->setRowClasses($rowc); diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/HeraldCommitAdapter.php @@ -475,9 +475,7 @@ $refs = DiffusionRepositoryRef::loadAllFromDictionaries($result); return mpull($refs, 'getShortName'); case self::FIELD_REPOSITORY_AUTOCLOSE_BRANCH: - return $this->repository->shouldAutocloseCommit( - $this->commit, - $this->commitData); + return $this->repository->shouldAutocloseCommit($this->commit); } return parent::getHeraldField($field); diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1,7 +1,8 @@ isBranchInFilter($branch, 'branch-filter'); } - public function shouldAutocloseBranch($branch) { - if ($this->isImporting()) { - return false; + public function formatCommitName($commit_identifier) { + $vcs = $this->getVersionControlSystem(); + + $type_git = PhabricatorRepositoryType::REPOSITORY_TYPE_GIT; + $type_hg = PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL; + + $is_git = ($vcs == $type_git); + $is_hg = ($vcs == $type_hg); + if ($is_git || $is_hg) { + $short_identifier = substr($commit_identifier, 0, 12); + } else { + $short_identifier = $commit_identifier; } - if ($this->getDetail('disable-autoclose', false)) { - return false; + return 'r'.$this->getCallsign().$short_identifier; + } + + public function isImporting() { + return (bool)$this->getDetail('importing', false); + } + + +/* -( Autoclose )---------------------------------------------------------- */ + + + /** + * Determine if autoclose is active for a branch. + * + * For more details about why, use @{method:shouldSkipAutocloseBranch}. + * + * @param string Branch name to check. + * @return bool True if autoclose is active for the branch. + * @task autoclose + */ + public function shouldAutocloseBranch($branch) { + return ($this->shouldSkipAutocloseBranch($branch) === null); + } + + /** + * Determine if autoclose is active for a commit. + * + * For more details about why, use @{method:shouldSkipAutocloseCommit}. + * + * @param PhabricatorRepositoryCommit Commit to check. + * @return bool True if autoclose is active for the commit. + * @task autoclose + */ + public function shouldAutocloseCommit(PhabricatorRepositoryCommit $commit) { + return ($this->shouldSkipAutocloseCommit($commit) === null); + } + + + /** + * Determine why autoclose should be skipped for a branch. + * + * This method gives a detailed reason why autoclose will be skipped. To + * perform a simple test, use @{method:shouldAutocloseBranch}. + * + * @param string Branch name to check. + * @return const|null Constant identifying reason to skip this branch, or null + * if autoclose is active. + * @task autoclose + */ + public function shouldSkipAutocloseBranch($branch) { + $all_reason = $this->shouldSkipAllAutoclose(); + if ($all_reason) { + return $all_reason; } if (!$this->shouldTrackBranch($branch)) { - return false; + return self::BECAUSE_BRANCH_UNTRACKED; + } + + if (!$this->isBranchInFilter($branch, 'close-commits-filter')) { + return self::BECAUSE_BRANCH_NOT_AUTOCLOSE; } - return $this->isBranchInFilter($branch, 'close-commits-filter'); + return null; } - public function shouldAutocloseCommit( - PhabricatorRepositoryCommit $commit, - PhabricatorRepositoryCommitData $data) { - if ($this->getDetail('disable-autoclose', false)) { - return false; + /** + * Determine why autoclose should be skipped for a commit. + * + * This method gives a detailed reason why autoclose will be skipped. To + * perform a simple test, use @{method:shouldAutocloseCommit}. + * + * @param PhabricatorRepositoryCommit Commit to check. + * @return const|null Constant identifying reason to skip this commit, or null + * if autoclose is active. + * @task autoclose + */ + public function shouldSkipAutocloseCommit( + PhabricatorRepositoryCommit $commit) { + + $all_reason = $this->shouldSkipAllAutoclose(); + if ($all_reason) { + return $all_reason; } switch ($this->getVersionControlSystem()) { case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - return true; + case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: + return null; case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: - return true; default: throw new Exception('Unrecognized version control system.'); } $closeable_flag = PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE; - if ($commit->isPartiallyImported($closeable_flag)) { - return true; + if (!$commit->isPartiallyImported($closeable_flag)) { + return self::BECAUSE_NOT_ON_AUTOCLOSE_BRANCH; } - // TODO: Remove this eventually, it's no longer written to by the import - // pipeline (however, old tasks may still be queued which don't reflect - // the new data format). - $branches = $data->getCommitDetail('seenOnBranches', array()); - foreach ($branches as $branch) { - if ($this->shouldAutocloseBranch($branch)) { - return true; - } - } - - return false; + return null; } - public function formatCommitName($commit_identifier) { - $vcs = $this->getVersionControlSystem(); - - $type_git = PhabricatorRepositoryType::REPOSITORY_TYPE_GIT; - $type_hg = PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL; - $is_git = ($vcs == $type_git); - $is_hg = ($vcs == $type_hg); - if ($is_git || $is_hg) { - $short_identifier = substr($commit_identifier, 0, 12); - } else { - $short_identifier = $commit_identifier; + /** + * Determine why all autoclose operations should be skipped for this + * repository. + * + * @return const|null Constant identifying reason to skip all autoclose + * operations, or null if autoclose operations are not blocked at the + * repository level. + * @task autoclose + */ + private function shouldSkipAllAutoclose() { + if ($this->isImporting()) { + return self::BECAUSE_REPOSITORY_IMPORTING; } - return 'r'.$this->getCallsign().$short_identifier; - } + if ($this->getDetail('disable-autoclose', false)) { + return self::BECAUSE_AUTOCLOSE_DISABLED; + } - public function isImporting() { - return (bool)$this->getDetail('importing', false); + return null; } diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -68,6 +68,16 @@ $commit->setSummary($data->getSummary()); $commit->save(); + // Figure out if we're going to try to "autoclose" related objects (e.g., + // close linked tasks and related revisions) and, if not, record why we + // aren't. Autoclose can be disabled for various reasons at the repository + // or commit levels. + + $autoclose_reason = $repository->shouldSkipAutocloseCommit($commit); + $data->setCommitDetail('autocloseReason', $autoclose_reason); + $should_autoclose = $repository->shouldAutocloseCommit($commit); + + // When updating related objects, we'll act under an omnipotent user to // ensure we can see them, but take actions as either the committer or // author (if we recognize their accounts) or the Diffusion application @@ -90,8 +100,6 @@ // someone probably did something very silly, though.) $revision = null; - $should_autoclose = $repository->shouldAutocloseCommit($commit, $data); - if ($revision_id) { $revision_query = id(new DifferentialRevisionQuery()) ->withIDs(array($revision_id)) diff --git a/src/docs/user/userguide/diffusion_autoclose.diviner b/src/docs/user/userguide/diffusion_autoclose.diviner new file mode 100644 --- /dev/null +++ b/src/docs/user/userguide/diffusion_autoclose.diviner @@ -0,0 +1,60 @@ +@title Diffusion User Guide: Autoclose +@group userguide + +Explains when Diffusion will close tasks and revisions upon discovery of related +commits. + +Overview +======== + +Diffusion can close tasks and revisions when related commits appear in a +repository. For example, if you make a commit with `Fixes T123` in the commit +message, Diffusion will close the task `T123`. + +This document explains how autoclose works, how to configure it, and how to +troubleshoot it. + +Troubleshooting Autoclose +========================= + +You can check if a branch is currently configured to autoclose on the main +repository view, or in the branches list view. Hover over the {icon check} or +{icon times} icon and you should see one of these messages: + + - {icon check} **Autoclose Enabled** Autoclose is active for this branch. + - {icon times} **Repository Importing** This repository is still importing. + Autoclose does not activate until a repository finishes importing for the + first time. This prevents situations where you import a repository and + accidentally close hundreds of related objects during import. Autoclose + will activate for new commits after the initial import completes. + - {icon times} **Repository Autoclose Disabled** Autoclose is disabled for + this entire repository. You can enable it in **Edit Repository**. + - {icon times} **Branch Untracked** This branch is not tracked. Because it + is not tracked, commits on it won't be seen and won't be discovered. + - {icon times} **Branch Autoclose Disabled** Autoclose is not enabled for + this branch. You can adjust which branches autoclose in **Edit Repository**. + This option is only available in Git. + +If a branch is in good shape, you can check a specific commit by viewing it +in the web UI and clicking **Edit Commit**. There should be an **Autoclose?** +field visible in the form, with possible values listed below. + +Note that this field records the state of the world at the time the commit was +processed, and does not necessarily reflect the current state of the world. +For example, if a commit did not trigger autoclose because it was processed +during initial import, the field will still show **No, Repository Importing** +even after import completes. This means that the commit did not trigger +autoclose because the repository was importing at the time it was processed, +not necessarily that the repository is still importing. + + - **Yes** At the time the commit was imported, autoclose triggered and + Phabricator attempted to close related objects. + - **No, Repository Importing** At the time the commit was processed, the + repository was still importing. Autoclose does not activate until a + repository fully imports for the first time. + - **No, Autoclose Disabled** At the time the commit was processed, the + repository had autoclose disabled. + - **No, Not On Autoclose Branch** At the time the commit was processed, + no containing branch was configured to autoclose. + - //Field Not Present// This commit was processed before we implemented + this diagnostic feature, and no information is available.