Automatically detect when to mark revisions committed

Summary:
See D945. We have this kludgy "remote_hooks_installed" mess right now, but we
have enough information on the server nowadays to figure this out without it.

Also reduce code duplication by sharing the "mark-committed" workflow.

This causes "arc merge" to effect commit marks.

Test Plan:
In Git, Mercurial and SVN working copies ran like a million
amend/merge/commit/mark-committed commands with and without --finalize in
various states of revision completion.

This change is really hard to exhaustively test because of the number of
combinations of VCS, revision state, command, command flags, repository state
and tracking state. All the reasonable tests I could come up with worked
correctly, though.

Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran

Reviewed By: jungejason

CC: aran, jungejason

Differential Revision: 967

.arcconfig

@@ -3,7 +3,6 @@
   "conduit_uri" : "https://secure.phabricator.com/",
   "lint_engine" : "PhutilLintEngine",
   "unit_engine" : "PhutilUnitTestEngine",
-  "remote_hooks_installed" : true,
   "copyright_holder" : "Facebook, Inc.",
   "phutil_libraries" : {
     "arcanist" : "src/"

src/repository/api/subversion/ArcanistSubversionAPI.php

@@ -490,4 +490,10 @@ EODIFF;
     return false;
   }
 
+  public function getFinalizedRevisionMessage() {
+    // In other VCSes we give push instructions here, but it never makes sense
+    // in SVN.
+    return "Done.";
+  }
+
 }

src/workflow/amend/ArcanistAmendWorkflow.php

@@ -126,28 +126,13 @@ EOTEXT
       $repository_api->amendGitHeadCommit($message);
       echo "Amended commit message.\n";
 
-      $working_copy = $this->getWorkingCopy();
-      $remote_hooks = $working_copy->getConfig('remote_hooks_installed', false);
-      if (!$remote_hooks) {
-        echo "According to .arcconfig, remote commit hooks are not installed ".
-             "for this project, so the revision will be marked committed now. ".
-             "Consult the documentation for instructions on installing hooks.".
-             "\n\n";
-        $mark_workflow = $this->buildChildWorkflow(
-          'mark-committed',
-          array($revision_id));
-        $mark_workflow->run();
-      }
-
-      $remote_message = ArcanistDifferentialCommitMessage::newFromRawCorpus(
-        $message);
-      $remote_message->pullDataFromConduit($conduit);
-      if ($remote_message->getFieldValue('reviewedByGUIDs') ||
-          $remote_message->getFieldValue('reviewedByPHIDs')) {
-        echo phutil_console_wrap(
-          "You may now push this commit upstream, as appropriate (e.g. with ".
-          "'git push', or 'git svn dcommit', or by printing and faxing it).\n");
-      }
+      $mark_workflow = $this->buildChildWorkflow(
+        'mark-committed',
+        array(
+          '--finalize',
+          $revision_id,
+        ));
+      $mark_workflow->run();
     }
 
     return 0;

src/workflow/commit/ArcanistCommitWorkflow.php

@@ -129,18 +129,13 @@ EOTEXT
       throw new Exception("Executing 'svn commit' failed!");
     }
 
-    $working_copy = $this->getWorkingCopy();
-    $remote_hooks = $working_copy->getConfig('remote_hooks_installed', false);
-    if (!$remote_hooks) {
-      echo "According to .arcconfig, remote commit hooks are not installed ".
-           "for this project, so the revision will be marked committed now. ".
-           "Consult the documentation for instructions on installing hooks.".
-           "\n\n";
-      $mark_workflow = $this->buildChildWorkflow(
-        'mark-committed',
-        array($revision_id));
-      $mark_workflow->run();
-    }
+    $mark_workflow = $this->buildChildWorkflow(
+      'mark-committed',
+      array(
+        '--finalize',
+        $revision_id,
+      ));
+    $mark_workflow->run();
 
     return $err;
   }

src/workflow/git-hook-pre-receive/ArcanistGitHookPreReceiveWorkflow.php

@@ -57,19 +57,6 @@ EOTEXT
         ".arcconfig.");
     }
 
