1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-09 16:32:39 +01:00
Commit graph

2109 commits

Author SHA1 Message Date
epriestley
f20d4b15c7 Add a lint error about use of PHP short array syntax ('[...]')
Summary: Ref T11409. Add lint to detect using `[...]` to define arrays. This doesn't work in PHP 5.2/5.3, which some of our users run.

Test Plan:
  - Ran `arc unit`.
  - Ran `arc lint src/applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php` to detect the issue in T11409.

Reviewers: yelirekim, chad

Reviewed By: chad

Maniphest Tasks: T11409

Differential Revision: https://secure.phabricator.com/D16357
2016-08-01 10:45:09 -07:00
epriestley
06c641f92c Remove command spelling correction from Arcanist
Summary: Fixes T7489. Depends on D16332, which moved this code to libphutil.

Test Plan:
```
$ arc banch --bystatus
(Assuming 'banch' is the British spelling of 'branch'.)
(Assuming '--bystatus' is the British spelling of '--by-status'.)
...
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7489

Differential Revision: https://secure.phabricator.com/D16333
2016-07-27 09:35:28 -07:00
Rabah Meradi
1bedfdccd7 Fixes help message for stop command
Test Plan: arc help stop will display 'Stop tracking work...'

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: avivey, chad, epriestley

Differential Revision: https://secure.phabricator.com/D16310
2016-07-27 05:42:07 -07:00
Alex Vandiver
8f69a5c378 Use phutil functions to copy/move files
Summary:
The `cp` and `mv` commands, when run from a Windows
environment, error out.  Use library functions from `Filesystem`
for cross-platform compatibility.

Test Plan:
Ran `arc lint` with auto-fix lint errors on both Linux and
Windows.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Spies: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D16273
2016-07-12 10:21:58 -07:00
epriestley
4d4d16f259 Validate Arcanist install-certificate URIs more carefully
Summary: Fixes T11222. This was lazy-future-proofed for Conduit SSH support, but users are boundlessly creative. Check protocols explicitly.

Test Plan:
```
$ arc install-certificate a.b:1/
Usage Exception: Server URI "a.b:1/" must include the "http" or "https" protocol. It should be in the form "https://phabricator.example.com/".
```

  - Also went through a successful workflow with a URI in the form provided in the example.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11222

Differential Revision: https://secure.phabricator.com/D16188
2016-06-28 15:20:06 -07:00
epriestley
2374403e8f Remove the "you have not specified reviewers" prompt from the arc client
Summary:
Ref T4631. Ref T10939. I don't have any good solutions here; this is perhaps the least-bad one.

  - This prompt is misleading/confusing in the presence of Herald/Owners.
  - This prompt is likely of very little value for experienced reviewers.
  - When it works, this prompt may be of some value for new reviewers, but getting it wrong is probably more confusing than getting it right is helpful, and there is a more accurate version of the warning in the web UI that new users are likely to see.
  - In the long run, this code should not live in the client.

Test Plan: Created this revision without specifying reviewers, probably didn't get prompted.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4631, T10939

Differential Revision: https://secure.phabricator.com/D16139
2016-06-17 08:04:08 -07:00
epriestley
c13e5a6295 Use an HTTPEngineExtension to implement "https.blindly-trust-domains" in Arcanist
Summary: Ref T10227. This converts weird hard-codey magic to the new HTTPEngineExtension.

Test Plan: See D16090.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10227

Differential Revision: https://secure.phabricator.com/D16091
2016-06-09 12:02:15 -07:00
epriestley
c75b671b22 Use "internal" smoothing for code diffs in Arcanist
Summary: Ref T7643. This moves the prefix/suffix smoothing behavior back to the old one, which seems better for code diffs.

Test Plan: `arc unit --everything`

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7643

Differential Revision: https://secure.phabricator.com/D16074
2016-06-07 14:15:20 -07:00
epriestley
41e8e30e8c Use EditDistanceMatrix diff smoothing in Arcanist
Summary:
Ref T7643. We've done the smoothing from D16068 for a long time, it just wasn't part of EditDistanceMatrix.

Since it now is, call it instead of keeping a copy of the logic around.

Previously, the unit tests were testing un-smoothed diffs. Just have them test smoothed diffs instead, since nothing actually uses unsmoothed diffs.

Test Plan: Pasted a raw diff into the web UI, got a sensible inline diff for it.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7643

Differential Revision: https://secure.phabricator.com/D16069
2016-06-07 09:13:34 -07:00
Aviv Eyal
ca33240942 Don't specify size 0 for deleted files
Summary: See D15828 - arc is reporting file size as `0` for unexisting files - make it stop.

Test Plan: `arc diff` with empty, deleted, added files - see size reported as `null` when appropriate.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: chad, Korvin

Differential Revision: https://secure.phabricator.com/D16059
2016-06-06 20:51:16 +00:00
epriestley
7891df6f25 Read arc aliases from all available configuration files
Summary: Fixes T10431. Updates the getAliases() method to read every `arc` configuration file.

Test Plan:
Added an `arc duck` alias to `/etc/arcconfig`, ran `arc duck`, saw "quack".

Added an `arc pig` alias to `~/.arcrc` via `arc alias`, ran `arc pig`, saw "oink oink".

Reviewers: nevogd, chad, #blessed_reviewers

Reviewed By: chad, #blessed_reviewers

Subscribers: eadler, epriestley

Tags: #twitter

Maniphest Tasks: T10431

Differential Revision: https://secure.phabricator.com/D15342
2016-06-06 13:15:59 -07:00
Allan Jude
19608cffe6 SVN buildSyntheticAdditionDiff: exit sooner if path is a directory
Summary:
When doing svn copy, or svn mv, a SynthenticAdditionDiff is generated.

If the path is a directory, an error will occur when checking the
mime-type of the directory. Immediately after the properties check,
the function returns null if the path is a directory. Move this
check to before the properties check to avoid exiting with an error.

```
Command failed with error #1!
COMMAND
svn propget 'svn:mime-type' '/home/trasz/svn/ports/cad/py-pycam'@

