1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2025-01-04 20:00:59 +01:00
Commit graph

913 commits

Author SHA1 Message Date
vrana
eb8b414cc7 Don't run disabled lint rules
Summary:
We always generate all messages and then filter them out based on minimum severity.
It's lots of useless work, especially in commit hook mode where we are interested only in errors.

Test Plan:
  $ arc lint --cache 0 --severity error ArcanistXHPASTLinter.php
  0.406 s before, 0.074 after

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4963
2013-02-14 15:31:10 -08:00
epriestley
39704f1e49 Fix "arc:this" and "arc:amended" base rules after D4838
Summary:
After D4383, we escape the base commit when constructing a command like this:

  hg log --rev (base::. - base)

However, if the base commit is a revset like ".^", we now escape it and Mercurial looks for a commit named ".^" (a valid mercurial branch name) instead.

Fix this by returning nodes for these rules instead of revsets. The "arc:this" rule is automatically used in some operations, like "arc amend", so users can hit this during normal workflows, not just with weird `--base` rules.

Test Plan: Ran "arc amend" in a Mercurial repository, didn't fatal out.

Reviewers: DurhamGoode, sid0

Reviewed By: DurhamGoode

CC: tido, aran

Differential Revision: https://secure.phabricator.com/D4949
2013-02-14 13:57:34 -08:00
Jakub Vrana
7803b7da27 Decide whether to commit or amend with arc diff -a
Test Plan:
  $ arc diff -a
  $ arc diff -a # saw amend instead of a new commit

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4947
2013-02-14 11:56:53 -08:00
vrana
619dd62f31 Respect granularity in reading cached lint messages
Summary:
We save repository version to lint cache but ignore it when reading the cache.
Fix it.

Test Plan: Made an error for linter with repo granularity, deleted the error from the cache. Relinted, didn't see the error. Changed another file and relinted, saw the error.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4841
2013-02-14 11:45:38 -08:00
durham
0795edfe1a Fix hg 'arc land' when the mq extension is not enabled
Summary:
Mercurial 'arc land' uses the 'hg strip' command to clean up after
itself, but this command isn't available unless the mq extension is enabled.
The fix is to enable it for that particular command only.

Test Plan: Ran 'arc land' with the mq extension disabled.  It worked.

Reviewers: epriestley, bos, sid0, dschleimer

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4955
2013-02-14 11:37:49 -08:00
vrana
299b9c4c6b Support arc bookmark in Mercurial
Summary:
Branch in Mercurial means something else.
Hopefully users wouldn't be too confused.

Test Plan:
  $ arc help
  $ arc help branch
  $ arc help feature
  $ arc feature
  $ arc bookmark
  $ arc branch
  # In hg repo:
  $ arc feature
  $ arc feature new
  $ arc feature new

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2332

Differential Revision: https://secure.phabricator.com/D4753
2013-02-13 13:58:07 -08:00
vrana
980889f1ba Handle safe HTML in intraline renderer
Test Plan: Used it in Phabricator.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4928
2013-02-13 12:30:27 -08:00
epriestley
42ae7cd92f Add lint warning for $o->m()[0], not just f()[0]
Summary: We only caught half of this.

Test Plan: Unit test.

Reviewers: edward

Reviewed By: edward

CC: aran

Maniphest Tasks: T1261

Differential Revision: https://secure.phabricator.com/D4920
2013-02-12 07:07:35 -08:00
vrana
2419718593 Merge PHT into dynamic string linter
Test Plan: Existing unit test.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4914
2013-02-11 18:59:53 -08:00
epriestley
446e5c4599 Apply rtrim() to arc:upstream
Summary: Git spits these out with \n at the end.

Test Plan:
```
>>> orbital ~/devtools/arcanist $ git branch --set-upstream trim master
Branch trim set up to track local branch master.
>>> orbital ~/devtools/arcanist $ arc which --base arc:upstream
RELATIVE COMMIT
If you run 'arc diff', changes between the commit:

    acf7600e6e  Temporarily restore apache/license linters

...and the current working copy state will be sent to Differential, because
it is the merge-base of the upstream of the current branch and HEAD, and
matched the rule 'arc:upstream' in your args 'base' configuration.

You can see the exact changes that will be sent by running this command:

    $ git diff acf7600e6e728395..HEAD

These commits will be included in the diff:

    3580555e4b30598f  WIP

MATCHING REVISIONS
These Differential revisions match the changes in this working copy:

    (No revisions match.)

Since there are no revisions in Differential which match this working copy, a
new revision will be created if you run 'arc diff'.
```

Reviewers: brennantaylor

Reviewed By: brennantaylor

CC: aran

Differential Revision: https://secure.phabricator.com/D4913
2013-02-11 15:58:47 -08:00
epriestley
acf7600e6e Temporarily restore apache/license linters
Summary:
Restores linters only, without unit tests or entry in ComprehensiveLinter. Marks them deprecated.

