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:
A user filed an issue on GitHub about not having 'json_decode()':
https://github.com/facebook/arcanist/issues/10
Detect this issue and raise a detailed error message.
Test Plan:
- Set minimum version to 6.0.0, arc told me to upgrade.
- Changed function/flag configuration and hit the flag and no-flag error
pathways.
Reviewers: btrahan, jungejason, rm
Reviewed By: rm
CC: aran, rm, epriestley
Differential Revision: https://secure.phabricator.com/D1267
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: (At least my build of) PHP tries to do readline magic on the terminal
when invoked from the shell, even inside bash's command substitution operator.
Work around it by piping the output of ##echo## to ##php##, so the input isn't a
terminal.
Test Plan: ##bash$ arc li<TAB>## now gives me possible completions rather than
breaking and inserting a literal tab
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1096
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: Make this text a little more informative, this stuff is fairly stable
now and has actual documentation available.
Test Plan: Read the document.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 1086
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
