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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary: If you dump `arcanist/` into "Program Files" or similar, the `.bat` currently breaks. Quote the path so it doesn't. Fixes T1436.
Test Plan: Ran `arc` with `arcanist/` in a path with spaces in it.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1436
Differential Revision: https://secure.phabricator.com/D4375
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
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
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
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
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
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
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
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