If use at Facebook isn't widespread I'd prefer to simply delete them.

Test Plan: none

Reviewers: vrana

Reviewed By: vrana

CC: aran

Maniphest Tasks: T2274

Differential Revision: https://secure.phabricator.com/D4906
2013-02-11 14:12:10 -08:00
vrana
8f0b0d379f Support short version of arc diff --add-all
Summary: Like `git commit -a`.

Test Plan:
  $ arc diff -a

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4903
2013-02-11 10:53:57 -08:00
vrana
f32b6aa6c5 Support short version of arc diff --add-all
Summary: Like `git commit -a`.

Test Plan:
  $ arc diff -a

Reviewers: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4903
2013-02-11 10:48:40 -08:00
vrana
e1d906ea18 Allow configuring PhutilXHPAST linter
Summary: Also move Phabricator stuff outside.

Test Plan: Updated unit test.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4894
2013-02-11 10:41:26 -08:00
Bryan Cuccioli
8b1215ffcf Delete license linters.
Summary: Remove all references to ArcanistLicenseLinter and ArcanistApacheLicenseLinter.

Test Plan: Rerun the linter and ensure nothing is broken.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4901
2013-02-11 09:04:19 -08:00
epriestley
84254f111a Minor, fix an exception variable.
Auditors: DurhamGoode
2013-02-11 04:54:20 -08:00
vrana
6a70964a40 Deprecate phutil_escape_html()
Summary:
My original idea was to return safe HTML from this function.
But we are down to 20 occurrences in Phabricator and you shouldn't need this function in safe HTML world at all.

Test Plan:
  $ arc lint src/applications/audit/controller/PhabricatorAuditListController.php

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4890
2013-02-09 15:18:19 -08:00
Nick Pellegrino
f10f2ffb9c Support arc diff --edit under hg
Summary: T1571

Test Plan: epriestley says it works

Reviewers: epriestley

Reviewed By: epriestley

CC: kwadwon, aran, Korvin

Differential Revision: https://secure.phabricator.com/D4883
2013-02-09 11:24:54 -08:00
epriestley
38d23516d1 Add lint warnings about use of phutil_render_tag and javelin_render_tag
Summary: Raise deprecation warnings for these methods. I won't commit this until the phutil_tag branch merges.

Test Plan: Unit tests.

Reviewers: vrana

Reviewed By: vrana

CC: aran

Maniphest Tasks: T2432

Differential Revision: https://secure.phabricator.com/D4770
2013-02-08 17:38:56 -08:00
vrana
ca37bfb2ab Display other locations of Reused lint errors
Summary: It's not trivial to find them inside 700+ lines long functions.

Test Plan:
Linted `reused-iterators.lint-test` renamed to `_.php`, saw other locations.
Repeated for `reused-local.lint-test`.
Repeated for `duplicate-key-in-array.lint-test`.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4871
2013-02-08 15:49:50 -08:00
durham
4c35af9283 Make 'arc diff X' in mercurial match git by using the GCA of X as the base
Summary:
Previously 'arc diff X' with mercurial meant to use X as the base
to diff against.  Now it means use gca(X,working directory) as the base to
diff against.  This matches the git behavior.

Test Plan:
Ran 'arc diff master' on a repo where master was ahead of the feature branch.
Verified that the diff result included only the diffs in the feature branch.

Reviewers: epriestley, sid0, bos, dschleimer

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4865
2013-02-08 07:09:30 -08:00
durham
db053e1eb7 Improve performance of mercurial arc diff
Summary:
arc diff on large mercurial repos was taking 14 seconds just to get
to the commit message prompt.  With these optimizations it takes 4.

- "ancestor(.) - ancestor(XYZ)" is expensive because it has to build the
entire 400000+ revision history for both.  "XYZ::. - XYZ" is much cheaper
because it only looks at the revisions between XYZ and the working directory.

- "hg outgoing" has to talk to the server, which is slow.  "hg log -r draft()"
gives us the same information and is much cheaper.  We fall back to 'outgoing'
on older versions of mercurial.

Of the remaining 4 seconds, 2.5 are spent in 'hg status', which is a bit harder
improve.

Test Plan: Ran arc diff on our hg repo. Verified it ran faster and the diff was created.

Reviewers: epriestley, sid0, bos, dschleimer

Reviewed By: epriestley

CC: aran, Korvin, tido

Differential Revision: https://secure.phabricator.com/D4838
2013-02-08 05:54:32 -08:00
vrana
7f9c286338 Upload binary files diffs in parallel
Summary: Makes sense after D2471.

Test Plan:
Swapped two binary files, ran `arc diff --only`.
Saw time 3.797 s instead of 4.361 s.

Changed `file.upload` to `file.uploa`, saw proper error.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4847
2013-02-07 17:27:36 -08:00
vrana
46dfc64337 Restore kept branch after landing
Test Plan: Ran it separately.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4846
2013-02-07 17:27:34 -08:00
epriestley
2c2cb3b78a Improve error message for phutil missing symbols
Summary: This is fairly confusing. Make the error message suggest the common remedy (update libphutil).