STDOUT
(empty)

STDERR
svn: warning: W200017: Property 'svn:mime-type' not found on '/home/trasz/svn/ports/cad/py-pycam@'
svn: E200000: A problem occurred; see other errors for details

(Run with `--trace` for a full exception trace.)
```

Test Plan: Created differentials of changes with `svn copy` and `svn mv`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Tags: #subversion

Differential Revision: https://secure.phabricator.com/D15985
2016-06-05 15:12:46 -07:00
epriestley
2234c8cacc Fix arc which for svn repos
Summary:
`arc which` is currently broken for svn repos.

Fixes T6635

(I think I independently wrote an identical change to yours)

Test Plan: `∴../../p/arcanist/bin/arc which` on a svn repo.

Reviewers: aik099, epriestley, #blessed_reviewers

Reviewed By: aik099, epriestley, #blessed_reviewers

Subscribers: aik099, Korvin

Maniphest Tasks: T6635

Differential Revision: https://secure.phabricator.com/D15922
2016-05-16 12:54:37 +00:00
Eitan Adler
c58f1b9a25 Prefer pip to easy_install
Summary:
Prefer pip to easy_install
also fixes incorrect install instructions for closure linters

Fixes T10892
Ref T10038

Test Plan: inspection

Reviewers: avivey, #blessed_reviewers, epriestley, tjstum

Reviewed By: avivey, #blessed_reviewers, epriestley, tjstum

Subscribers: avivey, Korvin

Maniphest Tasks: T10038, T10892

Differential Revision: https://secure.phabricator.com/D15818
2016-04-29 15:45:11 +00:00
epriestley
768e1a56bc When running XHPAST unit tests, include the "syntax error" lint rule
Summary:
See rARC3ffed59bd7. Currently, when a unit test includes a syntax error, it is raised in an unclear way ("error at line 10, char 1: XHP1 Unknown lint message!").

This is because each test case only activates rules it wants to test, so we lose the ID/name for the syntax message. However, we always want to test this and the lint engine can always raise it.

To get a better error message, include it unconditionally. So a test for rule `X` really tests two rules: syntax, and `X`.

Test Plan:
Ran `arc unit` at HEAD, got a better test failure:

```
   FAIL  ArcanistCallTimePassByReferenceXHPASTLinterRuleTestCase::testLinter
