Summary: This is fairly confusing. Make the error message suggest the common remedy (update libphutil).
Test Plan: Eyeballed it.
Reviewers: Afaque_Hussain, btrahan, vrana
Reviewed By: Afaque_Hussain
CC: aran
Differential Revision: https://secure.phabricator.com/D4834
Summary: If provided, have `arc patch` use `authorName` / `authorEmail`. This simplifies handling and makes patches more portable between version control systems (previously, information was generated in the diff's VCS, regardless of which VCS it was being applied to).
Test Plan: Created a diff with author `derp <derp@derp.com>`, ran `arc patch --diff x`, got a local commit with that author.
Reviewers: btrahan, edward, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4827
Summary:
Record author email information in `arc diff`, so we can recreate it in `arc patch` and elsewhere without creating any kind of email exposure issues.
In Mercurial, we currently store the whole string ("username <email@domain.com>"). Make this consistent with Git.
Test Plan: Created git and hg diffs, saw authorEmail populated.
Reviewers: btrahan, edward, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4825
Summary:
We want to use it for `yield` and `invariant_violation()` which throws.
Having node instead of token would be better but this would be enough.
Test Plan: Implemented a hook in FB repo and added a test case there.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4821
Summary:
We assume `git` is available now, but should not. Specifically, if a user runs a working copy operation like `arc list` in an SVN working copy without `git` available, they get this error: P707
We interpret git errors very narrowly; be more liberal in how we interpret them. This assumes users working with `git` will have a functional `git`, but this seems like a reasonable assumption and lets us remove some error text matching code.
Test Plan: Changed `git` to `girt`, ran `arc list`, saw a reasonable exception. Changed back to `git`, saw git detected.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: svemir, aran
Differential Revision: https://secure.phabricator.com/D4804
Test Plan: Copied the code in a script, changed `phutil_passthru()` to `echo csprintf()` and ran it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4805
Summary: I somehow missed it.
Test Plan: Sent this diff by different copy of Arcanist.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4800
Summary:
This disallows code like this:
$cmd = 'ls';
execx($cmd);
But I guess it's not that big deal?
Test Plan: Linted whole Arcanist and Phabricator codebases, most parts looks fixable.
Reviewers: epriestley
CC: nh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4794
Summary:
FB currently starts Sandcastle push before starting the diff workflow.
If `arc diff` commits something then we need to restart the push.
I want to avoid this by starting the push after commit.
Test Plan: Will test after implementing the listener.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4785
Test Plan: Didn't see a fatal in new test case.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4783
Summary: This is pretty lame but I didn't have a better idea.
Test Plan:
$ arc test # previously translated as list
$ arc lst
$ arc brnach
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4773
Summary:
Continuation of D4732 - when we don't care about loading an arcconfig,
allow that to be specified.
Test Plan: chmod -r .arcconfig; bin/arc help --skip-arcconfig
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4750
Summary: This can be useful by itself, we want to use it in FB linter.
Test Plan: This diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4744
Summary: Currently, we don't expose these at top level, so you can't disable coverage if your coverage is explosively broken. Expose them as passthrough arguments.
Test Plan:
- Touched a file in `arc` which triggered unit tests.
- Without `xdebug` installed:
- Ran `arc diff --preview`, `arc diff --preview --no-coverage` (both fine).
- Ran `arc diff --preview --coverage`, got exception about coverage not being available.
- Installed `xdebug`.
- Ran `arc diff --preview`, got coverage.
- Ran `arc diff --preview --coverage`, got coverage.
- Ran `arc diff --preview --no-coverage`, no coverage.
Reviewers: indiefan, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4745
Summary:
This test currently chdir()'s into a directory which is later removed. If another test tries to run a shell script while the CWD is invalid, the shell may emit this to stderr:
shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory
Among other things, this can cause the XHPAST test to fail, because it detects syntax errors by examining stderr.
Instead, retore the directory.
Test Plan: Ran "arc unit --everything", which could previously fail if XHPAST ran after Bundle.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4738
Summary: Also provides an example how to build custom linter using XHPAST.
Test Plan: Added debug output to `willLintPaths()`, verified that each path is parsed only once.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4718
Summary: `git pull` may fail in git-svn after rebasing (which is a side effect of dcommit).
Test Plan:
$ git svn rebase
$ git log trunk..master
$ git pull --ff-only; echo $?
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4716
Summary: I guess this is correct? See T2387 for discussion.
Test Plan: Unit tests.
Reviewers: bos, DurhamGoode
Reviewed By: DurhamGoode
CC: aran
Maniphest Tasks: T2387
Differential Revision: https://secure.phabricator.com/D4711
Summary: Some people have 2GB+ untracked files in repo which significantly slows down this or even crashes it.
Test Plan: Added a debug output here and linted repo with untracked path.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, arudolph
Differential Revision: https://secure.phabricator.com/D4713
Summary: Fixes T2438. We currently escape everything with '@', but SVN rejects that for '.'
Test Plan:
Unit tests. Performed this commit:
$ svn st
M .
A x@123
$ arc commit --conduit-uri=http://local.aphront.com:8080/ --revision 53
Revision 'D53: asdf' has not been accepted. Commit this revision anyway?
[y/N] y
Committing 'D53: asdf'...
Sending .
Adding x@123
Transmitting file data .
Committed revision 37.
Done.
I grepped for more '@' adding but couldn't find any. It's a bit tricky to grep for though, so it's possible I missed some.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T2438
Differential Revision: https://secure.phabricator.com/D4703
Test Plan: Ran PhpunitTestEngine unit test and used both test result parsers to generate test results.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D4676
Summary: PHPUnit 3.7 now includes user message as well.
Test Plan: Ran phpunit tests
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4662
Test Plan: Ran PhpunitTestEngine unit test. Also used refactored PhpunitTestEngine to run phpunit tests.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D4651
Summary:
arc land on a hg-svn repository would fail because arc land
uses 'hg push -r' to specify which revs to push which is not supported
by hg-svn. Now we just use 'hg push' which, when used against svn, only
pushes the current branch (which happens to be the branch we're trying to land).
We can't use standard 'hg push' for non-svn repos though because when used
against a vanilla hg repo 'hg push' pushes all branches.
Also remove --new-branch from 'hg push' because it's extremely
unlikely that a person wants to create a new branch on the server via
arc land.
Test Plan:
Ran arc land on a normal hg repo, verified it used 'hg push -r'.
Ran arc land on a hg-svn repo, verified it used 'hg push' and it pushed the
correct changes.
Reviewers: epriestley, sid0, dschleimer
Reviewed By: epriestley
CC: bos, aran, Korvin
Maniphest Tasks: T2403
Differential Revision: https://secure.phabricator.com/D4653
Summary:
The cache key is repository base revision and hash of all modified files.
It isn't perfect in SVN where every file might have a different revision.
It's also suboptimal as just committing or amending changes the cache key even if the files contents weren't modified.
We can improve it later, perhaps by using previous revision and files modified since it.
Test Plan:
Changed granularity of XHPAST linter to repository.
Linted the same repo twice, verified that it was read from cache the second file.
Changed a single file, verified that all files were re-linted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4608
Summary: The main reason for this is to not exit with 1 when no paths are lintable (which is more success than failure).
Test Plan:
Returned empty array from `buildLinters()`, then:
$ arc lint
$ echo $? # 0
Reviewers: epriestley
Reviewed By: epriestley
CC: wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4625
Summary: Currently, we get an exception on empty %Ls for `arc diff --no-ansi` or similar (see P698).
Test Plan: Ran `arc diff --no-ansi`.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4624
Test Plan: Deleted file in Git, ran `arc diff`, confirmed the question, saw the file as deleted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4603
Summary: Fixes T2112. These are fairly common now, and are used as the storage format for `hg export` and mq in most installs.
Test Plan: Ran unit tests, used `arc patch --patch`, uploaded some diffs manually.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2112
Differential Revision: https://secure.phabricator.com/D4592
Summary: The binary may not be built, in which case this raises a warning.
Test Plan: Will make @zeeg test.
Reviewers: zeeg, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4569
Summary:
Previously, trying to arc land in a mercurial repo would
fail if the local branch was already at the tip of the onto branch
(since hg rebase exited with code 1). This change makes it check
if a rebase is needed before executing the rebase.
Test Plan:
hg init foo
cd foo
hg bookmark master
touch a && hg add a && hg commit -ma
// setup your .arcconfig
cd ..
hg clone foo foo2
cd foo2
hg bookmark mybook
touch b && hg add b && hg commit -mb
arc land --onto master --revision <your rev number>
Arc land should succeed. I also tried landing when a rebase was
necessary and it still worked.
Reviewers: epriestley, dschleimer, bos
Reviewed By: epriestley
CC: sid0, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4588
Summary: Also include binary hash in the version.
Test Plan: New unit test.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4535
Summary:
A git submodule looks a lot like a normal git repo, but the .git
directory is replaced with a file that git reads to find the real
location of the git directory. When arcanist tries to write a file into
a directory inside of there, it was failing silently, and then crashing
silently when it couldn't read results back out. Instead of assuming the
git directory is a directory named .git at the toplevel of the tree, we
use the appropriate git command to get the correct git directory.
Test Plan: submit a diff from a submodule
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4482
Summary: This should be done for all external and configurable linters.
Test Plan: Linted file with lint problems, changed options, relinted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4475
