diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -343,6 +343,7 @@ 'DifferentialChangesetOneUpTestRenderer' => 'applications/differential/render/DifferentialChangesetOneUpTestRenderer.php', 'DifferentialChangesetParser' => 'applications/differential/parser/DifferentialChangesetParser.php', 'DifferentialChangesetParserTestCase' => 'applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php', + 'DifferentialChangesetQuery' => 'applications/differential/query/DifferentialChangesetQuery.php', 'DifferentialChangesetRenderer' => 'applications/differential/render/DifferentialChangesetRenderer.php', 'DifferentialChangesetTestRenderer' => 'applications/differential/render/DifferentialChangesetTestRenderer.php', 'DifferentialChangesetTwoUpRenderer' => 'applications/differential/render/DifferentialChangesetTwoUpRenderer.php', @@ -3018,6 +3019,7 @@ 'DifferentialChangesetOneUpRenderer' => 'DifferentialChangesetHTMLRenderer', 'DifferentialChangesetOneUpTestRenderer' => 'DifferentialChangesetTestRenderer', 'DifferentialChangesetParserTestCase' => 'PhabricatorTestCase', + 'DifferentialChangesetQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DifferentialChangesetTestRenderer' => 'DifferentialChangesetRenderer', 'DifferentialChangesetTwoUpRenderer' => 'DifferentialChangesetHTMLRenderer', 'DifferentialChangesetTwoUpTestRenderer' => 'DifferentialChangesetTestRenderer', diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -23,25 +23,33 @@ $id = (int)$id; $vs = (int)$vs; - $changeset = id(new DifferentialChangeset())->load($id); - if (!$changeset) { - return new Aphront404Response(); + $load_ids = array($id); + if ($vs && ($vs != -1)) { + $load_ids[] = $vs; } - // TODO: (T603) Make Changeset policy-aware. For now, just fake it - // by making sure we can see the diff. - $diff = id(new DifferentialDiffQuery()) + $changesets = id(new DifferentialChangesetQuery()) ->setViewer($request->getUser()) - ->withIDs(array($changeset->getDiffID())) - ->executeOne(); - if (!$diff) { + ->withIDs($load_ids) + ->needHunks(true) + ->execute(); + $changesets = mpull($changesets, null, 'getID'); + + $changeset = idx($changesets, $id); + if (!$changeset) { return new Aphront404Response(); } + $vs_changeset = null; + if ($vs && ($vs != -1)) { + $vs_changeset = idx($changesets, $vs); + if (!$vs_changeset) { + return new Aphront404Response(); + } + } $view = $request->getStr('view'); if ($view) { - $changeset->attachHunks($changeset->loadHunks()); $phid = idx($changeset->getMetadata(), "$view:binary-phid"); if ($phid) { return id(new AphrontRedirectResponse())->setURI("/file/info/$phid/"); @@ -50,12 +58,8 @@ case 'new': return $this->buildRawFileResponse($changeset, $is_new = true); case 'old': - if ($vs && ($vs != -1)) { - $vs_changeset = id(new DifferentialChangeset())->load($vs); - if ($vs_changeset) { - $vs_changeset->attachHunks($vs_changeset->loadHunks()); - return $this->buildRawFileResponse($vs_changeset, $is_new = true); - } + if ($vs_changeset) { + return $this->buildRawFileResponse($vs_changeset, $is_new = true); } return $this->buildRawFileResponse($changeset, $is_new = false); default: @@ -63,13 +67,6 @@ } } - if ($vs && ($vs != -1)) { - $vs_changeset = id(new DifferentialChangeset())->load($vs); - if (!$vs_changeset) { - return new Aphront404Response(); - } - } - if (!$vs) { $right = $changeset; $left = null; @@ -103,15 +100,6 @@ } if ($left) { - $left->attachHunks($left->loadHunks()); - } - - if ($right) { - $right->attachHunks($right->loadHunks()); - } - - if ($left) { - $left_data = $left->makeNewFile(); if ($right) { $right_data = $right->makeNewFile(); @@ -264,9 +252,30 @@ ), $detail->render())); + $crumbs = $this->buildApplicationCrumbs(); + + $revision_id = $changeset->getDiff()->getRevisionID(); + if ($revision_id) { + $crumbs->addTextCrumb('D'.$revision_id, '/D'.$revision_id); + } + + $diff_id = $changeset->getDiff()->getID(); + if ($diff_id) { + $crumbs->addTextCrumb( + pht('Diff %d', $diff_id), + $this->getApplicationURI('diff/'.$diff_id)); + } + + $crumbs->addTextCrumb($changeset->getDisplayFilename()); + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Standalone View')) + ->appendChild($panel); + return $this->buildApplicationPage( array( - $panel + $crumbs, + $box, ), array( 'title' => pht('Changeset View'), diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -844,9 +844,11 @@ $viewer = $this->getRequest()->getUser(); - foreach ($changesets as $changeset) { - $changeset->attachHunks($changeset->loadHunks()); - } + id(new DifferentialHunkQuery()) + ->setViewer($viewer) + ->withChangesets($changesets) + ->needAttachToChangesets(true) + ->execute(); $diff = new DifferentialDiff(); $diff->attachChangesets($changesets); diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1284,15 +1284,13 @@ $changeset_ids[$id] = $id; } - // TODO: We should write a proper Query class for this eventually. - $changesets = id(new DifferentialChangeset())->loadAllWhere( - 'id IN (%Ld)', - $changeset_ids); if ($show_context) { $hunk_parser = new DifferentialHunkParser(); - foreach ($changesets as $changeset) { - $changeset->attachHunks($changeset->loadHunks()); - } + $changesets = id(new DifferentialChangesetQuery()) + ->setViewer($this->getActor()) + ->withIDs($changeset_ids) + ->needHunks(true) + ->execute(); } $inline_groups = DifferentialTransactionComment::sortAndGroupInlines( diff --git a/src/applications/differential/query/DifferentialChangesetQuery.php b/src/applications/differential/query/DifferentialChangesetQuery.php new file mode 100644 --- /dev/null +++ b/src/applications/differential/query/DifferentialChangesetQuery.php @@ -0,0 +1,158 @@ +ids = $ids; + return $this; + } + + public function withDiffs(array $diffs) { + assert_instances_of($diffs, 'DifferentialDiff'); + $this->diffs = $diffs; + return $this; + } + + public function needAttachToDiffs($attach) { + $this->needAttachToDiffs = $attach; + return $this; + } + + public function needHunks($need) { + $this->needHunks = $need; + return $this; + } + + public function willExecute() { + // If we fail to load any changesets (which is possible in the case of an + // empty commit) we'll never call didFilterPage(). Attach empty changeset + // lists now so that we end up with the right result. + if ($this->needAttachToDiffs) { + foreach ($this->diffs as $diff) { + $diff->attachChangesets(array()); + } + } + } + + public function loadPage() { + $table = new DifferentialChangeset(); + $conn_r = $table->establishConnection('r'); + + $data = queryfx_all( + $conn_r, + 'SELECT * FROM %T %Q %Q %Q', + $table->getTableName(), + $this->buildWhereClause($conn_r), + $this->buildOrderClause($conn_r), + $this->buildLimitClause($conn_r)); + + return $table->loadAllFromArray($data); + } + + public function willFilterPage(array $changesets) { + + // First, attach all the diffs we already have. We can just do this + // directly without worrying about querying for them. When we don't have + // a diff, record that we need to load it. + if ($this->diffs) { + $have_diffs = mpull($this->diffs, null, 'getID'); + } else { + $have_diffs = array(); + } + + $must_load = array(); + foreach ($changesets as $key => $changeset) { + $diff_id = $changeset->getDiffID(); + if (isset($have_diffs[$diff_id])) { + $changeset->attachDiff($have_diffs[$diff_id]); + } else { + $must_load[$key] = $changeset; + } + } + + // Load all the diffs we don't have. + $need_diff_ids = mpull($must_load, 'getDiffID'); + $more_diffs = array(); + if ($need_diff_ids) { + $more_diffs = id(new DifferentialDiffQuery()) + ->setViewer($this->getViewer()) + ->setParentQuery($this) + ->withIDs($need_diff_ids) + ->execute(); + $more_diffs = mpull($more_diffs, null, 'getID'); + } + + // Attach the diffs we loaded. + foreach ($must_load as $key => $changeset) { + $diff_id = $changeset->getDiffID(); + if (isset($more_diffs[$diff_id])) { + $changeset->attachDiff($more_diffs[$diff_id]); + } else { + // We didn't have the diff, and could not load it (it does not exist, + // or we can't see it), so filter this result out. + unset($changesets[$key]); + } + } + + return $changesets; + } + + public function didFilterPage(array $changesets) { + if ($this->needAttachToDiffs) { + $changeset_groups = mgroup($changesets, 'getDiffID'); + foreach ($this->diffs as $diff) { + $diff_changesets = idx($changeset_groups, $diff->getID(), array()); + $diff->attachChangesets($diff_changesets); + } + } + + if ($this->needHunks) { + id(new DifferentialHunkQuery()) + ->setViewer($this->getViewer()) + ->setParentQuery($this) + ->withChangesets($changesets) + ->needAttachToChangesets(true) + ->execute(); + } + + return $changesets; + } + + private function buildWhereClause(AphrontDatabaseConnection $conn_r) { + $where = array(); + + if ($this->diffs !== null) { + $where[] = qsprintf( + $conn_r, + 'diffID IN (%Ld)', + mpull($this->diffs, 'getID')); + } + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn_r, + 'id IN (%Ld)', + $this->ids); + } + + $where[] = $this->buildPagingClause($conn_r); + + return $this->formatWhereClause($where); + } + + public function getQueryApplicationClass() { + return 'PhabricatorApplicationDifferential'; + } + + protected function getReversePaging() { + return true; + } + +} diff --git a/src/applications/differential/query/DifferentialDiffQuery.php b/src/applications/differential/query/DifferentialDiffQuery.php --- a/src/applications/differential/query/DifferentialDiffQuery.php +++ b/src/applications/differential/query/DifferentialDiffQuery.php @@ -88,26 +88,13 @@ } private function loadChangesets(array $diffs) { - $diff_ids = mpull($diffs, 'getID'); - - $changesets = id(new DifferentialChangeset())->loadAllWhere( - 'diffID IN (%Ld)', - $diff_ids); - - if ($changesets) { - id(new DifferentialHunkQuery()) - ->setViewer($this->getViewer()) - ->setParentQuery($this) - ->withChangesets($changesets) - ->needAttachToChangesets(true) - ->execute(); - } - - $changeset_groups = mgroup($changesets, 'getDiffID'); - foreach ($diffs as $diff) { - $diff_changesets = idx($changeset_groups, $diff->getID(), array()); - $diff->attachChangesets($diff_changesets); - } + id(new DifferentialChangesetQuery()) + ->setViewer($this->getViewer()) + ->setParentQuery($this) + ->withDiffs($diffs) + ->needAttachToDiffs(true) + ->needHunks(true) + ->execute(); return $diffs; } diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -62,15 +62,6 @@ return $this; } - public function loadHunks() { - if (!$this->getID()) { - return array(); - } - return id(new DifferentialHunk())->loadAllWhere( - 'changesetID = %d', - $this->getID()); - } - public function save() { $this->openTransaction(); $ret = parent::save(); @@ -84,9 +75,14 @@ public function delete() { $this->openTransaction(); - foreach ($this->loadHunks() as $hunk) { + + $hunks = id(new DifferentialHunk())->loadAllWhere( + 'changesetID = %d', + $this->getID()); + foreach ($hunks as $hunk) { $hunk->delete(); } + $this->unsavedHunks = array(); queryfx( @@ -174,6 +170,15 @@ return false; } + public function attachDiff(DifferentialDiff $diff) { + $this->diff = $diff; + return $this; + } + + public function getDiff() { + return $this->assertAttached($this->diff); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ @@ -185,13 +190,11 @@ } public function getPolicy($capability) { - // TODO: For now, these are never queried directly through the policy - // framework. Fix that up. - return PhabricatorPolicies::getMostOpenPolicy(); + return $this->getDiff()->getPolicy($capability); } public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { - return false; + return $this->getDiff()->hasAutomaticCapability($capability, $viewer); } public function describeAutomaticCapability($capability) {