Improve handling of commit message on --auto/--update/--create workflows

Summary:
Our failure behavior currently sucks. In particular:

  - If lint or unit fail (expectedly or unexpectedly), we throw away your
message. omglol sux 2 b u
  - If there's a parsing error, we dump the message to 'arc-commit-message' in
CWD and tell you to go deal with it.

This is awful. Improve the behavior:

  - Once we read a message, save it in .arc/commit-message so we always have it
if anything goes wrong.
  - If that file exists, prompt to read it without any "-F" nonsense.
  - If there's a parsing error, tell the user and prompt them to just edit the
file again, showing what they need to fix.

Test Plan: Ran "arc diff --auto" and made a bunch of intentional errors; arc
never screwed me over and was generally fairly pleasant.

Reviewers: btrahan, Makinde

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T614, T895

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

src/workflow/base/ArcanistBaseWorkflow.php

@@ -37,7 +37,16 @@
  * verified information about the user identity by calling @{method:getUserPHID}
  * or @{method:getUserName} after authentication occurs.
  *
+ * = Scratch Files =
+ *
+ * Arcanist workflows can read and write 'scratch files', which are temporary
+ * files stored in the project that persist across commands. They can be useful
+ * if you want to save some state, or keep a copy of a long message the user
+ * entered in something goes wrong..
+ *
+ *
  * @task  conduit   Conduit
+ * @task  scratch   Scratch Files
  * @group workflow
  * @stable
  */
@@ -601,6 +610,16 @@ abstract class ArcanistBaseWorkflow {
 
     $untracked = $api->getUntrackedChanges();
     if ($this->shouldRequireCleanUntrackedFiles()) {
+
+      // Exempt ".arc/" scratch files from this warning so that things work
+      // a little more smoothly if no one has gotten around to adding .arc to
+      // the ignore list.
+      foreach ($untracked as $key => $path) {
+        if (preg_match('@\.arc/@', $path)) {
+          unset($untracked[$key]);
+        }
+      }
+
       if (!empty($untracked)) {
         echo "You have untracked files in this working copy.\n\n".
              $working_copy_desc.
@@ -1002,4 +1021,125 @@ abstract class ArcanistBaseWorkflow {
     return implode('', $list);
   }
 
+/* -(  Scratch Files  )------------------------------------------------------ */
+
+
+  /**
+   * Try to read a scratch file, if it exists and is readable.
+   *
+   * @param string Scratch file name.
+   * @return mixed String for file contents, or false for failure.
+   * @task scratch
+   */
+  protected function readScratchFile($path) {
+    $full_path = $this->getScratchFilePath($path);
+    if (!$full_path) {
+      return false;
+    }
+
+    if (!Filesystem::pathExists($full_path)) {
+      return false;
+    }
+
+    try {
+      $result = Filesystem::readFile($full_path);
+    } catch (FilesystemException $ex) {
+      return false;
+    }
+
+    return $result;
+  }
+
+
+  /**
+   * Try to write a scratch file, if there's somewhere to put it and we can
+   * write there.
+   *
+   * @param  string Scratch file name to write.
+   * @param  string Data to write.
+   * @return bool   True on success, false on failure.
+   * @task scratch
+   */
+  protected function writeScratchFile($path, $data) {
+    $dir = $this->getScratchFilePath('');
+    if (!$dir) {
+      return false;
+    }
+
+    if (!Filesystem::pathExists($dir)) {
+      try {
+        execx('mkdir %s', $dir);
+      } catch (Exception $ex) {
+        return false;
+      }
+    }
+
+    try {
+      Filesystem::writeFile($this->getScratchFilePath($path), $data);
+    } catch (FilesystemException $ex) {
+      return false;
+    }
+
+    return true;
+  }
+
+
+  /**
+   * Try to remove a scratch file.
+   *
+   * @param   string  Scratch file name to remove.
+   * @return  bool    True if the file was removed successfully.
+   * @task scratch
+   */
+  protected function removeScratchFile($path) {
+    $full_path = $this->getScratchFilePath($path);
+    if (!$full_path) {
+      return false;
+    }
+
+    try {
+      Filesystem::remove($full_path);
+    } catch (FilesystemException $ex) {
+      return false;
+    }
+
+    return true;
+  }
+
+
+  /**
+   * Get a human-readable description of the scratch file location.
+   *
+   * @param string  Scratch file name.
+   * @return mixed  String, or false on failure.
+   * @task scratch
+   */
+  protected function getReadableScratchFilePath($path) {
+    $full_path = $this->getScratchFilePath($path);
+    if ($full_path) {
+      return Filesystem::readablePath(
+        $full_path,
+        $this->getRepositoryAPI()->getPath());
+    } else {
+      return false;
+    }
+  }
+
+
+  /**
+   * Get the path to a scratch file, if possible.
+   *
+   * @param string  Scratch file name.
+   * @return mixed  File path, or false on failure.
+   * @task scratch
+   */
+  protected function getScratchFilePath($path) {
+    if (!$this->repositoryAPI) {
+      return false;
+    }
+
+    $repository_api = $this->getRepositoryAPI();
+    return $repository_api->getPath('.arc/'.$path);
+  }
+
 }

src/workflow/diff/ArcanistDiffWorkflow.php

@@ -463,6 +463,8 @@ EOTEXT
       ob_get_clean();
     }
 
+    $this->removeScratchFile('create-message');
+
     return 0;
   }
 
