Rename "arc mark-committed" to "arc close-revision"

Summary:
  - Replace SVN-specific language with VCS-agnostic language.
  - Add new "arc close-revision", works exactly like "arc mark-committed" but with agnostic language.
  - Use status constants, not status strings.
  - Mark "arc mark-committed" deprecated.
  - Remove deprecated "arc merge".

Test Plan: Ran "arc mark-committed", "arc close-revision".

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: aran, Makinde

Maniphest Tasks: T909

Differential Revision: https://secure.phabricator.com/D2244

src/__phutil_library_map__.php

@@ -21,6 +21,7 @@ phutil_register_library_map(array(
     'ArcanistCapabilityNotSupportedException' => 'workflow/exception/notsupported',
     'ArcanistChooseInvalidRevisionException' => 'exception',
     'ArcanistChooseNoRevisionsException' => 'exception',
+    'ArcanistCloseRevisionWorkflow' => 'workflow/close-revision',
     'ArcanistCloseWorkflow' => 'workflow/close',
     'ArcanistCommitWorkflow' => 'workflow/commit',
     'ArcanistConduitLinter' => 'lint/linter/conduit',
@@ -70,7 +71,6 @@ phutil_register_library_map(array(
     'ArcanistMercurialAPI' => 'repository/api/mercurial',
     'ArcanistMercurialParser' => 'repository/parser/mercurial',
     'ArcanistMercurialParserTestCase' => 'repository/parser/mercurial/__tests__',
-    'ArcanistMergeWorkflow' => 'workflow/merge',
     'ArcanistNoEffectException' => 'exception/usage/noeffect',
     'ArcanistNoEngineException' => 'exception/usage/noengine',
     'ArcanistNoLintLinter' => 'lint/linter/nolint',
@@ -127,6 +127,7 @@ phutil_register_library_map(array(
     'ArcanistBranchWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistBundleTestCase' => 'ArcanistPhutilTestCase',
     'ArcanistCallConduitWorkflow' => 'ArcanistBaseWorkflow',
+    'ArcanistCloseRevisionWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistCloseWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistCommitWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistConduitLinter' => 'ArcanistLinter',
@@ -154,7 +155,6 @@ phutil_register_library_map(array(
     'ArcanistMarkCommittedWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistMercurialAPI' => 'ArcanistRepositoryAPI',
     'ArcanistMercurialParserTestCase' => 'ArcanistPhutilTestCase',
-    'ArcanistMergeWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistNoEffectException' => 'ArcanistUsageException',
     'ArcanistNoEngineException' => 'ArcanistUsageException',
     'ArcanistNoLintLinter' => 'ArcanistLinter',

src/differential/constants/revisionstatus/ArcanistDifferentialRevisionStatus.php

@@ -21,7 +21,8 @@ final class ArcanistDifferentialRevisionStatus {
   const NEEDS_REVIEW      = 0;
   const NEEDS_REVISION    = 1;
   const ACCEPTED          = 2;
-  const COMMITTED         = 3;
+  const COMMITTED         = 3; // TODO: Remove.
+  const CLOSED            = 3; // NOTE: Duplicate of "COMMITTED".
   const ABANDONED         = 4;
 
   public static function getNameForRevisionStatus($status) {
@@ -29,7 +30,7 @@ final class ArcanistDifferentialRevisionStatus {
       self::NEEDS_REVIEW      => 'Needs Review',
       self::NEEDS_REVISION    => 'Needs Revision',
       self::ACCEPTED          => 'Accepted',
-      self::COMMITTED         => 'Committed',
+      self::CLOSED            => 'Closed',
       self::ABANDONED         => 'Abandoned',
     );
 

src/workflow/amend/ArcanistAmendWorkflow.php

@@ -176,7 +176,7 @@ EOTEXT
       $repository_api->amendGitHeadCommit($message);
 
       $mark_workflow = $this->buildChildWorkflow(
-        'mark-committed',
+        'close-revision',
         array(
           '--finalize',
           $revision_id,

src/workflow/close-revision/ArcanistCloseRevisionWorkflow.php

@@ -0,0 +1,174 @@
+<?php
+
+/*
+ * Copyright 2012 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.
+ */
+
+/**
+ * Explicitly closes Differential revisions.
+ *
+ * @group workflow
+ */
+final class ArcanistCloseRevisionWorkflow extends ArcanistBaseWorkflow {
+
+  public function getCommandSynopses() {
+    return phutil_console_format(<<<EOTEXT
+      **close-revision** [__options__] __revision__
+EOTEXT
+      );
+  }
+
+  public function getCommandHelp() {
+    return phutil_console_format(<<<EOTEXT
+          Supports: git, hg, svn
+          Close a revision which has been committed (svn) or pushed (git, hg).
+          You should not normally need to do this: arc commit (svn), arc amend
+          (git), arc land (git), or repository tracking on the master remote
+          repository should do it for you. However, if these mechanisms have
+          failed for some reason you can use this command to manually change a
+          revision status from "Accepted" to "Closed".
+EOTEXT
+      );
+  }
+
+  public function getArguments() {
+    return array(
+      'finalize' => array(
+        'help' =>
+          "Close only if the repository is untracked and the revision is ".
+          "accepted. Continue even if the close can't happen. This is a soft ".
+          "version of 'close-revision' used by other workflows.",
+      ),
+      'quiet' => array(
+        'help' =>  'Do not print a success message.',
+      ),
+      '*' => 'revision',
+    );
+  }
+
+  public function requiresConduit() {
+    return true;
+  }
+
+  public function requiresAuthentication() {
+    return true;
+  }
+
+  public function requiresRepositoryAPI() {
+    // NOTE: Technically we only use this to generate the right message at
+    // the end, and you can even get the wrong message (e.g., if you run
+    // "arc close-revision D123" from a git repository, but D123 is an SVN
+    // revision). We could be smarter about this, but it's just display fluff.
+    return true;
+  }
+
+  public function run() {
+    $is_finalize = $this->getArgument('finalize');
+
+    $conduit = $this->getConduit();
+
+    $revision_list = $this->getArgument('revision', array());
+    if (!$revision_list) {
+      throw new ArcanistUsageException(
+        "close-revision requires a revision number.");
+    }
+    if (count($revision_list) != 1) {
+      throw new ArcanistUsageException(
+        "close-revision requires exactly one revision.");
+    }
+    $revision_id = reset($revision_list);
+    $revision_id = $this->normalizeRevisionID($revision_id);
+
+    $revision = null;
+    try {
+      $revision = $conduit->callMethodSynchronous(
+        'differential.getrevision',
+        array(
+          'revision_id' => $revision_id,
+        )
+      );
+    } catch (Exception $ex) {
+      if (!$is_finalize) {
+        throw new ArcanistUsageException(
+          "Revision D{$revision_id} does not exist."
+        );
+      }
+    }
+
+    $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
+    $status_closed   = ArcanistDifferentialRevisionStatus::CLOSED;
+
+    if (!$is_finalize && $revision['status'] != $status_accepted) {
+      throw new ArcanistUsageException(
+        "Revision D{$revision_id} can not be closed. You can only close ".
+        "revisions which have been 'accepted'.");
+    }
+
+    if ($revision) {
+      if (!$is_finalize && $revision['authorPHID'] != $this->getUserPHID()) {
+        $prompt = "You are not the author of revision D{$revision_id}, ".
+          'are you sure you want to close it?';
+        if (!phutil_console_confirm($prompt)) {
+          throw new ArcanistUserAbortException();
+        }
+      }
+
+      $actually_close = true;
+      if ($is_finalize) {
+        $project_info = $conduit->callMethodSynchronous(
+          'arcanist.projectinfo',
+          array(
+            'name' => $this->getWorkingCopy()->getProjectID(),
+          ));
+        if ($project_info['tracked'] ||
+            $revision['status'] != $status_accepted) {
+          $actually_close = false;
+        }
+      }
+      if ($actually_close) {
+        $revision_name = $revision['title'];
+
+        echo "Closing revision D{$revision_id} '{$revision_name}'...\n";
+
+        $conduit->callMethodSynchronous(
+          'differential.markcommitted',
+          array(
+            'revision_id' => $revision_id,
+          ));
+      }
+    }
+
+    $status = $revision['status'];
+    if ($status == $status_accepted || $status == $status_closed) {
+      // If this has already been attached to commits, don't show the
+      // "you can push this commit" message since we know it's been pushed
+      // already.
+      $is_finalized = empty($revision['commits']);
+    } else {
+      $is_finalized = false;
+    }
+
+    if (!$this->getArgument('quiet')) {
+      if ($is_finalized) {
+        $message = $this->getRepositoryAPI()->getFinalizedRevisionMessage();
+        echo phutil_console_wrap($message)."\n";
+      } else {
+        echo "Done.\n";
+      }
+    }
+
+    return 0;
+  }
+}

src/workflow/close-revision/__init__.php

@@ -6,10 +6,12 @@
 
 
 
+phutil_require_module('arcanist', 'differential/constants/revisionstatus');
 phutil_require_module('arcanist', 'exception/usage');
+phutil_require_module('arcanist', 'exception/usage/userabort');
 phutil_require_module('arcanist', 'workflow/base');
 
 phutil_require_module('phutil', 'console');
 
 
-phutil_require_source('ArcanistMergeWorkflow.php');
+phutil_require_source('ArcanistCloseRevisionWorkflow.php');

src/workflow/commit/ArcanistCommitWorkflow.php

@@ -169,7 +169,7 @@ EOTEXT
     }
 
     $mark_workflow = $this->buildChildWorkflow(
-      'mark-committed',
+      'close-revision',
       array(
         '--finalize',
         $revision_id,

src/workflow/land/ArcanistLandWorkflow.php

@@ -290,7 +290,7 @@ EOTEXT
       }
 
       $mark_workflow = $this->buildChildWorkflow(
-        'mark-committed',
+        'close-revision',
         array(
           '--finalize',
           '--quiet',

src/workflow/mark-committed/ArcanistMarkCommittedWorkflow.php

@@ -17,44 +17,27 @@
  */
 
 /**
- * Explicitly marks Differential revisions as "Committed".
- *
  * @group workflow
  */
 final class ArcanistMarkCommittedWorkflow extends ArcanistBaseWorkflow {
 
   public function getCommandSynopses() {
     return phutil_console_format(<<<EOTEXT
-      **mark-committed** __revision__
+      **mark-committed** (DEPRECATED)
 EOTEXT
       );
   }
 
   public function getCommandHelp() {
     return phutil_console_format(<<<EOTEXT
-          Supports: git, svn
+          Deprecated. Moved to "close-revision".
-          Manually mark a revision as committed. You should not normally need
-          to do this; arc commit (svn), arc amend (git), arc merge (git, hg) or
-          repository tracking on the master remote repository should do it for
-          you. However, if these mechanisms have failed for some reason you can
-          use this command to manually change a revision status from "Accepted"
-          to "Committed".
 EOTEXT
       );
   }
 
   public function getArguments() {
     return array(
-      'finalize' => array(
+      '*' => 'deprecated',
-        'help' =>
-          "Mark committed only if the repository is untracked and the ".
-          "revision is accepted. Continue even if the mark can't happen. This ".
-          "is a soft version of 'mark-committed' used by other workflows.",
-      ),
-      'quiet' => array(
-        'help' =>  'Do not print a success message.',
-      ),
-      '*' => 'revision',
     );
   }
 
@@ -67,106 +50,12 @@ EOTEXT
   }
 
   public function requiresRepositoryAPI() {
-    // NOTE: Technically we only use this to generate the right message at
-    // the end, and you can even get the wrong message (e.g., if you run
-    // "arc mark-committed D123" from a git repository, but D123 is an SVN
-    // revision). We could be smarter about this, but it's just display fluff.
     return true;
   }
 
   public function run() {
-    $is_finalize = $this->getArgument('finalize');
+    throw new ArcanistUsageException(
-
+      "'arc mark-committed' is now 'arc close-revision' (because ".
-    $conduit = $this->getConduit();
+      "'mark-committed' only really made sense under SVN).");
-
-    $revision_list = $this->getArgument('revision', array());
-    if (!$revision_list) {
-      throw new ArcanistUsageException(
-        "mark-committed requires a revision number.");
-    }
-    if (count($revision_list) != 1) {
-      throw new ArcanistUsageException(
-        "mark-committed requires exactly one revision.");
-    }
-    $revision_id = reset($revision_list);
-    $revision_id = $this->normalizeRevisionID($revision_id);
-
-    $revision = null;
-    try {
-      $revision = $conduit->callMethodSynchronous(
-        'differential.getrevision',
-        array(
-          'revision_id' => $revision_id,
-        )
-      );
-    } catch (Exception $ex) {
-      if (!$is_finalize) {
-        throw new ArcanistUsageException(
-          "Revision D{$revision_id} does not exist."
-        );
-      }
-    }
-
-    if (!$is_finalize && $revision['statusName'] != 'Accepted') {
-      throw new ArcanistUsageException(
-        "Revision D{$revision_id} is not committable. You can only mark ".
-        "revisions which have been 'accepted' as committed."
-      );
-    }
-
-    if ($revision) {
-      if (!$is_finalize && $revision['authorPHID'] != $this->getUserPHID()) {
-        $prompt = "You are not the author of revision D{$revision_id}, ".
-          'are you sure you want to mark it committed?';
-        if (!phutil_console_confirm($prompt)) {
-          throw new ArcanistUserAbortException();
-        }
-      }
-
-      $actually_mark = true;
-      if ($is_finalize) {
-        $project_info = $conduit->callMethodSynchronous(
-          'arcanist.projectinfo',
-          array(
-            'name' => $this->getWorkingCopy()->getProjectID(),
-          ));
-        if ($project_info['tracked'] || $revision['statusName'] != 'Accepted') {
-          $actually_mark = false;
-        }
-      }
-      if ($actually_mark) {
-        $revision_name = $revision['title'];
-
-        echo "Marking revision D{$revision_id} '{$revision_name}' ".
-             "committed...\n";
-
-        $conduit->callMethodSynchronous(
-          'differential.markcommitted',
-          array(
-            'revision_id' => $revision_id,
-          ));
-      }
-    }
-
-    $status = $revision['statusName'];
-    if ($status == 'Accepted' || $status == 'Committed') {
-      // If this has already been attached to commits, don't show the
-      // "you can push this commit" message since we know it's been committed
-      // already.
-      $is_finalized = empty($revision['commits']);
-    } else {
-      $is_finalized = false;
-    }
-
-    if (!$this->getArgument('quiet')) {
-      if ($is_finalized) {
-        $message = $this->getRepositoryAPI()->getFinalizedRevisionMessage();
-        echo phutil_console_wrap($message)."\n";
-      } else {
-        echo "Done.\n";
-      }
-    }
-
-    return 0;
   }
 }

src/workflow/mark-committed/__init__.php

@@ -7,7 +7,6 @@
 
 
 phutil_require_module('arcanist', 'exception/usage');
-phutil_require_module('arcanist', 'exception/usage/userabort');
 phutil_require_module('arcanist', 'workflow/base');
 
 phutil_require_module('phutil', 'console');

src/workflow/merge/ArcanistMergeWorkflow.php

@@ -1,65 +0,0 @@
-<?php
-
-/*
- * Copyright 2012 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.
- */
-
-/**
- * Deprecated.
- *
- * TODO: Remove soon, once git users have had a chance to see the "use land
- * instead" message.
- *
- * @group workflow
- */
-final class ArcanistMergeWorkflow extends ArcanistBaseWorkflow {
-
-  public function getCommandSynopses() {
-    return phutil_console_format(<<<EOTEXT
-      **merge**
-EOTEXT
-      );
-  }
-
-  public function getCommandHelp() {
-    return phutil_console_format(<<<EOTEXT
-          Deprecated.
-EOTEXT
-      );
-  }
-
-  public function requiresRepositoryAPI() {
-    return true;
-  }
-
-  public function getArguments() {
-    return array(
-      '*' => 'ignored',
-    );
-  }
-
-  public function run() {
-    $repository_api = $this->getRepositoryAPI();
-
-    if ($repository_api instanceof ArcanistGitAPI) {
-      throw new ArcanistUsageException(
-        "'arc merge' no longer supports git. Use ".
-        "'arc land --keep-branch --hold --merge <feature_branch>' instead.");
-    }
-
-    throw new ArcanistUsageException('arc merge is no longer supported.');
-  }
-
-}