From 06edcf270921a9d5c1c4be3c006dfc15253506fa Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 24 Sep 2019 16:20:23 -0700 Subject: [PATCH] Fix an issue where ancestors of permanent refs might not be published during import or if a branch is later made permanent Summary: Fixes T13284. See that task for substantial discussion. There are currently two cases where we'll skip over commits which we should publish: - if a branch is not permanent, then later made permanent; or - in some cases, the first time we examine branches in a repository. In both cases, this error is one-shot and things work correctly going forward. The root cause is conflation between the states "this ref currently permanent" and "this ref was permanent the last time we updated refs". Separate these pieces of state and cover all these cases. Also introduce a "--rebuild" flag to fix the state of bad commits. Test Plan: See T13284 for the three major cases: - initial import; - push changes to a nonpermanent branch, update, then make it permanent; - push chanegs to a nonpermanent branch, update, push more changes, then make it permanent. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13284 Differential Revision: https://secure.phabricator.com/D20829 --- .../20190924.diffusion.01.permanent.sql | 2 + .../20190924.diffusion.02.default.sql | 2 + .../DiffusionLowLevelResolveRefsQuery.php | 16 +-- .../engine/PhabricatorRepositoryRefEngine.php | 109 ++++++++++++++---- ...icatorRepositoryManagementRefsWorkflow.php | 7 ++ .../PhabricatorRepositoryRefCursor.php | 2 + 6 files changed, 109 insertions(+), 29 deletions(-) create mode 100644 resources/sql/autopatches/20190924.diffusion.01.permanent.sql create mode 100644 resources/sql/autopatches/20190924.diffusion.02.default.sql diff --git a/resources/sql/autopatches/20190924.diffusion.01.permanent.sql b/resources/sql/autopatches/20190924.diffusion.01.permanent.sql new file mode 100644 index 0000000000..4c84f32a52 --- /dev/null +++ b/resources/sql/autopatches/20190924.diffusion.01.permanent.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_repository.repository_refcursor + ADD isPermanent BOOL NOT NULL; diff --git a/resources/sql/autopatches/20190924.diffusion.02.default.sql b/resources/sql/autopatches/20190924.diffusion.02.default.sql new file mode 100644 index 0000000000..2f639a5f84 --- /dev/null +++ b/resources/sql/autopatches/20190924.diffusion.02.default.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_repository.repository_refcursor + SET isPermanent = 1; diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php index f9e9f74774..197fa285dc 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php @@ -195,13 +195,15 @@ final class DiffusionLowLevelResolveRefsQuery $alternate = null; if ($type == 'tag') { - $alternate = $identifier; - $identifier = idx($tag_map, $ref); - if (!$identifier) { - throw new Exception( - pht( - "Failed to look up tag '%s'!", - $ref)); + $tag_identifier = idx($tag_map, $ref); + if ($tag_identifier === null) { + // This can happen when we're asked to resolve the hash of a "tag" + // object created with "git tag --annotate" that isn't currently + // reachable from any ref. Just leave things as they are. + } else { + // Otherwise, we have a normal named tag. + $alternate = $identifier; + $identifier = $tag_identifier; } } diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php index 72fb683401..f823ead6d9 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php @@ -10,7 +10,16 @@ final class PhabricatorRepositoryRefEngine private $newPositions = array(); private $deadPositions = array(); private $closeCommits = array(); - private $hasNoCursors; + private $rebuild; + + public function setRebuild($rebuild) { + $this->rebuild = $rebuild; + return $this; + } + + public function getRebuild() { + return $this->rebuild; + } public function updateRefs() { $this->newPositions = array(); @@ -60,15 +69,17 @@ final class PhabricatorRepositoryRefEngine ->execute(); $cursor_groups = mgroup($all_cursors, 'getRefType'); - $this->hasNoCursors = (!$all_cursors); - - // Find all the heads of closing refs. + // Find all the heads of permanent refs. $all_closing_heads = array(); foreach ($all_cursors as $cursor) { - $should_close = $this->shouldCloseRef( - $cursor->getRefType(), - $cursor->getRefName()); - if (!$should_close) { + + // See T13284. Note that we're considering whether this ref was a + // permanent ref or not the last time we updated refs for this + // repository. This allows us to handle things properly when a ref + // is reconfigured from non-permanent to permanent. + + $was_permanent = $cursor->getIsPermanent(); + if (!$was_permanent) { continue; } @@ -76,6 +87,7 @@ final class PhabricatorRepositoryRefEngine $all_closing_heads[] = $identifier; } } + $all_closing_heads = array_unique($all_closing_heads); $all_closing_heads = $this->removeMissingCommits($all_closing_heads); @@ -88,12 +100,18 @@ final class PhabricatorRepositoryRefEngine $this->setCloseFlagOnCommits($this->closeCommits); } - if ($this->newPositions || $this->deadPositions) { + $save_cursors = $this->getCursorsForUpdate($all_cursors); + + if ($this->newPositions || $this->deadPositions || $save_cursors) { $repository->openTransaction(); $this->saveNewPositions(); $this->deleteDeadPositions(); + foreach ($save_cursors as $cursor) { + $cursor->save(); + } + $repository->saveTransaction(); } @@ -103,6 +121,28 @@ final class PhabricatorRepositoryRefEngine } } + private function getCursorsForUpdate(array $cursors) { + assert_instances_of($cursors, 'PhabricatorRepositoryRefCursor'); + + $results = array(); + + foreach ($cursors as $cursor) { + $ref_type = $cursor->getRefType(); + $ref_name = $cursor->getRefName(); + + $is_permanent = $this->isPermanentRef($ref_type, $ref_name); + + if ($is_permanent == $cursor->getIsPermanent()) { + continue; + } + + $cursor->setIsPermanent((int)$is_permanent); + $results[] = $cursor; + } + + return $results; + } + private function updateBranchStates( PhabricatorRepository $repository, array $branches) { @@ -301,11 +341,41 @@ final class PhabricatorRepositoryRefEngine $this->markPositionNew($new_position); } - if ($this->shouldCloseRef($ref_type, $name)) { - foreach ($added_commits as $identifier) { + if ($this->isPermanentRef($ref_type, $name)) { + + // See T13284. If this cursor was already marked as permanent, we + // only need to publish the newly created ref positions. However, if + // this cursor was not previously permanent but has become permanent, + // we need to publish all the ref positions. + + // This corresponds to users reconfiguring a branch to make it + // permanent without pushing any new commits to it. + + $is_rebuild = $this->getRebuild(); + $was_permanent = $ref_cursor->getIsPermanent(); + + if ($is_rebuild || !$was_permanent) { + $update_all = true; + } else { + $update_all = false; + } + + if ($update_all) { + $update_commits = $new_commits; + } else { + $update_commits = $added_commits; + } + + if ($is_rebuild) { + $exclude = array(); + } else { + $exclude = $all_closing_heads; + } + + foreach ($update_commits as $identifier) { $new_identifiers = $this->loadNewCommitIdentifiers( $identifier, - $all_closing_heads); + $exclude); $this->markCloseCommits($new_identifiers); } @@ -334,19 +404,11 @@ final class PhabricatorRepositoryRefEngine } } - private function shouldCloseRef($ref_type, $ref_name) { + private function isPermanentRef($ref_type, $ref_name) { if ($ref_type !== PhabricatorRepositoryRefCursor::TYPE_BRANCH) { return false; } - if ($this->hasNoCursors) { - // If we don't have any cursors, don't close things. Particularly, this - // corresponds to the case where you've just updated to this code on an - // existing repository: we don't want to requeue message steps for every - // commit on a closeable ref. - return false; - } - return $this->getRepository()->isBranchPermanentRef($ref_name); } @@ -505,10 +567,13 @@ final class PhabricatorRepositoryRefEngine $ref_type, $ref_name) { + $is_permanent = $this->isPermanentRef($ref_type, $ref_name); + $cursor = id(new PhabricatorRepositoryRefCursor()) ->setRepositoryPHID($repository->getPHID()) ->setRefType($ref_type) - ->setRefName($ref_name); + ->setRefName($ref_name) + ->setIsPermanent((int)$is_permanent); try { return $cursor->save(); diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementRefsWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementRefsWorkflow.php index 8c806edac8..8d3062195c 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementRefsWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementRefsWorkflow.php @@ -14,6 +14,12 @@ final class PhabricatorRepositoryManagementRefsWorkflow 'name' => 'verbose', 'help' => pht('Show additional debugging information.'), ), + array( + 'name' => 'rebuild', + 'help' => pht( + 'Publish commits currently reachable from any permanent ref, '. + 'ignoring the cached ref state.'), + ), array( 'name' => 'repos', 'wildcard' => true, @@ -41,6 +47,7 @@ final class PhabricatorRepositoryManagementRefsWorkflow $engine = id(new PhabricatorRepositoryRefEngine()) ->setRepository($repo) ->setVerbose($args->getArg('verbose')) + ->setRebuild($args->getArg('rebuild')) ->updateRefs(); } diff --git a/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php b/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php index 87f737d86e..91261f2b93 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php +++ b/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php @@ -19,6 +19,7 @@ final class PhabricatorRepositoryRefCursor protected $refNameHash; protected $refNameRaw; protected $refNameEncoding; + protected $isPermanent; private $repository = self::ATTACHABLE; private $positions = self::ATTACHABLE; @@ -34,6 +35,7 @@ final class PhabricatorRepositoryRefCursor 'refType' => 'text32', 'refNameHash' => 'bytes12', 'refNameEncoding' => 'text16?', + 'isPermanent' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( 'key_ref' => array(