Revenge of the git relative commit default

Summary:
The default of "arc diff" to "arc diff HEAD^" in git is universally confusing to everyone not at Facebook.

Drive the default with configuration instead. Even at Facebook, "origin/trunk" (or whatever) is probably a better default than "HEAD^".

See D863 for the last attempt at this.

NOTE: This is contentious!

Test Plan: Ran "arc diff", got prompted to set a default. Ran "arc diff" from a zero-commit repo, got sensible behavior

Reviewers: btrahan, vrana, nh, jungejason, tuomaspelkonen

Reviewed By: btrahan

CC: aran, epriestley, zeeg, davidreuss

Maniphest Tasks: T651

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

src/repository/api/base/ArcanistRepositoryAPI.php

@@ -209,4 +209,121 @@ abstract class ArcanistRepositoryAPI {
 
   abstract protected function buildLocalFuture(array $argv);
 
+
+/* -(  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
+   */
+  public 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
+   */
+  public function writeScratchFile($path, $data) {
+    $dir = $this->getScratchFilePath('');
+    if (!$dir) {
+      return false;
+    }
+
+    if (!Filesystem::pathExists($dir)) {
+      try {
+        Filesystem::createDirectory($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
+   */
+  public 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
+   */
+  public function getReadableScratchFilePath($path) {
+    $full_path = $this->getScratchFilePath($path);
+    if ($full_path) {
+      return Filesystem::readablePath(
+        $full_path,
+        $this->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
+   */
+  public function getScratchFilePath($path) {
+    return $this->getPath('.arc/'.$path);
+  }
+
 }

src/repository/api/git/ArcanistGitAPI.php

@@ -112,18 +112,80 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
 
   public function getRelativeCommit() {
     if ($this->relativeCommit === null) {
+
+      // Detect zero-commit or one-commit repositories. There is only one
+      // relative-commit value that makes any sense in these repositories: the
+      // empty tree.
       list($err) = $this->execManualLocal('rev-parse --verify HEAD^');
       if ($err) {
         list($err) = $this->execManualLocal('rev-parse --verify HEAD');
         if ($err) {
           $this->repositoryHasNoCommits = true;
         }
+
         $this->relativeCommit = self::GIT_MAGIC_ROOT_COMMIT;
 
-      } else {
+        return $this->relativeCommit;
-        $this->relativeCommit = 'HEAD^';
       }
+
+      $default_relative = $this->readScratchFile('default-relative-commit');
+      $do_write = false;
+      if (!$default_relative) {
+
+        // TODO: Remove the history lesson soon.
+
+        echo phutil_console_format(
+          "<bg:green>** Select a Default Commit Range **</bg>\n\n");
+        echo phutil_console_wrap(
+          "You're running a command which operates on a range of revisions ".
+          "(usually, from some revision to HEAD) but have not specified the ".
+          "revision that should determine the start of the range.\n\n".
+          "Previously, arc assumed you meant 'HEAD^' when you did not specify ".
+          "a start revision, but this behavior does not make much sense in ".
+          "most workflows outside of Facebook's historic git-svn workflow.\n\n".
+          "arc no longer assumes 'HEAD^'. You must specify a relative commit ".
+          "explicitly when you invoke a command (e.g., `arc diff HEAD^`, not ".
+          "just `arc diff`) or select a default for this working copy.\n\n".
+          "In most cases, the best default is 'origin/master'. You can also ".
+          "select 'HEAD^' to preserve the old behavior, or some other remote ".
+          "or branch. But you almost certainly want to select ".
+          "'origin/master'.\n\n".
+          "(Technically: the merge-base of the selected revision and HEAD is ".
+          "used to determine the start of the commit range.)");
+
+        $prompt = "What default do you want to use? [origin/master]";
+        $default = phutil_console_prompt($prompt);
+
+        if (!strlen(trim($default))) {
+          $default = 'origin/master';
+        }
+
+        $default_relative = $default;
+        $do_write = true;
+      }
+
+      list($object_type) = $this->execxLocal(
+        'cat-file -t %s',
+        $default_relative);
+
+      if (trim($object_type) !== 'commit') {
+        throw new Exception(
+          "Relative commit '{$relative}' is not the name of a commit!");
+      }
+
+      if ($do_write) {
+        // Don't perform this write until we've verified that the object is a
+        // valid commit name.
+        $this->writeScratchFile('default-relative-commit', $default_relative);
+      }
+
+      list($merge_base) = $this->execxLocal(
+        'merge-base %s HEAD',
+        $default_relative);
+
+      $this->relativeCommit = trim($merge_base);
     }
+
     return $this->relativeCommit;
   }
 

src/repository/api/git/__init__.php

@@ -11,6 +11,7 @@ phutil_require_module('arcanist', 'exception/usage');
 phutil_require_module('arcanist', 'parser/diff');
 phutil_require_module('arcanist', 'repository/api/base');
 
+phutil_require_module('phutil', 'console');
 phutil_require_module('phutil', 'future');
 phutil_require_module('phutil', 'future/exec');
 phutil_require_module('phutil', 'utils');

src/workflow/base/ArcanistBaseWorkflow.php

@@ -1007,6 +1007,7 @@ abstract class ArcanistBaseWorkflow {
     return implode('', $list);
   }
 
+
 /* -(  Scratch Files  )------------------------------------------------------ */
 
 
@@ -1018,22 +1019,10 @@ abstract class ArcanistBaseWorkflow {
    * @task scratch
    */
   protected function readScratchFile($path) {
-    $full_path = $this->getScratchFilePath($path);
+    if (!$this->repositoryAPI) {
-    if (!$full_path) {
       return false;
     }
-
+    return $this->getRepositoryAPI()->readScratchFile($path);
-    if (!Filesystem::pathExists($full_path)) {
-      return false;
-    }
-
-    try {
-      $result = Filesystem::readFile($full_path);
-    } catch (FilesystemException $ex) {
-      return false;
-    }
-
-    return $result;
   }
 
 
@@ -1047,26 +1036,10 @@ abstract class ArcanistBaseWorkflow {
    * @task scratch
    */
   protected function writeScratchFile($path, $data) {
-    $dir = $this->getScratchFilePath('');
+    if (!$this->repositoryAPI) {
-    if (!$dir) {
       return false;
     }
-
+    return $this->getRepositoryAPI()->writeScratchFile($path, $data);
-    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;
   }
 
 
@@ -1078,18 +1051,10 @@ abstract class ArcanistBaseWorkflow {
    * @task scratch
    */
   protected function removeScratchFile($path) {
-    $full_path = $this->getScratchFilePath($path);
+    if (!$this->repositoryAPI) {
-    if (!$full_path) {
       return false;
     }
-
+    return $this->getRepositoryAPI()->removeScratchFile($path);
-    try {
-      Filesystem::remove($full_path);
-    } catch (FilesystemException $ex) {
-      return false;
-    }
-
-    return true;
   }
 
 
@@ -1101,14 +1066,10 @@ abstract class ArcanistBaseWorkflow {
    * @task scratch
    */
   protected function getReadableScratchFilePath($path) {
-    $full_path = $this->getScratchFilePath($path);
+    if (!$this->repositoryAPI) {
-    if ($full_path) {
-      return Filesystem::readablePath(
-        $full_path,
-        $this->getRepositoryAPI()->getPath());
-    } else {
       return false;
     }
+    return $this->getRepositoryAPI()->getReadableScratchFilePath($path);
   }
 
 
@@ -1123,9 +1084,7 @@ abstract class ArcanistBaseWorkflow {
     if (!$this->repositoryAPI) {
       return false;
     }
-
+    return $this->getRepositoryAPI()->getScratchFilePath($path);
-    $repository_api = $this->getRepositoryAPI();
-    return $repository_api->getPath('.arc/'.$path);
   }
 
   protected function getRepositoryEncoding() {

src/workflow/diff/ArcanistDiffWorkflow.php

@@ -52,7 +52,7 @@ EOTEXT
 
           Under git, you can specify a commit (like __HEAD^^^__ or __master__)
           and Differential will generate a diff against the merge base of that
-          commit and HEAD. If you omit the commit, the default is __HEAD^__.
+          commit and HEAD.
 
           Under svn, you can choose to include only some of the modified files
           in the working copy in the diff by specifying their paths. If you