Summary:
This adds basic support for mutable history for arc diff and arc amend for
mercurial. This is purely opt-in (so it shouldn't affect anyone who doesn't want
this feature) by explicitly setting
"immutable_history" : false
in arc configuration.
This also fixes another instance of weird behaviour for multiple heads - the
first instance was fixed here:
https://github.com/facebook/arcanist/pull/28
Test Plan:
without "immutable_history" turned on)>
When ##arc diff## produces an update diff, it should list only commits that are
ancestors of the current revision - not ones from other heads.
Reviewers: epriestley
CC: csilvers, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2654
Summary:
Currently, when a file has a syntax error and you run "arc liberate", the symbol analyzer will fail and throw, which will make the mapper fail and throw, which will make arc fail and throw. This gives you a stack trace pile three scripts deep which is a giant pain to analyze visually.
Instead, raise a clear message.
Test Plan: Ran "arc liberate" with a syntax error. Got useful diagnostic output instead of 30 pages of stack mess.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2659
Summary:
We added "repeat" to PhutilArgumentParser a while ago, so we can modernize the rest of the argument parsing (allow multiple --load-phutil-library flags).
Add a "--conduit-timeout" flag to allow users to set the timeout. See D2602.
Test Plan:
$ arc diff --conduit-timeout=0.01
Exception
[cURL/28] <CURLE_OPERATION_TIMEOUTED> The request took too long to complete.
(Run with --trace for a full exception trace.)
$ arc diff --conduit-version 33
Exception
ERR-BAD-VERSION: Your 'arc' client version is '33', which is newer than the server version, '5'. Upgrade your Phabricator install.
(Run with --trace for a full exception trace.)
$ arc diff --conduit-uri http://derp.derp/
Usage Exception: YOU NEED TO INSTALL A CERTIFICATE TO LOGIN TO PHABRICATOR
You are trying to connect to 'http://derp.derp/api/' but do not have a certificate installed for this host. Run:
$ arc install-certificate
...to install one.
$ arc diff --load-phutil-library src --load-phutil-library ../phabricator/src --trace
Loading phutil library '0' from '/INSECURE/devtools/arcanist/src'...
Loading phutil library '1' from '/INSECURE/devtools/phabricator/src'...
Reviewers: phleet, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2603
Summary:
It makes me nervous.
Also move them to the dir.
Test Plan:
`arc lint`
`arc lint --output json`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2644
Summary: We don't support `git rebase -i` or other magic in `arc amend` so we can examine just HEAD which we always amend.
Test Plan: `arc amend` on a branch with two commits.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2643
Summary:
If a message has only "line", or "line" and "char", we don't point out the location in the context block.
When a message includes "line", show chevrons on the line.
When a message includes "line" and "char", show chevrons on the line and a caret on the next line.
Test Plan: Ran regex linters to capture line, line+char, and normal linters to capture everything. Output appeared sane.
Reviewers: csilvers, vrana, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2642
Summary:
`$this->getPaths()` gives us paths relative to repository root.
We resolve them relative to cwd.
Test Plan: [src] `arc unit difference`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2634
Summary:
The theory is that lint and unit errors are rare with no false positives.
Our reality is currently different.
This diff removes the question for providing explanation before actually providing it.
I hope that the wording is not confusing.
It also simplifies the code with less copy/paste errors (not me!) potential.
Test Plan:
`arc diff` on code with lint and unit errors.
Repeat with `--excuse .`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, meitros
Differential Revision: https://secure.phabricator.com/D2631
Summary: I wonder if this is not by purpose?
Test Plan: Modified two files, ran `arc lint`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2629
Summary:
Khan Academy is looking into lint configuration, but doesn't use ".arcconfig" because they have a large number of repositories. Making configuration more flexible generally gives us more options for onboarding installs.
- Currently, only project config (".arcconfig") can load libraries. Allow user config ("~/.arcrc") to load libraries as well.
- Currently, only project config can set lint/unit engines. Allow user config to set default lint/unit engines.
- Add some type checking to "arc set-config".
- Add "arc set-config --show".
Test Plan:
- **load**
- Ran `arc set-config load xxx`, got error about format.
- Ran `arc set-config load ["apple"]`, got warning on running 'arc' commands (no such library) but was able to run 'arc set-config' again to clear it.
- Ran `arc set-config load ["/path/to/a/lib/src/"]`, worked.
- Ran `arc list --trace`, verified my library loaded in addition to `.arcconfig` libraries.
- Ran `arc list --load-phutil-library=xxx --trace`, verified only that library loaded.
- Ran `arc list --trace --load-phutil-library=apple --trace`, got hard error about bad library.
- Set `.arcconfig` to point at a bad library, verified hard error.
- **lint.engine** / **unit.engine**
- Removed lint engine from `.arcconfig`, ran "arc lint", got a run with specified engine.
- Removed unit engine from `.arcconfig`, ran "arc unit", got a run with specified engine.
- **--show**
- Ran `arc set-config --show`.
- **misc**
- Ran `arc get-config`.
Reviewers: csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2618
Summary:
Move away from setModule(), to setPathPrefix(). Also simplify test location/selection.
Depends on D2621.
Test Plan: Ran "arc unit".
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2622
Summary:
- The library linter works fine on v1 or v2 libraries, so switch to it for all libraries.
- "arc liberate" on v1 libraries will still invoke the Module linter, and thus generate __init__.php files.
Test Plan: Ran "arc lint". Grepped for module linter.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2620
Summary: Mechanical changes from D2588. No "Class.php" moves yet because they aren't necessary for libraries to function.
Test Plan: See D2588.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2589
Summary:
- "arc liberate" now works for v1 or v2 libraries.
- "arc liberate --upgrade" converts a v1 to a v2.
- We delete __init__.php files but do not move Class.php files, since this is vastly simpler. Authors can do this on their own if they want. We'll do it separately.
- v2 has less lint support than v1, but I think we can move first and migrate lint support later. Much of the v1 lint is bad anyway.
Test Plan: Upgraded libphutil, arcanist and phabricator to v2
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2588
Summary: We recover from Conduit exceptions, but not from transport exceptions or other general classes of error here. Recover from everything, and add an explicit flag to skip uploads.
Test Plan: Added a "throw", created a diff, skiped upload. Removed throw, created a diff with --skip-binaries. Created a diff normally.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2598
Summary:
Adds a linter for v2 libraries which raises the relevant errors.
NOTE: Not hooked up anywhere yet, so this diff has no effect.
Test Plan:
Switched the ModuleLinter to LibraryLinter and ran it with a junk block to trigger errors:
>>> Lint for src/lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php:
Error (PHL3) One Class Per File
File 'lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php' mixes
function (id) and class/interface (ArcanistPhutilLibraryLinter)
definitions in the same file. A file which declares a class or an
interface MUST declare nothing else.
190 }
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Error (PHL2) Duplicate Symbol
Definition of function 'id' in
'lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php' in library
'arcanist' duplicates prior definition in 'utils/utils.php' in library
'phutil'.
190 }
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Error (PHL1) Unknown Symbol
Use of unknown class 'XYZ'. This symbol is not defined in any loaded
libphutil library.
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2597
Summary:
For `PhutilSymbolLoader` queries which include `setAncestorClass()`, we need the map of which classes/interfaces things extend/implement to issue the query efficiently.
Without this map, we need to load //every// class/interface and then do `is_subclass_of()`. This is doable, but not very performant if we don't have php-fpm warmup. There are a few performance-sensitive interfaces where we run queries like this, including some in Arcanist, where we'll never have warmup.
This map isn't particularly difficult to generate or maintain, so just include it in symbol generation and in the library map.
Also set parallelism with a flag, since it was arbitrarily hard-coded and adding flags is easy. 8 actually seems approximately optimal on my machine at least, though.
Test Plan: Ran "phutil_rebuild_map.php", opened library map, got a reasonable extension map.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2585
Summary:
Modernize `phutil_mapper.php` to prepare for killing `__init__.php`.
The current mapper is module-oriented and complex. Instead, make the mapper file-oriented and simpler.
We build a one-to-one cache of file content to symbols (built with `phutil_symbols.php`) and then write a simpler map. See some discussion in D2561.
Also make the script less messy/bad in general. It may be useful to compare this to phutil_mapper.php.
(Additionally, we now write versions into the library map and cache.)
NOTE: Nothing can read this new map right now, of course.
Test Plan: Ran "phutil_rebuild_map.php src/" in phabricator/ with --quiet, --drop-caches, etc. Verified cache file, cache behavior, and generated map output.
Reviewers: vrana, nh, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2562
Summary:
We have a linter that needs to read data from the repo that isn't in the commit,
so that data isn't part of the svn transaction, thus not available in svnlook
cat --transaction. This change provides a method to get data from a committed
file.
Test Plan:
Successfully ran arc svn-hook-pre-commit with a linter that needs something
not in the transaction.
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2582
Summary:
One line is usually more than enough for these excuses.
The excuse is quite often the same as in past so history is very useful.
Prompting user right below the errors is better than writing them below prompt.
Test Plan:
Produce a lint error.
Provide an empty explanation.
Provide a non-empty explanation.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2580
Summary:
With the current order of the include_path (checking in the parent dir of
arcanist last), it is possible to load the wrong libphutil which can have
bad side effects. Instead, the first place we check in include_path should
be the parent dir of arcanist.
(The issue I ran into is that I had a checkout of libphutil in my homedir,
and I was running arc from my homedir with --load-phutil-library to load
the libraries, and since ./ is the default value for include_path, we were
loading libphutil from my homedir instead of from alongside the copy of arc
that I was running. The libphutil alongside that copy of arc worked, but my
checkout of libphutil had D2545 in it, so it was broken.)
Test Plan:
ran arc (not in my homedir) with a broken libphutil in my homedir in my homedir
and it worked (before this change it didn't)
Reviewers: jungejason, vrana, epriestley
Reviewed By: vrana
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2576
Summary: I am trying to stop using `--verbatim` and miss this information.
Test Plan: This diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2575
Summary:
`arc diff` adds a space after each field in template.
It is later removed for the last field.
Test Plan: This diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2564
Summary:
Simple wrapper for PHP_CodeSniffer.
You need to have PHP_CodeSniffer and it's dependencies installed.
Test Plan: - Try running it with your custom lint engine
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2560
Summary: On windows paths are separated with \.
Test Plan: - Run filename linter on windows (or some path with \)
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2559
Summary: Git supports this feature (https://git.wiki.kernel.org/index.php/Aliases), it's fairly simple to implement, and gives us a little more ammunition to dissuade users with unusual workflow requirements from actually requesting features.
Test Plan: Defined the alias described in the documentation. Ran "arc ls", "arc ls -alh", etc.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2552
Summary:
there are two problems making the ArcanistSvnHookPreCommitWorkflow not working.
- pathExists($path) will always return false because it hasn't loaded the path yet. Because of this, PhutilLintEngine will unset the path at https://secure.phabricator.com/diffusion/ARC/browse/master/src/lint/engine/phutil/PhutilLintEngine.php;032b9b30b0721c3f$46, so ArcanistSvnHookPreCommitWorkflow will have no path to lint against and never detects a problem.
- In ArcanistSubversionHookAPI::getCurrentFileData(), the $path is already the full path in svnroot (path got from https://secure.phabricator.com/diffusion/ARC/browse/master/src/workflow/svn-hook-pre-commit/ArcanistSvnHookPreCommitWorkflow.php;032b9b30b0721c3f$72). Prepending the project root to it will make the file path to be wrong so that the file content is empty. Again, ArcanistSvnHookPreCommitWorkflow never detects a problem.
Test Plan:
changed --transaction to --revision in related code and manually ran the following command. It detected the syntaxt error correctly. 558937 is a revision with syntax error.
/usr/local/bin/php -f /home/jungejason/nfs_devtools/arcanist/bin/arc svn-hook-pre-commit --load-phutil-library=/tmp/testwww/www/lib/arcanist --load-phutil-library=/home/jungejason/nfs_devtools/facebook/src /svnroot 558937 2>&1
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, Girish, Koolvin
Differential Revision: https://secure.phabricator.com/D2485
Summary:
It's easy to forget to do this when writing a new lint engine (like I recently
just did), so I added it to the example to improve documentation on how to write
a lint engine. This code is copied from PhutilLintEngine (as well as many
others).
Test Plan: php -l
Reviewers: jungejason, vrana, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2553
Summary: If you develop in "master" and try to "arc land", you get an error. You can reasonably "arc amend" in this situation, most of the time. Give the user an explicit pointer.
Test Plan:
$ arc land master
Usage Exception: You can not land a branch onto itself -- you are trying to
land 'master' onto 'master'. For more information on how to push changes, see
'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the
documentation. You may be able to 'arc amend' instead.
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2547
Summary:
More & more use cases come up with empty suite names, guessing
and making test names nice is mess. Will leave it as it is, except
data set part, as it could be super long.
Test Plan: - Try running some phpunit tests with & without data sets
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2544
Summary: Request from @csilvers, whose team is alergic to $EDITOR.
Test Plan:
Adding reviewers and CCs to this diff via CLI. The initial commit message for this diff is:
Add "--reviewers" and "--ccs" flags to arc diff
Request from @csilvers, whose team is alergic to $EDITOR.
Tested: Adding reviewers and CCs to this diff via CLI. The initial commit message for this diff is:
(...infinite recursion omitted...)
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2538
Summary:
- When you run "arc diff" in Mercurial, we currently give you an empty template. Instead, prefill a likely template by parsing messages, as we do for Git.
- Unify Git and Mercurial logic for acquiring messages, since `getLocalCommitInformation()` now provides this information. This should improve Git performance, and allows us to delete some code.
- Merge messages more intelligently. Currently, we just overwrite fields. Instead, merge arrays (like ccs, reviewers, tasks) and concatenate strings (like summary and test plan). We need to special case "title", see inline.
- @csilvers: this is a precursor to implementing "--reviewers", etc.
Test Plan: See next post, test plan includes giant output.
Reviewers: btrahan, csilvers, Makinde, jungejason, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2536
