diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4f6ae109bb..12a5ad9333 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1430,12 +1430,16 @@ phutil_register_library_map(array( 'HarbormasterBuildStep' => 'applications/harbormaster/storage/configuration/HarbormasterBuildStep.php', 'HarbormasterBuildStepCoreCustomField' => 'applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php', 'HarbormasterBuildStepCustomField' => 'applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php', + 'HarbormasterBuildStepEditAPIMethod' => 'applications/harbormaster/conduit/HarbormasterBuildStepEditAPIMethod.php', + 'HarbormasterBuildStepEditEngine' => 'applications/harbormaster/editor/HarbormasterBuildStepEditEngine.php', 'HarbormasterBuildStepEditor' => 'applications/harbormaster/editor/HarbormasterBuildStepEditor.php', 'HarbormasterBuildStepGroup' => 'applications/harbormaster/stepgroup/HarbormasterBuildStepGroup.php', 'HarbormasterBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterBuildStepImplementation.php', 'HarbormasterBuildStepImplementationTestCase' => 'applications/harbormaster/step/__tests__/HarbormasterBuildStepImplementationTestCase.php', 'HarbormasterBuildStepPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildStepPHIDType.php', 'HarbormasterBuildStepQuery' => 'applications/harbormaster/query/HarbormasterBuildStepQuery.php', + 'HarbormasterBuildStepSearchAPIMethod' => 'applications/harbormaster/conduit/HarbormasterBuildStepSearchAPIMethod.php', + 'HarbormasterBuildStepSearchEngine' => 'applications/harbormaster/query/HarbormasterBuildStepSearchEngine.php', 'HarbormasterBuildStepTransaction' => 'applications/harbormaster/storage/configuration/HarbormasterBuildStepTransaction.php', 'HarbormasterBuildStepTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildStepTransactionQuery.php', 'HarbormasterBuildTarget' => 'applications/harbormaster/storage/build/HarbormasterBuildTarget.php', @@ -7627,18 +7631,23 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionInterface', 'PhabricatorPolicyInterface', 'PhabricatorCustomFieldInterface', + 'PhabricatorConduitResultInterface', ), 'HarbormasterBuildStepCoreCustomField' => array( 'HarbormasterBuildStepCustomField', 'PhabricatorStandardCustomFieldInterface', ), 'HarbormasterBuildStepCustomField' => 'PhabricatorCustomField', + 'HarbormasterBuildStepEditAPIMethod' => 'PhabricatorEditEngineAPIMethod', + 'HarbormasterBuildStepEditEngine' => 'PhabricatorEditEngine', 'HarbormasterBuildStepEditor' => 'PhabricatorApplicationTransactionEditor', 'HarbormasterBuildStepGroup' => 'Phobject', 'HarbormasterBuildStepImplementation' => 'Phobject', 'HarbormasterBuildStepImplementationTestCase' => 'PhabricatorTestCase', 'HarbormasterBuildStepPHIDType' => 'PhabricatorPHIDType', 'HarbormasterBuildStepQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildStepSearchAPIMethod' => 'PhabricatorSearchEngineAPIMethod', + 'HarbormasterBuildStepSearchEngine' => 'PhabricatorApplicationSearchEngine', 'HarbormasterBuildStepTransaction' => 'PhabricatorApplicationTransaction', 'HarbormasterBuildStepTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'HarbormasterBuildTarget' => array( diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index 5bed835d8f..31b3db9a7f 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -176,7 +176,7 @@ final class AphrontApplicationConfiguration } $host = AphrontRequest::getHTTPHeader('Host'); - $path = $_REQUEST['__path__']; + $path = PhabricatorStartup::getRequestPath(); $application = new self(); @@ -759,7 +759,7 @@ final class AphrontApplicationConfiguration } private static function newSelfCheckResponse() { - $path = idx($_REQUEST, '__path__', ''); + $path = PhabricatorStartup::getRequestPath(); $query = idx($_SERVER, 'QUERY_STRING', ''); $pairs = id(new PhutilQueryStringParser()) diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php index 37e8d563b4..8b7751c795 100644 --- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -54,8 +54,8 @@ final class PhabricatorCommitSearchEngine $query->withUnreachable($map['unreachable']); } - if ($map['unpublished'] !== null) { - $query->withUnpublished($map['unpublished']); + if ($map['permanent'] !== null) { + $query->withPermanent($map['permanent']); } if ($map['ancestorsOf']) { @@ -132,15 +132,15 @@ final class PhabricatorCommitSearchEngine 'Find or exclude unreachable commits which are not ancestors of '. 'any branch, tag, or ref.')), id(new PhabricatorSearchThreeStateField()) - ->setLabel(pht('Unpublished')) - ->setKey('unpublished') + ->setLabel(pht('Permanent')) + ->setKey('permanent') ->setOptions( pht('(Show All)'), - pht('Show Only Unpublished Commits'), - pht('Hide Unpublished Commits')) + pht('Show Only Permanent Commits'), + pht('Hide Permanent Commits')) ->setDescription( pht( - 'Find or exclude unpublished commits which are not ancestors of '. + 'Find or exclude permanent commits which are ancestors of '. 'any permanent branch, tag, or ref.')), id(new PhabricatorSearchStringListField()) ->setLabel(pht('Ancestors Of')) diff --git a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php index f763b0987f..a1fec4a6d2 100644 --- a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php @@ -181,6 +181,12 @@ final class PhabricatorAuthPasswordEngine $normal_password = phutil_utf8_strtolower($raw_password); if (strlen($normal_password) >= $minimum_similarity) { foreach ($normal_map as $term => $source) { + + // See T2312. This may be required if the term list includes numeric + // strings like "12345", which will be cast to integers when used as + // array keys. + $term = phutil_string_cast($term); + if (strpos($term, $normal_password) === false && strpos($normal_password, $term) === false) { continue; diff --git a/src/applications/config/option/PhabricatorCoreConfigOptions.php b/src/applications/config/option/PhabricatorCoreConfigOptions.php index 10f3e75b62..77c48115a4 100644 --- a/src/applications/config/option/PhabricatorCoreConfigOptions.php +++ b/src/applications/config/option/PhabricatorCoreConfigOptions.php @@ -169,7 +169,21 @@ EOREMARKUP 'Maniphest. If you\'d prefer more traditional UI strings like '. '"Add Comment", you can set this flag to disable most of the '. 'extra flavor.')), - $this->newOption('remarkup.ignored-object-names', 'string', '/^(Q|V)\d$/') + $this->newOption( + 'remarkup.ignored-object-names', + 'string', + + // Q1, Q2, etc., are common abbreviations for "Quarter". + // V1, V2, etc., are common abbreviations for "Version". + // P1, P2, etc., are common abbreviations for "Priority". + + // M1 is a computer chip manufactured by Apple. + // M2 (commonly spelled "M.2") is an expansion slot on motherboards. + // M4 is a carbine. + // M8 is a phonetic spelling of "mate", used in culturally significant + // copypasta about navy seals. + + '/^(Q|V|M|P)\d$/') ->setSummary( pht('Text values that match this regex and are also object names '. 'will not be linked.')) diff --git a/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php index 0b6ae19e32..49a34ac250 100644 --- a/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php @@ -35,22 +35,26 @@ final class DiffusionBrowseQueryConduitAPIMethod protected function getGitResult(ConduitAPIRequest $request) { $drequest = $this->getDiffusionRequest(); $repository = $drequest->getRepository(); + $path = $request->getValue('path'); + if (!strlen($path)) { + $path = null; + } + $commit = $request->getValue('commit'); $offset = (int)$request->getValue('offset'); $limit = (int)$request->getValue('limit'); $result = $this->getEmptyResultSet(); - if ($path == '') { + if ($path === null) { // Fast path to improve the performance of the repository view; we know // the root is always a tree at any commit and always exists. $stdout = 'tree'; } else { try { list($stdout) = $repository->execxLocalCommand( - 'cat-file -t %s:%s', - $commit, - $path); + 'cat-file -t -- %s', + sprintf('%s:%s', $commit, $path)); } catch (CommandException $e) { // The "cat-file" command may fail if the path legitimately does not // exist, but it may also fail if the path is a submodule. This can @@ -62,7 +66,7 @@ final class DiffusionBrowseQueryConduitAPIMethod list($sub_err, $sub_stdout) = $repository->execLocalCommand( 'ls-tree %s -- %s', - $commit, + gitsprintf('%s', $commit), $path); if (!$sub_err) { // If the path failed "cat-file" but "ls-tree" worked, we assume it @@ -86,8 +90,9 @@ final class DiffusionBrowseQueryConduitAPIMethod if (preg_match('/^fatal: Not a valid object name/', $stderr)) { // Grab two logs, since the first one is when the object was deleted. list($stdout) = $repository->execxLocalCommand( - 'log -n2 --format="%%H" %s -- %s', - $commit, + 'log -n2 %s %s -- %s', + '--format=%H', + gitsprintf('%s', $commit), $path); $stdout = trim($stdout); if ($stdout) { @@ -120,14 +125,20 @@ final class DiffusionBrowseQueryConduitAPIMethod return $result; } - list($stdout) = $repository->execxLocalCommand( - 'ls-tree -z -l %s:%s', - $commit, - $path); + if ($path === null) { + list($stdout) = $repository->execxLocalCommand( + 'ls-tree -z -l %s --', + gitsprintf('%s', $commit)); + } else { + list($stdout) = $repository->execxLocalCommand( + 'ls-tree -z -l %s -- %s', + gitsprintf('%s', $commit), + $path); + } $submodules = array(); - if (strlen($path)) { + if ($path !== null) { $prefix = rtrim($path, '/').'/'; } else { $prefix = ''; @@ -207,7 +218,7 @@ final class DiffusionBrowseQueryConduitAPIMethod // the wild. list($err, $contents) = $repository->execLocalCommand( - 'cat-file blob %s:.gitmodules', + 'cat-file blob -- %s:.gitmodules', $commit); if (!$err) { @@ -225,8 +236,8 @@ final class DiffusionBrowseQueryConduitAPIMethod $dict[$key] = $value; } - foreach ($submodules as $path) { - $full_path = $path->getFullPath(); + foreach ($submodules as $submodule_path) { + $full_path = $submodule_path->getFullPath(); $key = 'submodule.'.$full_path.'.url'; if (isset($dict[$key])) { $path->setExternalURI($dict[$key]); diff --git a/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php index 2d4a221171..7a7dbff598 100644 --- a/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionExistsQueryConduitAPIMethod.php @@ -25,7 +25,7 @@ final class DiffusionExistsQueryConduitAPIMethod $repository = $this->getDiffusionRequest()->getRepository(); $commit = $request->getValue('commit'); list($err, $merge_base) = $repository->execLocalCommand( - 'cat-file -t %s', + 'cat-file -t -- %s', $commit); return !$err; } diff --git a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php index ebce21dd6f..8c53ff0555 100644 --- a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php @@ -45,7 +45,12 @@ final class DiffusionHistoryQueryConduitAPIMethod $repository = $drequest->getRepository(); $commit_hash = $request->getValue('commit'); $against_hash = $request->getValue('against'); + $path = $request->getValue('path'); + if (!strlen($path)) { + $path = null; + } + $offset = $request->getValue('offset'); $limit = $request->getValue('limit'); @@ -55,18 +60,27 @@ final class DiffusionHistoryQueryConduitAPIMethod $commit_range = $commit_hash; } + $argv = array(); + + $argv[] = '--skip'; + $argv[] = $offset; + + $argv[] = '--max-count'; + $argv[] = $limit; + + $argv[] = '--format=%H:%P'; + + $argv[] = gitsprintf('%s', $commit_range); + + $argv[] = '--'; + + if ($path !== null) { + $argv[] = $path; + } + list($stdout) = $repository->execxLocalCommand( - 'log '. - '--skip=%d '. - '-n %d '. - '--pretty=format:%s '. - '%s -- %C', - $offset, - $limit, - '%H:%P', - $commit_range, - // Git omits merge commits if the path is provided, even if it is empty. - (strlen($path) ? csprintf('%s', $path) : '')); + 'log %Ls', + $argv); $lines = explode("\n", trim($stdout)); $lines = array_filter($lines); diff --git a/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php index ac241a282f..ea94ff71c4 100644 --- a/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php @@ -43,9 +43,9 @@ final class DiffusionInternalGitRawDiffQueryConduitAPIMethod // it requires the commit to have a parent that we can diff against. The // first commit doesn't, so "commit^" is not a valid ref. list($parents) = $repository->execxLocalCommand( - 'log -n1 --format=%s %s', - '%P', - $commit); + 'log -n1 %s %s --', + '--format=%P', + gitsprintf('%s', $commit)); $use_log = !strlen(trim($parents)); // First, get a fast raw diff without "--find-copies-harder". This flag @@ -96,18 +96,18 @@ final class DiffusionInternalGitRawDiffQueryConduitAPIMethod // NOTE: "--pretty=format: " is to disable diff output, we only want the // part we get from "--raw". $future = $repository->getLocalCommandFuture( - 'log %Ls --pretty=format: %s', + 'log %Ls --pretty=format: %s --', $flags, - $commit); + gitsprintf('%s', $commit)); } else { // Otherwise, we can use "diff", which will give us output for merges. // We diff against the first parent, as this is generally the expectation // and results in sensible behavior. $future = $repository->getLocalCommandFuture( - 'diff %Ls %s^1 %s', + 'diff %Ls %s %s --', $flags, - $commit, - $commit); + gitsprintf('%s^1', $commit), + gitsprintf('%s', $commit)); } // Don't spend more than 30 seconds generating the slower output. diff --git a/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php index 1135420a6e..8d404c656a 100644 --- a/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php @@ -33,8 +33,9 @@ final class DiffusionLastModifiedQueryConduitAPIMethod continue; } list($hash) = $repository->execxLocalCommand( - 'log -n1 --format=%%H %s -- %s', - $commit, + 'log -n1 %s %s -- %s', + '--format=%H', + gitsprintf('%s', $commit), $path); $results[$path] = trim($hash); } diff --git a/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php index 79587a2e5e..b3851802fc 100644 --- a/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php @@ -35,9 +35,9 @@ final class DiffusionMergedCommitsQueryConduitAPIMethod $limit = $this->getLimit($request); list($parents) = $repository->execxLocalCommand( - 'log -n 1 --format=%s %s', - '%P', - $commit); + 'log -n 1 %s %s --', + '--format=%P', + gitsprintf('%s', $commit)); $parents = preg_split('/\s+/', trim($parents)); if (count($parents) < 2) { @@ -50,12 +50,12 @@ final class DiffusionMergedCommitsQueryConduitAPIMethod $first_parent = head($parents); list($logs) = $repository->execxLocalCommand( - 'log -n %d --format=%s %s %s --', + 'log -n %d %s %s %s --', // NOTE: "+ 1" accounts for the merge commit itself. $limit + 1, - '%H', - $commit, - '^'.$first_parent); + '--format=%H', + gitsprintf('%s', $commit), + gitsprintf('%s', '^'.$first_parent)); $hashes = explode("\n", trim($logs)); diff --git a/src/applications/diffusion/conduit/DiffusionQueryPathsConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionQueryPathsConduitAPIMethod.php index 09c07ec28f..98cc006419 100644 --- a/src/applications/diffusion/conduit/DiffusionQueryPathsConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionQueryPathsConduitAPIMethod.php @@ -45,7 +45,7 @@ final class DiffusionQueryPathsConduitAPIMethod $future = $repository->getLocalCommandFuture( 'ls-tree --name-only -r -z %s -- %s', - $commit, + gitsprintf('%s', $commit), $path); $lines = id(new LinesOfALargeExecFuture($future))->setDelimiter("\0"); diff --git a/src/applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php index 00bb28b385..8bdd2d8571 100644 --- a/src/applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php @@ -28,9 +28,9 @@ final class DiffusionRefsQueryConduitAPIMethod $commit = $request->getValue('commit'); list($stdout) = $repository->execxLocalCommand( - 'log --format=%s -n 1 %s --', - '%d', - $commit); + 'log -n 1 %s %s --', + '--format=%d', + gitsprintf('%s', $commit)); // %d, gives a weird output format // similar to (remote/one, remote/two, remote/three) diff --git a/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php index e2c56bb0f8..ba4c824061 100644 --- a/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionSearchQueryConduitAPIMethod.php @@ -64,7 +64,7 @@ final class DiffusionSearchQueryConduitAPIMethod $future = $repository->getLocalCommandFuture( // NOTE: --perl-regexp is available only with libpcre compiled in. 'grep --extended-regexp --null -n --no-color -f - %s -- %s', - $drequest->getStableCommit(), + gitsprintf('%s', $drequest->getStableCommit()), $path); // NOTE: We're writing the pattern on stdin to avoid issues with UTF8 diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php index 60d5c1578d..1a1383368f 100644 --- a/src/applications/diffusion/controller/DiffusionServeController.php +++ b/src/applications/diffusion/controller/DiffusionServeController.php @@ -922,18 +922,26 @@ final class DiffusionServeController extends DiffusionController { // This is a pretty funky fix: it would be nice to more precisely detect // that a request is a `--depth N` clone request, but we don't have any code // to decode protocol frames yet. Instead, look for reasonable evidence - // in the error and output that we're looking at a `--depth` clone. + // in the output that we're looking at a `--depth` clone. - // For evidence this isn't completely crazy, see: - // https://github.com/schacon/grack/pull/7 + // A valid x-git-upload-pack-result response during packfile negotiation + // should end with a flush packet ("0000"). As long as that packet + // terminates the response body in the response, we'll assume the response + // is correct and complete. + + // See https://git-scm.com/docs/pack-protocol#_packfile_negotiation $stdout_regexp = '(^Content-Type: application/x-git-upload-pack-result)m'; - $stderr_regexp = '(The remote end hung up unexpectedly)'; $has_pack = preg_match($stdout_regexp, $stdout); - $is_hangup = preg_match($stderr_regexp, $stderr); - return $has_pack && $is_hangup; + if (strlen($stdout) >= 4) { + $has_flush_packet = (substr($stdout, -4) === "0000"); + } else { + $has_flush_packet = false; + } + + return ($has_pack && $has_flush_packet); } private function getCommonEnvironment(PhabricatorUser $viewer) { diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index 6ebaf6bdc7..0f6b05b578 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -608,9 +608,9 @@ final class DiffusionCommitHookEngine extends Phobject { // repository. Particularly, this will cover the cases of a new branch, a // completely moved tag, etc. $futures[$key] = $this->getRepository()->getLocalCommandFuture( - 'log --format=%s %s --not --all', - '%H', - $ref_update->getRefNew()); + 'log %s %s --not --all --', + '--format=%H', + gitsprintf('%s', $ref_update->getRefNew())); } $content_updates = array(); diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index 2854aecbdd..7b4e80bfec 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -15,7 +15,7 @@ final class DiffusionCommitQuery private $statuses; private $packagePHIDs; private $unreachable; - private $unpublished; + private $permanent; private $needAuditRequests; private $needAuditAuthority; @@ -154,8 +154,8 @@ final class DiffusionCommitQuery return $this; } - public function withUnpublished($unpublished) { - $this->unpublished = $unpublished; + public function withPermanent($permanent) { + $this->permanent = $permanent; return $this; } @@ -859,18 +859,18 @@ final class DiffusionCommitQuery } } - if ($this->unpublished !== null) { - if ($this->unpublished) { - $where[] = qsprintf( - $conn, - '(commit.importStatus & %d) = 0', - PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE); - } else { + if ($this->permanent !== null) { + if ($this->permanent) { $where[] = qsprintf( $conn, '(commit.importStatus & %d) = %d', - PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE, - PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE); + PhabricatorRepositoryCommit::IMPORTED_PERMANENT, + PhabricatorRepositoryCommit::IMPORTED_PERMANENT); + } else { + $where[] = qsprintf( + $conn, + '(commit.importStatus & %d) = 0', + PhabricatorRepositoryCommit::IMPORTED_PERMANENT); } } diff --git a/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php b/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php index 0eb15cd018..3525546111 100644 --- a/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php +++ b/src/applications/diffusion/query/blame/DiffusionGitBlameQuery.php @@ -13,7 +13,7 @@ final class DiffusionGitBlameQuery extends DiffusionBlameQuery { return $repository->getLocalCommandFuture( '--no-pager blame --root -s -l %s -- %s', - $commit, + gitsprintf('%s', $commit), $path); } diff --git a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php index e0fb8bd9ee..f450a8adeb 100644 --- a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php @@ -10,9 +10,8 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery { $commit = $drequest->getCommit(); return $repository->getLocalCommandFuture( - 'cat-file blob %s:%s', - $commit, - $path); + 'cat-file blob -- %s', + sprintf('%s:%s', $commit, $path)); } } diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php index f4c31f7c24..8b9233f128 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php @@ -55,12 +55,15 @@ final class DiffusionLowLevelCommitQuery $split_body = true; } - // Even though we pass --encoding here, git doesn't always succeed, so - // we try a little harder, since git *does* tell us what the actual encoding - // is correctly (unless it doesn't; encoding is sometimes empty). - list($info) = $repository->execxLocalCommand( - 'log -n 1 --encoding=%s --format=%s %s --', - 'UTF-8', + $argv = array(); + + $argv[] = '-n'; + $argv[] = '1'; + + $argv[] = '--encoding=UTF-8'; + + $argv[] = sprintf( + '--format=%s', implode( '%x00', array( @@ -78,8 +81,15 @@ final class DiffusionLowLevelCommitQuery // so include an explicit terminator: this makes sure the exact // body text is surrounded by "\0" characters. '~', - )), - $this->identifier); + ))); + + // Even though we pass --encoding here, git doesn't always succeed, so + // we try a little harder, since git *does* tell us what the actual encoding + // is correctly (unless it doesn't; encoding is sometimes empty). + list($info) = $repository->execxLocalCommand( + 'log -n 1 %Ls %s --', + $argv, + gitsprintf('%s', $this->identifier)); $parts = explode("\0", $info); $encoding = array_shift($parts); diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelFilesizeQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelFilesizeQuery.php index 9d97a134ca..8984003711 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelFilesizeQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelFilesizeQuery.php @@ -33,7 +33,7 @@ final class DiffusionLowLevelFilesizeQuery $paths_future = $repository->getLocalCommandFuture( 'diff-tree -z -r --no-commit-id %s --', - $identifier); + gitsprintf('%s', $identifier)); // With "-z" we get "\0\0" for each line. Process the // delimited text as ", " pairs. diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php index a2673b0ce2..193f45a029 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php @@ -37,9 +37,9 @@ final class DiffusionLowLevelParentsQuery $repository = $this->getRepository(); list($stdout) = $repository->execxLocalCommand( - 'log -n 1 --format=%s %s', - '%P', - $this->identifier); + 'log -n 1 %s %s --', + '--format=%P', + gitsprintf('%s', $this->identifier)); return preg_split('/\s+/', trim($stdout)); } diff --git a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php index 41d91c00ca..9c894437ef 100644 --- a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php +++ b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php @@ -23,9 +23,9 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery { // Check if this is the root commit by seeing if it has parents, since // `git diff X^ X` does not work if "X" is the initial commit. list($parents) = $repository->execxLocalCommand( - 'log -n 1 --format=%s %s --', - '%P', - $commit); + 'log -n 1 %s %s --', + '--format=%P', + gitsprintf('%s', $commit)); if (strlen(trim($parents))) { $against = $commit.'^'; @@ -42,8 +42,8 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery { return $repository->getLocalCommandFuture( 'diff %Ls %s %s -- %s', $options, - $against, - $commit, + gitsprintf('%s', $against), + gitsprintf('%s', $commit), $path); } diff --git a/src/applications/harbormaster/conduit/HarbormasterBuildStepEditAPIMethod.php b/src/applications/harbormaster/conduit/HarbormasterBuildStepEditAPIMethod.php new file mode 100644 index 0000000000..e29f27cb10 --- /dev/null +++ b/src/applications/harbormaster/conduit/HarbormasterBuildStepEditAPIMethod.php @@ -0,0 +1,20 @@ +buildPlan = $build_plan; + return $this; + } + + public function getBuildPlan() { + if ($this->buildPlan === null) { + throw new PhutilInvalidStateException('setBuildPlan'); + } + + return $this->buildPlan; + } + + public function isEngineConfigurable() { + return false; + } + + public function getEngineName() { + return pht('Harbormaster Build Steps'); + } + + public function getSummaryHeader() { + return pht('Edit Harbormaster Build Step Configurations'); + } + + public function getSummaryText() { + return pht('This engine is used to edit Harbormaster build steps.'); + } + + public function getEngineApplicationClass() { + return 'PhabricatorHarbormasterApplication'; + } + + protected function newEditableObject() { + $viewer = $this->getViewer(); + + + $plan = HarbormasterBuildPlan::initializeNewBuildPlan($viewer); + $this->setBuildPlan($plan); + + $plan = $this->getBuildPlan(); + + $step = HarbormasterBuildStep::initializeNewStep($viewer); + + $step->setBuildPlanPHID($plan->getPHID()); + $step->attachBuildPlan($plan); + + return $step; + } + + protected function newObjectQuery() { + return new HarbormasterBuildStepQuery(); + } + + protected function getObjectCreateTitleText($object) { + return pht('Create Build Step'); + } + + protected function getObjectCreateButtonText($object) { + return pht('Create Build Step'); + } + + protected function getObjectEditTitleText($object) { + return pht('Edit Build Step: %s', $object->getName()); + } + + protected function getObjectEditShortText($object) { + return pht('Edit Build Step'); + } + + protected function getObjectCreateShortText() { + return pht('Create Build Step'); + } + + protected function getObjectName() { + return pht('Build Step'); + } + + protected function getEditorURI() { + return '/harbormaster/step/edit/'; + } + + protected function getObjectCreateCancelURI($object) { + return '/harbormaster/step/'; + } + + protected function getObjectViewURI($object) { + $id = $object->getID(); + return "/harbormaster/step/{$id}/"; + } + + protected function buildCustomEditFields($object) { + $fields = array(); + + return $fields; + } + +} diff --git a/src/applications/harbormaster/query/HarbormasterBuildStepSearchEngine.php b/src/applications/harbormaster/query/HarbormasterBuildStepSearchEngine.php new file mode 100644 index 0000000000..b866a1dbc9 --- /dev/null +++ b/src/applications/harbormaster/query/HarbormasterBuildStepSearchEngine.php @@ -0,0 +1,58 @@ +newQuery(); + + return $query; + } + + protected function getURI($path) { + return '/harbormaster/step/'.$path; + } + + protected function getBuiltinQueryNames() { + return array( + 'all' => pht('All Steps'), + ); + } + + public function buildSavedQueryFromBuiltin($query_key) { + $query = $this->newSavedQuery(); + $query->setQueryKey($query_key); + + switch ($query_key) { + case 'all': + return $query; + } + + return parent::buildSavedQueryFromBuiltin($query_key); + } + + protected function renderResultList( + array $plans, + PhabricatorSavedQuery $query, + array $handles) { + assert_instances_of($plans, 'HarbormasterBuildStep'); + return null; + } + +} diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php index dd0ebdc507..71b050adff 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php @@ -4,7 +4,8 @@ final class HarbormasterBuildStep extends HarbormasterDAO implements PhabricatorApplicationTransactionInterface, PhabricatorPolicyInterface, - PhabricatorCustomFieldInterface { + PhabricatorCustomFieldInterface, + PhabricatorConduitResultInterface { protected $name; protected $description; @@ -169,5 +170,45 @@ final class HarbormasterBuildStep extends HarbormasterDAO return $this; } +/* -( PhabricatorConduitResultInterface )---------------------------------- */ + + + public function getFieldSpecificationsForConduit() { + return array( + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('name') + ->setType('string') + ->setDescription(pht('The name of the build step.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('description') + ->setType('remarkup') + ->setDescription(pht('The build step description.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('buildPlanPHID') + ->setType('phid') + ->setDescription( + pht( + 'The PHID of the build plan this build step belongs to.')), + ); + } + + public function getFieldValuesForConduit() { + // T6203: This can be removed once the field becomes non-nullable. + $name = $this->getName(); + $name = phutil_string_cast($name); + + return array( + 'name' => $name, + 'description' => array( + 'raw' => $this->getDescription(), + ), + 'buildPlanPHID' => $this->getBuildPlanPHID(), + ); + } + + public function getConduitSearchAttachments() { + return array(); + } + } diff --git a/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php index a1fad69c0b..903b385ceb 100644 --- a/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php +++ b/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php @@ -64,24 +64,6 @@ final class PhabricatorMetaMTAMailQuery $this->actorPHIDs); } - if ($this->recipientPHIDs !== null) { - $where[] = qsprintf( - $conn, - 'recipient.dst IN (%Ls)', - $this->recipientPHIDs); - } - - if ($this->actorPHIDs === null && $this->recipientPHIDs === null) { - $viewer = $this->getViewer(); - if (!$viewer->isOmnipotent()) { - $where[] = qsprintf( - $conn, - 'edge.dst = %s OR actorPHID = %s', - $viewer->getPHID(), - $viewer->getPHID()); - } - } - if ($this->createdMin !== null) { $where[] = qsprintf( $conn, @@ -102,26 +84,29 @@ final class PhabricatorMetaMTAMailQuery protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { $joins = parent::buildJoinClauseParts($conn); - if ($this->actorPHIDs === null && $this->recipientPHIDs === null) { + if ($this->shouldJoinRecipients()) { $joins[] = qsprintf( $conn, - 'LEFT JOIN %T edge ON mail.phid = edge.src AND edge.type = %d', + 'JOIN %T recipient + ON mail.phid = recipient.src + AND recipient.type = %d + AND recipient.dst IN (%Ls)', PhabricatorEdgeConfig::TABLE_NAME_EDGE, - PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST); - } - - if ($this->recipientPHIDs !== null) { - $joins[] = qsprintf( - $conn, - 'LEFT JOIN %T recipient '. - 'ON mail.phid = recipient.src AND recipient.type = %d', - PhabricatorEdgeConfig::TABLE_NAME_EDGE, - PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST); + PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST, + $this->recipientPHIDs); } return $joins; } + private function shouldJoinRecipients() { + if ($this->recipientPHIDs === null) { + return false; + } + + return true; + } + protected function getPrimaryTableAlias() { return 'mail'; } @@ -134,4 +119,14 @@ final class PhabricatorMetaMTAMailQuery return 'PhabricatorMetaMTAApplication'; } + protected function shouldGroupQueryResultRows() { + if ($this->shouldJoinRecipients()) { + if (count($this->recipientPHIDs) > 1) { + return true; + } + } + + return parent::shouldGroupQueryResultRows(); + } + } diff --git a/src/applications/packages/application/PhabricatorPackagesApplication.php b/src/applications/packages/application/PhabricatorPackagesApplication.php index 9de74dddc0..874a9b5f31 100644 --- a/src/applications/packages/application/PhabricatorPackagesApplication.php +++ b/src/applications/packages/application/PhabricatorPackagesApplication.php @@ -15,7 +15,7 @@ final class PhabricatorPackagesApplication extends PhabricatorApplication { } public function getBaseURI() { - return '/packages/'; + return '/packages/package/'; } public function getIcon() { diff --git a/src/applications/repository/daemon/PhabricatorGitGraphStream.php b/src/applications/repository/daemon/PhabricatorGitGraphStream.php index b175053528..39b1fe39ed 100644 --- a/src/applications/repository/daemon/PhabricatorGitGraphStream.php +++ b/src/applications/repository/daemon/PhabricatorGitGraphStream.php @@ -19,13 +19,13 @@ final class PhabricatorGitGraphStream if ($start_commit !== null) { $future = $repository->getLocalCommandFuture( - 'log --format=%s %s --', - '%H%x01%P%x01%ct', - $start_commit); + 'log %s %s --', + '--format=%H%x01%P%x01%ct', + gitsprintf('%s', $start_commit)); } else { $future = $repository->getLocalCommandFuture( - 'log --format=%s --all --', - '%H%x01%P%x01%ct'); + 'log %s --all --', + '--format=%H%x01%P%x01%ct'); } $this->iterator = new LinesOfALargeExecFuture($future); diff --git a/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php b/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php index 31c859d5d8..148b99f3e0 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php +++ b/src/applications/repository/engine/PhabricatorRepositoryCommitRef.php @@ -5,7 +5,7 @@ final class PhabricatorRepositoryCommitRef extends Phobject { private $identifier; private $epoch; private $branch; - private $canCloseImmediately; + private $isPermanent; private $parents = array(); public function setIdentifier($identifier) { @@ -35,13 +35,13 @@ final class PhabricatorRepositoryCommitRef extends Phobject { return $this->branch; } - public function setCanCloseImmediately($can_close_immediately) { - $this->canCloseImmediately = $can_close_immediately; + public function setIsPermanent($is_permanent) { + $this->isPermanent = $is_permanent; return $this; } - public function getCanCloseImmediately() { - return $this->canCloseImmediately; + public function getIsPermanent() { + return $this->isPermanent; } public function setParents(array $parents) { diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php index c3e4169b53..21d95b227d 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php @@ -101,7 +101,7 @@ final class PhabricatorRepositoryDiscoveryEngine $repository, $ref->getIdentifier(), $ref->getEpoch(), - $ref->getCanCloseImmediately(), + $ref->getIsPermanent(), $ref->getParents(), $task_priority); @@ -189,7 +189,7 @@ final class PhabricatorRepositoryDiscoveryEngine $head_refs = $this->discoverStreamAncestry( new PhabricatorGitGraphStream($repository, $commit), $commit, - $publisher->shouldPublishRef($ref)); + $publisher->isPermanentRef($ref)); $this->didDiscoverRefs($head_refs); @@ -250,7 +250,7 @@ final class PhabricatorRepositoryDiscoveryEngine $refs[$identifier] = id(new PhabricatorRepositoryCommitRef()) ->setIdentifier($identifier) ->setEpoch($epoch) - ->setCanCloseImmediately(true); + ->setIsPermanent(true); if ($upper_bound === null) { $upper_bound = $identifier; @@ -354,7 +354,7 @@ final class PhabricatorRepositoryDiscoveryEngine $branch_refs = $this->discoverStreamAncestry( new PhabricatorMercurialGraphStream($repository, $commit), $commit, - $close_immediately = true); + $is_permanent = true); $this->didDiscoverRefs($branch_refs); @@ -371,7 +371,7 @@ final class PhabricatorRepositoryDiscoveryEngine private function discoverStreamAncestry( PhabricatorRepositoryGraphStream $stream, $commit, - $close_immediately) { + $is_permanent) { $discover = array($commit); $graph = array(); @@ -424,7 +424,7 @@ final class PhabricatorRepositoryDiscoveryEngine $refs[] = id(new PhabricatorRepositoryCommitRef()) ->setIdentifier($commit) ->setEpoch($epoch) - ->setCanCloseImmediately($close_immediately) + ->setIsPermanent($is_permanent) ->setParents($stream->getParents($commit)); } @@ -459,13 +459,6 @@ final class PhabricatorRepositoryDiscoveryEngine return true; } - if ($this->repairMode) { - // In repair mode, rediscover the entire repository, ignoring the - // database state. We can hit the local cache above, but if we miss it - // stop the script from going to the database cache. - return false; - } - $this->fillCommitCache(array($identifier)); return isset($this->commitCache[$identifier]); @@ -476,6 +469,13 @@ final class PhabricatorRepositoryDiscoveryEngine return; } + if ($this->repairMode) { + // In repair mode, rediscover the entire repository, ignoring the + // database state. The engine still maintains a local cache (the + // "Working Set") but we just give up before looking in the database. + return; + } + $max_size = self::MAX_COMMIT_CACHE_SIZE; // If we're filling more identifiers than would fit in the cache, ignore @@ -507,9 +507,9 @@ final class PhabricatorRepositoryDiscoveryEngine } /** - * Sort branches so we process closeable branches first. This makes the - * whole import process a little cheaper, since we can close these commits - * the first time through rather than catching them in the refs step. + * Sort refs so we process permanent refs first. This makes the whole import + * process a little cheaper, since we can publish these commits the first + * time through rather than catching them in the refs step. * * @task internal * @@ -523,7 +523,7 @@ final class PhabricatorRepositoryDiscoveryEngine $head_refs = array(); $tail_refs = array(); foreach ($refs as $ref) { - if ($publisher->shouldPublishRef($ref)) { + if ($publisher->isPermanentRef($ref)) { $head_refs[] = $ref; } else { $tail_refs[] = $ref; @@ -538,7 +538,7 @@ final class PhabricatorRepositoryDiscoveryEngine PhabricatorRepository $repository, $commit_identifier, $epoch, - $close_immediately, + $is_permanent, array $parents, $task_priority) { @@ -570,8 +570,8 @@ final class PhabricatorRepositoryDiscoveryEngine $commit->setRepositoryID($repository->getID()); $commit->setCommitIdentifier($commit_identifier); $commit->setEpoch($epoch); - if ($close_immediately) { - $commit->setImportStatus(PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE); + if ($is_permanent) { + $commit->setImportStatus(PhabricatorRepositoryCommit::IMPORTED_PERMANENT); } $data = new PhabricatorRepositoryCommitData(); @@ -655,7 +655,12 @@ final class PhabricatorRepositoryDiscoveryEngine $epoch, $task_priority) { - $this->insertTask($repository, $commit, $task_priority); + $this->queueCommitImportTask( + $repository, + $commit->getID(), + $commit->getPHID(), + $task_priority, + $via = 'discovery'); // Update the repository summary table. queryfx( @@ -679,36 +684,6 @@ final class PhabricatorRepositoryDiscoveryEngine } } - private function insertTask( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit, - $task_priority, - $data = array()) { - - $vcs = $repository->getVersionControlSystem(); - switch ($vcs) { - case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: - $class = 'PhabricatorRepositoryGitCommitMessageParserWorker'; - break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $class = 'PhabricatorRepositorySvnCommitMessageParserWorker'; - break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: - $class = 'PhabricatorRepositoryMercurialCommitMessageParserWorker'; - break; - default: - throw new Exception(pht("Unknown repository type '%s'!", $vcs)); - } - - $data['commitID'] = $commit->getID(); - - $options = array( - 'priority' => $task_priority, - ); - - PhabricatorWorker::scheduleTask($class, $data, $options); - } - private function isInitialImport(array $refs) { $commit_count = count($refs); @@ -926,71 +901,4 @@ final class PhabricatorRepositoryDiscoveryEngine $data['epoch']); } - private function getImportTaskPriority( - PhabricatorRepository $repository, - array $refs) { - - // If the repository is importing for the first time, we schedule tasks - // at IMPORT priority, which is very low. Making progress on importing a - // new repository for the first time is less important than any other - // daemon task. - - // If the repository has finished importing and we're just catching up - // on recent commits, we usually schedule discovery at COMMIT priority, - // which is slightly below the default priority. - - // Note that followup tasks and triggered tasks (like those generated by - // Herald or Harbormaster) will queue at DEFAULT priority, so that each - // commit tends to fully import before we start the next one. This tends - // to give imports fairly predictable progress. See T11677 for some - // discussion. - - if ($repository->isImporting()) { - $this->log( - pht( - 'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '. - 'because this repository is still importing.', - phutil_count($refs))); - - return PhabricatorWorker::PRIORITY_IMPORT; - } - - // See T13369. If we've discovered a lot of commits at once, import them - // at lower priority. - - // This is mostly aimed at reducing the impact that synchronizing thousands - // of commits from a remote upstream has on other repositories. The queue - // is "mostly FIFO", so queueing a thousand commit imports can stall other - // repositories. - - // In a perfect world we'd probably give repositories round-robin queue - // priority, but we don't currently have the primitives for this and there - // isn't a strong case for building them. - - // Use "a whole lot of commits showed up at once" as a heuristic for - // detecting "someone synchronized an upstream", and import them at a lower - // priority to more closely approximate fair scheduling. - - if (count($refs) >= PhabricatorRepository::LOWPRI_THRESHOLD) { - $this->log( - pht( - 'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '. - 'because many commits were discovered at once.', - phutil_count($refs))); - - return PhabricatorWorker::PRIORITY_IMPORT; - } - - // Otherwise, import at normal priority. - - if ($refs) { - $this->log( - pht( - 'Importing %s commit(s) at normal priority ("PRIORITY_COMMIT").', - phutil_count($refs))); - } - - return PhabricatorWorker::PRIORITY_COMMIT; - } - } diff --git a/src/applications/repository/engine/PhabricatorRepositoryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryEngine.php index 12cd8f1ed6..c1ad203f0b 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryEngine.php @@ -83,4 +83,114 @@ abstract class PhabricatorRepositoryEngine extends Phobject { return $this; } + final protected function queueCommitImportTask( + PhabricatorRepository $repository, + $commit_id, + $commit_phid, + $task_priority, + $via = null) { + + $vcs = $repository->getVersionControlSystem(); + switch ($vcs) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + $class = 'PhabricatorRepositoryGitCommitMessageParserWorker'; + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + $class = 'PhabricatorRepositorySvnCommitMessageParserWorker'; + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: + $class = 'PhabricatorRepositoryMercurialCommitMessageParserWorker'; + break; + default: + throw new Exception( + pht( + 'Unknown repository type "%s"!', + $vcs)); + } + + $data = array( + 'commitID' => $commit_id, + ); + + if ($via !== null) { + $data['via'] = $via; + } + + $options = array( + 'priority' => $task_priority, + 'objectPHID' => $commit_phid, + ); + + PhabricatorWorker::scheduleTask($class, $data, $options); + } + + final protected function getImportTaskPriority( + PhabricatorRepository $repository, + array $refs) { + assert_instances_of($refs, 'PhabricatorRepositoryCommitRef'); + + // If the repository is importing for the first time, we schedule tasks + // at IMPORT priority, which is very low. Making progress on importing a + // new repository for the first time is less important than any other + // daemon task. + + // If the repository has finished importing and we're just catching up + // on recent commits, we usually schedule discovery at COMMIT priority, + // which is slightly below the default priority. + + // Note that followup tasks and triggered tasks (like those generated by + // Herald or Harbormaster) will queue at DEFAULT priority, so that each + // commit tends to fully import before we start the next one. This tends + // to give imports fairly predictable progress. See T11677 for some + // discussion. + + if ($repository->isImporting()) { + $this->log( + pht( + 'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '. + 'because this repository is still importing.', + phutil_count($refs))); + + return PhabricatorWorker::PRIORITY_IMPORT; + } + + // See T13369. If we've discovered a lot of commits at once, import them + // at lower priority. + + // This is mostly aimed at reducing the impact that synchronizing thousands + // of commits from a remote upstream has on other repositories. The queue + // is "mostly FIFO", so queueing a thousand commit imports can stall other + // repositories. + + // In a perfect world we'd probably give repositories round-robin queue + // priority, but we don't currently have the primitives for this and there + // isn't a strong case for building them. + + // Use "a whole lot of commits showed up at once" as a heuristic for + // detecting "someone synchronized an upstream", and import them at a lower + // priority to more closely approximate fair scheduling. + + if (count($refs) >= PhabricatorRepository::LOWPRI_THRESHOLD) { + $this->log( + pht( + 'Importing %s commit(s) at low priority ("PRIORITY_IMPORT") '. + 'because many commits were discovered at once.', + phutil_count($refs))); + + return PhabricatorWorker::PRIORITY_IMPORT; + } + + // Otherwise, import at normal priority. + + if ($refs) { + $this->log( + pht( + 'Importing %s commit(s) at normal priority ("PRIORITY_COMMIT").', + phutil_count($refs))); + } + + return PhabricatorWorker::PRIORITY_COMMIT; + } + + } diff --git a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php index a1b7ae5f55..aafea1f81a 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryRefEngine.php @@ -9,7 +9,7 @@ final class PhabricatorRepositoryRefEngine private $newPositions = array(); private $deadPositions = array(); - private $closeCommits = array(); + private $permanentCommits = array(); private $rebuild; public function setRebuild($rebuild) { @@ -24,7 +24,7 @@ final class PhabricatorRepositoryRefEngine public function updateRefs() { $this->newPositions = array(); $this->deadPositions = array(); - $this->closeCommits = array(); + $this->permanentCommits = array(); $repository = $this->getRepository(); $viewer = $this->getViewer(); @@ -96,11 +96,11 @@ final class PhabricatorRepositoryRefEngine $this->updateCursors($cursor_group, $refs, $type, $all_closing_heads); } - if ($this->closeCommits) { - $this->setCloseFlagOnCommits($this->closeCommits); + if ($this->permanentCommits) { + $this->setPermanentFlagOnCommits($this->permanentCommits); } - $save_cursors = $this->getCursorsForUpdate($all_cursors); + $save_cursors = $this->getCursorsForUpdate($repository, $all_cursors); if ($this->newPositions || $this->deadPositions || $save_cursors) { $repository->openTransaction(); @@ -121,17 +121,19 @@ final class PhabricatorRepositoryRefEngine } } - private function getCursorsForUpdate(array $cursors) { + private function getCursorsForUpdate( + PhabricatorRepository $repository, + array $cursors) { assert_instances_of($cursors, 'PhabricatorRepositoryRefCursor'); + $publisher = $repository->newPublisher(); + $results = array(); foreach ($cursors as $cursor) { - $ref_type = $cursor->getRefType(); - $ref_name = $cursor->getRefName(); - - $is_permanent = $this->isPermanentRef($ref_type, $ref_name); + $diffusion_ref = $cursor->newDiffusionRepositoryRef(); + $is_permanent = $publisher->isPermanentRef($diffusion_ref); if ($is_permanent == $cursor->getIsPermanent()) { continue; } @@ -217,9 +219,9 @@ final class PhabricatorRepositoryRefEngine return $this; } - private function markCloseCommits(array $identifiers) { + private function markPermanentCommits(array $identifiers) { foreach ($identifiers as $identifier) { - $this->closeCommits[$identifier] = $identifier; + $this->permanentCommits[$identifier] = $identifier; } return $this; } @@ -259,6 +261,7 @@ final class PhabricatorRepositoryRefEngine $ref_type, array $all_closing_heads) { $repository = $this->getRepository(); + $publisher = $repository->newPublisher(); // NOTE: Mercurial branches may have multiple branch heads; this logic // is complex primarily to account for that. @@ -341,7 +344,7 @@ final class PhabricatorRepositoryRefEngine $this->markPositionNew($new_position); } - if ($this->isPermanentRef($ref_type, $name)) { + if ($publisher->isPermanentRef(head($refs))) { // See T13284. If this cursor was already marked as permanent, we // only need to publish the newly created ref positions. However, if @@ -377,7 +380,7 @@ final class PhabricatorRepositoryRefEngine $identifier, $exclude); - $this->markCloseCommits($new_identifiers); + $this->markPermanentCommits($new_identifiers); } } } @@ -404,14 +407,6 @@ final class PhabricatorRepositoryRefEngine } } - private function isPermanentRef($ref_type, $ref_name) { - if ($ref_type !== PhabricatorRepositoryRefCursor::TYPE_BRANCH) { - return false; - } - - return $this->getRepository()->isBranchPermanentRef($ref_name); - } - /** * Find all ancestors of a new closing branch head which are not ancestors * of any old closing branch head. @@ -483,17 +478,17 @@ final class PhabricatorRepositoryRefEngine $ref_list = implode("\n", $ref_list)."\n"; $future = $this->getRepository()->getLocalCommandFuture( - 'log --format=%s --stdin', - '%H'); + 'log %s --stdin --', + '--format=%H'); list($stdout) = $future ->write($ref_list) ->resolvex(); } else { list($stdout) = $this->getRepository()->execxLocalCommand( - 'log --format=%s %s', - '%H', - $new_head); + 'log %s %s --', + '--format=%H', + gitsprintf('%s', $new_head)); } $stdout = trim($stdout); @@ -507,10 +502,10 @@ final class PhabricatorRepositoryRefEngine } /** - * Mark a list of commits as closeable, and queue workers for those commits + * Mark a list of commits as permanent, and queue workers for those commits * which don't already have the flag. */ - private function setCloseFlagOnCommits(array $identifiers) { + private function setPermanentFlagOnCommits(array $identifiers) { $repository = $this->getRepository(); $commit_table = new PhabricatorRepositoryCommit(); $conn = $commit_table->establishConnection('w'); @@ -552,7 +547,23 @@ final class PhabricatorRepositoryRefEngine } } - $closeable_flag = PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE; + $commit_refs = array(); + foreach ($identifiers as $identifier) { + + // See T13591. This construction is a bit ad-hoc, but the priority + // function currently only cares about the number of refs we have + // discovered, so we'll get the right result even without filling + // these records out in detail. + + $commit_refs[] = id(new PhabricatorRepositoryCommitRef()) + ->setIdentifier($identifier); + } + + $task_priority = $this->getImportTaskPriority( + $repository, + $commit_refs); + + $permanent_flag = PhabricatorRepositoryCommit::IMPORTED_PERMANENT; $published_flag = PhabricatorRepositoryCommit::IMPORTED_PUBLISH; $all_commits = ipull($all_commits, null, 'commitIdentifier'); @@ -568,9 +579,9 @@ final class PhabricatorRepositoryRefEngine } $import_status = $row['importStatus']; - if (!($import_status & $closeable_flag)) { - // Set the "closeable" flag. - $import_status = ($import_status | $closeable_flag); + if (!($import_status & $permanent_flag)) { + // Set the "permanent" flag. + $import_status = ($import_status | $permanent_flag); // See T13580. Clear the "published" flag, so publishing executes // again. We may have previously performed a no-op "publish" on the @@ -584,17 +595,12 @@ final class PhabricatorRepositoryRefEngine $import_status, $row['id']); - $data = array( - 'commitID' => $row['id'], - ); - - PhabricatorWorker::scheduleTask( - $class, - $data, - array( - 'priority' => PhabricatorWorker::PRIORITY_COMMIT, - 'objectPHID' => $row['phid'], - )); + $this->queueCommitImportTask( + $repository, + $row['id'], + $row['phid'], + $task_priority, + $via = 'ref'); } } @@ -606,13 +612,17 @@ final class PhabricatorRepositoryRefEngine $ref_type, $ref_name) { - $is_permanent = $this->isPermanentRef($ref_type, $ref_name); - $cursor = id(new PhabricatorRepositoryRefCursor()) ->setRepositoryPHID($repository->getPHID()) ->setRefType($ref_type) - ->setRefName($ref_name) - ->setIsPermanent((int)$is_permanent); + ->setRefName($ref_name); + + $publisher = $repository->newPublisher(); + + $diffusion_ref = $cursor->newDiffusionRepositoryRef(); + $is_permanent = $publisher->isPermanentRef($diffusion_ref); + + $cursor->setIsPermanent((int)$is_permanent); try { return $cursor->save(); diff --git a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php index a15cb3d79f..cc94e558e4 100644 --- a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php +++ b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php @@ -4,6 +4,8 @@ final class PhabricatorWorkingCopyDiscoveryTestCase extends PhabricatorWorkingCopyTestCase { public function testSubversionCommitDiscovery() { + $this->requireBinaryForTest('svn'); + $refs = $this->discoverRefs('ST'); $this->assertEqual( array( diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php index c534edf907..eb9437dbf9 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php @@ -7,21 +7,22 @@ final class PhabricatorRepositoryManagementDiscoverWorkflow $this ->setName('discover') ->setExamples('**discover** [__options__] __repository__ ...') - ->setSynopsis(pht('Discover __repository__.')) + ->setSynopsis(pht('Discover commits in __repository__.')) ->setArguments( array( array( - 'name' => 'verbose', - 'help' => pht('Show additional debugging information.'), + 'name' => 'verbose', + 'help' => pht('Show additional debugging information.'), ), array( - 'name' => 'repair', - 'help' => pht( - 'Repair a repository with gaps in commit history.'), + 'name' => 'repair', + 'help' => pht( + 'Discover all commits, even if they are ancestors of known '. + 'commits. This can repair gaps in repository history.'), ), array( - 'name' => 'repos', - 'wildcard' => true, + 'name' => 'repos', + 'wildcard' => true, ), )); } diff --git a/src/applications/repository/query/PhabricatorRepositoryPublisher.php b/src/applications/repository/query/PhabricatorRepositoryPublisher.php index ad374bd034..fae1b07f00 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPublisher.php +++ b/src/applications/repository/query/PhabricatorRepositoryPublisher.php @@ -38,6 +38,10 @@ final class PhabricatorRepositoryPublisher return !$this->getCommitHoldReasons($commit); } + public function isPermanentRef(DiffusionRepositoryRef $ref) { + return !$this->getRefImpermanentReasons($ref); + } + /* -( Hold Reasons )------------------------------------------------------- */ public function getRepositoryHoldReasons() { @@ -59,18 +63,8 @@ final class PhabricatorRepositoryPublisher $repository = $this->getRepository(); $reasons = $this->getRepositoryHoldReasons(); - if (!$ref->isBranch()) { - $reasons[] = self::HOLD_REF_NOT_BRANCH; - } else { - $branch_name = $ref->getShortName(); - - if (!$repository->shouldTrackBranch($branch_name)) { - $reasons[] = self::HOLD_UNTRACKED; - } - - if (!$repository->isBranchPermanentRef($branch_name)) { - $reasons[] = self::HOLD_NOT_PERMANENT_REF; - } + foreach ($this->getRefImpermanentReasons($ref) as $reason) { + $reasons[] = $reason; } return $reasons; @@ -89,4 +83,25 @@ final class PhabricatorRepositoryPublisher return $reasons; } + public function getRefImpermanentReasons(DiffusionRepositoryRef $ref) { + $repository = $this->getRepository(); + $reasons = array(); + + if (!$ref->isBranch()) { + $reasons[] = self::HOLD_REF_NOT_BRANCH; + } else { + $branch_name = $ref->getShortName(); + + if (!$repository->shouldTrackBranch($branch_name)) { + $reasons[] = self::HOLD_UNTRACKED; + } + + if (!$repository->isBranchPermanentRef($branch_name)) { + $reasons[] = self::HOLD_NOT_PERMANENT_REF; + } + } + + return $reasons; + } + } diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 586f1e2d0a..5fb089953d 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -36,7 +36,7 @@ final class PhabricatorRepositoryCommit const IMPORTED_PUBLISH = 8; const IMPORTED_ALL = 11; - const IMPORTED_CLOSEABLE = 1024; + const IMPORTED_PERMANENT = 1024; const IMPORTED_UNREACHABLE = 2048; private $commitData = self::ATTACHABLE; @@ -467,7 +467,7 @@ final class PhabricatorRepositoryCommit } public function isPermanentCommit() { - return (bool)$this->isPartiallyImported(self::IMPORTED_CLOSEABLE); + return (bool)$this->isPartiallyImported(self::IMPORTED_PERMANENT); } public function newCommitAuthorView(PhabricatorUser $viewer) { diff --git a/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php b/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php index 91261f2b93..e661e94295 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php +++ b/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php @@ -88,6 +88,12 @@ final class PhabricatorRepositoryRefCursor return mpull($this->getPositions(), 'getCommitIdentifier'); } + public function newDiffusionRepositoryRef() { + return id(new DiffusionRepositoryRef()) + ->setRefType($this->getRefType()) + ->setShortName($this->getRefName()); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php b/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php index 2f4ee2f4f5..113f3348d5 100644 --- a/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php +++ b/src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php @@ -21,6 +21,10 @@ abstract class PhabricatorWorkerManagementWorkflow 'param' => 'int', 'help' => pht('Limit to tasks with at least this many failures.'), ), + array( + 'name' => 'active', + 'help' => pht('Select all active tasks.'), + ), ); } @@ -28,10 +32,13 @@ abstract class PhabricatorWorkerManagementWorkflow $ids = $args->getArg('id'); $class = $args->getArg('class'); $min_failures = $args->getArg('min-failure-count'); + $active = $args->getArg('active'); - if (!$ids && !$class && !$min_failures) { + if (!$ids && !$class && !$min_failures && !$active) { throw new PhutilArgumentUsageException( - pht('Use --id, --class, or --min-failure-count to select tasks.')); + pht( + 'Use "--id", "--class", "--active", and/or "--min-failure-count" '. + 'to select tasks.')); } $active_query = new PhabricatorWorkerActiveTaskQuery(); @@ -56,7 +63,13 @@ abstract class PhabricatorWorkerManagementWorkflow } $active_tasks = $active_query->execute(); - $archive_tasks = $archive_query->execute(); + + if ($active) { + $archive_tasks = array(); + } else { + $archive_tasks = $archive_query->execute(); + } + $tasks = mpull($active_tasks, null, 'getID') + mpull($archive_tasks, null, 'getID'); diff --git a/src/infrastructure/markup/rule/PhabricatorKeyboardRemarkupRule.php b/src/infrastructure/markup/rule/PhabricatorKeyboardRemarkupRule.php index cdbfea7b74..56bab68e44 100644 --- a/src/infrastructure/markup/rule/PhabricatorKeyboardRemarkupRule.php +++ b/src/infrastructure/markup/rule/PhabricatorKeyboardRemarkupRule.php @@ -61,6 +61,22 @@ final class PhabricatorKeyboardRemarkupRule extends PhutilRemarkupRule { 'escape', ), ), + array( + 'name' => pht('Enter'), + 'symbol' => "\xE2\x8F\x8E", + 'aliases' => array( + 'enter', + 'return', + ), + ), + array( + 'name' => pht('Control'), + 'symbol' => "\xE2\x8C\x83", + 'aliases' => array( + 'ctrl', + 'control', + ), + ), array( 'name' => pht('Up'), 'symbol' => "\xE2\x86\x91", diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php index a770c326f9..c43edaefcb 100644 --- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php @@ -234,7 +234,7 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { // T11773 for some discussion. $this->isOverheated = false; - // See T13386. If we on an old offset-based paging workflow, we need + // See T13386. If we are on an old offset-based paging workflow, we need // to base the overheating limit on both the offset and limit. $overheat_limit = $need * 10; $total_seen = 0; diff --git a/support/startup/PhabricatorStartup.php b/support/startup/PhabricatorStartup.php index 5dac7505d9..870d342464 100644 --- a/support/startup/PhabricatorStartup.php +++ b/support/startup/PhabricatorStartup.php @@ -35,6 +35,7 @@ * @task validation Validation * @task ratelimit Rate Limiting * @task phases Startup Phase Timers + * @task request-path Request Path */ final class PhabricatorStartup { @@ -47,6 +48,7 @@ final class PhabricatorStartup { private static $phases; private static $limits = array(); + private static $requestPath; /* -( Accessing Request Information )-------------------------------------- */ @@ -119,6 +121,7 @@ final class PhabricatorStartup { self::$phases = array(); self::$accessLog = null; + self::$requestPath = null; static $registered; if (!$registered) { @@ -140,7 +143,7 @@ final class PhabricatorStartup { self::normalizeInput(); - self::verifyRewriteRules(); + self::readRequestPath(); self::beginOutputCapture(); } @@ -552,17 +555,29 @@ final class PhabricatorStartup { /** - * @task validation + * @task request-path */ - private static function verifyRewriteRules() { + private static function readRequestPath() { + + // See T13575. The request path may be provided in: + // + // - the "$_GET" parameter "__path__" (normal for Apache and nginx); or + // - the "$_SERVER" parameter "REQUEST_URI" (normal for the PHP builtin + // webserver). + // + // Locate it wherever it is, and store it for later use. Note that writing + // to "$_REQUEST" here won't always work, because later code may rebuild + // "$_REQUEST" from other sources. + if (isset($_REQUEST['__path__']) && strlen($_REQUEST['__path__'])) { + self::setRequestPath($_REQUEST['__path__']); return; } + // Compatibility with PHP 5.4+ built-in web server. if (php_sapi_name() == 'cli-server') { - // Compatibility with PHP 5.4+ built-in web server. - $url = parse_url($_SERVER['REQUEST_URI']); - $_REQUEST['__path__'] = $url['path']; + $path = parse_url($_SERVER['REQUEST_URI']); + self::setRequestPath($path['path']); return; } @@ -580,6 +595,30 @@ final class PhabricatorStartup { } } + /** + * @task request-path + */ + public static function getRequestPath() { + $path = self::$requestPath; + + if ($path === null) { + self::didFatal( + 'Request attempted to access request path, but no request path is '. + 'available for this request. You may be calling web request code '. + 'from a non-request context, or your webserver may not be passing '. + 'a request path to Phabricator in a format that it understands.'); + } + + return $path; + } + + /** + * @task request-path + */ + public static function setRequestPath($path) { + self::$requestPath = $path; + } + /* -( Rate Limiting )------------------------------------------------------ */