From e910c76e65e54a439a0af4735ae89c70076673d5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 14 Apr 2019 12:17:38 -0700 Subject: [PATCH] Add "Fetch Rules" to observed Git repositories Summary: Depends on D20421. Ref T13277. I'd generally like to move away from "Track Only". Some of the use cases for "Track Only" (or adjacent to "Track Only") are better resolved with "Fetch Rules" -- basically, rules to fetch only some subset of refs from the observed remote. Add configurable "Fetch Rules" for Git repositories. For example, if you only want to fetch `master`, you can now speify: ``` refs/heads/master ``` If you only want to fetch branches and tags, you can use: ``` refs/heads/* refs/tags/* ``` In theory, this is slightly less powerful in the general case than "Track Only", but gives us better behavior in some cases (e.g., when the remote has 50K random temporary branches). In practice, I think this and a better "Autoclose Only" will let us move away from "Track Only", get default behavior which is better aligned with what users actually expect, and dodge all the "track tags/refs" questions. Test Plan: Configured repositories with "Fetch Refs" rules, used `bin/repository pull --verbose --trace ...` to run pulls, saw expected pull/fetch behavior. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13277 Differential Revision: https://secure.phabricator.com/D20422 --- src/__phutil_library_map__.php | 2 + .../editor/DiffusionRepositoryEditEngine.php | 12 +++ ...usionRepositoryBranchesManagementPanel.php | 11 +++ .../PhabricatorRepositoryPullEngine.php | 77 +++++++++++++-- .../storage/PhabricatorRepository.php | 16 ++++ ...bricatorRepositoryFetchRefsTransaction.php | 94 +++++++++++++++++++ 6 files changed, 202 insertions(+), 10 deletions(-) create mode 100644 src/applications/repository/xaction/PhabricatorRepositoryFetchRefsTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9dfec9c718..36edb62c70 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4350,6 +4350,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryEngine' => 'applications/repository/engine/PhabricatorRepositoryEngine.php', 'PhabricatorRepositoryEnormousTransaction' => 'applications/repository/xaction/PhabricatorRepositoryEnormousTransaction.php', 'PhabricatorRepositoryFerretEngine' => 'applications/repository/search/PhabricatorRepositoryFerretEngine.php', + 'PhabricatorRepositoryFetchRefsTransaction' => 'applications/repository/xaction/PhabricatorRepositoryFetchRefsTransaction.php', 'PhabricatorRepositoryFilesizeLimitTransaction' => 'applications/repository/xaction/PhabricatorRepositoryFilesizeLimitTransaction.php', 'PhabricatorRepositoryFulltextEngine' => 'applications/repository/search/PhabricatorRepositoryFulltextEngine.php', 'PhabricatorRepositoryGitCommitChangeParserWorker' => 'applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php', @@ -10603,6 +10604,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryEngine' => 'Phobject', 'PhabricatorRepositoryEnormousTransaction' => 'PhabricatorRepositoryTransactionType', 'PhabricatorRepositoryFerretEngine' => 'PhabricatorFerretEngine', + 'PhabricatorRepositoryFetchRefsTransaction' => 'PhabricatorRepositoryTransactionType', 'PhabricatorRepositoryFilesizeLimitTransaction' => 'PhabricatorRepositoryTransactionType', 'PhabricatorRepositoryFulltextEngine' => 'PhabricatorFulltextEngine', 'PhabricatorRepositoryGitCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker', diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index 4e02ee99a0..b4bb587cda 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -212,6 +212,7 @@ final class DiffusionRepositoryEditEngine ->setObject($object) ->execute(); + $fetch_value = $object->getFetchRules(); $track_value = $object->getTrackOnlyRules(); $autoclose_value = $object->getAutocloseOnlyRules(); @@ -364,6 +365,17 @@ final class DiffusionRepositoryEditEngine ->setConduitDescription(pht('Set the default branch name.')) ->setConduitTypeDescription(pht('New default branch name.')) ->setValue($object->getDetail('default-branch')), + id(new PhabricatorTextAreaEditField()) + ->setIsStringList(true) + ->setKey('fetchRefs') + ->setLabel(pht('Fetch Refs')) + ->setTransactionType( + PhabricatorRepositoryFetchRefsTransaction::TRANSACTIONTYPE) + ->setIsCopyable(true) + ->setDescription(pht('Fetch only these refs.')) + ->setConduitDescription(pht('Set the fetched refs.')) + ->setConduitTypeDescription(pht('New fetched refs.')) + ->setValue($fetch_value), id(new PhabricatorTextAreaEditField()) ->setIsStringList(true) ->setKey('trackOnly') diff --git a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php index bdd0a3f71d..708263ae93 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php @@ -36,6 +36,7 @@ final class DiffusionRepositoryBranchesManagementPanel protected function getEditEngineFieldKeys() { return array( 'defaultBranch', + 'fetchRefs', 'trackOnly', 'autocloseOnly', ); @@ -78,6 +79,16 @@ final class DiffusionRepositoryBranchesManagementPanel phutil_tag('em', array(), $repository->getDefaultBranch())); $view->addProperty(pht('Default Branch'), $default_branch); + if ($repository->supportsFetchRules()) { + $fetch_only = $repository->getFetchRules(); + if ($fetch_only) { + $fetch_display = implode(', ', $fetch_only); + } else { + $fetch_display = phutil_tag('em', array(), pht('Fetch All Refs')); + } + $view->addProperty(pht('Fetch Refs'), $fetch_display); + } + $track_only_rules = $repository->getTrackOnlyRules(); $track_only_rules = implode(', ', $track_only_rules); $track_only = nonempty( diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index 5d0edb1f10..940c7fe84e 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -339,8 +339,17 @@ final class PhabricatorRepositoryPullEngine throw new Exception($message); } - $remote_refs = $this->loadGitRemoteRefs($repository); - $local_refs = $this->loadGitLocalRefs($repository); + // Load the refs we're planning to fetch from the remote repository. + $remote_refs = $this->loadGitRemoteRefs( + $repository, + $repository->getRemoteURIEnvelope()); + + // Load the refs we're planning to fetch from the local repository, by + // using the local working copy path as the "remote" repository URI. + $local_refs = $this->loadGitRemoteRefs( + $repository, + new PhutilOpaqueEnvelope($path)); + if ($remote_refs === $local_refs) { $this->log( pht( @@ -351,16 +360,49 @@ final class PhabricatorRepositoryPullEngine $this->logRefDifferences($remote_refs, $local_refs); + $fetch_rules = $this->getGitFetchRules($repository); + $future = $repository->getRemoteCommandFuture( - 'fetch %P %s --prune', + 'fetch --prune -- %P %Ls', $repository->getRemoteURIEnvelope(), - '+refs/*:refs/*'); + $fetch_rules); $future ->setCWD($path) ->resolvex(); } + private function getGitRefRules(PhabricatorRepository $repository) { + $ref_rules = $repository->getFetchRules($repository); + + if (!$ref_rules) { + $ref_rules = array( + 'refs/*', + ); + } + + return $ref_rules; + } + + private function getGitFetchRules(PhabricatorRepository $repository) { + $ref_rules = $this->getGitRefRules($repository); + + // Rewrite each ref rule "X" into "+X:X". + + // The "X" means "fetch ref X". + // The "...:X" means "...and copy it into local ref X". + // The "+..." means "...and overwrite the local ref if it already exists". + + $fetch_rules = array(); + foreach ($ref_rules as $key => $ref_rule) { + $fetch_rules[] = sprintf( + '+%s:%s', + $ref_rule, + $ref_rule); + } + + return $fetch_rules; + } /** * @task git @@ -378,15 +420,30 @@ final class PhabricatorRepositoryPullEngine $this->installHook($root.$path); } - private function loadGitRemoteRefs(PhabricatorRepository $repository) { - $remote_envelope = $repository->getRemoteURIEnvelope(); + private function loadGitRemoteRefs( + PhabricatorRepository $repository, + PhutilOpaqueEnvelope $remote_envelope) { + + $ref_rules = $this->getGitRefRules($repository); // NOTE: "git ls-remote" does not support "--" until circa January 2016. - // See T12416. None of the flags to "ls-remote" appear dangerous, and - // other checks make it difficult to configure a suspicious remote URI. + // See T12416. None of the flags to "ls-remote" appear dangerous, but + // refuse to list any refs beginning with "-" just in case. + + foreach ($ref_rules as $ref_rule) { + if (preg_match('/^-/', $ref_rule)) { + throw new Exception( + pht( + 'Refusing to list potentially dangerous ref ("%s") beginning '. + 'with "-".', + $ref_rule)); + } + } + list($stdout) = $repository->execxRemoteCommand( - 'ls-remote %P', - $remote_envelope); + 'ls-remote %P %Ls', + $remote_envelope, + $ref_rules); // Empty repositories don't have any refs. if (!strlen(rtrim($stdout))) { diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index cd5aa1d479..46e499e4be 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1213,6 +1213,22 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return $this; } + public function supportsFetchRules() { + if ($this->isGit()) { + return true; + } + + return false; + } + + public function getFetchRules() { + return $this->getDetail('fetch-rules', array()); + } + + public function setFetchRules(array $rules) { + return $this->setDetail('fetch-rules', $rules); + } + /* -( Repository URI Management )------------------------------------------ */ diff --git a/src/applications/repository/xaction/PhabricatorRepositoryFetchRefsTransaction.php b/src/applications/repository/xaction/PhabricatorRepositoryFetchRefsTransaction.php new file mode 100644 index 0000000000..2965b0b742 --- /dev/null +++ b/src/applications/repository/xaction/PhabricatorRepositoryFetchRefsTransaction.php @@ -0,0 +1,94 @@ +getFetchRules(); + } + + public function applyInternalEffects($object, $value) { + $object->setFetchRules($value); + } + + public function getTitle() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + if (!$new) { + return pht( + '%s set this repository to fetch all refs.', + $this->renderAuthor()); + } else if (!$old) { + return pht( + '%s set this repository to fetch refs: %s.', + $this->renderAuthor(), + $this->renderValue(implode(', ', $new))); + } else { + return pht( + '%s changed fetched refs from %s to %s.', + $this->renderAuthor(), + $this->renderValue(implode(', ', $old)), + $this->renderValue(implode(', ', $new))); + } + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + foreach ($xactions as $xaction) { + $new_value = $xaction->getNewValue(); + + if (!is_array($new_value) || !phutil_is_natural_list($new_value)) { + $errors[] = $this->newInvalidError( + pht( + 'Fetch rules must be a list of strings, got "%s".', + phutil_describe_type($new_value)), + $xaction); + continue; + } + + foreach ($new_value as $idx => $rule) { + if (!is_string($rule)) { + $errors[] = $this->newInvalidError( + pht( + 'Fetch rule (at index "%s") must be a string, got "%s".', + $idx, + phutil_describe_type($rule)), + $xaction); + continue; + } + + if (!strlen($rule)) { + $errors[] = $this->newInvalidError( + pht( + 'Fetch rule (at index "%s") is empty. Fetch rules must '. + 'contain text.', + $idx), + $xaction); + continue; + } + + // Since we fetch ref "X" as "+X:X", don't allow rules to include + // colons. This is specific to Git and may not be relevant if + // Mercurial repositories eventually get fetch rules. + if (preg_match('/:/', $rule)) { + $errors[] = $this->newInvalidError( + pht( + 'Fetch rule ("%s", at index "%s") is invalid: fetch rules '. + 'must not contain colons.', + $rule, + $idx), + $xaction); + continue; + } + + } + } + + return $errors; + } + +}