Summary: Clover reports generated from PHPUnit sometimes give false positives of lines that were not covered by a test that should have actually been not coverable, apparently due to inaccurate static analysis of files that weren't actually executed. This filters coverage reports of files that show no coverage which avoids these false positives. Fixes T10420.
Test Plan:
Looked at coverage reports of files before and after the change
Before: {F1124115}
After: {F1124113}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, aurelijus
Maniphest Tasks: T10420
Differential Revision: https://secure.phabricator.com/D15343
Summary:
Fixes T10314. In `arc land`, we currently run `git fetch` as a subprocess.
However, this can prevent password prompts (when fetching over HTTP with basic authentication) from working properly.
Instead, use passthru to redirect stdin/stdout to the subprocess so the user can type their password.
This adds a little extra clutter to the `arc land` output but I think that's OK.
Test Plan: See T10314, @maxie confirmed this fixes the issue.
Reviewers: chad
Reviewed By: chad
Subscribers: maxie
Maniphest Tasks: T10314
Differential Revision: https://secure.phabricator.com/D15233
Summary: Adds support for running all unit tests with NoseTestEngine.
Test Plan:
`ran arc unit --everything` and got unit test results
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: joshuaspence, Korvin
Differential Revision: https://secure.phabricator.com/D14362
Summary:
- Corrected typo in install instructions
- Sets the default binary to cpplint.py
Test Plan:
- Running the unit tests.
You may need to check your test environment is set up with the latest
cpplint.py available, since the default binary is being changed.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10157
Differential Revision: https://secure.phabricator.com/D15030
Summary:
This fixes the regex in "getLintCode..." so that it accepts lint
codes such as `build/c++11` which have become valid lint codes
in later versions of cpplint.
It also corrects the install instructions for the linter (Google's
style guide is no longer available on SVN and has been migrated to
Github).
Test Plan:
For the Regex:
- Create an .arclint in a project such as:
```
{
"linters": {
"cpplint": {
"type": "cpplint",
"severity": {
"build/c++11": "advice"
}
}
}
}
```
- Run `arc lint` with the existing linter. This should fail. Patch the linter, and this should now be accepted.
For the Instructions
- Verify the download location `wget https://raw.github.com/google/styleguide/gh-pages/cpplint/cpplint.py`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10118
Differential Revision: https://secure.phabricator.com/D15019
Summary: Fixes T10124.
Test Plan:
Added this "linter" to `.arclint`:
```
"thing": {
"type": "script-and-regex",
"script-and-regex.script": "echo every file is very bad #",
"script-and-regex.regex": "/^(?P<message>.*)/"
}
```
...to produce this diff. Also made a variant of it which matches lines to make sure that still works.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10124
Differential Revision: https://secure.phabricator.com/D15000
Summary:
Currently, `git show | arc diff --raw` and similar doesn't work because we try to figure out what the "Branch: feature (branched from whatever)" value is, which doesn't make sense.
```
$ git show | arc diff --raw --trace
ARGV '/Users/epriestley/dev/core/lib/arcanist/bin/../scripts/arcanist.php' 'diff' '--raw' '--trace'
LOAD Loaded "phutil" from "/Users/epriestley/dev/core/lib/libphutil/src".
LOAD Loaded "arcanist" from "/Users/epriestley/dev/core/lib/arcanist/src".
Config: Reading user configuration file "/Users/epriestley/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/Users/epriestley/dev/core/lib/arcanist/.arcconfig".
Working Copy: Path "/Users/epriestley/dev/core/lib/arcanist" is part of `git` working copy "/Users/epriestley/dev/core/lib/arcanist".
Working Copy: Project root is at "/Users/epriestley/dev/core/lib/arcanist".
Config: Reading local configuration file "/Users/epriestley/dev/core/lib/arcanist/.git/arc/config"...
Loading phutil library from '/Users/epriestley/dev/core/lib/arcanist/src'...
>>> [0] <conduit> conduit.connect() <bytes = 489>
>>> [1] <http> https://secure.phabricator.com/api/conduit.connect
<<< [1] <http> 211,217 us
<<< [0] <conduit> 212,001 us
>>> [2] <event> diff.didCollectChanges <listeners = 0>
<<< [2] <event> 140 us
>>> [3] <event> diff.didBuildMessage <listeners = 0>
<<< [3] <event> 46 us
Reading diff from stdin...
>>> [4] <conduit> differential.creatediff() <bytes = 10,542>
>>> [5] <http> https://secure.phabricator.com/api/differential.creatediff
<<< [5] <http> 120,215 us
<<< [4] <conduit> 120,411 us
>>> [6] <event> diff.wasCreated <listeners = 0>
<<< [6] <event> 41 us
SKIP STAGING Raw changes can not be pushed to a staging area.
>>> [7] <conduit> harbormaster.queryautotargets() <bytes = 290>
>>> [8] <http> https://secure.phabricator.com/api/harbormaster.queryautotargets
<<< [8] <http> 217,717 us
<<< [7] <conduit> 217,944 us
>>> [9] <conduit> harbormaster.sendmessage() <bytes = 274>
>>> [10] <http> https://secure.phabricator.com/api/harbormaster.sendmessage
>>> [11] <conduit> harbormaster.sendmessage() <bytes = 274>
>>> [12] <http> https://secure.phabricator.com/api/harbormaster.sendmessage
<<< [10] <http> 123,821 us
<<< [9] <conduit> 134,329 us
<<< [12] <http> 227,580 us
<<< [11] <conduit> 227,787 us
[2016-01-05 10:08:58] EXCEPTION: (Exception) This workflow ('ArcanistDiffWorkflow') requires a Repository API, override requiresRepositoryAPI() to return true. at [<arcanist>/src/workflow/ArcanistWorkflow.php:804]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=stable, ref.master=adb8a9c074ba, ref.stable=7b8d38cd2d4e)
#0 ArcanistWorkflow::getRepositoryAPI() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2421]
#1 ArcanistDiffWorkflow::getDiffOntoTargets() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2411]
#2 ArcanistDiffWorkflow::updateOntoDiffProperty() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:534]
#3 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:392]
```
Test Plan: Ran `arc diff --raw` in `phabricator/`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14946
Summary: `instanceof` should be used instead of `is_a`. I need to do a bit more research here to see if there are any edge cases.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14573
Summary:
Extends `ArcanistSelfMemberReferenceXHPASTLinterRule` to warn about the use of a hardcoded class name instead of `self` when instantiating a class. Arguably, we should maybe rename the linter rule from `ArcanistSelfMemberReferenceXHPASTLinterRule` to `ArcanistSelfUsageXHPASTLinterRule`, or even maybe split this rule into three separate rules:
- `ArcanistSelfMemberReferenceXHPASTLinterRule`
- `ArcanistSelfSpacingXHPASTLinterRule`
- `ArcanistSelfClassReferenceXHPASTLinterRule`.
Test Plan: Added to existing test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13935
Summary: Fixes T10008. Git tries to fix some issues by default (apparently? empirically; not consistent with documentation, I think?), but patches from `arc patch` are "always" accurate (disregarding other bugs we might have -- basically, they haven't been emailed or copy/pasted or anything like that) so we can just tell it to apply the patch exactly as-is.
Test Plan: {F1029182}
Reviewers: chad, joshuaspence
Reviewed By: chad, joshuaspence
Maniphest Tasks: T10008
Differential Revision: https://secure.phabricator.com/D14816
Summary:
Ref T9993. Users may have shell aliases or wrapper scripts that they forget about.
Print out the arguments we received to make it obvious that something went through an indirection layer.
Test Plan: Ran `arc version --help`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9993
Differential Revision: https://secure.phabricator.com/D14797
Summary: Fixes T9973. When users run `arc land --hold`, give them the commands to move forward (push) or backward (checkout the branch/tag/commit they were on) and be explicit that branches have not changed.
Test Plan: Ran `arc land --hold`, got lots of explanatory text.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9973
Differential Revision: https://secure.phabricator.com/D14762
Summary:
This fixes T9970 in an alternate manner, with the same effect: the
binary_safe_diff.sh script returns 0 if the diff succeeds, 1 in all
other cases.
Test Plan:
Tested locally with a fixed binary_safe_diff.sh, resulting in this
correct review:
https://reviews.freebsd.org/D4542
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: eadler, Korvin, stevenh
Maniphest Tasks: T9970
Differential Revision: https://secure.phabricator.com/D14759
Summary:
Some versions of Subversion (1.9 in any case, maybe others) will
duplicate diff headers, if the diff command run through --diff-cmd
returns 0.
This lead to T9970, where the addition of a new file with properties
only shows the properties themselves in the review, not the content of
the new file.
Test Plan: This is a trivial change, is a test needed at all?
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: stevenh, Korvin, eadler
Differential Revision: https://secure.phabricator.com/D14755
Summary: Ref T9973. Make this language unambiguously clear about the underlying operations.
Test Plan: Ran `arc help land`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9973
Differential Revision: https://secure.phabricator.com/D14754
Summary:
Ref T9952. Ref T3462. My primary goal is to improve prefilling of the "Onto Branch:" field in the "Land Revision" dialog.
When uploading a diff with `arc diff`, add a property with some information about which branch to target. In particular:
- If the local branch tracks an upstream branch (or tracks something which tracks something which tracks the upstream), target that.
- If not, but "arc.land.onto.default" is set, target that.
This doesn't try to guess in other cases, since they're more involved. I'll add some context about this in T3462.
I don't //love// using "diff properties" for this, but it doesn't make cleaning them up any harder since we already use it for other stuff which isn't going away (lint/unit excuses).
Test Plan:
- Added some `var_dump()` and used `arc diff --only` to generate diffs.
- Saw upstream tracking and config-based rules generate reasonable values and submit them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T3462, T9952
Differential Revision: https://secure.phabricator.com/D14736
Summary:
Fixes T9953.
- "-c" was introduced in 1.7.2.
- "--no-color" has existed forever as far as I can tell.
- "--no-column" was introducd in 1.7.11, but there was nothing that needed to be disabled before that (hopefully).
Test Plan:
- Ran `arc which --trace` and observed a reasonable `git branch` command with correct output.
- Ran `arc which --trace` with a faked older Git version, observed command omit `--no-column`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9953
Differential Revision: https://secure.phabricator.com/D14735
Summary: Add a linter rule which advises against public class properties.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14641
Summary: Adds a linter rule which ensures that binary integers are written in lowercase (i.e. `0b1` instead of `0B1`).
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14644
Summary: Binary integers (such as `0b1`) were added to PHP 5.4 and cannot be used in earlier versions.
Test Plan: Added a test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14643
Summary: PHP doesn't handle octals very well. Basically, it seems that any numeric scalar matching `/^0\d+$/` will be treated as an octal, whereas this should be `/^0[0-7]+$/`. As a result, `08` and `09` are both treated as `0` (because they are invalid octals. This diff adds a linter rule to detect this abnormality.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14604
Summary: Demonstrates the use of `CaseInsensitiveArray`. Depends on D14624.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14625
Summary: Fix a minor issue in which changing a function call to a type cast affects the result of an expression.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14623
Summary: Hexadecimal numbers should be written as `0xFF` and not `0xff` or `0Xff`.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14612
Summary: Adds a linter rule for spacing after the `&` token, similar to `ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule`.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14602
Summary: Type casts should be used instead of calls to the `*val` functions as function calls impose additional overhead.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14572
Summary: Fixes T9858. Reasonable typos and misunderstandings currently produce very confusing error messages.
Test Plan:
```
$ arc install certificate
Usage Exception: Server URI "certificate" must include a protocol and domain. It should be in the form "https://phabricator.example.com/".
```
- Also used a good URI.
- Also used no URI.
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T9858
Differential Revision: https://secure.phabricator.com/D14577
Summary: If a `namespace` is used within a PHP script, it must be the first statement.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14526
Summary:
`interface`s cannot contain `abstract` methods. This construct will cause a PHP fatal error:
```
Access type for interface method SomeInterface::someMethod() must be omitted
```
Test Plan: Added test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14562
Summary:
`interface` methods cannot contain a body. This construct will cause a fatal error:
```
PHP Fatal error: Interface function X::Y() cannot contain body in /home/josh/workspace/github.com/phacility/arcanist/test.php on line 4
```
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14561
Summary: A user reported to me that `arc lint` was failing with an error message along the lines of "argument 1 passed to `idx` must be of type array, bool given". I suspect that the user is running an older version of PHP, which means that `array_combine(array(), array())` is returning `false` instead of the expected `array()`. Instead, avoid calling `array_combine` on an empty array.
Test Plan: I don't have PHP 5.3 easily accessible to test this.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14567
Summary: Return values within constructors are acceptable if they are within a closure or anonymous function.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14564
Summary:
`abstract` methods cannot contain a body.
```
PHP Fatal error: Abstract function X::Y() cannot contain body in /home/josh/workspace/github.com/phacility/arcanist/test.php on line 4
```
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14560
Summary:
A class containing `abstract` methods must itself be marked as `abstract`.
```
PHP Fatal error: Class X contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (X::Y) in /home/josh/workspace/github.com/phacility/arcanist/test.php on line 5
```
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14559
Summary:
Methods cannot be marked as both `abstract` and `private`. This language construct will cause a fatal error:
```
PHP Fatal error: Abstract function X::Y() cannot be declared private in /home/josh/workspace/github.com/phacility/arcanist/test.php on line 4
```
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14558
Summary: Add a linter rule to detect symbols which are aliased with the same name, such as `use X\Y\Z as Z`. This is unnecessary and is more-simply expressed as `use X\Y\Z`.
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14557
Summary:
When importing or aliases a symbol with a `use` statement, the leading namespace separator is optional and does not modify the behavior. That is, `use \X` is equivalent to `use X`. As such, the latter syntax should be preferred because it is more concise.
According to the [[http://php.net/manual/en/language.namespaces.importing.php | PHP documentation]]:
> Note that for namespaced names (fully qualified namespace names containing namespace separator, such as `Foo\Bar` as opposed to global names that do not, such as `FooBar`), the leading backslash is unnecessary and not recommended, as import names must be fully qualified, and are not processed relative to the current namespace.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14517
