From 69246b282dcc3ee2c0c9e1794405aa6e217a124c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Jun 2012 10:50:43 -0700 Subject: [PATCH] Simplify "arc branch" and make it work in immutable history repositories Summary: - Move "arc branch"-specific code to the branch workflow. - Instead of doing "git rev-parse", just do "git branch --verbose --abbrev=40". - Use revision owners to identify ownership, not working copy identity. Particularly with the advent of "Commandeer", you might not own commits you made. - Do a batch lookup for commits by hash (depends on D2859, but doesn't break without it). - Use PhutilConsole for console stuff. - Removed color from "arc list" for the moment. - The "--by-status" flag has a slightly different output format now. Test Plan: Ran "arc branch" in various circumstances, verified it identifies branches in immutable history repositories. Reviewers: btrahan, vrana, jungejason, nh, slawekbiel Reviewed By: slawekbiel CC: aran Maniphest Tasks: T693 Differential Revision: https://secure.phabricator.com/D2860 --- src/__phutil_library_map__.php | 1 - src/branch/BranchInfo.php | 180 ---------------- src/repository/api/ArcanistGitAPI.php | 57 ++---- src/workflow/ArcanistBranchWorkflow.php | 262 +++++++++++++++--------- src/workflow/ArcanistListWorkflow.php | 8 +- 5 files changed, 187 insertions(+), 321 deletions(-) delete mode 100644 src/branch/BranchInfo.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1250ef8d..7c63e511 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -127,7 +127,6 @@ phutil_register_library_map(array( 'ArcanistXHPASTLintNamingHookTestCase' => 'lint/linter/xhpast/__tests__/ArcanistXHPASTLintNamingHookTestCase.php', 'ArcanistXHPASTLinter' => 'lint/linter/ArcanistXHPASTLinter.php', 'ArcanistXHPASTLinterTestCase' => 'lint/linter/__tests__/ArcanistXHPASTLinterTestCase.php', - 'BranchInfo' => 'branch/BranchInfo.php', 'ComprehensiveLintEngine' => 'lint/engine/ComprehensiveLintEngine.php', 'ExampleLintEngine' => 'lint/engine/ExampleLintEngine.php', 'NoseTestEngine' => 'unit/engine/NoseTestEngine.php', diff --git a/src/branch/BranchInfo.php b/src/branch/BranchInfo.php deleted file mode 100644 index 41bc7e7a..00000000 --- a/src/branch/BranchInfo.php +++ /dev/null @@ -1,180 +0,0 @@ -getAllBranches(); - $branches = array(); - foreach ($branches_raw as $branch_raw) { - $branch_info = new BranchInfo($branch_raw['name']); - $branch_info->setSha1($branch_raw['sha1']); - if ($branch_raw['current']) { - $branch_info->setCurrent(); - } - $branches[] = $branch_info; - } - - $name_sha1_map = mpull($branches, 'getSha1', 'getName'); - $commits_list = $api->multigetCommitMessages( - array_unique(array_values($name_sha1_map)), - "%ct%n%an%n%s%n%b"); - foreach ($branches as $branch) { - $sha1 = $name_sha1_map[$branch->getName()]; - $branch->setSha1($sha1); - $branch->parseCommitMessage($commits_list[$sha1]); - } - $branches = msort($branches, 'getCommitTime'); - return $branches; - } - - public function __construct($branch_name) { - $this->branchName = $branch_name; - } - - public function setSha1($sha1) { - $this->sha1 = $sha1; - return $this; - } - - public function getSha1() { - return $this->sha1; - } - - public function setCurrent() { - $this->currentHead = true; - return $this; - } - - public function isCurrentHead() { - return $this->currentHead; - } - - - public function setStatus($status) { - $this->status = $status; - return $this; - } - - public function getStatus() { - return $this->status; - } - - public function getRevisionID() { - return $this->revisionID; - } - - public function getCommitTime() { - return $this->commitTime; - } - - public function getCommitSubject() { - return $this->commitSubject; - } - - public function getCommitDisplayName() { - if ($this->revisionID) { - return 'D'.$this->revisionID.': '.$this->commitSubject; - } else { - return $this->commitSubject; - } - } - - public function getCommitAuthor() { - return $this->commitAuthor; - } - - public function getName() { - return $this->branchName; - } - - /** - * Based on the 'git show' output extracts the commit date, author, - * subject nad Differential revision . - * 'Differential Revision:' - * - * @param string message output of git show -s --format="format:%ct%n%cn%n%b" - */ - public function parseCommitMessage($message) { - $message_lines = explode("\n", trim($message)); - $this->commitTime = $message_lines[0]; - $this->commitAuthor = $message_lines[1]; - $this->commitSubject = trim($message_lines[2]); - $this->revisionID = - ArcanistDifferentialCommitMessage::newFromRawCorpus($message) - ->getRevisionID(); - } - - public function getFormattedName() { - $res = ""; - if ($this->currentHead) { - $res = '* '; - } - $res .= $this->branchName; - return phutil_console_format('**%s**', $res); - - } - - /** - * Generates a colored status name - */ - public function getFormattedStatus() { - return self::renderColorizedRevisionStatus($this->status); - } - - /** - * Assigns a pretty color based on the status - */ - private static function getColorForStatus($status) { - static $status_to_color = array( - 'Closed' => 'cyan', - 'Needs Review' => 'magenta', - 'Needs Revision' => 'red', - 'Accepted' => 'green', - 'No Revision' => 'blue', - 'Abandoned' => 'default', - ); - return idx($status_to_color, $status, 'default'); - } - - public static function renderColorizedRevisionStatus($status) { - return phutil_console_format( - '%s', - $status); - } - -} diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index b0e1b570..c71a0253 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -637,63 +637,34 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { /** * Returns names of all the branches in the current repository. * - * @return array where each element is a triple ('name', 'sha1', 'current') + * @return list> Dictionary of branch information. */ public function getAllBranches() { - list($branch_info) = $this->execxLocal('branch --no-color'); - $lines = explode("\n", trim($branch_info)); + list($branch_info) = $this->execxLocal( + 'branch --verbose --abbrev=40 --no-color'); + $lines = explode("\n", rtrim($branch_info)); + $result = array(); foreach ($lines as $line) { - $match = array(); - preg_match('/^(\*?)\s*(.*)$/', $line, $match); - $name = $match[2]; - if ($name == '(no branch)') { - // Just ignore this, we could theoretically try to figure out the ref - // and treat it like a real branch but that's sort of ridiculous. + + if (preg_match('/^[* ]+\(no branch\)/', $line)) { + // This is indicating that the working copy is in a detached state; + // just ignore it. continue; } + + list($current, $name, $hash, $desc) = preg_split('/\s+/', $line, 4); $result[] = array( - 'current' => !empty($match[1]), + 'current' => !empty($current), 'name' => $name, + 'hash' => $hash, + 'desc' => $desc, ); } - $all_names = ipull($result, 'name'); - // Calling 'git branch' first and then 'git rev-parse' is way faster than - // 'git branch -v' for some reason. - list($sha1s_string) = $this->execxLocal('rev-parse %Ls', $all_names); - $sha1_map = array_combine($all_names, explode("\n", trim($sha1s_string))); - foreach ($result as &$branch) { - $branch['sha1'] = $sha1_map[$branch['name']]; - } return $result; } - /** - * Returns git commit messages for the given revisions, - * in the specified format (see git show --help for options). - * - * @param array $revs a list of commit hashes - * @param string $format the format to show messages in - */ - public function multigetCommitMessages($revs, $format) { - - list($commits_string) = $this->execxLocal( - "show -s --pretty='format:'%s%s %Ls", - $format, - '%x00', - $revs); - - $commits_list = array_slice(explode("\0", $commits_string), 0, -1); - $commits_list = array_combine($revs, $commits_list); - return $commits_list; - } - - public function getRepositoryOwner() { - list($owner) = $this->execxLocal('config --get user.name'); - return trim($owner); - } - public function getWorkingCopyRevision() { list($stdout) = $this->execxLocal('rev-parse HEAD'); return rtrim($stdout, "\n"); diff --git a/src/workflow/ArcanistBranchWorkflow.php b/src/workflow/ArcanistBranchWorkflow.php index 88566e8a..030915ed 100644 --- a/src/workflow/ArcanistBranchWorkflow.php +++ b/src/workflow/ArcanistBranchWorkflow.php @@ -27,7 +27,7 @@ final class ArcanistBranchWorkflow extends ArcanistBaseWorkflow { public function getCommandSynopses() { return phutil_console_format(<< array( - 'help' => - "Include closed and abandoned revisions", + 'help' => 'Include closed and abandoned revisions', ), 'by-status' => array( - 'help' => 'Group output by revision status.', + 'help' => 'Sort branches by status instead of time.', ), ); } @@ -73,103 +75,179 @@ EOTEXT $repository_api = $this->getRepositoryAPI(); if (!($repository_api instanceof ArcanistGitAPI)) { throw new ArcanistUsageException( - "arc branch is only supported under git." + 'arc branch is only supported under git.'); + } + + $branches = $repository_api->getAllBranches(); + if (!$branches) { + throw new ArcanistUsageException('No branches in this working copy.'); + } + + $commit_map = $this->loadCommitInfo($branches, $repository_api); + foreach ($branches as $key => $branch) { + $branches[$key] += $commit_map[$branch['hash']]; + } + + $revisions = $this->loadRevisions($branches); + + $this->printBranches($branches, $revisions); + + return 0; + } + + private function loadCommitInfo( + array $branches, + ArcanistRepositoryAPI $repository_api) { + + $commits = ipull($branches, 'hash'); + list($info) = $repository_api->execxLocal( + 'log --format=%C %Ls --', + '%H%x01%ct%x01%T%x01%s%n%b%x02', + $commits); + + $commit_map = array(); + + $info = array_filter(explode("\2", trim($info))); + foreach ($info as $line) { + list($hash, $epoch, $tree, $text) = explode("\1", trim($line), 4); + $message = ArcanistDifferentialCommitMessage::newFromRawCorpus($text); + $id = $message->getRevisionID(); + + $commit_map[$hash] = array( + 'epoch' => (int)$epoch, + 'tree' => $tree, + 'revisionID' => $id, ); } - $this->branches = BranchInfo::loadAll($repository_api); - $all_revisions = array_unique( - array_filter(mpull($this->branches, 'getRevisionId'))); - $revision_status = $this->loadDifferentialStatuses($all_revisions); - $owner = $repository_api->getRepositoryOwner(); - foreach ($this->branches as $branch) { - if ($branch->getCommitAuthor() != $owner) { - $branch->setStatus('Not Yours'); - continue; + return $commit_map; + } + + private function loadRevisions(array $branches) { + $ids = array(); + $hashes = array(); + + foreach ($branches as $branch) { + if ($branch['revisionID']) { + $ids[] = $branch['revisionID']; + } + $hashes[] = array('gtcm', $branch['hash']); + $hashes[] = array('gttr', $branch['tree']); + } + + $calls = array(); + + if ($ids) { + $calls[] = $this->getConduit()->callMethod( + 'differential.query', + array( + 'ids' => $ids, + )); + } + + if ($hashes) { + $calls[] = $this->getConduit()->callMethod( + 'differential.query', + array( + 'commitHashes' => $hashes, + )); + } + + $results = array(); + foreach (Futures($calls) as $call) { + $results[] = $call->resolve(); + } + + return array_mergev($results); + } + + private function printBranches(array $branches, array $revisions) { + $revisions = ipull($revisions, null, 'id'); + + static $color_map = array( + 'Closed' => 'cyan', + 'Needs Review' => 'magenta', + 'Needs Revision' => 'red', + 'Accepted' => 'green', + 'No Revision' => 'blue', + 'Abandoned' => 'default', + ); + + static $ssort_map = array( + 'Closed' => 1, + 'No Revision' => 2, + 'Needs Review' => 3, + 'Needs Revision' => 4, + 'Accepted' => 5, + ); + + $out = array(); + foreach ($branches as $branch) { + $revision = idx($revisions, idx($branch, 'revisionID')); + + // If we haven't identified a revision by ID, try to identify it by hash. + if (!$revision) { + foreach ($revisions as $rev) { + $hashes = idx($rev, 'hashes', array()); + foreach ($hashes as $hash) { + if (($hash[0] == 'gtcm' && $hash[1] == $branch['hash']) || + ($hash[0] == 'gttr' && $hash[1] == $branch['tree'])) { + $revision = $rev; + break; + } + } + } } - $rev_id = $branch->getRevisionID(); - if ($rev_id) { - $status = idx($revision_status, $rev_id, 'Unknown Status'); - $branch->setStatus($status); + if ($revision) { + $desc = 'D'.$revision['id'].': '.$revision['title']; + $status = $revision['statusName']; } else { - $branch->setStatus('No Revision'); + $desc = $branch['desc']; + $status = 'No Revision'; } - } - if (!$this->getArgument('view-all')) { - $this->filterOutFinished(); - } - $this->printInColumns(); - } - - - /** - * Makes a conduit call to differential to find out revision statuses - * based on their IDs - */ - private function loadDifferentialStatuses($rev_ids) { - $conduit = $this->getConduit(); - $revisions = $conduit->callMethodSynchronous( - 'differential.query', - array( - 'ids' => $rev_ids, - )); - $statuses = ipull($revisions, 'statusName', 'id'); - return $statuses; - } - - /** - * Removes the branches with status either closed or abandoned. - */ - private function filterOutFinished() { - foreach ($this->branches as $id => $branch) { - if ($branch->isCurrentHead() ) { - continue; //never filter the current branch - } - $status = $branch->getStatus(); - if ($status == 'Closed' || $status == 'Abandoned') { - unset($this->branches[$id]); - } - } - } - - public function printInColumns() { - $longest_name = 0; - $longest_status = 0; - foreach ($this->branches as $branch) { - $longest_name = max(strlen($branch->getFormattedName()), $longest_name); - $longest_status = max(strlen($branch->getStatus()), $longest_status); - } - - if ($this->getArgument('by-status')) { - $by_status = mgroup($this->branches, 'getStatus'); - foreach (array('Accepted', 'Needs Revision', - 'Needs Review', 'No Revision') as $status) { - $branches = idx($by_status, $status); - if (!$branches) { + if (!$this->getArgument('view-all')) { + if ($status == 'Closed' || $status == 'Abandoned') { continue; } - echo reset($branches)->getFormattedStatus()."\n"; - foreach ($branches as $branch) { - $name_markdown = $branch->getFormattedName(); - $subject = $branch->getCommitDisplayName(); - $name_markdown = str_pad($name_markdown, $longest_name + 4, ' '); - echo " $name_markdown $subject\n"; - } } + + $epoch = $branch['epoch']; + + $color = idx($color_map, $status, 'default'); + $ssort = sprintf('%d%012d', idx($ssort_map, $status, 0), $epoch); + + $out[] = array( + 'name' => $branch['name'], + 'current' => $branch['current'], + 'status' => $status, + 'desc' => $desc, + 'color' => $color, + 'esort' => $epoch, + 'ssort' => $ssort, + ); + } + + $len_name = max(array_map('strlen', ipull($out, 'name'))) + 2; + $len_status = max(array_map('strlen', ipull($out, 'status'))) + 2; + + if ($this->getArgument('by-status')) { + $out = isort($out, 'ssort'); } else { - foreach ($this->branches as $branch) { - $name_markdown = $branch->getFormattedName(); - $status_markdown = $branch->getFormattedStatus(); - $subject = $branch->getCommitDisplayName(); - $subject_pad = $longest_status - strlen($branch->getStatus()) + 4; - $name_markdown = - str_pad($name_markdown, $longest_name + 4, ' '); - $subject = - str_pad($subject, strlen($subject) + $subject_pad, ' ', STR_PAD_LEFT); - echo "$name_markdown $status_markdown $subject\n"; - } + $out = isort($out, 'esort'); + } + + $console = PhutilConsole::getConsole(); + foreach ($out as $line) { + $color = $line['color']; + $console->writeOut( + "%s **%s** %s %s\n", + $line['current'] ? '* ' : ' ', + str_pad($line['name'], $len_name), + str_pad($line['status'], $len_status), + $line['desc']); } } + } diff --git a/src/workflow/ArcanistListWorkflow.php b/src/workflow/ArcanistListWorkflow.php index d165156d..5c5d8bff 100644 --- a/src/workflow/ArcanistListWorkflow.php +++ b/src/workflow/ArcanistListWorkflow.php @@ -81,12 +81,10 @@ EOTEXT $info[$key]['here'], $revision['status'], $revision['id']); - $info[$key]['statusColorized'] = - BranchInfo::renderColorizedRevisionStatus( - $revision['statusName']); + $info[$key]['statusName'] = $revision['statusName']; $status_len = max( $status_len, - strlen($info[$key]['statusColorized'])); + strlen($info[$key]['statusName'])); } $info = isort($info, 'sort'); @@ -97,7 +95,7 @@ EOTEXT $spec['here'] ? phutil_console_format('**%s**', '*') : ' ', - $spec['statusColorized'], + $spec['statusName'], $revision['id'], $revision['title']); }