From 23ada21d3552c81985e528b19e595b950f372b45 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 13 May 2014 13:53:06 -0700 Subject: [PATCH] Remove `commit` from DiffusionRequest Summary: Ref T2683. This field is //almost// entirely redundant with `symbolicCommit`. Improve how some of the diff query stuff works a bit, then remove it. Test Plan: Browsed around in all interfaces, looked at a bunch of diffs, etc. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T2683 Differential Revision: https://secure.phabricator.com/D9099 --- .../ConduitAPI_diffusion_diffquery_Method.php | 8 +++--- .../controller/DiffusionChangeController.php | 5 ++-- .../controller/DiffusionDiffController.php | 2 +- .../controller/DiffusionHistoryController.php | 2 +- .../rawdiff/DiffusionGitRawDiffQuery.php | 2 +- .../DiffusionMercurialRawDiffQuery.php | 2 +- .../query/rawdiff/DiffusionRawDiffQuery.php | 15 ++++++++++- .../rawdiff/DiffusionSvnRawDiffQuery.php | 2 +- .../diffusion/request/DiffusionRequest.php | 25 +++++++++++-------- 9 files changed, 40 insertions(+), 23 deletions(-) diff --git a/src/applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php b/src/applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php index 6becde1908..4b4e8876ac 100644 --- a/src/applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php +++ b/src/applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php @@ -168,7 +168,7 @@ final class ConduitAPI_diffusion_diffquery_Method $drequest, 'diffusion.lastmodifiedquery', array( - 'paths' => array($path => $drequest->getCommit()), + 'paths' => array($path => $drequest->getStableCommit()), )); $this->effectiveCommit = idx($result, $path); @@ -199,10 +199,10 @@ final class ConduitAPI_diffusion_diffquery_Method if (!$effective_commit) { return $this->getEmptyResult(1); } - // TODO: This side effect is kind of sketchy. - $drequest->setCommit($effective_commit); - $raw_query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest); + $raw_query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest) + ->setAnchorCommit($effective_commit); + $raw_diff = $raw_query->loadRawDiff(); if (!$raw_diff) { return $this->getEmptyResult(2); diff --git a/src/applications/diffusion/controller/DiffusionChangeController.php b/src/applications/diffusion/controller/DiffusionChangeController.php index 01de42dead..520862e8e4 100644 --- a/src/applications/diffusion/controller/DiffusionChangeController.php +++ b/src/applications/diffusion/controller/DiffusionChangeController.php @@ -18,7 +18,9 @@ final class DiffusionChangeController extends DiffusionController { 'commit' => $drequest->getCommit(), 'path' => $drequest->getPath(), )); - $drequest->setCommit($data['effectiveCommit']); + + $drequest->updateSymbolicCommit($data['effectiveCommit']); + $raw_changes = ArcanistDiffChange::newFromConduit($data['changes']); $diff = DifferentialDiff::newFromRawChanges($raw_changes); $changesets = $diff->getChangesets(); @@ -31,7 +33,6 @@ final class DiffusionChangeController extends DiffusionController { $repository = $drequest->getRepository(); $callsign = $repository->getCallsign(); - $commit = $drequest->getSymbolicCommit(); $changesets = array( 0 => $changeset, ); diff --git a/src/applications/diffusion/controller/DiffusionDiffController.php b/src/applications/diffusion/controller/DiffusionDiffController.php index 5f6565bfb4..b8bc9680de 100644 --- a/src/applications/diffusion/controller/DiffusionDiffController.php +++ b/src/applications/diffusion/controller/DiffusionDiffController.php @@ -51,7 +51,7 @@ final class DiffusionDiffController extends DiffusionController { array( 'commit' => $drequest->getCommit(), 'path' => $drequest->getPath())); - $drequest->setCommit($data['effectiveCommit']); + $drequest->updateSymbolicCommit($data['effectiveCommit']); $raw_changes = ArcanistDiffChange::newFromConduit($data['changes']); $diff = DifferentialDiff::newFromRawChanges($raw_changes); $changesets = $diff->getChangesets(); diff --git a/src/applications/diffusion/controller/DiffusionHistoryController.php b/src/applications/diffusion/controller/DiffusionHistoryController.php index 91e4479acf..9010dc9f02 100644 --- a/src/applications/diffusion/controller/DiffusionHistoryController.php +++ b/src/applications/diffusion/controller/DiffusionHistoryController.php @@ -152,7 +152,7 @@ final class DiffusionHistoryController extends DiffusionController { ->setUser($viewer) ->setActionList($actions); - $stable_commit = $drequest->getStableCommitName(); + $stable_commit = $drequest->getStableCommit(); $callsign = $drequest->getRepository()->getCallsign(); $view->addProperty( diff --git a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php index ee6cbf13a6..566f8f9d02 100644 --- a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php +++ b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php @@ -6,7 +6,7 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); - $commit = $drequest->getCommit(); + $commit = $this->getAnchorCommit(); $options = array( '-M', diff --git a/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php index f4dee9c840..37aeed045e 100644 --- a/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php +++ b/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php @@ -10,7 +10,7 @@ final class DiffusionMercurialRawDiffQuery extends DiffusionRawDiffQuery { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); - $commit = $drequest->getCommit(); + $commit = $this->getAnchorCommit(); // If there's no path, get the entire raw diff. $path = nonempty($drequest->getPath(), '.'); diff --git a/src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php index c6d741ae47..da16b9e609 100644 --- a/src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php +++ b/src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php @@ -2,9 +2,9 @@ abstract class DiffusionRawDiffQuery extends DiffusionQuery { - private $request; private $timeout; private $linesOfContext = 65535; + private $anchorCommit; private $againstCommit; private $byteLimit; @@ -53,6 +53,19 @@ abstract class DiffusionRawDiffQuery extends DiffusionQuery { return $this->againstCommit; } + public function setAnchorCommit($anchor_commit) { + $this->anchorCommit = $anchor_commit; + return $this; + } + + public function getAnchorCommit() { + if ($this->anchorCommit) { + return $this->anchorCommit; + } + + return $this->getRequest()->getStableCommit(); + } + protected function configureFuture(ExecFuture $future) { if ($this->getTimeout()) { $future->setTimeout($this->getTimeout()); diff --git a/src/applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php index c66ec335a7..bc8384cb75 100644 --- a/src/applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php +++ b/src/applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php @@ -6,7 +6,7 @@ final class DiffusionSvnRawDiffQuery extends DiffusionRawDiffQuery { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); - $commit = $drequest->getCommit(); + $commit = $this->getAnchorCommit(); $arc_root = phutil_get_library_root('arcanist'); $against = $this->getAgainstCommit(); diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php index 65b717716c..eebc3ce92d 100644 --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -12,7 +12,6 @@ abstract class DiffusionRequest { protected $callsign; protected $path; protected $line; - protected $commit; protected $branch; protected $lint; @@ -241,11 +240,6 @@ abstract class DiffusionRequest { // TODO: Probably remove all of this. - // Required for sketchy sins that `diffusion.diffquery` commits. - if ($this->commit) { - return $this->commit; - } - if ($this->getSymbolicCommit() !== null) { return $this->getSymbolicCommit(); } @@ -272,6 +266,20 @@ abstract class DiffusionRequest { } + /** + * Modify the request to move the symbolic commit elsewhere. + * + * @param string New symbolic commit. + * @return this + */ + public function updateSymbolicCommit($symbol) { + $this->symbolicCommit = $symbol; + $this->symbolicType = null; + $this->stableCommit = null; + return $this; + } + + /** * Get the ref type (`commit` or `tag`) of the location associated with this * request. @@ -380,11 +388,6 @@ abstract class DiffusionRequest { return $this->repositoryCommitData; } - public function setCommit($commit) { - $this->commit = $commit; - return $this; - } - /* -( Managing Diffusion URIs )-------------------------------------------- */