diff --git a/src/internationalization/ArcanistUSEnglishTranslation.php b/src/internationalization/ArcanistUSEnglishTranslation.php index 3b2c7141..04140399 100644 --- a/src/internationalization/ArcanistUSEnglishTranslation.php +++ b/src/internationalization/ArcanistUSEnglishTranslation.php @@ -68,6 +68,16 @@ final class ArcanistUSEnglishTranslation extends PhutilTranslation { 'Ignore this untracked file and continue?', 'Ignore these untracked files and continue?', ), + + '%s submodule(s) have uncommitted or untracked changes:' => array( + 'A submodule has uncommitted or untracked changes:', + 'Submodules have uncommitted or untracked changes:', + ), + + 'Ignore the changes to these %s submodule(s) and continue?' => array( + 'Ignore the changes to this submodule and continue?', + 'Ignore the changes to these submodules and continue?', + ), ); } diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php index d4de43b8..29f9ab92 100644 --- a/src/lint/ArcanistLintMessage.php +++ b/src/lint/ArcanistLintMessage.php @@ -90,6 +90,23 @@ final class ArcanistLintMessage extends Phobject { } public function setCode($code) { + $code = (string)$code; + + $maximum_bytes = 128; + $actual_bytes = strlen($code); + + if ($actual_bytes > $maximum_bytes) { + throw new Exception( + pht( + 'Parameter ("%s") passed to "%s" when constructing a lint message '. + 'must be a scalar with a maximum string length of %s bytes, but is '. + '%s bytes in length.', + $code, + 'setCode()', + new PhutilNumber($maximum_bytes), + new PhutilNumber($actual_bytes))); + } + $this->code = $code; return $this; } diff --git a/src/lint/engine/ArcanistLintEngine.php b/src/lint/engine/ArcanistLintEngine.php index 4a092fdb..0bef3e0a 100644 --- a/src/lint/engine/ArcanistLintEngine.php +++ b/src/lint/engine/ArcanistLintEngine.php @@ -217,6 +217,8 @@ abstract class ArcanistLintEngine extends Phobject { foreach ($runnable as $linter) { foreach ($linter->getLintMessages() as $message) { + $this->validateLintMessage($linter, $message); + if (!$this->isSeverityEnabled($message->getSeverity())) { continue; } @@ -598,5 +600,18 @@ abstract class ArcanistLintEngine extends Phobject { $this->endLintServiceCall($call_id); } + private function validateLintMessage( + ArcanistLinter $linter, + ArcanistLintMessage $message) { + + $name = $message->getName(); + if (!strlen($name)) { + throw new Exception( + pht( + 'Linter "%s" generated a lint message that is invalid because it '. + 'does not have a name. Lint messages must have a name.', + get_class($linter))); + } + } } diff --git a/src/lint/linter/ArcanistPhpcsLinter.php b/src/lint/linter/ArcanistPhpcsLinter.php index da3d118c..9556ea0a 100644 --- a/src/lint/linter/ArcanistPhpcsLinter.php +++ b/src/lint/linter/ArcanistPhpcsLinter.php @@ -109,15 +109,17 @@ final class ArcanistPhpcsLinter extends ArcanistExternalLinter { $prefix = 'W'; } - $code = 'PHPCS.'.$prefix.'.'.$child->getAttribute('source'); + $source = $child->getAttribute('source'); + $code = 'PHPCS.'.$prefix.'.'.$source; - $message = new ArcanistLintMessage(); - $message->setPath($path); - $message->setLine($child->getAttribute('line')); - $message->setChar($child->getAttribute('column')); - $message->setCode($code); - $message->setDescription($child->nodeValue); - $message->setSeverity($this->getLintMessageSeverity($code)); + $message = id(new ArcanistLintMessage()) + ->setPath($path) + ->setName($source) + ->setLine($child->getAttribute('line')) + ->setChar($child->getAttribute('column')) + ->setCode($code) + ->setDescription($child->nodeValue) + ->setSeverity($this->getLintMessageSeverity($code)); $messages[] = $message; } diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index f5ceae9c..84ac017f 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -672,7 +672,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { $result = new PhutilArrayWithDefaultValue(); list($stdout) = $uncommitted_future->resolvex(); - $uncommitted_files = $this->parseGitStatus($stdout); + $uncommitted_files = $this->parseGitRawDiff($stdout); foreach ($uncommitted_files as $path => $mask) { $result[$path] |= ($mask | self::FLAG_UNCOMMITTED); } @@ -704,7 +704,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { $this->getDiffBaseOptions(), $this->getBaseCommit()); - return $this->parseGitStatus($stdout); + return $this->parseGitRawDiff($stdout); } public function getGitConfig($key, $default = null) { @@ -759,7 +759,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return $this; } - private function parseGitStatus($status, $full = false) { + private function parseGitRawDiff($status, $full = false) { static $flags = array( 'A' => self::FLAG_ADDED, 'M' => self::FLAG_MODIFIED, @@ -777,17 +777,51 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { $files = array(); foreach ($lines as $line) { $mask = 0; + + // "git diff --raw" lines begin with a ":" character. + $old_mode = ltrim($line[0], ':'); + $new_mode = $line[1]; + + // The hashes may be padded with "." characters for alignment. Discard + // them. + $old_hash = rtrim($line[2], '.'); + $new_hash = rtrim($line[3], '.'); + $flag = $line[4]; $file = $line[5]; - foreach ($flags as $key => $bits) { - if ($flag == $key) { - $mask |= $bits; - } + + $new_value = intval($new_mode, 8); + $is_submodule = (($new_value & 0160000) === 0160000); + + if (($is_submodule) && + ($flag == 'M') && + ($old_hash === $new_hash) && + ($old_mode === $new_mode)) { + // See T9455. We see this submodule as "modified", but the old and new + // hashes are the same and the old and new modes are the same, so we + // don't directly see a modification. + + // We can end up here if we have a submodule which has uncommitted + // changes inside of it (for example, the user has added untracked + // files or made uncommitted changes to files in the submodule). In + // this case, we set a different flag because we can't meaningfully + // give users the same prompt. + + // Note that if the submodule has real changes from the parent + // perspective (the base commit has changed) and also has uncommitted + // changes, we'll only see the real changes and miss the uncommitted + // changes. At the time of writing, there is no reasonable porcelain + // for finding those changes, and the impact of this error seems small. + + $mask |= self::FLAG_EXTERNALS; + } else if (isset($flags[$flag])) { + $mask |= $flags[$flag]; } + if ($full) { $files[$file] = array( 'mask' => $mask, - 'ref' => rtrim($line[3], '.'), + 'ref' => $new_hash, ); } else { $files[$file] = $mask; @@ -807,7 +841,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { list($stdout) = $this->execxLocal( 'diff --raw %s', $since_commit); - return $this->parseGitStatus($stdout); + return $this->parseGitRawDiff($stdout); } public function getBlame($path) { diff --git a/src/repository/api/ArcanistRepositoryAPI.php b/src/repository/api/ArcanistRepositoryAPI.php index 292a7d32..a77a8c90 100644 --- a/src/repository/api/ArcanistRepositoryAPI.php +++ b/src/repository/api/ArcanistRepositoryAPI.php @@ -15,6 +15,9 @@ abstract class ArcanistRepositoryAPI extends Phobject { const FLAG_MISSING = 32; const FLAG_UNSTAGED = 64; const FLAG_UNCOMMITTED = 128; + + // Occurs in SVN when you have uncommitted changes to a modified external, + // or in Git when you have uncommitted or untracked changes in a submodule. const FLAG_EXTERNALS = 256; // Occurs in SVN when you replace a file with a directory without telling @@ -192,6 +195,14 @@ abstract class ArcanistRepositoryAPI extends Phobject { } + /** + * @task status + */ + final public function getDirtyExternalChanges() { + return $this->getUncommittedPathsWithMask(self::FLAG_EXTERNALS); + } + + /** * @task status */ diff --git a/src/workflow/ArcanistFeatureWorkflow.php b/src/workflow/ArcanistFeatureWorkflow.php index 4ebbc7cf..f9695e55 100644 --- a/src/workflow/ArcanistFeatureWorkflow.php +++ b/src/workflow/ArcanistFeatureWorkflow.php @@ -356,8 +356,8 @@ EOTEXT foreach ($out as $line) { $table->addRow(array( 'current' => $line['current'] ? '*' : '', - 'name' => phutil_console_format('**%s**', $line['name']), - 'status' => phutil_console_format( + 'name' => tsprintf('**%s**', $line['name']), + 'status' => tsprintf( "%s", $line['status']), 'descr' => $line['desc'], )); diff --git a/src/workflow/ArcanistListWorkflow.php b/src/workflow/ArcanistListWorkflow.php index 2cc6a48f..5095ef2e 100644 --- a/src/workflow/ArcanistListWorkflow.php +++ b/src/workflow/ArcanistListWorkflow.php @@ -91,11 +91,11 @@ EOTEXT $revision = $revisions[$key]; $table->addRow(array( - 'exists' => $spec['exists'] ? phutil_console_format('**%s**', '*') : '', - 'status' => phutil_console_format( + 'exists' => $spec['exists'] ? tsprintf('**%s**', '*') : '', + 'status' => tsprintf( "%s", $spec['statusName']), - 'title' => phutil_console_format( + 'title' => tsprintf( '**D%d:** %s', $revision['id'], $revision['title']), diff --git a/src/workflow/ArcanistPhrequentWorkflow.php b/src/workflow/ArcanistPhrequentWorkflow.php index fd6202c4..3157c8ee 100644 --- a/src/workflow/ArcanistPhrequentWorkflow.php +++ b/src/workflow/ArcanistPhrequentWorkflow.php @@ -56,7 +56,7 @@ abstract class ArcanistPhrequentWorkflow extends ArcanistWorkflow { $table->addRow(array( 'type' => '('.$column_type.')', - 'time' => phutil_format_relative_time($result['time']), + 'time' => tsprintf($result['time']), 'name' => $phid_map[$result['phid']], )); diff --git a/src/workflow/ArcanistTasksWorkflow.php b/src/workflow/ArcanistTasksWorkflow.php index c0d15c3f..95cc7de4 100644 --- a/src/workflow/ArcanistTasksWorkflow.php +++ b/src/workflow/ArcanistTasksWorkflow.php @@ -111,7 +111,7 @@ EOTEXT // Render the "T123" column. $task_id = 'T'.$task['id']; - $formatted_task_id = phutil_console_format('**%s**', $task_id); + $formatted_task_id = tsprintf('**%s**', $task_id); $output['id'] = $formatted_task_id; // Render the "Title" column. @@ -145,7 +145,7 @@ EOTEXT } else { $color = 'white'; } - $formatted_priority = phutil_console_format( + $formatted_priority = tsprintf( " %s", $task['priority']); $output['priority'] = $formatted_priority; @@ -159,7 +159,7 @@ EOTEXT $status_text = $task['statusName']; $status_color = 'green'; } - $formatted_status = phutil_console_format( + $formatted_status = tsprintf( " %s", $status_text); $output['status'] = $formatted_status; diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php index 047477e5..54649103 100644 --- a/src/workflow/ArcanistWorkflow.php +++ b/src/workflow/ArcanistWorkflow.php @@ -893,11 +893,47 @@ abstract class ArcanistWorkflow extends Phobject { implode("\n ", $missing))); } + $externals = $api->getDirtyExternalChanges(); + + // TODO: This state can exist in Subversion, but it is currently handled + // elsewhere. It should probably be handled here, eventually. + if ($api instanceof ArcanistSubversionAPI) { + $externals = array(); + } + + if ($externals) { + $message = pht( + '%s submodule(s) have uncommitted or untracked changes:', + new PhutilNumber(count($externals))); + + $prompt = pht( + 'Ignore the changes to these %s submodule(s) and continue?', + new PhutilNumber(count($externals))); + + $list = id(new PhutilConsoleList()) + ->setWrap(false) + ->addItems($externals); + + id(new PhutilConsoleBlock()) + ->addParagraph($message) + ->addList($list) + ->draw(); + + $ok = phutil_console_confirm($prompt, $default_no = false); + if (!$ok) { + throw new ArcanistUserAbortException(); + } + } + $uncommitted = $api->getUncommittedChanges(); $unstaged = $api->getUnstagedChanges(); + // We already dealt with externals. + $unstaged = array_diff($unstaged, $externals); + // We only want files which are purely uncommitted. $uncommitted = array_diff($uncommitted, $unstaged); + $uncommitted = array_diff($uncommitted, $externals); $untracked = $api->getUntrackedChanges(); if (!$this->shouldRequireCleanUntrackedFiles()) {