1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-12-29 17:00:58 +01:00
Commit graph

864 commits

Author SHA1 Message Date
vrana
cd50b0884e Don't run disabled lint rules in other linters.
Summary: D4963 for other linters.

Test Plan: Saw time 0.001 instead of 0.113 in spellcheck linter.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4965
2013-02-14 15:54:10 -08:00
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