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

@@ -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',

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;
   }
 
 }

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');

src/applications/differential/field/specification/revisionid/__tests__/DifferentialRevisionIDFieldParserTestCase.php

@@ -0,0 +1,48 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialRevisionIDFieldParserTestCase
+  extends PhabricatorTestCase {
+
+  public function testFieldParser() {
+
+    $this->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);
+  }
+
+}

src/applications/differential/field/specification/revisionid/__tests__/__init__.php

@@ -0,0 +1,14 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/revisionid');
+phutil_require_module('phabricator', 'infrastructure/env');
+phutil_require_module('phabricator', 'infrastructure/testing/testcase');
+
+
+phutil_require_source('DifferentialRevisionIDFieldParserTestCase.php');

src/applications/repository/parser/default/PhabricatorRepositoryDefaultCommitMessageDetailParser.php

@@ -27,6 +27,9 @@ class PhabricatorRepositoryDefaultCommitMessageDetailParser
     $message = $data->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);

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');