Summary: Ref T5141. Currently, `php_compat_info.json` is hardcoded to support PHP 5.2.3. Instead, store as much version information as possible in `php_compat_info.json` and filter accordingly in `ArcanistXHPASTLinter.`
Test Plan: Ran `arc lint` and made sure no additional warnings were raised.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5141
Differential Revision: https://secure.phabricator.com/D9247
Summary:
Currently, when code has a block like:
} catch (Exception $ex) {
...we attempt to mark "$ex" as declared. However, we incorrectly mark every variable used anywhere in the block as declared. This means we'll never raise this warning in a `catch` block.
Instead //only// mark the caught exception as declared.
Test Plan: Added a failing unit test and made it pass.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: lpriestley, epriestley
Differential Revision: https://secure.phabricator.com/D9239
Summary: Handles an issue in windows with large code bases not squash merging correctly, this only catches the issue it doesn't really do any recovery.
Test Plan: We have been running this change in our work environment, it's a hard bug to replicate but when it has reared it's head this has caught it.
Reviewers: waynea, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T4884
Differential Revision: https://secure.phabricator.com/D8729
Summary: Pyflakes doesn't have any message codes, so we can't do anything with severities.
Test Plan: Ran `arc lint` with severities on one of these linters, got an error.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9222
Summary:
Fixes T4952. Several issues:
- You review configuration values with `arc set-config --show`. This makes no sense and never has, I think it just predated `arc get-config` or was easier or something.
- Instead, review values with `arc get-config` and review details with `arc get-config --verbose`.
- Show better and more detailed information about all config sources.
- Establish and show default values from a new "default" source.
- With `--trace` include more information about attempts to read configuration files.
Test Plan:
Ran `arc get-config --trace --verbose` in various working directories and received sensible-looking output.
{F156247}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4952
Differential Revision: https://secure.phabricator.com/D9172
Summary: This string is quite silly!
Test Plan: `arc land --hold` in an affected working copy.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9212
Summary: Ref T2039. Now that the `ArcanistConfigurationDrivenLintEngine` has gained more widespread use, we do no longer expect users to need to write their own lint engine. The documentation has already been updated to reflect this.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9198
Summary: After D9057 and D9064 this class is no longer used.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9197
Summary: The XHPAST double quotes rule autofixes strings which use double quotes where single quotes would suffice. In the case in which the double-quoted string contained an escaped double quote, the autofix string can be improved by removing the escape symbol.
Test Plan: Modified existing unit tests to cover this case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9196
Summary: Fixes T3922. Add a linter which complains if files are executable when they shoudn't be.
Test Plan: Tested by modifying the `.arclint` file of the rARC repository and running `arc lint --everything`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3922
Differential Revision: https://secure.phabricator.com/D9187
Summary: To me, it seems that these methods should not be overridden by subclasses
Test Plan: `arc lint` and `arc unit`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: LegNeato, aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D7960
Summary: To me, it seems that these methods should never be overwritten in subclasses.
Test Plan: `arc lint` and `arc unit`.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: LegNeato, aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D7959
Conflicts:
src/workflow/ArcanistBaseWorkflow.php
Summary: Ref T2039. Convert the `ArcanistScalaSBTLinter` into an `ArcanistExternalLinter` and make it compatible with `.arclint`.
Test Plan: I can't really test this because I don't have any Scala projects and we don't have any test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: codeblock, epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9097
Summary: Previously, the `ArcanistXHPASTLinter` was suggesting that `"\1"` be written as `'\1'`, which is incorrect. Therefore, I ensured that all of the cases listed in the [[http://www.php.net/manual/en/language.types.string.php | PHP documentation]] were handled correctly.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9182
Summary: Ref T4954. There was a typo in the original implementation. It wasn't noticed until now because the `getVersion` function isn't really used anywhere.
Test Plan: Ran `arc linters` in a repository with `ArcanistJSONLintLinter` configured, noticed that the version number appeared in the output.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4954
Differential Revision: https://secure.phabricator.com/D9160
Summary: As noticed by @epriestley in D9060, `flake8 --version` can emit a string like "2.0" rather than the expected "2.0.0".
Test Plan: Uninstalled my existing version of `flake8` (v2.1.0) and installed a prior version (`pip install flake8==2.0.0`). Ran `flake8 --version` and inspected the output.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D9174
Summary: As identified by @epriestley in D9060, `jshint --version` emits version information on stderr, not stdout.
Test Plan: Ran `jshint --version` locally and verified that the version information is output to stderr.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4954
Differential Revision: https://secure.phabricator.com/D9173
Summary: Fixes T5082. We try to access the repository API in some cases when we don't have one.
Test Plan:
- Made revisions with "arc diff --raw-command --create".
- Made this revision, without "--raw-command".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5082
Differential Revision: https://secure.phabricator.com/D9165
Summary:
Add a wrapper around [[http://www.coffeelint.org/ | CoffeeLint]] as an `ArcanistExternalLinter`.
Depends on D9041.
Test Plan: Wrote and executed unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9051
Summary: Fixes T5085. Currently, the `ExecFuture` instances that are used to call an external linter are executed in the current working directory. This means that if a path is specified in the `.arclint` file, relative to the project root directory, that the path will not be properly interpreted by the external linter when `arc lint` is called from a level deeper than the project root.
Test Plan: Ran `arc lint` from a subdirectory of a project and verified that the linter did not throw an exception due to not being able to find the specified configuration file.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5085
Differential Revision: https://secure.phabricator.com/D9159
Summary:
Allow `.jshintrc` and `.jshintignore` paths to be passed to `jshint`.
Ref T2039.
Test Plan: Added the `jshintrc` and `jshintignore` keys to an `.arclint` file that was configured to use `ArcanistJSHintLinter`. Ran `arc lint --trace` and inspected the flags that were passed to `jshint`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: john.snow, epriestley, Korvin
Maniphest Tasks: T2039, T5085
Differential Revision: https://secure.phabricator.com/D9112
Summary: Personally, I am a strong fan of this rule. There is currently a similar rule provided by PHP_CodeSniffer.
Test Plan: Wrote and executed unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9117
Summary: Since this method is only used within this class, it makes sense to move it here.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9114
Summary: Fixes T5071. If you run `arc diff --raw` from some arbitrary, non-repository directory we incorrectly try to read information out of the working copy. Even if this information is available, `arc diff --raw` should not assume it's accurate (there's no guarantee the diff comes from the working copy).
Test Plan: Ran `arc diff --raw` in an arbitrary directory.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5071
Differential Revision: https://secure.phabricator.com/D9142
Summary:
Found https://secure.phabricator.com/D4519 but it was horribly out of date, so decided to write a new version.
Kinda clobbered together form that diff and the new version of ArcanistJSHintLinter
Test Plan: Tested on personal projects with js files.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9131
Summary: This fixes an issue with the C# linter where a message could be returned from cslint that wasn't intended for use with parameters. This just ensures there's enough parameters so that it won't crash (and consequently ignore lint messages).
Test Plan: Ran the linter, it didn't crash.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9116
Summary: Ref T4948. Move the `startDocument` code to a `renderPreamble` function so that, at least theoretically, the renderer can be reused. Otherwise, the only way to reuse the renderer would be to construct a new instance.
Test Plan: Ran `arc lint --output xml` and verified that the output looked reasonable.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4948
Differential Revision: https://secure.phabricator.com/D9108
Summary: Although this provides less context in terms of the error message (for example, `Parameter has invalid type. Expected type 'optional regex|list<regex>', got type 'list<string>'.`), I think that it is the right approach. I think that `PhutilTypeSpec::checkMap` should be improved such that additional context is provided in the exception message.
Test Plan: Ran `arc lint`. Modified `.arclint` to contain an invalid regex and ran `arc lint` again.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9089
Summary: The Checkstyle XML output format is not intended to be an interactive workflow.
Test Plan: Introduced linter issues and ran `arc lint`. Verified that no interactive prompt was shown.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9107
Summary: Fixes T4948. Add a lint renderer which supports outputting the lint results in the Checkstyle XML format.
Test Plan: Ran `arc lint --xml` and inspected the output.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4948
Differential Revision: https://secure.phabricator.com/D9083
Summary:
Just some minor cleaning up, including:
- Removing old annotations for Diviner.
- Renaming some `lint-test` files so that the directory name bears a closer resemblance to that of the linter.
- Remove some useless `return` statements.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9096
Summary: Rename this test method for consistency with other linter test classes.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9095
Summary: Arcanist is a dirty rotten liar. I made it less of a dirty, rotten liar.
Test Plan: http://imgur.com/36qXcgI
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9081
Summary: It looks like this command is just hanging if you skim the documentation and miss that you have to echo parameters into it. Print out a hint.
Test Plan: Ran `arc call-conduit` and got a hint.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9050
Summary: To me, it seems that these methods should never be overwritten in subclasses.
Test Plan: `arc unit`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D7958
Summary: Ref T2039. The type specs are right in theory but not quite correct in practice, since we pass strings in rather than objects.
Test Plan: While tweaking `phabricator/`, adjusted these to get desirable results.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9071
Summary: Ref T2039. It looks like "phutil-library" didn't make it over, I'll add that to the other two (unless I'm wrong and this isn't an oversight?).
Test Plan: Used `arc linters` to read help.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9070
Summary: It seems that there is a lot of overlap between `getConfig` / `setConfig` and `getLinterConfigurationOptions` / `setLinterConfigurationValue` respectively.
Test Plan: `arc lint` and `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9067
Summary: Currently, `PhabricatorLintEngine` configures the `ArcanistPhutilXHPASTLinter` linter. In order to use `.arclint` instead, we need to expose the `ArcanistPhutilXHPASTLinter` configuration to the `.arclint` format.
Test Plan: See D9064.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9066
Summary: Ref T2039. Make the `ArcanistCppcheckLinter` compatible with `.arclint`.
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9068
Summary:
Ref T2039. I'll update the corresponding documentation.
It feels a little awkward that this is disconnected from `getLinterConfigurationOptions()`, but I dislike returning weird ad-hoc structures more than I dislike having two methods. Most linters don't implement either of these anyway.
Test Plan: Ran `arc linters` and `arc linters --verbose`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9062
Summary: Ref T2039. The `.arclint` file is reasonably complete now and we should start using it if possible, since we are trying to recommend it to others.
Test Plan: `arc lint`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9057
Summary:
Currently, the Phutil XHPAST Linter and vanilla XHPAST Linter reuse the same parse tree, but do this by having explicit knowledge of one another.
Instead, let them synchronize by writing to a glorified array of globals on the Engine. They no longer require knowledge of one another, so this can work under `.arclint`.
(This could probably be a little cleaner by putting more logic in the shared base class, but Facebook has some kind of goofy subclass of this thing and //this// patch won't disrupt it, while a cleaner one might.)
This should unblock D9057.
Test Plan: Ran unit tests and normal lint, got accurate looking results without duplicate invocations showing up in `--trace`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9059
Summary: The `ArcanistGeneratedLinter` and `ArcanistNoLintLinter` don't actually ever raise any linter messages, so it doesn't make sense to set custom severities for these linters. Instead, don't expose this configuration.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9038
Summary:
Ref T2039. Addresses two issues:
- Issues a warning for use of config which is deprecated by `.arclint`.
- We no longer require an engine to be present for these linters, so `arc linters` doesn't fatal if they aren't configured.
Test Plan:
- Ran `arc linters` in a repo with no JSHint.
- Ran `arc linters` in a repo with junk in .arcconfig and got a warning when it was read. Verified it still took effect.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9058
Summary: Not all linters run on a `command file` fashion. In particular, the maven checkstyle plugin runs like `command --flag=file`.
Test Plan: Run a linter that extends `ArcanistExternalLinter`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9026
Summary: Ref T2039. This isn't exhaustive, but moves things forward by a decent chunk.
Test Plan: Used `arc linters` and read the messages.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9056
Summary: Ref T2039. We're starting to get kind of a lot of linters; provide `arc linters` to help users review and understand them and construct `.arclint` files.
Test Plan: {F152205}
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9041
Summary: Currently, paths to be excluded from linting need to be specified for each linter individually. This is a pain for projects that are using even a moderate number of linters and which have common paths which should be excluded from linting completely.
Test Plan: Unfortunately, it's hard to test this sort of stuff. I cloned the `arclint-examples` repository and tested my changes there,
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9054
Summary: Explicitly specify the types of the function parameters. This change is basically the same as D8388.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9040
Summary: The only real change here is adding a `getLinterConfigurationName` method so that this linter can be used with an `.arclint` file. Everything else is just some minor tidying.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9039
Summary: This isn't really necessary, nor does it bear any significant benefits, however semantically it seems to make sense. From `csslint --help`, the `--quiet` flag causes `csslint` to "Only output when errors are present".
Test Plan:
Tested using `csslint` directly:
```
> csslint test.css
csslint: No errors in /home/joshua/workspace/github.com/facebook/arcanist/foo.css.
> csslint --quiet test.css
```
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9036
Summary: No subclass should need to override these methods. Additionally, none currently do.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9035
Summary: PHPCS does actually support reading data from stdin, so let's make `ArcanistExternalLinter` aware of this.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9034
Summary: Convert the `cpplint.py` wrapper linter to `ArcanistExternalLinter`. This is in preparation for T2039.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9033
Test Plan:
Verified manually. Something like this in .arclint:
"linters" : {
"text" : {
"type" : "text",
"include" : "(\\.(txt|py|html?)$)",
"text.max-line-length": 200
}
changes the line length. Something other than an integer there raises an error.
Reviewers: epriestley, #blessed_reviewers, #arcanist
Reviewed By: epriestley, #blessed_reviewers, #arcanist
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D9029
Test Plan: Ran `arc lint` locally. Now it doesn't exit with an error.
Reviewers: #blessed_reviewers, #arcanist, epriestley
Reviewed By: #blessed_reviewers, #arcanist, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9028
Summary: Having a `debug` key in the `.arclint` file format doesn't seem right. Instead, it would be better to just use a `PhutilConsole` and the `writeLog` method so that "debug" messages are output when using `arc --trace`.
Test Plan: Ran `arc lint --trace` in a repository using `.arclint`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9021
Summary: The `lessc` binary is a part of the NPM `less` module.
Test Plan: `npm install less` works whereas `npm install lessc` does not.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9006
Summary: Contine is not a word methinks.
Test Plan: looks better
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8997
Summary: This class provides an adapter for [[https://github.com/less/less.js/ | lessc]].
Test Plan: Wrote and executed unit tests.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8992
Summary:
It seems that in some situations, JSHint does not set the `evidence`
property. In such cases, PHP fails with `Undefined property:
stdClass::$evidence`. It would be safer to access the error object as an
associative array rather than as `stdClass`.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8993
Summary: This linter is a wrapper around [[http://puppet-lint.com/ | puppet-lint[]].
Test Plan: Wrote an executed unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8990
Summary: These methods are declared `public`, but there are meant to be `protected` (as they are declared in `ArcanistExternalLinter`).
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8991
Summary: Add a linter which uses [[http://php.net/simplexml | SimpleXML]] to detect errors and potential problems in XML files.
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8989
Summary: Provide bindings for [[https://github.com/zaach/jsonlint | JSONLint]], which is a useful tool for linting and validating JSON. Theoretically, this could be done with pure PHP, however it would not be trivial (`json_decode`, for example, does not provide any context as to JSON validation errors).
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8988
Summary:
This unit test is failing for me. It seems that I have a newer version of `flake8`. We should probably provide support for the most recent versions of any external tools, at least until we can implement version-specific stuff.
I am running `2.1.0 (pep8: 1.5.6, pyflakes: 0.8.1, mccabe: 0.2.1) CPython 2.7.6 on Linux`.
Test Plan: `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8987
Summary:
Ref T3186. Ref T2039.
- Convert JSHint to modern format; improve granularity of errors.
- Convert PyFlakes to modern format;
- Remove ApacheLicenseLinter and LicenseLinter (these have been deprecated for a very long time).
This is somewhat disruptive and will break some users by no longer respecting various path/config options. I'll sequence documentation and deprecation warnings in front of these.
Test Plan: Ran unit tests.
Reviewers: btrahan, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, joshuaspence, aran
Maniphest Tasks: T3186, T2039
Differential Revision: https://secure.phabricator.com/D6810
Summary:
This method will, theoretically, allow `arc lint` to be configured to require some minimum version of an external linter (although this would probably require significantly more work).
Additionally, the existence of this method simplifies the `getCacheVersion` function which, previously, was implemented by the external linters individually. Instead, a general approach to determining the version for cacheing purposes can be used.
Fixes T4954.
Test Plan: I'm not sure how to test this.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4954
Differential Revision: https://secure.phabricator.com/D8971
Summary: Modernize `ArcanistJSHintLinter` by extending from `ArcanistExternalLinter` instead of `ArcanistLinter`.
Test Plan: Wrote and executed unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8965
Summary:
Fixes T4938. Now that we have customs statuses, this workflow needs to know about 'em.
One fun caveat is that Conduit isn't up and running to generate the help method, so I had to add a parameter to have the statuses list out. This could maybe be omitted since entering erroneous status options does indeed tell you the correct options.
Test Plan:
`arc close --list-status` -- got a list of statuses
`arc close T1` -- closed T1
`arc close T1` -- error T1 closed already
`arc close T1 -s foobar` -- T1 status set to foobar
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1, T4938
Differential Revision: https://secure.phabricator.com/D8938
Summary: Like git, it is also valid to have empty net change under hg. Should not throw if that is the case.
Test Plan: Tried on a hg repo.
Reviewers: lifeihuang, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8888
See: <https://github.com/facebook/arcanist/pull/162>
It looks like the word `argument` appears many more times than `augment`, so I'm assuming `agument` is most likely to be a typo of `argument`.
Reviewed by: epriestley
Summary: Personally, I prefer to specify command lines flags as an array rather than a string.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aran, epriestley, Korvin, chad
Differential Revision: https://secure.phabricator.com/D8387
Summary:
Fixes T4809. When landing a revision, check for a (non-manual) buildable of the current diff. If we find one, check its status:
- If it passed, print out a message to inform the user that we checked.
- If it failed or is still building, print out details about the issue and require a confirmation to continue.
- Just ignore other cases.
Test Plan:
- Ran `arc land` on a revision with no buildable, a passing buildable, a failed buildable, and a building buildable for the current diff.
- Got sensible output / prompts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4809
Differential Revision: https://secure.phabricator.com/D8801
Summary: Fixes T4605. Smart waits (see D8782) are presumably good on the balance, but may cause some delays when changes are made to rarely updated repositories. To help mitigate this, have `arc land` hint that a repository has changed.
Test Plan: Will run `arc land`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4605
Differential Revision: https://secure.phabricator.com/D8783
Summary: make it a real member variable. See rARC77a9c1814063 and D8753#33914.
Test Plan: sending up this diff!
Reviewers: shadowhand, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8761
Summary:
Fixes T4163. Re-word the question so its the same as in "arc diff". Draw the line though and don't automagically do anything if the user says "Y", 'cuz amending / adding files last minute in 'arc land' and other workflows is batshit insane. Assuming infinite growth in the future, I think its best to get this language consistent now.
I changed the shouldAmend member variable to a shouldAmend() function with a static inside. Previously shouldAmend was getting set as a side effect and it was kind of weird. I thought maybe it was written this way because the calls to the vcs are a little slow or something. As such, I figured caching it in the static was a good idea? Didn't seem awful but maybe a premature optimization with whatever the performance reality turns out to be.
Also a modest amount of bonus pht.
Test Plan: this very diff. i'm going to arc land it laters too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4163
Differential Revision: https://secure.phabricator.com/D8753
Summary: Fixes T4291. Also pht user-facing strings.
Test Plan:
made a branch `foo` off master. made a commit and a diff in `foo`. switched backed to master and cowboy committed some thing. went back to branch`foo`, did an arc land, and saw the error message. went back to master, did a git resert --hard HEAD^1, went back to branch `foo`, and then successfully arc landed.
also ran arc help land and things looked good
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4291
Differential Revision: https://secure.phabricator.com/D8738
Summary:
I changed the rendering of the bar color for the `priority` column when running `arc tasks` to match the `priorityColor` property.
If the `priorityColor` property is one of the basic colors already supported by ansi, then the bar is set to that color, otherwise it is set to white.
This will allow the user to customize maniphest priorities and then set their own colors and have those colors display correctly when running `arc tasks`
Fixed some linting errors
Test Plan:
Run `arc tasks` and ensure that:
* the priority column is displayed with a colored bar
* the priority column bar is the correct color (or white if it is an unsupported color)
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8735
Summary: I changed the way `arc tasks` was checking to see if a maniphest task was closed from looking for a non-empty status to looking for the isClosed property submitted in D8731.
Test Plan: Run `arc tasks` and check that the tasks show open/closed correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8732
Summary:
We recently moved our HEAD and it caused some issues on `arc patch` with git-svn repos. The base revision is incorrect and patch will fail. Add the check in such case to make it work.
The check was there before but removed in change b202158. The reason wasn't mentioned there though.
Test Plan: Tried it on svn.
Reviewers: lifeihuang, JoelB, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8717
Summary: Ref T4670.
Test Plan: arc patch D8685; arc land --hold; verified i got a nice message asking me to be sure i wanted to land epriestley's code
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4670
Differential Revision: https://secure.phabricator.com/D8709
Summary: Remove any-author flag from 'arc which' make any-author be the default behavior. Annotate revision with the owners username.
Test Plan:
Apply a patch that you don't own (arc patch Dxxx) run 'arc which' verify that:
1. You see the revision
2. You see the original authors username next to the revision (owned by sjobs) etc.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8690
Summary: Of note this now forces the pytest-cov plugin to be installed unless they turn it off.
Test Plan:
```
../arcanist/bin/arc unit
COVERAGE REPORT
97% changes/models/test.py
100% tests/changes/api/serializer/models/test_testcase.py
100% changes/api/serializer/models/testcase.py
UNIT OKAY No unit test failures.
Updated an existing Differential revision:
Revision URI: https://tails.corp.dropbox.com/D43387
Included changes:
M changes/api/serializer/models/testcase.py
M changes/models/test.py
M tests/changes/api/serializer/models/test_testcase.py
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8667
Summary:
Fixes T4596. I misunderstood this issue and D8512 was not correct. Specifically:
- The `hg log` needs to be escaped, since otherwise "arcpatch-x" is interpreted as a revset.
- The `hg update` does not need to be escaped, since updating to a revset doesn't make sense and the command never treats its argument as a revset.
- The `hg bookmark` does not need to be escaped, for similar reasons.
Test Plan:
- Ran these commands in isolation and got sensible, consistent results.
- Ran `arc patch` several times in a row and got proper bookmark names.
Reviewers: btrahan, durham, rvanvelzen
Reviewed By: rvanvelzen
Subscribers: epriestley
Maniphest Tasks: T4596
Differential Revision: https://secure.phabricator.com/D8661
Summary: Fixes T4649. The issue in that task is caused because we're submitting a block of text including comments. We've probably been doing this for a long time, but maybe were more liberal in parsing before. Instead, strip them.
Test Plan: Ran `arc diff --verbatim` and got "Dxxx" prefilled correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4649
Differential Revision: https://secure.phabricator.com/D8658
Summary: Ref T4697. This is similar to the existing stuff, but `svnlook diff ...` can also produce a "Copied" header. Currently, we choke on it in Herald when running pre-commit rules.
Test Plan: Added and ran tests. In the next diff, used this in a real system.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4697
Differential Revision: https://secure.phabricator.com/D8653
Summary:
see https://github.com/facebook/phabricator/issues/546 - arc complete blows up when not
in a workdir.
There's no "is ArcanistWorkingCopyIdentity object valid" method, but getVCSType() looks like the
closest match.
git grep for `getProjectRoot` didn't reveal any more problmatic call sites.
Test Plan: `arc [tab] [tab]`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8578
Summary:
Fixes T4063. The `git format-patch` command produces a special header
and footer which we need to detect, strip, and parse.
Test Plan:
- Added and ran unit tests.
- Submitted a diff with `git format-patch HEAD^ --stdout | arc diff --raw`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4063
Differential Revision: https://secure.phabricator.com/D8547
Summary:
Fixes T4603. We fire `arc close-revision --finalize` implicitly from `arc land`, which may close a corresponding Differential revision.
We want to close if the repository is not present in Phabricator (i.e., we'll never be able to close in response to the commit message, since we'll never see it). Historically, we used Arcanist Project -> "Tracked" to make this determination. Instead, just check if the working copy is associated with a repository. This is simpler, easier, and works better.
Test Plan: Ran `arc close-revision --finalize`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4603
Differential Revision: https://secure.phabricator.com/D8523
Summary:
Ref T4603. This workflow predates `arc land` and doesn't make much sense in modern Phabricator/Arcanist. It is surprising that `arc amend` will sometimes close accepted revisions, and we're better at detecting that repositories are tracked, and tracking repositories is easier.
Also fix some inaccuracies and old claims in the documenation and help.
Test Plan: Ran `arc amend`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4603
Differential Revision: https://secure.phabricator.com/D8522
Summary: Fixes T4596. I couldn't immediately reproduce this, but the `hgsprintf()` version is clearly more correct.
Test Plan: Used `arc patch --trace` to examine hg commands.
Reviewers: btrahan, durham
Reviewed By: durham
Subscribers: aran, epriestley
Maniphest Tasks: T4596
Differential Revision: https://secure.phabricator.com/D8512
Summary:
We recently tried to advance the arcanist HEAD in our release branch but failed, due to an exception in SVN pre-commit hook like this:
abort: Commit blocked by pre-commit hook (exit code 1) with output:
LINT 1.3s FacebookWebJSLintLinter (1 file)
Exception
Some linters failed:
- FacebookWebCopyrightLinter: BadMethodCallException: Call to a member function getConfigFromAnySource() on a non-object
(Run with --trace for a full exception trace.)
However `arc lint` works just fine. By searching the change history, it looks related to a few commits D7271, D7377, D7382, especially D7377, where configuration manager is added to the lint engine. Add it to the SVN precommit hook workflow too.
Test Plan: I am not quite sure how to test it out easily. Any suggestions?
Reviewers: lifeihuang, JoelB, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aran, epriestley, mikemag, Korvin
Differential Revision: https://secure.phabricator.com/D8492
Summary:
pretty straight-forward stuff here. Note that in other events we use "fields" or "specification" rather than "revision"; I think "revision" is best particularly in this context where it is in fact the revision being landed.
Fixes T4565.
Test Plan: php -l
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin
Maniphest Tasks: T4565
Differential Revision: https://secure.phabricator.com/D8484
Summary:
Fixes T4559.
It looks like the code currently (at least partially) handles this by checking for `(no branch)`. I suspect that the behaviour of `git` has changed (I am running version 1.9.0) because I haven't figured out what state to be in to cause `git` to output `(no branch)`.
Test Plan: Ran `arc branch` when on a detached branch.
Reviewers: #blessed_reviewers, epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4559
Differential Revision: https://secure.phabricator.com/D8466
Summary: See discussion in D8460. Primarily, this prints out the failing value when a true/false assertion fails, so if it was something useful (like a function result) it's visible.
Test Plan:
Added `assertTrue("quack")`:
FAIL ArcanistDiffParserTestCase::testParser
Assertion failed, expected 'true' (at ArcanistDiffParserTestCase.php:16).
ACTUAL VALUE
quack
Added `assertFalse("quack")`:
FAIL ArcanistDiffParserTestCase::testParser
Assertion failed, expected 'false' (at ArcanistDiffParserTestCase.php:16).
ACTUAL VALUE
quack
Added `assertEqual("quack", "moo")`:
FAIL ArcanistDiffParserTestCase::testParser
Assertion failed, expected values to be equal (at ArcanistDiffParserTestCase.php:16).
Expected: quack
Actual: moo
Reviewers: joshuaspence
Reviewed By: joshuaspence
CC: aran
Differential Revision: https://secure.phabricator.com/D8465
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
Summary:
I noticed that code coverage wasn't showing in Differential for
some repositories that we are using with Phabricator.
`arc unit` would should unit test coverage, but the paths were messy
(for example, `.//foo/bar.py` instead of `foo/bar.py`). As a result,
the code coverage info wasn't recognised as being for the correct
module.
I'm not sure why this logic is the way that it is... perhaps this is to
do with an older version of `nose` (I am using v1.3.0).
Test Plan:
I created a diff for an internal repository that we have, and
observed that code coverage was displayed in Differential.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, seporaitis, avive, dctrwatson, zeeg
Differential Revision: https://secure.phabricator.com/D8433
Summary:
Fixes T4570. When a test case doesn't make any assertions, fail it:
- A tiny fraction of tests pass by not throwing. These tests can easily make a trivial assertion. It took about 5 minutes to fix them all (D8435, D8436).
- In other cases, no assertions means a test construction problem, as with T4570. In these cases, failing loudly catches a severe error.
- Fixes the no-assertion test cases in `arcanist/`
- Makes the PHP 5.4 test pass for the moment, see discussion in T4334.
Test Plan: Ran `arc unit --everything`.
Reviewers: leebyron, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4570
Differential Revision: https://secure.phabricator.com/D8437
Summary: Ref T4570. Testing a directory with no recognized tests currently passes, but should fail.
Test Plan:
- Ran `arc unit`.
- Removed tests from a test directory, ran `arc unit`, got test failure.
Reviewers: leebyron, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4570
Differential Revision: https://secure.phabricator.com/D8434
Summary: D7271 added this line but the signature of function ArcanistWorkingCopyIdentity::newFromRootAndConfigFile(...) was never changed. I have no idea why it was added here and it's causing failures in facebook. I am just removing it now.
Test Plan: No idea how to test
Reviewers: wez, lifeihuang, JoelB, #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8432
Summary: Ref T988. Not pretty or polished, but works fine.
Test Plan: Generated the docs, made sure they were all there.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D8413
Summary:
Ref T2222. Discussion in T4481. Ref T2543.
Primarily, this will let me fix some of the rough edges that came out of ApplicationTransactions and state-based transitions.
I'm also adding a constant for T2543 while I'm in here.
Test Plan: Used `grep` to look for any weird special behavior, didn't see any.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2543, T2222
Differential Revision: https://secure.phabricator.com/D8400
Summary: Allow `@todo` comments to be linted as TODOs as well as `TODO` comments.
Test Plan: I added a new test case (`todo.lint-test`)
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8389
Summary:
- Tidied up `ArcanistCSSLintLinter::getDefaultBinary`.
- Tidied up `CSSLintLinter::getDefaultFlags` function.
- Tidied up `ArcanistPhpcsLinter::getDefaultBinary` function.
- Tidied up `ArcanistPEP8Linter::getDefaultFlags` function
- Tidied up `ArcanistFlake8Linter::getDefaultFlags`.
- Tidied up `ArcanistCppcheckLinter::getLintOptions`.
- Tidied up `ArcanistCppcheckLinter::getLintPath`.
- Tidied up `ArcanistCpplintLinter::getLintOptions`.
- Tidied up `ArcanistCpplintLinter::getLintPath`.
- Removed child functions which are identical to the corresponding parent functions.
Test Plan: `arc lint` and `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: chad, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8385
Summary: To me, it seems that these comments add no value. Personal opinion I suppose.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: chad, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8382
Summary: See <https://github.com/facebook/arcanist/pull/131>
It looks like the behaviour of csslint has changed since the ArcanistCSSLintLinter was written. Consequently, ArcanistCSSLintLinter does not work with the latest version of csslint (v0.10.0).
- `csslint` has a non-zero exit status
- Fixed `csslint` parsing for v0.10.0
Reviewed by: epriestley
Summary:
Do a little cleanup:
- Remove copyright header (we removed all of these a long time ago, this one just snuck through somehow).
- Remove `@group` comment (obsolete with new Diviner).
- Note support for all VCSes.
- Add pht() for translation.
- Hint `arc browse .`.
- Fail on no paths sooner.
- Raise a useful error if we can't figure out which repository we're heading to.
- Clarify "open" comment.
- Use `Filesystem::binaryExists()`.
- Some minor wordsmithing.
Test Plan: `arc browse`, `arc browse .`, `arc browse README`, `arc browse README src`, ran `arc browse` in valid working copy with no associated repo.
Reviewers: btrahan, spicyj
Reviewed By: spicyj
CC: aran
Differential Revision: https://secure.phabricator.com/D8176
Test Plan: Ran 'arc browse' in a repo that contains a .arcconfig and one without.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8173
Summary:
Especially on Windows it is hard to use "\1" type escapes in shell commands. The direct usage resulted in some undefined variables because the \1 and \2 weren't actually passed as control characters.
By passing them through the regular arguments list they get sent in the "correct way" regardless of OS
Test Plan: Executed `arc diff` in a HG repo and did not get undefined indexes back
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8129
Summary: Fixes T3929. If arc is merging it will say merging * into * vs always saying rebasing * onto *
Test Plan: Make arc do a rebase, then a merge
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3929
Differential Revision: https://secure.phabricator.com/D8085
Summary:
Unlike git, Mercurial considers an `hg commit --amend` which doesn't change the working copy to be an error.
The current behavior for this is fairly bad, since the user gets an exception wrapping the entire command ("Command Failed! ...") and it's pretty verbose and not obvious what has happened.
Some alternatives are:
1. Detect this condition and raise a more tailored exception, like a UsageException.
2. Detect this condition and succeed.
Although I tend to think (1) is the right approach in general (that is, `arc x` should usually behave like `git x` or `hg x`), I went with (2) here because we have a handful of amend callsites and they all assume git semantics (no-op amends are successful), and because I think Mercurial's behavior is a little silly (the working copy ends up in the correct / expected state, which seems fairly clearly like a success to me).
Test Plan:
- Had reporting user verify patch.
- Ran `arc amend --revision Dxxx` twice in a Mercurial working copy.
The old output looked like this:
$ arc amend --revision 922
Amending commit message to reflect revision D922: Improve performance of PhutilSymbolLoader::selectAndLoadSymbols().
Exception
Command failed with error #1!
COMMAND
HGPLAIN=1 hg commit --amend -l '/private/var/folders/8k/c3vkmjy5335gcxdzxkhwq82w0000gn/T/71k8q057844c4g84/3539-gfkvV4'
STDOUT
nothing changed
STDERR
(empty)
(Run with --trace for a full exception trace.)
The new output looks like this:
$ arc amend --revision 922
Amending commit message to reflect revision D922: Improve performance of PhutilSymbolLoader::selectAndLoadSymbols().
Done.
Reviewers: btrahan, richardvanvelzen
Reviewed By: richardvanvelzen
CC: aran
Differential Revision: https://secure.phabricator.com/D8083
Summary:
- The modern name for the config is "project.name".
- Missing parameter in a pht().
- When the value is set, but not valid, we gave you a misleading error message.
Test Plan: Ran `arc which`.
Reviewers: talshiri, btrahan
Reviewed By: talshiri
CC: aran
Differential Revision: https://secure.phabricator.com/D8081
Summary:
Fixes T4349. Two issues:
- As discussed in T4349, we would trim the entire output and then require spaces when matching. This choked incorrectly if the last line of a file contained only whitespace. Use `phutil_split_lines()` instead, and regexp things more reasonably.
- We were capturing the line text, not the commit, as "revision". This isn't actually used elsewhere, but was obviously wrong. Make this consistent with Git/SVN.
Test Plan: Rigged a call up and saw reasonable output after the patch, on a working copy which threw before the patch.
Reviewers: durham, btrahan
Reviewed By: durham
CC: aran
Maniphest Tasks: T4349
Differential Revision: https://secure.phabricator.com/D8078
Summary:
Ref T4343. Continues the process of reducing the prominence of Arcanist Projects. Primarily:
- Query Phabricator to identify the working copy based on explicit configuration, or guess based on heuristics.
- Enhance `arc which` to explain the process to the user.
- The `project_id` key is no longer required in `.arcconfig`.
Minor/cleanup changes:
- Rename `project_id` to `project.name` (consistency, clarity).
- Rename `conduit_uri` to `phabricator.uri` (consistency, clairty).
- These both need documentation updates.
- Add `repository.callsign` to explicitly bind to a repository.
- Updated `.arcconfig` for the new values.
- Fix a unit test which broke a while ago when we fixed a rare definition of "unstaged".
- Make `getRepositoryUUID()` generic so we can get rid of one `instanceof`.
Test Plan:
- Ran `arc which`.
- Ran `arc diff`.
- This doesn't really change anything, so the only real risk is version compatibility breaks. This //does// introduce such a break, but the window is very narrow: if you upgrade `arc` after this commit, and try to diff against a Phabricator which was updated after yesterday (D8068) but before D8072 lands, the lookup will work so we'll add `repositoryPHID` to the `differential.creatediff` call, but it won't exist in Phabricator yet. This window is so narrow that I'm not going to try to fix it, as I'd guess there is a significant chance that no users will be affected. I don't see a clever way to fix it that doesn't involve a lot of work, either.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4343
Differential Revision: https://secure.phabricator.com/D8073
Summary: I'm going to deprecate `user.find`, `user.query` is more modern/powerful and obsoletes it.
Test Plan: Ran `arc tasks --owner epriestley`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8070
FreeBSD 9.2 comes with diff tool version 2.8.7 which behaves
a bit different from how it is expected to. Namely for diff
between two binary files it says:
Files A and B are differ
This was leading to an exception when browsing revisions with
changes in binary files.
Tweaked parse patterns in order to fix this issue. Now both
older and newer diff tools are supported.
See: <https://github.com/facebook/arcanist/pull/139>
Reviewed by: epriestley
Summary:
Some callpaths end up here, which doesn't cause the internal linter's willLintPath() method. This can mean its `activePath` is set wrong, which results in us raising lint for the wrong file.
Particularly, if you apply D7979 (diff 18069) and `arc lint` it, you'll get a syntax error message in the wrong file.
Test Plan: Applied D7979 and linted it, got proper error message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aarwine, aran
Differential Revision: https://secure.phabricator.com/D7988
Summary: This might not be universally desireable, but I found myself writing an additional linter (which I had called `WhitespaceTextLinter`) for the sake of these two linter tests. I figured it may be of use upstream, and so I decided to submit it as a diff. I won't be offended if it is rejected however.
Test Plan: `arc lint` and `arc unit` are both okay with it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7957
Summary: Currently, running `arc unit -- src/` returns with `No tests to run`, even if there are test classes in `src/__tests__/`. This diff changes this behaviour so that `arc unit -- src/` executes unit tests in `src/__tests__/`.
Test Plan: N/A. I suppose you could create a file `src/__tests__/SomeTestCase.php` and see for yourself.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7915
Summary: See <https://github.com/facebook/arcanist/issues/133>. Treat "!" files like "C" and "?" files and make the user deal with them.
Test Plan:
>>> orbital ~/repos/INIS $ svn st
! README
>>> orbital ~/repos/INIS $ arc diff
Usage Exception: You have missing files in this working copy. Revert or formally remove them (with `svn rm`) before proceeding.
Working copy: /INSECURE/repos/INIS/
Missing files in working copy:
README
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7886
Summary:
`arc set-config --show` only show the user config
It would be better to contain local/global/system config
Test Plan: set config by local/global/system/user/project, and check the result of `arc set-config --show`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D7851
Summary:
Before, if PHPUnit crashed (syntax error, undefined constant, etc), you would get a relatively unhelpful error message:
firehed@Eric-Sterns-Mac-Pro ~/dev/php-lcd: arc unit
Exception
Clover coverage XML report file is empty, it probably means that phpunit failed to run tests. Try running arc unit with --trace option and then run generated phpunit command yourself, you might get the answer.
(Run with --trace for a full exception trace.)
This now checks the json and code coverage reports and tries to pull a useful error message:
firehed@Eric-Sterns-Mac-Pro ~/dev/php-lcd: arc unit
Exception
The test '/Users/firehed/dev/php-lcd/tests/PlateButtonsTest.php' crashed with the following output:
Fatal error: Undefined class constant 'EFT' in /Users/firehed/dev/php-lcd/tests/PlateButtonsTest.php on line 25
Call Stack:
0.0002 233104 1. {main}() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/composer/bin/phpunit:0
0.0039 564872 2. PHPUnit_TextUI_Command::main() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/composer/bin/phpunit:63
0.0039 565496 3. PHPUnit_TextUI_Command->run() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:129
0.0247 2280168 4. PHPUnit_TextUI_TestRunner->doRun() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/TextUI/Command.php:176
0.0293 2730760 5. PHPUnit_Framework_TestSuite->run() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/TextUI/TestRunner.php:349
0.1211 3996832 6. PHPUnit_Framework_TestSuite->runTest() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:745
0.1211 3996832 7. PHPUnit_Framework_TestCase->run() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestSuite.php:775
0.1211 3996832 8. PHPUnit_Framework_TestResult->run() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php:783
0.1233 3999752 9. PHPUnit_Framework_TestCase->runBare() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestResult.php:648
0.1236 4016432 10. PHPUnit_Framework_TestCase->runTest() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php:838
0.1237 4017240 11. ReflectionMethod->invokeArgs() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php:983
0.1237 4017520 12. Firehed\PlateButtonsTest->testSingleButtonOnButtonDown() /Users/firehed/dev/php-lcd/vendor/phpunit/phpunit/PHPUnit/Framework/TestCase.php:983
(Run with --trace for a full exception trace.)
Or if nothing was written to `stderr`, you may get something like this:
firehed@Eric-Sterns-Mac-Pro ~/dev/php-lcd: arc unit --no-coverage
Exception
Test Firehed\PlateButtonsTest::testSingleButtonOnButtonDown did not finish
(Run with --trace for a full exception trace.)
Test Plan:
`arc unit` for arcanist itself
`arc unit` before and after the change on a project where:
* all tests pass
* some tests are failing
* a test causes a fatal error (undefined constant, bad method call, etc)
* a test file is invalid (syntax error)
No regressions that I could find, and all crashes now display a more useful error.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: aurelijus, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7848
Summary:
This currently output like this:
file_a:
file_b:
file_c:
Warning on line 29: blah blah
This isn't especially useful and can't be piped to other tools. Instead, emit output like:
file_c:29:Warning: blah blah
This is greppable / pipeable.
Test Plan: Ran `arc lint --output summary`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7788
Summary: Ref T4195. When figuring out changed content in SVN, the easiest approach is to use `svnlook diff`, but it has a slightly different header than we're used to. Adjust the parser for it and add some tests.
Test Plan: Clean unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7790
Summary: A lot of new contributors don't resolve this. Make it easier to resolve, more comprehensive, and more explicit about not being ignorable.
Test Plan:
>>> orbital ~/devtools/arcanist $ arc lint
>>> Lint for src/lint/linter/ArcanistPhutilLibraryLinter.php:
Error (PHL1) Unknown Symbol
Use of unknown class 'BlerpBarp'. Common causes are:
- Your libphutil/ is out of date.
This is the most common cause.
Update this copy of libphutil: /INSECURE/devtools/libphutil
- Some other library is out of date.
Update the library this symbol appears in.
- This symbol is misspelled.
Spell the symbol name correctly.
Symbol name spelling is case-sensitive.
- This symbol was added recently.
Run `arc liberate` on the library it was added to.
- This symbol is external. Use `@phutil-external-symbol`.
Use `grep` to find usage examples of this directive.
*** ALTHOUGH USUALLY EASY TO FIX, THIS IS A SERIOUS ERROR.
*** THIS ERROR IS YOUR FAULT. YOU MUST RESOLVE IT.
181 "*** THIS ERROR IS YOUR FAULT. YOU MUST RESOLVE IT.");
182
183 if (false) {
>>> 184 new BlerpBarp();
185 }
186 }
187 }
>>> orbital ~/devtools/arcanist $
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7752
Summary:
Fixes T1277. The rules we use to figure out the root of the working copy get a bunch of edge cases wrong right now. A particularly troublesome one is when a user has a `/.arcconfig` or `/home/.arcconfig` or similar, which raises a completely useless and confusing error message (T1277).
Rewrite these rules to get all the edge cases correct and do reasonable things in the presence of stray `.arcconfig`. There are a bunch of comments, but basically the algorithm is:
- From the top, go down one directory at a time until we find ".svn", ".git", or ".hg".
- In Subversion, keep going down looking for ".arcconfig". In Git and Mercurial, look for ".arcconfig" only in the same directory.
- Now that we've figured out the VCS root (where the ".vcs" directory is) and the project root (where the ".arcconfig" file is, if it exists), build an identity.
This logic was also spread across three different places. Consolidate it into one and add some logging so we can figure out what's going wrong if users run into trouble.
Test Plan:
- Ran VCS (`arc list`) and non-VCS (`arc help`) commands in Git, Mercurial, and Subversions roots and subdirectories. Also ran them in non-VCS directories. Ran them with and without .arcconfig. All the outputs seemed completely reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1277
Differential Revision: https://secure.phabricator.com/D7686
Summary: This updates the C# tools unit test engine (the derived version of xUnit.NET test engine that supports code coverage) to work after the updates in D7594.
Test Plan:
Switched the test engine from 'XUnitTestEngine' to 'CSharpToolsTestEngine' and ran `arc unit --everything`. The results were identical except that code coverage was provided.
Tested on Linux; will update or comment based on whether it works on Windows as well.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7640
Summary:
The current format of arc feature is not very friendly to parsing. This adds a json output
format.
Test Plan: Run the command and ensure output is valid
Reviewers: lifeihuang, #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: epriestley, aran, Korvin
Differential Revision: https://secure.phabricator.com/D7647
Summary: This just removes the `print "Discovered test .."` code in the xUnit.NET unit test engine. If users need to diagnose what tests are being debugged, it can be found out with `--trace` and observing what build commands are invoked.
Test Plan: Ran `arc unit --everything` and didn't see the message any more.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7639
Summary: This fixes a few issues in the C# unit test engine. It fixes tests sitting in subdirectories not being tested correctly (the location of both the test assembly and the results file would be wrong). It also fixes a very strange issue where xUnit.NET seems to not output the resulting XML file when it executes; in this case we just retry running the test until the XML file appears after completion (and eventually it works).
Test Plan: Ran `arc unit --everything` and `arc unit --everything --no-coverage` and verified that it's all reliably working.
Reviewers: epriestley, #blessed_reviewers, hach-que
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7594
As documented,
$ git ls-files -m
fails to exclude ignored submodules. Replace it with an equivalent:
$ git diff-files --name-only
which does not suffer from this defect.
See: <https://github.com/facebook/arcanist/pull/121>
Reviewed by: epriestley
When a submodule is ignored (ignore=all in .gitconfig),
$ git ls-files -m
fails to exclude the submodule from the listing. Other commands like
$ git diff-index --name-only HEAD
exclude it just fine.
See: <https://github.com/facebook/arcanist/pull/120>
Reviewed by: epriestley
Summary: This sets the limit for future execution in the C# linter to 8. See D7606 for more information.
Test Plan:
Ran
```
arc lint --everything --trace --never-apply-patches --output json
```
and saw it only run 8 commands at once.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7607
Summary:
This upgrades `cslint` to support linting multiple files at a time. As this required a backwards-incompatible to `cslint`, I've added a SUPPORTED_VERSION constant which can be used to detect these kinds of breaking changes in the future (and prompt users to upgrade the `cslint` they have in their repository).
The reason for this upgrade is mainly around running `arc lint --everything`, where there are significant performance benefits gained when bulk linting lots of files per command execution.
Test Plan: Upgraded `cslint` in the Tychaia repository and ran `arc lint --everything --trace`. Saw a substantially less number of executions happening for linting and all of the results came through as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, waynea
Differential Revision: https://secure.phabricator.com/D7599
Summary: Fixed wrong markup which was 1 space instead of 2
Test Plan: I couldn't test it as I don't know how to build documentation
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7587
Summary: Ref T1493. Also consolidate this a bit more.
Test Plan: Unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1493
Differential Revision: https://secure.phabricator.com/D7481
Summary:
CppCheck shows lint messages from included files as well as the current
file. Filter out those, since they don't make much sense in the context
of `arc lint`.
Test Plan:
Before this patch, `arc lint` using `ArcanistCppcheckLinter`. Note that lint messages
from included files appear pointing to a line in the active file.
After this patch, only messages from the active file are included.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7444
Summary: allow specifing an replacement .arcrc file, for the odd cases.
Test Plan: `echo {} | arc call-conduit user.whoami` with and without `--arcrc-file=`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7208
Summary:
Lingers on from D7271; Rename `ArcanistWorkingCopyIdentity.getConfig()`.
Changed all linters (Except one) to use `getConfigFromAnySource()`, because it seems to make sense.
Test Plan: arc unit --everything; arc lint in github.com:epriestley/arclint-examples.git (Except for phpcs, flake8, cpplint and csslint which I don't have installed).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: chad, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7382
Summary:
somewhat related to D7271 and D7377.
Not actually broken right now, but might be worth it for completeness.
Test Plan: arc unit invokes PHPUnit; Can't test Csharp/XUnit on my end.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: aurelijus, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7381
Summary:
Create a new class for them, pass instance around as need.
This looks like it's mostly working, but I'd like to replace the various `new ArcanistConfigurationManager()`
calls with something more suitable.
And maybe get a better name for ArcanistConfigurationManager ("Configuration" is already taken).
Test Plan: arc unit --everything, and then some.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D7271
Summary: Nice title. Ref T479.
Test Plan:
Actually, help on that? I want to make sure I properly build up the "depends" on data. Is it as simple as
-- observe at some commit hash RAZZMATAZZ
-- git checkout -B "foo"
-- <work>
-- git commit -m "stash"
-- arc diff -> yields DX
-- git checkout -B "foo_prime"
-- <work>
-- git commit -m "stash"
-- arc diff -> yield DY
-- git checkout RAZZMATAZZ
-- arc patch DY
-- get prompted in workflow, agree
-- git log and observe DX and DY applied
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, davidressman
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D6790
Summary: Fixes T3920. Added a slash to the path and external name so that "public" and "publicnotexternal" won't appear to be the same root.
Test Plan:
We've had this issue in one of our projects for some time, just ran into it again today. Ran the patched arc against the same directory structure and the troublesome file was added to the diff. Confirmed that files
modified in the "public" (svn external) folder are still caught as external modifications.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3920
Differential Revision: https://secure.phabricator.com/D7226
Summary:
makes jshint slightly more useful by printing out error numbers. I love memorizing numbers.
Sorry about the crappy getLintMessageName(). There was no list of error names, just the long descriptions that are already rendered as 'reason'.
Test Plan: Verify that the numbers are mezmorizing. Been tested on OSX with jshint v2.1.11
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7213
Summary:
See <https://github.com/facebook/phabricator/issues/400>. Since all of `patch`, `git apply` and `hg export` either accept or emit header comments, parse them unconditionally.
This is a tiny bit messy because we already had a less-general parser for `hg export` diffs, which have a large header section.
This is less permissive than GNU `patch`, which allows comments anywhere. We could do that, but `git apply` won't read them and they seem pretty crazy.
Test Plan: Added and ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7207
Summary: Needed for it to be usable from ArcanistConfigurationDrivenLintEngine, which is pretty ok.
Test Plan: put jshint in your .arclint and feel the electricity in the air.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7200
Summary:
Completes T3859. This implements a C# linter for Arcanist, which in turn uses `cslint` from `cstools` to actually perform the linting. `cslint` internally uses StyleCop in addition to it's own lint rules.
Unlike other linters, C# is a compiled language, which means that the StyleCop integration must be aware of the full project. To this end, there is the `discovery` setting in `.arclint`. This allows users to define mappings between C# files and the projects they belong to. Here is an configuration for `.arclint` (and is the one we use):
```
{
"linters": {
"csharp": {
"type": "csharp",
"include": "(\\.cs$)",
"binary": "cstools/cslint/bin/Debug/cslint.exe",
"discovery": {
"([^/]+)/(.*?)\\.cs": [
"$1/$1.Linux.csproj"
],
"([^\\\\]+)\\\\(.*?)\\.cs": [
"$1\\$1.Windows.csproj"
]
}
}
}
}
```
Test Plan: Tested under both Linux and Windows. Changed some files, ran `arc lint` and it all worked correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, jamesr
Maniphest Tasks: T3859
Differential Revision: https://secure.phabricator.com/D7170
Summary:
In case that the $source_path is `.` or empty, it produces filenames that
look like `./foo.py`, which differential doesn't like.
Test Plan: arc diff, see coverage data.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7181
Summary:
Many test frameworks can format their output in xUnit-like format.
Test Plan: Tested the Nose engine with a Nose one, and the pytest with a demo project.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7011
Conflicts:
src/__phutil_library_map__.php
Summary: See error message in D7170 -- this should be a `.`, not a `,`.
Test Plan:
Faked interprerter and got reasonable error message:
> Unable to locate interpreter "TESTpython2.6" to run linter ArcanistPEP8Linter. You may need to install the intepreter, or adjust your linter configuration.
> TO INSTALL: Install PEP8 using `easy_install pep8`.
This doesn't fix the //real// error, which is that the test should skip if you don't have the interpreter/binary, but that's a little more involved.
Reviewers: hach-que, chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7172
Summary:
If in a subdirectory, any changes made in a different location
is missed from the patch with no errors from git. The other
option was to run some logic to compare the files being changed.
Test Plan:
Run arc patch from a subdirectory that would miss some files
previously.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3733
Differential Revision: https://secure.phabricator.com/D7167
Summary:
The existing lint configurations do not allow for linting only the lines that have
changed when paths are added. --lintall has a default behavior of true when paths are specified,
and false when paths are not specified. Because of this (and because it does not take a boolean
param) it is not possible to lint a path for only the errors on changed lines - only-new is not
working presently.
Test Plan: play around with the linter
Reviewers: lifeihuang
Reviewed By: lifeihuang
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D7055
Summary:
This allows users to specify that they want to implicitly trust their weird self-signed certificate without verification.
This can either be specified per user (which will make it apply every time the user runs `arc`, in any project):
arc set-config https.blindly-trust-domains '["example.mycompany.com"]'
...or added to a `.arcconfig` file (which will make it apply to every user who runs `arc` in that project):
"https.blindly-trust-domains" : ["example.mycompany.com"]
Depends on D7130.
Test Plan: Tweaked config and verified this setting sends HTTPSFuture down the right branch.
Reviewers: btrahan
Reviewed By: btrahan
CC: hlau, aran
Differential Revision: https://secure.phabricator.com/D7131
Summary: PHP whines about this:
> [2013-09-23 08:09:22] ERROR 2048: Declaration of CSharpToolsTestEngine::loadEnvironment() should be compatible with XUnitTestEngine::loadEnvironment($config_item = 'unit.xunit...') at [/INSECURE/devtools/arcanist/src/unit/engine/CSharpToolsTestEngine.php:13]
Auditors: jamesr, btrahan
Summary:
This implements Arcanist support for the xUnit testing framework. It also supports code coverage by way of `cstools` (which is something I've written).
The unit test support works under both Linux and Windows, while the code coverage support has only been tested under Linux (one would assume it would also work under Windows given that Windows has a super-set of functionality in the C# world).
The Arcanist support assumes that the directory layout will be something like:
* MyProject
* MyProject.Tests
When files are changed in either MyProject or MyProject.Tests, it causes MyProject.Tests to be built and the xUnit runner to be executed on the resulting binary.
Test Plan: I guess if really wanted to, you could create a C# project in MonoDevelop, set up `.arcconfig` to point to this unit test engine, and download and build xUnit and cstools. Run `arc unit --coverage` to see the results of your unit test coverage.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3859
Differential Revision: https://secure.phabricator.com/D7058
Summary:
Fixes T3856.
Detailed code coverage iterates over all of the files with changes in the current repository, however the code coverage tool might not generate a report for all changed files in the repository, and this would result in an undefined index error.
As an additional bonus, since changes to binary files won't be reported by code coverage tools, this also prevents binary data from being outputted to the console.
Test Plan:
Change a binary file in a repository and run a code coverage tool. The binary file should be reported as 0% code coverage, but the file contents should not be rendered to the console.
Change a code file that is not covered by a code coverage tool and run code coverage. The file should be reported as 0% code coverage, and the file contents should not be displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3856
Differential Revision: https://secure.phabricator.com/D7051
Summary: PhpunitTestEngine incorrectly included some source files as tests when run on a case-insensive fiesystem. This fixes that behavior. It could introduce problems for case-sensive users, but it's extremely unlikely to be an issue in practice (who would put a file in both Tests/ and tests/ and expect different results for the two?)
Test Plan: arc unit before and after change on OS X. Stubs and autoloaders in .../tests/... directories are no longer picked up.
Reviewers: epriestley
Reviewed By: epriestley
CC: aurelijus, Korvin, aran
Differential Revision: https://secure.phabricator.com/D7048
Test Plan: Saw 'Do you want to add these files to the commit?'
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6991
Summary: Automatically correct `New` to `new` in lint.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6948
Summary: See discussion in D6893. Different versions of `svn` do different stuff, just ignore any possible error here.
Test Plan: Ran unit tests.
Reviewers: andrewjcg, btrahan
Reviewed By: andrewjcg
CC: aran
Differential Revision: https://secure.phabricator.com/D6946
Summary:
git ls-tree gives type 'commit' for submodules.
This code-path is only used when a binary file is present in the diff.
Test Plan: arc diff with a binary file
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6943
Summary:
See <https://github.com/facebook/arcanist/issues/102>. PHPCS changed its output format sometime between 1.4.6 (stable) and 1.5.0RC3.
Add a "no errors" test and make the linter work on both versions.
Test Plan: Ran `arc unit` on PHPCS 1.5.0RC3.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6926
Summary:
I got bit by this one earlier (I swear I thought it was delimeter),
so I figured I would add it to the spelling data lint rule.
Test Plan:
changed an instance of delimiter to delimeter, ran `arc lint` fixed the
typo.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6936
Summary:
See IRC. This fixes an issue when deleting an SVN file that ends with one of the extensions in the regexp, which may only affect newer versions of SVN.
Possibly we shouldn't have this heuristic, or should move it elsewhere or make it more explicit, but at least stop it from being broken for now.
Test Plan: Ran `arc diff --only` in a working copy with a deleted binary file ending in ".jpg".
Reviewers: btrahan, nh
Reviewed By: nh
CC: aran, mbishopim3
Differential Revision: https://secure.phabricator.com/D6893
Summary:
Make sure on failure (restoreBranch()) we call `git submodule update --init --recursive` to handle all those purdy submodules. For the pushing step, wrap the push commands in the try / catch block so everything gets cleaned up nice if there's failure. BONUS - add --recursive to arc patch workflow to so nested submodules work correctly. (Crazy git users)
Fixes T3407, T2945.
Test Plan: I wasn't sure how to simulate a good "push" failure but I think this should work.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2945, T3407
Differential Revision: https://secure.phabricator.com/D6885
Summary:
When running arc diff in a repository that supports commit ranges, it is
possible that the setting for the default relative commit hasn't been set.
If this is the case, the user will be prompted. This change makes sure that
the prompt happens (and thus the setting is set) before we run the
background lint and unit runs.
Test Plan:
```
rm .git/arc/default-relative-commit
arc diff
```
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T2351
Differential Revision: https://secure.phabricator.com/D6854
Summary:
We have some linters that trigger based on the path name
in the tree (some rules apply in some dirs and not others).
The changes in D6798 caused all the paths to appear to be outside
the tree, so allow for passing a fake through from those test cases
that are sensitive to this.
We also have a test for the copyright linter, and that needs to read
settings from the .arcconfig file. The change to faking a working
copy meant that this config option was effectively unset, so add a way
to pass the entire arcconfig through from the tests that need it.
Lastly, the logic to skip deleted files needs to be special cased
when we're faking paths like this: if we've added data for a file
in the testable engine, we should also consider that file as existing.
Test Plan:
`arc unit --everything` here, and passing our tests in
our repo over there.
Reviewers: epriestley, mareksapota
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6841
Summary:
Ref T3186. Ref T2039. Ref T3771. A few effects here:
# Expose PHPCS as a `.arclint` linter.
# Turn PHPCS into an ArcanistExternalLinter linter.
# Add test coverage for PHPCS.
# Add a `severity.rules` option to `.arclint`. Some linters have very explicit builtin severities ("error", "warning") but their meanings are different from how arc interprets these terms. For example, PHPCS raises "wrong indentation level" as an "error". You can already use the "severity" map to adjust individual rules, but if you want to adjust an entire linter it's currently difficult. This rule map makes it easy. There's substantial precedent for this in other linters, notably all the Python linters.
For `severity.rules`, for example, this will turn all PHPCS "errors" into warnings, and all of its warnings into advice:
"severity.rules" : {
"(^PHPCS\\.E\\.)" : "warning",
"(^PHPCS\\.W\\.)" : "advice"
}
The user can use `severity` (or more rules) to get additional granularity adjustments if they desire.
Test Plan: 5bb919bc3a
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, ajtrichards
Maniphest Tasks: T2039, T3186, T3771
Differential Revision: https://secure.phabricator.com/D6830
Summary:
Ref T3776, Ref T479. Say you have some DN, with a submodule X@Y. Later, X@Z in your working copy / repo. If you run arc patch DN, you'd end up with a dirty working copy claiming that X@Z was wrong and it should be X@Y.
To fix, basically run 'submodule init' and 'submodule update'. This makes it so after "arc patch" if you run "git status" it looks clean.
Gross part though now is if you then "git checkout master" you'll have a dirty checkout the other way. I think this is better though.
Test Plan: made a new repository where I added libphutil @ X, did some work (DX), then made libphutil @ y. When I arc patch'd DX, things looked good!
Reviewers: epriestley
Reviewed By: epriestley
CC: csilvers, Korvin, aran
Maniphest Tasks: T479, T3776
Differential Revision: https://secure.phabricator.com/D6837
Summary:
Currently, we prompt the user to mark non-UTF8 files as binary, but don't actually attach the data to the change when they do. This means we don't upload the data, and can't patch it later.
A simple reproduction case is to build a test file (I used one with bytes from 1..255):
$ # Don't include \0, since Git treats that specially.
$ ./echo_every_byte_from_1_to_255_inclusive.erl > example.txt
Then add it:
$ git add example.txt
$ git commit -a -m derp
$ arc diff --only HEAD^
You'll be prompted to convert the file to binary:
Do you want to mark this file as binary and continue? [Y/n] y
Before this patch, that would be followed by:
Uploading 0 files...
...which is incorrect; we need to upload the new data. After this patch, this shows:
Uploading 1 files...
...which is also incorrect, but only grammatically. Diffs created after this patch apply back cleanly with `arc patch` and restore the file properly.
Test Plan: Followed instructions above, restoring a textual binary conversion by using `arc patch`.
Reviewers: zeeg, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6815
Summary:
If present, this will override the default phpunit path. Allows easy
integration of Composer-provided installs of phpunit (ex. set phpunit_binary to
vendor/bin/phpunit). If the path provided doesn't resolve to an executable it
will assume the path is relative to the project root.
fix line length issue
Test Plan:
Added phpunit_binary to .arcconfig in a simple project using Composer with
phpunit/phpunit package as part of require-dev (installed to
$ROOT/vendor/bin/phpunit). Phpunit not otherwise installed on the system. Set
unit.engine to PhpunitTestEngine. Confirmed that 'arc unit' used the specified
binary, both at project root and from subdirectories.
Reviewers: epriestley
CC: aurelijus, Korvin, aran
Differential Revision: https://secure.phabricator.com/D6791
Summary: Fixes T3696. Currently, we abort. If stdin is not a TTY, we should just continue. A script which cares could conceivably run `arc lint` and `arc unit` separately, but it seems unlikely that any script would ever want to fail here.
Test Plan: Ran `echo -n '' | arc diff --create --verbatim` with a lint error and got a revision (D6720).
Reviewers: Firehed, btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3696
Differential Revision: https://secure.phabricator.com/D6721
Summary:
Ref T3186. Ref T2039. Continues work on readying linters for `.arclint`.
- **Ruby**: Make this an ExternalLinter.
- **Priority**: Currently, linters have an implicit "correct" order (notably, the "NoLint" linter needs to run before other linters). Make this explicit by introducing `getLinterPriority()`.
- **Binaries**: Currently, linters manually reject binary files. Instead, reject binary files by default (linters can override this if they do want to lint binary files).
- **Deleted Files**: Currently, linters manually reject deleted files (usually in engines). Instead, reject deleted files by default (linters can override this).
- **Severity**: Move this `.arclint` config option up to top level.
- **willLintPaths()**: This method is abstract, but almost all linters provide a trivial implementation. Provide a trivial implementation in the base class.
- **getLintSeverityMap()/getLintNameMap()**: A bunch of linters have empty implementations; these are redundant. Remove them.
- **Spelling**: clean up some dead / test-only / unconventional code.
- **`.arclint`**: Allow the filename, generated, nolint, text, spelling and ruby linters to be configured via `.arclint`.
Test Plan:
458beca3d6
Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: Firehed, aran
Maniphest Tasks: T2039, T3186
Differential Revision: https://secure.phabricator.com/D6805
Summary:
The existing heuristic checks for the existence of .hg/svn, but it
turns out that this directory can exist in a non-svn hg repo if the user or a
script ever runs a 'hg svn' command.
Now we check for a file inside .hg/svn. The hgsubversion maintainer said this
particular file will always be present in hgsubversion repos.
Test Plan:
arc land --trace
Verified it used 'hg push' and not 'hg push -r ...'. This indicates it
considered the repo to be an hgsubversion repo.
Reviewers: epriestley
Reviewed By: epriestley
CC: dschleimer, sid0, Korvin, aran
Differential Revision: https://secure.phabricator.com/D6807
Summary:
Ref T3186. Brings another linter onboard. This one uses the stdin stuff.
The unit test was ostensibly broken so I fixed it, but that might just be some kind of version issue.
Test Plan: Unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3186
Differential Revision: https://secure.phabricator.com/D6802
Summary:
Ref T3186. We have about 50 linters which run programs and read the results, all of which have ad-hoc one-off custom config that isn't formalized anywhere.
Consolidate all this stuff into `ArcanistExternalLinter`, which is configurable through `.arclint` (although nothing supports this quite yet).
Extend CSSLint and Pep8Lint from `ArcanistExternalLinter`.
Add unit tests for both.
There are still some rough edges here, but it mostly seems to work pretty well.
Test Plan: Ran unit tests, hit some (most?) of the error cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran, Firehed
Maniphest Tasks: T3186
Differential Revision: https://secure.phabricator.com/D6800
Summary:
Ref T3186.
- Every linter builds a WorkingCopyIdentity in the same way, with no specialized data. Don't do that.
- Linters get passed a goofy hardcoded ".php" path. Don't do that.
- Linters generally run on an imaginary path, which might not work. Just give them a real path by building a tiny working copy in `/tmp`.
- Fix a TODO now that we have better typechecking.
Test Plan: `arc unit --everything`, intentionally broke a test to make sure that still works.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3186
Differential Revision: https://secure.phabricator.com/D6798
Summary:
Ref T2039. That task has a bunch of discussion, but basically we do a poor job of serving the midrange of lint configuration right now.
If you have something simple, the default linters work.
If you have something complex, building your own engine lets you do whatever you want.
But many users want something in between, which isn't really well accommodated. The idea is to let you write a `.arclint` file, which looks something like this:
{
"linters" : {
"css" : {
"type" : "csslint",
"include" : "(\.css$)",
"exclude" : "(^externals/)",
"bin" : "/usr/local/bin/csslint"
},
"js" : {
"type" : "jshint",
"include" : "(\.js$)",
"exclude" : "(^externals/)",
"bin" : "support/bin/jshint",
"interpreter" : "/usr/local/bin/node"
}
}
}
...which will provide a bunch of common options around lint severity, interpreter and binary locaitons, included and excluded files, etc.
This implements some basics, and very rough support in the Filename linter.
Test Plan:
Generated a `.arclint` file and saw it apply filename lint correctly. Used `debug` mode and tried invalid regexps.
{
"debug" : true,
"linters" : {
"filename" : {
"type" : "filename",
"exclude" : ["@^externals/@"]
}
}
}
Next steps include:
- Provide an external linter archetype (T3186) and expose a common set of configuration here ("bin", "interpreter", "flags", "severity").
- Provide a `.arcunit` file which works similarly (it can probably be simpler).
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D6797
Summary: Found an issue where if arc unit runs phpunit and the datasets included a dataprovider which spanned multiple lines, it wasn't filtered out correctly in the result parser, this fixes it
Test Plan: accidentally ran tests against phpunit itself which exhibits this problem. no longer a problem after this fix, nothing else breaks.
Reviewers: epriestley
Reviewed By: epriestley
CC: aurelijus, epriestley, aran
Differential Revision: https://secure.phabricator.com/D6773
Summary:
arc for mercurial on windows was broken in several way.
Executing a command via passthru failed because passthru on windows
skips the shell so 'set HGPLAIN=1 & ...' was an invalid command. The
fix was to just not set HGPLAIN for passthru commands on windows.
Also removed hardcoded '' quotes in mercurial commands since windows
doesn't support single quots.
Test Plan: arc land --hold on a windows machine
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6763
Summary: Corrects relative vs absolute branch name when using 'arc commit'
Test Plan: 'arc commit' on a release branch gave an error before making the change (change was generated from 'branches/yyy' but working copy root is 'https://xxx/branches/yyy', now it does not.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6743
Summary: My recent change adding --everything to arc lint could sometimes cause a "diff is empty" error, this patch fixes it.
Test Plan: Ran "arc lint --everything" before and after patch. No longer errors out. Only appeared to originally happen when there were uncommited changes in an svn repo.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6732
Summary:
When trying to find an unused bookmark we append hyphened suffixes to the end of the bookmark name. In Mercurial, these are treated the same as the bookmark name without the suffix, here's the output from hg log -r:
```
[mehdi] HGPLAIN=1 hg log -r "arcpatch-1"
abort: unknown revision 'arcpatch'!
[mehdi] HGPLAIN=1 hg log -r "arcpatch_1"
abort: unknown revision 'arcpatch_1'!
```
Test Plan: None.
Reviewers: epriestley, dschleimer
Reviewed By: dschleimer
CC: aran, Korvin, DurhamGoode, dschleimer
Differential Revision: https://secure.phabricator.com/D6698
Summary:
If you are trying to commit someone else's diff, arc commit gives warnings about path mismatch. This changes the path comparison to be based on the repo url rather than the local working directory. E.g. if both the author and committer are working in branches/release/2013_08_07 despite being checked out in ~/dev/2013_08_07 (system user being different, of course) it no longer warns that the WC path is different
Original behavior:
eric@Eric-MBP ~/dev/2013_07_31: arc commit --revision 21
You are not the author of 'D21: WeMerge Automatic Request'. Commit this
revision anyway? [y/N] y
Revision 'D21: WeMerge Automatic Request' was generated from '', but
current working copy root is '/Users/eric/dev/2013_07_31/'. Commit this
revision anyway? [y/N] y
Committing 'D21: WeMerge Automatic Request'...
Adding test
Transmitting file data .
Committed revision 52676.
Closing revision D21 'WeMerge Automatic Request'...
Exception
ERR-CONDUIT-CORE: You can not mark a revision you don't own as closed.
(Run with --trace for a full exception trace.)
New behavior:
eric@Eric-MBP ~/dev/2013_07_31: arc commit --revision 24
You are not the author of 'D24: WeMerge Automatic Request'. Commit this
revision anyway? [y/N] y
Committing 'D24: WeMerge Automatic Request'...
Adding test
Transmitting file data .
Committed revision 52679.
Closing revision D24 'WeMerge Automatic Request'...
Exception
ERR-CONDUIT-CORE: You can not mark a revision you don't own as closed.
(Run with --trace for a full exception trace.)
Test Plan: 'arc diff' changes with one user. 'arc patch Dxx' on a different working copy by a different user to review and test changes. accept review. 'arc commit --revision xx' as reviewer to land the patch. complaint goes away.
Reviewers: epriestley, ghostwriter78
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D6665
Summary: Update and clarify precedence of CA bundles
Test Plan:
tested with "arc call-conduit user.whoami" from project root
Same from other subdirectory in project
Repeated tests with both https.cabundle and https.cacert settings, both in
.arcconfig and ~/.arcrc
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3668
Differential Revision: https://secure.phabricator.com/D6647
Summary:
Support for no output from arc unit (For scripts, etc).
Also include --output param, analogous to arc lint --output.
Test Plan: run all 6 variants + `--output bad-value`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6642
Summary:
Single quotes aren't valid in the windows cmd prompt, so arc feature
didn't work in mercurial when it got to this line.
I have no idea why %C was used before. Nothing in that string should be
broken by the escaping.
Test Plan:
Ran arc feature --trace on my mac. Verified the command was escaped correctly
and the correct feature results were printed.
I don't have a windows machine to try it on, but the builtin escaping should
now account for windows machines.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, aran, Korvin
Differential Revision: https://secure.phabricator.com/D6637
Summary:
When adding arcanist support to a new project or adding a new linter,
it's helpful to be able to run new linters against the entire codebase. This
patch adds support for this with an '--everything' option, similar to 'arc unit
--everything'
Test Plan:
Run 'arc lint --everything' and check out the code. Optionally dump
the paths to test in the current lint engine's buildLinters() function to
demonstrate that it's receiving all files in the project rather than just the
changed and/or specified ones
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D6592
Summary: The test result parser in PhpunitTestEngine was receiving $test_path from the previous loop instead of $path from the current one. The variable isn't actually used in the PhpunitResultParser object (it exists for strict compatibility with the parent class) so it didn't cause any problems, but who knows if that could change in the future
Test Plan: Review diff. No changes to the output of running 'arc unit' when using the Phpunit engine, as expected
Reviewers: epriestley
CC: aran, epriestley, aurelijus, chad
Differential Revision: https://secure.phabricator.com/D6587
Summary: We use custom `assert*` functions here and there. Remove them from backtrace.
Test Plan: Ran `XHPASTTreeTestCase`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6411
Summary:
I suspect that when people use 'algorithmical', they mean it as an adjective and not an adverb.
'algorithmic' = adjective
'algorithmically' = adverb
Test Plan: Add the word 'algorithmical' to a file. Run `arc lint` on the file. See suggestion to correct it to 'algorithmic'.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6373
Summary:
- Replace `maniphest.find` with `maniphest.query`. These calls are nearly identical, it was just a rename for consistency.
- Replace `differential.find` with `differential.query`.
Test Plan:
- Ran `arc tasks`.
- Ran `arc close-revision` on valid, nonexistent, and existent-but-invalid revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6336
Summary:
I wanted to write a couple of workflows that shared some code in an abstract superclass, but ##arc## would fail trying to instantiate the abstract class.
As it turns out, we can modernize the ##buildAllWorkflows## function a bit, and it will only load concrete objects.
Test Plan:
1. Define an abstract subclass of ##ArcanistBaseWorkflow##.
2. ##arc liberate## the source directory that contains it.
3. Try any ##arc## operation without this diff, and see it fail.
4. Patch this diff and see that ##arc## operations work now.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6324
Summary: Makes sense now
Test Plan: Get someone else to test it
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T3342
Differential Revision: https://secure.phabricator.com/D6307
Summary: Enables the process to pass the user input for library name. Fixes T3342
Test Plan: Little help... `arc liberate` doesn't run on windows. Though I saw the flag :)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3342
Differential Revision: https://secure.phabricator.com/D6306
Summary:
When `arc diff` runs unit tests it uses all of the affected files as the base array of paths. These files may have been deleted. If the deleted file fits the test criteria `unit` will try and run the test and in some cases fail.
An example of this fataling is with PHPUnit;
> Fatal error: Uncaught exception 'Exception' with message 'JSON report file is em
pty, it probably means that phpunit failed to run tests. Try running arc unit wi
th --trace option and then run generated phpunit command yourself, you might get
the answer.' in C:\Websites\facebook\arcanist\src\unit\engine\PhpunitResultPars
er.php on line 156
Test Plan: Re-ran the tests that were causing issues with `arc unit --rev HEAD^ --trace`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D6246
Summary:
Previously, updating a commit via arc diff in mercurial would
prepopulate the update message with part of the commit message. In an
amend workflow this doesn't make sense, so I disabled it. Git already
does this, so now mercurial matches git in this scenario.
We had users complain that new users would often submit diffs with the
default update message, and it wasn't useful since they were using a
amend flow.
Test Plan:
arc diff on a commit that already had a diff
Verified the editor did not have a update message
arc diff on a stack of commits where the bottom one had a diff
Verified the editor provided a default update message
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6215
Summary: We currently swallow the exception message, but this isn't useful. Fixes T3354.
Test Plan:
$ arc diff HEAD^ --conduit-uri=http://local.aphront.com:8080/ --only
Uploading 1 files...
Failed to upload new binary 'large.png'.
[HTTP/500] Internal Server Error
As received by the server, this request had a nonzero content length but no POST data.
Normally, this indicates that it exceeds the 'post_max_size' setting in the PHP configuration on the server. Increase the 'post_max_size' setting or reduce the size of the request.
Request size according to 'Content-Length' was '2093052', 'post_max_size' is set to '100K'.
Continue? [Y/n]
Reviewers: jamesr, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3354
Differential Revision: https://secure.phabricator.com/D6186
Summary: Now that we examine these in lint, this came up. There is no `ConduitException`; the class is `ConduitClientException`.
Test Plan: Lint.
Reviewers: vrana, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6158
Summary: If one wishes to implement a linter which finds unused resources or variables the current scheme does not work as the lint engine filters all changes from lines which were not introduced in a diff. To solve that, I've added an "always show" configuration for a lint message allowing creation of such linters.
Test Plan:
1. Created a custom linter for finding unused Android resources in a project
2. Ran arc lint with linter added and received warnings as expected
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6119
Summary:
Piping data around on Windows doesn't work well when it contains zany characters like "null" and "newline". Fixes T3266.
Instead of piping data into `git apply`, write to a temporary file.
Test Plan:
Ran `arc patch`, got good results.
>>> [17] <exec> $ git apply --index --reject -- '/private/var/folders/8k/c3vkmjy5335gcxdzxkhwq82w0000gn/T/7z9iea6srikoo0sc/4266-ZEyvz9'
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T3266
Differential Revision: https://secure.phabricator.com/D6070
Summary: On Windows consoles, the star on the unit tests (I'm assuming it's a Unicode character of some kind), doesn't render correctly and you just get garbage. This fixes it so that on Windows, it falls back to just using an ASCII asterisk.
Test Plan: On Windows run some unit tests, the star should now be a plain asterisk.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6087
Summary:
@Afaque_Hussain has done a bunch of utf8 work here; combined with PhutilEditDistanceMatrix we can now do utf8 diffs correctly, in a general way, without a significant performance impact.
Use PhutilEditDistanceMatrix and `phutil_utf8v_combined()` to compute accurate diffs for all (or, at least, most) UTF8 text.
The only thing this doesn't handle completely correctly is lines beginning with combining characters. This is messy/expensive to handle and will probably never actually happen, so I'm punting for now. Nothing should actually break.
The utf8 stuff will be slow, but we only pay for it when we need it.
Test Plan:
Ran unit tests. I changed a few unit tests to use a non-combining character (snowman) for clarity, and some results are different now (since we get combining characters right).
{F44064}
Reviewers: btrahan, Afaque_Hussain
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2379
Differential Revision: https://secure.phabricator.com/D6019
Summary: Replace this old hard-coded implementation with the new vector-based, unicode-capable one.
Test Plan: Ran unit tests. Looked at revisions in Differential, using whitespace modes to bypass cache.
Reviewers: btrahan, Afaque_Hussain
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2379
Differential Revision: https://secure.phabricator.com/D6016
Summary: Also warn against functions not available on Windows at all.
Test Plan: Compared old and new file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5975
Summary: These were in my sandbox, but I forgot about them. Without this things break post D5896. Ref T2784
Test Plan: my sandbox works and soon so shall others
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5929
Summary: @alex has git 1.7.0.4 which doesn't have this flag. We don't actually need it: we always provide a commit message when calling this method. Remove the flag for compatibility, leaving a note in case we bump into this in the future.
Test Plan: N/A
Reviewers: btrahan, vrana, alex
Reviewed By: alex
CC: aran
Differential Revision: https://secure.phabricator.com/D5926
Summary:
Arc Revert does the following:
1. Git revert
2. Go to the differential of the rev you are reverting and either repoen it or set it to a reverted state
3. File a hipri task to orig author
[Preview] Creating Arc Revert workflow
Porting arc revert from FB4A to phabricator for general usage. This is my first stab,
so totally appreciate feedback and assistance. I'm currently focused
on making this work for git. However, I built out the functions through the GitAPI so this
could be easily extendable to Mercurial later on.
Stuck on the following (help):
1. Creating a task for FB internal. I tried building on top of existing arc listeners
but getting errors on failures to load the TaskCreator (and other) tasks.
2. I'm using a hacky way to grab the diff revision id from the newly created
revert diff. (see line 204) I'm looking for a way to just fetch the diff ID
from arc after the diff is created; is this possible?
Test Plan:
-
1. Ran arc revert on a www and fbcode diff
2. Confirmed that revert was run on the diffs and a proper diff filed
-
Reviewers: royw, sdwilsh, nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin, pti, keir
Maniphest Tasks: T1751
Differential Revision: https://secure.phabricator.com/D5553
Summary:
The NoseTestEngine class has some usefull code for parsing nosetest, but it assumes a very specific code/test
layout. I've pulled this logic abit apart, which lets me reuse the nose-related parts with my existing
project layout, by having my custom engine just call runTests() with the relevant paths.
Test Plan: Run on a customized project with coverage, see coverage results.
Reviewers: epriestley, roman.barzyczak
Reviewed By: epriestley
CC: seporaitis, aran, Korvin, zeeg
Differential Revision: https://secure.phabricator.com/D5921
Summary:
During my attept to `arc land master` on `master` branch
I have discovered that error message is missing few spaces between words.
I have added them and used this ugly readonly command:
pcregrep --include="\.php$" -M -r '(?<!\\n| )("|'"'"')\.\n\s*\1(?!\\n| )' ~/arc/arcanist/src
to detect other instances of this serious bug.
Two more were found.
This time they were probably introduced in order to abide
to the draconian lint rule about number of columns.
Since I want to be a good citizen,
I have added this missing space to the begining of the next line in both cases.
It is an ugly hack, but I think user should not suffer due to missing spaces.
Another solution could be preserving no leading spaces and splitting long lines.
Or just providing excuse to lint.
Test Plan: Ran `arc land master` on `master` branch.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3008
Differential Revision: https://secure.phabricator.com/D5827
Summary:
Fixes T2266. Motivation:
- The lint cache does not always invalidate correctly. Because of the nature of the cache, this is a hard problem (right after "naming things").
- We already have a fair amount of complexity in trying to invalidate it, and are still discovering new places where it doesn't work (e.g., Windows with "/" vs "\" paths).
- One invalidation failure is when linter code changes, which seems unresolvable in the general case (e.g., changes to external linters).
- It's not obvious what's happening when the lint cache causes some kind of issue.
- Particularly while developing or debugging linters, your changes often won't be reflected in the lint output. Some of this is theoretically tractable but the external linter case probably isn't.
- When someone reports a problem with the lint cache in IRC or elsewhere, there is essentially never a way for me to fix it. The lint cache can't be debugged effectively without access to a working copy where the problem reproduces.
- The cache provides limited benefit outside of Facebook's install.
To remedy these issues:
- Introduce configuration which controls cache usage.
- Default it off.
- Print a message when the cache is in use.
(I'd tentatively support removing the cache entirely, but I don't know how @vrana and Facebook feel about that.)
Test Plan: Ran `arc set-config --show`, `arc lint --cache 0`, `arc lint --cache 1`.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, mbishopim3, nh, edward, wez
Maniphest Tasks: T2266
Differential Revision: https://secure.phabricator.com/D5766
named the same as a file path.
Summary: (In fbcode and www at least, the only places I tried) if a
branch happens to be named the same as a file path (from root) (like if
a branch is called `spec` and there is a directory in `www` called
`spec`) then `arc land` will fail with git complaining that the argument
(to `git log ...`) is ambiguous as it could refer to both a path and a
revision; so this diff adds a `--` to the end so that git knows that
both are revisions and not paths.
Test Plan: Try and land something from www (I did, see D752219) where
the branch is named `spec`.
Reviewed by: epriestley
Summary: See D5714. Ref T2971.
Test Plan: Built a library map for libphutil's test library.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2971
Differential Revision: https://secure.phabricator.com/D5715
Test Plan:
$ set # on Windows
Reviewers: epriestley, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5733
Summary:
Arc lint for git is currently adding all untracked files when it
amends the commit with lint fixes. This changes the git add -A to be
git add -u. This only adds files that were already tracked. -A was adding
untracked files as well which was not the desired behavior here.
Test Plan:
Create an untracked file.
Commit a lint failure another file.
arc diff and choose to amend the lint patches.
Verify that the untracked file was not added but the tracked file was amended.
Reviewers: epriestley, wez, chad
Reviewed By: chad
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5731
Summary:
arc lint was hardcoded for git for amending commits with lint
patches. This enables the same functionality for mercurial.
Test Plan:
Made some changes that would result in a lint patch.
arc diff
Verify that the patches it produces were amended into the commit.
Verified it still works in git as well.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5716
Summary:
When doing an arc diff with pending changes in your working copy
it was creating a new commit with the pending changes instead of amending
the existing one. The problem was the author comparison was comparing
values like "John Smith <john@foo.com>" with "John Smith". The fix changes
$api->getAuthor() to return "John Smith" instead of the full string. This
matches the behavior (and implementation) found in the git api.
Test Plan:
hg book foo
touch a && hg commit -Ama
touch b && hg add b
arc diff
When prompted, amend the pending changes to the existing commit.
Verified that the changes were amended instead of making a new commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5706
Summary:
@ender is reporting a parsing issue in SVN, but we don't build a parser with setWriteDiffOnFailure() set in this workflow right now so I can't get the raw file to fix the issue.
Use the onboard mechanism to build a parser with `setWriteDiffOnFailure()` set, so it will write the diff, so I can get a copy so I can fix the problem.
Test Plan: Ran `arc diff --preview` in an SVN repo with a linter.
Reviewers: ender, btrahan
Reviewed By: ender
CC: aran
Differential Revision: https://secure.phabricator.com/D5699
Summary: Detect and fix unconventional spellings of `true`, `false`, `null` and `array` (these are the only keywords I've seen spelled unconventionally in the wild).
Test Plan: Unit tests.
Reviewers: DurhamGoode, btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2985
Differential Revision: https://secure.phabricator.com/D5686
Summary:
This adds a arcconfig setting to allow specifying whether to use the merge
or rebase strategy when doing the feature branch update.
arc.land.update.default can be set to either 'rebase' or 'merge'. The command
line flags will override this setting.
We have had trouble with arc land producing merge commits (introduced
with D4080) in git. They usually appear when arc land fails, and our users
are confused by the presence of a merge commit afterwards. Today it got even
worse since a user managed to get arc land to push the merge commit to the
server. This setting will allow us to turn it off for our uses.
Test Plan:
Verified the following combinations:
update.default not set + arc land (saw git merge in the trace)
update.default = 'rebase' + arc land (saw git rebase)
update.default = 'merge' + arc land (saw git merge)
update.default = 'rebase' + arc land --update-with-merge (saw git merge)
update.default = 'merge' + arc land --update-with-rebase (saw git rebase)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5683
Summary:
Changes arc diff to choose the base commit as the first ancestor
that has a diff. So if your tree looks like master->A->B->C->D, if you
have a diff on B (which will include A), when you run arc diff on D it will
only include C and D.
This makes the scenario for stacked diffs nicer. A user can commit A, commit B,
arc diff, commit C, commit D, arc diff, arc land B, arc land D.
Test Plan:
Commit A on top of master
Commit B on top of A
arc diff
Commit C on top of B
Commit D on top of C
arc diff
Verify the second diff contains the changes in C and D, but not A and B.
hg up B
arc land --preview
Verify that arc land shows A and B
hg up D
arc land --preview
Verify that arc land shows A, B, C, and D
(arc land should be unaffected by this change.
It always tries to land the entire branch)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5639
Summary:
If we're removing a binary file that didn't have svn:mime-type set properly,
we can't propset it (because the file doesn't exist locally). Instead, just
return a synthetic diff for the removed file.
Test Plan:
run arc diff in an svn working copy where I ran svn rm on a binary file that
doesn't have svn:mime-type set, and the diff correctly gets uploaded to
phabricator instead of erroring when trying to set properties.
Reviewers: epriestley, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5655
Summary:
Previously arc diff for hg only allowed bookmark names, rev numbers,
and commit hashes as the input base commit. This was because it escaped all
inputs and treated them as raw identifiers.
This change makes it treat the input as a revset if the escaped version fails.
This allows users to do things like "arc diff .^" when they only want to diff
the top commit.
Test Plan:
Created a stack of commits, master->A->B.
hg up B
arc diff .^
Verified the diff message only showed B as part of the diff.
arc diff .^~
Verified an error occurred ("Commit '.^~' is not a valid Mercurial commit id.")
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2888
Differential Revision: https://secure.phabricator.com/D5638
Summary:
I'm guessing this was refactored somewhere down the line and it meant
that Python and Perl files were no longer considered text files.
I'm imagining the old regex was: p(hp|y|l). Therefore I blame CSS.
Test Plan:
Perform an arc lint on a Python or Perl file that has trailing whitespace
on a line. It should prompt you for an autocorrecting lint.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5637
Summary:
The `emailuser` template is a relatively recent addition to Mercurial, and a few users have complained about it. It also doesn't actually do what I thought it did, e.g. in an address like this:
"Abraham Lincoln" <alincoln@whitehouse.gov>
^^^^^^^^^^^^^^^ ^^^^^^^^
(1) (2)
^^^^^^^^^^^^^^^^^^^^^^^
(3)
...I want (1), but `emailuser` means (2). Instead, extract (1) with `getDisplayName()` and (3) with `getAddress()` using PhutilEmailAddress.
The implementation in Mercurial is not particularly sophisticated or magical (it just looks for "@" and "<") so we aren't really missing anything by doing this ourselves, at least today.
Also fix some issues in `arc export`, which literally no one uses, but which is occasionally useful for testing (as here).
Test Plan:
- Ran `arc diff --only` in an `hg` repo, checked DB to see that name/email were correctly extracted.
- Ran `arc export --git` in an `hg` repo, didn't get a long series of fatals.
Reviewers: btrahan, DurhamGoode
Reviewed By: DurhamGoode
CC: aran
Maniphest Tasks: T2866, T2858
Differential Revision: https://secure.phabricator.com/D5539
Summary:
This adds a hook to allow external parties to provide config settings at runtime.
The hook is technically for when a RepositoryAPI is created, but that moment can
be used to set new config settings using the new setRuntimeConfig() api.
For example you could have a external hook that looks for keys like 'git:foo.bar'
or 'hg:foo.bar' and writes the value of 'foo.bar' based on whether the repo is a
git or a hg repo.
Test Plan:
Created a hook that looks for hg/git prefix versions of config keys.
Set hg:arc.feature.start.default to be "master" and set arc.feature.start.default
to be "trunk".
Ran arc feature in the hg repo. It made a bookmark on master.
Ran arc feature in the git repo. It made a branch on trunk.
Did it again, but with git:arc.feature... set instead.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5357
Summary:
Arc land is a bit magical and some users have gotten bitten by
the fact that it collapses and lands every commit on the branch. To make
it explicit what is being landed, it now shows a list of the commits
that are being landed. I also added a --preview flag that will just
print the commits that would be landed, but does nothing else.
Hopefully this make arc land a little less magical for people.
Test Plan:
arc land in the following scenarios:
- Landing one change
- Landing no changes
- Landing a stack of changes
Did it with hg and git.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, sid0, dschleimer, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5460
Summary:
Previously, arc patch would create a new commit under the existing
current bookmark in mercurial. There have been two discussions about what the
right behavior should be (D3334 and D3658). One side wants no commit at all,
and one side wants a commit under a new bookmark. The current implementation is
the worst of both worlds :(
This change makes it create a new bookmark at the revision's base before commiting,
same as the --bookmark flag used to do (which is now obsolete). That way the
existing bookmark doesn't move (in mercurial >=1.8). This is the same behavior
git has, which is convienent for groups migrating between the two.
Also makes hg's getCanonicalRevision handle svn revisions just like git. This way
arc patch will try to apply the patch to the appropriate revision in the history.
Test Plan:
Ran:
arc patch - Verified it created a new bookmark and commited on top of the
revision's base commit.
arc patch --nobranch - Verified it put the new commit on top of the current
bookmark without a new bookmark.
arc patch --nocommit - Verified it left all the changes in the working copy.
Also verified arc patch still works with git.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5408
Summary:
- Added arc.autostash option to have this behaviour off by
default (but configurable on a per-project basis).
- Automatic stashing of changes now informs the user of how
to restore their working directory if Arcanist unexpectedly
terminates.
- Fixed an issue with finalizeWorkingCopy when the workflow
didn't require a clean working copy.
Test Plan:
Test `arc diff` when there are changes in the working
directory; by default it should tell you to stash or commit.
Turn on the arc.autostash option and try again; it should
automatically stash with a message on how to recover, and
it should restore the working directory automatically under
almost any circumstances (other than an unrecoverable error).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5385
Summary:
Adds error handling for several kinds of failure in arc land for
mercurial. Previously it would often leave the repo in a confusing state
if something failed.
- Aborts the land if pull brings in a diverged 'onto' branch.
- Aborts the rebase if there is a conflict. This leaves the repo exactly as
it was before, so the user is not left with a half finished rebase.
- Don't delete the original non-squashed branch until the push succeeds.
- If the push fails, strip the temporary squashed commit. This leaves the
'onto' branch back on the latest commit from the server, and leaves the users
original nonsquashed branch around.
- Always leave the user back on their original branch after an error.
Test Plan:
Ran arc land:
- with pull causing a diverged 'onto' bookmark
- with the 'onto' bookmark already diverged
- with the rebase causing conflicts
- with a push that failed due to a commit hook
- with a successful land
- with a successful collapse and land
In all failure cases the repo was left exactly as it was before arc land,
except for the push-failed case, where the only change was that the branch
was correctly on top of the destination branch due to a successful rebase.
Used bookmark name "foo bar-gah" to test that crazy bookmark names still work.
Reviewers: epriestley, dschleimer, sid0
Reviewed By: epriestley
CC: nh, wez, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5394
Summary:
Arc patch was committing with -A (--addremove) which meant any random
files that were sitting around in the repo (like conflict .orig files) were
added to the commit. The -A isn't even necessary since the hg import
adds and removes all the appropriate files for you.
Test Plan:
touch foo
arc patch --diff some-diff-id
Verified that foo was not added to the commit
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: dschleimer, bos, sid0, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5396
Summary:
On systems with an ancient version of python, the pep8 linter won't run.
Instead of blowing up in the user's face, we should display a nice error
message.
Test Plan:
Put /usr/bin (where the ancient version of python is) at the beginning of
my path and tried to lint some python. I got a nice error instead of a
stack trace.
Reviewers: epriestley, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5356
Summary: Improves Windows compatibility.
Test Plan: Ran failing unit test on Windows.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5344
Summary: Added some sample rcsdiffs for adding and deleting a line from a file. Wrote some test cases to be tested by ArcanistDiffParser.
Test Plan: By making all the test cases pass.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5324
Summary:
Changes arc feature to read 'arc.feature.start.default' instead
of 'arc.land.onto.default'. In our usage we actually need to fork off
a different branch than we land to, so separating these is useful.
Test Plan:
Set arc.land.onto.default = master
Set arc.feature.start.default = bar
arc feature foo
cat .git/config
Verified the foo branch tracked bar
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: wez, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5336
Summary: If user changes the file contents during linting (usually when prompted to apply a patch) then we save the old messages to the new file contents. Fix that by computing the hash before linting (or after applying patch).
Test Plan: Changed the file during linting, verified that the file hash didn't change.
Reviewers: epriestley
Reviewed By: epriestley
CC: ptarjan, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5320
Summary: I looked at the pros & cons at adding hooks in git/hg vs arc land and I prefer arc land.
Test Plan:
* added an event listener and made sure I could handle the event.
* made sure things get reverted when the event handler throws an exception.
Reviewers: vrana, epriestley, pieter
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5268
Summary: The message suggests that only one revision would land.
Test Plan: Read it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5277
Summary:
This probably indicates some none fatal error, e.g.:
> remote: Certificate invalid: name is not a listed principal
The best thing here would be to avoid the error but we shouldn't explode even if it is there.
I tried to mute the error from the output but didn't find a switch or config option to do it.
Test Plan:
$ hg outgoing --branch default --style default
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5267
Summary: Have arc land inspect the revision if it depends on some other revisions which haven't been closed yet. If yes, then warn users.
Test Plan: Will test them locally.
Reviewers: epriestley, AnhNhan
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5262
Summary: unlike the hooks I copy/pasted, these hooks have parameters
Test Plan: read it
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5264
Summary: These hooks allow test cases to build shared resources -- notably, database fixtures.
Test Plan: See next diff.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5258
Summary: We don't set $paths when running --everything.
Test Plan: Ran `arc unit --everything` with coverage.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5260
Summary:
Adds arc.feature.start.default arcconfig setting to specify
a default value for 'start' in 'arc feature name start'. This lets
users always branch from origin/master (or whatever the main branch is).
Also cleaned up the 'feature' help text a little. The stuff about sorting
and closed/abandoned revisions is explained via the options list already.
Test Plan:
Ran arc feature with/without a start and with/without the config
setting set.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: bos, sid0, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5184
Summary:
Added a lint rule that warns about reusing iterator reference
variables.
Test Plan:
- Add a file with examples found in with https://secure.phabricator.com/T2536
- Did not make a unit test yet
Reviewers: vrana, bill, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Maniphest Tasks: T2536
Differential Revision: https://secure.phabricator.com/D5179
Summary:
When using arc feature, it should set up the tracking branch to be
master.
Test Plan:
./bin/arc feature tracking
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5169
Summary: These are the errors I really do.
Test Plan:
$ arc diff --no-lint
(Assuming '--no-lint' is the British spelling of '--nolint'.)
$ arc diff --reviewer a
(Assuming '--reviewer' is the British spelling of '--reviewers'.)
New unit test.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5185
Summary:
There was an accidental ! in the phase vs outgoing condition
which caused it to use 'hg outgoing' when it should have used the draft()
phase. Fixing this shaves 4.5 seconds off 'arc diff' on large repos.
Test Plan:
Ran arc diff --trace. Noted that the draft() was used and that the diff
contained the correct files and commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5182
Summary: Changes message for `arc land` that displays current branch or bookmark (if none is specified) to appropriately use the term 'bookmark' when on a bookmark in an hg repository.
Test Plan:
Ran `arc land` on new git and hg repositories, checking for correct identification of 'branch' or 'bookmark'.
~/test$ mkdir hg-test
~/test$ mkdir git-test
~/test$ cd hg-test
~/test/hg-test$ hg init
~/test/hg-test$ hg branch
default
~/test/hg-test$ hg bookmarks
no bookmarks set
~/test/hg-test$ arc land
Landing current branch 'default'.
Usage Exception: You can not land a branch onto itself -- you are trying to land 'default' onto 'default'. For more information on how to push changes, see 'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the documentation. You may be able to 'arc amend' instead.
~/test/hg-test$ hg bookmark testmark
~/test/hg-test$ hg bookmarks
* testmark -1:000000000000
~/test/hg-test$ hg branch
default
~/test/hg-test$ arc land
Landing current bookmark 'testmark'.
Usage Exception: Source testmark is a bookmark but destination default is not a bookmark. When landing a bookmark, the destination must also be a bookmark. Use --onto to specify a bookmark, or set arc.land.onto.default in .arcconfig.
Confirm still works on a git branch:
~/test/hg-test$ cd ../git-test/
~/test/git-test$ ls
~/test/git-test$ git init
Initialized empty Git repository in ~/test/git-test/.git/
~/test/git-test$ touch testfile
~/test/git-test$ git commit -am 'Test file'
~/test/git-test$ git branch
* master
~/test/git-test$ arc land
Landing current branch 'master'.
Usage Exception: You can not land a branch onto itself -- you are trying to land 'master' onto 'master'. For more information on how to push changes, see 'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the documentation. You may be able to 'arc amend' instead.
Reviewers: DurhamGoode, epriestley
Reviewed By: DurhamGoode
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D5163
Summary:
Some tests take longer (fixtures usually around 1 second for me) and also FB runs all tests on deploy.
I want to see all results immediately.
Test Plan:
Added `usleep(200000)` to `resultTest()`, then:
$ arc unit
Saw results printed one by one.
Also didn't pass `$renderer` to `ArcanistPhutilTestCase` and saw empty output.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5141
Summary:
arc diff called 'hg cat' twice for every image binary in the diff.
This turns out to take 1 second per call on a large repo because mercurial
has to parse the manifest every time.
Now arc diff batches up all the files and does only two 'hg cat'
commands. This makes the cost constant relative to the number of
images being uploaded.
Test Plan:
Ran arc diff on a diff with 30 images on both git and hg.
Verified that it was fast and that the images showed up in the web ui.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, dschleimer, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5144
Summary:
Previously, running arc lint on a set of changes that only
existed in your working copy threw an exception in mercurial repos.
It was trying to use the revset "...." (i.e. the range from . to .),
which didn't parse. Even if I fix that it still doesn't work because
getRawDiffText did not include the working copy changes (which it does
in git). I removed the check so the function now acts the same as in
git and arc lint works on working copy changes. I've seen this error before
in other places so hopefully this change will also fix any other areas,
that depended on getRawDiffText working the same as git.
The logic I removed was added in D1954 to support diffing against
uncommited changes. That workflow should be unchanged. arc diff will
still prompt the user if there are uncommited changes, and the user can
still choose to abort or continue.
Let me know if I missed something important which makes this a bad idea.
Test Plan:
Edited a file in the working directory of a hg repo.
arc lint
Verified lint ran successfully.
Also ran arc diff and land with and without working copy changes to make sure
they still work. I'd kill for some tests in this area...
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: dschleimer, bos, sid0, aran, Korvin
Maniphest Tasks: T1631
Differential Revision: https://secure.phabricator.com/D5130
Summary:
People constantly forget to bump the linter version and I don't see a way how to stop it.
This may bump the version even if it wouldn't be required but let's rather undercache than overcache.
Test Plan: `var_dump($version)`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5129
Test Plan: Threw in `didRunLinters()` of one linter, still saw the result of other linters and "Some linters failed" at the end.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5124
Summary:
I want to run lint on background and I'm interested only in side effect of caching (and maybe exit status).
This is better than discarding stdout later because we don't do unnecessary work and error conditions are still printed.
Test Plan:
$ arc lint --output none # with error
$ echo $?
$ arc lint --output none # with no lintable paths
$ arc lint --output none # witout errors
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5106
Summary:
The perf fix actually catches some real problems.
I didn't find anything in libphutil, Arcanist and Phabricator though.
Also bump version.
Also allow configuring the hook.
Test Plan:
Added a test, saw it fail with the old code.
Repeat for hook.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5097
Summary: If there's other revision in last commit message and I said `--create` then it is clear that I want to create a new commit.
Test Plan:
$ arc diff --create # in dirty working copy on top of my open revision
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5078
Summary: At least on my sample file.
Test Plan: Saw time 0.073 s instead of 12.606 s.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5086
Summary: If I have //Differential Revision// in my commit message then `arc diff --create` updates that revision instead of creating a new one.
Test Plan:
$ arc diff --create # on top of commit message with Differential Revision
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5077
Summary: This number seems more interesting and it includes time for resolving futures which is the main part of some linters.
Test Plan:
$ arc --trace lint
Reviewers: fdeliege, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D5075
Summary: Price transpositions very cheaply. We might need to edit these weights a bit, but this covers the two previous cases ("test", "alnd") and gets them right.
Test Plan: Unit tests, various `arc x` tests.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5034
Summary: Currently, if we match "alnd" to "land" and "amend" equally (distance 2), we drop "land" with the other part of the rule. Stop doing that.
Test Plan:
Unit tests. Also:
```
$ arc alnd
Usage Exception: Unknown command 'alnd'. Try 'arc help'.
Did you mean:
amend
land
```
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5033
Summary:
The message is too defensive.
Test Plan: Tested in the fork.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin, s.o.butler
Differential Revision: https://secure.phabricator.com/D5043
Summary: This is a little bit tricky - if both XHPAST and PhutilXHPAST linters lint the same path then they get the same future wrapped in two different Future iterators.
Test Plan:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5015
Summary: Nobody needs it because `raiseLintAtLine()` returns the message.
Test Plan:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4870
Summary:
The arc feature command wasn't actually creating bookmarks
on mercurial. It needs to call 'hg bookmark' instead of 'hg update'
Also removed an unnecessary hgsprintf since there were no arguments.
Test Plan:
Ran 'arc feature foo' and 'arc feature bar tip^'.
The former created a bookmark at my current location.
The latter created a bookmark at tip^ and moved me to that revision.
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5023
Summary:
Make all the broad-spectrum text linters use the new binary check.
I didn't touch `ComprehensiveLintEngine` because it's a nest of vipers and no one has complained; see T2039.
Test Plan: Ran `arc lint` on text files and binaries in a libphutil project (arcanist).
Reviewers: lisp
Reviewed By: lisp
CC: aran
Differential Revision: https://secure.phabricator.com/D5040
Summary:
Add linting capability for detecting files which contain
syntax introduced by unresolved merge conflicts. The detection is
file-type-agnostic (the only requirement is that the file is a text
file).
Test Plan:
Tested in three ways.
The first way is to add all three forms of syntax to a file to
indicate a merge conflict. HPHP will pick this up as a syntax error
before this linter reaches it.
The second way is to add the syntax in a comment. In that case, this
linter will show three warnings. For example:
$ arc lint ArcanistMergeConflictLinter.php
>>> Lint for arcanist/src/lint/linter/ArcanistMergeConflictLinter.php:
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
20
21 foreach ($lines as $lineno => $line) {
22 /*
>>> 23 >>>>>>>
24
25 =======
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
22 /*
23 >>>>>>>
24
>>> 25 =======
26
27 <<<<<<<
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
24
25 =======
26
>>> 27 <<<<<<<
28
29 */
The last test was to test on various different file types, including
JavaScript, PHP, an animated GIF, a PNG, and a Bash file to make sure
the file type detection worked. Each of the aforementioned tests
passed.
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Maniphest Tasks: T2547
Differential Revision: https://secure.phabricator.com/D4966
Summary: We were incorrectly matching `$` in the regexp against a possible `\r\n`. I missed this earlier when trying to catch all of these.
Test Plan:
- Added unit test and made it pass.
- Did another search for `getLine()` to see if I could spot any more of these, but failed to identify any via inspection.
Reviewers: vrana, mbishopim3
Reviewed By: mbishopim3
CC: aran
Differential Revision: https://secure.phabricator.com/D5038
Summary:
Following @epriestley suggestion to use PhutilServiceProfiler to log lint as a service call
Test Plan: arc lint
Reviewers: vrana
CC: phunt, aran, epriestley
Differential Revision: https://secure.phabricator.com/D4820
Summary: This is technically documented, but not currently enforced and we aren't consistent about it in the codebase.
Test Plan: See D5002.
Reviewers: chad, vrana
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5003
Summary:
Mercurial 'arc land --hold' was taking 90+ seconds on our large
repository. Since most of arc land doesn't require any particular working
directory, I've changed the mercurial logic to avoid all updates except for
two: the one prior to finding the revision (only applies if the user specified
--branch), and the one at the end to leave the user in a good state.
Also got rid of a 'hg outgoing' call when phases are supported. Also changed
the hg-subversion detection to just look for .hg/svn instead of running 'hg
svn info', which was taking 4 seconds.
Now arc land takes about 50 seconds. Still much worse than git's 25 seconds.
One big hot spot is in the two 'hg rebase' calls, which account for 25 seconds
(versus 11 seconds of git).
Test Plan:
Tested arc land with mercurial and git. Tested with and without the --branch
options.
Reviewers: epriestley, bos, sid0, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5014
