From 9d8b5481aec52b39c6790ccfd6babd9059525a74 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 20 Dec 2011 15:40:24 -0800 Subject: [PATCH] Emit full URIs to identify Differential revisions Summary: - Previously, used IDs like "33" to match a commit to a Differential revision. This has a namespacing problem because we now have an arbitrarily large number of Phabricator installs in the world, and they may want to track commits from other installs. - In Differential, parse raw IDs or full URIs. Emit only full URIs. - In Repositories, parse only full URIs. - This might cause a few commits to not be picked up in rare circumstances. Users can fix them with "arc mark-committed". This should be exceedingly rare because of hash matching. - There are some caveats for reparsing older repositories, see comments inline. I don't think there's much broad impact here. Test Plan: - Created a new revision, got a full URI. - Updated revision, worked correctly. - Ran unit tests. - Monkeyed with "Differential Revision" field. - Reviewers: btrahan, jungejason Reviewers: btrahan, jungejason Reviewed By: jungejason CC: aran, epriestley, jungejason Maniphest Tasks: T54, T692 Differential Revision: 1250 --- src/__phutil_library_map__.php | 2 + ...fferentialRevisionIDFieldSpecification.php | 39 ++++++++++++++- .../specification/revisionid/__init__.php | 5 ++ ...ferentialRevisionIDFieldParserTestCase.php | 48 +++++++++++++++++++ .../revisionid/__tests__/__init__.php | 14 ++++++ ...sitoryDefaultCommitMessageDetailParser.php | 13 ++++- .../repository/parser/default/__init__.php | 1 + 7 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 src/applications/differential/field/specification/revisionid/__tests__/DifferentialRevisionIDFieldParserTestCase.php create mode 100644 src/applications/differential/field/specification/revisionid/__tests__/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6f68cf1bc3..3c32b2cb86 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -224,6 +224,7 @@ phutil_register_library_map(array( 'DifferentialRevisionEditController' => 'applications/differential/controller/revisionedit', 'DifferentialRevisionEditor' => 'applications/differential/editor/revision', 'DifferentialRevisionHash' => 'applications/differential/constants/revisionhash', + 'DifferentialRevisionIDFieldParserTestCase' => 'applications/differential/field/specification/revisionid/__tests__', 'DifferentialRevisionIDFieldSpecification' => 'applications/differential/field/specification/revisionid', 'DifferentialRevisionListController' => 'applications/differential/controller/revisionlist', 'DifferentialRevisionListData' => 'applications/differential/data/revisionlist', @@ -952,6 +953,7 @@ phutil_register_library_map(array( 'DifferentialRevisionCommentView' => 'AphrontView', 'DifferentialRevisionDetailView' => 'AphrontView', 'DifferentialRevisionEditController' => 'DifferentialController', + 'DifferentialRevisionIDFieldParserTestCase' => 'PhabricatorTestCase', 'DifferentialRevisionIDFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialRevisionListController' => 'DifferentialController', 'DifferentialRevisionListView' => 'AphrontView', diff --git a/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php b/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php index 645400a83c..21fd05382e 100644 --- a/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php +++ b/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php @@ -47,11 +47,46 @@ final class DifferentialRevisionIDFieldSpecification } public function renderValueForCommitMessage($is_edit) { - return $this->id; + return PhabricatorEnv::getProductionURI('/D'.$this->id); } public function parseValueFromCommitMessage($value) { - return $value; + $rev = trim($value); + + if (!strlen($rev)) { + return null; + } + + if (is_numeric($rev)) { + // TODO: Eventually, remove support for bare revision numbers. + return (int)$rev; + } + + $rev = self::parseRevisionIDFromURI($rev); + if ($rev !== null) { + return $rev; + } + + $example_uri = PhabricatorEnv::getProductionURI('/D123'); + throw new DifferentialFieldParseException( + "Commit references invalid 'Differential Revision'. Expected a ". + "Phabricator URI like '{$example_uri}', got '{$value}'."); + } + + public static function parseRevisionIDFromURI($uri) { + $path = id(new PhutilURI($uri))->getPath(); + + $matches = null; + if (preg_match('#^/D(\d+)$#', $path, $matches)) { + $id = (int)$matches[1]; + // Make sure the URI is the same as our URI. Basically, we want to ignore + // commits from other Phabricator installs. + if ($uri == PhabricatorEnv::getProductionURI('/D'.$id)) { + return $id; + } + } + + return null; } } diff --git a/src/applications/differential/field/specification/revisionid/__init__.php b/src/applications/differential/field/specification/revisionid/__init__.php index 1b3dc16ecc..0cc935b8b9 100644 --- a/src/applications/differential/field/specification/revisionid/__init__.php +++ b/src/applications/differential/field/specification/revisionid/__init__.php @@ -6,7 +6,12 @@ +phutil_require_module('phabricator', 'applications/differential/field/exception/parse'); phutil_require_module('phabricator', 'applications/differential/field/specification/base'); +phutil_require_module('phabricator', 'infrastructure/env'); + +phutil_require_module('phutil', 'parser/uri'); +phutil_require_module('phutil', 'utils'); phutil_require_source('DifferentialRevisionIDFieldSpecification.php'); diff --git a/src/applications/differential/field/specification/revisionid/__tests__/DifferentialRevisionIDFieldParserTestCase.php b/src/applications/differential/field/specification/revisionid/__tests__/DifferentialRevisionIDFieldParserTestCase.php new file mode 100644 index 0000000000..f2bb9e3d32 --- /dev/null +++ b/src/applications/differential/field/specification/revisionid/__tests__/DifferentialRevisionIDFieldParserTestCase.php @@ -0,0 +1,48 @@ +assertEqual( + null, + $this->parse('123')); + + $this->assertEqual( + null, + $this->parse('D123')); + + // NOTE: We expect foreign, validly-formatted URIs to be ignored. + $this->assertEqual( + null, + $this->parse('http://phabricator.example.com/D123')); + + $this->assertEqual( + 123, + $this->parse(PhabricatorEnv::getProductionURI('/D123'))); + + } + + private function parse($value) { + return DifferentialRevisionIDFieldSpecification::parseRevisionIDFromURI( + $value); + } + +} diff --git a/src/applications/differential/field/specification/revisionid/__tests__/__init__.php b/src/applications/differential/field/specification/revisionid/__tests__/__init__.php new file mode 100644 index 0000000000..9395db7acd --- /dev/null +++ b/src/applications/differential/field/specification/revisionid/__tests__/__init__.php @@ -0,0 +1,14 @@ +getCommitMessage(); $author_name = $data->getAuthorName(); + // TODO: Some day, it would be good to drive all of this via + // DifferentialFieldSpecification configuration directly. + $match = null; if (preg_match( @@ -34,7 +37,15 @@ class PhabricatorRepositoryDefaultCommitMessageDetailParser $message, $match)) { - $id = (int)$match[1]; + // NOTE: We now accept ONLY full URIs because if we accept numeric IDs + // then anyone importing the Phabricator repository will have their + // first few thousand revisions marked committed. This does mean that + // some older revisions won't re-parse correctly, but that shouldn't + // really affect anyone. If necesary, an install can extend the parser + // and restore the older, more-liberal parsing fairly easily. + + $id = DifferentialRevisionIDFieldSpecification::parseRevisionIDFromURI( + $match[1]); if ($id) { $details['differential.revisionID'] = (int)$match[1]; $revision = id(new DifferentialRevision())->load($id); diff --git a/src/applications/repository/parser/default/__init__.php b/src/applications/repository/parser/default/__init__.php index b3a36da8fa..158a330f3e 100644 --- a/src/applications/repository/parser/default/__init__.php +++ b/src/applications/repository/parser/default/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/differential/field/specification/revisionid'); phutil_require_module('phabricator', 'applications/differential/storage/revision'); phutil_require_module('phabricator', 'applications/repository/parser/base');