@@ -1185,38 +1187,110 @@ EOTEXT
   private function getCommitMessageFromUser() {
     $conduit = $this->getConduit();
 
+    $template = null;
+
+    $saved = $this->readScratchFile('create-message');
+    if ($saved) {
+      $where = $this->getReadableScratchFilePath('create-message');
+
+      $preview = explode("\n", $saved);
+      $preview = array_shift($preview);
+      $preview = trim($preview);
+      $preview = phutil_utf8_shorten($preview, 64);
+
+      if ($preview) {
+        $preview = "Message begins:\n\n       {$preview}\n\n";
+      } else {
+        $preview = null;
+      }
+
+      echo
+        "You have a saved revision message in '{$where}'.\n".
+        "{$preview}".
+        "You can use this message, or discard it.";
+
+      $use = phutil_console_confirm(
+        "Do you want to use this message?",
+        $default_no = false);
+      if ($use) {
+        $template = $saved;
+      } else {
+        $this->removeScratchFile('create-message');
+      }
+    }
+
+    $template_is_default = false;
+
+    if (!$template) {
       $template = $conduit->callMethodSynchronous(
         'differential.getcommitmessage',
         array(
           'revision_id' => null,
           'edit' => true,
         ));
+      $template_is_default = true;
+    }
 
-    $template =
-      $template.
-      "\n\n".
-      "# Describe this revision.".
-      "\n";
-    $template = id(new PhutilInteractiveEditor($template))
+    $issues = array('Describe this revision.');
+
+    $done = false;
+    while (!$done) {
+      $template = rtrim($template)."\n\n";
+      foreach ($issues as $issue) {
+        $template .= '# '.$issue."\n";
+      }
+      $template .= "\n";
+
+      $new_template = id(new PhutilInteractiveEditor($template))
         ->setName('new-commit')
         ->editInteractively();
-    $template = preg_replace('/^\s*#.*$/m', '', $template);
+
+      if ($template_is_default && ($new_template == $template)) {
+        throw new ArcanistUsageException(
+          "Template not edited.");
+      }
+
+      $template = preg_replace('/^\s*#.*$/m', '', $new_template);
+      $template = rtrim($template)."\n";
+      $wrote = $this->writeScratchFile('create-message', $template);
+      $where = $this->getReadableScratchFilePath('create-message');
 
       try {
+
         $message = ArcanistDifferentialCommitMessage::newFromRawCorpus(
           $template);
         $message->pullDataFromConduit($conduit);
         $this->validateCommitMessage($message);
+        $done = true;
+      } catch (ArcanistDifferentialCommitMessageParserException $ex) {
+        echo "Commit message has errors:\n\n";
+        $issues = array('Resolve these errors:');
+        foreach ($ex->getParserErrors() as $error) {
+          echo "      - ".$error."\n";
+          $issues[] = '  - '.$error;
+        }
+        echo "\n";
+        echo "You must resolve these errors to continue.";
+        $again = phutil_console_confirm(
+          "Do you want to edit the message?",
+          $default_no = false);
+        if ($again) {
+          // Keep going.
+        } else {
+          $saved = null;
+          if ($wrote) {
+            $saved = "A copy was saved to '{$where}'.";
+          }
+          throw new ArcanistUsageException(
+            'Message has unresolved errrors. {$saved}');
+        }
       } catch (Exception $ex) {
-      $path = Filesystem::writeUniqueFile('arc-commit-message', $template);
-
-      echo phutil_console_wrap(
-        "\n".
-        "Exception while parsing commit message! Message saved to ".
-        "'{$path}'. Use -F <file> to specify a commit message file.\n");
-
+        if ($wrote) {
+          echo phutil_console_wrap("(Commit messaged saved to '{$where}'.)");
+        }
         throw $ex;
       }
+    }
 
     return $message;
   }