From 35ccda922a3101957d25beb1c2e66aba79324f84 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 17 Jan 2014 16:11:04 -0800 Subject: [PATCH] Merge `diffusion.commitbranchesquery` into `diffusion.branchquery` Summary: Ref T4327. This is general cleanup since I was in this area of the code. Primarily, the Mercurial implementation here was completely broken and wrong: - It returned only one branch, but a commit can be present on many branches. - It did not account for multiple branch heads. - It returned a result implying the branch head pointed at the queried commit, which is no consistent or accurate. Simplify the amount of API we're dealing with by collapsing this method into the very similar `diffusion.branchquery` method. Test Plan: Looked at mercurial and git repositories and commits, branch information seemed correct. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4327 Differential Revision: https://secure.phabricator.com/D8003 --- src/__phutil_library_map__.php | 2 - ...onduitAPI_diffusion_branchquery_Method.php | 77 ++++++++++++++++--- ...I_diffusion_commitbranchesquery_Method.php | 66 ---------------- .../DiffusionCommitBranchesController.php | 14 ++-- 4 files changed, 75 insertions(+), 84 deletions(-) delete mode 100644 src/applications/diffusion/conduit/ConduitAPI_diffusion_commitbranchesquery_Method.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7c21701ba5..202bb4a14e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -155,7 +155,6 @@ phutil_register_library_map(array( 'ConduitAPI_diffusion_abstractquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_abstractquery_Method.php', 'ConduitAPI_diffusion_branchquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_branchquery_Method.php', 'ConduitAPI_diffusion_browsequery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_browsequery_Method.php', - 'ConduitAPI_diffusion_commitbranchesquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_commitbranchesquery_Method.php', 'ConduitAPI_diffusion_commitparentsquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_commitparentsquery_Method.php', 'ConduitAPI_diffusion_createcomment_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_createcomment_Method.php', 'ConduitAPI_diffusion_diffquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php', @@ -2615,7 +2614,6 @@ phutil_register_library_map(array( 'ConduitAPI_diffusion_abstractquery_Method' => 'ConduitAPI_diffusion_Method', 'ConduitAPI_diffusion_branchquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', 'ConduitAPI_diffusion_browsequery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', - 'ConduitAPI_diffusion_commitbranchesquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', 'ConduitAPI_diffusion_commitparentsquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', 'ConduitAPI_diffusion_createcomment_Method' => 'ConduitAPI_diffusion_Method', 'ConduitAPI_diffusion_diffquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', diff --git a/src/applications/diffusion/conduit/ConduitAPI_diffusion_branchquery_Method.php b/src/applications/diffusion/conduit/ConduitAPI_diffusion_branchquery_Method.php index 4d4bd37390..98a1b7932e 100644 --- a/src/applications/diffusion/conduit/ConduitAPI_diffusion_branchquery_Method.php +++ b/src/applications/diffusion/conduit/ConduitAPI_diffusion_branchquery_Method.php @@ -1,35 +1,59 @@ '; } protected function defineCustomParamTypes() { return array( 'limit' => 'optional int', - 'offset' => 'optional int' + 'offset' => 'optional int', + 'contains' => 'optional string', ); } - protected function getGitResult(ConduitAPIRequest $request) { $drequest = $this->getDiffusionRequest(); $repository = $drequest->getRepository(); - $refs = id(new DiffusionLowLevelGitRefQuery()) - ->setRepository($repository) - ->withIsOriginBranch(true) - ->execute(); + $contains = $request->getValue('contains'); + if (strlen($contains)) { + // NOTE: We can't use DiffusionLowLevelGitRefQuery here because + // `git for-each-ref` does not support `--contains`. + if ($repository->isWorkingCopyBare()) { + list($stdout) = $repository->execxLocalCommand( + 'branch --verbose --no-abbrev --contains %s --', + $contains); + $ref_map = DiffusionGitBranch::parseLocalBranchOutput( + $stdout); + } else { + list($stdout) = $repository->execxLocalCommand( + 'branch -r --verbose --no-abbrev --contains %s --', + $contains); + $ref_map = DiffusionGitBranch::parseRemoteBranchOutput( + $stdout, + DiffusionGitBranch::DEFAULT_GIT_REMOTE); + } + + $refs = array(); + foreach ($ref_map as $ref => $commit) { + $refs[] = id(new DiffusionRepositoryRef()) + ->setShortName($ref) + ->setCommitIdentifier($commit); + } + } else { + $refs = id(new DiffusionLowLevelGitRefQuery()) + ->setRepository($repository) + ->withIsOriginBranch(true) + ->execute(); + } return $this->processBranchRefs($request, $refs); } @@ -42,6 +66,37 @@ final class ConduitAPI_diffusion_branchquery_Method ->setRepository($repository) ->execute(); + // If we have a 'contains' query, filter these branches down to just the + // ones which contain the commit. + $contains = $request->getValue('contains'); + if (strlen($contains)) { + list($branches_raw) = $repository->execxLocalCommand( + 'log --template %s --limit 1 --rev %s --', + '{branches}', + hgsprintf('%s', $contains)); + + $branches_raw = trim($branches_raw); + if (!strlen($branches_raw)) { + $containing_branches = array('default'); + } else { + $containing_branches = explode(' ', $branches_raw); + } + + $containing_branches = array_fuse($containing_branches); + + // NOTE: We get this very slightly wrong: a branch may have multiple + // heads and we'll keep all of the heads of the branch, even if the + // commit is only on some of the heads. This should be rare, is probably + // more clear to users as is, and would potentially be expensive to get + // right since we'd have to do additional checks. + + foreach ($refs as $key => $ref) { + if (empty($containing_branches[$ref->getShortName()])) { + unset($refs[$key]); + } + } + } + return $this->processBranchRefs($request, $refs); } diff --git a/src/applications/diffusion/conduit/ConduitAPI_diffusion_commitbranchesquery_Method.php b/src/applications/diffusion/conduit/ConduitAPI_diffusion_commitbranchesquery_Method.php deleted file mode 100644 index 4ff454f0f5..0000000000 --- a/src/applications/diffusion/conduit/ConduitAPI_diffusion_commitbranchesquery_Method.php +++ /dev/null @@ -1,66 +0,0 @@ - 'required string', - ); - } - - protected function getGitResult(ConduitAPIRequest $request) { - $drequest = $this->getDiffusionRequest(); - $repository = $drequest->getRepository(); - $commit = $request->getValue('commit'); - - // NOTE: We can't use DiffusionLowLevelGitRefQuery here because - // `git for-each-ref` does not support `--contains`. - if ($repository->isWorkingCopyBare()) { - list($contains) = $repository->execxLocalCommand( - 'branch --verbose --no-abbrev --contains %s', - $commit); - return DiffusionGitBranch::parseLocalBranchOutput( - $contains); - } else { - list($contains) = $repository->execxLocalCommand( - 'branch -r --verbose --no-abbrev --contains %s', - $commit); - return DiffusionGitBranch::parseRemoteBranchOutput( - $contains, - DiffusionGitBranch::DEFAULT_GIT_REMOTE); - } - } - - protected function getMercurialResult(ConduitAPIRequest $request) { - $drequest = $this->getDiffusionRequest(); - $repository = $drequest->getRepository(); - $commit = $request->getValue('commit'); - - // TODO: This should use `branches`. Also, this entire method's API needs - // to be fixed to support multiple branch heads in Mercurial. We should - // probably return `DiffusionRepositoryRefs` and probably merge this into - // `diffusion.branchesquery`. - - list($contains) = $repository->execxLocalCommand( - 'log --template %s --limit 1 --rev %s --', - '{branch}', - $commit); - - return array( - trim($contains) => $commit, - ); - - } -} diff --git a/src/applications/diffusion/controller/DiffusionCommitBranchesController.php b/src/applications/diffusion/controller/DiffusionCommitBranchesController.php index 069a353a8f..df60c0563d 100644 --- a/src/applications/diffusion/controller/DiffusionCommitBranchesController.php +++ b/src/applications/diffusion/controller/DiffusionCommitBranchesController.php @@ -17,26 +17,30 @@ final class DiffusionCommitBranchesController extends DiffusionController { $branches = array(); try { $branches = $this->callConduitWithDiffusionRequest( - 'diffusion.commitbranchesquery', - array('commit' => $request->getCommit())); + 'diffusion.branchquery', + array( + 'contains' => $request->getCommit(), + )); } catch (ConduitException $ex) { if ($ex->getMessage() != 'ERR-UNSUPPORTED-VCS') { throw $ex; } } + $branches = DiffusionRepositoryRef::loadAllFromDictionaries($branches); + $branch_links = array(); - foreach ($branches as $branch => $commit) { + foreach ($branches as $branch) { $branch_links[] = phutil_tag( 'a', array( 'href' => $request->generateURI( array( 'action' => 'browse', - 'branch' => $branch, + 'branch' => $branch->getShortName(), )), ), - $branch); + $branch->getShortName()); } return id(new AphrontAjaxResponse())