Summary:
under git, we now create a branch that is at the patch's base revision if we
know it or whatever the working copy happened to be at if we don't. This diff
also adds the --nobranch flag to disable this new behavior.
Also added an --update flag. When specified, we run the appropriate "update"
command in the VCS. By default this is off.
Finally, tried to give the user more information about what the heck arc just
did to their working copy.
Test Plan:
// verify --update flag works
// -- git
Assume we are at HEAD and we got to HEAD from HEAD^1 via DX
git reset --hard <THE BEGINNING>
arc patch --update DX
// ...versus svn
Assume we are at HEAD and we got to HEAD from HEAD^1 via DX
svn checkout -r 1
arc patch --update DX
// ...versus hg
Assume we are at HEAD and we got to HEAD from HEAD^1 via DX
hg update -r 1
arc patch --update DX
// verify under git a nice branch is made
// -- test where we should get a good name
// -- test where we have a base revision to check out the branch at
Assume we are at HEAD and we got to HEAD from HEAD^1 via DX
git reset --hard HEAD^1
arc patch DX
// verify under git an "okay" branch is made if we can't get "nice"
// -- test where we should get a "bad" name
// -- test where we DON'T have a base revision to check out the branch at
git diff HEAD^1 > ~/example.patch
git reset --hard HEAD^1
arc patch --patch ~/example.patch
// verify --nobranch flag skips the test for git
Assume we are at HEAD and we got to HEAD from HEAD^1 via DX
git reset --head HEAD^1
arc patch --nobranch DX
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D1459
Summary:
Julien built a really cool static analysis database of our codebase. One
capability is that it can suggest typehints that are not in the code. The
analysis to do this is very expensive, so it can't reasonably be run locally.
But it can remain indexed on a server.
The idea here is to provide a familiar interface to it through arc lint, via a
generic Conduit service call.
In our lint engine, this will probably be gated on --advice for performance.
This will introduce a slight awkwardness in that running with --advice can add
new non-advice lint if the server chooses, but this isn't likely to cause a
practical problem.
Test Plan:
Construct a fake Conduit lint endpoint, attach this linter to it, and see bogus
lint
appear with --advice.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1462
Summary:
without "nocommit" we commit the patch to the working copy. add the nocommit
flag and this commit does not happen.
making this happen required adding revisionID to arc bundle to fetch the proper
commit message. if we can't get a commit message -- suppose the fetch fails or
the source is self::SOURCE_PATCH, we ask the user for the commit message on the
command line
Test Plan:
git reset --hard <SOME_REV>
arc patch DX, where this got us to <SOME_REV + 1>
observe correct patch committed with correct commit message in working copy
git reset --hard <SOME_REV>
arc patch --nocommit DX, where this got us to <SOME_REV + 1>
observe correct patch landed BUT NOT committed. note "commit message" does not
exist since there isn't a commit...!
git diff HEAD^1 > ~/file.patch
git reset --hard HEAD^1
arc patch --patch ~/file.patch
observe prompted for commit message and patch committed with commit message i
typed in
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D1450
Summary:
This allows engines to check the canRun method on linters,
which should determine if a linter is configured and can
be run in the current environment.
Test Plan:
Implement a linter with canRun as false, and ensure
it doesnt run.
Ensure all existing linters still run by default.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1445
Summary:
This cleans up the PEP8 linter to bring it inline
with the new JSHint Linter's level of quality.
It adds a getPEP8Path method which gives the ability
for users to override the pep8 binary with two new
options in .arcconfig:
* lint.pep8.prefix
* lint.pep8.bin
* lint.pep8.options
Test Plan:
Adjust your engine to use the 'ArcanistPEP8Linter' and run
arc lint against Python files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1440
Summary:
This adds a new lint engine, ComprehensiveLintEngine, which
includes sane defaults for some generic languages.
Ideally this would include *all* available language linters,
but that can be enhanced at a later point. Right now it's mostly
the base linter with additional JavaScript and Python linters.
Test Plan:
Adjust the lint_engine to be "ComprehensiveLinterEngine". You'll
also need jshint, pyflakes, and pep8 to all be available on PATH.
Run arc lint against files which contain .php, .py, and .js.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1439
Summary:
pretty easy stuff as mercurial accepts git style patches...!
also fixed two issues where we were 1) storing the short hash and 2) storing it
with a trailing "\n". This diff makes us store the full hash AND no trailing
return character
Test Plan:
in my mercurial repo
<note repo at revision $foo>
<did something dumb>
hg commit -m "something dumb"
arc diff
<go to web and fill out stuff for DX>
hg checkout $foo
arc patch DX
<verify patch DX successfully applied!>
use conduit console to verify a few diffs were returning the correct full
revision hash with a trailing \n
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T630
Differential Revision: https://secure.phabricator.com/D1431
Summary:
Inspired by http://news.ycombinator.com/item?id=3464671 and a lot of
diffs I've seen @ FB, I've added a spell checking linter. To reduce
false positives, it's only a blacklist. Still, it catches a large
number of 'issues'.
Test Plan:
Unit tests. Ran on FB's codebase. No false positives
noticed but a lot of cases caught.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, jack
Differential Revision: https://secure.phabricator.com/D1409
Summary:
Extended the install-certificate workflow's timeout from 5
seconds to rely on the ConduitClient default (30 seconds), which matches the HTTP interface.
Test Plan: Run arc install-certificate
Reviewers: epriestley
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D1404
This patch adds ArcanistJSHintLinter class and three new .arcconfig options:
* lint.jshint.prefix - directory where JSHint binary resides
* lint.jshint.bin - JSHint binary name (if different from 'jshint')
* lint.jshint.config - a JSON file with JSHint project-wide options
By default, this linter assumes that JSHint is installed on user's system
as an NPM package.
Test Plan:
(1)
Run `npm install jshint -g` to install JSHint and add
ArcanistJSHintLinter to your Lint Engine (I didn't see PEP8 or
PyFlakes in the PhutilLintEngine so I decided to not to put
JSHint in there). After that you can do `arc lint my.js`.
(2)
Create a new file, config.json, and add `{ "white": true }` in it.
Add `"lint.jshint.config": "/path/to/config.json"` to your .arcconfig and
run `arc lint my.js`. After that, unless your name is Douglas Crockford,
you'll see tons of JSHint warnings about minor PEP8-like things (spacing, etc.)
Summary:
Currently, we throw a fairly perplexing error when there are multiple valid
commit messages. Installs can also remove the "test plan" field entirely, which
is the only really strong discriminator here.
When the message to use is ambiguous, show the user all the valid messages and
prompt them to choose one.
Also add a -C flag like "git commit -C", so they can choose a message
explicitly.
Test Plan: Ran "arc diff HEAD^^^^^", "arc diff -C <rev>".
Reviewers: cpiro, btrahan, jungejason
Reviewed By: cpiro
CC: aran, cpiro
Differential Revision: https://secure.phabricator.com/D1385
Summary:
If the else clause does not have braces (one-liner), XHPASTLinter eats the
newline and brings the body statement of the else clause to the same line.
Test Plan: added a new test to space-after-control-keywords.lint-test
Reviewers: epriestley
CC: aran, epriestley, kiyoto
Differential Revision: https://secure.phabricator.com/D1367
revision is the correct base revision relative to the patch.
Summary: What the title says. If not correct, warn the user. This check
honors the --force flag to skip all these checks. This change also includes
moving some Differential constants into Arc so they can be used for both
projects. There is a corresponding phabricator diff (incoming) to address this
part of the change.
Test Plan:
For a project with actual diffs, a git repository tracked by phabricator, *AND*
development in master branch only, do some...
- git reset --hard HEAD^1
- arc patch DX, where X is what got us to HEAD in the first place
- verify successful patch
...then...
- git reset --hard HEAD^^
- arc patch DX, where X is what got us to HEAD in the first place
- verify warning
- verify Y versus N continues versus stops appropriately
Note if development were done outside the master branch this warning message
will fire early / often as git commit hashes are based on the commit *and* the
rest of the source code the commit is made against. This is (unfortunately) the
"typical" case so this warning is pretty active at the moment. T201 will
eventually land and when parsing a given commit update the corresponding diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1328
Summary: See T645. These commands take inconsistent and overly-magical arguments
right now. Instead, make them behave consistently and allow them both to operate
on "arc <workflow> path path2 path3 ...", which is a generally useful workflow.
Test Plan: Ran "arc lint <path>", "arc unit <path>", "arc lint --rev
HEAD^^^^^^", "arc unit --rev HEAD^^^^^^^^^^^^", etc. Ran "arc diff --trace" and
verified --rev argument to child workflows.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T645
Differential Revision: https://secure.phabricator.com/D1348
Summary:
See T614. This flag explicitly tells Arcanist to use the message for an existing
revision, and attach the change to it directly.
Next step is to have "arc diff" automatically choose "--create" or "--update" in
the absence of "--create", "--update", "--only", "--preview" or a template
commit message.
Test Plan:
Ran "arc diff --update <n>" for various revisions. Got updates or
errors as expected.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, jungejason
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1346
Summary: Find the relative commit by finding the first non-outgoing commit, so
we don't show changes caused by merges we've performed since the last time we
pushed.
Test Plan: Checked out two hg working copies, A and B. Made a change in A. Made
a change in B. Pushed B. Merged in A. Made another change in A. Ran "arc diff"
in A. Got only changes I made in A in the diff, not the change from B.
Reviewers: Makinde, btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1339
Summary:
Some Mercurial users at Dropbox have very specific diff preparation
needs. Allow "arc" to read an arbitrary diff off stdin. This disables most
features.
Test Plan:
Ran "git diff HEAD | arc diff --raw", "git show | arc diff --raw",
"hg diff --rev 8 | arc diff --raw".
Reviewers: btrahan, jungejason, Makinde
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T617
Differential Revision: https://secure.phabricator.com/D1323
Summary:
I want to make some changes to this workflow but it a huge mess right now. Try
to refactor a bit to make it a little more manageable.
Broadly:
- Moved lint/unit constant mapping into separate methods.
- Moved lint/unit/local properties into separate methods.
- Moved diff spec construction into a separate method.
- Moved some message stuff into separate methods and reorganized related
methods near to one another.
- Removed an unused findRevisionInformation() method.
I fixed a couple of small bugs, too:
- --create now conflicts with --only and --preview.
- --create now probably works in Mercurial.
- --create messages now have basic reviewer validation.
This should have not have any significant behavioral changes.
Test Plan:
- Created this revision.
- Ran "arc diff --create".
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1320
Summary:
- Remove XHP tests so I can remove XHP support from XHPAST.
- Update license tests so they don't break every year.
Test Plan: - Ran all unit tests.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, pad, jungejason, btrahan
Maniphest Tasks: T635
Differential Revision: https://secure.phabricator.com/D1318
Test Plan:
Run ##arc lint## after changing a binary file
"Warning: Invalid argument supplied for foreach() in
src/workflow/cover/ArcanistCoverWorkflow.php on line 88" should not be displayed
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1314
Summary: Previously, we would not correct missing space before "{".
Test Plan: Ran unit tests.
Reviewers: btrahan, jungejason, kiyoto
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1313
Summary: Previously, we would not correct excessive space after "if", etc.
Test Plan: Ran test case.
Reviewers: btrahan, jungejason, kiyoto
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: https://secure.phabricator.com/D1311
Summary:
- Show file/line so you can tell which assertion failed if there's a block
with a zillion of them and they don't have messages.
- Try to format stuff a little better.
Test Plan: - Ran some failing unit tests.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1304
Summary:
Creates a new hook API that can be used to interface with
SVN/Git/Mercurial in the context of a commit hook. Currently only adds a
function to read the modified file data in a Subversion commit hook.
An object of this API is created in the SvnHookPreCommitWorkflow and
passed on the Lint Engine which then uses it to access current file
data, of the way the APIs seem to be structured); linters use the
getData function which is essentially a wrapper around the engine's
call, with another layer of caching.
Task ID: #770556
Blame Rev:
Test Plan:
- Create a local svn repository and add a minimal hook to run the local
version of arc to test commits
(http://phabricator.com/docs/arcanist/article/Installing_Arcanist_SVN_Hooks.html)
- Create a temporary repository that can trigger any of the linters
available, and test against a temporary linter by committing against
the test repository: the linter should be able to access all required
files by using loadData/getData in the LintEngine and Linter.
Revert Plan:
Tags: lint, svn-hook-pre-commit
Reviewers: jungejason, asukhachev, epriestley, aran
Reviewed By: epriestley
CC: aran, jungejason, epriestley, kunalb, asukhachev
Differential Revision: https://secure.phabricator.com/D1256
Summary:
in arcanist we are using $_SERVER['PWD'], but in some cases it
is not returning the correct result. For example, when I'm using
PhpStorm (an php IDE) to launch the script, $_SERVER['PWD'] returns the
path of the binary of the IDE:
/home/jungejason/tools/PhpStorm-107.658/bin, not the path where arcanist
is at. getcwd() returns the correct value.
One difference between getcwd() and $_SERVER['PWD'] is that getcwd()
resolves symlink where $_SERVER['PWD'] does not. From what I can see,
using realpath should work.
Test Plan:
* ran arcanist as normal and it worked;
* run arcanist in PhpStorm and it worked.
* created a symlink pointing to the repository inside which
I ran the arc command, and it worked.
* created a symlink pointing to arcanist project, run `.
* resources/shell/bash-completion` and it worked
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1285
Summary:
See T419 for some philosophical musing on this. The "ideal" way to represent
these changes is two or more COPY_HERE plus a DELETE, but we can't build the
DELETE patch yet.
Just turn one of the COPY_HERE changes into a MOVE_HERE. While less idealistic,
I think this should always work.
Test Plan: This seemed to cleanly apply TenXer D265. TenXer guys, can you
confirm that this makes it actually patch correctly?
Reviewers: jonathanhester, bizrad6, kdeggelman, jungejason, btrahan
Reviewed By: jungejason
CC: aran, jonathanhester, jungejason, epriestley
Maniphest Tasks: T419
Differential Revision: https://secure.phabricator.com/D1270
Summary:
Thinking about this, I think it's worthwhile to bump versions for T1249/T1250.
The problem is that if you have an old Arcanist, trying to update diffs against
a new Phabricator will create new diffs instead, and it probably won't be
obvious what's wrong.
Bump the versions so users will get a message like "oh, hey, you should
upgrade".
Test Plan:
- Tried to diff against a mismatched version.
- Diffed against a matching version.
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: aran
Differential Revision: https://secure.phabricator.com/D1257
Summary: We fatal confusingly if you specify a valid class that isn't of the
right subclass (like a linter rather than a lint engine). Improve the error
message.
Test Plan:
- Ran "arc lint --engine PhutilSymbolLoader", "arc unit --engine
PhutilSymbolLoader", got expected failures.
- Ran "arc diff" normally without errors.
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: aran
Differential Revision: https://secure.phabricator.com/D1259
Summary:
- Allow Arcanist to parse either "123", "D123" (existing behaviors) or
"http://phabricator.example.com/D123" (new behavior) values.
- Drop support for labeling this field "DiffCamp". This should only impact
people trying to update revisions that are more than ~a year old, which should
be very very few.
Test Plan: - Ran "arc diff" with values "74", "D74", "x74",
"http://local.aphront.com/D74", "http://local.aphront.com/x74". Got the expected
behaviors.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T54, T692
Differential Revision: 1249
Summary: ... in case the head commit is empty. Empty commits are useful for
injecting an Arcanist commit message in a branch then sending the whole thing
off for review. As far as I know there is no situation in which an empty commit
would exist unintentionally and using ##--allow-empty## would suppress an error.
Test Plan: works fine for a branch I just sent for review
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1241
Summary: --output json will be used for scripts, so support script writers a
little better.
Test Plan:
arc lint [--output {json, summary}]
arc lint --output json {--apply-patches, --never-apply-patches}
Reviewers: epriestley
Reviewed By: epriestley
CC: jack, aran, epriestley
Maniphest Tasks: T675
Differential Revision: 1220
Summary:
Adds a 'desc' field to json output so I can use that inside my vim
plugin.
Test Plan:
Lint a few things. Description text shows up in vim
Reviewers: epriestley
Reviewed By: epriestley
Differential Revision: 1210
Summary:
--revision doesn't allow the revision to be prefixed with 'D'. Also the error
message showed when specified revision doesn't exist is hard to understand.
Test Plan:
Used `arc amend --revision D123`, tried it without 'D' and with a non existing
revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1193
Summary: This adds parent directories of modified files to commit paths if
needed.
Test Plan:
Used https://reviews.facebook.net/D603 patch on Apache Hive repository, called
`arc commit` and SVN didn't complain about missing paths.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T667
Differential Revision: 1189
Summary:
See T614. This adds a "--create" flag which I think works properly, but doesn't
make it the default.
Once I add "--update" and am confident the flags actually work, I'll work on
some heuristics to make "arc diff" automatically choose "--udpate" or "--create"
as per T614.
Test Plan:
This revision was created with "--create" and a bogus commit message ("derp").
I intentionally goofed the message at first to test the fail + file workflow.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Differential Revision: 1162
Summary:
Commit templates are fully configurable on the server now, so we should be doing
validation there, since an install can add or remove fields and change
validation rules. Remove these outdated client validations, as per comment.
Also update the API call to use the 'errors' field, which allows us to show the
user all the parse errors at once. See:
https://secure.phabricator.com/diffusion/P/browse/origin:master/src/applications/conduit/method/differential/parsecommitmessage/ConduitAPI_differential_parsecommitmessage_Method.php;cfaab709df37739b$75
Test Plan:
Ran "arc diff" on a change with multiple errors:
```$ arc diff --conduit-uri=http://local.aphront.com/
Usage Exception: Commit message is not properly formatted:
Error parsing field 'Reviewers': Commit message references nonexistent users:
jjjderp.
Error parsing field 'CC': Commit message references nonexistent users and
mailing lists: jjjderp.
You should use the standard git commit template to provide a commit message. If
you only want to create a diff (not a revision), use --preview to ignore commit
messages.```
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1154
Summary: adds a little bit of sanity checking to the arc patch workflow. in
short, if the working copy project is not the same as the patch project, don't
apply the patch
Test Plan:
ran
arc patch DX
arc patch DX --force
in the top line directory for project A and proejct B. DX is for project A.
verified for project A that the patch was applied and for project B i was issued
warnings as expected. also verified in project B case that saying Y or N to the
warning had the desired effect.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1140
Summary:
If any hunks is detected as non-utf8, and you've never submitted diffs
for a certain project before, you would get a ERR-BAD-ARCANIST-PROJECT
exception. This makes it possible to submit the patch properly, so you
can set the encoding in the interface afterwards. Further this fixes
cases where you don't supply a diff but will result in hunks getting
treated as binary, but that still beats the exception behaviour.
Test Plan:
Ran `arc diff` with and without the new --encoding param and
got the expected results. Also ensured the diff (with non utf-8 hunks)
would be properly created even when no encoding is specified.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1135
Summary: If you modify a file at the root level (like the celerity map) we run
the dir/path checks in the wrong order and fail to detect that we can't possibly
find any modules. This leads to an infinite loop inside the while loop below.
Test Plan: Ran "arc unit" on a change which affects the celerity map.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1146
Summary:
arc unit supports paths parameter
It is not documented in arc help
Test Plan:
arc help
Search for unit
Reviewers: slawekbiel, aran
Reviewed By: aran
CC: aran, vrana
Differential Revision: 1143
Summary:
Git works completely differently for commits zero and one than for 2..N so add
more special casing to handle them. See:
- {T206}
- {T596}
The getCommitRange() block is also fatal land, although I wasn't able to reach
it. I'll follow up with @s on T596.
Test Plan:
- Created a new, empty repository ("mkdir x; cd x; git init").
- Ran "arc lint", "arc unit", "arc diff" against it with no commits (the first
two work, the third fails helpfully).
- Made an initial commit.
- Ran "arc lint", "arc unit", "arc diff" against it (all work correctly).
Reviewers: btrahan, jungejason, aran
Reviewed By: aran
CC: s, aran
Differential Revision: 1142
Summary:
add method setXHPASTTreeForPath() so that a child class of
ArcanistXHPASTLinter can set the tree easily.
Test Plan: wrote a subclass of ArcanistXHPASTLinter and it worked.
Reviewers: pad, epriestley
Reviewed By: pad
CC: aran, pad, jungejason
Differential Revision: 1134
modules
Summary:
Right now, if you add a file to "__tests__/data/blah.testcase" it doesn't
trigger those __tests__, but it should. Trigger __tests__ in all parent modules
when a file changes.
(We could be a little more complex about choosing what to run but all the tests
are super fast so it hardly matters if we run "too many" tests.)
Test Plan: Ran "arc unit" on this, got a test trigger. Added a file to a
__tests__/data/ directory and got a test trigger. Added some var_dump() and
spot-checked things for sanity.
Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1127
removed
Summary: Mercurial output diverges from git output when binary files are
removed. Parse the Mercurial flavor.
Test Plan:
- Unit test fails before this change and passes afterward.
- Removed a binary file in my hg local and successfully "arc diff --only"'d
the change.
NOTE: Git can't apply these patches, and reports:
$ git apply < ~/Desktop/patch.txt
error: removal patch leaves file contents
error: level.png: patch does not apply
Reviewers: Makinde, btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, Makinde
Differential Revision: 1126
Summary: if we don't care and don't expect it to break all other linters then
continue
Test Plan: ##arc lint --trace## on a commit with non-ascii characters, saw other
linters continue
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, cpiro
Differential Revision: 1122
Summary:
When a unit test throws an exception, provide more data (type, trace) in the
test failure message.
Previously, we would show only the message itself, which may not be very useful
in debugging test failures.
Test Plan: Ran "arc unit" on a test which throws, got a stack trace.
Reviewers: edward, btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1123
Test Plan:
Wrote an event listener modifying the commit message and the message was
successfully changed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 1103
Test Plan:
Added a test event listener, added an event dispatch in diff workflow, run the
workflow, the listener was called.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 1102
Summary:
We used square distance that optimized for the wrong thing. Making the same
markers spread out instead of being together
Also added a very little cost for switching type. That will make diff types
stick together a bit more
Task ID: #623
Blame Rev:
Test Plan:
Ran my new unit test. And tested a few diff in phabricator
Revert Plan:
Tags:
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1112
Summary:
just a little try / catch action in ArcanistDiffWorkflow.
"Ideally, it would be good to flag these changes somehow so that "arc patch" can
issue the inverse warning (e.g., "This patch could not be completely applied
because some binary data was not uploaded.") but that hasn't come up in a real
use case yet."
I think a change w/ filetype of FILE_IMAGE || FILE_BINARY *and* no phid means
that the upload failed so there's no additional flag needed. (True?) however,
it would be easy enough to store metadata that explicilty stated whether or not
the file upload succeeded.
(also / related - i looked through the arc patch workflow a bit and i don't
understand how the svn codepath loads up the actual binary files... for git,
toGitPatch => buildBinaryChange => getBlob is the right path )
Test Plan:
- set 'storage.mysql-engine.max-size' to 0 in my conf, uploaded diffs with
files
- noted in differential that it correctly detected images versus binary despite
file upload failing
- noted in differential that images had some empty UI
- reverted conf change, uploaded diffs with files
- noted in differential file showed up
- ran arc patch with DX, where DX had broken files
- noted "Downloading binary data...
Exception:
ERR-BAD-PHID: No such file exists."
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1100
Summary:
If arc amend is run on a rev that isn't accepted, it runs arc mark-committed
--finalize. The rev shouldn't be marked as committed if it hasn't been accepted,
so this diff adds in that check.
Test Plan:
Ran arc amend on a rev that hasn't been accepted, checked that it didn't get
marked as completed.
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, epriestley, nh
Differential Revision: 1104
Summary: 'patch' chokes on hunks with too much trailing stuff. 'git apply'
chokes on overlapping hunks. Make them both happy. Write some test cases so I
stop breaking this stuff.
Test Plan:
- Applied a previously-failing patch via SVN.
- Applied a previously-failing-then-succeeding patch via Git.
- Ran unit tests.
Reviewers: jungejason, btrahan, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1099
Summary:
In D1079, I added "--color never", but this flag is provided by the "color"
extension, which is why I missed it originally, because it doesn't show up until
you enable that extension. Providing it causes installs which don't have it
enabled (disabled is the default) to fail.
Use "--config" to disable color instead. This sets a configuration setting and
works regardless of whether the color extension is present.
Test Plan: Ran "arc diff" in a mercurial working copy with the color extension
enabled and disabled.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1092
Summary: We ostensibly now support Mercurial at least mostly, so don't
underpromise quite so much.
Test Plan: Ran "arc diff" in a Mercurial working copy without being warned.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1085
Summary: We currently may generate patches which have hunks that include overlapping context. Git was okay with this until at least 1.7.3.4, but started rejecting these patches some time later.
We already attempt to detect and prevent this condition, we just don't do a very good job of it. Fix the check so that we avoid generating overlapping hunks.
Test Plan:
- Cleanly applied troublesome patches under Git 1.7.7.2.
- Used "arc export --git" to verify patches generate without overlapping sections.
- Locally changed $context to 2 and 1, verified patch behavior was reasonable.
Reviewers: kdeggelman, bizrad6, jonathanhester, jungejason, nh, tuomaspelkonen, aran
CC:
Differential Revision: 1084
Summary: Mercurial has a --color flag which disables the use of ANSI colors. Use
it to prevent "hg diff" from giving us colorized diff output.
Test Plan: Ran "hg diff --color never", verified it disabled ANSI color in diff
output.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran
Differential Revision: 1079
Summary:
Adds a secret, undoucmented "encoding" key to ".arcconfig" which makes a very
half-hearted effort to convert encodings. This is probably good enough that
Differential can be used for code review, but there will be issues with 'arc
patch', 'arc export', paste, maybe conduit stuff, Diffusion, and whatever else I
haven't thought of.
This also doesn't store the original encoding so anything converted like this
won't reasonably be able to be made to work with all that stuff in the future.
See T452 for a broader discussion of the issues involved.
Test Plan:
Short circuited the UTF-8 detection to always fail, had my files "converted"
from ISO-8859-1 to UTF-8.
@davidreuss: you can test this by applying this patch to arcanist/, adding
'"encoding" : "ISO-8859-1"' to your .arcconfig, touching some non-ASCII file,
and then running "arc diff".
Reviewers: davidreuss, jungejason, tuomaspelkonen, aran
Reviewed By: davidreuss
CC: aran, davidreuss, epriestley, nshamg123
Differential Revision: 812
Summary:
Save commited revision id in workflow, so it can be used in Arcanist
didRunWorkflow hook.
Test Plan:
Running getRevisionID on the workflow should return id of the committed
revision.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley, mareksapota
Differential Revision: 1078
Summary:
D1061 introduced a 'text file' check, but it fails under SVN for new
directories.
- Revert D1061 (This reverts commit b2cd18252701be2093b52652fb3d1d94c5df571e.)
- Make getChangedLines() return null to indicate that the operation doesn't
make sense. I think this was the intent of the code in the lint engine.
- Fix a bug where running "arc lint" on a change in an SVN working copy from a
subdirectory would fail.
- Fix a bug where warnings with no line information were incorrectly
discarded.
Test Plan:
- Ran "arc lint" in an SVN working copy with a new directory (no failure).
- Forced FilenameLinter to always raise a warning. Added a binary file and ran
"arc lint". The warning was reported for the new binary file, a new text file,
and a new directory.
Reviewers: jungejason, andrewjcg, nh, tuomaspelkonen, aran
Reviewed By: andrewjcg
CC: aran, andrewjcg, epriestley
Differential Revision: 1076
Summary: Apparently I just never tested this or something. Make it work
correctly.
Test Plan: Ran "arc diff" in a Mercurial working copy with a binary file in
outgoing.
Reviewers: Makinde
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1074
Summary: We incorrectly add the "Uncommitted" flag to untracked files, which
causes them to raise various prompts and not respect "--allow-untracked".
Test Plan: Ran "arc diff --allow-untracked" in a clean working copy with
untracked files, was not fatally error'd.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde, epriestley
Differential Revision: 1073
Summary:
Allow `arc patch` without authentication if Phabricator instance has
'differential.anonymous-access' set to true.
Test Plan:
Set 'differential.anonymous-access' in Phabricator to true and run `arc patch`
without installing a certificate. `arc patch` should work as expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1069
Summary:
In D962, I switched us from using "hg summary" to "hg id" to find the working
copy revision. However, "hg id" reports the revision with a trailing "+" if the
working copy is dirty, so we fail before hitting the explicit check for this
later.
Trim off the trailing '+' if it is present.
Test Plan:
Ran "arc diff" in a dirty Mercurial working copy and got a good error message
about uncommitted changes.
~/repos/hg-working-copy $ arc diff
WARNING: Mercurial support is largely imaginary right now.
Usage Exception: You have uncommitted changes in this branch. Commit (or
revert) them before proceeding.
Working copy: /INSECURE/repos/hg-working-copy/
Uncommitted changes in working copy
hello.c
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1070
Summary: newer PyLint includes commas and a column number for every message
after the line number. ignore that if it's present.
Test Plan: ##arc lint --trace --lintall## a file with messages with the old and
new pylint (2.5), works fine with both
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1062
Summary:
Currently, lint messages are only included if they are errors or
if they affect lines which the diff changed. The implementation
of this caused issues for non-text files (e.g. binaries), as line
change information is not available, and the corresponding lint
messages were dropped (for non-errors).
In this diff, only lint messages concerning text files are dropped
based on this line filtering.
Test Plan: arc lint with binary file
Reviewers: jungejason, epriestley
Reviewed By: epriestley
CC: aran, aravindn, andrewjcg, liat, epriestley
Differential Revision: 1061
Test Plan:
Using SVN make some changes to the repo, run `arc diff`. As other user accept
the revision, add the changes to your repo using `arc patch` and then run `arc
commit --revision revisionID` to commit them. Arcanist should ask if you are
sure that you want to commit this revision and if you answer `Y` it should
commit to SNV repo.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, mareksapota, epriestley
Differential Revision: 1055
Test Plan:
Tried viewing a diff detected as binary in diffusion with and without
setting my desired encoding. Worked as expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1008
Summary: allow adding PYTHONPATHs directly in addition to the hardcoded few
already allowed.
Test Plan: ##arc lint## successfully picks up the paths listed in my .arcconfig,
and no longer errors on modules not found from those directories
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1050
Test Plan:
Run `arc mark-committed` on a revision that you don't own, you should see a
prompt asking you if you really want to do that and if you answer `Y` it should
mark the revision as committed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota
Differential Revision: 1052
mercurial changesets
Summary: Makes the parsing slightly more liberal so a test case from @Makinde
passes.
Test Plan: Ran unit tests.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1054
Summary: add the ability to specify an rcfile path either absolute or relative
to the project root.
Test Plan: added one for a project, ran ##arc lint --trace## to see the expected
result
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1046
Summary:
- The top documentation is more readable, and the details about the severity
mapping regexps is moved to the top from inline -- they're important.
- array_merge() is used when appending command line args from
##lint.pylint.options## instead of '+'. The latter merges by key instead of
appending the two lists as intended.
- rely on the exit code instead of magic text that may or may not be there
depending on how ##pylint## is invoked.
These bugs were introduced in d762311a9d.
Test Plan: ##arc lint --trace##'d a bunch of Python source files in a project,
with and without PyLint messages. Added some ##lint.pylint.options## to see
proper appending behavior.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, cpiro
Differential Revision: 1045
Summary:
See T584. Git renders these types of changes somewhat unusually. Ensure they are
rendered as changes.
The fact that a symlink was replaced is not explicitly rendered (e.g., "This
path was changed from a symlink to a regular file."), but can be inferred from
the 'unix:filemode' property change that will accompany the changeset. We could
also make it explicit but this type of change is rare enough that it's probably
good enough as-is.
Test Plan:
- Ran unit test, which previously failed and now passes.
- Created a diff which replaces a file with a symlink, verified it rendered a
little more sensibly.
Reviewers: aravindn, dschleimer, jungejason, nh, tuomaspelkonen
Reviewed By: nh
CC: aran, goldshlager, nh
Differential Revision: 1030
Summary: See T587. Reduce the strictness of working copy checks when using
"--show", since there's a reasonable workflow where you 'arc patch' and then
'arc amend --revision X --show | git commit -a -F -' that currently won't work.
There are other ways to accomplish the same thing but this increases flexibility
overall.
Test Plan: Ran 'arc amend --show' with a dirty working copy, didn't get yelled
at.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 1033
changes which add or alter binary files
Summary: See test cases, derived from "hg diff --git" and "git diff --binary".
Test Plan: Test cases work now.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1047
Summary:
This is separated from D812. See D812, D1008, D1009. Separation allows us to
land these patches safely.
See T452 for a broader discussion of the issues involved.
Test Plan: See D812.
Reviewers: davidreuss, jungejason, nh, tuomaspelkonen, aran
Reviewed By: davidreuss
CC: aran, davidreuss
Differential Revision: 1040
Test Plan:
Go to /differential/diff/create and try to upload a patch that was created with
`git diff` when diff.mnemonicprefix was set to true.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1022
Summary:
D935 missed one place of parseGitRelativeCommit() in
ArcanistExportWorkflow.
Test Plan: ran arc export and verify that it worked.
Reviewers: epriestley, tuomaspelkonen, aran
Reviewed By: aran
CC: aran
Differential Revision: 1015
Summary: A minor bug in the parser prevented it from handling git diffs where an
empty file appears at the end of the diff. Since git appends an extra newline,
we failed to jump into the '$line === null' block.
Test Plan: Ran unit tests. Generated a revision which only deleted an empty
file.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh, epriestley
Differential Revision: 998
Summary:
See D945. We have this kludgy "remote_hooks_installed" mess right now, but we
have enough information on the server nowadays to figure this out without it.
Also reduce code duplication by sharing the "mark-committed" workflow.
This causes "arc merge" to effect commit marks.
Test Plan:
In Git, Mercurial and SVN working copies ran like a million
amend/merge/commit/mark-committed commands with and without --finalize in
various states of revision completion.
This change is really hard to exhaustively test because of the number of
combinations of VCS, revision state, command, command flags, repository state
and tracking state. All the reasonable tests I could come up with worked
correctly, though.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 967
Summary:
Expand 12-character hashes to 40-character hashes so other things will work
properly.
Also use "hg id" instead of "hg summary" to figure out where the working copy
is, since it's substantially simpler.
Test Plan: Ran "arc diff" and got properly 40-character hashes.
Reviewers: Makinde, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 962
Summary: For Mercurial parsing in Diffusion, it looks like the only command we
can use to get move/copy information is "hg status -C --rev <rev>", so add a
parser for it.
Test Plan: Ran unit tests.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 958
Summary: I renamed this at some point but missed a callsite. See T519.
Test Plan: Ran "arc patch" in an SVN working copy.
Reviewers: Girish, jungejason
Reviewed By: Girish
CC: aran, Girish
Differential Revision: 959
Summary: Move code to actually parse "hg" output into a separate class with some
tests, so I can reuse it in the import scripts. We should probably do this for
Git/SVN at some point, too.
Test Plan: Ran unit tests, used this class in Phabricator importers, grepped for
calls to removed private methods.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 942
Summary: When a project uses a conservative history mutability doctrine, never
try to amend history.
Test Plan: Ran "arc amend" in an "immutable_history" working copy.
Reviewers: fratrik, Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde, epriestley
Differential Revision: 862
Summary:
This should support conservative rewrite policies in git fairly well, under an
assumed workflow of:
- Develop in local branches, never rewrite history.
- Commit with "-m" or by typing a brief, non-template commit message
describing the checkpoint.
- Provide rich information in the web console (reviewers, etc.)
- Finalize with "git checkout master && arc merge branch && git push" or some
flavor thereof.
This supports Mercurial somewhat. The major problem is that "hg merge" fails if
the local is a fastforward of the remote, at which point there's nowhere we can
throw the commit message. Oh well. Just push it and we'll do our best to link
them up based on local commit info.
I am increasingly forming an opinion that Mercurial is "saftey-scissors git".
But also maybe I have no clue what I'm doing. I just don't understand why anyone
would think it's a good idea to have a trunk consisting of ~50% known-broken
revisions, random checkpoint parts, whitespace changes, typo fixes, etc. If you
use git with branching you can avoid this by making a trunk out of merges or
with rebase/amend, but there seems to be no way to have "one commit = one idea"
in any real sense in Mercurial.
Test Plan: Execute "arc merge" in git and mercurial.
Reviewers: fratrik, Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, epriestley, Makinde
Differential Revision: 860
Summary:
We have some git-specific logic on main pathways which should be in the API
class, move it around so "arc lint" with an engine works under Mercurial. This
resovles the error @makinde reported:
> PHP Catchable fatal error: Argument 1 passed to
ArcanistBaseWorkflow::parseGitRelativeCommit() must be an instance of
ArcanistGitAPI, instance of ArcanistMercurialAPI given, called in
/home/makinde/.arc_install/arcanist/src/workflow/lint/ArcanistLintWorkflow.php
on line 131 and defined in
/home/makinde/.arc_install/arcanist/src/workflow/base/ArcanistBaseWorkflow.php
on line 830
Test Plan: Ran "arc diff" in git and hg working copies, plus "arc lint" with a
configured "lint_engine".
Reviewers: Makinde, aran, jungejason, nh, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 935
Summary:
- Use "hg parents" to figure out where outgoing changes originate from, not
"~1", since that's a new feature in Mercurial.
- Add "--style default" all over the place since hg config can override this
(similar to the date config issues we saw in git).
- Cache working copy status so we don't run full hg diffs like 30 times
(similar to git/svn APIs).
- Use full "--git" flag instead of rather cryptic "-g".
Test Plan: Ran "arc diff" in my hg working copy, got the diff I expected.
Reviewers: Makinde, aran, jungejason, nh, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 934
Summary:
- Noncontentious Mercurial stuff from D863.
- Use "hg outgoing --branch" to make sure we aren't including outgoing
revisions on other branches.
- Use "hg summary" to figure out where the working copy is, so "hg up <older
rev> && arc diff" works like expected.
Test Plan: Ran "arc diff" from multiple branches with the working copy in
various states.
Reviewers: Makinde, aran, jungejason, nh, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 936
Summary: Parse some other fields which I hadn't seen yet from "hg log". Select
the right "hg log" range for local commit info.
Test Plan: Ran "arc diff" in my test mercurial repository from a branch with
tags. Looked at "local commits" in Differential.
Reviewers: Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 880
Summary:
1) unit engine getter method in the unit workflow
we store some information (unit test results) in the engine that we
need to access in the "arc unit" post hook
2) make requireCleanWorkingCopy() public
"arc unit" doesn't need to be clean in general, but we'd like the
option to upgrade the workflow to require it if necessary. We use
this for ensuring a repo is clean before updating unit test
results.
Test Plan: Used both features in a custom post hook in arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, epriestley
Differential Revision: 917
Summary:
See T326. Allow lint rules to be selectively overridden, e.g. for Conduit
methods.
Since FB has a long history of suggesting crazy patches for this stuff I think
we're safer just adding a hook class than trying to do some kind of regexp
magic.
Test Plan: Wrote a hook for Phabricator and linted some Conduit files without
issues. Ran unit tests.
Reviewers: nh, jungejason, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 874
Summary: When a revision is created, attach relevant information about the local
commits which it came from if applicable. This supports T473, for DCVSes and
DCVS workflows with immutable history where we can't just amend commit messages.
It will also allow us to enrich the web interface.
Test Plan: Will verify this info shows up for this very diff.
Reviewers: fratrik, aran, jungejason, tuomaspelkonen
Reviewed By: fratrik
CC: aran, epriestley, fratrik
Differential Revision: 857
Summary:
The unix utility `patch` will not move a file (unless it is in or out of
/dev/null). If a diff copies or moves a file and also makes changes to the file,
the generated patch needs to list the new filename as the path to both the
original and new files so the changes get written to the new file.
(When arc applies this patch, it copies or moves the original as needed before
running `patch`.)
(This only matters for svn repos, since arc uses git commands for git repos
instead of using `patch`.)
Test Plan:
Created an arc bundle of a diff that copied a file to a new location and made
changes in both locations, and then ran arc patch with this bundle (in an svn
repo) to see that it correctly patches.
Reviewed By: epriestley
Reviewers: epriestley, jungejason, tuomaspelkonen
CC: aran, epriestley, nh
Differential Revision: 813
Summary:
A better definition of "binary" is "not utf-8", instead of "has some characters
not in this arbitrary regexp". Principally, this makes files with windows
newlines not autodetect as binary.
This might fix some of the issues in T365.
Test Plan: @egillth applied this patch and verified that Diffusion now shows
file content instead of detecting everything as binary in his repo full of
Windows newlines.
Reviewed By: jungejason
Reviewers: egillth, tuomaspelkonen, jungejason, aran
CC: aran, jungejason
Differential Revision: 799
Summary:
- Build the manifest of file changes so unit and lint workflows work.
- Default to creating a diff between the parent of the first outgoing change
and the tip.
Test Plan:
- Ran "arc diff" in a dirty mercurial repo, got warned about
untracked/uncommitted changes.
- Ran "arc diff" in a clean mercurial repo, got a diff of everything I'd done
locally.
Reviewed By: aran
Reviewers: Makinde, aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 796
Summary:
There's a lot of ground left to cover but this makes "arc diff" work (on one
trivial diff) in my sandbox, at least, and supports parsing of Mercurial native
diffs (which are unified + a custom header). Piles of missing features, still.
Some of this is blocked by me not understanding the mercurial model well yet.
This is also a really good opportunity for cleanup (especially, reducing the
level of "instanceof" in the diff workflow), I'll try to do a bunch of that in
followup diffs.
Test Plan: Ran "arc diff" in a mercurial repository, got a diff out of it.
Reviewed By: aran
Reviewers: Makinde, jungejason, tuomaspelkonen, aran, codeblock
CC: aran, epriestley, codeblock, fratrik
Differential Revision: 792
Summary: This stopped being available in scope when I refactored this
recentlyish.
Test Plan: Got error, saw useful message.
Reviewed By: jungejason
Reviewers: mgummelt, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 787
Summary:
Requires
https://secure.phabricator.com/rPHU0acf708b9b0fdbf59e4399f14dd8295b6a96972c
from libphutil, which allows a backslash prefix to control escape
sequences such as bold, underline, and invert.
Test Plan: "arc help liberate"
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 767
Summary:
The module analyzer reads "phutil_require_module" in the source of a module as a
dependency, and tries to regenerate __init__.php if symbols from that module
aren't actually used. This creates patches which don't actually resolve the
problem, since changing __init__.php won't change the dependency.
Instead, trust that anyone using phutil_require_module in the source of a module
knows what they're doing and don't mark it as a dependency.
We currently have an issue with this in phabricator's Setup process since I load
some other libraries' modules just to test if they can be loaded
@lesha, this might be the issue you reported a while ago.
Test Plan: Ran "arc lint" on a module which pulls in another module explicitly
in the source, didn't get a no-op lint error.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran, lesha
CC: aran, epriestley, jungejason
Differential Revision: 770
Summary: See D753. Let's just have lint fix this. Depends on D754 (it fails to
detect all the blocks without that patch, but doesn't do anything bad).
Test Plan: Ran unit tests.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: hunterbridges, aran, epriestley, jungejason
Differential Revision: 755
Summary:
- Provide a "--json" flag for "arc upload"
- Unify some of the stderr stuff across upload/download/paste.
Test Plan:
- Ran "arc upload" and "arc upload --json", piped stderr away with 2>/dev/null
to verify only JSON got emitted to stdout
- Ran "arc paste"
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock
Differential Revision: 749
Summary: Read and write in the same workflow! Dogs and cats living together!
Test Plan: - Performed a bunch of paste reads and writes and they looked ok?
Reviewed By: aran
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 748
Summary:
right now 'arc blame' uses the user's blame.date setting, but
it will only work when the format is iso. So it will fail for user who
has customized it.
Test Plan:
run arc cover with 'blame.date' set to relative and verified
that 'arc cover' still works
Reviewed By: epriestley
Reviewers: epriestley, codeblock
CC: hwang, aran, epriestley
Differential Revision: 745
Summary: Mechanisms for interacting with Files via Arcanist.
Test Plan:
- Ran 'arc upload x', 'arc upload x y z'
- Ran 'arc download' with --as and --show.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
CC: aran, codeblock, epriestley
Differential Revision: 742
Summary:
remove the size check. The conduit call will fail later if the
size is too large, and we will get better error message.
Test Plan:
run arc diff with files smaller and larger than the size
limit.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 729
Summary:
When phutil_analyzer builds the dependency graph I convert all class names w.r.t
XHP's internal naming scheme. It actually wouldn't be a terrible idea to do this
munging when phutil loads the symbols. I guess it doesn't really matter at the
moment since only Arcanist generates phutil maps and only libphutil reads them,
but it'd be nice to do this munging in as few places as possible in the future.
The XHP grammar does not allow elements to be interfaces
(dafff2cc18/xhp/parser.y (L1688))
so I've only applied this to classes.
Test Plan:
Created a sample project:
./__phutil_library_init__.php
hopping/__init__.php
/hophophop.php
where that last file contains
<?php
class :bunny:hop-hop-hop extends ❌element {
protected function render() {
return <p>bunny goes hop hop hop</p>;
}
}
Ran phutil_mapper.php and generated __phutil_library_map__.php:
<?php
/**
* This file is automatically generated. Use 'phutil_mapper.php' to rebuild i\
t.
* @generated
*/
phutil_register_library_map(array(
'class' =>
array(
'xhp_bunny__hop_hop_hop' => 'hopping',
),
'function' =>
array(
),
'requires_class' =>
array(
'xhp_bunny__hop_hop_hop' => 'xhp_x__element',
),
'requires_interface' =>
array(
),
));
Reviewed By: epriestley
Reviewers: epriestley
Commenters: aran
CC: aran, ide, epriestley
Differential Revision: 715
Summary:
In hphp's version of idx, it uses debug_rlog() which is not
available in phabricator's code. It will be executed if null is passed
as the second value to idx.
Test Plan:
ran arc lint which still report error message, and php
./scripts/arcanist.php unit src/lint/linter/xhpast/__test__ doesn't
throw.
Reviewed By: epriestley
Reviewers: tuomaspelkonen, epriestley
CC: aran, jungejason, epriestley
Differential Revision: 666
Summary: One day all "GUID" references will be gone, maybe.
Test Plan: grep, ran workflow before changing and got a warning about
deprecation
Reviewed By: aran
Reviewers: mgummelt, tuomaspelkonen, aran, jungejason
CC: aran
Differential Revision: 671
Summary:
The primary goal of this is to allow pre/post workflow hooks to upgrade a
workflow which doesn't require conduit into one which does, or one which doesn't
require authentication into one which does. They do this by calling
$workflow->establishConduit() or $workflow->authenticateConduit() respectively.
It also removes a bunch of dead code and a bunch of now-unnecessary public
interfaces.
Test Plan:
Broke my certificate and ran "arc list", "arc unit", "arc help", "arc
call-conduit".
Restored my certificate and re-ran the commands.
Reviewed By: mgummelt
Reviewers: mgummelt, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, mgummelt
Differential Revision: 664
Summary:
In order to support more flexible post hooks. Also send the
error code to the hook for use by client code.
Test Plan: None
Reviewed By: epriestley
Reviewers: epriestley
CC: dpepper, aran, epriestley
Differential Revision: 656
Summary:
in https://secure.phabricator.com/rARC36de84ee, we changed to
use 'binary-phid' from 'binary-guid'. Several places are still using
'binary-guid'. Fix them.
Test Plan:
run 'arc patch' against a revision which was throwing
exception because of this issue and it worked.
Reviewed By: epriestley
Reviewers: epriestley, sgrimm
CC: aran, epriestley
Differential Revision: 624
Summary:
executes the calls to git in parallel to improve startup performance of
arc lint and possibly other commands
Gets about a 2~3x speedup when repo is in buffer cache, with 4 cores.
Test Plan:
arc linted a repo with unstaged, untracked, staged, and committed
changes, see that the same files were linted.
Reviewed By: jungejason
Reviewers: jungejason, epriestley
Commenters: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 594
Summary: This allows us to detect and complain about mismatched client and
server host identities. It causes some subtle and not-so-subtle problems if the
client and server don't agree on the install's primary URI.
Test Plan: Ran "arc install-certificate" and "arc list" against my local host
with intentionally bogus configs and was warned. Ran with normal configs and
everything worked.
Reviewed By: tuomaspelkonen
Reviewers: jungejason, llorca, tuomaspelkonen, aran
CC: aran, tuomaspelkonen
Differential Revision: 591
Summary:
Keeping unit tests speedy keeps them useful since people actually won't mind
running them. This diff records the time taken by each test and displays it nice
and colorized. Really, I just want to discourage non-unit tests from making
their way into ##__tests__##.
Some thoughts:
- The "acceptableness" times are subjective but if dependencies are properly
mocked the times seem to be ok. Integration tests that make network requests to
third-party endpoints and pull in megabytes of data will not survive. This is a
good thing.
- Fast tests get a gold star, encouraging small tests. I am sorry that the star
does not sparkle.
- There is no way for a programmer to admit that their test is going to be slow
in some cases. They will be shamed with red text for the life of their test.
- It might be confusing that fast but failing tests get green text and maybe a
gold star.
Test Plan: Ran some of the unit tests within Arcanist and libphutil. See
https://secure.phabricator.com/file/view/PHID-FILE-cdd3c94c219e0fd7470b/ for
sample output.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 588
Summary:
We do this check on the web interface but not from the CLI.
Also clean up some GUID/PHID stuff (eventually all GUID references should be
replaced with PHID, although they mean the same thing).
Test Plan: Tried to create a revision with myself on the reviewer line, got
called out.
Reviewed By: jungejason
Reviewers: moskov, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 564
Summary:
Document the relationship between lint engines and linters. Provide an example
linter. Improve the documentation of PyLintLinter, which has a bunch of
configuration stuff which you had to dig into the code to get.
Test Plan:
Ran "arc lint --engine ExampleLintEngine --lintall derp.py" on a file with a
Python syntax error in it. Read documentation.
Reviewed By: j3kuntz
Reviewers: j3kuntz, andrewjcg
CC: aran, j3kuntz
Differential Revision: 557
Summary:
If you checkout some commit you end up on "(no branch)" which currently breaks
the parser. Ignore that, and add revision IDs to the output. They aren't very
big and I hate flags so I didn't add a flag for this. You can add a flag to turn
them off if you really want.
Test Plan:
Ran "arc branch" and "arc branch --by-status" from a "(no branch)" working copy.
Reviewed By: slawekbiel
Reviewers: slawekbiel, ahupp, jungejason, tuomaspelkonen, aran, schrockn
CC: aran, slawekbiel
Differential Revision: 555
Summary:
See task. This is a bizarre edge case which we incorrectly reject but should do
our best with.
Test Plan:
Created an added copy of a file and diffed it successfully.
Reviewed By: alex
Reviewers: alex, jungejason, tuomaspelkonen, aran
CC: aran, alex
Differential Revision: 547
Summary:
This diff adds a '--by-status' argument to arc branch that sorts the
output by status. Example output:
Accepted
my-branch Amazing change
Needs Revision
nah-nah Not so good change
Needs Review
in-progress-change I have no idea
No Revision
etc etc
Blame Rev:
Task ID: #
Reviewers: epriestley, slawekbiel
Test Plan:
Ran it with and without --by-status, saw expected output in both cases.
DiffCamp Revision:
Revert Plan:
Database Impact:
Memcache Impact:
Other Notes:
EImportant:
- begin *PUBLIC* platform impact section -
Bugzilla: #
- end platform impact -
Differential Revision: 512
Summary:
Currently, when arc-lint processes lint warnings and errors, it
accumulates the warnings into an "unresolvedMessages" array. As
soon as it sees an errors, it breaks out of the loop that does the
collection and returns, causing individual lint errors messages to
not show up in differential.
This diff collects all lint warning and error messages into the
unresolved array.
Test Plan:
arc-diff on patch that had lint errors. Verified that,
after this change, linte errors messages showed in differential.
Reviewed By: epriestley
Reviewers: jungejason, epriestley
Commenters: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 543
Summary:
Since this has auth information in it now, we should prevent other users on the
system from reading it. Detect readable files and prompt the user to fix them.
Test Plan:
Did "o+r" on my ~/.arcrc, ran "arc list", got prompted, hit "Y", verified it set
perms to 600, ran "arc list" again and wasn't prompted.
Reviewed By: jungejason
Reviewers: fratrik, jungejason, aran, tuomaspelkonen
CC: aran, jungejason, epriestley
Differential Revision: 532
Summary: This is sort of cheating, but just have this feature disable itself if the input contains multibyte UTF-8 characters. We can clean it up in the future, maybe when we have better utf8 tools.
This means that all the algorithms are safe to pass utf8 to, so we can get rid of all the "<?>" silliness.
Test Plan: Added a UTF8 character to a line, diffed it out, and got the entire line highlighted as changed. See: https://secure.phabricator.com/file/view/PHID-FILE-70fb54eb3f88dc057ab3/
Reviewers: jungejason, aran, tuomaspelkonen
CC:
Differential Revision: 514
Summary:
W293 reports a lint warning for the same problem that TXT6 reports a
lint error for. This isn't so terrible in and of itself.
However, when you are then prompted to apply a patch to fix TXT6, lint
doesn't realize that the W293 warning was also handled, and still
prompts you about ignoring unresolved warnings. This is misleading.
Test Plan:
> python pep8.py hasBlankLineWS.py
hasBlankLineWS.py:3:1: W293 blank line contains whitespace
> python pep8.py --ignore=W293 hasBlankLineWS.py
(no output)
Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: epriestley, aran
Differential Revision: 525
Summary:
Turns out you can hava a branch named |foo.
(Good that '&& rm -fR /usr' is not a valid name)
Test Plan:
created a branch |test, run arc branch
Reviewed By: epriestley
Reviewers: epriestley, dschafer
CC: aran, epriestley, slawekbiel
Revert Plan:
sure
Other Notes:
Differential Revision: 507
Summary:
This should be implemented more elegantly, but this is a mostly-reasonable
attempt at it.
Test Plan:
Ran 'arc diff --only --json' with this diff.
Reviewed By: gc3
Reviewers: gc3
CC: aran, epriestley, gc3
Differential Revision: 509
Summary:
cpojer fixed a JS instance of this in D481, but we can catch it in PHP with
XHPAST. Add a lint rule to fail if nested loops use the same iterator.
Test Plan:
Ran unit test.
Reviewed By: tomo
Reviewers: aran, pad, cpojer, jungejason, tuomaspelkonen, tomo
Commenters: cpojer
CC: aran, cpojer, epriestley, tomo
Differential Revision: 489
Summary:
This documentation isn't terribly clear and it isn't obvious how to use the
workflow. Make it more clear and provide examples.
Test Plan:
Ran "arc help call-conduit"
Reviewed By: gc3
Reviewers: gc3, aran, jungejason
CC: aran, gc3, epriestley
Differential Revision: 502
Summary:
Appending differential status, sorting, filtering and coloring git
branches.
I think it turned out rather nicely. On my repository with 70 branches
it takes 1.6s, not terrible, though 1.2s is in the conduit call - seems
like there is potential for optimization.
I didn't end up changing 'arc list', as their semmantics are slightly
different, but I'm open to ideas of consolidating them
Test Plan:
- Tested on both facebook www and arcanist repositories.
- Validated that view-all flag works
- Validated that the ordering is correct
- Validated that the statuses match the differential status.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley, slawekbiel
Revert Plan:
sure
Other Notes:
Differential Revision: 497
Summary:
When installing a certificate, it's being written to ~/.arcrc not ~/.arcconfig
Test Plan:
Installed a certificate it said the right thing.
Reviewed By: aran
Reviewers: epriestley, aran
Commenters: epriestley
CC: aran, epriestley
Differential Revision: 491
Summary:
Provide an "install-certificate" workflow to simplify ~/.arcrc edits. See also
D460.
Test Plan:
Installed certificates via "arc install-certificate".
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 465
Summary:
I looked at this quickly to see what was involved and there were only a couple
of issues so here's a patch:
- On OSX, the "-i" flag does not mean "--mime". Use "--mime" explicitly
instead. This is a minor fix which affects only OS X.
- I wasn't able to repro the "crazy executables" behavior and think it might
have been me messing something up, so nuke it until we see an issue.
- Some guid vs phid shenanigans. Differential reads "phid" but arcanist set
"guid". We should move toward "phid"; I started using "guid" before I realized
it was an overloaded term that also refers to a specific GUID implementation
(Microsoft's UUID).
Test Plan:
Uploaded an image diff, saw images in Differential.
Reviewed By: aran
Reviewers: jianfeng, jungejason, aran, tuomaspelkonen
CC: aran
Differential Revision: 466
commit template
Summary:
When users run into this, point them at the documentation explicitly.
Test Plan:
Tried to "arc diff" with a commit message of 'derp', got a live link to
documentation instead of a vague set of general instructions.
Reviewed By: aran
Reviewers: moskov, aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 468
Summary:
We have some "@generated" files of these types now, hit them with the text
linters
Test Plan:
Ran 'arc lint' on D444
Reviewed By: aran
Reviewers: aran
CC: aran
Differential Revision: 451
Summary:
This is pretty coarse and could be refined, but I often do this when testing:
lang=diff
- if (some_complicated_condition())
+ if (true || some_complicated_condition())
aran has caught me not only doing it but sending out diffs with it like 30
times. Catch it in lint instead.
Test Plan:
Unit test, added a "true || $junk" to the code and linted it.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, pad
CC: aran
Differential Revision: 447
unit'
Summary:
We want to handle 'arc unit' and 'arc diff' differently in our test
framework.
Test Plan:
Tested that with 'arc unit' the value was set to 'unit' and with 'arc diff'
the value was set to 'diff'.
Reviewed By: epriestley
Reviewers: slawekbiel, epriestley
CC: jungejason, grglr, aran, tuomaspelkonen, epriestley
Differential Revision: 430
Summary:
Differential showed 'okay' as the arc unit status even when there were
postponed tests.
Test Plan:
Tested that test results were pushed to differential when there were
postponed tests.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
CC: slawekbiel, aran, jungejason
Differential Revision: 417
Summary:
The amend process used "git log HEAD^..HEAD" to get log for the
commit being amended. When run on a merge commit this can return
any number of commits from the non-first parents. Since only a
single commit was expected, arc fails here.
This diff changes the amend process to use the '--first-parent' flag
to be consistent with using '^', which references the first parent.
This should guarantee a single commit log every time.
Test Plan:
arc amend on a merge commit
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley, andrewjcg
Differential Revision: 415
Summary:
We changed our Facebook implementation to echo test results while the
tests are running. We do not want to echo the test results twice.
Test Plan:
Tested that implementing the function in PhutilUnitTestEngine and
returning true showed the results and returning false didn't show the
results.
Reviewed By: epriestley
Reviewers: jungejason, epriestley, grglr, slawekbiel
Commenters: slawekbiel, aran
CC: sgrimm, slawekbiel, aran, tuomaspelkonen, epriestley
Differential Revision: 400
Summary:
I typed the wrong number of spaces into some of these.
Test Plan:
Visual inspection, ran 'arc unit'
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 402
Summary:
Test Engine classes might need the differential Diff ID to be
able to attach postponed test results to diffs.
Test Plan:
Added setDifferentialDiff function to PhutilUnitTestEngine class
and made sure it was called with the correct diff ID when running
'arc diff --preview'
Reviewed By: jungejason
Reviewers: jungejason, grglr
Commenters: sgrimm
CC: epriestley, sgrimm, slawekbiel, aran, tuomaspelkonen, jungejason
Differential Revision: 395
Summary:
There are a bunch of different ways we could approach this, but practically I
think this is probably the best one even though it's kind of yucky.
A big part of my motivation here is that if we just reject UTF-8 outright, users
are going to end up in a situation they don't understand how to resolve.
UPDATE: after discussion, going with a more conservative approach until such
time as we have a more compelling case for the less strict functionality.
Test Plan:
Created a diff with a text file that contained invalid UTF-8 subsequences.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 325
Summary:
Adds "--no-textconv" to all 'git diff' commands so we don't invoke textconv. See
T178 for discussion.
Test Plan:
Added something like this to .gitattributes:
*.txt diff=uppercase
And then this to .git/config:
[diff "uppercase"]
textconv = /path/to/uppercase
...where "uppercase" is a script which takes a file and emits an uppercase
version of it.
Then I added a "wisdom.txt" text file:
The cow goes "moo".
The duck goes "quack".
Without this patch, the file appears in uppercase in Differential, i.e. textconv
runs. With this patch, it appears as the original text.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: elgenie, aran, tuomaspelkonen
Differential Revision: 372
Summary:
Raise an error if an array is initialized with the same key present more than
once.
Test Plan:
bin/arc unit
Also added some duplicates to ArcanistXHPASTLinter.php and verified the output
of bin/arc lint.
Reviewed By: epriestley
Reviewers: jungejason, epriestley, aran
CC: aran, epriestley
Revert Plan:
Tags:
Differential Revision: 346
Summary:
Provides a lint class as a wrapper around the external project PyLint.
This exposes some arc config variables to control the behavior:
lint.pylint.prefix - non-standard installation location of pylint
lint.pylint.logilab_astng.prefix - non-standard installation location
of logilab-astng, a dependency of pylint
lint.pylint.logilab_common.prefix - non-standard installation location
of logilab-common, a dependency of pylint
lint.pylint.codes.{error,warning,advice} - regexes matching against
PyLint message codes which should trigger arc errors/warnings/advice
lint.pylint.options - options to pass PyLint
Test Plan:
used to lint python code
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 343
Summary:
Arc patch for image files failed, because they were treated like text
files.
Test Plan:
Tested that 'arc patch' worked for a png file.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
CC: aran, jungejason
Differential Revision: 341
Summary:
I'm probably missing some edge cases but it took me almost 3 hours to get this
far and I think it only makes things work that didn't work before. Some stuff
like SVN binary patches still won't work, although they should be far easier to
implement.
Most of the magic here just comes from reading the git source code. It appears
to work correctly; I sprinkled printf() around git liberally and recompiled it
during development. Took me about 45 minutes to figure out that "Index" vs
"index" causes git to silently fail in a confusing way. :/
Git has a diff mode for binary changes but I don't think we lose much by always
using the full binaries. We can enhance it later if we want.
Test Plan:
Exported and patched binary changes (a picture of a duck) into a working copy.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: simpkins, aran, epriestley
Differential Revision: 327
Summary:
Provide a simple linter wrapper around pyflakes. This relies on finding
pyflakes via:
- lint.pyflakes.path - arcconfig setting of absolute path to pyflakes
- lint.pyflakes.prefix - arcconfig setting of the prefix that pyflakes
was installed under
- users path
Test Plan:
linted python code with PyFlakes warnings
Reviewed By: epriestley
Reviewers: jungejason, epriestley
Commenters: jungejason
CC: aran, epriestley, andrewjcg, jungejason
Differential Revision: 310
Summary:
passthru() doesn't show up on --trace, and one time a while ago someone had an
issue which was harder than necessary to debug because the command wasn't
avialable in the log. Use the logged version.
Also fix a locale issue I ran into on my local machine, with "en_US.utf8" not
being a valid locale.
Test Plan:
Created an SVN repo and used "arc commit" to make commits against it.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 306
Summary:
This diff:
- Adds the PEP8 linter to the externals directory
- Changes the path for finding pep8.py
- Removes use of execx since pep8.py return an errors code
when it finds PEP8 violations
Test Plan:
tested linting python code
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 309
Summary:
The --only flag implies --nounit and --nolint, so there's really no
conflict if these flags are specified together.
(My wrapper around arc diff automatically specifies --nounit in some
cases. It is useful to be able to manually add --only in some cases,
and not have it fail because it conflicts with the automatically
specified --nounit flag.)
Test Plan:
Ran "arc diff --only --nounit".
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley, simpkins
Differential Revision: 298
Summary:
Update the lint workflow to exit immediately it there is no lint engine
configured. Previously it scanned the repository to find the paths to
check before failing in this case. Scanning the repository can be
relatively slow on large repositories.
Test Plan:
Ran "arc lint" in a large repository with no lint engine configured.
Verified that it failed quickly, without scanning the repository first.
Reviewed By: jungejason
Reviewers: jungejason, epriestley
CC: aran, jungejason
Differential Revision: 297
Summary:
The story for creating and maintaining libphutil libraries and modules
is pretty terrible right now: you need to know a bunch of secret scripts and
dark magic. Provide 'arc liberate' which endeavors to always do the right thing
and put a library in the correct state.
Test Plan:
Ran liberate on libphutil, arcanist, phabricator; created new
libphutil libraries, added classes to them, liberated everything, introduced
errors etc and liberated that stuff, nothing was obviously broken in a terrible
way..?
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 269
Summary:
This isn't necessarily a root-cause solution but this syntax is really really
bad. If we want to fix the root cause, I'd recommend making its use a lint
error?
Test Plan:
Unit tests failed before patch, passed afterward.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 270
Summary:
This is necessary for the Javelin linters. The libphutil and flib linters do it
implicitly, so there's no real tradeoff here.
Test Plan:
Ran javelin linters.
Reviewed By: tuomaspelkonen
Reviewers: tomo, aran, jungejason, tuomaspelkonen
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 238
Summary:
I only dropped this because it's slightly inconvient to accommodate, but
empirically it's pretty confusing to users (who often use --diff 12345 when they
mean --revision 12345).
Test Plan:
Ran "arc patch D45", "arc patch --revision 45", "arc help patch"
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 241
Summary:
Provide a formal mechanism for making conduit calls from other scripts.
Test Plan:
Called 'conduit.ping'.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 239
Summary:
the current command to apply a git diff is 'git apply --index',
which will fail the whole patch and does not touch the working tree when
some of the hunks do not apply. We want to allow the user to patch the
ones which apply.
Test Plan:
run 'arc apply' against a diff whcih partially applies and
verify that the hunks which apply are applied, and .rej files are
generated for the rest.
Reviewed By: simpkins
Reviewers: simpkins, epriestley
Commenters: ju
CC: aran, slawekbiel, simpkins, ju
Differential Revision: 235
Summary:
I'm making a move on Lisk stubbability and need these so I can set up
Lisk isolation unconditionally in Phabricator tests. Also document and organize
this class.
Test Plan:
Changed the expected counts in the meta-tests and got failures. Ran
all unit tests.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 192
Summary:
Displaying each postponed test separately was spammy.
Now it will only display a single line with the total count
Test Plan:
~/local/arcanist/bin/arc unit --sandcastle-tests
35 tests to run.
Completed 'ParentInfoTestCase.php'
Interrupted 'MentionsUntagTestCase.php'
PASS ParentInfoTestCase
POSTPONED 34 tests
Reviewed By: sgrimm
Reviewers: jungejason, sgrimm
CC: sgrimm
Revert Plan:
sure
Other Notes:
Differential Revision: 173
Summary:
Sometimes we need to show abbreviated hashes, passing --short is more
reliable than substring of fixed number of characters.
Test Plan:
- called with and without the parameter, got correct results
-
Reviewed By: jungejason
Reviewers: jungejason
CC: jungejason
Revert Plan:
sure
Other Notes:
Differential Revision: 170
Summary:
Modifying a new file and running 'arc cover' before committing the
modifications confused arc. The problem was that 'unstaged' status
erased 'added' status and this caused problems.
Test Plan:
Tested that arc cover ignores added files when they are modified, but
old files that are modified are still handled correctly by arc cover.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 122
Summary:
Declare victory like it's the United States.
Test Plan:
www copyright, apache copyright, control keyword whitespace and trailing
whitespace lint warnings
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 117
Summary:
I simplified this code at some point and broke it horribly
in the process. Mask in the right flag here and combine the maps
correctly. This solves the great mystery of increased developer
clowniness.
Test Plan:
Ran 'arc diff' in a working copy with staged but
uncommitted files, got an error message (previously, I didn't).
Reviewed By: aran
Reviewers: mroch, aran
CC: aran
Differential Revision: 104
Summary:
Refactor ArcanistLintRenderer into three independent classes, and let
the Workflow select between them based on its 'output' parameter.
Test Plan:
Introduce a Lint warning and lint with no --output, with --output summary and
with --output JSON.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 96
Summary:
That's for the tests that will be run asynchronously.
Test Plan:
run arc unit with some tests that returned postponed status - they showed up.
Background was yellow.
Reviewed By: epriestley
CC: epriestley
Revert Plan:
sure
Other Notes:
Differential Revision: 77
Summary:
Test Plan:
$ arc patch --diff 553961
Patch deletes file 'html/miniprofile.php', but the file does not exist in
the working copy. Continue anyway? [y/N] y
Patch deletes file 'html/index.php', but the file does not exist in the
working copy. Continue anyway? [y/N] y
OKAY Successfully applied patch to the working copy.
Reviewers:
CC:
Summary: this was all kinds of fail when running from a subdirectory or on
invalid paths. Detect nonexistent paths; resolve valid paths.
Test Plan: ran "arc lint --lintall" on a file from a subdirectory, and on a
file which did not exist. Received patches and an error, respectively
Reviewers: arudolph
CC:
Differential Revision: 72
Summary:
This is realistically always wrong and the author means "."
Test Plan:
lint / unit
Reviewed By: crackerjack
Reviewers: crackerjack, aran
CC: crackerjack
Differential Revision: 68
Summary:
When an arcbundle includes a symlink, we fail to apply it correctly
when applying to a subversion working copy.
Test Plan:
created a diff which added a symlink, removed it locally, bundled
and applied the patch, got a good symlink out
Reviewed By: aran
Reviewers: aran
CC: epriestley, aran
Differential Revision: 63
Summary:
If you apply an arcbundle to a subversion working copy which adds
files in new subdirectories, we fail to create and add the parent directories
so the whole operation fails. Make sure we create and add any missing parent
directories before apply patches.
Test Plan:
Applied Facebook diff #542056 to www@rE349795 cleanly, while it
failed previously.
Reviewed By: aran
Reviewers: aran
CC: aran
Differential Revision: 62
Summary:
SVN has a special 'replaced' status which was not being parsed
correctly. Parse it correctly.
Test Plan:
Replaced a file, ran arc diff, got a sensible diff.
Reviewed By: aran
Reviewers: aran
CC: aran
Differential Revision: 60
Summary:
Git (the world's hardest revision control system) allows you to change
output formats by accident and/or without your direct knowledge. Protect users
from themselves.
Test Plan:
Changed "pretty" in [format] to "format:quack" so every log just
outputs the word "quack". Ran "arc diff" successfully.
Reviewed By: aran
Reviewers: aran
CC: epriestley, aran
Differential Revision: 56
Summary:
XHPAST encounters a parse-depth problem on some files because PHP
5.2 has an un-overridable parse depth limit for JSON. The text of this error
says it is a "warning" but we currently raise an error. Make it a warning
instead.
The JSON depth is 20 until PHP 5.2.3, where it becomes 128. After PHP 5.3
it defaults to 512 and is user-configurable, which will allow us to resolve
this issue in nearly all cases.
Since I made if/else express as a list in the AST, this only actually arises
in long binary chains, most commonly string concatenation, like:
$out = 'a'.'a'.'a'.'a'...
...where each string is a variable or HTML tag and the program is constructing
a complicated document.
At some point I'll add some PHP 5.3 massaging to the XHPAST decoder itself to
raise this limit to something more huge.
Test Plan:
Ran "arc lint --lintall" on a file with a very deep binary
expression tree ("1 + 1 + 1 ...") and received a warning instead of an error.
Reviewed By: aran
Reviewers: pad, aran
CC: epriestley, aran
Differential Revision: 54
directories
Summary:
If you had changes to a directory covered by a .arcconfig residing
inside another directory also covered by an .arcconfig, the hook would
incorrectly attribute the first file it attempted to resolve to the topmost
.arcconfig because it failed to break out of the loop after resolving it.
All other files would be correctly attributed, because they'd hit the cache.
Test Plan:
Created an svn repository and checked it out locally. Created a
structure like this:
.arcconfig
shallow
a/
b/
c/
.arcconfig
d/
deep
deep2
deep3
Made modifications to "deep", "deep2", and "deep3". Received an error message
about multiple .arcconfig changes, attributing one file to the topmost
arcconfig and the other two to the deeper one.
Applied patch.
Commit now goes through. Made a commit affecting 'shallow' and 'deep' and
'deep2', commit was correctly blocked and files were attributed to the
corresponding .arcconfigs.
Reviewed By: mroch
Reviewers: mroch
CC: mroch
Differential Revision: 50
Summary: Haiping is getting a pretty confusing error message when trying to
commit.
Test Plan: Created a mock repository, installed the hook, made commits against
directories with bad .arcconfigs.
Reviewers:
CC:
Summary: When running "arc diff" in a mixed-base-revision working copy, we
prevent the operation. Relax this restriction so that having a different root
revision is okay, so long as all affected files share the same revision. This
facilitates multiple similar edits without updates.
Test Plan: Reverted a file to an older revision in an SVN working copy, ran
"arc diff", was rejected. Applied patch, ran "arc diff", diff went through and
was recorded with the right SVN base revision in the database.
Reviewers:
CC:
Summary: We sync from local right now, which is fairly terrible. But
fixing this correctly is also not trivial and I don't have the right
primitives yet.
Test Plan:
Reviewers:
CC:
Summary: The biggest blocker on getting rid of arc in trunk is that the lint
rules in the commit hooks are still running the old version. Push arc
commit hook support toward some reasonable state of approximately working.
Test Plan:
Reviewers:
CC:
Summary: The multi-line comment regexp was potentially too greedy. See
"greedy.lint-test".
- Made it less greedy.
- Added test coverage.
- Fixed an issue with the Apache license getting applied with too much
whitespace against C files.
Test Plan: Ran unit tests.
Reviewers: aran
CC:
Differential Revision: 36
Summary:
Tweak license linter to support licenses on line immediately after <?php
Test Plan:
Corrupt an apache license and re-lint twice.
Differential Revision: 34
Reviewed By: epriestley
Reviewers: epriestley
Summary:
"-m ''" will still let you do a truly empty update if
you are insistent on that.
Test Plan:
meta
Differential Revision: 209194
Reviewed By: dschleimer
Reviewers: dschleimer
CC: epriestley
Revert Plan:
OK
Summary:
When you add new arguments to the unit command, they need to be
threaded through to the unit engine. This enables you to specify passthru
command which will be shipped through the callstack.
Test Plan:
Ran 'arc diff --maxtests 8 --apply-patches --trace' and verified
resulting behavior was correct.
Differential Revision: 207877
Reviewed By: dschleimer
Reviewers: dschleimer
CC: dschleimer
Revert Plan:
OK
Summary:
This should only happen on the 'diff' workflow.
Test Plan:
ran 'arc lint' and didn't get prompted to amend, ran 'arc diff' and got yelled
at for uncommitted changes, committed, ran 'arc diff' and got prompted to amend
Differential Revision: 206696
Reviewed By: adonohue
Reviewers: adonohue
CC: adonohue
Revert Plan:
OK
Summary:
Adds data-driven shell completion help to arcanist.
Test Plan:
ran various commands in git and svn working copies,
output seemed reasonable
Differential Revision: 201754
Reviewed By: adonohue
Reviewers: mroch, adonohue
Commenters: crackerjack
CC: epriestley, adonohue, achao
Revert Plan:
OK
Summary:
Be more clear about what happened.
Test Plan:
ran 'arc patch' on good/bad patches
Differential Revision: 201085
Reviewed By: adonohue
Reviewers: adonohue
CC: adonohue
Revert Plan:
OK
Summary: Send skipped lint warnings to Differential. This also fixes a nasty
bug with lint excluding too many warnings based on line changes.
Test Plan: meta
Reviewers:
CC:
Differential Revision: 200387
Summary: We have some diffs which predate property tracking and are shipping
down sketch data, work around it in the client for now.
Test Plan:
Reviewers:
CC:
Summary: These flags let you force the behavior of 'arc lint' instead of
prompting. Also made the amend behavior default to false since I've screwed
this up about 300 times in the mere hours I've been using git as a primary VCS.
Test Plan: ran arc lint with these flags
Reviewers:
CC:
svn.
Summary: when you "svn cp" with a revision, we end up with some path collision
problems. This isn't a complete fix but it gets the change to Differential,
at least.
Test Plan:
Reviewers:
CC:
broken __init__.php.
Summary: This allows phutilmodulelinter to generate missing __init__.php and
recover from missing modules without horrible fataltown.
Test Plan:
Reviewers:
CC:
