From 812c41a18abdd94be85f6902977ee3cf44b7eaf7 Mon Sep 17 00:00:00 2001 From: Christopher Speck Date: Mon, 12 Oct 2015 17:50:26 -0700 Subject: [PATCH] Conditionally use `hg files` vs. `hg locate` depending on version of Mercurial Summary: In Mercurial 3.2 the `locate` command was deprecated in favor of `files` command. This change updates the DiffusionLowLevelMercurialPathsQuery command to conditionally use `locate` or `files` based on the version of Mercurial used. Closes T7375 Test Plan: My test/develop Phabricator instance is setup to run Mercurial 3.5.1. The test procedure to verify valid file listings are being returned: 1. I navigated to `http://192.168.0.133/conduit/method/diffusion.querypaths/` 2. I populated the following fields: - path: `"/"` - commit: `"d721d5b57fc9ef72e47ff9d4e0c583d74a46590c"` - callsign: `"HGTEST"` 3. I submitted request and verified that result contained all files in the repository: ``` { "0": "README", "1": "alpha/beta/trifle", "2": "test/Chupacabra.cow", "3": "test/socket.ks" } ``` I repeated the above steps after setting up Mercurial 2.6.2, which I installed in the following manner: 1. I downloaded Mercurial 2.6.2 source and run `make local` which will only compile it to work from its own directory (`/opt/mercurial-2.6.2`) 2. I linked `/usr/local/bin/hg -> /opt/mercurial-2.6.2/hg` (there's also a `/usr/bin/hg` which is a link to `/usr/local/bin/hg`) 3. I navigated to my home directory and verify that `hg --version` returns 2.6.2. 4. I restarted phabricator services (probably unnecessary). With the Multimeter application active 1. I verified that `/usr/local/bin/hg` referred to version 2.6 2. I ran the same conduit call from the conduit application 3. I verified that `http://192.168.0.133/multimeter/?type=2&group=label` incremented values for `bin.hg locate`. 4. I swapped out mercurial versions for 3.5.1 5. I ran the same conduit call from the conduit application 6. I verified that `http://192.168.0.133/multimeter/?type=2&group=label` incremented values for `bin.hg files` Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T7375 Differential Revision: https://secure.phabricator.com/D14253 --- src/__phutil_library_map__.php | 2 ++ .../DiffusionLowLevelMercurialPathsQuery.php | 9 +++++- ...fusionLowLevelMercurialPathsQueryTests.php | 31 +++++++++++++++++++ .../multimeter/data/MultimeterControl.php | 1 + .../PhabricatorRepositoryVersion.php | 18 +++++++++++ 5 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ddf92ee46c..bc8b6a17d0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -607,6 +607,7 @@ phutil_register_library_map(array( 'DiffusionLowLevelGitRefQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelGitRefQuery.php', 'DiffusionLowLevelMercurialBranchesQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialBranchesQuery.php', 'DiffusionLowLevelMercurialPathsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php', + 'DiffusionLowLevelMercurialPathsQueryTests' => 'applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php', 'DiffusionLowLevelParentsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php', 'DiffusionLowLevelQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelQuery.php', 'DiffusionLowLevelResolveRefsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php', @@ -4342,6 +4343,7 @@ phutil_register_library_map(array( 'DiffusionLowLevelGitRefQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelMercurialBranchesQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelMercurialPathsQuery' => 'DiffusionLowLevelQuery', + 'DiffusionLowLevelMercurialPathsQueryTests' => 'PhabricatorTestCase', 'DiffusionLowLevelParentsQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelQuery' => 'Phobject', 'DiffusionLowLevelResolveRefsQuery' => 'DiffusionLowLevelQuery', diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php index aca82df79f..12b9e661d7 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php @@ -24,10 +24,17 @@ final class DiffusionLowLevelMercurialPathsQuery $path = $this->path; $commit = $this->commit; + $hg_paths_command = 'locate --print0 --rev %s -I %s'; + $hg_version = PhabricatorRepositoryVersion::getMercurialVersion(); + if (PhabricatorRepositoryVersion::isMercurialFilesCommandAvailable( + $hg_version)) { + $hg_paths_command = 'files --print0 --rev %s -I %s'; + } + $match_against = trim($path, '/'); $prefix = trim('./'.$match_against, '/'); list($entire_manifest) = $repository->execxLocalCommand( - 'locate --print0 --rev %s -I %s', + $hg_paths_command, hgsprintf('%s', $commit), $prefix); return explode("\0", $entire_manifest); diff --git a/src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php b/src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php new file mode 100644 index 0000000000..075ca2f1a5 --- /dev/null +++ b/src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php @@ -0,0 +1,31 @@ + pht('Versions which should not use `files`'), + 'versions' => array('2.6.2', '2.9', '3.1'), + 'match' => false, + ), + + array( + 'name' => pht('Versions which should use `files`'), + 'versions' => array('3.2', '3.3', '3.5.2'), + 'match' => true, + ), + ); + + foreach ($cases as $case) { + foreach ($case['versions'] as $version) { + $actual = PhabricatorRepositoryVersion + ::isMercurialFilesCommandAvailable($version); + $expect = $case['match']; + $this->assertEqual($expect, $actual, $case['name']); + } + } + } + +} diff --git a/src/applications/multimeter/data/MultimeterControl.php b/src/applications/multimeter/data/MultimeterControl.php index 1b01ce2e35..6658319469 100644 --- a/src/applications/multimeter/data/MultimeterControl.php +++ b/src/applications/multimeter/data/MultimeterControl.php @@ -265,6 +265,7 @@ final class MultimeterControl extends Phobject { 'init' => true, 'diff' => true, 'cat' => true, + 'files' => true, ), 'svnadmin' => array( 'create' => true, diff --git a/src/applications/repository/constants/PhabricatorRepositoryVersion.php b/src/applications/repository/constants/PhabricatorRepositoryVersion.php index a68e07d56a..5f722fa40a 100644 --- a/src/applications/repository/constants/PhabricatorRepositoryVersion.php +++ b/src/applications/repository/constants/PhabricatorRepositoryVersion.php @@ -19,4 +19,22 @@ final class PhabricatorRepositoryVersion extends Phobject { return null; } + /** + * The `locate` command is deprecated as of Mercurial 3.2, to be + * replaced with `files` command, which supports most of the same + * arguments. This determines whether the new `files` command should + * be used instead of the `locate` command. + * + * @param string $mercurial_version - The current version of mercurial + * which can be retrieved by calling: + * PhabricatorRepositoryVersion::getMercurialVersion() + * + * @return boolean True if the version of Mercurial is new enough to support + * the `files` command, or false if otherwise. + */ + public static function isMercurialFilesCommandAvailable($mercurial_version) { + $min_version_for_files = '3.2'; + return version_compare($mercurial_version, $min_version_for_files, '>='); + } + }