From 2d0e7c37e1f599111ea199d502d236788a63341f Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 22 Jan 2021 13:32:37 -0800 Subject: [PATCH] Rename "IMPORTED_CLOSEABLE" to "IMPORTED_PERMANENT" to clarify the meaning of the flag Summary: Ref T13591. This is an old flag with an old name, and there's an import bug because the outdated concept of "closable" is confusing two different behaviors. This flag should mean only "is this commit reachable from a permanent ref?". Rename it to "IMPORTED_PERMANENT" to make that more clear. Rename the "Unpublished" query to "Permanent" to make that more clear, as well. Test Plan: - Grepped for all affected symbols. - Queried for all commmits, permament commits, and impermanent commits. - Ran repository discovery. - See also further changes in this change series for more extensive tests. Maniphest Tasks: T13591 Differential Revision: https://secure.phabricator.com/D21514 --- .../query/PhabricatorCommitSearchEngine.php | 14 +++++----- .../diffusion/query/DiffusionCommitQuery.php | 24 ++++++++--------- .../engine/PhabricatorRepositoryCommitRef.php | 10 +++---- .../PhabricatorRepositoryDiscoveryEngine.php | 20 +++++++------- .../engine/PhabricatorRepositoryRefEngine.php | 26 +++++++++---------- .../storage/PhabricatorRepositoryCommit.php | 4 +-- 6 files changed, 49 insertions(+), 49 deletions(-) diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php index 37e8d563b4..8b7751c795 100644 --- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -54,8 +54,8 @@ final class PhabricatorCommitSearchEngine $query->withUnreachable($map['unreachable']); } - if ($map['unpublished'] !== null) { - $query->withUnpublished($map['unpublished']); + if ($map['permanent'] !== null) { + $query->withPermanent($map['permanent']); } if ($map['ancestorsOf']) { @@ -132,15 +132,15 @@ final class PhabricatorCommitSearchEngine 'Find or exclude unreachable commits which are not ancestors of '. 'any branch, tag, or ref.')), id(new PhabricatorSearchThreeStateField()) - ->setLabel(pht('Unpublished')) - ->setKey('unpublished') + ->setLabel(pht('Permanent')) + ->setKey('permanent') ->setOptions( pht('(Show All)'), - pht('Show Only Unpublished Commits'), - pht('Hide Unpublished Commits')) + pht('Show Only Permanent Commits'), + pht('Hide Permanent Commits')) ->setDescription( pht( - 'Find or exclude unpublished commits which are not ancestors of '. + 'Find or exclude permanent commits which are ancestors of '. 'any permanent branch, tag, or ref.')), id(new PhabricatorSearchStringListField()) ->setLabel(pht('Ancestors Of')) diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index 2854aecbdd..7b4e80bfec 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -15,7 +15,7 @@ final class DiffusionCommitQuery private $statuses; private $packagePHIDs; private $unreachable; - private $unpublished; + private $permanent; private $needAuditRequests; private $needAuditAuthority; @@ -154,8 +154,8 @@ final class DiffusionCommitQuery return $this; } - public function withUnpublished($unpublished) { - $this->unpublished = $unpublished; + public function withPermanent($permanent) { + $this->permanent = $permanent; return $this; } @@ -859,18 +859,18 @@ final class DiffusionCommitQuery } } - if ($this->unpublished !== null) { - if ($this->unpublished) { - $where[] = qsprintf( - $conn, - '(commit.importStatus & %d) = 0', - PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE); - } else { + if ($this->permanent !== null) { + if ($this->permanent) { $where[] = qsprintf( $conn, '(commit.importStatus & %d) = %d', - PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE, - PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE); + PhabricatorRepositoryCommit::IMPORTED_PERMANENT, + PhabricatorRepositoryCommit::IMPORTED_PERMANENT); + } else { + $where[] = qsprintf( + $conn, + '(commit.importStatus & %d) = 0', + PhabricatorRepositoryCommit::IMPORTED_PERMANENT); } } diff --git a/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php b/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php index 31c859d5d8..148b99f3e0 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php +++ b/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php @@ -5,7 +5,7 @@ final class PhabricatorRepositoryCommitRef extends Phobject { private $identifier; private $epoch; private $branch; - private $canCloseImmediately; + private $isPermanent; private $parents = array(); public function setIdentifier($identifier) { @@ -35,13 +35,13 @@ final class PhabricatorRepositoryCommitRef extends Phobject { return $this->branch; } - public function setCanCloseImmediately($can_close_immediately) { - $this->canCloseImmediately = $can_close_immediately; + public function setIsPermanent($is_permanent) { + $this->isPermanent = $is_permanent; return $this; } - public function getCanCloseImmediately() { - return $this->canCloseImmediately; + public function getIsPermanent() { + return $this->isPermanent; } public function setParents(array $parents) { diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php index 8ac52b0b57..1c27e38e99 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php @@ -101,7 +101,7 @@ final class PhabricatorRepositoryDiscoveryEngine $repository, $ref->getIdentifier(), $ref->getEpoch(), - $ref->getCanCloseImmediately(), + $ref->getIsPermanent(), $ref->getParents(), $task_priority); @@ -250,7 +250,7 @@ final class PhabricatorRepositoryDiscoveryEngine $refs[$identifier] = id(new PhabricatorRepositoryCommitRef()) ->setIdentifier($identifier) ->setEpoch($epoch) - ->setCanCloseImmediately(true); + ->setIsPermanent(true); if ($upper_bound === null) { $upper_bound = $identifier; @@ -354,7 +354,7 @@ final class PhabricatorRepositoryDiscoveryEngine $branch_refs = $this->discoverStreamAncestry( new PhabricatorMercurialGraphStream($repository, $commit), $commit, - $close_immediately = true); + $is_permanent = true); $this->didDiscoverRefs($branch_refs); @@ -371,7 +371,7 @@ final class PhabricatorRepositoryDiscoveryEngine private function discoverStreamAncestry( PhabricatorRepositoryGraphStream $stream, $commit, - $close_immediately) { + $is_permanent) { $discover = array($commit); $graph = array(); @@ -424,7 +424,7 @@ final class PhabricatorRepositoryDiscoveryEngine $refs[] = id(new PhabricatorRepositoryCommitRef()) ->setIdentifier($commit) ->setEpoch($epoch) - ->setCanCloseImmediately($close_immediately) + ->setIsPermanent($is_permanent) ->setParents($stream->getParents($commit)); } @@ -507,8 +507,8 @@ final class PhabricatorRepositoryDiscoveryEngine } /** - * Sort branches so we process closeable branches first. This makes the - * whole import process a little cheaper, since we can close these commits + * Sort branches so we process permanent branches first. This makes the + * whole import process a little cheaper, since we can publish these commits * the first time through rather than catching them in the refs step. * * @task internal @@ -538,7 +538,7 @@ final class PhabricatorRepositoryDiscoveryEngine PhabricatorRepository $repository, $commit_identifier, $epoch, - $close_immediately, + $is_permanent, array $parents, $task_priority) { @@ -570,8 +570,8 @@ final class PhabricatorRepositoryDiscoveryEngine $commit->setRepositoryID($repository->getID()); $commit->setCommitIdentifier($commit_identifier); $commit->setEpoch($epoch); - if ($close_immediately) { - $commit->setImportStatus(PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE); + if ($is_permanent) { + $commit->setImportStatus(PhabricatorRepositoryCommit::IMPORTED_PERMANENT); } $data = new PhabricatorRepositoryCommitData(); diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php index 0166e8e53c..18dbf754f9 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php @@ -9,7 +9,7 @@ final class PhabricatorRepositoryRefEngine private $newPositions = array(); private $deadPositions = array(); - private $closeCommits = array(); + private $permanentCommits = array(); private $rebuild; public function setRebuild($rebuild) { @@ -24,7 +24,7 @@ final class PhabricatorRepositoryRefEngine public function updateRefs() { $this->newPositions = array(); $this->deadPositions = array(); - $this->closeCommits = array(); + $this->permanentCommits = array(); $repository = $this->getRepository(); $viewer = $this->getViewer(); @@ -96,8 +96,8 @@ final class PhabricatorRepositoryRefEngine $this->updateCursors($cursor_group, $refs, $type, $all_closing_heads); } - if ($this->closeCommits) { - $this->setCloseFlagOnCommits($this->closeCommits); + if ($this->permanentCommits) { + $this->setPermanentFlagOnCommits($this->permanentCommits); } $save_cursors = $this->getCursorsForUpdate($all_cursors); @@ -217,9 +217,9 @@ final class PhabricatorRepositoryRefEngine return $this; } - private function markCloseCommits(array $identifiers) { + private function markPermanentCommits(array $identifiers) { foreach ($identifiers as $identifier) { - $this->closeCommits[$identifier] = $identifier; + $this->permanentCommits[$identifier] = $identifier; } return $this; } @@ -377,7 +377,7 @@ final class PhabricatorRepositoryRefEngine $identifier, $exclude); - $this->markCloseCommits($new_identifiers); + $this->markPermanentCommits($new_identifiers); } } } @@ -507,10 +507,10 @@ final class PhabricatorRepositoryRefEngine } /** - * Mark a list of commits as closeable, and queue workers for those commits + * Mark a list of commits as permanent, and queue workers for those commits * which don't already have the flag. */ - private function setCloseFlagOnCommits(array $identifiers) { + private function setPermanentFlagOnCommits(array $identifiers) { $repository = $this->getRepository(); $commit_table = new PhabricatorRepositoryCommit(); $conn = $commit_table->establishConnection('w'); @@ -552,7 +552,7 @@ final class PhabricatorRepositoryRefEngine } } - $closeable_flag = PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE; + $permanent_flag = PhabricatorRepositoryCommit::IMPORTED_PERMANENT; $published_flag = PhabricatorRepositoryCommit::IMPORTED_PUBLISH; $all_commits = ipull($all_commits, null, 'commitIdentifier'); @@ -568,9 +568,9 @@ final class PhabricatorRepositoryRefEngine } $import_status = $row['importStatus']; - if (!($import_status & $closeable_flag)) { - // Set the "closeable" flag. - $import_status = ($import_status | $closeable_flag); + if (!($import_status & $permanent_flag)) { + // Set the "permanent" flag. + $import_status = ($import_status | $permanent_flag); // See T13580. Clear the "published" flag, so publishing executes // again. We may have previously performed a no-op "publish" on the diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 586f1e2d0a..5fb089953d 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -36,7 +36,7 @@ final class PhabricatorRepositoryCommit const IMPORTED_PUBLISH = 8; const IMPORTED_ALL = 11; - const IMPORTED_CLOSEABLE = 1024; + const IMPORTED_PERMANENT = 1024; const IMPORTED_UNREACHABLE = 2048; private $commitData = self::ATTACHABLE; @@ -467,7 +467,7 @@ final class PhabricatorRepositoryCommit } public function isPermanentCommit() { - return (bool)$this->isPartiallyImported(self::IMPORTED_CLOSEABLE); + return (bool)$this->isPartiallyImported(self::IMPORTED_PERMANENT); } public function newCommitAuthorView(PhabricatorUser $viewer) {