Summary: If user changes the file contents during linting (usually when prompted to apply a patch) then we save the old messages to the new file contents. Fix that by computing the hash before linting (or after applying patch).
Test Plan: Changed the file during linting, verified that the file hash didn't change.
Reviewers: epriestley
Reviewed By: epriestley
CC: ptarjan, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5320
Summary: I looked at the pros & cons at adding hooks in git/hg vs arc land and I prefer arc land.
Test Plan:
* added an event listener and made sure I could handle the event.
* made sure things get reverted when the event handler throws an exception.
Reviewers: vrana, epriestley, pieter
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5268
Summary: The message suggests that only one revision would land.
Test Plan: Read it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5277
Summary:
This probably indicates some none fatal error, e.g.:
> remote: Certificate invalid: name is not a listed principal
The best thing here would be to avoid the error but we shouldn't explode even if it is there.
I tried to mute the error from the output but didn't find a switch or config option to do it.
Test Plan:
$ hg outgoing --branch default --style default
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5267
Summary: Have arc land inspect the revision if it depends on some other revisions which haven't been closed yet. If yes, then warn users.
Test Plan: Will test them locally.
Reviewers: epriestley, AnhNhan
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5262
Summary: This shell script needs more quotes.
Test Plan: Ran "arc" with `arcanist/` inside a directory called `s p a c e s`.
Reviewers: irinav, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5265
Summary: unlike the hooks I copy/pasted, these hooks have parameters
Test Plan: read it
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5264
Summary: These hooks allow test cases to build shared resources -- notably, database fixtures.
Test Plan: See next diff.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5258
Summary: We don't set $paths when running --everything.
Test Plan: Ran `arc unit --everything` with coverage.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5260
Summary: Also allow `sTaTiC::$x` which is a valid PHP (contrary to `sElF::$x` which is invalid PHP but I allowed that too).
Test Plan:
new self;
Reviewers: epriestley
Reviewed By: epriestley
CC: wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5241
Summary:
Adds arc.feature.start.default arcconfig setting to specify
a default value for 'start' in 'arc feature name start'. This lets
users always branch from origin/master (or whatever the main branch is).
Also cleaned up the 'feature' help text a little. The stuff about sorting
and closed/abandoned revisions is explained via the options list already.
Test Plan:
Ran arc feature with/without a start and with/without the config
setting set.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: bos, sid0, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5184
Summary:
Added a lint rule that warns about reusing iterator reference
variables.
Test Plan:
- Add a file with examples found in with https://secure.phabricator.com/T2536
- Did not make a unit test yet
Reviewers: vrana, bill, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Maniphest Tasks: T2536
Differential Revision: https://secure.phabricator.com/D5179
Summary:
When using arc feature, it should set up the tracking branch to be
master.
Test Plan:
./bin/arc feature tracking
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5169
Summary: These are the errors I really do.
Test Plan:
$ arc diff --no-lint
(Assuming '--no-lint' is the British spelling of '--nolint'.)
$ arc diff --reviewer a
(Assuming '--reviewer' is the British spelling of '--reviewers'.)
New unit test.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5185
Summary:
There was an accidental ! in the phase vs outgoing condition
which caused it to use 'hg outgoing' when it should have used the draft()
phase. Fixing this shaves 4.5 seconds off 'arc diff' on large repos.
Test Plan:
Ran arc diff --trace. Noted that the draft() was used and that the diff
contained the correct files and commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5182
Summary: Changes message for `arc land` that displays current branch or bookmark (if none is specified) to appropriately use the term 'bookmark' when on a bookmark in an hg repository.
Test Plan:
Ran `arc land` on new git and hg repositories, checking for correct identification of 'branch' or 'bookmark'.
~/test$ mkdir hg-test
~/test$ mkdir git-test
~/test$ cd hg-test
~/test/hg-test$ hg init
~/test/hg-test$ hg branch
default
~/test/hg-test$ hg bookmarks
no bookmarks set
~/test/hg-test$ arc land
Landing current branch 'default'.
Usage Exception: You can not land a branch onto itself -- you are trying to land 'default' onto 'default'. For more information on how to push changes, see 'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the documentation. You may be able to 'arc amend' instead.
~/test/hg-test$ hg bookmark testmark
~/test/hg-test$ hg bookmarks
* testmark -1:000000000000
~/test/hg-test$ hg branch
default
~/test/hg-test$ arc land
Landing current bookmark 'testmark'.
Usage Exception: Source testmark is a bookmark but destination default is not a bookmark. When landing a bookmark, the destination must also be a bookmark. Use --onto to specify a bookmark, or set arc.land.onto.default in .arcconfig.
Confirm still works on a git branch:
~/test/hg-test$ cd ../git-test/
~/test/git-test$ ls
~/test/git-test$ git init
Initialized empty Git repository in ~/test/git-test/.git/
~/test/git-test$ touch testfile
~/test/git-test$ git commit -am 'Test file'
~/test/git-test$ git branch
* master
~/test/git-test$ arc land
Landing current branch 'master'.
Usage Exception: You can not land a branch onto itself -- you are trying to land 'master' onto 'master'. For more information on how to push changes, see 'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the documentation. You may be able to 'arc amend' instead.
Reviewers: DurhamGoode, epriestley
Reviewed By: DurhamGoode
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D5163
Summary:
Some tests take longer (fixtures usually around 1 second for me) and also FB runs all tests on deploy.
I want to see all results immediately.
Test Plan:
Added `usleep(200000)` to `resultTest()`, then:
$ arc unit
Saw results printed one by one.
Also didn't pass `$renderer` to `ArcanistPhutilTestCase` and saw empty output.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5141
Summary:
arc diff called 'hg cat' twice for every image binary in the diff.
This turns out to take 1 second per call on a large repo because mercurial
has to parse the manifest every time.
Now arc diff batches up all the files and does only two 'hg cat'
commands. This makes the cost constant relative to the number of
images being uploaded.
Test Plan:
Ran arc diff on a diff with 30 images on both git and hg.
Verified that it was fast and that the images showed up in the web ui.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, dschleimer, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5144
Summary:
Previously, running arc lint on a set of changes that only
existed in your working copy threw an exception in mercurial repos.
It was trying to use the revset "...." (i.e. the range from . to .),
which didn't parse. Even if I fix that it still doesn't work because
getRawDiffText did not include the working copy changes (which it does
in git). I removed the check so the function now acts the same as in
git and arc lint works on working copy changes. I've seen this error before
in other places so hopefully this change will also fix any other areas,
that depended on getRawDiffText working the same as git.
The logic I removed was added in D1954 to support diffing against
uncommited changes. That workflow should be unchanged. arc diff will
still prompt the user if there are uncommited changes, and the user can
still choose to abort or continue.
Let me know if I missed something important which makes this a bad idea.
Test Plan:
Edited a file in the working directory of a hg repo.
arc lint
Verified lint ran successfully.
Also ran arc diff and land with and without working copy changes to make sure
they still work. I'd kill for some tests in this area...
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: dschleimer, bos, sid0, aran, Korvin
Maniphest Tasks: T1631
Differential Revision: https://secure.phabricator.com/D5130
Summary:
People constantly forget to bump the linter version and I don't see a way how to stop it.
This may bump the version even if it wouldn't be required but let's rather undercache than overcache.
Test Plan: `var_dump($version)`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5129
Test Plan: Threw in `didRunLinters()` of one linter, still saw the result of other linters and "Some linters failed" at the end.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5124
Summary:
I want to run lint on background and I'm interested only in side effect of caching (and maybe exit status).
This is better than discarding stdout later because we don't do unnecessary work and error conditions are still printed.
Test Plan:
$ arc lint --output none # with error
$ echo $?
$ arc lint --output none # with no lintable paths
$ arc lint --output none # witout errors
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5106
Summary:
The perf fix actually catches some real problems.
I didn't find anything in libphutil, Arcanist and Phabricator though.
Also bump version.
Also allow configuring the hook.
Test Plan:
Added a test, saw it fail with the old code.
Repeat for hook.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5097
Summary: If there's other revision in last commit message and I said `--create` then it is clear that I want to create a new commit.
Test Plan:
$ arc diff --create # in dirty working copy on top of my open revision
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5078
Summary: At least on my sample file.
Test Plan: Saw time 0.073 s instead of 12.606 s.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5086
Summary: If I have //Differential Revision// in my commit message then `arc diff --create` updates that revision instead of creating a new one.
Test Plan:
$ arc diff --create # on top of commit message with Differential Revision
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5077
Summary: This number seems more interesting and it includes time for resolving futures which is the main part of some linters.
Test Plan:
$ arc --trace lint
Reviewers: fdeliege, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D5075
Summary: Price transpositions very cheaply. We might need to edit these weights a bit, but this covers the two previous cases ("test", "alnd") and gets them right.
Test Plan: Unit tests, various `arc x` tests.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5034
Summary: Currently, if we match "alnd" to "land" and "amend" equally (distance 2), we drop "land" with the other part of the rule. Stop doing that.
Test Plan:
Unit tests. Also:
```
$ arc alnd
Usage Exception: Unknown command 'alnd'. Try 'arc help'.
Did you mean:
amend
land
```
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5033
Summary:
The message is too defensive.
Test Plan: Tested in the fork.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin, s.o.butler
Differential Revision: https://secure.phabricator.com/D5043
Summary: This is a little bit tricky - if both XHPAST and PhutilXHPAST linters lint the same path then they get the same future wrapped in two different Future iterators.
Test Plan:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5015
Summary: Nobody needs it because `raiseLintAtLine()` returns the message.
Test Plan:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4870
Summary:
The arc feature command wasn't actually creating bookmarks
on mercurial. It needs to call 'hg bookmark' instead of 'hg update'
Also removed an unnecessary hgsprintf since there were no arguments.
Test Plan:
Ran 'arc feature foo' and 'arc feature bar tip^'.
The former created a bookmark at my current location.
The latter created a bookmark at tip^ and moved me to that revision.
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5023
Summary:
Make all the broad-spectrum text linters use the new binary check.
I didn't touch `ComprehensiveLintEngine` because it's a nest of vipers and no one has complained; see T2039.
Test Plan: Ran `arc lint` on text files and binaries in a libphutil project (arcanist).
Reviewers: lisp
Reviewed By: lisp
CC: aran
Differential Revision: https://secure.phabricator.com/D5040
Summary:
Add linting capability for detecting files which contain
syntax introduced by unresolved merge conflicts. The detection is
file-type-agnostic (the only requirement is that the file is a text
file).
Test Plan:
Tested in three ways.
The first way is to add all three forms of syntax to a file to
indicate a merge conflict. HPHP will pick this up as a syntax error
before this linter reaches it.
The second way is to add the syntax in a comment. In that case, this
linter will show three warnings. For example:
$ arc lint ArcanistMergeConflictLinter.php
>>> Lint for arcanist/src/lint/linter/ArcanistMergeConflictLinter.php:
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
20
21 foreach ($lines as $lineno => $line) {
22 /*
>>> 23 >>>>>>>
24
25 =======
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
22 /*
23 >>>>>>>
24
>>> 25 =======
26
27 <<<<<<<
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
24
25 =======
26
>>> 27 <<<<<<<
28
29 */
The last test was to test on various different file types, including
JavaScript, PHP, an animated GIF, a PNG, and a Bash file to make sure
the file type detection worked. Each of the aforementioned tests
passed.
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Maniphest Tasks: T2547
Differential Revision: https://secure.phabricator.com/D4966
Summary: We were incorrectly matching `$` in the regexp against a possible `\r\n`. I missed this earlier when trying to catch all of these.
Test Plan:
- Added unit test and made it pass.
- Did another search for `getLine()` to see if I could spot any more of these, but failed to identify any via inspection.
Reviewers: vrana, mbishopim3
Reviewed By: mbishopim3
CC: aran
Differential Revision: https://secure.phabricator.com/D5038
