Fix "local commits" for Git and Mercurial to align with user expectations, and fix a parsing bug

Summary:
  - "git log" still includes "\n", so we're currently generating nonsense hashes like "\nabd9879bab86ad78ab...".
  - Correct the log range we use in Git. See comment. When users perform merges, their expectations about what the "included commits" and what the included changes are are different. Represent them with two different ranges.
  - Same deal for Mercurial

Test Plan: Ran "arc which" in various contexts.

Reviewers: btrahan, aurelijus, Makinde

Reviewed By: Makinde

CC: aran, nh, jungejason

Maniphest Tasks: T873

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

src/repository/api/git/ArcanistGitAPI.php

@@ -72,8 +72,28 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       // One commit.
       $against = 'HEAD';
     } else {
-      // 2..N commits.
-      $against = $this->getRelativeCommit().'..HEAD';
+
+      // 2..N commits. We include commits reachable from HEAD which are
+      // not reachable from the relative commit; this is consistent with
+      // user expectations even though it is not actually the diff range.
+      // Particularly:
+      //
+      //    |
+      //    D <----- master branch
+      //    |
+      //    C  Y <- feature branch
+      //    | /|
+      //    B  X
+      //    | /
+      //    A
+      //    |
+      //
+      // If "A, B, C, D" are master, and the user is at Y, when they run
+      // "arc diff B" they want (and get) a diff of B vs Y, but they think about
+      // this as being the commits X and Y. If we log "B..Y", we only show
+      // Y. With "Y --not B", we show X and Y.
+
+      $against = csprintf('%s --not %s', 'HEAD', $this->getRelativeCommit());
     }
 
     // NOTE: Windows escaping of "%" symbols apparently is inherently broken;
@@ -85,8 +105,8 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
 
     list($info) = $this->execxLocal(
       phutil_is_windows()
-        ? 'log %s --format=%C --'
-        : 'log %s --format=%s --',
+        ? 'log %C --format=%C --'
+        : 'log %C --format=%s --',
       $against,
       // NOTE: "%B" is somewhat new, use "%s%n%n%b" instead.
       '%H%x01%T%x01%P%x01%at%x01%an%x01%s%x01%s%n%n%b%x02');
@@ -101,7 +121,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     $info = explode("\2", $info);
     foreach ($info as $line) {
       list($commit, $tree, $parents, $time, $author, $title, $message)
-        = explode("\1", $line, 7);
+        = explode("\1", trim($line), 7);
       $message = rtrim($message);
 
       $commits[$commit] = array(

src/repository/api/git/__init__.php

@@ -17,6 +17,7 @@ phutil_require_module('phutil', 'filesystem/tempfile');
 phutil_require_module('phutil', 'future');
 phutil_require_module('phutil', 'future/exec');
 phutil_require_module('phutil', 'utils');
+phutil_require_module('phutil', 'xsprintf/csprintf');
 
 
 phutil_require_source('ArcanistGitAPI.php');

src/repository/api/mercurial/ArcanistMercurialAPI.php

@@ -170,15 +170,15 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
   public function getLocalCommitInformation() {
     if ($this->localCommitInfo === null) {
       list($info) = $this->execxLocal(
-        "log --template '%C' --rev %s..%s --",
+        "log --template '%C' --prune %s --rev %s --branch %s --",
         "{node}\1{rev}\1{author}\1{date|rfc822date}\1".
           "{branch}\1{tag}\1{parents}\1{desc}\2",
         $this->getRelativeCommit(),
-        $this->getWorkingCopyRevision());
+        $this->getWorkingCopyRevision().':0',
+        $this->getBranchName());
       $logs = array_filter(explode("\2", $info));
 
       $last_node = null;
-      $is_first = true;
 
       $futures = array();
 
@@ -198,7 +198,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
           }
         }
 
-        if (!$commit_parents && !$is_first) {
+        if (!$commit_parents) {
           // We didn't get a cheap hit on previous commit, so do the full-cost
           // "hg parents" call. We can run these in parallel, at least.
           $futures[$node] = $this->execFutureLocal(
@@ -220,15 +220,8 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
         );
 
         $last_node = $node;
-        $is_first = false;
       }
 
-      // Get rid of the first log, it's not actually part of the diff. "hg log"
-      // is inclusive, while "hg diff" is exclusive. We do this after processing
-      // so we can take advantage of the cheaper lookup for the parents of the
-      // first commit we keep, in the common case.
-      array_shift($commits);
-
       foreach (Futures($futures)->limit(4) as $node => $future) {
         list($parents) = $future->resolvex();
         $parents = array_filter(explode("\n", $parents));