From e51e1c3d44640d60c9843c6f41ee06434cadf697 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Oct 2015 14:35:14 -0700 Subject: [PATCH 1/3] Allow Script-and-Regex linter to have optional/empty capturing patterns for char/line Summary: See discussion in D13737. If you're using this linter to match messages which //sometimes// have a character, you can get `""` (empty string) matches when the expression doesn't match. We'll complain about these later. Instead, cast the matches the expected types. Test Plan: @csilvers confirmed fix, see D13737. Reviewers: chad, csilvers Reviewed By: csilvers Subscribers: spicyj, csilvers Differential Revision: https://secure.phabricator.com/D14307 --- src/lint/linter/ArcanistScriptAndRegexLinter.php | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/lint/linter/ArcanistScriptAndRegexLinter.php b/src/lint/linter/ArcanistScriptAndRegexLinter.php index 70ce944a..ca8883cb 100644 --- a/src/lint/linter/ArcanistScriptAndRegexLinter.php +++ b/src/lint/linter/ArcanistScriptAndRegexLinter.php @@ -324,7 +324,7 @@ final class ArcanistScriptAndRegexLinter extends ArcanistLinter { * Get the line and character of the message from the regex match. * * @param dict Captured groups from regex. - * @return pair Line and character of the message. + * @return pair Line and character of the message. * * @task parse */ @@ -336,8 +336,19 @@ final class ArcanistScriptAndRegexLinter extends ArcanistLinter { return array($line + 1, $char + 1); } - $line = idx($match, 'line', 1); + $line = idx($match, 'line'); + if ($line) { + $line = (int)$line; + } else { + $line = 1; + } + $char = idx($match, 'char'); + if ($char) { + $char = (int)$char; + } else { + $char = null; + } return array($line, $char); } From b3ea439f4d86353466e7007289fb36c38ee0df09 Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Wed, 21 Oct 2015 09:46:20 -0700 Subject: [PATCH 2/3] External linters can now specify a version requirement. Summary: Linters can now use the `version` configuration value to specify the required version of the external binary. The version number may be prefixed with <, <=, >, >=, or = to specify the version comparison operator (default: =). PHP's native `version_compare()` function is used to perform the comparison. Fixes T4954. Test Plan: Tested against a sample of external linters. Reviewers: joshuaspence, epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, joshuaspence Projects: #lint Maniphest Tasks: T4954 Differential Revision: https://secure.phabricator.com/D14298 --- src/lint/linter/ArcanistExternalLinter.php | 92 ++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/src/lint/linter/ArcanistExternalLinter.php b/src/lint/linter/ArcanistExternalLinter.php index c10cbfdc..d25c10a2 100644 --- a/src/lint/linter/ArcanistExternalLinter.php +++ b/src/lint/linter/ArcanistExternalLinter.php @@ -13,6 +13,7 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { private $bin; private $interpreter; private $flags; + private $versionRequirement; /* -( Interpreters, Binaries and Flags )----------------------------------- */ @@ -42,6 +43,16 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { */ abstract public function getInstallInstructions(); + /** + * Return a human-readable string describing how to upgrade the linter. + * + * @return string Human readable upgrade instructions + * @task bin + */ + public function getUpgradeInstructions() { + return null; + } + /** * Return true to continue when the external linter exits with an error code. * By default, linters which exit with an error code are assumed to have @@ -102,6 +113,18 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { return $this; } + /** + * Set the binary's version requirement. + * + * @param string Version requirement. + * @return this + * @task bin + */ + final public function setVersionRequirement($version) { + $this->versionRequirement = trim($version); + return $this; + } + /** * Return the binary or script to execute. This method synthesizes defaults * and configuration. You can override the binary with @{method:setBinary}. @@ -258,6 +281,64 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { } } + /** + * If a binary version requirement has been specified, compare the version + * of the configured binary to the required version, and if the binary's + * version is not supported, throw an exception. + * + * @param string Version string to check. + * @return void + */ + final protected function checkBinaryVersion($version) { + if (!$this->versionRequirement) { + return; + } + + if (!$version) { + $message = pht( + 'Linter %s requires %s version %s. Unable to determine the version '. + 'that you have installed.', + get_class($this), + $this->getBinary(), + $this->versionRequirement); + + $instructions = $this->getUpgradeInstructions(); + if ($instructions) { + $message .= "\n".pht('TO UPGRADE: %s', $instructions); + } + + throw new ArcanistMissingLinterException($message); + } + + $operator = '=='; + $compare_to = $this->versionRequirement; + + $matches = null; + if (preg_match('/^([<>]=?|=)\s*(.*)$/', $compare_to, $matches)) { + $operator = $matches[1]; + $compare_to = $matches[2]; + if ($operator === '=') { + $operator = '=='; + } + } + + if (!version_compare($version, $compare_to, $operator)) { + $message = pht( + 'Linter %s requires %s version %s. You have version %s.', + get_class($this), + $this->getBinary(), + $this->versionRequirement, + $version); + + $instructions = $this->getUpgradeInstructions(); + if ($instructions) { + $message .= "\n".pht('TO UPGRADE: %s', $instructions); + } + + throw new ArcanistMissingLinterException($message); + } + } + /** * Get the composed executable command, including the interpreter and binary * but without flags or paths. This can be used to execute `--version` @@ -308,6 +389,7 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { $version = $this->getVersion(); if ($version) { + $this->checkBinaryVersion($version); return $version.'-'.json_encode($this->getCommandFlags()); } else { // Either we failed to parse the version number or the `getVersion` @@ -395,6 +477,13 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { 'Provide a list of additional flags to pass to the linter on the '. 'command line.'), ), + 'version' => array( + 'type' => 'optional string', + 'help' => pht( + 'Specify a version requirement for the binary. The version number '. + 'may be prefixed with <, <=, >, >=, or = to specify the version '. + 'comparison operator (default: =).'), + ), ); if ($this->shouldUseInterpreter()) { @@ -456,6 +545,9 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { case 'flags': $this->setFlags($value); return; + case 'version': + $this->setVersionRequirement($value); + return; } return parent::setLinterConfigurationValue($key, $value); From dfde57ff812b1cd552c0fd2ca519c7e15171b3a4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Oct 2015 19:55:16 +0000 Subject: [PATCH 3/3] Improve two error handling behaviors in `arc upgrade` Summary: Fixes T9222. Two issues here: - First, we currently continue on error. Throw instead. I just swapped us from "phutil_passthru()" to "execx()" since I don't think printing out the "pulling from remote..." status messages is very important, and this makes it easier to raise a useful exception. - Second, if you have a dirty working copy we currently may try to do some sort of silly stuff which won't work, like prompt you to amend changes. Instead, do a slightly lower-level check and just bail. Test Plan: - Ran `arc upgrade` with a dirty working copy and got a tailored, useful error. - Ran `arc upgrade` with an artificially bad `git pull` command, got a failure with a specific error message. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9222 Differential Revision: https://secure.phabricator.com/D14317 --- src/workflow/ArcanistUpgradeWorkflow.php | 29 +++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/workflow/ArcanistUpgradeWorkflow.php b/src/workflow/ArcanistUpgradeWorkflow.php index c9e0e858..50449ef7 100644 --- a/src/workflow/ArcanistUpgradeWorkflow.php +++ b/src/workflow/ArcanistUpgradeWorkflow.php @@ -53,7 +53,27 @@ EOTEXT $this->setRepositoryAPI($repository); - $this->requireCleanWorkingCopy(); + // NOTE: Don't use requireCleanWorkingCopy() here because it tries to + // amend changes and generally move the workflow forward. We just want to + // abort if there are local changes and make the user sort things out. + $uncommitted = $repository->getUncommittedStatus(); + if ($uncommitted) { + $message = pht( + 'You have uncommitted changes in the working copy for this '. + 'library:'); + + $list = id(new PhutilConsoleList()) + ->setWrap(false) + ->addItems(array_keys($uncommitted)); + + id(new PhutilConsoleBlock()) + ->addParagraph($message) + ->addList($list) + ->draw(); + + throw new ArcanistUsageException( + pht('`arc upgrade` can only upgrade clean working copies.')); + } $branch_name = $repository->getBranchName(); if ($branch_name != 'master' && $branch_name != 'stable') { @@ -71,10 +91,13 @@ EOTEXT } chdir($root); + try { - phutil_passthru('git pull --rebase'); + execx('git pull --rebase'); } catch (Exception $ex) { - phutil_passthru('git rebase --abort'); + // If we failed, try to go back to the old state, then throw the + // original exception. + exec_manual('git rebase --abort'); throw $ex; } }