From 79a6dfd7a99f1caed0209ce88ebeedab2a55ef9f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Jul 2020 14:05:16 -0700 Subject: [PATCH 1/7] Fix a MarkerRef call to get the active bookmark in Mercurial Summary: See PHI1805. This call is constructed improperly and can lead to a fatal in `arc patch` under Mercurial. Test Plan: In Mercurial, ran a valid `arc patch` operation. Differential Revision: https://secure.phabricator.com/D21391 --- src/repository/api/ArcanistMercurialAPI.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php index cffb9306..cbc6b5a0 100644 --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -890,7 +890,10 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { public function getActiveBookmark() { $bookmark = $this->newMarkerRefQuery() - ->withMarkerTypes(ArcanistMarkerRef::TYPE_BOOKMARK) + ->withMarkerTypes( + array( + ArcanistMarkerRef::TYPE_BOOKMARK, + )) ->withIsActive(true) ->executeOne(); From a28e76b7b3df25949c3be091715db05c7a08c2b3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Jul 2020 15:24:30 -0700 Subject: [PATCH 2/7] Allow "hg arc-ls-markers" to run under Python 2 or Python 3 Summary: Ref T13546. See PHI1805. Currently, the "arc-ls-markers" extension doesn't run under Python 3: - some stuff needs "b'...'" to mark it as a byte string; - "dict.iteritems()" is gone in Python 3, and "mercurial.pycompat" isn't always available; - in Python 3, "json" refuses to print byte strings; and - the compiler caching behavior in Python 3 has changed. Try to get these things working in the same way under Python 2 and Python 3. Test Plan: Ran this command (with `python` as Python 2, locally): ``` $ python /usr/local/bin/hg --config 'extensions.arc-hg=/Users/epriestley/dev/core/lib/arcanist/support/hg/arc-hg.py' arc-ls-markers -- ``` ...and this command: ``` $ python3 /usr/local/bin/hg --config 'extensions.arc-hg=/Users/epriestley/dev/core/lib/arcanist/support/hg/arc-hg.py' arc-ls-markers -- ``` ..and saw the same output in both cases (previously, `python3 ...` fataled in various ways). Maniphest Tasks: T13546 Differential Revision: https://secure.phabricator.com/D21392 --- .gitignore | 1 + support/hg/arc-hg.py | 31 ++++++++++++++++++++++++++----- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index d40646a4..0e13f861 100644 --- a/.gitignore +++ b/.gitignore @@ -36,3 +36,4 @@ # Python extension compiled files. /support/hg/arc-hg.pyc +/support/hg/__pycache__/ diff --git a/support/hg/arc-hg.py b/support/hg/arc-hg.py index a11cef73..94e4d3b1 100644 --- a/support/hg/arc-hg.py +++ b/support/hg/arc-hg.py @@ -1,4 +1,14 @@ from __future__ import absolute_import +import sys + +is_python_3 = sys.version_info[0] >= 3 + +if is_python_3: + def arc_items(dict): + return dict.items() +else: + def arc_items(dict): + return dict.iteritems() import os import json @@ -19,8 +29,8 @@ cmdtable = {} command = registrar.command(cmdtable) @command( - "arc-ls-markers", - [('', 'output', '', + b'arc-ls-markers', + [(b'', b'output', b'', _('file to output refs to'), _('FILE')), ] + cmdutil.remoteopts, _('[--output FILENAME] [SOURCE]')) @@ -42,6 +52,16 @@ def lsmarkers(ui, repo, source=None, **opts): else: markers = remotemarkers(ui, repo, source, opts) + for m in markers: + if m['name'] != None: + m['name'] = m['name'].decode('utf-8') + + if m['node'] != None: + m['node'] = m['node'].decode('utf-8') + + if m['description'] != None: + m['description'] = m['description'].decode('utf-8') + json_opts = { 'indent': 2, 'sort_keys': True, @@ -54,14 +74,15 @@ def lsmarkers(ui, repo, source=None, **opts): with open(output_file, 'w+') as f: json.dump(markers, f, **json_opts) else: - print json.dumps(markers, output_file, **json_opts) + json_data = json.dumps(markers, **json_opts) + print(json_data) return 0 def localmarkers(ui, repo): markers = [] - active_node = repo['.'].node() + active_node = repo[b'.'].node() all_heads = set(repo.heads()) current_name = repo.dirstate.branch() saw_current = False @@ -117,7 +138,7 @@ def localmarkers(ui, repo): bookmarks = repo._bookmarks active_bookmark = repo._activebookmark - for bookmark_name, bookmark_node in bookmarks.iteritems(): + for bookmark_name, bookmark_node in arc_items(bookmarks): is_active = (active_bookmark == bookmark_name) description = repo[bookmark_node].description() From 41774ba9cceb2a8ba6c15b3f256a04898d811fd4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 7 Jul 2020 10:12:09 -0700 Subject: [PATCH 3/7] Fix additional Mercurial/Python compatibility issues in "arc land" Summary: Ref PHI1805. Under some combination of versions (Python 3.8?), "arc-ls-markers" is running into additional Python runtime issues. Sprinkle more "b" around to resolve them? Also clean up a couple of plain "arc" issues. Test Plan: Landed a change in Mercurial. Some of this works fine without changes in Python 3.7/2.7 against Mercurial 4.7/5.4, so this may not be exhaustive. Differential Revision: https://secure.phabricator.com/D21393 --- src/land/engine/ArcanistMercurialLandEngine.php | 6 +++--- support/hg/arc-hg.py | 14 ++++++++------ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/land/engine/ArcanistMercurialLandEngine.php b/src/land/engine/ArcanistMercurialLandEngine.php index 0fb8c6a9..59ccabab 100644 --- a/src/land/engine/ArcanistMercurialLandEngine.php +++ b/src/land/engine/ArcanistMercurialLandEngine.php @@ -116,7 +116,7 @@ final class ArcanistMercurialLandEngine throw new PhutilArgumentUsageException( pht( 'Symbol "%s" is ambiguous.', - $symbol)); + $raw_symbol)); } $marker = head($named_markers); @@ -493,6 +493,7 @@ final class ArcanistMercurialLandEngine } protected function selectIntoCommit() { + $api = $this->getRepositoryAPI(); $log = $this->getLogEngine(); if ($this->getIntoEmpty()) { @@ -508,7 +509,6 @@ final class ArcanistMercurialLandEngine if ($this->getIntoLocal()) { // If we're running under "--into-local", just make sure that the // target identifies some actual commit. - $api = $this->getRepositoryAPI(); $local_ref = $this->getIntoRef(); // TODO: This error handling could probably be cleaner, it will just @@ -834,7 +834,7 @@ final class ArcanistMercurialLandEngine try { foreach ($body as $command) { - $this->newPasthru('%Ls', $command); + $this->newPassthru('%Ls', $command); } } finally { foreach ($tail as $command) { diff --git a/support/hg/arc-hg.py b/support/hg/arc-hg.py index 94e4d3b1..9bf18aa0 100644 --- a/support/hg/arc-hg.py +++ b/support/hg/arc-hg.py @@ -31,9 +31,9 @@ command = registrar.command(cmdtable) @command( b'arc-ls-markers', [(b'', b'output', b'', - _('file to output refs to'), _('FILE')), + _(b'file to output refs to'), _(b'FILE')), ] + cmdutil.remoteopts, - _('[--output FILENAME] [SOURCE]')) + _(b'[--output FILENAME] [SOURCE]')) def lsmarkers(ui, repo, source=None, **opts): """list markers @@ -166,7 +166,7 @@ def localmarkers(ui, repo): 'isClosed': False, 'isTip': False, 'isCurrent': True, - 'description': repo['.'].description(), + 'description': repo[b'.'].description(), }) return markers @@ -181,7 +181,7 @@ def remotemarkers(ui, repo, source, opts): remote = hg.peer(repo, opts, source) with remote.commandexecutor() as e: - branchmap = e.callcommand('branchmap', {}).result() + branchmap = e.callcommand(b'branchmap', {}).result() for branch_name in branchmap: for branch_node in branchmap[branch_name]: @@ -189,11 +189,12 @@ def remotemarkers(ui, repo, source, opts): 'type': 'branch', 'name': branch_name, 'node': node.hex(branch_node), + 'description': None, }) with remote.commandexecutor() as e: - remotemarks = bookmarks.unhexlifybookmarks(e.callcommand('listkeys', { - 'namespace': 'bookmarks', + remotemarks = bookmarks.unhexlifybookmarks(e.callcommand(b'listkeys', { + b'namespace': b'bookmarks', }).result()) for mark in remotemarks: @@ -201,6 +202,7 @@ def remotemarkers(ui, repo, source, opts): 'type': 'bookmark', 'name': mark, 'node': node.hex(remotemarks[mark]), + 'description': None, }) return markers From 710bceab10319beec84c7e4cb204c5462a88a30d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Jul 2020 13:31:55 -0700 Subject: [PATCH 4/7] When "arc land" fails a Mercurial push, actually raise it as an exception Summary: See PHI1808. Some refactoring of the "passthru" API resulted in error conditinos here being dropped. Instead, raise them as exceptions. Test Plan: Forced "hg push" to fail, used "arc land" against a failed push, saw error behavior instead of "success" feedback. Differential Revision: https://secure.phabricator.com/D21394 --- src/land/engine/ArcanistMercurialLandEngine.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/land/engine/ArcanistMercurialLandEngine.php b/src/land/engine/ArcanistMercurialLandEngine.php index 59ccabab..cbab0033 100644 --- a/src/land/engine/ArcanistMercurialLandEngine.php +++ b/src/land/engine/ArcanistMercurialLandEngine.php @@ -834,7 +834,12 @@ final class ArcanistMercurialLandEngine try { foreach ($body as $command) { - $this->newPassthru('%Ls', $command); + $err = $this->newPassthru('%Ls', $command); + if ($err) { + throw new ArcanistUsageException( + pht( + 'Push failed! Fix the error and run "arc land" again.')); + } } } finally { foreach ($tail as $command) { From 3633364bb96fa65a1dfcd754279a7b0d108b9943 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Jul 2020 13:35:18 -0700 Subject: [PATCH 5/7] Clean up push failure messaging in "arc land" slightly Summary: Ref PHI1808. Currently, push failures are messaged awkwardly. Make this exception handling more selective and the user-facing behavior more readable. Test Plan: Ran "arc land" against a failing remote, saw a human-readable message instead of a stack trace. Differential Revision: https://secure.phabricator.com/D21395 --- src/__phutil_library_map__.php | 2 ++ src/land/engine/ArcanistGitLandEngine.php | 10 ++-------- src/land/engine/ArcanistLandEngine.php | 5 +++-- src/land/engine/ArcanistMercurialLandEngine.php | 2 +- .../exception/ArcanistLandPushFailureException.php | 4 ++++ 5 files changed, 12 insertions(+), 11 deletions(-) create mode 100644 src/land/exception/ArcanistLandPushFailureException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 90e2e2ef..23a9df4b 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/land/engine/ArcanistGitLandEngine.php b/src/land/engine/ArcanistGitLandEngine.php index b67d0792..76cce096 100644 --- a/src/land/engine/ArcanistGitLandEngine.php +++ b/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( diff --git a/src/land/engine/ArcanistLandEngine.php b/src/land/engine/ArcanistLandEngine.php index a3e36ee7..1bafeb23 100644 --- a/src/land/engine/ArcanistLandEngine.php +++ b/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) { diff --git a/src/land/engine/ArcanistMercurialLandEngine.php b/src/land/engine/ArcanistMercurialLandEngine.php index cbab0033..f737c087 100644 --- a/src/land/engine/ArcanistMercurialLandEngine.php +++ b/src/land/engine/ArcanistMercurialLandEngine.php @@ -836,7 +836,7 @@ final class ArcanistMercurialLandEngine foreach ($body as $command) { $err = $this->newPassthru('%Ls', $command); if ($err) { - throw new ArcanistUsageException( + throw new ArcanistLandPushFailureException( pht( 'Push failed! Fix the error and run "arc land" again.')); } diff --git a/src/land/exception/ArcanistLandPushFailureException.php b/src/land/exception/ArcanistLandPushFailureException.php new file mode 100644 index 00000000..09b1c22a --- /dev/null +++ b/src/land/exception/ArcanistLandPushFailureException.php @@ -0,0 +1,4 @@ + Date: Wed, 8 Jul 2020 14:10:00 -0700 Subject: [PATCH 6/7] When saving and restoring local state in Mercurial, also save and restore bookmarks Summary: Ref PHI1808. In Mercurial, we must save and restore bookmark state explicitly. - Save and restore bookmarks. - Clean up concepts in "arc-ls-markers" slightly, so we don't need separate "isCurrent" and "isActive" flags, hopefully. I believe the totality of Mercurial state is: - A (non-bare) working copy points at exactly one commit (which might be the empty/null commit, in an empty repository). - A working copy has exactly one active branch. - Each branch has zero or more heads. - Each head may be closed. - Each (non-null) commit belongs to exactly one branch. - Note that the active branch may have zero heads and zero commits which belong to it! - A working copy has zero or one active bookmark. To capture this, we now emit: - A list of branch heads. If a branch head is a working copy commit, that head is flagged as active. - A list of bookmarks. If a bookmark is the current bookmark, that bookmark is flagged as active. - A single "branch-state" virtual marker. This covers the case where you have run "hg branch X" to create X, but no objects in the working copy actually correspond to X yet. It also covers the case where you are on a concrete branch, but not any head of that branch. - A single "commit-state" virtual marker. This always shows the current commit in the working copy. Test Plan: - Useful states to test are: - Empty repository (not all commands currently work here). - Normal repository, on a bookmark. - Normal repository, no bookmark. - "hg up 123" to update to somewhere in history. - "hg branch X", to start a new branch with no commits. - Ran "arc branches" and "arc bookmarks" in various states. Saw generally sensible output. - Ran "arc land --hold ..." in various states against a failing remote. Saw generally sensible output, and saw working properly restored to the original state. Differential Revision: https://secure.phabricator.com/D21396 --- src/repository/marker/ArcanistMarkerRef.php | 10 ++ ...ArcanistMercurialRepositoryMarkerQuery.php | 16 +-- .../state/ArcanistMercurialLocalState.php | 97 ++++++++++++++++--- support/hg/arc-hg.py | 79 +++++++-------- 4 files changed, 134 insertions(+), 68 deletions(-) diff --git a/src/repository/marker/ArcanistMarkerRef.php b/src/repository/marker/ArcanistMarkerRef.php index c580ab56..bc196c59 100644 --- a/src/repository/marker/ArcanistMarkerRef.php +++ b/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); } diff --git a/src/repository/marker/ArcanistMercurialRepositoryMarkerQuery.php b/src/repository/marker/ArcanistMercurialRepositoryMarkerQuery.php index 0cca7d76..fd12e9aa 100644 --- a/src/repository/marker/ArcanistMercurialRepositoryMarkerQuery.php +++ b/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); diff --git a/src/repository/state/ArcanistMercurialLocalState.php b/src/repository/state/ArcanistMercurialLocalState.php index a9ad0a31..fd551547 100644 --- a/src/repository/state/ArcanistMercurialLocalState.php +++ b/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; } diff --git a/support/hg/arc-hg.py b/support/hg/arc-hg.py index 9bf18aa0..01ac3907 100644 --- a/support/hg/arc-hg.py +++ b/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 From 65cda1596f25bb9daea1d78c46402ab61df073d5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 Jul 2020 16:16:54 -0700 Subject: [PATCH 7/7] Preserve bookmarks across "hg rebase --keep --collapse", and destroy them before "hg strip/prune" Summary: See PHI1808. Currently, "arc land " does not destroy the bookmark in Mercurial. There are three issues here: - "hg rebase --keep --collapse" moves bookmarks to the rewritten commits; - "hg strip" moves bookmarks backwards; - "hg prune" moves bookmarks backwards. To get around "hg rebase", save and restore bookmark state. To get around "hg strip" and "hg prune", explicitly destroy bookmarks pointing at commits before we strip/prune those commits. Test Plan: - Ran "arc land --trace". Saw arc reset the bookmark position after rebasing, and destroy the bookmark explicitly before stripping. - When the workflow exited, saw no more bookmark (previously: bookmark existed and pointed at a possibly-intermediate state). Differential Revision: https://secure.phabricator.com/D21397 --- .../engine/ArcanistMercurialLandEngine.php | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/src/land/engine/ArcanistMercurialLandEngine.php b/src/land/engine/ArcanistMercurialLandEngine.php index f737c087..6c27abca 100644 --- a/src/land/engine/ArcanistMercurialLandEngine.php +++ b/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); @@ -1003,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. @@ -1015,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).')'; @@ -1026,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',