Page MenuHomePhabricator
Files F15462687

D14986.id.diff
No OneTemporary
Actions

D14986.id.diff
View Options

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 @@
+<?php
+
+$table = new PhabricatorRepository();
+$conn_w = $table->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));
+ }
+ }
+
}

File Metadata

Mime Type
text/plain
Expires
Wed, Apr 2, 4:50 PM (1 w, 2 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7381573
Default Alt Text
D14986.id.diff (15 KB)

Event Timeline

Phabricator · Built by Phacility