From 3e15c3580daa90a54c0771dec0f8d7d574fb5c0d Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 10:38:25 -0700 Subject: [PATCH] Simplify build file from data-or-hash code Summary: We have a bit more copy-paste than we need, consolidate a bit. (Also switch Mercurial to download git diffs, which it handles well; we use them in "arc patch".) Test Plan: - Downloaded a raw diff from Differential. - Downloaded a raw change from Diffusion. - Downloaded a raw file from Diffusion. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2942 --- .../DifferentialRevisionViewController.php | 51 +++++++------------ .../DiffusionBrowseFileController.php | 24 ++------- .../controller/DiffusionCommitController.php | 22 ++------ .../files/storage/PhabricatorFile.php | 39 ++++++++++++++ 4 files changed, 68 insertions(+), 68 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 3b5a157d29..5b5c824969 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -940,49 +940,36 @@ final class DifferentialRevisionViewController extends DifferentialController { $vcs = $repository ? $repository->getVersionControlSystem() : null; switch ($vcs) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: $raw_diff = $bundle->toGitPatch(); break; case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: default: $raw_diff = $bundle->toUnifiedDiff(); break; } - $hash = PhabricatorHash::digest($raw_diff); + $request_uri = $this->getRequest()->getRequestURI(); - $file = id(new PhabricatorFile())->loadOneWhere( - 'contentHash = %s LIMIT 1', - $hash); - - if (!$file) { - $request_uri = $this->getRequest()->getRequestURI(); - - // this ends up being something like - // D123.diff - // or the verbose - // D123.vs123.id123.whitespaceignore-all.diff - // lame but nice to include these options - $file_name = ltrim($request_uri->getPath(), '/') . '.'; - foreach ($request_uri->getQueryParams() as $key => $value) { - if ($key == 'download') { - continue; - } - $file_name .= $key . $value . '.'; + // this ends up being something like + // D123.diff + // or the verbose + // D123.vs123.id123.whitespaceignore-all.diff + // lame but nice to include these options + $file_name = ltrim($request_uri->getPath(), '/').'.'; + foreach ($request_uri->getQueryParams() as $key => $value) { + if ($key == 'download') { + continue; } - $file_name .= 'diff'; - - // We're just caching the data; this is always safe. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - - $file = PhabricatorFile::newFromFileData( - $raw_diff, - array( - 'name' => $file_name, - )); - - unset($unguarded); + $file_name .= $key.$value.'.'; } + $file_name .= 'diff'; + + $file = PhabricatorFile::buildFromFileDataOrHash( + $raw_diff, + array( + 'name' => $file_name, + )); return id(new AphrontRedirectResponse())->setURI($file->getBestURI()); diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index e6822d8667..cb04b5eb06 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -639,25 +639,11 @@ final class DiffusionBrowseFileController extends DiffusionController { } private function loadFileForData($path, $data) { - $hash = PhabricatorHash::digest($data); - - $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( - $data, - array( - 'name' => basename($path), - )); - - unset($unguarded); - } - - return $file; + return PhabricatorFile::buildFromFileDataOrHash( + $data, + array( + 'name' => basename($path), + )); } private function buildRawResponse($path, $data) { diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 893f0c308b..62d8b7023e 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -872,23 +872,11 @@ final class DiffusionCommitController extends DiffusionController { $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); - } + $file = PhabricatorFile::buildFromFileDataOrHash( + $raw_diff, + array( + 'name' => $drequest->getCommit().'.diff', + )); return id(new AphrontRedirectResponse())->setURI($file->getBestURI()); } diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index c0be163cbd..b55d402fbc 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -101,6 +101,45 @@ final class PhabricatorFile extends PhabricatorFileDAO { } } + + /** + * Given a block of data, try to load an existing file with the same content + * if one exists. If it does not, build a new file. + * + * This method is generally used when we have some piece of semi-trusted data + * like a diff or a file from a repository that we want to show to the user. + * We can't just dump it out because it may be dangerous for any number of + * reasons; instead, we need to serve it through the File abstraction so it + * ends up on the CDN domain if one is configured and so on. However, if we + * simply wrote a new file every time we'd potentially end up with a lot + * of redundant data in file storage. + * + * To solve these problems, we use file storage as a cache and reuse the + * same file again if we've previously written it. + * + * NOTE: This method unguards writes. + * + * @param string Raw file data. + * @param dict Dictionary of file information. + */ + public static function buildFromFileDataOrHash( + $data, + array $params = array()) { + + $file = id(new PhabricatorFile())->loadOneWhere( + 'contentHash = %s LIMIT 1', + PhabricatorHash::digest($data)); + + if (!$file) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $file = PhabricatorFile::newFromFileData($data, $params); + unset($unguarded); + } + + return $file; + } + + public static function newFromFileData($data, array $params = array()) { $selector = PhabricatorEnv::newObjectFromConfig('storage.engine-selector');