mirror of
https://we.phorge.it/source/phorge.git
synced 2025-01-11 07:11:04 +01:00
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
This commit is contained in:
parent
78aab6d4a5
commit
e910c76e65
6 changed files with 202 additions and 10 deletions
|
@ -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',
|
||||
|
|
|
@ -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')
|
||||
|
|
|
@ -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(
|
||||
|
|
|
@ -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))) {
|
||||
|
|
|
@ -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 )------------------------------------------ */
|
||||
|
||||
|
|
|
@ -0,0 +1,94 @@
|
|||
<?php
|
||||
|
||||
final class PhabricatorRepositoryFetchRefsTransaction
|
||||
extends PhabricatorRepositoryTransactionType {
|
||||
|
||||
const TRANSACTIONTYPE = 'fetch-refs';
|
||||
|
||||
public function generateOldValue($object) {
|
||||
return $object->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;
|
||||
}
|
||||
|
||||
}
|
Loading…
Reference in a new issue