(stable) Promote 2020 Week 27

src/__phutil_library_map__.php

@@ -307,6 +307,7 @@ phutil_register_library_map(array(
     'ArcanistLandCommit' => 'land/ArcanistLandCommit.php',
     'ArcanistLandCommitSet' => 'land/ArcanistLandCommitSet.php',
     'ArcanistLandEngine' => 'land/engine/ArcanistLandEngine.php',
+    'ArcanistLandPushFailureException' => 'land/exception/ArcanistLandPushFailureException.php',
     'ArcanistLandSymbol' => 'land/ArcanistLandSymbol.php',
     'ArcanistLandTarget' => 'land/ArcanistLandTarget.php',
     'ArcanistLandWorkflow' => 'workflow/ArcanistLandWorkflow.php',
@@ -1352,6 +1353,7 @@ phutil_register_library_map(array(
     'ArcanistLandCommit' => 'Phobject',
     'ArcanistLandCommitSet' => 'Phobject',
     'ArcanistLandEngine' => 'ArcanistWorkflowEngine',
+    'ArcanistLandPushFailureException' => 'Exception',
     'ArcanistLandSymbol' => 'Phobject',
     'ArcanistLandTarget' => 'Phobject',
     'ArcanistLandWorkflow' => 'ArcanistArcWorkflow',

src/land/engine/ArcanistGitLandEngine.php

@@ -491,7 +491,7 @@ final class ArcanistGitLandEngine
         $flags_argv,
         $into_commit);
       if ($err) {
-        throw new ArcanistUsageException(
+        throw new ArcanistLandPushFailureException(
           pht(
             'Submit failed! Fix the error and run "arc land" again.'));
       }
@@ -509,16 +509,10 @@ final class ArcanistGitLandEngine
       $this->newOntoRefArguments($into_commit));
 
     if ($err) {
-      throw new ArcanistUsageException(
+      throw new ArcanistLandPushFailureException(
         pht(
           'Push failed! Fix the error and run "arc land" again.'));
     }
-
-    // TODO
-    // if ($this->isGitSvn) {
-    //   $err = phutil_passthru('git svn dcommit');
-    //   $cmd = 'git svn dcommit';
-
   }
 
   protected function reconcileLocalState(

src/land/engine/ArcanistLandEngine.php

@@ -1259,7 +1259,7 @@ abstract class ArcanistLandEngine
             try {
               $this->pushChange($into_commit);
               $this->setHasUnpushedChanges(false);
-            } catch (Exception $ex) {
+            } catch (ArcanistLandPushFailureException $ex) {
 
               // TODO: If the push fails, fetch and retry if the remote ref
               // has moved ahead of us.
@@ -1280,7 +1280,8 @@ abstract class ArcanistLandEngine
                 continue;
               }
 
-              throw $ex;
+              throw new PhutilArgumentUsageException(
+                $ex->getMessage());
             }
 
             if ($need_cascade) {

src/land/engine/ArcanistMercurialLandEngine.php

@@ -768,6 +768,19 @@ final class ArcanistMercurialLandEngine
       throw new Exception(pht('TODO: Support merge strategies'));
     }
 
+    // See PHI1808. When we "hg rebase ..." below, Mercurial will move
+    // bookmarks which point at the old commit range to point at the rebased
+    // commit. This is somewhat surprising and we don't want this to happen:
+    // save the old bookmark state so we can put the bookmarks back before
+    // we continue.
+
+    $bookmark_refs = $api->newMarkerRefQuery()
+      ->withMarkerTypes(
+        array(
+          ArcanistMarkerRef::TYPE_BOOKMARK,
+        ))
+      ->execute();
+
     // TODO: Add a Mercurial version check requiring 2.1.1 or newer.
 
     $api->execxLocal(
@@ -817,6 +830,28 @@ final class ArcanistMercurialLandEngine
       throw $ex;
     }
 
+    // Find all the bookmarks which pointed at commits we just rebased, and
+    // put them back the way they were before rebasing moved them. We aren't
+    // deleting the old commits yet and don't want to move the bookmarks.
+
+    $obsolete_map = array();
+    foreach ($set->getCommits() as $commit) {
+      $obsolete_map[$commit->getHash()] = true;
+    }
+
+    foreach ($bookmark_refs as $bookmark_ref) {
+      $bookmark_hash = $bookmark_ref->getCommitHash();
+
+      if (!isset($obsolete_map[$bookmark_hash])) {
+        continue;
+      }
+
+      $api->execxLocal(
+        'bookmark --force --rev %s -- %s',
+        $bookmark_hash,
+        $bookmark_ref->getName());
+    }
+
     list($stdout) = $api->execxLocal('log --rev tip --template %s', '{node}');
     $new_cursor = trim($stdout);
 
@@ -834,7 +869,12 @@ final class ArcanistMercurialLandEngine
 
     try {
       foreach ($body as $command) {
-        $this->newPassthru('%Ls', $command);
+        $err = $this->newPassthru('%Ls', $command);
+        if ($err) {
+          throw new ArcanistLandPushFailureException(
+            pht(
+              'Push failed! Fix the error and run "arc land" again.'));
+        }
       }
     } finally {
       foreach ($tail as $command) {
@@ -998,6 +1038,7 @@ final class ArcanistMercurialLandEngine
     }
 
     $revs = array();
+    $obsolete_map = array();
 
     // We've rebased all descendants already, so we can safely delete all
     // of these commits.
@@ -1010,6 +1051,10 @@ final class ArcanistMercurialLandEngine
       $max_commit = last($commits)->getHash();
 
       $revs[] = hgsprintf('%s::%s', $min_commit, $max_commit);
+
+      foreach ($commits as $commit) {
+        $obsolete_map[$commit->getHash()] = true;
+      }
     }
 
     $rev_set = '('.implode(') or (', $revs).')';
@@ -1021,6 +1066,33 @@ final class ArcanistMercurialLandEngine
     // removes the obsolescence marker and revives the predecessor. This is
     // not desirable: we want to destroy all predecessors of these commits.
 
+    // See PHI1808. Both "hg strip" and "hg prune" move bookmarks backwards in
+    // history rather than destroying them. Instead, we want to destroy any
+    // bookmarks which point at these now-obsoleted commits.
+
+    $bookmark_refs = $api->newMarkerRefQuery()
+      ->withMarkerTypes(
+        array(
+          ArcanistMarkerRef::TYPE_BOOKMARK,
+        ))
+      ->execute();
+    foreach ($bookmark_refs as $bookmark_ref) {
+      $bookmark_hash = $bookmark_ref->getCommitHash();
+      $bookmark_name = $bookmark_ref->getName();
+
+      if (!isset($obsolete_map[$bookmark_hash])) {
+        continue;
+      }
+
+      $log->writeStatus(
+        pht('CLEANUP'),
+        pht('Deleting bookmark "%s".', $bookmark_name));
+
+      $api->execxLocal(
+        'bookmark --delete -- %s',
+        $bookmark_name);
+    }
+
     if ($api->getMercurialFeature('evolve')) {
       $api->execxLocal(
         'prune --rev %s',

src/land/exception/ArcanistLandPushFailureException.php

@@ -0,0 +1,4 @@
+<?php
+
+final class ArcanistLandPushFailureException
+  extends Exception {}

src/repository/marker/ArcanistMarkerRef.php

@@ -9,6 +9,8 @@ final class ArcanistMarkerRef
 
   const TYPE_BRANCH = 'branch';
   const TYPE_BOOKMARK = 'bookmark';
+  const TYPE_COMMIT_STATE = 'commit-state';
+  const TYPE_BRANCH_STATE = 'branch-state';
 
   private $name;
   private $markerType;
@@ -148,6 +150,14 @@ final class ArcanistMarkerRef
     return ($this->getMarkerType() === self::TYPE_BRANCH);
   }
 
+  public function isCommitState() {
+    return ($this->getMarkerType() === self::TYPE_COMMIT_STATE);
+  }
+
+  public function isBranchState() {
+    return ($this->getMarkerType() === self::TYPE_BRANCH_STATE);
+  }
+
   public function attachCommitRef(ArcanistCommitRef $ref) {
     return $this->attachHardpoint(self::HARDPOINT_COMMITREF, $ref);
   }

src/repository/marker/ArcanistMercurialRepositoryMarkerQuery.php

@@ -71,10 +71,6 @@ final class ArcanistMercurialRepositoryMarkerQuery
       }
 
       $node = $item['node'];
-      if (!$node) {
-        // NOTE: For now, we ignore the virtual "current branch" marker.
-        continue;
-      }
 
       switch ($item['type']) {
         case 'branch':
@@ -83,8 +79,11 @@ final class ArcanistMercurialRepositoryMarkerQuery
         case 'bookmark':
           $marker_type = ArcanistMarkerRef::TYPE_BOOKMARK;
           break;
-        case 'commit':
-          $marker_type = null;
+        case 'commit-state':
+          $marker_type = ArcanistMarkerRef::TYPE_COMMIT_STATE;
+          break;
+        case 'branch-state':
+          $marker_type = ArcanistMarkerRef::TYPE_BRANCH_STATE;
           break;
         default:
           throw new Exception(
@@ -94,11 +93,6 @@ final class ArcanistMercurialRepositoryMarkerQuery
               $item['type']));
       }
 
-      if ($marker_type === null) {
-        // NOTE: For now, we ignore the virtual "head" marker.
-        continue;
-      }
-
       $commit_ref = $api->newCommitRef()
         ->setCommitHash($node);
 

src/repository/state/ArcanistMercurialLocalState.php

@@ -5,40 +5,105 @@ final class ArcanistMercurialLocalState
 
   private $localCommit;
   private $localBranch;
+  private $localBookmark;
 
   protected function executeSaveLocalState() {
     $api = $this->getRepositoryAPI();
     $log = $this->getWorkflow()->getLogEngine();
 
-    // TODO: Both of these can be pulled from "hg arc-ls-markers" more
-    // efficiently.
+    $markers = $api->newMarkerRefQuery()
+      ->execute();
 
-    $this->localCommit = $api->getCanonicalRevisionName('.');
+    $local_commit = null;
+    foreach ($markers as $marker) {
+      if ($marker->isCommitState()) {
+        $local_commit = $marker->getCommitHash();
+      }
+    }
 
-    list($branch) = $api->execxLocal('branch');
-    $this->localBranch = trim($branch);
+    if ($local_commit === null) {
+      throw new Exception(
+        pht(
+          'Unable to identify the current commit in the working copy.'));
+    }
 
-    $log->writeTrace(
-      pht('SAVE STATE'),
-      pht(
+    $this->localCommit = $local_commit;
+
+    $local_branch = null;
+    foreach ($markers as $marker) {
+      if ($marker->isBranchState()) {
+        $local_branch = $marker->getName();
+        break;
+      }
+    }
+
+    if ($local_branch === null) {
+      throw new Exception(
+        pht(
+          'Unable to identify the current branch in the working copy.'));
+    }
+
+    if ($local_branch !== null) {
+      $this->localBranch = $local_branch;
+    }
+
+    $local_bookmark = null;
+    foreach ($markers as $marker) {
+      if ($marker->isBookmark()) {
+        if ($marker->getIsActive()) {
+          $local_bookmark = $marker->getName();
+          break;
+        }
+      }
+    }
+
+    if ($local_bookmark !== null) {
+      $this->localBookmark = $local_bookmark;
+    }
+
+    $has_bookmark = ($this->localBookmark !== null);
+
+    if ($has_bookmark) {
+      $location = pht(
+        'Saving local state (at "%s" on branch "%s", bookmarked as "%s").',
+        $api->getDisplayHash($this->localCommit),
+        $this->localBranch,
+        $this->localBookmark);
+    } else {
+      $location = pht(
         'Saving local state (at "%s" on branch "%s").',
         $api->getDisplayHash($this->localCommit),
-        $this->localBranch));
+        $this->localBranch);
+    }
+
+    $log->writeTrace(pht('SAVE STATE'), $location);
   }
 
   protected function executeRestoreLocalState() {
     $api = $this->getRepositoryAPI();
     $log = $this->getWorkflow()->getLogEngine();
 
-    $log->writeStatus(
-      pht('LOAD STATE'),
-      pht(
+    if ($this->localBookmark !== null) {
+      $location = pht(
+        'Restoring local state (at "%s" on branch "%s", bookmarked as "%s").',
+        $api->getDisplayHash($this->localCommit),
+        $this->localBranch,
+        $this->localBookmark);
+    } else {
+      $location = pht(
         'Restoring local state (at "%s" on branch "%s").',
         $api->getDisplayHash($this->localCommit),
-        $this->localBranch));
+        $this->localBranch);
+    }
+
+    $log->writeStatus(pht('LOAD STATE'), $location);
 
     $api->execxLocal('update -- %s', $this->localCommit);
     $api->execxLocal('branch --force -- %s', $this->localBranch);
+
+    if ($this->localBookmark !== null) {
+      $api->execxLocal('bookmark --force -- %s', $this->localBookmark);
+    }
   }
 
   protected function executeDiscardLocalState() {
@@ -70,6 +135,12 @@ final class ArcanistMercurialLocalState
       'hg branch --force -- %s',
       $this->localBranch);
 
+    if ($this->localBookmark !== null) {
+      $commands[] = csprintf(
+        'hg bookmark --force -- %s',
+        $this->localBookmark);
+    }
+
     return $commands;
   }
 

support/hg/arc-hg.py

@@ -85,21 +85,17 @@ def localmarkers(ui, repo):
   active_node = repo[b'.'].node()
   all_heads = set(repo.heads())
   current_name = repo.dirstate.branch()
-  saw_current = False
-  saw_active = False
 
   branch_list = repo.branchmap().iterbranches()
   for branch_name, branch_heads, tip_node, is_closed in branch_list:
     for head_node in branch_heads:
-      is_active = (head_node == active_node)
+
+      is_active = False
+      if branch_name == current_name:
+        if head_node == active_node:
+          is_active = True
+
       is_tip = (head_node == tip_node)
-      is_current = (branch_name == current_name)
-
-      if is_current:
-        saw_current = True
-
-      if is_active:
-        saw_active = True
 
       if is_closed:
         head_closed = True
@@ -115,26 +111,9 @@ def localmarkers(ui, repo):
         'isActive': is_active,
         'isClosed': head_closed,
         'isTip': is_tip,
-        'isCurrent': is_current,
         'description': description,
       })
 
