From 912b4c564d6f9b2a857ff36b087534c9d0d7fe17 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Aug 2014 13:28:55 -0700 Subject: [PATCH] Allow "Track Only" and "Autoclose" to accept regular expressions Summary: Fixes T2564. See screenshot. Test Plan: {F194796} - Made a bunch of valid and invalid adjustments here and verified that the branches table showed autoclose state and branches consistent with the settings. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T2564 Differential Revision: https://secure.phabricator.com/D10349 --- ...fusionRepositoryEditBranchesController.php | 152 ++++++++++++++---- .../editor/PhabricatorRepositoryEditor.php | 46 ++++++ .../storage/PhabricatorRepository.php | 43 ++++- 3 files changed, 205 insertions(+), 36 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditBranchesController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditBranchesController.php index 84cc7938ff..7d185d90f2 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditBranchesController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditBranchesController.php @@ -43,17 +43,26 @@ final class DiffusionRepositoryEditBranchesController $edit_uri = $this->getRepositoryControllerURI($repository, 'edit/'); $v_default = $repository->getHumanReadableDetail('default-branch'); - $v_track = $repository->getHumanReadableDetail( + $v_track = $repository->getDetail( 'branch-filter', array()); - $v_autoclose = $repository->getHumanReadableDetail( + $v_track = array_keys($v_track); + $v_autoclose = $repository->getDetail( 'close-commits-filter', array()); + $v_autoclose = array_keys($v_autoclose); + $e_track = null; + $e_autoclose = null; + + $validation_exception = null; if ($request->isFormPost()) { $v_default = $request->getStr('default'); - $v_track = $request->getStrList('track'); - $v_autoclose = $request->getStrList('autoclose'); + + $v_track = $this->processBranches($request->getStr('track')); + if (!$is_hg) { + $v_autoclose = $this->processBranches($request->getStr('autoclose')); + } $xactions = array(); $template = id(new PhabricatorRepositoryTransaction()); @@ -76,13 +85,20 @@ final class DiffusionRepositoryEditBranchesController ->setNewValue($v_autoclose); } - id(new PhabricatorRepositoryEditor()) + $editor = id(new PhabricatorRepositoryEditor()) ->setContinueOnNoEffect(true) ->setContentSourceFromRequest($request) - ->setActor($viewer) - ->applyTransactions($repository, $xactions); + ->setActor($viewer); - return id(new AphrontRedirectResponse())->setURI($edit_uri); + try { + $editor->applyTransactions($repository, $xactions); + return id(new AphrontRedirectResponse())->setURI($edit_uri); + } catch (PhabricatorApplicationTransactionValidationException $ex) { + $validation_exception = $ex; + + $e_track = $validation_exception->getShortMessage($type_track); + $e_autoclose = $validation_exception->getShortMessage($type_autoclose); + } } $content = array(); @@ -97,44 +113,107 @@ final class DiffusionRepositoryEditBranchesController ->setObject($repository) ->execute(); + $rows = array(); + $rows[] = array( + array( + 'master', + ), + pht('Select only master.'), + ); + $rows[] = array( + array( + 'master', + 'develop', + 'release', + ), + pht('Select master, develop, and release.'), + ); + $rows[] = array( + array( + 'master', + 'regexp(/^release-/)', + ), + pht('Select master, and all branches which start with "release-".'), + ); + $rows[] = array( + array( + 'regexp(/^(?!temp-)/)', + ), + pht('Select all branches which do not start with "temp-".'), + ); + + foreach ($rows as $k => $row) { + $rows[$k][0] = phutil_tag( + 'pre', + array(), + implode("\n", $row[0])); + } + + $example_table = id(new AphrontTableView($rows)) + ->setHeaders( + array( + pht('Example'), + pht('Effect'), + )) + ->setColumnClasses( + array( + '', + 'wide', + )); + + $v_track = implode("\n", $v_track); + $v_autoclose = implode("\n", $v_autoclose); + $form = id(new AphrontFormView()) ->setUser($viewer) ->appendRemarkupInstructions( pht( - 'You can choose a **Default Branch** for viewing this repository.'. - "\n\n". - 'If you want to import only some branches into Diffusion, you can '. - 'list them in **Track Only**. Other branches will be ignored. If '. - 'you do not specify any branches, all branches are tracked.'. - "\n\n". - 'If you have **Autoclose** enabled, Phabricator can close tasks and '. - 'revisions when corresponding commits are pushed to the repository. '. - 'If you want to autoclose objects only when commits appear on '. - 'specific branches, you can list those branches in **Autoclose '. - 'Only**. By default, all branches autoclose objects.')) + 'You can choose a **Default Branch** for viewing this repository.')) ->appendChild( id(new AphrontFormTextControl()) ->setName('default') ->setLabel(pht('Default Branch')) - ->setValue($v_default) - ->setCaption( - pht('Example: %s', phutil_tag('tt', array(), 'develop')))) + ->setValue($v_default)) + ->appendRemarkupInstructions( + pht( + 'If you want to import only some branches into Diffusion, you can '. + 'list them in **Track Only**. Other branches will be ignored. If '. + 'you do not specify any branches, all branches are tracked.')); + + if (!$is_hg) { + $form->appendRemarkupInstructions( + pht( + 'If you have **Autoclose** enabled for this repository, Phabricator '. + 'can close tasks and revisions when corresponding commits are '. + 'pushed to the repository. If you want to autoclose objects only '. + 'when commits appear on specific branches, you can list those '. + 'branches in **Autoclose Only**. By default, all tracked branches '. + 'will autoclose objects.')); + } + + $form + ->appendRemarkupInstructions( + pht( + 'When specifying branches, you should enter one branch name per '. + 'line. You can use regular expressions to match branches by '. + 'wrapping an expression in `regexp(...)`. For example:')) ->appendChild( - id(new AphrontFormTextControl()) + id(new AphrontFormMarkupControl()) + ->setValue($example_table)) + ->appendChild( + id(new AphrontFormTextAreaControl()) ->setName('track') ->setLabel(pht('Track Only')) - ->setValue($v_track) - ->setCaption( - pht('Example: %s', phutil_tag('tt', array(), 'master, develop')))); + ->setError($e_track) + ->setValue($v_track)); if (!$is_hg) { $form->appendChild( - id(new AphrontFormTextControl()) + id(new AphrontFormTextAreaControl()) ->setName('autoclose') ->setLabel(pht('Autoclose Only')) - ->setValue($v_autoclose) - ->setCaption( - pht('Example: %s', phutil_tag('tt', array(), 'master, release')))); + ->setError($e_autoclose) + ->setValue($v_autoclose)); } $form->appendChild( @@ -143,6 +222,7 @@ final class DiffusionRepositoryEditBranchesController ->addCancelButton($edit_uri)); $form_box = id(new PHUIObjectBoxView()) + ->setValidationException($validation_exception) ->setHeaderText($title) ->setForm($form); @@ -156,4 +236,16 @@ final class DiffusionRepositoryEditBranchesController )); } + private function processBranches($string) { + $lines = phutil_split_lines($string, $retain_endings = false); + foreach ($lines as $key => $line) { + $lines[$key] = trim($line); + if (!strlen($lines[$key])) { + unset($lines[$key]); + } + } + + return array_values($lines); + } + } diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index 0116527267..5505f9c6c1 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -325,6 +325,52 @@ final class PhabricatorRepositoryEditor $errors = parent::validateTransaction($object, $type, $xactions); switch ($type) { + case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE: + case PhabricatorRepositoryTransaction::TYPE_TRACK_ONLY: + foreach ($xactions as $xaction) { + foreach ($xaction->getNewValue() as $pattern) { + // Check for invalid regular expressions. + $regexp = PhabricatorRepository::extractBranchRegexp($pattern); + if ($regexp !== null) { + $ok = @preg_match($regexp, ''); + if ($ok === false) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Expression "%s" is not a valid regular expression. Note '. + 'that you must include delimiters.', + $regexp), + $xaction); + $errors[] = $error; + continue; + } + } + + // Check for formatting mistakes like `regex(...)` instead of + // `regexp(...)`. + $matches = null; + if (preg_match('/^([^(]+)\\(.*\\)\z/', $pattern, $matches)) { + switch ($matches[1]) { + case 'regexp': + break; + default: + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Matching function "%s(...)" is not recognized. Valid '. + 'functions are: regexp(...).', + $matches[1]), + $xaction); + $errors[] = $error; + break; + } + } + } + } + break; + case PhabricatorRepositoryTransaction::TYPE_REMOTE_URI: foreach ($xactions as $xaction) { $new_uri = $xaction->getNewValue(); diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 41e387313f..10f225173f 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -577,16 +577,47 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $is_git = ($vcs == PhabricatorRepositoryType::REPOSITORY_TYPE_GIT); $use_filter = ($is_git); + if (!$use_filter) { + // If this VCS doesn't use filters, pass everything through. + return true; + } - if ($use_filter) { - $filter = $this->getDetail($filter_key, array()); - if ($filter && empty($filter[$branch])) { - return false; + + $filter = $this->getDetail($filter_key, array()); + + // If there's no filter set, let everything through. + if (!$filter) { + return true; + } + + // If this branch isn't literally named `regexp(...)`, and it's in the + // filter list, let it through. + if (isset($filter[$branch])) { + if (self::extractBranchRegexp($branch) === null) { + return true; } } - // By default, all branches pass. - return true; + // If the branch matches a regexp, let it through. + foreach ($filter as $pattern => $ignored) { + $regexp = self::extractBranchRegexp($pattern); + if ($regexp !== null) { + if (preg_match($regexp, $branch)) { + return true; + } + } + } + + // Nothing matched, so filter this branch out. + return false; + } + + public static function extractBranchRegexp($pattern) { + $matches = null; + if (preg_match('/^regexp\\((.*)\\)\z/', $pattern, $matches)) { + return $matches[1]; + } + return null; } public function shouldTrackBranch($branch) {