diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9aeff71e..627de584 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -51,6 +51,7 @@ phutil_register_library_map(array( 'ArcanistBrowseURIHardpointLoader' => 'browse/loader/ArcanistBrowseURIHardpointLoader.php', 'ArcanistBrowseURIRef' => 'browse/ref/ArcanistBrowseURIRef.php', 'ArcanistBrowseWorkflow' => 'browse/workflow/ArcanistBrowseWorkflow.php', + 'ArcanistBuildRef' => 'ref/ArcanistBuildRef.php', 'ArcanistBundle' => 'parser/ArcanistBundle.php', 'ArcanistBundleTestCase' => 'parser/__tests__/ArcanistBundleTestCase.php', 'ArcanistCSSLintLinter' => 'lint/linter/ArcanistCSSLintLinter.php', @@ -497,6 +498,7 @@ phutil_register_library_map(array( 'ArcanistBrowseURIHardpointLoader' => 'ArcanistHardpointLoader', 'ArcanistBrowseURIRef' => 'ArcanistRef', 'ArcanistBrowseWorkflow' => 'ArcanistWorkflow', + 'ArcanistBuildRef' => 'Phobject', 'ArcanistBundle' => 'Phobject', 'ArcanistBundleTestCase' => 'PhutilTestCase', 'ArcanistCSSLintLinter' => 'ArcanistExternalLinter', diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php index 0d98951f..5b99e7c5 100644 --- a/src/lint/renderer/ArcanistConsoleLintRenderer.php +++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php @@ -167,36 +167,35 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { $old_impact--; $new_impact--; - if ($old_impact < 0 || $new_impact < 0) { - throw new Exception( - pht( - 'Modified prefix line range has become negative '. - '(old = %d, new = %d).', - $old_impact, - $new_impact)); + // We can end up here if a patch removes a line which occurs before + // another identical line. + if ($old_impact <= 0 || $new_impact <= 0) { + break; } } while (true); // If the lines at the end of the changed line range are actually the // same, shrink the range. This happens when a patch just removes a // line. - do { - $old_suffix = idx($old_lines, $start + $old_impact - 2, null); - $new_suffix = idx($new_lines, $start + $new_impact - 2, null); + if ($old_impact > 0 && $new_impact > 0) { + do { + $old_suffix = idx($old_lines, $start + $old_impact - 2, null); + $new_suffix = idx($new_lines, $start + $new_impact - 2, null); - if ($old_suffix !== $new_suffix) { - break; - } + if ($old_suffix !== $new_suffix) { + break; + } - $old_impact--; - $new_impact--; + $old_impact--; + $new_impact--; - // We can end up here if a patch removes a line which occurs after - // another identical line. - if ($old_impact <= 0 || $new_impact <= 0) { - break; - } - } while (true); + // We can end up here if a patch removes a line which occurs after + // another identical line. + if ($old_impact <= 0 || $new_impact <= 0) { + break; + } + } while (true); + } } else { diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php index 6a94d594..98149a58 100644 --- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php +++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php @@ -142,6 +142,20 @@ EOTEXT; 'replacement' => "\nX\nY\n", ), + 'rmmulti' => array( + 'line' => 2, + 'char' => 1, + 'original' => "\n", + 'replacement' => '', + ), + + 'rmmulti2' => array( + 'line' => 1, + 'char' => 2, + 'original' => "\n", + 'replacement' => '', + ), + ); $defaults = array( diff --git a/src/lint/renderer/__tests__/data/rmmulti.expect b/src/lint/renderer/__tests__/data/rmmulti.expect new file mode 100644 index 00000000..1e4518cf --- /dev/null +++ b/src/lint/renderer/__tests__/data/rmmulti.expect @@ -0,0 +1,10 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 A + 2 ~ + >>> - 3 ~ + 4 B diff --git a/src/lint/renderer/__tests__/data/rmmulti.txt b/src/lint/renderer/__tests__/data/rmmulti.txt new file mode 100644 index 00000000..82cb3214 --- /dev/null +++ b/src/lint/renderer/__tests__/data/rmmulti.txt @@ -0,0 +1,4 @@ +A + + +B diff --git a/src/lint/renderer/__tests__/data/rmmulti2.expect b/src/lint/renderer/__tests__/data/rmmulti2.expect new file mode 100644 index 00000000..cd49440f --- /dev/null +++ b/src/lint/renderer/__tests__/data/rmmulti2.expect @@ -0,0 +1,10 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 A + >>> - 2 ~ + 3 ~ + 4 B diff --git a/src/lint/renderer/__tests__/data/rmmulti2.txt b/src/lint/renderer/__tests__/data/rmmulti2.txt new file mode 100644 index 00000000..82cb3214 --- /dev/null +++ b/src/lint/renderer/__tests__/data/rmmulti2.txt @@ -0,0 +1,4 @@ +A + + +B diff --git a/src/parser/ArcanistBundle.php b/src/parser/ArcanistBundle.php index 8586e281..7ec8b89d 100644 --- a/src/parser/ArcanistBundle.php +++ b/src/parser/ArcanistBundle.php @@ -424,6 +424,9 @@ final class ArcanistBundle extends Phobject { $cur_target = 'b/'.$cur_path; } + $old_target = $this->encodeGitTargetPath($old_target); + $cur_target = $this->encodeGitTargetPath($cur_target); + $result[] = "diff --git {$old_index} {$cur_index}".$eol; if ($type == ArcanistDiffChangeType::TYPE_ADD) { @@ -591,6 +594,24 @@ final class ArcanistBundle extends Phobject { return $results; } + private function encodeGitTargetPath($path) { + // See T8768. If a target path contains spaces, it must be terminated with + // a tab. If we don't do this, Mercurial has the wrong behavior when + // applying the patch. This results in a semantic trailing whitespace + // character: + // + // +++ b/X Y.txt\t + // + // Everyone is at fault here and there are no winners. + + if (strpos($path, ' ') !== false) { + $path = $path."\t"; + } + + return $path; + } + + private function getOldPath(ArcanistDiffChange $change) { $old_path = $change->getOldPath(); $type = $change->getType(); diff --git a/src/parser/__tests__/ArcanistBundleTestCase.php b/src/parser/__tests__/ArcanistBundleTestCase.php index 8fa78f0a..94b3517f 100644 --- a/src/parser/__tests__/ArcanistBundleTestCase.php +++ b/src/parser/__tests__/ArcanistBundleTestCase.php @@ -33,6 +33,37 @@ final class ArcanistBundleTestCase extends PhutilTestCase { return ArcanistBundle::newFromDiff($diff); } + public function testTabEncoding() { + // See T8768. Test that we add semantic trailing tab literals to diffs + // touching files with spaces in them. This is a pain to encode using the + // support toolset here so just do it manually. + + // Note that the "b/X Y.txt" line has a trailing tab literal. + + $diff = <<getChanges(); + $this->assertEqual(1, count($changes)); + + // The path should parse as "X Y.txt" despite the trailing tab. + $change = head($changes); + $this->assertEqual('X Y.txt', $change->getCurrentPath()); + + // The tab should be restored when the diff is output again. + $this->assertEqual($diff, $bundle->toGitPatch()); + } + /** * Unarchive a saved git repository and apply each commit as though via * "arc patch", verifying that the resulting tree hash is identical to the diff --git a/src/ref/ArcanistBuildRef.php b/src/ref/ArcanistBuildRef.php new file mode 100644 index 00000000..a173e692 --- /dev/null +++ b/src/ref/ArcanistBuildRef.php @@ -0,0 +1,115 @@ +parameters = $data; + return $ref; + } + + private function getStatusMap() { + // The modern "harbormaster.build.search" API method returns this in the + // "fields" list; the older API method returns it at the root level. + if (isset($this->parameters['fields']['buildStatus'])) { + $status = $this->parameters['fields']['buildStatus']; + } else if (isset($this->parameters['buildStatus'])) { + $status = $this->parameters['buildStatus']; + } else { + $status = 'unknown'; + } + + // We may either have an array or a scalar here. The array comes from + // "harbormaster.build.search", or from "harbormaster.querybuilds" if + // the server is newer than August 2016. The scalar comes from older + // versions of that method. See PHI261. + if (is_array($status)) { + $map = $status; + } else { + $map = array( + 'value' => $status, + ); + } + + // If we don't have a name, try to fill one in. + if (!isset($map['name'])) { + $name_map = array( + 'inactive' => pht('Inactive'), + 'pending' => pht('Pending'), + 'building' => pht('Building'), + 'passed' => pht('Passed'), + 'failed' => pht('Failed'), + 'aborted' => pht('Aborted'), + 'error' => pht('Error'), + 'paused' => pht('Paused'), + 'deadlocked' => pht('Deadlocked'), + 'unknown' => pht('Unknown'), + ); + + $map['name'] = idx($name_map, $map['value'], $map['value']); + } + + // If we don't have an ANSI color code, try to fill one in. + if (!isset($map['color.ansi'])) { + $color_map = array( + 'failed' => 'red', + 'passed' => 'green', + ); + + $map['color.ansi'] = idx($color_map, $map['value'], 'yellow'); + } + + return $map; + } + + public function getID() { + return $this->parameters['id']; + } + + public function getName() { + if (isset($this->parameters['fields']['name'])) { + return $this->parameters['fields']['name']; + } + + return $this->parameters['name']; + } + + public function getStatus() { + $map = $this->getStatusMap(); + return $map['value']; + } + + public function getStatusName() { + $map = $this->getStatusMap(); + return $map['name']; + } + + public function getStatusANSIColor() { + $map = $this->getStatusMap(); + return $map['color.ansi']; + } + + public function getObjectName() { + return pht('Build %d', $this->getID()); + } + + public function getStatusSortVector() { + $status = $this->getStatus(); + + // For now, just sort passed builds first. + if ($this->getStatus() == 'passed') { + $status_class = 1; + } else { + $status_class = 2; + } + + return id(new PhutilSortVector()) + ->addInt($status_class) + ->addString($status); + } + + +} diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index e1cb2e1b..cfc92c36 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -52,8 +52,12 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { } public function getGitVersion() { - list($stdout) = $this->execxLocal('--version'); - return rtrim(str_replace('git version ', '', $stdout)); + static $version = null; + if ($version === null) { + list($stdout) = $this->execxLocal('--version'); + $version = rtrim(str_replace('git version ', '', $stdout)); + } + return $version; } public function getMetadataPath() { @@ -645,8 +649,70 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return $this->executeSVNFindRev($hash, 'SVN'); } + private function buildUncommittedStatusViaStatus() { + $status = $this->buildLocalFuture( + array( + 'status --porcelain=2 -z', + )); + list($stdout) = $status->resolvex(); + + $result = new PhutilArrayWithDefaultValue(); + $parts = explode("\0", $stdout); + while (count($parts) > 1) { + $entry = array_shift($parts); + $entry_parts = explode(' ', $entry); + if ($entry_parts[0] == '1') { + $path = $entry_parts[8]; + } else if ($entry_parts[0] == '2') { + $path = $entry_parts[9]; + } else if ($entry_parts[0] == 'u') { + $path = $entry_parts[10]; + } else if ($entry_parts[0] == '?') { + $result[$entry_parts[1]] = self::FLAG_UNTRACKED; + continue; + } + + $result[$path] |= self::FLAG_UNCOMMITTED; + $index_state = substr($entry_parts[1], 0, 1); + $working_state = substr($entry_parts[1], 1, 1); + if ($index_state == 'A') { + $result[$path] |= self::FLAG_ADDED; + } else if ($index_state == 'M') { + $result[$path] |= self::FLAG_MODIFIED; + } else if ($index_state == 'D') { + $result[$path] |= self::FLAG_DELETED; + } + if ($working_state != '.') { + $result[$path] |= self::FLAG_UNSTAGED; + if ($index_state == '.') { + if ($working_state == 'A') { + $result[$path] |= self::FLAG_ADDED; + } else if ($working_state == 'M') { + $result[$path] |= self::FLAG_MODIFIED; + } else if ($working_state == 'D') { + $result[$path] |= self::FLAG_DELETED; + } + } + } + $submodule_tracked = substr($entry_parts[2], 2, 1); + $submodule_untracked = substr($entry_parts[2], 3, 1); + if ($submodule_tracked == 'M' || $submodule_untracked == 'U') { + $result[$path] |= self::FLAG_EXTERNALS; + } + + if ($entry_parts[0] == '2') { + $result[array_shift($parts)] = $result[$path] | self::FLAG_DELETED; + $result[$path] |= self::FLAG_ADDED; + } + } + return $result->toArray(); + } protected function buildUncommittedStatus() { + if (version_compare($this->getGitVersion(), '2.11.0', '>=')) { + return $this->buildUncommittedStatusViaStatus(); + } + $diff_options = $this->getDiffBaseOptions(); if ($this->repositoryHasNoCommits) { @@ -719,7 +785,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { protected function buildCommitRangeStatus() { list($stdout, $stderr) = $this->execxLocal( - 'diff %C --raw %s --', + 'diff %C --raw %s HEAD --', $this->getDiffBaseOptions(), $this->getBaseCommit()); diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php index 301cc88e..73225955 100644 --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -372,41 +372,18 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { } protected function buildCommitRangeStatus() { - // TODO: Possibly we should use "hg status --rev X --rev ." for this - // instead, but we must run "hg diff" later anyway in most cases, so - // building and caching it shouldn't hurt us. + list($stdout) = $this->execxLocal( + 'status --rev %s --rev tip', + $this->getBaseCommit()); - $diff = $this->getFullMercurialDiff(); - if (!$diff) { - return array(); + $results = new PhutilArrayWithDefaultValue(); + + $working_status = ArcanistMercurialParser::parseMercurialStatus($stdout); + foreach ($working_status as $path => $mask) { + $results[$path] |= $mask; } - $parser = new ArcanistDiffParser(); - $changes = $parser->parseDiff($diff); - - $status_map = array(); - foreach ($changes as $change) { - $flags = 0; - switch ($change->getType()) { - case ArcanistDiffChangeType::TYPE_ADD: - case ArcanistDiffChangeType::TYPE_MOVE_HERE: - case ArcanistDiffChangeType::TYPE_COPY_HERE: - $flags |= self::FLAG_ADDED; - break; - case ArcanistDiffChangeType::TYPE_CHANGE: - case ArcanistDiffChangeType::TYPE_COPY_AWAY: // Check for changes? - $flags |= self::FLAG_MODIFIED; - break; - case ArcanistDiffChangeType::TYPE_DELETE: - case ArcanistDiffChangeType::TYPE_MOVE_AWAY: - case ArcanistDiffChangeType::TYPE_MULTICOPY: - $flags |= self::FLAG_DELETED; - break; - } - $status_map[$change->getCurrentPath()] = $flags; - } - - return $status_map; + return $results->toArray(); } protected function didReloadWorkingCopy() { diff --git a/src/repository/api/ArcanistSubversionAPI.php b/src/repository/api/ArcanistSubversionAPI.php index 887fa4f3..d8066e96 100644 --- a/src/repository/api/ArcanistSubversionAPI.php +++ b/src/repository/api/ArcanistSubversionAPI.php @@ -45,9 +45,9 @@ final class ArcanistSubversionAPI extends ArcanistRepositoryAPI { } protected function buildCommitRangeStatus() { - // In SVN, the commit range is always "uncommitted changes", so these - // statuses are equivalent. - return $this->getUncommittedStatus(); + // In SVN, there are never any previous commits in the range -- it is all in + // the uncommitted status. + return array(); } protected function buildUncommittedStatus() { diff --git a/src/repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php b/src/repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php index 689588d7..b6b4c596 100644 --- a/src/repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php +++ b/src/repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php @@ -5,6 +5,8 @@ final class ArcanistRepositoryAPIStateTestCase extends PhutilTestCase { public function testGitStateParsing() { if (Filesystem::binaryExists('git')) { $this->parseState('git_basic.git.tgz'); + $this->parseState('git_submodules_dirty.git.tgz'); + $this->parseState('git_submodules_staged.git.tgz'); } else { $this->assertSkipped(pht('Git is not installed')); } @@ -54,6 +56,16 @@ final class ArcanistRepositoryAPIStateTestCase extends PhutilTestCase { } private function assertCorrectState($test, ArcanistRepositoryAPI $api) { + if ($api instanceof ArcanistGitAPI) { + $version = $api->getGitVersion(); + if (version_compare($version, '2.11.0', '<')) { + // Behavior differs slightly on older versions of git; rather than code + // both variants, skip the tests in the presence of such a git. + $this->assertSkipped(pht('Behavior differs slightly on git < 2.11.0')); + return; + } + } + $f_mod = ArcanistRepositoryAPI::FLAG_MODIFIED; $f_add = ArcanistRepositoryAPI::FLAG_ADDED; $f_del = ArcanistRepositoryAPI::FLAG_DELETED; @@ -68,7 +80,7 @@ final class ArcanistRepositoryAPIStateTestCase extends PhutilTestCase { switch ($test) { case 'svn_basic.svn.tgz': - $expect = array( + $expect_uncommitted = array( 'ADDED' => $f_add, 'COPIED_TO' => $f_add, 'DELETED' => $f_del, @@ -78,8 +90,22 @@ final class ArcanistRepositoryAPIStateTestCase extends PhutilTestCase { 'PROPCHANGE' => $f_mod, 'UNTRACKED' => $f_unt, ); - $this->assertEqual($expect, $api->getUncommittedStatus()); - $this->assertEqual($expect, $api->getCommitRangeStatus()); + $this->assertEqual($expect_uncommitted, $api->getUncommittedStatus()); + + $expect_range = array(); + $this->assertEqual($expect_range, $api->getCommitRangeStatus()); + + $expect_working = array( + 'ADDED' => $f_add, + 'COPIED_TO' => $f_add, + 'DELETED' => $f_del, + 'MODIFIED' => $f_mod, + 'MOVED' => $f_del, + 'MOVED_TO' => $f_add, + 'PROPCHANGE' => $f_mod, + 'UNTRACKED' => $f_unt, + ); + $this->assertEqual($expect_working, $api->getWorkingCopyStatus()); break; case 'git_basic.git.tgz': $expect_uncommitted = array( @@ -94,10 +120,41 @@ final class ArcanistRepositoryAPIStateTestCase extends PhutilTestCase { 'ADDED' => $f_add, 'DELETED' => $f_del, 'MODIFIED' => $f_mod, - 'UNCOMMITTED' => $f_add, 'UNSTAGED' => $f_add, ); $this->assertEqual($expect_range, $api->getCommitRangeStatus()); + + $expect_working = array( + 'ADDED' => $f_add, + 'DELETED' => $f_del, + 'MODIFIED' => $f_mod, + 'UNCOMMITTED' => $f_add | $f_unc, + 'UNSTAGED' => $f_add | $f_mod | $f_uns | $f_unc, + 'UNTRACKED' => $f_unt, + ); + $this->assertEqual($expect_working, $api->getWorkingCopyStatus()); + break; + case 'git_submodules_dirty.git.tgz': + $expect_uncommitted = array( + '.gitmodules' => $f_mod | $f_uns | $f_unc, + 'added/' => $f_unt, + 'deleted' => $f_del | $f_uns | $f_unc, + 'modified-commit' => $f_mod | $f_uns | $f_unc, + 'modified-commit-dirty' => $f_ext | $f_mod | $f_uns | $f_unc, + 'modified-dirty' => $f_ext | $f_mod | $f_uns | $f_unc, + ); + $this->assertEqual($expect_uncommitted, $api->getUncommittedStatus()); + break; + case 'git_submodules_staged.git.tgz': + $expect_uncommitted = array( + '.gitmodules' => $f_mod | $f_unc, + 'added' => $f_add | $f_unc, + 'deleted' => $f_del | $f_unc, + 'modified-commit' => $f_mod | $f_unc, + 'modified-commit-dirty' => $f_ext | $f_mod | $f_uns | $f_unc, + 'modified-dirty' => $f_ext | $f_mod | $f_uns | $f_unc, + ); + $this->assertEqual($expect_uncommitted, $api->getUncommittedStatus()); break; case 'hg_basic.hg.tgz': $expect_uncommitted = array( @@ -113,6 +170,15 @@ final class ArcanistRepositoryAPIStateTestCase extends PhutilTestCase { 'UNCOMMITTED' => $f_add, ); $this->assertEqual($expect_range, $api->getCommitRangeStatus()); + + $expect_working = array( + 'ADDED' => $f_add, + 'DELETED' => $f_del, + 'MODIFIED' => $f_mod, + 'UNCOMMITTED' => $f_add | $f_mod | $f_unc, + 'UNTRACKED' => $f_unt, + ); + $this->assertEqual($expect_working, $api->getWorkingCopyStatus()); break; default: throw new Exception( diff --git a/src/repository/api/__tests__/state/git_submodules_dirty.git.tgz b/src/repository/api/__tests__/state/git_submodules_dirty.git.tgz new file mode 100644 index 00000000..60cf0551 Binary files /dev/null and b/src/repository/api/__tests__/state/git_submodules_dirty.git.tgz differ diff --git a/src/repository/api/__tests__/state/git_submodules_staged.git.tgz b/src/repository/api/__tests__/state/git_submodules_staged.git.tgz new file mode 100644 index 00000000..f90621d7 Binary files /dev/null and b/src/repository/api/__tests__/state/git_submodules_staged.git.tgz differ diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php index b3c2b181..ada0168c 100644 --- a/src/workflow/ArcanistLandWorkflow.php +++ b/src/workflow/ArcanistLandWorkflow.php @@ -694,6 +694,8 @@ EOTEXT $rev_title = $this->revision['title']; $rev_auxiliary = idx($this->revision, 'auxiliary', array()); + $full_name = pht('D%d: %s', $rev_id, $rev_title); + if ($this->revision['authorPHID'] != $this->getUserPHID()) { $other_author = $this->getConduit()->callMethodSynchronous( 'user.query', @@ -706,17 +708,46 @@ EOTEXT "This %s has revision '%s' but you are not the author. Land this ". "revision by %s?", $this->branchType, - "D{$rev_id}: {$rev_title}", + $full_name, $other_author)); if (!$ok) { throw new ArcanistUserAbortException(); } } - if ($rev_status != ArcanistDifferentialRevisionStatus::ACCEPTED) { - $ok = phutil_console_confirm(pht( - "Revision '%s' has not been accepted. Continue anyway?", - "D{$rev_id}: {$rev_title}")); + $state_warning = null; + $state_header = null; + if ($rev_status == ArcanistDifferentialRevisionStatus::CHANGES_PLANNED) { + $state_header = pht('REVISION HAS CHANGES PLANNED'); + $state_warning = pht( + 'The revision you are landing ("%s") is currently in the "%s" state, '. + 'indicating that you expect to revise it before moving forward.'. + "\n\n". + 'Normally, you should resubmit it for review and wait until it is '. + '"%s" by reviewers before you continue.'. + "\n\n". + 'To resubmit the revision for review, either: update the revision '. + 'with revised changes; or use "Request Review" from the web interface.', + $full_name, + pht('Changes Planned'), + pht('Accepted')); + } else if ($rev_status != ArcanistDifferentialRevisionStatus::ACCEPTED) { + $state_header = pht('REVISION HAS NOT BEEN ACCEPTED'); + $state_warning = pht( + 'The revision you are landing ("%s") has not been "%s" by reviewers.', + $full_name, + pht('Accepted')); + } + + if ($state_warning !== null) { + $prompt = pht('Land revision in the wrong state?'); + + id(new PhutilConsoleBlock()) + ->addParagraph(tsprintf('** %s **', $state_header)) + ->addParagraph(tsprintf('%B', $state_warning)) + ->draw(); + + $ok = phutil_console_confirm($prompt); if (!$ok) { throw new ArcanistUserAbortException(); } @@ -1373,13 +1404,12 @@ EOTEXT switch ($buildable['buildableStatus']) { case 'building': $message = pht( - 'Harbormaster is still building the active diff for this revision:'); + 'Harbormaster is still building the active diff for this revision.'); $prompt = pht('Land revision anyway, despite ongoing build?'); break; case 'failed': $message = pht( - 'Harbormaster failed to build the active diff for this revision. '. - 'Build failures:'); + 'Harbormaster failed to build the active diff for this revision.'); $prompt = pht('Land revision anyway, despite build failures?'); break; default: @@ -1387,28 +1417,25 @@ EOTEXT return; } - $builds = $this->getConduit()->callMethodSynchronous( - 'harbormaster.querybuilds', + $builds = $this->queryBuilds( array( 'buildablePHIDs' => array($buildable['phid']), )); $console->writeOut($message."\n\n"); - foreach ($builds['data'] as $build) { - switch ($build['buildStatus']) { - case 'failed': - $color = 'red'; - break; - default: - $color = 'yellow'; - break; - } - $console->writeOut( - " ** %s ** %s: %s\n", - phutil_utf8_strtoupper($build['buildStatusName']), - pht('Build %d', $build['id']), - $build['name']); + $builds = msort($builds, 'getStatusSortVector'); + foreach ($builds as $build) { + $ansi_color = $build->getStatusANSIColor(); + $status_name = $build->getStatusName(); + $object_name = $build->getObjectName(); + $build_name = $build->getName(); + + echo tsprintf( + " ** %s ** %s: %s\n", + $status_name, + $object_name, + $build_name); } $console->writeOut( @@ -1447,4 +1474,34 @@ EOTEXT $mark_workflow->run(); } + private function queryBuilds(array $constraints) { + $conduit = $this->getConduit(); + + // NOTE: This method only loads the 100 most recent builds. It's rare for + // a revision to have more builds than that and there's currently no paging + // wrapper for "*.search" Conduit API calls available in Arcanist. + + try { + $raw_result = $conduit->callMethodSynchronous( + 'harbormaster.build.search', + array( + 'constraints' => $constraints, + )); + } catch (Exception $ex) { + // If the server doesn't have "harbormaster.build.search" yet (Aug 2016), + // try the older "harbormaster.querybuilds" instead. + $raw_result = $conduit->callMethodSynchronous( + 'harbormaster.querybuilds', + $constraints); + } + + $refs = array(); + foreach ($raw_result['data'] as $raw_data) { + $refs[] = ArcanistBuildRef::newFromConduit($raw_data); + } + + return $refs; + } + + } diff --git a/src/workflow/ArcanistLintWorkflow.php b/src/workflow/ArcanistLintWorkflow.php index e8b7f36c..75d0667f 100644 --- a/src/workflow/ArcanistLintWorkflow.php +++ b/src/workflow/ArcanistLintWorkflow.php @@ -331,7 +331,8 @@ EOTEXT pht('Automatically amending HEAD with lint patches.')); $amend = true; } else { - $amend = phutil_console_confirm(pht('Amend HEAD with lint patches?')); + $amend = phutil_console_confirm(pht('Amend HEAD with lint patches?'), + false); } if ($amend) {