-    if (!$working_copy->getConfig('remote_hooks_installed')) {
-      echo phutil_console_wrap(
-        "\n".
-        "NOTE: Arcanist is installed as a git pre-receive hook in the git ".
-        "remote you are pushing to, but the project's '.arcconfig' does not ".
-        "have the 'remote_hooks_installed' flag set. Until you set the flag, ".
-        "some code will run needlessly in both the local and remote, and ".
-        "revisions will be marked 'committed' in Differential when they are ".
-        "amended rather than when they are actually pushed to the remote ".
-        "origin.".
-        "\n\n");
-    }
-
     // Git repositories have special rules in pre-receive hooks. We need to
     // construct the API against the .git directory instead of the project
     // root or commands don't work properly.

src/workflow/mark-committed/ArcanistMarkCommittedWorkflow.php

@@ -28,16 +28,23 @@ class ArcanistMarkCommittedWorkflow extends ArcanistBaseWorkflow {
       **mark-committed** __revision__
           Supports: git, svn
           Manually mark a revision as committed. You should not normally need
-          to do this; arc commit (svn), arc amend (git) or commit hooks in 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".
+          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(
+        '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.",
+      ),
       '*' => 'revision',
     );
   }
@@ -50,7 +57,16 @@ EOTEXT
     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 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');
 
     $conduit = $this->getConduit();
 
@@ -73,6 +89,7 @@ EOTEXT
         ),
       ));
 
+    $revision = null;
     try {
       $revision_id = reset($revision_list);
       $revision_id = $this->normalizeRevisionID($revision_id);
@@ -80,23 +97,61 @@ EOTEXT
         $revision_data,
         $revision_id);
     } catch (ArcanistChooseInvalidRevisionException $ex) {
-      throw new ArcanistUsageException(
-        "Revision D{$revision_id} is not committable. You can only mark ".
-        "revisions which have been 'accepted' as committed.");
+      if (!$is_finalize) {
+        throw new ArcanistUsageException(
+          "Revision D{$revision_id} is not committable. You can only mark ".
+          "revisions which have been 'accepted' as committed.");
+      }
     }
 
-    $revision_id = $revision->getID();
-    $revision_name = $revision->getName();
+    if ($revision) {
+      $actually_mark = true;
+      if ($is_finalize) {
+        $project_info = $conduit->callMethodSynchronous(
+          'arcanist.projectinfo',
+          array(
+            'name' => $this->getWorkingCopy()->getProjectID(),
+          ));
+        if ($project_info['tracked']) {
+          $actually_mark = false;
+        }
+      }
+      if ($actually_mark) {
+        $revision_id = $revision->getID();
+        $revision_name = $revision->getName();
 
-    echo "Marking revision D{$revision_id} '{$revision_name}' committed...\n";
+        echo "Marking revision D{$revision_id} '{$revision_name}' ".
+             "committed...\n";
 
-    $conduit->callMethodSynchronous(
-      'differential.markcommitted',
+        $conduit->callMethodSynchronous(
+          'differential.markcommitted',
+          array(
+            'revision_id' => $revision_id,
+          ));
+      }
+    }
+
+    $revision_info = $conduit->callMethodSynchronous(
+      'differential.getrevision',
       array(
         'revision_id' => $revision_id,
       ));
+    $status = $revision_info['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_info['commits']);
+    } else {
+      $is_finalized = false;
+    }
 
-    echo "Done.\n";
+    if ($is_finalized) {
+      $message = $this->getRepositoryAPI()->getFinalizedRevisionMessage();
+      echo phutil_console_wrap($message)."\n";
+    } else {
+      echo "Done.\n";
+    }
 
     return 0;
   }

src/workflow/merge/ArcanistMergeWorkflow.php

@@ -144,15 +144,20 @@ EOTEXT
       $repository_api->performLocalBranchMerge($branch, $message);
       echo "Merged '{$branch}'.\n";
 
-      $done_message = $repository_api->getFinalizedRevisionMessage();
-      echo phutil_console_wrap($done_message."\n");
+      $mark_workflow = $this->buildChildWorkflow(
+        'mark-committed',
+        array(
+          '--finalize',
+          $revision_id,
+        ));
+      $mark_workflow->run();
     }
 
     return 0;
   }
 
   protected function getSupportedRevisionControlSystems() {
-    return array('git');
+    return array('git', 'hg');
   }
 
 }