Summary:
Ref T3186. Ref T2039. Continues work on readying linters for `.arclint`.
- **Ruby**: Make this an ExternalLinter.
- **Priority**: Currently, linters have an implicit "correct" order (notably, the "NoLint" linter needs to run before other linters). Make this explicit by introducing `getLinterPriority()`.
- **Binaries**: Currently, linters manually reject binary files. Instead, reject binary files by default (linters can override this if they do want to lint binary files).
- **Deleted Files**: Currently, linters manually reject deleted files (usually in engines). Instead, reject deleted files by default (linters can override this).
- **Severity**: Move this `.arclint` config option up to top level.
- **willLintPaths()**: This method is abstract, but almost all linters provide a trivial implementation. Provide a trivial implementation in the base class.
- **getLintSeverityMap()/getLintNameMap()**: A bunch of linters have empty implementations; these are redundant. Remove them.
- **Spelling**: clean up some dead / test-only / unconventional code.
- **`.arclint`**: Allow the filename, generated, nolint, text, spelling and ruby linters to be configured via `.arclint`.
Test Plan:
458beca3d6
Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: Firehed, aran
Maniphest Tasks: T2039, T3186
Differential Revision: https://secure.phabricator.com/D6805
Summary:
The existing heuristic checks for the existence of .hg/svn, but it
turns out that this directory can exist in a non-svn hg repo if the user or a
script ever runs a 'hg svn' command.
Now we check for a file inside .hg/svn. The hgsubversion maintainer said this
particular file will always be present in hgsubversion repos.
Test Plan:
arc land --trace
Verified it used 'hg push' and not 'hg push -r ...'. This indicates it
considered the repo to be an hgsubversion repo.
Reviewers: epriestley
Reviewed By: epriestley
CC: dschleimer, sid0, Korvin, aran
Differential Revision: https://secure.phabricator.com/D6807
Summary:
Ref T3186. Brings another linter onboard. This one uses the stdin stuff.
The unit test was ostensibly broken so I fixed it, but that might just be some kind of version issue.
Test Plan: Unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3186
Differential Revision: https://secure.phabricator.com/D6802
Summary:
Ref T3186. We have about 50 linters which run programs and read the results, all of which have ad-hoc one-off custom config that isn't formalized anywhere.
Consolidate all this stuff into `ArcanistExternalLinter`, which is configurable through `.arclint` (although nothing supports this quite yet).
Extend CSSLint and Pep8Lint from `ArcanistExternalLinter`.
Add unit tests for both.
There are still some rough edges here, but it mostly seems to work pretty well.
Test Plan: Ran unit tests, hit some (most?) of the error cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran, Firehed
Maniphest Tasks: T3186
Differential Revision: https://secure.phabricator.com/D6800
Summary:
Ref T3186.
- Every linter builds a WorkingCopyIdentity in the same way, with no specialized data. Don't do that.
- Linters get passed a goofy hardcoded ".php" path. Don't do that.
- Linters generally run on an imaginary path, which might not work. Just give them a real path by building a tiny working copy in `/tmp`.
- Fix a TODO now that we have better typechecking.
Test Plan: `arc unit --everything`, intentionally broke a test to make sure that still works.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3186
Differential Revision: https://secure.phabricator.com/D6798
Summary:
Ref T2039. That task has a bunch of discussion, but basically we do a poor job of serving the midrange of lint configuration right now.
If you have something simple, the default linters work.
If you have something complex, building your own engine lets you do whatever you want.
But many users want something in between, which isn't really well accommodated. The idea is to let you write a `.arclint` file, which looks something like this:
{
"linters" : {
"css" : {
"type" : "csslint",
"include" : "(\.css$)",
"exclude" : "(^externals/)",
"bin" : "/usr/local/bin/csslint"
},
"js" : {
"type" : "jshint",
"include" : "(\.js$)",
"exclude" : "(^externals/)",
"bin" : "support/bin/jshint",
"interpreter" : "/usr/local/bin/node"
}
}
}
...which will provide a bunch of common options around lint severity, interpreter and binary locaitons, included and excluded files, etc.
This implements some basics, and very rough support in the Filename linter.
Test Plan:
Generated a `.arclint` file and saw it apply filename lint correctly. Used `debug` mode and tried invalid regexps.
{
"debug" : true,
"linters" : {
"filename" : {
"type" : "filename",
"exclude" : ["@^externals/@"]
}
}
}
Next steps include:
- Provide an external linter archetype (T3186) and expose a common set of configuration here ("bin", "interpreter", "flags", "severity").
- Provide a `.arcunit` file which works similarly (it can probably be simpler).
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D6797
Summary: Found an issue where if arc unit runs phpunit and the datasets included a dataprovider which spanned multiple lines, it wasn't filtered out correctly in the result parser, this fixes it
Test Plan: accidentally ran tests against phpunit itself which exhibits this problem. no longer a problem after this fix, nothing else breaks.
Reviewers: epriestley
Reviewed By: epriestley
CC: aurelijus, epriestley, aran
Differential Revision: https://secure.phabricator.com/D6773
Summary:
arc for mercurial on windows was broken in several way.
Executing a command via passthru failed because passthru on windows
skips the shell so 'set HGPLAIN=1 & ...' was an invalid command. The
fix was to just not set HGPLAIN for passthru commands on windows.
Also removed hardcoded '' quotes in mercurial commands since windows
doesn't support single quots.
Test Plan: arc land --hold on a windows machine
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6763
Summary: Corrects relative vs absolute branch name when using 'arc commit'
Test Plan: 'arc commit' on a release branch gave an error before making the change (change was generated from 'branches/yyy' but working copy root is 'https://xxx/branches/yyy', now it does not.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6743
Summary: My recent change adding --everything to arc lint could sometimes cause a "diff is empty" error, this patch fixes it.
Test Plan: Ran "arc lint --everything" before and after patch. No longer errors out. Only appeared to originally happen when there were uncommited changes in an svn repo.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6732
Summary:
When trying to find an unused bookmark we append hyphened suffixes to the end of the bookmark name. In Mercurial, these are treated the same as the bookmark name without the suffix, here's the output from hg log -r:
```
[mehdi] HGPLAIN=1 hg log -r "arcpatch-1"
abort: unknown revision 'arcpatch'!
[mehdi] HGPLAIN=1 hg log -r "arcpatch_1"
abort: unknown revision 'arcpatch_1'!
```
Test Plan: None.
Reviewers: epriestley, dschleimer
Reviewed By: dschleimer
CC: aran, Korvin, DurhamGoode, dschleimer
Differential Revision: https://secure.phabricator.com/D6698
Summary:
If you are trying to commit someone else's diff, arc commit gives warnings about path mismatch. This changes the path comparison to be based on the repo url rather than the local working directory. E.g. if both the author and committer are working in branches/release/2013_08_07 despite being checked out in ~/dev/2013_08_07 (system user being different, of course) it no longer warns that the WC path is different
Original behavior:
eric@Eric-MBP ~/dev/2013_07_31: arc commit --revision 21
You are not the author of 'D21: WeMerge Automatic Request'. Commit this
revision anyway? [y/N] y
Revision 'D21: WeMerge Automatic Request' was generated from '', but
current working copy root is '/Users/eric/dev/2013_07_31/'. Commit this
revision anyway? [y/N] y
Committing 'D21: WeMerge Automatic Request'...
Adding test
Transmitting file data .
Committed revision 52676.
Closing revision D21 'WeMerge Automatic Request'...
Exception
ERR-CONDUIT-CORE: You can not mark a revision you don't own as closed.
(Run with --trace for a full exception trace.)
New behavior:
eric@Eric-MBP ~/dev/2013_07_31: arc commit --revision 24
You are not the author of 'D24: WeMerge Automatic Request'. Commit this
revision anyway? [y/N] y
Committing 'D24: WeMerge Automatic Request'...
Adding test
Transmitting file data .
Committed revision 52679.
Closing revision D24 'WeMerge Automatic Request'...
Exception
ERR-CONDUIT-CORE: You can not mark a revision you don't own as closed.
(Run with --trace for a full exception trace.)
Test Plan: 'arc diff' changes with one user. 'arc patch Dxx' on a different working copy by a different user to review and test changes. accept review. 'arc commit --revision xx' as reviewer to land the patch. complaint goes away.
Reviewers: epriestley, ghostwriter78
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D6665
Summary: Update and clarify precedence of CA bundles
Test Plan:
tested with "arc call-conduit user.whoami" from project root
Same from other subdirectory in project
Repeated tests with both https.cabundle and https.cacert settings, both in
.arcconfig and ~/.arcrc
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3668
Differential Revision: https://secure.phabricator.com/D6647
Summary:
Support for no output from arc unit (For scripts, etc).
Also include --output param, analogous to arc lint --output.
Test Plan: run all 6 variants + `--output bad-value`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6642
Summary:
Single quotes aren't valid in the windows cmd prompt, so arc feature
didn't work in mercurial when it got to this line.
I have no idea why %C was used before. Nothing in that string should be
broken by the escaping.
Test Plan:
Ran arc feature --trace on my mac. Verified the command was escaped correctly
and the correct feature results were printed.
I don't have a windows machine to try it on, but the builtin escaping should
now account for windows machines.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, aran, Korvin
Differential Revision: https://secure.phabricator.com/D6637
Summary:
When adding arcanist support to a new project or adding a new linter,
it's helpful to be able to run new linters against the entire codebase. This
patch adds support for this with an '--everything' option, similar to 'arc unit
--everything'
Test Plan:
Run 'arc lint --everything' and check out the code. Optionally dump
the paths to test in the current lint engine's buildLinters() function to
demonstrate that it's receiving all files in the project rather than just the
changed and/or specified ones
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D6592
Summary: The test result parser in PhpunitTestEngine was receiving $test_path from the previous loop instead of $path from the current one. The variable isn't actually used in the PhpunitResultParser object (it exists for strict compatibility with the parent class) so it didn't cause any problems, but who knows if that could change in the future
Test Plan: Review diff. No changes to the output of running 'arc unit' when using the Phpunit engine, as expected
Reviewers: epriestley
CC: aran, epriestley, aurelijus, chad
Differential Revision: https://secure.phabricator.com/D6587
Summary: We use custom `assert*` functions here and there. Remove them from backtrace.
Test Plan: Ran `XHPASTTreeTestCase`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6411
Summary:
I suspect that when people use 'algorithmical', they mean it as an adjective and not an adverb.
'algorithmic' = adjective
'algorithmically' = adverb
Test Plan: Add the word 'algorithmical' to a file. Run `arc lint` on the file. See suggestion to correct it to 'algorithmic'.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6373
Summary:
- Replace `maniphest.find` with `maniphest.query`. These calls are nearly identical, it was just a rename for consistency.
- Replace `differential.find` with `differential.query`.
Test Plan:
- Ran `arc tasks`.
- Ran `arc close-revision` on valid, nonexistent, and existent-but-invalid revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6336
Summary:
I wanted to write a couple of workflows that shared some code in an abstract superclass, but ##arc## would fail trying to instantiate the abstract class.
As it turns out, we can modernize the ##buildAllWorkflows## function a bit, and it will only load concrete objects.
Test Plan:
1. Define an abstract subclass of ##ArcanistBaseWorkflow##.
2. ##arc liberate## the source directory that contains it.
3. Try any ##arc## operation without this diff, and see it fail.
4. Patch this diff and see that ##arc## operations work now.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6324
Summary: Makes sense now
Test Plan: Get someone else to test it
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T3342
Differential Revision: https://secure.phabricator.com/D6307
Summary: Enables the process to pass the user input for library name. Fixes T3342
Test Plan: Little help... `arc liberate` doesn't run on windows. Though I saw the flag :)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3342
Differential Revision: https://secure.phabricator.com/D6306
Summary:
When `arc diff` runs unit tests it uses all of the affected files as the base array of paths. These files may have been deleted. If the deleted file fits the test criteria `unit` will try and run the test and in some cases fail.
An example of this fataling is with PHPUnit;
> Fatal error: Uncaught exception 'Exception' with message 'JSON report file is em
pty, it probably means that phpunit failed to run tests. Try running arc unit wi
th --trace option and then run generated phpunit command yourself, you might get
the answer.' in C:\Websites\facebook\arcanist\src\unit\engine\PhpunitResultPars
er.php on line 156
Test Plan: Re-ran the tests that were causing issues with `arc unit --rev HEAD^ --trace`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D6246
Summary:
Previously, updating a commit via arc diff in mercurial would
prepopulate the update message with part of the commit message. In an
amend workflow this doesn't make sense, so I disabled it. Git already
does this, so now mercurial matches git in this scenario.
We had users complain that new users would often submit diffs with the
default update message, and it wasn't useful since they were using a
amend flow.
Test Plan:
arc diff on a commit that already had a diff
Verified the editor did not have a update message
arc diff on a stack of commits where the bottom one had a diff
Verified the editor provided a default update message
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6215
Summary: We currently swallow the exception message, but this isn't useful. Fixes T3354.
Test Plan:
$ arc diff HEAD^ --conduit-uri=http://local.aphront.com:8080/ --only
Uploading 1 files...
Failed to upload new binary 'large.png'.
[HTTP/500] Internal Server Error
As received by the server, this request had a nonzero content length but no POST data.
Normally, this indicates that it exceeds the 'post_max_size' setting in the PHP configuration on the server. Increase the 'post_max_size' setting or reduce the size of the request.
Request size according to 'Content-Length' was '2093052', 'post_max_size' is set to '100K'.
Continue? [Y/n]
Reviewers: jamesr, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3354
Differential Revision: https://secure.phabricator.com/D6186
Summary: Now that we examine these in lint, this came up. There is no `ConduitException`; the class is `ConduitClientException`.
Test Plan: Lint.
Reviewers: vrana, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6158
Summary: If one wishes to implement a linter which finds unused resources or variables the current scheme does not work as the lint engine filters all changes from lines which were not introduced in a diff. To solve that, I've added an "always show" configuration for a lint message allowing creation of such linters.
Test Plan:
1. Created a custom linter for finding unused Android resources in a project
2. Ran arc lint with linter added and received warnings as expected
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6119
Summary:
Piping data around on Windows doesn't work well when it contains zany characters like "null" and "newline". Fixes T3266.
Instead of piping data into `git apply`, write to a temporary file.
Test Plan:
Ran `arc patch`, got good results.
>>> [17] <exec> $ git apply --index --reject -- '/private/var/folders/8k/c3vkmjy5335gcxdzxkhwq82w0000gn/T/7z9iea6srikoo0sc/4266-ZEyvz9'
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T3266
Differential Revision: https://secure.phabricator.com/D6070
Summary: On Windows consoles, the star on the unit tests (I'm assuming it's a Unicode character of some kind), doesn't render correctly and you just get garbage. This fixes it so that on Windows, it falls back to just using an ASCII asterisk.
Test Plan: On Windows run some unit tests, the star should now be a plain asterisk.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6087
Summary:
@Afaque_Hussain has done a bunch of utf8 work here; combined with PhutilEditDistanceMatrix we can now do utf8 diffs correctly, in a general way, without a significant performance impact.
Use PhutilEditDistanceMatrix and `phutil_utf8v_combined()` to compute accurate diffs for all (or, at least, most) UTF8 text.
The only thing this doesn't handle completely correctly is lines beginning with combining characters. This is messy/expensive to handle and will probably never actually happen, so I'm punting for now. Nothing should actually break.
The utf8 stuff will be slow, but we only pay for it when we need it.
Test Plan:
Ran unit tests. I changed a few unit tests to use a non-combining character (snowman) for clarity, and some results are different now (since we get combining characters right).
{F44064}
Reviewers: btrahan, Afaque_Hussain
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2379
Differential Revision: https://secure.phabricator.com/D6019
Summary: Replace this old hard-coded implementation with the new vector-based, unicode-capable one.
Test Plan: Ran unit tests. Looked at revisions in Differential, using whitespace modes to bypass cache.
Reviewers: btrahan, Afaque_Hussain
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2379
Differential Revision: https://secure.phabricator.com/D6016
Summary: Also warn against functions not available on Windows at all.
Test Plan: Compared old and new file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5975
Summary: These were in my sandbox, but I forgot about them. Without this things break post D5896. Ref T2784
Test Plan: my sandbox works and soon so shall others
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5929
Summary: @alex has git 1.7.0.4 which doesn't have this flag. We don't actually need it: we always provide a commit message when calling this method. Remove the flag for compatibility, leaving a note in case we bump into this in the future.
Test Plan: N/A
Reviewers: btrahan, vrana, alex
Reviewed By: alex
CC: aran
Differential Revision: https://secure.phabricator.com/D5926
Summary:
Arc Revert does the following:
1. Git revert
2. Go to the differential of the rev you are reverting and either repoen it or set it to a reverted state
3. File a hipri task to orig author
[Preview] Creating Arc Revert workflow
Porting arc revert from FB4A to phabricator for general usage. This is my first stab,
so totally appreciate feedback and assistance. I'm currently focused
on making this work for git. However, I built out the functions through the GitAPI so this
could be easily extendable to Mercurial later on.
Stuck on the following (help):
1. Creating a task for FB internal. I tried building on top of existing arc listeners
but getting errors on failures to load the TaskCreator (and other) tasks.
2. I'm using a hacky way to grab the diff revision id from the newly created
revert diff. (see line 204) I'm looking for a way to just fetch the diff ID
from arc after the diff is created; is this possible?
Test Plan:
-
1. Ran arc revert on a www and fbcode diff
2. Confirmed that revert was run on the diffs and a proper diff filed
-
Reviewers: royw, sdwilsh, nh, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin, pti, keir
Maniphest Tasks: T1751
Differential Revision: https://secure.phabricator.com/D5553
Summary:
The NoseTestEngine class has some usefull code for parsing nosetest, but it assumes a very specific code/test
layout. I've pulled this logic abit apart, which lets me reuse the nose-related parts with my existing
project layout, by having my custom engine just call runTests() with the relevant paths.
Test Plan: Run on a customized project with coverage, see coverage results.
Reviewers: epriestley, roman.barzyczak
Reviewed By: epriestley
CC: seporaitis, aran, Korvin, zeeg
Differential Revision: https://secure.phabricator.com/D5921
Summary:
During my attept to `arc land master` on `master` branch
I have discovered that error message is missing few spaces between words.
I have added them and used this ugly readonly command:
pcregrep --include="\.php$" -M -r '(?<!\\n| )("|'"'"')\.\n\s*\1(?!\\n| )' ~/arc/arcanist/src
to detect other instances of this serious bug.
Two more were found.
This time they were probably introduced in order to abide
to the draconian lint rule about number of columns.
Since I want to be a good citizen,
I have added this missing space to the begining of the next line in both cases.
It is an ugly hack, but I think user should not suffer due to missing spaces.
Another solution could be preserving no leading spaces and splitting long lines.
Or just providing excuse to lint.
Test Plan: Ran `arc land master` on `master` branch.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3008
Differential Revision: https://secure.phabricator.com/D5827
Summary:
Fixes T2266. Motivation:
- The lint cache does not always invalidate correctly. Because of the nature of the cache, this is a hard problem (right after "naming things").
- We already have a fair amount of complexity in trying to invalidate it, and are still discovering new places where it doesn't work (e.g., Windows with "/" vs "\" paths).
- One invalidation failure is when linter code changes, which seems unresolvable in the general case (e.g., changes to external linters).
- It's not obvious what's happening when the lint cache causes some kind of issue.
- Particularly while developing or debugging linters, your changes often won't be reflected in the lint output. Some of this is theoretically tractable but the external linter case probably isn't.
- When someone reports a problem with the lint cache in IRC or elsewhere, there is essentially never a way for me to fix it. The lint cache can't be debugged effectively without access to a working copy where the problem reproduces.
- The cache provides limited benefit outside of Facebook's install.
To remedy these issues:
- Introduce configuration which controls cache usage.
- Default it off.
- Print a message when the cache is in use.
(I'd tentatively support removing the cache entirely, but I don't know how @vrana and Facebook feel about that.)
Test Plan: Ran `arc set-config --show`, `arc lint --cache 0`, `arc lint --cache 1`.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, mbishopim3, nh, edward, wez
Maniphest Tasks: T2266
Differential Revision: https://secure.phabricator.com/D5766
named the same as a file path.
Summary: (In fbcode and www at least, the only places I tried) if a
branch happens to be named the same as a file path (from root) (like if
a branch is called `spec` and there is a directory in `www` called
`spec`) then `arc land` will fail with git complaining that the argument
(to `git log ...`) is ambiguous as it could refer to both a path and a
revision; so this diff adds a `--` to the end so that git knows that
both are revisions and not paths.
Test Plan: Try and land something from www (I did, see D752219) where
the branch is named `spec`.
Reviewed by: epriestley
Summary: See D5714. Ref T2971.
Test Plan: Built a library map for libphutil's test library.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2971
Differential Revision: https://secure.phabricator.com/D5715
Test Plan:
$ set # on Windows
Reviewers: epriestley, dschleimer
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5733
Summary:
Arc lint for git is currently adding all untracked files when it
amends the commit with lint fixes. This changes the git add -A to be
git add -u. This only adds files that were already tracked. -A was adding
untracked files as well which was not the desired behavior here.
Test Plan:
Create an untracked file.
Commit a lint failure another file.
arc diff and choose to amend the lint patches.
Verify that the untracked file was not added but the tracked file was amended.
Reviewers: epriestley, wez, chad
Reviewed By: chad
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5731
Summary:
arc lint was hardcoded for git for amending commits with lint
patches. This enables the same functionality for mercurial.
Test Plan:
Made some changes that would result in a lint patch.
arc diff
Verify that the patches it produces were amended into the commit.
Verified it still works in git as well.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5716
Summary:
When doing an arc diff with pending changes in your working copy
it was creating a new commit with the pending changes instead of amending
the existing one. The problem was the author comparison was comparing
values like "John Smith <john@foo.com>" with "John Smith". The fix changes
$api->getAuthor() to return "John Smith" instead of the full string. This
matches the behavior (and implementation) found in the git api.
Test Plan:
hg book foo
touch a && hg commit -Ama
touch b && hg add b
arc diff
When prompted, amend the pending changes to the existing commit.
Verified that the changes were amended instead of making a new commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5706
Summary:
@ender is reporting a parsing issue in SVN, but we don't build a parser with setWriteDiffOnFailure() set in this workflow right now so I can't get the raw file to fix the issue.
Use the onboard mechanism to build a parser with `setWriteDiffOnFailure()` set, so it will write the diff, so I can get a copy so I can fix the problem.
Test Plan: Ran `arc diff --preview` in an SVN repo with a linter.
Reviewers: ender, btrahan
Reviewed By: ender
CC: aran
Differential Revision: https://secure.phabricator.com/D5699
Summary: Detect and fix unconventional spellings of `true`, `false`, `null` and `array` (these are the only keywords I've seen spelled unconventionally in the wild).
Test Plan: Unit tests.
Reviewers: DurhamGoode, btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2985
Differential Revision: https://secure.phabricator.com/D5686
Summary:
This adds a arcconfig setting to allow specifying whether to use the merge
or rebase strategy when doing the feature branch update.
arc.land.update.default can be set to either 'rebase' or 'merge'. The command
line flags will override this setting.
We have had trouble with arc land producing merge commits (introduced
with D4080) in git. They usually appear when arc land fails, and our users
are confused by the presence of a merge commit afterwards. Today it got even
worse since a user managed to get arc land to push the merge commit to the
server. This setting will allow us to turn it off for our uses.
Test Plan:
Verified the following combinations:
update.default not set + arc land (saw git merge in the trace)
update.default = 'rebase' + arc land (saw git rebase)
update.default = 'merge' + arc land (saw git merge)
update.default = 'rebase' + arc land --update-with-merge (saw git merge)
update.default = 'merge' + arc land --update-with-rebase (saw git rebase)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5683
Summary:
Changes arc diff to choose the base commit as the first ancestor
that has a diff. So if your tree looks like master->A->B->C->D, if you
have a diff on B (which will include A), when you run arc diff on D it will
only include C and D.
This makes the scenario for stacked diffs nicer. A user can commit A, commit B,
arc diff, commit C, commit D, arc diff, arc land B, arc land D.
Test Plan:
Commit A on top of master
Commit B on top of A
arc diff
Commit C on top of B
Commit D on top of C
arc diff
Verify the second diff contains the changes in C and D, but not A and B.
hg up B
arc land --preview
Verify that arc land shows A and B
hg up D
arc land --preview
Verify that arc land shows A, B, C, and D
(arc land should be unaffected by this change.
It always tries to land the entire branch)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5639
Summary:
If we're removing a binary file that didn't have svn:mime-type set properly,
we can't propset it (because the file doesn't exist locally). Instead, just
return a synthetic diff for the removed file.
Test Plan:
run arc diff in an svn working copy where I ran svn rm on a binary file that
doesn't have svn:mime-type set, and the diff correctly gets uploaded to
phabricator instead of erroring when trying to set properties.
Reviewers: epriestley, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5655
Summary:
Previously arc diff for hg only allowed bookmark names, rev numbers,
and commit hashes as the input base commit. This was because it escaped all
inputs and treated them as raw identifiers.
This change makes it treat the input as a revset if the escaped version fails.
This allows users to do things like "arc diff .^" when they only want to diff
the top commit.
Test Plan:
Created a stack of commits, master->A->B.
hg up B
arc diff .^
Verified the diff message only showed B as part of the diff.
arc diff .^~
Verified an error occurred ("Commit '.^~' is not a valid Mercurial commit id.")
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2888
Differential Revision: https://secure.phabricator.com/D5638
Summary:
I'm guessing this was refactored somewhere down the line and it meant
that Python and Perl files were no longer considered text files.
I'm imagining the old regex was: p(hp|y|l). Therefore I blame CSS.
Test Plan:
Perform an arc lint on a Python or Perl file that has trailing whitespace
on a line. It should prompt you for an autocorrecting lint.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5637
Summary:
The `emailuser` template is a relatively recent addition to Mercurial, and a few users have complained about it. It also doesn't actually do what I thought it did, e.g. in an address like this:
"Abraham Lincoln" <alincoln@whitehouse.gov>
^^^^^^^^^^^^^^^ ^^^^^^^^
(1) (2)
^^^^^^^^^^^^^^^^^^^^^^^
(3)
...I want (1), but `emailuser` means (2). Instead, extract (1) with `getDisplayName()` and (3) with `getAddress()` using PhutilEmailAddress.
The implementation in Mercurial is not particularly sophisticated or magical (it just looks for "@" and "<") so we aren't really missing anything by doing this ourselves, at least today.
Also fix some issues in `arc export`, which literally no one uses, but which is occasionally useful for testing (as here).
Test Plan:
- Ran `arc diff --only` in an `hg` repo, checked DB to see that name/email were correctly extracted.
- Ran `arc export --git` in an `hg` repo, didn't get a long series of fatals.
Reviewers: btrahan, DurhamGoode
Reviewed By: DurhamGoode
CC: aran
Maniphest Tasks: T2866, T2858
Differential Revision: https://secure.phabricator.com/D5539
Summary:
This adds a hook to allow external parties to provide config settings at runtime.
The hook is technically for when a RepositoryAPI is created, but that moment can
be used to set new config settings using the new setRuntimeConfig() api.
For example you could have a external hook that looks for keys like 'git:foo.bar'
or 'hg:foo.bar' and writes the value of 'foo.bar' based on whether the repo is a
git or a hg repo.
Test Plan:
Created a hook that looks for hg/git prefix versions of config keys.
Set hg:arc.feature.start.default to be "master" and set arc.feature.start.default
to be "trunk".
Ran arc feature in the hg repo. It made a bookmark on master.
Ran arc feature in the git repo. It made a branch on trunk.
Did it again, but with git:arc.feature... set instead.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5357
Summary:
Arc land is a bit magical and some users have gotten bitten by
the fact that it collapses and lands every commit on the branch. To make
it explicit what is being landed, it now shows a list of the commits
that are being landed. I also added a --preview flag that will just
print the commits that would be landed, but does nothing else.
Hopefully this make arc land a little less magical for people.
Test Plan:
arc land in the following scenarios:
- Landing one change
- Landing no changes
- Landing a stack of changes
Did it with hg and git.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, sid0, dschleimer, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5460
Summary:
Previously, arc patch would create a new commit under the existing
current bookmark in mercurial. There have been two discussions about what the
right behavior should be (D3334 and D3658). One side wants no commit at all,
and one side wants a commit under a new bookmark. The current implementation is
the worst of both worlds :(
This change makes it create a new bookmark at the revision's base before commiting,
same as the --bookmark flag used to do (which is now obsolete). That way the
existing bookmark doesn't move (in mercurial >=1.8). This is the same behavior
git has, which is convienent for groups migrating between the two.
Also makes hg's getCanonicalRevision handle svn revisions just like git. This way
arc patch will try to apply the patch to the appropriate revision in the history.
Test Plan:
Ran:
arc patch - Verified it created a new bookmark and commited on top of the
revision's base commit.
arc patch --nobranch - Verified it put the new commit on top of the current
bookmark without a new bookmark.
arc patch --nocommit - Verified it left all the changes in the working copy.
Also verified arc patch still works with git.
Reviewers: epriestley
Reviewed By: epriestley
CC: sid0, bos, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5408
Summary:
- Added arc.autostash option to have this behaviour off by
default (but configurable on a per-project basis).
- Automatic stashing of changes now informs the user of how
to restore their working directory if Arcanist unexpectedly
terminates.
- Fixed an issue with finalizeWorkingCopy when the workflow
didn't require a clean working copy.
Test Plan:
Test `arc diff` when there are changes in the working
directory; by default it should tell you to stash or commit.
Turn on the arc.autostash option and try again; it should
automatically stash with a message on how to recover, and
it should restore the working directory automatically under
almost any circumstances (other than an unrecoverable error).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5385
Summary:
Adds error handling for several kinds of failure in arc land for
mercurial. Previously it would often leave the repo in a confusing state
if something failed.
- Aborts the land if pull brings in a diverged 'onto' branch.
- Aborts the rebase if there is a conflict. This leaves the repo exactly as
it was before, so the user is not left with a half finished rebase.
- Don't delete the original non-squashed branch until the push succeeds.
- If the push fails, strip the temporary squashed commit. This leaves the
'onto' branch back on the latest commit from the server, and leaves the users
original nonsquashed branch around.
- Always leave the user back on their original branch after an error.
Test Plan:
Ran arc land:
- with pull causing a diverged 'onto' bookmark
- with the 'onto' bookmark already diverged
- with the rebase causing conflicts
- with a push that failed due to a commit hook
- with a successful land
- with a successful collapse and land
In all failure cases the repo was left exactly as it was before arc land,
except for the push-failed case, where the only change was that the branch
was correctly on top of the destination branch due to a successful rebase.
Used bookmark name "foo bar-gah" to test that crazy bookmark names still work.
Reviewers: epriestley, dschleimer, sid0
Reviewed By: epriestley
CC: nh, wez, bos, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5394
Summary:
Arc patch was committing with -A (--addremove) which meant any random
files that were sitting around in the repo (like conflict .orig files) were
added to the commit. The -A isn't even necessary since the hg import
adds and removes all the appropriate files for you.
Test Plan:
touch foo
arc patch --diff some-diff-id
Verified that foo was not added to the commit
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: dschleimer, bos, sid0, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5396
Summary:
On systems with an ancient version of python, the pep8 linter won't run.
Instead of blowing up in the user's face, we should display a nice error
message.
Test Plan:
Put /usr/bin (where the ancient version of python is) at the beginning of
my path and tried to lint some python. I got a nice error instead of a
stack trace.
Reviewers: epriestley, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5356
Summary: Improves Windows compatibility.
Test Plan: Ran failing unit test on Windows.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5344
Summary: Added some sample rcsdiffs for adding and deleting a line from a file. Wrote some test cases to be tested by ArcanistDiffParser.
Test Plan: By making all the test cases pass.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5324
Summary:
Changes arc feature to read 'arc.feature.start.default' instead
of 'arc.land.onto.default'. In our usage we actually need to fork off
a different branch than we land to, so separating these is useful.
Test Plan:
Set arc.land.onto.default = master
Set arc.feature.start.default = bar
arc feature foo
cat .git/config
Verified the foo branch tracked bar
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: wez, dschleimer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D5336
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: 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:
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