Test Plan: Eyeballed it.

Reviewers: Afaque_Hussain, btrahan, vrana

Reviewed By: Afaque_Hussain

CC: aran

Differential Revision: https://secure.phabricator.com/D4834
2013-02-06 19:11:06 -08:00
epriestley
d952502f12 Make "arc patch" read authorName and authorEmail
Summary: If provided, have `arc patch` use `authorName` / `authorEmail`. This simplifies handling and makes patches more portable between version control systems (previously, information was generated in the diff's VCS, regardless of which VCS it was being applied to).

Test Plan: Created a diff with author `derp <derp@derp.com>`, ran `arc patch --diff x`, got a local commit with that author.

Reviewers: btrahan, edward, vrana

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4827
2013-02-05 20:11:39 -08:00
epriestley
99feb5c28e Record commit email information from arc
Summary:
Record author email information in `arc diff`, so we can recreate it in `arc patch` and elsewhere without creating any kind of email exposure issues.

In Mercurial, we currently store the whole string ("username <email@domain.com>"). Make this consistent with Git.

Test Plan: Created git and hg diffs, saw authorEmail populated.

Reviewers: btrahan, edward, vrana

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4825
2013-02-05 20:11:20 -08:00
vrana
bd71ba675e Implement hook for checking switch lint
Summary:
We want to use it for `yield` and `invariant_violation()` which throws.
Having node instead of token would be better but this would be enough.

Test Plan: Implemented a hook in FB repo and added a test case there.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4821
2013-02-05 11:46:59 -08:00
epriestley
be73bd8716 Allow arc diff and similar to run on systems with no git
Summary:
We assume `git` is available now, but should not. Specifically, if a user runs a working copy operation like `arc list` in an SVN working copy without `git` available, they get this error: P707

We interpret git errors very narrowly; be more liberal in how we interpret them. This assumes users working with `git` will have a functional `git`, but this seems like a reasonable assumption and lets us remove some error text matching code.

Test Plan: Changed `git` to `girt`, ran `arc list`, saw a reasonable exception. Changed back to `git`, saw git detected.

Reviewers: btrahan, vrana

Reviewed By: vrana

CC: svemir, aran

Differential Revision: https://secure.phabricator.com/D4804
2013-02-04 11:34:53 -08:00
vrana
8e34e2bd03 Fix dynamic string usage as safe input
Test Plan: Copied the code in a script, changed `phutil_passthru()` to `echo csprintf()` and ran it.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4805
2013-02-04 11:34:32 -08:00
vrana
a9e316bf9c Fix dynamic string usage as safe input
Summary: This fixes some real issues.

Test Plan:
  $ arc lint

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, btrahan

Differential Revision: https://secure.phabricator.com/D4795
2013-02-02 16:28:15 -08:00
vrana
03199df925 Lint dynamic string usage in parameters treated as safe
Summary:
This disallows code like this:

  $cmd = 'ls';
  execx($cmd);

But I guess it's not that big deal?

Test Plan: Linted whole Arcanist and Phabricator codebases, most parts looks fixable.

Reviewers: epriestley

CC: nh, aran, Korvin

Differential Revision: https://secure.phabricator.com/D4794
2013-02-02 16:26:52 -08:00
vrana
39cef6cb99 Dispatch event after collecting changes in diff
Summary:
FB currently starts Sandcastle push before starting the diff workflow.
If `arc diff` commits something then we need to restart the push.
I want to avoid this by starting the push after commit.

Test Plan: Will test after implementing the listener.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4785
2013-02-02 12:45:19 -08:00
vrana
5d7c77da72 Add typehint to setting workflow configuration
Test Plan: This diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4784
2013-02-01 11:56:56 -08:00
vrana
8374dbc4e3 Handle $var::method() in XHPAST linter
Test Plan: Didn't see a fatal in new test case.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4783
2013-02-01 11:21:06 -08:00
vrana
cdb01f23a7 Make British spelling stricter
Summary: This is pretty lame but I didn't have a better idea.

Test Plan:
  $ arc test # previously translated as list
  $ arc lst
  $ arc brnach

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4773
2013-01-31 17:14:08 -08:00
vrana
6cd3d8e995 Fix getting changed files in SVN
Summary:
This is pretty awkward but I don't have anything better.

Also don't compute cache key if caching is disabled.

Test Plan:
  $ svn diff --xml --summarize '' -r 701319:HEAD
  $ svn diff --xml --summarize svn+ssh://tubbs/svnroot/projects/lolbunny -r 701319:HEAD

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4771
2013-01-31 13:47:59 -08:00
vrana
6d521f3b3f Disable pull stat in arc land
Test Plan:
  $ git pull --no-stat

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4754
2013-01-31 11:54:38 -08:00
Nick Harper
dff1342efc Add --skip-arcconfig option
Summary:
Continuation of D4732 - when we don't care about loading an arcconfig,
allow that to be specified.

Test Plan: chmod -r .arcconfig; bin/arc help --skip-arcconfig

Reviewers: epriestley, vrana

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4750
2013-01-30 16:26:41 -08:00
vrana
3f747be644 Move isConstantString() to base class
Summary: This can be useful by itself, we want to use it in FB linter.

Test Plan: This diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4744
2013-01-30 12:56:59 -08:00
vrana
98e8d0c9c4 Split php-tags-bad.lint-test to two tests
Summary: D1818 effectivelly disabled this test case.

Test Plan: This diff.

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4746
2013-01-30 12:46:30 -08:00
epriestley
3acbf9f3fa Expose --coverage and --no-coverage flags from arc diff
Summary: Currently, we don't expose these at top level, so you can't disable coverage if your coverage is explosively broken. Expose them as passthrough arguments.

Test Plan:

  - Touched a file in `arc` which triggered unit tests.
  - Without `xdebug` installed:
    - Ran `arc diff --preview`, `arc diff --preview --no-coverage` (both fine).
    - Ran `arc diff --preview --coverage`, got exception about coverage not being available.
  - Installed `xdebug`.
    - Ran `arc diff --preview`, got coverage.
    - Ran `arc diff --preview --coverage`, got coverage.
    - Ran `arc diff --preview --no-coverage`, no coverage.

Reviewers: indiefan, btrahan, vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4745
2013-01-30 12:35:30 -08:00
vrana
9746d2b2a1 Use all changed files since base revision in repository cache key
Test Plan: Added debug output for repository version, linted, committed, linted again, saw the same version, changed file, saw different version.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4731
2013-01-30 11:25:16 -08:00
epriestley
51f32bdde7 Restore directory after executing ArcanistBundleTestCase
Summary:
This test currently chdir()'s into a directory which is later removed. If another test tries to run a shell script while the CWD is invalid, the shell may emit this to stderr:

  shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory

Among other things, this can cause the XHPAST test to fail, because it detects syntax errors by examining stderr.

Instead, retore the directory.

Test Plan: Ran "arc unit --everything", which could previously fail if XHPAST ran after Bundle.

Reviewers: btrahan, vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4738
2013-01-30 11:24:41 -08:00
vrana
c45053da4c Reset master after unsuccessful push
Test Plan: None.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4717
2013-01-29 12:12:31 -08:00
vrana
5aa3bc6ec0 Separate Phutil specific lint rules from XHPAST
Summary: Also provides an example how to build custom linter using XHPAST.

Test Plan: Added debug output to `willLintPaths()`, verified that each path is parsed only once.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4718
2013-01-29 10:54:40 -08:00
vrana
0586b12d2f Allow running arc land after git svn rebase
Summary: `git pull` may fail in git-svn after rebasing (which is a side effect of dcommit).

Test Plan:
  $ git svn rebase
  $ git log trunk..master
  $ git pull --ff-only; echo $?

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4716
2013-01-28 17:47:17 -08:00
epriestley
410b58a2e2 Correct a bunch of mercurial revset construction
Summary: Possibly improves T2387 state of the world?

Test Plan: not yet

Reviewers: bos, DurhamGoode

Reviewed By: DurhamGoode

CC: aran

Maniphest Tasks: T2387

Differential Revision: https://secure.phabricator.com/D4712
2013-01-28 16:28:14 -08:00
epriestley
2613ea196f Provide hgsprintf() for building hg revsets
Summary: I guess this is correct? See T2387 for discussion.

Test Plan: Unit tests.

Reviewers: bos, DurhamGoode

Reviewed By: DurhamGoode

CC: aran

Maniphest Tasks: T2387

Differential Revision: https://secure.phabricator.com/D4711
2013-01-28 16:27:47 -08:00
vrana
fd909479d0 Ignore untracked files in lint repository cache key
Summary: Some people have 2GB+ untracked files in repo which significantly slows down this or even crashes it.

Test Plan: Added a debug output here and linted repo with untracked path.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, arudolph

Differential Revision: https://secure.phabricator.com/D4713
2013-01-28 15:40:47 -08:00
vrana
edd585a3d7 lint_untracked 2013-01-28 15:15:18 -08:00
epriestley
53691cdcf9 Fix an escaping issue with "svn commit"
Summary: Fixes T2438. We currently escape everything with '@', but SVN rejects that for '.'

Test Plan:
Unit tests. Performed this commit:

  $ svn st
   M      .
  A       x@123
  $ arc commit --conduit-uri=http://local.aphront.com:8080/ --revision 53

      Revision 'D53: asdf' has not been accepted. Commit this revision anyway?
      [y/N] y

  Committing 'D53: asdf'...
  Sending        .
  Adding         x@123
  Transmitting file data .
  Committed revision 37.
  Done.

I grepped for more '@' adding but couldn't find any. It's a bit tricky to grep for though, so it's possible I missed some.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran, mbishopim3

Maniphest Tasks: T2438

Differential Revision: https://secure.phabricator.com/D4703
2013-01-28 14:11:31 -08:00
indiefan
ce0a491bcd Added base test result parser class. Implemented Go test result parser based on Go's built in test utility output.
Test Plan: Ran PhpunitTestEngine unit test and used both test result parsers to generate test results.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, aurelijus

Differential Revision: https://secure.phabricator.com/D4676
2013-01-27 17:17:09 -08:00
Jesse Mullan
c49997a6e8 Added paths to search for python linters
Summary: Added paths to search for python linters

Test Plan: Linted in ubuntu

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2414

Differential Revision: https://secure.phabricator.com/D4648
2013-01-26 05:32:26 -08:00
Aurelijus
df04c79a90 Support PHPUnit 3.7 message changes
Summary: PHPUnit 3.7 now includes user message as well.

Test Plan: Ran phpunit tests

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4662
2013-01-25 17:19:41 -08:00
epriestley
44e25495ca Raise a lint warning on array_combine(x, x)
Summary: Suggest array_fuse().

Test Plan:
Unit tests, and:

  $ arc lint --lintall src/unit/engine/phutil/ArcanistPhutilTestCase.php
  >>> Lint for src/unit/engine/phutil/ArcanistPhutilTestCase.php:

     Warning  (XHP37) array_combine() Unreliable
      Prior to PHP 5.4, array_combine() fails when given empty arrays. Prefer
      to write array_combine(x, x) as array_fuse(x).

               252     $callable,
               253     $exception_class = 'Exception') {
               254     return $this->tryTestCases(
      >>>      255       array_combine(array_keys($map), array_keys($map)),
               256       array_values($map),
               257       $callable,
               258       $exception_class);

Reviewers: btrahan, vrana, chad

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4659
2013-01-25 17:06:49 -08:00
indiefan
9946e23f9e Decoupling phpunit test parsing from running and gathering. Should enable third party test engines to reuse common business logic around phpunit without requiring common logic for the typically application-specific running and gathering.
Test Plan: Ran PhpunitTestEngine unit test. Also used refactored PhpunitTestEngine to run phpunit tests.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, aurelijus

Differential Revision: https://secure.phabricator.com/D4651
2013-01-25 17:06:38 -08:00
durham
c8efb41811 Fix hg arc land on hg-svn repos
Summary:
arc land on a hg-svn repository would fail because arc land
uses 'hg push -r' to specify which revs to push which is not supported
by hg-svn. Now we just use 'hg push' which, when used against svn, only
pushes the current branch (which happens to be the branch we're trying to land).

We can't use standard 'hg push' for non-svn repos though because when used
against a vanilla hg repo 'hg push' pushes all branches.

Also remove --new-branch from 'hg push' because it's extremely
unlikely that a person wants to create a new branch on the server via
arc land.

Test Plan:
Ran arc land on a normal hg repo, verified it used 'hg push -r'.
Ran arc land on a hg-svn repo, verified it used 'hg push' and it pushed the
correct changes.

Reviewers: epriestley, sid0, dschleimer

Reviewed By: epriestley

CC: bos, aran, Korvin

Maniphest Tasks: T2403

Differential Revision: https://secure.phabricator.com/D4653
2013-01-25 14:00:52 -08:00
vrana
93ebde0d12 Revert "Delete useless code in ArcanistBaseWorkflow"
This reverts commit rARC501e2b7.
We call this method from FB workflow.
I'll commit it again in two weeks.
2013-01-24 16:33:32 -08:00
vrana
a1b902b190 Cache results of repository linters
Summary:
The cache key is repository base revision and hash of all modified files.
It isn't perfect in SVN where every file might have a different revision.
It's also suboptimal as just committing or amending changes the cache key even if the files contents weren't modified.
We can improve it later, perhaps by using previous revision and files modified since it.

Test Plan:
Changed granularity of XHPAST linter to repository.
Linted the same repo twice, verified that it was read from cache the second file.
Changed a single file, verified that all files were re-linted.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4608
2013-01-24 16:15:55 -08:00
vrana
e56f47aed5 Don't bail on no effect exception
Summary: The main reason for this is to not exit with 1 when no paths are lintable (which is more success than failure).

Test Plan:
Returned empty array from `buildLinters()`, then:

  $ arc lint
  $ echo $? # 0

Reviewers: epriestley

Reviewed By: epriestley

CC: wez, aran, Korvin

Differential Revision: https://secure.phabricator.com/D4625
2013-01-24 16:07:50 -08:00
epriestley
aec212069e Fix arc diff --no-ansi
Summary: Currently, we get an exception on empty %Ls for `arc diff --no-ansi` or similar (see P698).

Test Plan: Ran `arc diff --no-ansi`.

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4624
2013-01-24 14:28:28 -08:00
vrana
d3a6e456a6 Mark deleted paths as removed in committing
Test Plan: Deleted file in Git, ran `arc diff`, confirmed the question, saw the file as deleted.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4603
2013-01-23 14:42:49 -08:00
vrana
501e2b7e1d Delete useless code in ArcanistBaseWorkflow
Test Plan: This diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4601
2013-01-23 13:22:33 -08:00
Asher Baker
087ad5f84a Added support for hg diffs generated in git mode
Summary: Fixes T2112. These are fairly common now, and are used as the storage format for `hg export` and mq in most installs.

Test Plan: Ran unit tests, used `arc patch --patch`, uploaded some diffs manually.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2112

Differential Revision: https://secure.phabricator.com/D4592
2013-01-22 18:14:14 -08:00
epriestley
7f93b7de02 Don't use xhpast binary to compute xhpastlinter cache version if the binary doesn't exist
Summary: The binary may not be built, in which case this raises a warning.

Test Plan: Will make @zeeg test.

Reviewers: zeeg, vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4569
2013-01-22 17:16:37 -08:00
durham
880964af83 Allow hg arc land when no rebase is necessary
Summary:
Previously, trying to arc land in a mercurial repo would
fail if the local branch was already at the tip of the onto branch
(since hg rebase exited with code 1).  This change makes it check
if a rebase is needed before executing the rebase.

Test Plan:
hg init foo
cd foo
hg bookmark master
touch a && hg add a && hg commit -ma
// setup your .arcconfig
cd ..
hg clone foo foo2
cd foo2
hg bookmark mybook
touch b && hg add b && hg commit -mb
arc land --onto master --revision <your rev number>

Arc land should succeed.  I also tried landing when a rebase was
necessary and it still worked.

Reviewers: epriestley, dschleimer, bos

Reviewed By: epriestley

CC: sid0, aran, Korvin

Differential Revision: https://secure.phabricator.com/D4588
2013-01-22 15:20:10 -08:00
vrana
98fec27752 Fix PHT linter
Summary: Variables in strings and concatenation lists.

Test Plan:
New unit tests.

  preg_match('/^((?>[^$\\\\]*)|\\\\.)*$/s', str_repeat('a', 1e6));

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4540
2013-01-19 11:48:17 -08:00
vrana
78017033bd Warn against usage of nowdoc
Summary: Also include binary hash in the version.

Test Plan: New unit test.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4535
2013-01-19 09:23:17 -08:00
vrana
85c0ed13d9 Set used variable 2013-01-18 13:32:48 -08:00
Nick Harper
40102252bf Fix arc diff when run from a git submodule
Summary:
A git submodule looks a lot like a normal git repo, but the .git
directory is replaced with a file that git reads to find the real
location of the git directory. When arcanist tries to write a file into
a directory inside of there, it was failing silently, and then crashing
silently when it couldn't read results back out. Instead of assuming the
git directory is a directory named .git at the toplevel of the tree, we
use the appropriate git command to get the correct git directory.

Test Plan: submit a diff from a submodule

Reviewers: epriestley, vrana

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4482
2013-01-17 11:28:13 -08:00
epriestley
a39b591c95 Allow pht(<<<HEREDOC ... HEREDOC)
Summary: Don't raise lint on heredoc literals.

Test Plan: Linted pht() + heredoc in D4477.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4478
2013-01-16 14:57:10 -08:00
vrana
90129c5432 Include real PEP8 version and options in lint cache key
Summary: This should be done for all external and configurable linters.

Test Plan: Linted file with lint problems, changed options, relinted.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4475
2013-01-16 12:17:11 -08:00
vrana
8142aab9d3 Fix whitespace around else 2013-01-16 12:15:46 -08:00
vrana
f25c01606b Cache stopped paths in lint
Summary:
If linter with file cache granularity stops linting then we don't run it on the same path next time.
But we still run linters with non-file cache granularity causing that they run even on the paths that would be stopped otherwise.

Test Plan:
Put global granularity linter after Generated linter.
Caused an error from this linter but in ignored path.
Verified that 'stopped' is saved in `lint-cache.json`.
Linted the same files second time, verified that the path is still skipped (wasn't before).

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4446
2013-01-15 17:58:32 -08:00
vrana
d4359a838a Use assert_instances_of(, 'array')
Summary: To have at least one real callsite.

Test Plan:
  $message = new ArcanistLintMessage();
  $message->setOtherLocations(array());
  $message->setOtherLocations(array(array()));
  $message->setOtherLocations(array(1));

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4419
2013-01-14 11:36:29 -08:00
vrana
e8decd2062 Display other locations of lint errors in console renderer
Test Plan: Created function named `f_a`, manually set other location.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4412
2013-01-11 15:25:58 -08:00
vrana
ff73a90482 Add hook after running linters
Summary: This resolves D4380#7 by reverting https://secure.phabricator.com/D4380?vs=9112&id=9123.

Test Plan: Used it in our linters.

Reviewers: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4409
2013-01-11 14:15:04 -08:00
vrana
5c5d347544 Format whitespace in XHPAST linter
Test Plan: Linted a method named `f_a`.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4405
2013-01-11 13:23:21 -08:00
vrana
a82493a84d Allow specifying multiple locations of the same lint error
Summary:
Some errors (duplicate declaration, invalid number of arguments) have more related places.
We need to notify user if he changes any related place.
This could be currently achieved by triggering errors instead of warnings or by including both files in the range (impossible if the locations are in different files) or by issuing multiple errors.
All options are too aggressive.

Test Plan: Issued error on unmodified line with other location on modified line.

Reviewers: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4392
2013-01-10 15:35:11 -08:00
vrana
80cd881fb1 Fix checking for lint binary paths
Summary: Like D4379.

Test Plan:
  $ arc unit

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4391
2013-01-10 15:34:42 -08:00
Gregor Stocks
767f9457c1 Add some new misspellings of "hierarchy"
Summary: turns out people really like misspelling "hierarchy"

Test Plan: hope
2013-01-10 14:32:23 -08:00
vrana
35de4a5d1f Run all willLintPaths() before any lintPath()
Summary:
FB runs some linters on background and it's a magnificent hack using `ParallelLinter` (two instances), `BackgroundLinter` and `FutureLinter`.
I want to simplify this by resolving the futures in engine instead of in some virtual linter.
It also seems like a better place to do it.
It should also fix caching problems I have with them (because the virtual linters don't know about the cache at all).

Test Plan: None yet.

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4380
2013-01-09 17:15:31 -08:00
epriestley
38e2e1336f Generate Git patches with "\n" metadata line endings unconditionally
Summary:
Fixes T2175. Git generates patches which have "\n" line endings on every system. We currently generate patches with system-dependent line endings.

Git accepts system-dependent line endings in almost call cases, but part of the parser tests for "\n" explicitly. T2175 has an example of this.

Test Plan:
Ran `arc export --git --revision D4366 > export.git` on a Windows machine, verified "\n" line endings. Ran the same with `--unified`, verified "\r\n" line endings.

(I didn't add any unit tests for this because it's Windows-dependent and very difficult to test meaningfully right now -- i.e., test that appliable patches are generated -- since the git reconstitution test doesn't run on Windows either, because we can't yet untar things there.)

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2175

Differential Revision: https://secure.phabricator.com/D4373
2013-01-09 13:51:51 -08:00
vrana
983537e620 Fix checking for flake8 path
Test Plan:
  $ arc unit # without flake8 in path

Reviewers: zeeg

Reviewed By: zeeg

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D4379
2013-01-09 13:41:10 -08:00
epriestley
d399354822 Normalize Windows directory separators in SVN
Summary: Fixes T1946.

Test Plan: Ran `arc diff --only` with added paths inside directories. Saw only "/" entries in the diff.

Reviewers: btrahan, vrana

Reviewed By: vrana

CC: aran, mbishopim3

Maniphest Tasks: T1946

Differential Revision: https://secure.phabricator.com/D4374
2013-01-09 12:34:37 -08:00
epriestley
57ec5a026d Fix arc diff x y in SVN
Summary:
D4186 added an "svn status --xml x y" form to getSVNStatus(), but the parser doesn't work for multiple files, since we get multiple <target /> elements in the XML output. So, curently, `arc diff` works (one target, all files) and `arc diff x` works (one target, x) but `arc diff x y` does not (more than one target, hits the exception).

  $ arc diff QUACK2 QUACK3
  Exception
  Expected exactly one XML status target.

Test Plan: Ran `arc diff QUACK2 QUACK3` in a working copy with modified QUACK2, QUACK3. Ran `arc diff`; `arc diff QUACK2`.

Reviewers: vrana, btrahan, codeblock, JThramer

Reviewed By: codeblock

CC: aran

Differential Revision: https://secure.phabricator.com/D4372
2013-01-09 09:11:26 -08:00
epriestley
ea1585d7fa Fix "empty diff" error in arcanist
Summary: Ref T2296. This error is unreachable right now -- when I fixed all the "\r\n" stuff, we always end up with a nonempty first line for an empty input. Do this test earlier and more explicitly. This results in a less useful error: "expected (some junk) on line 1" instead of "can't parse an empty diff".

Test Plan: Tried to parse an empty diff, got a "you can't parse an empty diff" error.

Reviewers: btrahan, vrana, codeblock

Reviewed By: codeblock

CC: aran

Maniphest Tasks: T2296

Differential Revision: https://secure.phabricator.com/D4370
2013-01-09 08:15:53 -08:00
Ricky Elrod
a5ddd7ebc0 Add a comma.
Summary: Add a comma because it was going to annoy the crap out of me.

Test Plan: See the comma. :)

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4365
2013-01-08 15:51:36 -08:00
Roy Williams
7547ef7e96 Pass the constructed message to the TYPE_DIFF_DIDBUILDMESSAGE event.
Summary: We want to use TYPE_DIFF_DIDBUILDMESSAGE to abort arc diff when a message doesn't fit some

Test Plan: Created an EventListener subscribed to TYPE_DIFF_DIDBUILDMESSAGE, validated the 'message' field was filled in

Reviewers: vrana, epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D4361
2013-01-08 15:06:46 -08:00
Jack Lindamood
48f5ecb05c Fix cppcheck path finding
Summary: This fix lets you run arc lint from any directory in the repository

Test Plan: Ran arc lint from any directory

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4356
2013-01-07 12:47:00 -08:00
epriestley
72b2b7b22c Limit arc branch to 16 concurrent processes
Summary:
Currently, this spawns 125 concurrent processes on my machine, which overflows some limit and gives me an error:

  PHP Warning:  proc_open(): unable to create pipe Too many open files in /INSECURE/devtools/libphutil/src/future/exec/ExecFuture.php on line 491

Instead, limit parallelism to 16. The runtime is approximately the same for me, and dominated by other concerns (conduit calls).

Test Plan: Ran `arc branch` successfully. Ran `arc branch --trace`, observed behavior.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4336
2013-01-07 08:44:55 -08:00
Jack Lindamood
cf8d445c9f CppCheck linter
Summary:
Lints cpp code using the cppcheck static linter.  This linter needs to
be downloaded and built from http://cppcheck.sourceforge.net/

Test Plan: Used it on a few files.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4353
2013-01-07 08:18:33 -08:00
Jack Lindamood
854c1b67b1 Add cpplint.py engine
Summary: Adds arc lint support for cpp files with Google's cpplint.py lint checking.

Test Plan: ran it on some cpp files. Added unit tests

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4344
2013-01-07 08:16:34 -08:00
Bob Trahan
0c8fa90040 make arc install-certificate <uri> work if uri is specified
Summary: see title

Test Plan: ran arc install-certificate <uri> in a directory without an .arcconfig and it worked! ran arc install-certificate in a directory with an .arcconfig and it worked! ran arc install-certificate <uri> in a directory with an .arcconfig and noted it correctly overrode the .arcconfig

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2251

Differential Revision: https://secure.phabricator.com/D4332
2013-01-03 09:23:20 -08:00
epriestley
59e13f666f Correct spelling for "arc" commands
Summary:
I type "arc brnach" about 300 times per day.

  - Allow arc commands to be specified by unique prefix ("exp" for "export", "lib" for "liberate").
  - Allow arc commands to be specified by unique levenshtein edit distance <= 2 ("brnach" for "branch", "halp" for "help", "ptach" for "patch").
  - Reorganize code out of "arcanist.php".

I think this will be uncontentious because arc commands are rarely destructive, but if people complain we can either require certain commands be typed exactly (maybe "land"?) or allow this feature to be disabled in configuration.

Test Plan:
  $ arc br
  Usage Exception: Unknown command 'br'. Try 'arc help'.

  Did you mean:
      branch
      browse

  $ arc brnachh
  Usage Exception: Unknown command 'brnachh'. Try 'arc help'.
  $ arc brnach
  (Assuming 'brnach' is the British spelling of 'branch'.)
     doc-security        No Revision      security
     sms                 Needs Revision   D319: Add SMS support to Phabricator
     phxtag              No Revision      derp
     arantag             No Revision      derp
  ...

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4305
2012-12-30 16:01:59 -08:00
David Cramer
0b833c2f02 Correct description and severity on Flake8 linter
Test Plan: do it live

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4274
2012-12-21 16:13:33 -08:00
David Cramer
4ca70d4e48 Add a flake8 linter
Summary: flake8 is the better maintained combination of pep8 and pyflakes

Test Plan: There's a test!

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, jack

Differential Revision: https://secure.phabricator.com/D4082
2012-12-21 15:28:26 -08:00
epriestley
cffe1942ec Improve arc land recovery from issues with the remote.
Summary:
Fixes T2138.

  - When a pull fails, restore the original branch.
  - When a push fails, complain about it really loudly.

NOTE: No test plan for push yet since I'm not sure this is the right remedy, see T2138 for discsusion.

Test Plan:
  - Tested pull by changing "git pull" to "git xxpull" and running "arc land". Saw the pull fail and my original branch restored.

Reviewers: vrana, aran

Reviewed By: vrana

Maniphest Tasks: T2138

Differential Revision: https://secure.phabricator.com/D4265
2012-12-21 14:18:07 -08:00
vrana
f830b3bf3f Don't require clean working copy for arc diff path in SVN
Summary: If you are explicit then there is no need to ask you.

Test Plan:
  $ touch a
  $ arc diff
  $ arc diff a
  $ arc diff existing

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4186
2012-12-21 12:34:06 -08:00