From 63ce37248032a77e7ae5604b59c2bcc6567677e3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 May 2012 13:43:45 -0700 Subject: [PATCH] Add "DiffusionRawDiffQuery" Summary: - This is only slightly useful for updating Differential, since DiffQuery (vs RawDiffQuery) already gets you most of what you need. The only thing is that DiffQuery returns the diff for one path only right now(and the SVN version is very "special"). Should be easy to fix in the Git/HG cases at least, though (or maybe just use RawDiffQuery to avoid the SVN mess). - Added a "download raw diff" link. Test Plan: Viewed Diffusion and raw commits for SVN, Mercurial and Git repositories. Reviewers: vrana, btrahan, jungejason Reviewed By: vrana CC: aran Differential Revision: https://secure.phabricator.com/D2350 --- conf/default.conf.php | 1 + src/__phutil_library_map__.php | 8 ++ .../commit/DiffusionCommitController.php | 38 +++++++- .../diffusion/controller/commit/__init__.php | 5 ++ .../query/diff/git/DiffusionGitDiffQuery.php | 40 +-------- .../diffusion/query/diff/git/__init__.php | 2 +- .../mercurial/DiffusionMercurialDiffQuery.php | 9 +- .../query/diff/mercurial/__init__.php | 1 + .../rawdiff/base/DiffusionRawDiffQuery.php | 52 +++++++++++ .../diffusion/query/rawdiff/base/__init__.php | 12 +++ .../rawdiff/git/DiffusionGitRawDiffQuery.php | 85 ++++++++++++++++++ .../diffusion/query/rawdiff/git/__init__.php | 16 ++++ .../DiffusionMercurialRawDiffQuery.php | 45 ++++++++++ .../query/rawdiff/mercurial/__init__.php | 14 +++ .../rawdiff/svn/DiffusionSvnRawDiffQuery.php | 42 +++++++++ .../diffusion/query/rawdiff/svn/__init__.php | 12 +++ .../css/aphront/headsup-action-list-view.css | 4 + webroot/rsrc/image/icon/tango/go-down.png | Bin 0 -> 683 bytes 18 files changed, 339 insertions(+), 47 deletions(-) create mode 100644 src/applications/diffusion/query/rawdiff/base/DiffusionRawDiffQuery.php create mode 100644 src/applications/diffusion/query/rawdiff/base/__init__.php create mode 100644 src/applications/diffusion/query/rawdiff/git/DiffusionGitRawDiffQuery.php create mode 100644 src/applications/diffusion/query/rawdiff/git/__init__.php create mode 100644 src/applications/diffusion/query/rawdiff/mercurial/DiffusionMercurialRawDiffQuery.php create mode 100644 src/applications/diffusion/query/rawdiff/mercurial/__init__.php create mode 100644 src/applications/diffusion/query/rawdiff/svn/DiffusionSvnRawDiffQuery.php create mode 100644 src/applications/diffusion/query/rawdiff/svn/__init__.php create mode 100644 webroot/rsrc/image/icon/tango/go-down.png diff --git a/conf/default.conf.php b/conf/default.conf.php index 2d5f697122..fb23e659f8 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -645,6 +645,7 @@ return array( 'image/png' => 'image/png', 'image/gif' => 'image/gif', 'text/plain' => 'text/plain; charset=utf-8', + 'text/x-diff' => 'text/plain; charset=utf-8', // ".ico" favicon files, which have mime type diversity. See: // http://en.wikipedia.org/wiki/ICO_(file_format)#MIME_type diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index dfd985b953..52f8772e47 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -325,6 +325,7 @@ phutil_register_library_map(array( 'DiffusionGitHistoryQuery' => 'applications/diffusion/query/history/git', 'DiffusionGitLastModifiedQuery' => 'applications/diffusion/query/lastmodified/git', 'DiffusionGitMergedCommitsQuery' => 'applications/diffusion/query/mergedcommits/git', + 'DiffusionGitRawDiffQuery' => 'applications/diffusion/query/rawdiff/git', 'DiffusionGitRenameHistoryQuery' => 'applications/diffusion/query/renamehistory/git', 'DiffusionGitRequest' => 'applications/diffusion/request/git', 'DiffusionGitTagListQuery' => 'applications/diffusion/query/taglist/git', @@ -345,6 +346,7 @@ phutil_register_library_map(array( 'DiffusionMercurialHistoryQuery' => 'applications/diffusion/query/history/mercurial', 'DiffusionMercurialLastModifiedQuery' => 'applications/diffusion/query/lastmodified/mercurial', 'DiffusionMercurialMergedCommitsQuery' => 'applications/diffusion/query/mergedcommits/mercurial', + 'DiffusionMercurialRawDiffQuery' => 'applications/diffusion/query/rawdiff/mercurial', 'DiffusionMercurialRenameHistoryQuery' => 'applications/diffusion/query/renamehistory/mercurial', 'DiffusionMercurialRequest' => 'applications/diffusion/request/mercurial', 'DiffusionMercurialTagListQuery' => 'applications/diffusion/query/taglist/mercurial', @@ -357,6 +359,7 @@ phutil_register_library_map(array( 'DiffusionPathQueryTestCase' => 'applications/diffusion/query/pathid/base/__tests__', 'DiffusionPathValidateController' => 'applications/diffusion/controller/pathvalidate', 'DiffusionQuery' => 'applications/diffusion/query/base', + 'DiffusionRawDiffQuery' => 'applications/diffusion/query/rawdiff/base', 'DiffusionRenameHistoryQuery' => 'applications/diffusion/query/renamehistory/base', 'DiffusionRepositoryController' => 'applications/diffusion/controller/repository', 'DiffusionRepositoryPath' => 'applications/diffusion/data/repositorypath', @@ -371,6 +374,7 @@ phutil_register_library_map(array( 'DiffusionSvnHistoryQuery' => 'applications/diffusion/query/history/svn', 'DiffusionSvnLastModifiedQuery' => 'applications/diffusion/query/lastmodified/svn', 'DiffusionSvnMergedCommitsQuery' => 'applications/diffusion/query/mergedcommits/svn', + 'DiffusionSvnRawDiffQuery' => 'applications/diffusion/query/rawdiff/svn', 'DiffusionSvnRenameHistoryQuery' => 'applications/diffusion/query/renamehistory/svn', 'DiffusionSvnRequest' => 'applications/diffusion/request/svn', 'DiffusionSvnTagListQuery' => 'applications/diffusion/query/taglist/svn', @@ -1301,6 +1305,7 @@ phutil_register_library_map(array( 'DiffusionGitHistoryQuery' => 'DiffusionHistoryQuery', 'DiffusionGitLastModifiedQuery' => 'DiffusionLastModifiedQuery', 'DiffusionGitMergedCommitsQuery' => 'DiffusionMergedCommitsQuery', + 'DiffusionGitRawDiffQuery' => 'DiffusionRawDiffQuery', 'DiffusionGitRenameHistoryQuery' => 'DiffusionRenameHistoryQuery', 'DiffusionGitRequest' => 'DiffusionRequest', 'DiffusionGitTagListQuery' => 'DiffusionTagListQuery', @@ -1321,6 +1326,7 @@ phutil_register_library_map(array( 'DiffusionMercurialHistoryQuery' => 'DiffusionHistoryQuery', 'DiffusionMercurialLastModifiedQuery' => 'DiffusionLastModifiedQuery', 'DiffusionMercurialMergedCommitsQuery' => 'DiffusionMergedCommitsQuery', + 'DiffusionMercurialRawDiffQuery' => 'DiffusionRawDiffQuery', 'DiffusionMercurialRenameHistoryQuery' => 'DiffusionRenameHistoryQuery', 'DiffusionMercurialRequest' => 'DiffusionRequest', 'DiffusionMercurialTagListQuery' => 'DiffusionTagListQuery', @@ -1328,6 +1334,7 @@ phutil_register_library_map(array( 'DiffusionPathCompleteController' => 'DiffusionController', 'DiffusionPathQueryTestCase' => 'PhabricatorTestCase', 'DiffusionPathValidateController' => 'DiffusionController', + 'DiffusionRawDiffQuery' => 'DiffusionQuery', 'DiffusionRenameHistoryQuery' => 'DiffusionQuery', 'DiffusionRepositoryController' => 'DiffusionController', 'DiffusionSvnBrowseQuery' => 'DiffusionBrowseQuery', @@ -1339,6 +1346,7 @@ phutil_register_library_map(array( 'DiffusionSvnHistoryQuery' => 'DiffusionHistoryQuery', 'DiffusionSvnLastModifiedQuery' => 'DiffusionLastModifiedQuery', 'DiffusionSvnMergedCommitsQuery' => 'DiffusionMergedCommitsQuery', + 'DiffusionSvnRawDiffQuery' => 'DiffusionRawDiffQuery', 'DiffusionSvnRenameHistoryQuery' => 'DiffusionRenameHistoryQuery', 'DiffusionSvnRequest' => 'DiffusionRequest', 'DiffusionSvnTagListQuery' => 'DiffusionTagListQuery', diff --git a/src/applications/diffusion/controller/commit/DiffusionCommitController.php b/src/applications/diffusion/controller/commit/DiffusionCommitController.php index b9346efb64..ec3afefaae 100644 --- a/src/applications/diffusion/controller/commit/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/commit/DiffusionCommitController.php @@ -34,6 +34,10 @@ final class DiffusionCommitController extends DiffusionController { $request = $this->getRequest(); $user = $request->getUser(); + if ($request->getStr('diff')) { + return $this->buildRawDiffResponse($drequest); + } + $callsign = $drequest->getRepository()->getCallsign(); $content = array(); @@ -676,7 +680,8 @@ final class DiffusionCommitController extends DiffusionController { private function renderHeadsupActionList( PhabricatorRepositoryCommit $commit) { - $user = $this->getRequest()->getUser(); + $request = $this->getRequest(); + $user = $request->getUser(); $actions = array(); @@ -725,6 +730,12 @@ final class DiffusionCommitController extends DiffusionController { $action->setClass('transcripts-herald'); $actions[] = $action; + $action = new AphrontHeadsupActionView(); + $action->setName('Download Raw Diff'); + $action->setURI($request->getRequestURI()->alter('diff', true)); + $action->setClass('action-download'); + $actions[] = $action; + $action_list = new AphrontHeadsupActionListView(); $action_list->setActions($actions); @@ -795,4 +806,29 @@ final class DiffusionCommitController extends DiffusionController { return trim($stdout, "() \n"); } + private function buildRawDiffResponse(DiffusionRequest $drequest) { + $raw_query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest); + $raw_diff = $raw_query->loadRawDiff(); + + $hash = PhabricatorHash::digest($raw_diff); + + $file = id(new PhabricatorFile())->loadOneWhere( + 'contentHash = %s LIMIT 1', + $hash); + if (!$file) { + // We're just caching the data; this is always safe. + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + + $file = PhabricatorFile::newFromFileData( + $raw_diff, + array( + 'name' => $drequest->getCommit().'.diff', + )); + + unset($unguarded); + } + + return id(new AphrontRedirectResponse())->setURI($file->getBestURI()); + } + } diff --git a/src/applications/diffusion/controller/commit/__init__.php b/src/applications/diffusion/controller/commit/__init__.php index 75743da859..03fb3bdd9e 100644 --- a/src/applications/diffusion/controller/commit/__init__.php +++ b/src/applications/diffusion/controller/commit/__init__.php @@ -6,6 +6,8 @@ +phutil_require_module('phabricator', 'aphront/response/redirect'); +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/audit/constants/action'); phutil_require_module('phabricator', 'applications/audit/constants/commitstatus'); phutil_require_module('phabricator', 'applications/audit/constants/status'); @@ -25,11 +27,13 @@ phutil_require_module('phabricator', 'applications/diffusion/query/parents/base' phutil_require_module('phabricator', 'applications/diffusion/query/path'); phutil_require_module('phabricator', 'applications/diffusion/query/pathchange/base'); phutil_require_module('phabricator', 'applications/diffusion/query/pathid/base'); +phutil_require_module('phabricator', 'applications/diffusion/query/rawdiff/base'); phutil_require_module('phabricator', 'applications/diffusion/request/base'); phutil_require_module('phabricator', 'applications/diffusion/view/commentlist'); phutil_require_module('phabricator', 'applications/diffusion/view/commitchangetable'); phutil_require_module('phabricator', 'applications/diffusion/view/historytable'); phutil_require_module('phabricator', 'applications/draft/storage/draft'); +phutil_require_module('phabricator', 'applications/files/storage/file'); phutil_require_module('phabricator', 'applications/flag/constants/color'); phutil_require_module('phabricator', 'applications/flag/query/flag'); phutil_require_module('phabricator', 'applications/markup/engine'); @@ -41,6 +45,7 @@ phutil_require_module('phabricator', 'infrastructure/edges/constants/config'); phutil_require_module('phabricator', 'infrastructure/edges/query/edge'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'infrastructure/javelin/api'); +phutil_require_module('phabricator', 'infrastructure/util/hash'); phutil_require_module('phabricator', 'storage/queryfx'); phutil_require_module('phabricator', 'view/form/base'); phutil_require_module('phabricator', 'view/form/control/select'); diff --git a/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php b/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php index b7a34f4d83..550d1c96b7 100644 --- a/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php +++ b/src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php @@ -29,44 +29,8 @@ final class DiffusionGitDiffQuery extends DiffusionDiffQuery { // TODO: This side effect is kind of skethcy. $drequest->setCommit($effective_commit); - $options = array( - '-M', - '-C', - '--no-ext-diff', - '--no-color', - '--src-prefix=a/', - '--dst-prefix=b/', - '-U65535', - ); - $options = implode(' ', $options); - - try { - list($raw_diff) = $repository->execxLocalCommand( - 'diff %C %s^ %s -- %s', - $options, - $effective_commit, - $effective_commit, - $drequest->getPath()); - } catch (CommandException $ex) { - // Check if this is the root commit by seeing if it has parents. - list($parents) = $repository->execxLocalCommand( - 'log --format=%s %s --', - '%P', // "parents" - $effective_commit); - if (!strlen(trim($parents))) { - // No parents means we're looking at the root revision. Diff against - // the empty tree hash instead, since there is no parent so "^" does - // not work. See ArcanistGitAPI for more discussion. - list($raw_diff) = $repository->execxLocalCommand( - 'diff %C %s %s -- %s', - $options, - ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT, - $effective_commit, - $drequest->getPath()); - } else { - throw $ex; - } - } + $raw_query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest); + $raw_diff = $raw_query->loadRawDiff(); if (!$raw_diff) { return null; diff --git a/src/applications/diffusion/query/diff/git/__init__.php b/src/applications/diffusion/query/diff/git/__init__.php index 766d74997d..2550760944 100644 --- a/src/applications/diffusion/query/diff/git/__init__.php +++ b/src/applications/diffusion/query/diff/git/__init__.php @@ -7,10 +7,10 @@ phutil_require_module('arcanist', 'parser/diff'); -phutil_require_module('arcanist', 'repository/api/git'); phutil_require_module('phabricator', 'applications/differential/storage/diff'); phutil_require_module('phabricator', 'applications/diffusion/query/diff/base'); +phutil_require_module('phabricator', 'applications/diffusion/query/rawdiff/base'); phutil_require_source('DiffusionGitDiffQuery.php'); diff --git a/src/applications/diffusion/query/diff/mercurial/DiffusionMercurialDiffQuery.php b/src/applications/diffusion/query/diff/mercurial/DiffusionMercurialDiffQuery.php index 7cfe68e988..d6525e95fb 100644 --- a/src/applications/diffusion/query/diff/mercurial/DiffusionMercurialDiffQuery.php +++ b/src/applications/diffusion/query/diff/mercurial/DiffusionMercurialDiffQuery.php @@ -29,13 +29,8 @@ final class DiffusionMercurialDiffQuery extends DiffusionDiffQuery { // TODO: This side effect is kind of skethcy. $drequest->setCommit($effective_commit); - $path = $drequest->getPath(); - - list($raw_diff) = $repository->execxLocalCommand( - 'diff -U %d --git --change %s -- %s', - 65535, - $effective_commit, - $path); + $query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest); + $raw_diff = $query->loadRawDiff(); $parser = new ArcanistDiffParser(); diff --git a/src/applications/diffusion/query/diff/mercurial/__init__.php b/src/applications/diffusion/query/diff/mercurial/__init__.php index 8daa080d53..e28ac04e6b 100644 --- a/src/applications/diffusion/query/diff/mercurial/__init__.php +++ b/src/applications/diffusion/query/diff/mercurial/__init__.php @@ -10,6 +10,7 @@ phutil_require_module('arcanist', 'parser/diff'); phutil_require_module('phabricator', 'applications/differential/storage/diff'); phutil_require_module('phabricator', 'applications/diffusion/query/diff/base'); +phutil_require_module('phabricator', 'applications/diffusion/query/rawdiff/base'); phutil_require_source('DiffusionMercurialDiffQuery.php'); diff --git a/src/applications/diffusion/query/rawdiff/base/DiffusionRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/base/DiffusionRawDiffQuery.php new file mode 100644 index 0000000000..d9eab546ea --- /dev/null +++ b/src/applications/diffusion/query/rawdiff/base/DiffusionRawDiffQuery.php @@ -0,0 +1,52 @@ +executeQuery(); + } + + final public function setTimeout($timeout) { + $this->timeout = $timeout; + return $this; + } + + final public function getTimeout() { + return $this->timeout; + } + + final public function setLinesOfContext($lines_of_context) { + $this->linesOfContext = $lines_of_context; + return $this; + } + + final public function getLinesOfContext() { + return $this->linesOfContext; + } + +} diff --git a/src/applications/diffusion/query/rawdiff/base/__init__.php b/src/applications/diffusion/query/rawdiff/base/__init__.php new file mode 100644 index 0000000000..3d228bac08 --- /dev/null +++ b/src/applications/diffusion/query/rawdiff/base/__init__.php @@ -0,0 +1,12 @@ +getRequest(); + $repository = $drequest->getRepository(); + + $commit = $drequest->getCommit(); + + $options = array( + '-M', + '-C', + '--no-ext-diff', + '--no-color', + '--src-prefix=a/', + '--dst-prefix=b/', + '-U'.(int)$this->getLinesOfContext(), + ); + $options = implode(' ', $options); + + // If there's no path, get the entire raw diff. + $path = nonempty($drequest->getPath(), '.'); + + $future = $repository->getLocalCommandFuture( + "diff %C %s^ %s -- %s", + $options, + $commit, + $commit, + $path); + + if ($this->getTimeout()) { + $future->setTimeout($this->getTimeout()); + } + + try { + list($raw_diff) = $future->resolvex(); + } catch (CommandException $ex) { + // Check if this is the root commit by seeing if it has parents. + list($parents) = $repository->execxLocalCommand( + 'log --format=%s %s --', + '%P', // "parents" + $effective_commit); + + if (strlen(trim($parents))) { + throw $ex; + } + + // No parents means we're looking at the root revision. Diff against + // the empty tree hash instead, since there is no parent so "^" does + // not work. See ArcanistGitAPI for more discussion. + $future = $repository->getLocalCommandFuture( + 'diff %C %s %s -- %s', + $options, + ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT, + $commit, + $drequest->getPath()); + + if ($this->getTimeout()) { + $future->setTimeout($this->getTimeout()); + } + + list($raw_diff) = $future->resolvex(); + } + + return $raw_diff; + } + +} diff --git a/src/applications/diffusion/query/rawdiff/git/__init__.php b/src/applications/diffusion/query/rawdiff/git/__init__.php new file mode 100644 index 0000000000..e619a17d62 --- /dev/null +++ b/src/applications/diffusion/query/rawdiff/git/__init__.php @@ -0,0 +1,16 @@ +getRequest(); + $repository = $drequest->getRepository(); + + $commit = $drequest->getCommit(); + + // If there's no path, get the entire raw diff. + $path = nonempty($drequest->getPath(), '.'); + + $future = $repository->getLocalCommandFuture( + 'diff -U %d --git --change %s -- %s', + $this->getLinesOfContext(), + $commit, + $path); + + if ($this->getTimeout()) { + $future->setTimeout($this->getTimeout()); + } + + list($raw_diff) = $future->resolvex(); + + return $raw_diff; + } + +} diff --git a/src/applications/diffusion/query/rawdiff/mercurial/__init__.php b/src/applications/diffusion/query/rawdiff/mercurial/__init__.php new file mode 100644 index 0000000000..0151f1d2d0 --- /dev/null +++ b/src/applications/diffusion/query/rawdiff/mercurial/__init__.php @@ -0,0 +1,14 @@ +getRequest(); + $repository = $drequest->getRepository(); + + $commit = $drequest->getCommit(); + + $future = $repository->getRemoteCommandFuture( + 'diff --diff-cmd diff -x -U%d -c %d %s%s@', + $this->getLinesOfContext(), + $commit, + $repository->getRemoteURI(), + $drequest->getPath()); + + if ($this->getTimeout()) { + $future->setTimeout($this->getTimeout()); + } + + list($raw_diff) = $future->resolvex(); + return $raw_diff; + } + +} diff --git a/src/applications/diffusion/query/rawdiff/svn/__init__.php b/src/applications/diffusion/query/rawdiff/svn/__init__.php new file mode 100644 index 0000000000..f17276965c --- /dev/null +++ b/src/applications/diffusion/query/rawdiff/svn/__init__.php @@ -0,0 +1,12 @@ +i}4~9FCHr5K@{qtRq-ka(iJ?ZP=uTm zA)-=KTXM11RS2~$6nm&uv{cQSuKf$?CbP4jht?XK7~kVJ-#qia^UW|KLbc-EY953z z>WRV7dqi_}NvUZfgl}C)!*&F0M_|b+V97E80CykVr~%gk05Haon|Y41T|%KagO77# zXg_$ht|?xxSRKlv`0H+L2mld?91sx{?rsQB5|>q-9K_b`yI?say^?H5vawt?QN0%L zQr8VKjyDQ9NJT;|(TgXq`xKW7BF8I9f|vxjL{S!?dO5hlaQ@UaF9;B#f^;@jnqQnt zF(N{uTTQn`k0*~rlb-kaSCCvlqKp-L5!3TI5NItHN89$(7@Zg?Pfm^Zz3vh1d@XXv z%dw2{#h9WC`nbQFn~GCVFR$PcMeY21IKx0j@A8ZjM9Y6Ue=LTlryr z>(@2W+wdKbgN~t*f$yfVK)ah_*yWGG{JKoJQ9bX-)!YpMx+aPwk<4VDSzECWL8lc@ z`=3~j{FA#{Y~yeIt$3GuF4Da7HUP}#KVRBt{l5SJIDAFD4*