Various linter fixes

Summary:
Fixed various linter issues that were identified by running `arc lint --everything`.

- Removed trailing whitespace.
- Added newline at EOF.
- Added whitespace after `if` statement

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: aurelijus, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8337
Removed useless comments.

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
Made `ArcanistNoLintTestCaseMisnamed` final.

Summary: Self explanatory.

Test Plan: `arc unit`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8386
Simplified the logic in various functions.

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
Use PHP type hinting in `ArcanistXHPASTLinter`.

Summary: Explicitly specify the types of the function parameters.

Test Plan: `arc unit`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8388
Correctly identify `@todo` comments as TODOs.

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
Add "Changes Planned" and "In Preparation" states for revisions

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
Generate a documentation book for Arcanist

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
Remove a parameter

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
Fail lint tests if no tests are found in the test directory

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
Fail Arcanist tests when they make zero assertions

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
Fix unit test coverage for `NoseTestEngine`.

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
Added some additional assertion methods.

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
Utilize `assertFalse` and `assertTrue` methods.

Summary:
Ref D8460.

Use `$this->assertFalse(...)` and `$this->assertTrue(...)` instead of `$this->assertEqual(false, ...)` and `$this->assertEqual(true, ...)` respectively.

Test Plan: `arc unit`

Reviewers: #blessed_reviewers, epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8461
Make assertTrue() / assertFalse() messages more descriptive

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
Ignore detached git branches.

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
Arcanist - add revision data to TYPE_LAND_WILLPUSHREVISION event

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
Lint engine seems to get null configuration manager in svn precommit hook workflow

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
Use hgsprintf() when managing bookmarks in arc patch in mercurial

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
Don't close revisions on "arc amend"

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