diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1651,6 +1651,7 @@ protected function buildCurtain(DiffusionRequest $drequest) { $viewer = $this->getViewer(); + $repository = $drequest->getRepository(); $curtain = $this->newCurtainView($drequest); @@ -1666,6 +1667,21 @@ ->setIcon('fa-list')); $behind_head = $drequest->getSymbolicCommit(); + + if ($repository->supportsBranchComparison()) { + $compare_uri = $drequest->generateURI( + array( + 'action' => 'compare', + )); + + $curtain->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Compare Against...')) + ->setIcon('fa-code-fork') + ->setWorkflow(true) + ->setHref($compare_uri)); + } + $head_uri = $drequest->generateURI( array( 'commit' => '', diff --git a/src/applications/diffusion/controller/DiffusionCompareController.php b/src/applications/diffusion/controller/DiffusionCompareController.php --- a/src/applications/diffusion/controller/DiffusionCompareController.php +++ b/src/applications/diffusion/controller/DiffusionCompareController.php @@ -16,104 +16,87 @@ $drequest = $this->getDiffusionRequest(); $repository = $drequest->getRepository(); - $content = array(); - - $crumbs = $this->buildCrumbs(array('view' => 'compare')); - - $empty_title = null; - $error_message = null; - - if ($repository->getVersionControlSystem() != - PhabricatorRepositoryType::REPOSITORY_TYPE_GIT) { - $empty_title = pht('Not supported'); - $error_message = pht( - 'This feature is not yet supported for %s repositories.', - $repository->getVersionControlSystem()); - $content[] = id(new PHUIInfoView()) - ->setTitle($empty_title) - ->setSeverity(PHUIInfoView::SEVERITY_WARNING) - ->setErrors(array($error_message)); + if (!$repository->supportsBranchComparison()) { + return $this->newDialog() + ->setTitle(pht('Not Supported')) + ->appendParagraph( + pht( + 'Branch comparison is not supported for this version control '. + 'system.')) + ->addCancelButton($this->getApplicationURI(), pht('Okay')); } $head_ref = $request->getStr('head'); $against_ref = $request->getStr('against'); - $refs = id(new DiffusionCachedResolveRefsQuery()) - ->setRepository($repository) - ->withRefs(array($head_ref, $against_ref)) - ->execute(); + $must_prompt = false; + if (!$request->isFormPost()) { + if (!strlen($head_ref)) { + $head_ref = $drequest->getSymbolicCommit(); + if (!strlen($head_ref)) { + $head_ref = $drequest->getBranch(); + } + } + if (!strlen($against_ref)) { + $default_branch = $repository->getDefaultBranch(); + if ($default_branch != $head_ref) { + $against_ref = $default_branch; - if (count($refs) == 2) { - $content[] = $this->buildCompareContent($drequest); - } else { - $content[] = $this->buildCompareForm($request, $refs); + // If we filled this in by default, we want to prompt the user to + // confirm that this is really what they want. + $must_prompt = true; + } + } } - - return $this->newPage() - ->setTitle( + $refs = $drequest->resolveRefs( + array_filter( array( - $repository->getName(), - $repository->getDisplayName(), - )) - ->setCrumbs($crumbs) - ->appendChild($content); - } - - private function buildCompareForm(AphrontRequest $request, array $resolved) { - $viewer = $this->getViewer(); - - $head_ref = $request->getStr('head'); - $against_ref = $request->getStr('against'); + $head_ref, + $against_ref, + ))); - if (idx($resolved, $head_ref)) { - $e_head = null; + $identical = false; + if ($head_ref === $against_ref) { + $identical = true; } else { - $e_head = 'Not Found'; + if (count($refs) == 2) { + if ($refs[$head_ref] === $refs[$against_ref]) { + $identical = true; + } + } } - if (idx($resolved, $against_ref)) { - $e_against = null; - } else { - $e_against = 'Not Found'; + if ($must_prompt || count($refs) != 2 || $identical) { + return $this->buildCompareDialog( + $head_ref, + $against_ref, + $refs, + $identical); } + if ($request->isFormPost()) { + // Redirect to a stable URI that can be copy/pasted. + $compare_uri = $drequest->generateURI( + array( + 'action' => 'compare', + 'head' => $head_ref, + 'against' => $against_ref, + )); - $form = id(new AphrontFormView()) - ->setUser($viewer) - ->setMethod('GET') - ->appendControl( - id(new AphrontFormTextControl()) - ->setLabel(pht('Head')) - ->setName('head') - ->setError($e_head) - ->setValue($head_ref)) - ->appendControl( - id(new AphrontFormTextControl()) - ->setLabel(pht('Against')) - ->setName('against') - ->setError($e_against) - ->setValue($against_ref)) - ->appendControl( - id(new AphrontFormSubmitControl()) - ->setValue('Compare')); - - return $form; - } - - private function buildCompareContent(DiffusionRequest $drequest) { - $request = $this->getRequest(); - $repository = $drequest->getRepository(); - - $head_ref = $request->getStr('head'); - $against_ref = $request->getStr('against'); + return id(new AphrontRedirectResponse())->setURI($compare_uri); + } - $content = array(); + $crumbs = $this->buildCrumbs( + array( + 'view' => 'compare', + )); $pager = id(new PHUIPagerView()) ->readFromRequest($request); + $history = null; try { $history_results = $this->callConduitWithDiffusionRequest( 'diffusion.historyquery', @@ -126,111 +109,189 @@ )); $history = DiffusionPathChange::newFromConduit( $history_results['pathChanges']); - $history = $pager->sliceResults($history); - $history_exception = null; + $history_view = $this->newHistoryView( + $history_results, + $history, + $pager, + $head_ref, + $against_ref); + } catch (Exception $ex) { - $history_results = null; - $history = null; - $history_exception = $ex; + if ($repository->isImporting()) { + $history_view = $this->renderStatusMessage( + pht('Still Importing...'), + pht( + 'This repository is still importing. History is not yet '. + 'available.')); + } else { + $history_view = $this->renderStatusMessage( + pht('Unable to Retrieve History'), + $ex->getMessage()); + } } - if ($history_results) { - $content[] = $this->buildCompareProperties($drequest, $history_results); - } - $content[] = $this->buildCompareForm( - $request, - array($head_ref => true, $against_ref => true)); + $header = id(new PHUIHeaderView()) + ->setHeader( + pht( + 'Changes on %s but not %s', + phutil_tag('em', array(), $head_ref), + phutil_tag('em', array(), $against_ref))); - $content[] = $this->buildHistoryTable( - $history_results, - $history, - $pager, - $history_exception); + $curtain = $this->buildCurtain($head_ref, $against_ref); - $content[] = $this->renderTablePagerBox($pager); + $column_view = id(new PHUITwoColumnView()) + ->setHeader($header) + ->setCurtain($curtain) + ->setMainColumn( + array( + $history_view, + )); - return $content; + return $this->newPage() + ->setTitle( + array( + $repository->getName(), + $repository->getDisplayName(), + )) + ->setCrumbs($crumbs) + ->appendChild($column_view); } - private function buildCompareProperties($drequest, array $history_results) { - $viewer = $this->getViewer(); + private function buildCompareDialog( + $head_ref, + $against_ref, + array $resolved, + $identical) { + $viewer = $this->getViewer(); $request = $this->getRequest(); + $drequest = $this->getDiffusionRequest(); $repository = $drequest->getRepository(); - $head_ref = $request->getStr('head'); - $against_ref = $request->getStr('against'); + $e_head = null; + $e_against = null; + $errors = array(); + if ($request->isFormPost()) { + if (!strlen($head_ref)) { + $e_head = pht('Required'); + $errors[] = pht( + 'You must provide two different commits to compare.'); + } else if (!isset($resolved[$head_ref])) { + $e_head = pht('Not Found'); + $errors[] = pht( + 'Commit "%s" is not a valid commit in this repository.', + $head_ref); + } + + if (!strlen($against_ref)) { + $e_against = pht('Required'); + $errors[] = pht( + 'You must provide two different commits to compare.'); + } else if (!isset($resolved[$against_ref])) { + $e_against = pht('Not Found'); + $errors[] = pht( + 'Commit "%s" is not a valid commit in this repository.', + $against_ref); + } - $reverse_uri = $repository->getPathURI( - "compare/?head=${against_ref}&against=${head_ref}"); - $actions = id(new PhabricatorActionListView()); - $actions->setUser($viewer); + if ($identical) { + $e_head = pht('Identical'); + $e_against = pht('Identical'); + $errors[] = pht( + 'Both references identify the same commit. You can not compare a '. + 'commit against itself.'); + } + } + $form = id(new AphrontFormView()) + ->setViewer($viewer) + ->appendControl( + id(new AphrontFormTextControl()) + ->setLabel(pht('Head')) + ->setName('head') + ->setError($e_head) + ->setValue($head_ref)) + ->appendControl( + id(new AphrontFormTextControl()) + ->setLabel(pht('Against')) + ->setName('against') + ->setError($e_against) + ->setValue($against_ref)); + + $cancel_uri = $repository->generateURI( + array( + 'action' => 'browse', + )); + + return $this->newDialog() + ->setTitle(pht('Compare Against')) + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->setErrors($errors) + ->appendForm($form) + ->addSubmitButton(pht('Compare')) + ->addCancelButton($cancel_uri, pht('Cancel')); + } - $actions->addAction(id(new PhabricatorActionView()) + private function buildCurtain($head_ref, $against_ref) { + $viewer = $this->getViewer(); + $request = $this->getRequest(); + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + + $curtain = $this->newCurtainView(null); + + $reverse_uri = $drequest->generateURI( + array( + 'action' => 'compare', + 'head' => $against_ref, + 'against' => $head_ref, + )); + + $curtain->addAction( + id(new PhabricatorActionView()) ->setName(pht('Reverse Comparison')) ->setHref($reverse_uri) - ->setIcon('fa-list')); + ->setIcon('fa-refresh')); - $view = id(new PHUIPropertyListView()) - ->setUser($viewer) - ->setActionList($actions); + $compare_uri = $drequest->generateURI( + array( + 'action' => 'compare', + 'head' => $head_ref, + )); - $readme = - 'These are the commits that are reachable from **Head** commit and not '. - 'from the **Against** commit.'; - $readme = new PHUIRemarkupView($viewer, $readme); - $view->addTextContent($readme); + $curtain->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Compare Against...')) + ->setIcon('fa-code-fork') + ->setWorkflow(true) + ->setHref($compare_uri)); - $view->addProperty(pht('Head'), $head_ref); - $view->addProperty(pht('Against'), $against_ref); + // TODO: Provide a "Show Diff" action. - $header = id(new PHUIHeaderView()) - ->setUser($viewer) - ->setPolicyObject($drequest->getRepository()); - - $box = id(new PHUIObjectBoxView()) - ->setUser($viewer) - ->setHeader($header) - ->addPropertyList($view); - return $box; + return $curtain; } - private function buildHistoryTable( - $history_results, - $history, - $pager, - $history_exception) { + private function newHistoryView( + array $results, + array $history, + PHUIPagerView $pager, + $head_ref, + $against_ref) { $request = $this->getRequest(); - $viewer = $request->getUser(); + $viewer = $this->getViewer(); $drequest = $this->getDiffusionRequest(); $repository = $drequest->getRepository(); - if ($history_exception) { - if ($repository->isImporting()) { - return $this->renderStatusMessage( - pht('Still Importing...'), - pht( - 'This repository is still importing. History is not yet '. - 'available.')); - } else { - return $this->renderStatusMessage( - pht('Unable to Retrieve History'), - $history_exception->getMessage()); - } - } - if (!$history) { return $this->renderStatusMessage( - pht('Up to date'), - new PHUIRemarkupView( - $viewer, - pht( - '**Against** is strictly ahead of **Head** '. - '- no commits are missing.'))); + pht('Up To Date'), + pht( + 'There are no commits on %s that are not already on %s.', + phutil_tag('strong', array(), $head_ref), + phutil_tag('strong', array(), $against_ref))); } $history_table = id(new DiffusionHistoryTableView()) @@ -238,23 +299,27 @@ ->setDiffusionRequest($drequest) ->setHistory($history); - // TODO: Super sketchy? $history_table->loadRevisions(); - if ($history_results) { - $history_table->setParents($history_results['parents']); + if ($history) { + $history_table->setParents($results['parents']); $history_table->setIsHead(!$pager->getOffset()); $history_table->setIsTail(!$pager->getHasMorePages()); } - // TODO also expose diff. - - $panel = new PHUIObjectBoxView(); $header = id(new PHUIHeaderView()) - ->setHeader(pht('Missing Commits')); - $panel->setHeader($header); - $panel->setTable($history_table); + ->setHeader(pht('Commits')); + + $object_box = id(new PHUIObjectBoxView()) + ->setHeader($header) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setTable($history_table); + + $pager_box = $this->renderTablePagerBox($pager); - return $panel; + return array( + $object_box, + $pager_box, + ); } } diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -644,7 +644,7 @@ return $match; } - private function resolveRefs(array $refs, array $types) { + public function resolveRefs(array $refs, array $types = array()) { // First, try to resolve refs from fast cache sources. $cached_query = id(new DiffusionCachedResolveRefsQuery()) ->setRepository($this->getRepository()) diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -689,6 +689,7 @@ case 'lint': case 'pathtree': case 'refs': + case 'compare': break; case 'branch': // NOTE: This does not actually require a branch, and won't have one @@ -710,6 +711,9 @@ $commit = idx($params, 'commit'); $line = idx($params, 'line'); + $head = idx($params, 'head'); + $against = idx($params, 'against'); + if ($req_commit && !strlen($commit)) { throw new Exception( pht( @@ -734,11 +738,13 @@ $path = phutil_escape_uri($path); } + $raw_branch = $branch; if (strlen($branch)) { $branch = phutil_escape_uri_path_component($branch); $path = "{$branch}/{$path}"; } + $raw_commit = $commit; if (strlen($commit)) { $commit = str_replace('$', '$$', $commit); $commit = ';'.phutil_escape_uri($commit); @@ -748,6 +754,7 @@ $line = '$'.phutil_escape_uri($line); } + $query = array(); switch ($action) { case 'change': case 'history': @@ -760,6 +767,20 @@ case 'refs': $uri = $this->getPathURI("/{$action}/{$path}{$commit}{$line}"); break; + case 'compare': + $uri = $this->getPathURI("/{$action}/"); + if (strlen($head)) { + $query['head'] = $head; + } else if (strlen($raw_commit)) { + $query['commit'] = $raw_commit; + } else if (strlen($raw_branch)) { + $query['head'] = $raw_branch; + } + + if (strlen($against)) { + $query['against'] = $against; + } + break; case 'branch': if (strlen($path)) { $uri = $this->getPathURI("/repository/{$path}"); @@ -791,8 +812,10 @@ ); } - if (idx($params, 'params')) { - $uri->setQueryParams($params['params']); + $query = idx($params, 'params', array()) + $query; + + if ($query) { + $uri->setQueryParams($query); } return $uri; @@ -2066,6 +2089,10 @@ return $result; } + public function supportsBranchComparison() { + return $this->isGit(); + } + /* -( Repository URIs )---------------------------------------------------- */