diff --git a/resources/sql/autopatches/20160110.repo.01.slug.sql b/resources/sql/autopatches/20160110.repo.01.slug.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20160110.repo.01.slug.sql @@ -0,0 +1,5 @@ +ALTER TABLE {$NAMESPACE}_repository.repository + ADD repositorySlug VARCHAR(64) COLLATE {$COLLATE_SORT}; + +ALTER TABLE {$NAMESPACE}_repository.repository + ADD UNIQUE KEY `key_slug` (repositorySlug); diff --git a/resources/sql/autopatches/20160110.repo.02.slug.php b/resources/sql/autopatches/20160110.repo.02.slug.php new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20160110.repo.02.slug.php @@ -0,0 +1,49 @@ +establishConnection('w'); + +foreach (new LiskMigrationIterator($table) as $repository) { + $slug = $repository->getRepositorySlug(); + + if ($slug !== null) { + continue; + } + + $clone_name = $repository->getDetail('clone-name'); + + if (!strlen($clone_name)) { + continue; + } + + if (!PhabricatorRepository::isValidRepositorySlug($clone_name)) { + echo tsprintf( + "%s\n", + pht( + 'Repository "%s" has a "Clone/Checkout As" name which is no longer '. + 'valid ("%s"). You can edit the repository to give it a new, valid '. + 'short name.', + $repository->getDisplayName(), + $clone_name)); + continue; + } + + try { + queryfx( + $conn_w, + 'UPDATE %T SET repositorySlug = %s WHERE id = %d', + $table->getTableName(), + $clone_name, + $repository->getID()); + } catch (AphrontDuplicateKeyQueryException $ex) { + echo tsprintf( + "%s\n", + pht( + 'Repository "%s" has a duplicate "Clone/Checkout As" name ("%s"). '. + 'Each name must now be unique. You can edit the repository to give '. + 'it a new, unique short name.', + $repository->getDisplayName(), + $clone_name)); + } + +} diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditBasicController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditBasicController.php --- a/src/applications/diffusion/controller/DiffusionRepositoryEditBasicController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditBasicController.php @@ -17,13 +17,15 @@ $v_name = $repository->getName(); $v_desc = $repository->getDetail('description'); - $v_clone_name = $repository->getDetail('clone-name'); + $v_clone_name = $repository->getRepositorySlug(); $v_projects = PhabricatorEdgeQuery::loadDestinationPHIDs( $repository->getPHID(), PhabricatorProjectObjectHasProjectEdgeType::EDGECONST); $e_name = true; + $e_slug = null; $errors = array(); + $validation_exception = null; if ($request->isFormPost()) { $v_name = $request->getStr('name'); $v_desc = $request->getStr('description'); @@ -71,13 +73,20 @@ '=' => array_fuse($v_projects), )); - 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_slug = $ex->getShortMessage($type_clone_name); + } } } @@ -102,6 +111,7 @@ ->setName('cloneName') ->setLabel(pht('Clone/Checkout As')) ->setValue($v_clone_name) + ->setError($e_slug) ->setCaption( pht( 'Optional directory name to use when cloning or checking out '. @@ -130,6 +140,7 @@ $object_box = id(new PHUIObjectBoxView()) ->setHeaderText($title) + ->setValidationException($validation_exception) ->setForm($form) ->setFormErrors($errors); diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php --- a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php @@ -286,7 +286,7 @@ $view->addProperty(pht('Type'), $type); $view->addProperty(pht('Callsign'), $repository->getCallsign()); - $clone_name = $repository->getDetail('clone-name'); + $clone_name = $repository->getRepositorySlug(); if ($repository->isHosted()) { $view->addProperty( diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -99,7 +99,7 @@ case PhabricatorRepositoryTransaction::TYPE_DANGEROUS: return $object->shouldAllowDangerousChanges(); case PhabricatorRepositoryTransaction::TYPE_CLONE_NAME: - return $object->getDetail('clone-name'); + return $object->getRepositorySlug(); case PhabricatorRepositoryTransaction::TYPE_SERVICE: return $object->getAlmanacServicePHID(); case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE: @@ -141,13 +141,18 @@ case PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY: case PhabricatorRepositoryTransaction::TYPE_CREDENTIAL: case PhabricatorRepositoryTransaction::TYPE_DANGEROUS: - case PhabricatorRepositoryTransaction::TYPE_CLONE_NAME: case PhabricatorRepositoryTransaction::TYPE_SERVICE: case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE: case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES: case PhabricatorRepositoryTransaction::TYPE_STAGING_URI: case PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS: return $xaction->getNewValue(); + case PhabricatorRepositoryTransaction::TYPE_CLONE_NAME: + $name = $xaction->getNewValue(); + if (strlen($name)) { + return $name; + } + return null; case PhabricatorRepositoryTransaction::TYPE_NOTIFY: case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE: return (int)$xaction->getNewValue(); @@ -216,7 +221,7 @@ $object->setDetail('allow-dangerous-changes', $xaction->getNewValue()); return; case PhabricatorRepositoryTransaction::TYPE_CLONE_NAME: - $object->setDetail('clone-name', $xaction->getNewValue()); + $object->setRepositorySlug($xaction->getNewValue()); return; case PhabricatorRepositoryTransaction::TYPE_SERVICE: $object->setAlmanacServicePHID($xaction->getNewValue()); @@ -448,9 +453,69 @@ } } break; + + case PhabricatorRepositoryTransaction::TYPE_CLONE_NAME: + foreach ($xactions as $xaction) { + $old = $xaction->getOldValue(); + $new = $xaction->getNewValue(); + + if (!strlen($new)) { + continue; + } + + if ($new === $old) { + continue; + } + + try { + PhabricatorRepository::asssertValidRepositorySlug($new); + } catch (Exception $ex) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + $ex->getMessage(), + $xaction); + continue; + } + + $other = id(new PhabricatorRepositoryQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withSlugs(array($new)) + ->executeOne(); + if ($other && ($other->getID() !== $object->getID())) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Duplicate'), + pht( + 'The selected repository short name is already in use by '. + 'another repository. Choose a unique short name.'), + $xaction); + continue; + } + } + break; + } return $errors; } + protected function didCatchDuplicateKeyException( + PhabricatorLiskDAO $object, + array $xactions, + Exception $ex) { + + $errors = array(); + + $errors[] = new PhabricatorApplicationTransactionValidationError( + null, + pht('Invalid'), + pht( + 'The chosen callsign or repository short name is already in '. + 'use by another repository.'), + null); + + throw new PhabricatorApplicationTransactionValidationException($errors); + } + } diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php @@ -11,6 +11,7 @@ private $nameContains; private $remoteURIs; private $datasourceQuery; + private $slugs; private $numericIdentifiers; private $callsignIdentifiers; @@ -114,6 +115,11 @@ return $this; } + public function withSlugs(array $slugs) { + $this->slugs = $slugs; + return $this; + } + public function needCommitCounts($need_counts) { $this->needCommitCounts = $need_counts; return $this; @@ -564,6 +570,13 @@ $callsign); } + if ($this->slugs !== null) { + $where[] = qsprintf( + $conn, + 'r.repositorySlug IN (%Ls)', + $this->slugs); + } + return $where; } 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 @@ -46,6 +46,7 @@ protected $name; protected $callsign; + protected $repositorySlug; protected $uuid; protected $viewPolicy; protected $editPolicy; @@ -93,6 +94,7 @@ self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'sort255', 'callsign' => 'sort32', + 'repositorySlug' => 'sort64?', 'versionControlSystem' => 'text32', 'uuid' => 'text64?', 'pushPolicy' => 'policy', @@ -100,11 +102,6 @@ 'almanacServicePHID' => 'phid?', ), self::CONFIG_KEY_SCHEMA => array( - 'key_phid' => null, - 'phid' => array( - 'columns' => array('phid'), - 'unique' => true, - ), 'callsign' => array( 'columns' => array('callsign'), 'unique' => true, @@ -115,6 +112,10 @@ 'key_vcs' => array( 'columns' => array('versionControlSystem'), ), + 'key_slug' => array( + 'columns' => array('repositorySlug'), + 'unique' => true, + ), ), ) + parent::getConfiguration(); } @@ -297,7 +298,7 @@ * @return string */ public function getCloneName() { - $name = $this->getDetail('clone-name'); + $name = $this->getRepositorySlug(); // Make some reasonable effort to produce reasonable default directory // names from repository names. @@ -314,6 +315,82 @@ return $name; } + public static function isValidRepositorySlug($slug) { + try { + self::asssertValidRepositorySlug($slug); + return true; + } catch (Exception $ex) { + return false; + } + } + + public static function asssertValidRepositorySlug($slug) { + if (!strlen($slug)) { + throw new Exception( + pht( + 'The empty string is not a valid repository short name. '. + 'Repository short names must be at least one character long.')); + } + + if (strlen($slug) > 64) { + throw new Exception( + pht( + 'The name "%s" is not a valid repository short name. Repository '. + 'short names must not be longer than 64 characters.', + $slug)); + } + + if (preg_match('/[^a-zA-Z0-9._-]/', $slug)) { + throw new Exception( + pht( + 'The name "%s" is not a valid repository short name. Repository '. + 'short names may only contain letters, numbers, periods, hyphens '. + 'and underscores.', + $slug)); + } + + if (!preg_match('/^[a-zA-Z0-9]/', $slug)) { + throw new Exception( + pht( + 'The name "%s" is not a valid repository short name. Repository '. + 'short names must begin with a letter or number.', + $slug)); + } + + if (!preg_match('/[a-zA-Z0-9]\z/', $slug)) { + throw new Exception( + pht( + 'The name "%s" is not a valid repository short name. Repository '. + 'short names must end with a letter or number.', + $slug)); + } + + if (preg_match('/__|--|\\.\\./', $slug)) { + throw new Exception( + pht( + 'The name "%s" is not a valid repository short name. Repository '. + 'short names must not contain multiple consecutive underscores, '. + 'hyphens, or periods.', + $slug)); + } + + if (preg_match('/^[A-Z]+\z/', $slug)) { + throw new Exception( + pht( + 'The name "%s" is not a valid repository short name. Repository '. + 'short names may not contain only uppercase letters.', + $slug)); + } + + if (preg_match('/^\d+\z/', $slug)) { + throw new Exception( + pht( + 'The name "%s" is not a valid repository short name. Repository '. + 'short names may not contain only numbers.', + $slug)); + } + } + /* -( Remote Command Execution )------------------------------------------- */ diff --git a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php --- a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php +++ b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php @@ -152,4 +152,68 @@ } } + public function testRepositoryShortNameValidation() { + $good = array( + 'sensible-repository', + 'AReasonableName', + 'ACRONYM-project', + 'sol-123', + '46-helixes', + 'node.io', + 'internet.com', + 'www.internet-site.com.repository', + 'with_under-scores', + + // Can't win them all. + 'A-_._-_._-_._-_._-_._-_._-1', + + // 64-character names are fine. + str_repeat('a', 64), + ); + + $poor = array( + '', + '1', + '.', + '-_-', + 'AAAA', + '..', + 'a/b', + '../../etc/passwd', + '/', + '!', + '@', + 'ca$hmoney', + 'repo with spaces', + 'hyphen-', + '-ated', + '_underscores_', + 'yes!', + + // 65-character names are no good. + str_repeat('a', 65), + ); + + foreach ($good as $nice_name) { + $actual = PhabricatorRepository::isValidRepositorySlug($nice_name); + $this->assertEqual( + true, + $actual, + pht( + 'Expected "%s" to be a valid repository short name.', + $nice_name)); + } + + foreach ($poor as $poor_name) { + $actual = PhabricatorRepository::isValidRepositorySlug($poor_name); + $this->assertEqual( + false, + $actual, + pht( + 'Expected "%s" to be rejected as an invalid repository '. + 'short name.', + $poor_name)); + } + } + }