From 99231ebba42422e81039876c69e2183fa8f96a9e Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 29 Dec 2011 09:14:55 -0800 Subject: [PATCH] Allow Git repositories to track only some branches Summary: Some installs use Git as the backbone of a CI framework or use a Git remote to share patches. The tracker scripts currently recognize associated revisions as "Committed" when they appear in any branch, even if that branch is "alincoln-personal-development_test_hack" or whatever. To address the broadest need here, allow Git repositories to be configured to track only certain branches instead of all branches. This doesn't allow you to import a branch into Diffusion but ignore it in Differential. Supporting that is somewhat technically complicated because the parser currently goes like this: - Look at HEAD of all branches. - For any commits we haven't seen before, follow them back to something we have seen (or the root). - "Discover" everything new. Since this doesn't track pairs, we currently don't have enough information to tell when a commit appears in a branch for the first time, so we don't have anywhere we can put a test for whether that branch is tracked and do the Differential hook only if it is. However, I think this cruder patch satisfies most of the need and is simple and obvious in its implementation. See also D1263. Test Plan: - Updated a Git repository with various filters: "", "master, remote", "derp", " ,,, master ,,,,," - Edited SVN and Mercurial repositories to verify they didn't get caught in the crossfire. - Ran daemon in debug mode on libphutil with filter "derp", got exception about no tracked branches. Ran with filter "master", got tracking. Ran with no filter, got tracking. - Looked at Diffusion with "derp" and "master", saw no branches and "master" respectively. - Added unit tests to cover filtering logic. Reviewers: btrahan, jungejason, nh, fratrik Reviewed By: fratrik CC: aran, fratrik, epriestley Maniphest Tasks: T270 Differential Revision: https://secure.phabricator.com/D1290 --- .../branch/git/DiffusionGitBranchQuery.php | 6 +++- .../PhabricatorRepositoryEditController.php | 29 ++++++++++++++++-- ...ricatorRepositoryCommitDiscoveryDaemon.php | 12 ++++---- ...atorRepositoryGitCommitDiscoveryDaemon.php | 17 ++++++++++- .../repository/PhabricatorRepository.php | 20 ++++++++++++- .../PhabricatorRepositoryTestCase.php | 30 ++++++++++++++++++- .../storage/repository/__tests__/__init__.php | 1 + 7 files changed, 104 insertions(+), 11 deletions(-) diff --git a/src/applications/diffusion/query/branch/git/DiffusionGitBranchQuery.php b/src/applications/diffusion/query/branch/git/DiffusionGitBranchQuery.php index a420cbd8bb..dc58784ebc 100644 --- a/src/applications/diffusion/query/branch/git/DiffusionGitBranchQuery.php +++ b/src/applications/diffusion/query/branch/git/DiffusionGitBranchQuery.php @@ -1,7 +1,7 @@ $head) { + if (!$repository->shouldTrackBranch($name)) { + continue; + } + $branch = new DiffusionBranchInformation(); $branch->setName($name); $branch->setHeadCommitIdentifier($head); diff --git a/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php b/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php index 25c95ee1ab..b8b34f07ca 100644 --- a/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php +++ b/src/applications/repository/controller/edit/PhabricatorRepositoryEditController.php @@ -1,7 +1,7 @@ isFormPost()) { @@ -238,6 +239,14 @@ class PhabricatorRepositoryEditController if ($has_local) { $repository->setDetail('local-path', $request->getStr('path')); } + + if ($has_branch_filter) { + $branch_filter = $request->getStrList('branch-filter'); + $branch_filter = array_fill_keys($branch_filter, true); + + $repository->setDetail('branch-filter', $branch_filter); + } + $repository->setDetail( 'pull-frequency', max(1, $request->getInt('frequency'))); @@ -548,6 +557,22 @@ class PhabricatorRepositoryEditController ->setError($e_path)); } + if ($has_branch_filter) { + $branch_filter_str = implode( + ', ', + array_keys($repository->getDetail('branch-filter', array()))); + $form + ->appendChild( + id(new AphrontFormTextControl()) + ->setName('branch-filter') + ->setLabel('Track Only') + ->setValue($branch_filter_str) + ->setCaption( + 'Optional list of branches to track. Other branches will be '. + 'completely ignored. If left empty, all branches are tracked. '. + 'Example: master, release')); + } + $form ->appendChild( id(new AphrontFormTextControl()) @@ -584,7 +609,7 @@ class PhabricatorRepositoryEditController $default_branch_name)) ->setError($e_branch) ->setCaption( - 'Default remote branch to show in Diffusion.')); + 'Default branch to show in Diffusion.')); } $form diff --git a/src/applications/repository/daemon/commitdiscovery/base/PhabricatorRepositoryCommitDiscoveryDaemon.php b/src/applications/repository/daemon/commitdiscovery/base/PhabricatorRepositoryCommitDiscoveryDaemon.php index bbc5422718..c4d60a6d94 100644 --- a/src/applications/repository/daemon/commitdiscovery/base/PhabricatorRepositoryCommitDiscoveryDaemon.php +++ b/src/applications/repository/daemon/commitdiscovery/base/PhabricatorRepositoryCommitDiscoveryDaemon.php @@ -1,7 +1,7 @@ repository = $this->loadRepository(); - - $sleep = $this->repository->getDetail('pull-frequency'); while (true) { + // Reload the repository every time to pick up changes from the web + // console. + $this->repository = $this->loadRepository(); $this->discoverCommits(); - $this->sleep(max(2, $sleep)); + + $sleep = max(2, $this->getRepository()->getDetail('pull-frequency')); + $this->sleep($sleep); } } diff --git a/src/applications/repository/daemon/commitdiscovery/git/PhabricatorRepositoryGitCommitDiscoveryDaemon.php b/src/applications/repository/daemon/commitdiscovery/git/PhabricatorRepositoryGitCommitDiscoveryDaemon.php index 9f59c2550c..275d02eaca 100644 --- a/src/applications/repository/daemon/commitdiscovery/git/PhabricatorRepositoryGitCommitDiscoveryDaemon.php +++ b/src/applications/repository/daemon/commitdiscovery/git/PhabricatorRepositoryGitCommitDiscoveryDaemon.php @@ -1,7 +1,7 @@ $commit) { + if (!$repository->shouldTrackBranch($name)) { + continue; + } + + $tracked_something = true; + if ($this->isKnownCommit($commit)) { continue; } else { @@ -61,6 +68,14 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon } } + if (!$tracked_something) { + $repo_name = $repository->getName(); + $repo_callsign = $repository->getCallsign(); + throw new Exception( + "Repository r{$repo_callsign} '{$repo_name}' has no tracked branches! ". + "Verify that your branch filtering settings are correct."); + } + return $got_something; } diff --git a/src/applications/repository/storage/repository/PhabricatorRepository.php b/src/applications/repository/storage/repository/PhabricatorRepository.php index 87647e028f..1bcf27e3d2 100644 --- a/src/applications/repository/storage/repository/PhabricatorRepository.php +++ b/src/applications/repository/storage/repository/PhabricatorRepository.php @@ -1,7 +1,7 @@ getDetail('tracking-enabled', false); } + public function shouldTrackBranch($branch) { + $vcs = $this->getVersionControlSystem(); + + $is_git = ($vcs == PhabricatorRepositoryType::REPOSITORY_TYPE_GIT); + + $use_filter = ($is_git); + + if ($use_filter) { + $filter = $this->getDetail('branch-filter', array()); + if ($filter && !isset($filter[$branch])) { + return false; + } + } + + // By default, track all branches. + return true; + } + } diff --git a/src/applications/repository/storage/repository/__tests__/PhabricatorRepositoryTestCase.php b/src/applications/repository/storage/repository/__tests__/PhabricatorRepositoryTestCase.php index 0bc70a892c..7af6246422 100644 --- a/src/applications/repository/storage/repository/__tests__/PhabricatorRepositoryTestCase.php +++ b/src/applications/repository/storage/repository/__tests__/PhabricatorRepositoryTestCase.php @@ -1,7 +1,7 @@ setVersionControlSystem($git); + + $this->assertEqual( + true, + $repo->shouldTrackBranch('imaginary'), + 'Track all branches by default.'); + + $repo->setDetail( + 'branch-filter', + array( + 'master' => true, + )); + + $this->assertEqual( + true, + $repo->shouldTrackBranch('master'), + 'Track listed branches.'); + + $this->assertEqual( + false, + $repo->shouldTrackBranch('imaginary'), + 'Do not track unlisted branches.'); + } + } diff --git a/src/applications/repository/storage/repository/__tests__/__init__.php b/src/applications/repository/storage/repository/__tests__/__init__.php index 8676e844d1..60c3a605b7 100644 --- a/src/applications/repository/storage/repository/__tests__/__init__.php +++ b/src/applications/repository/storage/repository/__tests__/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/repository/constants/repositorytype'); phutil_require_module('phabricator', 'applications/repository/storage/repository'); phutil_require_module('phabricator', 'infrastructure/testing/testcase');