In 'call-time-pass-by-reference.lint-test', expected lint to raise error on line 10 at char 8, but no error was raised. Actually raised:
  error at line 10, char 1: XHP1 PHP Syntax Error!
```

NOTE: This doesn't pass tests yet, it just makes the test failure easier to understand. I'll see about fixing the test in the next change.

Reviewers: chad, richardvanvelzen

Reviewed By: richardvanvelzen

Differential Revision: https://secure.phabricator.com/D15819
2016-04-29 06:30:04 -07:00
Richard van Velzen
3ffed59bd7 Update xhpast linter rules for new function AST format
Summary: See D15678. A node containing a return type hint is added right before the statement body node. This updates all relevant linter rules to now retrieve the body from the correct index.

Test Plan: Ran `arc unit --everything`, inspected every single message to make sure all xhpast test cases succeeded. Also grepped for `getChildByIndex(5` and `getChildOfType(5`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15814
2016-04-29 14:38:10 +02:00
Joshua Spence
ea6fc77892 Fix incorrect variable for linter standards
Summary: It is currently not possible to apply multiple linter standards because `$value` was used instead of `$standard`.

Test Plan: Was able to apply multiple linter standards.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: eadler, thaiphv, Korvin

Differential Revision: https://secure.phabricator.com/D15212
2016-04-29 08:00:07 +00:00
Jon Parise
9b07d1c480 Recognize error codes from Flake8 extensions
Summary:
Flake8 extensions are allowed to use their own letter-prefixed codes. For
example, the flake8-debugger extension emits 'T002'-tagged messages.

This change relaxes getLintCodeFromLinterConfigurationKey() to also recognize
extension codes. Otherwise, attempting to configure message severities for
e.g. 'T002' would result in an exception.

Messages from extensions continue to default to ERROR severity, as they did
before this change.

Test Plan:
Successfully reduced the severity of 'T002' to a warning via .arclint:

    "severity": {
        "T002": "warning"
    }

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15810
2016-04-27 15:11:46 -07:00
epriestley
a2ab38df78 Improve performance of arc branch in Git with many branches
Summary:
This is mostly just a personal quality-of-life fix. I run this command fairly often and having it return a little faster is nice.

This replaces a `git show` for each individual branch with a big `git for-each-ref` which we were already running anyway. This is quite a bit faster.

This command also occasionally hangs or segfaults for me while executing the huge pile of subprocesses. This is unreliable to reproduce, probably some bug in some PHP extension I have, and likely hard to narrow down, and this approach is better in every way anyway.

Test Plan:
  - Ran `arc branch` in Git, observed faster output (in my `phabricator/`, about 2000ms -> 1200ms).
  - Ran `arc feature` in Mercurial.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D15735
2016-04-16 16:39:32 -07:00
Mukunda Modell
737f5c0df9 Allow amending revisions without commandeering first
Summary:
It is common practice in Wikimedia's projects to amend a contributor's
change without taking over authorship of the change. We found that
the only enforcement of commandeering before amending is in arcanist,
not validated server-side. While it would be fairly straightforward to
maintain this as a patch to arcanist, I thought I would see if upstream
is willing to support making this optional.

With this change, amending without commandeering is enabled by a flag in
`.arcconfig` and it defaults to the old behavior.

For background see [wmf T121751](https://phabricator.wikimedia.org/T121751)

Test Plan:
* ran `arc patch D146` to locally apply a revision that I did not author,
* made a trivial change and amended the commit.
* ran `arc diff --update D146 HEAD^` to send the update to differential
* Saw that https://phabricator.wikimedia.org/D146 updated as it should.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: greggrossmeier, aklapper, Luke081515.2, Korvin, dereckson

Maniphest Tasks: T10584

Differential Revision: https://secure.phabricator.com/D15468
2016-04-09 11:57:42 -05:00
epriestley
8701e6c1f3 Strip tips out of commit messages from arc backout
Summary:
Fixes T10707. Currently, `arc backout` creates a commit message which includes questionably-helpful "tips" in the message itself.

Strip these out.

Test Plan:
Used `arc backout` to revert any commit, then `git show` to see the generated message.

  - Before patch: included tips.
  - After patch: no tips.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10707

Differential Revision: https://secure.phabricator.com/D15573
2016-04-02 07:26:02 -07:00
Aviv Eyal
3d7ac867f5 Make callsigns optional in arcanist
Summary:
Remove couple of references to callsigns:
- `arc which` now prints repository name
- `getShouldAmend()` can now use new format of commit name

a quick git-grep looks like the remaining references are all about `repository.callsign` config.

Ref T4245

Test Plan:
- `arc which` on a repository with no callsign
- trigger `requireCleanWorkingCopy()`, see both "Do you want to amend this change" and "Do you want to create a new commit" prompts.
- fire this diff with new code.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D15472
2016-03-15 03:58:46 +00:00
epriestley
ccbaee585e When arc pushes to the staging area, tell Phabricator what we did
Summary:
Ref T10093. Right now, Phabricator kind of guesses that `arc` probably pushed stuff to the staging area.

This can cause confusing/misleading errors later, if it didn't actually push.

Instead, tell Phabricator that we pushed, so we can raise more tailored messages in the web UI (e.g., make "Land Revision" say "this wasn't pushed to the staging area" instead of "whoops, error!!~").

Test Plan:
Ran `arc diff` a few times, then looked in the database for properties.

{F1161655}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10093

Differential Revision: https://secure.phabricator.com/D15426
2016-03-07 07:24:16 -08:00
epriestley
4a1160e0c3 When pushing changes to staging, also push the base commit
Summary:
Fixes T10509. Pushing changes to staging can be inefficient. What happens, roughly, is:

  - Master is at commit "W" -- "W" is the most recent published commit in the main repository.
  - The local working copy has one change on top of that, "X", so its history is commits "A, B, C, D, E, F, ..., U, V, W, X".
  - The remote has some other previous changes that I or other users have made, maybe like "A, B, C, ..., S, T, U, Y" and "A, B, C, ..., T, U, V, Z", from previous pushes to staging areas.
  - "X", "Y" and "Z" will never actually make it to master, because they'll be squash-merged/amended by `arc land`.

So the local says "I want to push 'X'", and the remote says "I know about 'Y' and 'Z', are those in the history of 'W'? You only need to send me new stuff if they are".

But they aren't, so the local says "nope, so here's the whole history for you". This is slow and sends a ton of data that the remote already has over the network.

In theory, Git could use a slightly different algorithm to tell the local about more commits, but this is hard, rarely useful, and not the kind of thing I'd be excited about changing if I was the Git upstream.

Instead, when pushing "X", also push "W", to trick Git into telling future clients about it.

Now, the remote should say "I know about 'W', 'Y' and 'Z'", and the local will say "oh, great, 'W' is in history, here's just the changes since then".

Also, fail `arc diff` if the push to staging fails, and tell users to use `--skip-staging`. This code has been in production for a while and doesn't seem to have any issues, and a failed push to staging prevents builds, lands, etc.

Test Plan:
  - Ran `arc diff`, saw two changes push.
  - Ran `arc diff --base arc:empty`, saw only one change push.
  - Ran `arc diff` with an intentionally broken staging area, saw an error.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10509

Differential Revision: https://secure.phabricator.com/D15424
2016-03-07 06:53:49 -08:00
Eitan Adler
604ceb4fe5 Use python2 instead of python in arc anoid shebang`
Summary:
With the old shebang of `#!/usr/bin/env python` on machines with python 3 as the default python it would fail.
Prefer an explicit python2 like PEP 394 suggests.

Test Plan: ran ./arc anoid and played a game

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15408
2016-03-05 14:29:46 -08:00
epriestley
3876d93583 Properly URL encode branches in arc browse --branch ...
Summary: Fixes T10511. If you `arc browse --branch x/y/z`, we do not encode the URI properly.

Test Plan:
Ran `arc browse --branch x/y/z/ something.c`.

Before, got an error about "x" does not exist. This is wrong; the error should be about "x/y/z".

After, got the proper error:

{F1141096}

Reviewers: chad, avivey

Reviewed By: avivey

Maniphest Tasks: T10511

Differential Revision: https://secure.phabricator.com/D15397
2016-03-04 15:52:58 -08:00
epriestley
b1de04aa68 Report unit test details from Arcanist to Harbormaster
Summary: Ref T10457. Provides information for D15363.

Test Plan: See D15363.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9951, T10457

Differential Revision: https://secure.phabricator.com/D15364
2016-03-04 15:49:46 -08:00
Eric Stern
086f5399bf Filter out some Clover coverage to prevent false positives
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
2016-02-24 12:58:07 -08:00
epriestley
fcc11b3a27 Add --quiet to git fetch on arc land
Summary: This makes it a bit quieter.

Test Plan: shh

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D15236
2016-02-10 14:00:24 -08:00
epriestley
d6b1531b94 Use passthru to run git fetch in arc land so password prompts work
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
2016-02-10 12:31:18 -08:00
Mukunda Modell
57f6fb59d7 Make arc unit NoseTestEngine work in a sub-dir
Summary:
D14362 didn't actually work in a subdirectory.

This globs the tests/ directory to find test_*.py, now you get
more than just a coverage report when running `arc unit --everything`
whether it's run from the project root or a subdirectory.

Test Plan:
ran `arc unit --everything` from project root and also ran it from
a sub-directory.

```
$ arc unit --everything
   PASS    1ms★  test_log.FilterTest.test_filter_can_invert_behavior
   PASS    1ms★  test_log.FilterTest.test_filter_filters_matching
   PASS   <1ms★  test_log.FilterTest.test_filter_filters_matching_lambda
   PASS    1ms★  test_log.FilterTest.test_filter_filters_matching_regex
   PASS    3ms★  test_log.FilterTest.test_filter_loads
   PASS    2ms★  test_log.FilterTest.test_filter_loads_filters_correctly
   PASS   <1ms★  test_log.FilterTest.test_filter_loads_supports_numeric_comparisons
   PASS    1ms★  test_log.FilterTest.test_filter_parse
   PASS    1ms★  test_log.JSONFormatterTest.test_format_includes_all_serializable_logrecord_fields
   PASS    1ms★  test_log.JSONFormatterTest.test_format_includes_exceptions_as_text
   PASS   <1ms★  test_log.JSONFormatterTest.test_format_includes_extra_fields
   PASS    1ms★  test_log.JSONFormatterTest.test_make_record
   PASS    1ms★  test_nrpe.NRPETest.test_load
   PASS   <1ms★  test_nrpe.NRPETest.test_registered_check
   PASS   <1ms★  test_nrpe.NRPETest.test_unregistered_check
   PASS   <1ms★  test_ssh.JSONOutputHandlerTest.test_accept_ignores_bad_json
   PASS   <1ms★  test_ssh.JSONOutputHandlerTest.test_accept_parses_and_logs_json_messages
   PASS   <1ms★  test_ssh.JSONOutputHandlerTest.test_lines_buffers_partial_lines
   PASS    4ms★  test_checks.ChecksConfigTest.test_custom_type
   PASS    2ms★  test_checks.ChecksConfigTest.test_load_bad_type
   PASS    5ms★  test_checks.ChecksConfigTest.test_load_valid_config
   PASS   19ms★  test_checks.ChecksExecuteTest.test_execute
   PASS  222ms   test_checks.ChecksExecuteTest.test_execute_concurrency
   PASS   18ms★  test_checks.ChecksExecuteTest.test_execute_failure
   PASS   15ms★  test_checks.ChecksExecuteTest.test_execute_timeout
   PASS   <1ms★  test_utils.UtilsTest.test_check_php_opening_tag
   PASS    3ms★  test_utils.UtilsTest.test_check_target_hosts
   PASS    1ms★  test_utils.UtilsTest.test_check_valid_json_file
   PASS   <1ms★  test_utils.UtilsTest.test_get_env_specific_filename
```

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14363
2016-01-25 04:18:15 -06:00
Mukunda Modell
cb270ac0ee Merge branch 'NoseTestEverything' 2016-01-25 04:13:51 -06:00
Mukunda Modell
607cd77ca1 Add support for '--everything' in NoseTestEngine
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
2016-01-25 04:08:53 -06:00
Michael Akinde
b87138356a Cleans up some minor issues in cpplint
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
2016-01-15 08:19:23 -08:00
Michael Akinde
22af67c1c0 Minor fixes to arcanist cpplint
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
2016-01-14 06:59:10 -08:00
epriestley
05c12eb9d9 If the Script-and-Regex linter captures no "line" text, treat the message as affecting the entire file
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
2016-01-11 17:37:34 -08:00
John Allen
aeb374b333 Fix cpplint message regex
Summary:
ref T8404

The issue is caused, not by the non-zero return code, but by the fact that
$messages is coming back empty due to the incorrect regex. tyhoff pointed out
that the regex matched correctly when we used STDIN, but now it is failing.

https://secure.phabricator.com/diffusion/ARC/browse/master/src/lint/linter/ArcanistExternalLinter.php;8c589f1f759f0913135b8cc6959a6c1589e14ae4$357

Test Plan:
arc lint cpp file containing lint error. run cpplint on the
file directly to confirm that there are errors and that the return code
is non-zero

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T8404

Differential Revision: https://secure.phabricator.com/D14960
2016-01-06 13:15:09 -08:00
epriestley
98d71571e4 Fix arc diff --raw with "onto" target properties
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
2016-01-05 10:16:39 -08:00
Joshua Spence
b3e68c9f17 Add a linter rule for use of is_a
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
2015-12-23 08:44:39 +11:00
Joshua Spence
9ee14d2849 Improve the "self member reference" linter rule
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
2015-12-23 08:39:44 +11:00
epriestley
8762e3f367 Use "--whitespace nowarn" in arc patch to respect trailing whitespace
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
2015-12-17 17:36:26 -08:00
epriestley
a183c2c4ba When arc is run with --trace, print out argv explicitly
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
2015-12-17 16:15:00 -08:00
epriestley
9a373c88d7 Explain how to move forward or backward from arc land --hold
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
2015-12-13 08:19:04 -08:00
Dimitry Andric
7d25fcdbdb Simplify diff exit code handling.
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
2015-12-13 02:35:07 -08:00
Dimitry Andric
97056a3b85 Preserve the exit code of the diff command in binary_safe_diff.sh
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
2015-12-13 02:21:43 -08:00
epriestley
74c7495b1a Clarify that "arc land" means it is merging changes, not branch refences
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
2015-12-12 10:25:12 -08:00
epriestley
dae2f0073f In arc diff, try to guess where a change should land
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
2015-12-10 15:24:38 -08:00
epriestley
d0e73bb656 Don't use "-c" flag in "git:branch-unique()" revision range rule
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
2015-12-10 14:19:39 -08:00
Joshua Spence
0c8124a272 Fix handling of empty array index
Fix handling of empty array index by `ArcanistCurlyBraceArrayIndexXHPASTLinterRule`.

Auditors: epriestley
2015-12-09 08:09:02 +11:00
Joshua Spence
d2e7785497 Add a linter rule for curly brace array indexes
Summary: In PHP, both `$x['key']` and `$x{'key'}` can be used to access an array (see  http://stackoverflow.com/questions/8092248/php-curly-braces-in-array-notation), but the former should be preferred.

Test Plan: Added test cases.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14603
2015-12-09 07:16:12 +11:00