-  # If the current branch (selected with "hg branch X") is not reflected in
-  # the list of heads we selected, add a virtual head for it so callers get
-  # a complete picture of repository marker state.
-
-  if not saw_current:
-    markers.append({
-      'type': 'branch',
-      'name': current_name,
-      'node': None,
-      'isActive': False,
-      'isClosed': False,
-      'isTip': False,
-      'isCurrent': True,
-      'description': None,
-    })
-
   bookmarks = repo._bookmarks
   active_bookmark = repo._activebookmark
 
@@ -142,9 +121,6 @@ def localmarkers(ui, repo):
     is_active = (active_bookmark == bookmark_name)
     description = repo[bookmark_node].description()
 
-    if is_active:
-      saw_active = True
-
     markers.append({
       'type': 'bookmark',
       'name': bookmark_name,
@@ -153,21 +129,36 @@ def localmarkers(ui, repo):
       'description': description,
     })
 
-  # If the current working copy state is not the head of a branch and there is
-  # also no active bookmark, add a virtual marker for it so callers can figure
-  # out exactly where we are.
+  # Add virtual markers for the current commit state and current branch state
+  # so callers can figure out exactly where we are.
 
-  if not saw_active:
-    markers.append({
-      'type': 'commit',
-      'name': None,
-      'node': node.hex(active_node),
-      'isActive': False,
-      'isClosed': False,
-      'isTip': False,
-      'isCurrent': True,
-      'description': repo[b'.'].description(),
-    })
+  # Common cases where this matters include:
+
+  # You run "hg update 123" to update to an older revision. Your working
+  # copy commit will not be a branch head or a bookmark.
+
+  # You run "hg branch X" to create a new branch, but have not made any commits
+  # yet. Your working copy branch will not be reflected in any commits.
+
+  markers.append({
+    'type': 'branch-state',
+    'name': current_name,
+    'node': None,
+    'isActive': True,
+    'isClosed': False,
+    'isTip': False,
+    'description': None,
+  })
+
+  markers.append({
+    'type': 'commit-state',
+    'name': None,
+    'node': node.hex(active_node),
+    'isActive': True,
+    'isClosed': False,
+    'isTip': False,
+    'description': repo[b'.'].description(),
+  })
 
   return markers