Summary: Adds "arc unit --everything", which runs every available test, provided the test engine supports it. Also add JSON output.
Test Plan: Ran `arc unit --everything` in arcanist/, libphutil/ and phabricator/. Saw all tests run.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2065
Differential Revision: https://secure.phabricator.com/D4214
Summary:
See D4049, D4096.
- Move commit range storage from Mercurial and Git APIs to the base API.
- Move caching up to the base level.
- Store symbolic name and resolved name separately, so we can re-resolve the correct commit from the symbolic name after dirtying caches.
- Rename `supportsRelativeLocalCommit()` to `supportsCommitRanges()` (old name wasn't very good, and not consistent with new terminology like the `--base` flag).
- Rename `getRelativeCommit()` and `setRelativeCommit()` to `getBaseCommit()` and `setBaseCommit()`.
- Introduce `reloadCommitRange()` and call it from `reloadWorkingCopy()`.
I think this fixes the problem in D4049, and provides a general solution for the class of problems we're running into here, with D4096. Specifically:
- We no longer get dirty caches, as long as you call reloadWorkingCopy() after changing the working copy (or call a method which calls it for you).
- We no longer get order-of-parsing-things problems, because setBaseCommit() reloads the appropriate caches.
- We no longer get nasty effects from calling `requireCleanWorkingCopy()` too early.
Test Plan: This is pretty far-reaching and hard to test. Unit tests; ran various arc commands. :/
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4097
Summary:
This method is used in three cases:
# For unit tests, to set the range to 'HEAD^' or '.^' in an agnostic way.
# For "amend", to set the range to the commit to be amended (also 'HEAD^' or '.^').
# For "patch" and "upgrade" so we don't fail just because there's an invalid "base" rule somewhere in the config when doing clean-working-copy tests.
For cases (1) and (2), introduce an "arc:this" rule to mean "the current commit". For case (3), remove the call; it is no longer necessary to check the commit range in order to do tests for the working copy state after D4095.
Test Plan: Ran unit tests, "arc upgrade", "arc patch", "arc amend".
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4096
Summary:
This allows using new methods without the need for bumping version number.
The usage is not neccessary because we already bumped the version number for this but I wanted to have a callsite.
Test Plan:
Made a typo in method name, then:
$ arc lint --only-new 1
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4193
Summary:
This changes arc land's hg support to allow you to land a branch or bookmark that has nested branches/bookmarks. Example:
// Initial state:
// -a--------b master
// \
// w--x mybranch
// \--y subbranch1
// \--z subbranch2
//
// arc land --branch mybranch --onto master :
// -a--b--wx master
// \--y subbranch1
// \--z subbranch2
Test Plan:
Created several repos like in the summary and ran 'arc land' and 'arc land --keep-branch'. Did this with both bookmarks and named branches. Scenarios tested:
- mybranch having no child commits
- mybranch having a child branch with several commits
- mybranch having two child branches
- mybranch having a child branch which has two more child branches
No code was added outside of a "if ($this->isHg)" so I didn't run git arc land.
Reviewers: epriestley, dschleimer, bos, sid0
Reviewed By: dschleimer
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4089
Summary:
See chatlog from https://secure.phabricator.com/chatlog/channel/%23phabricator/?at=37257
Basically, the install's developers sometimes use "git merge master" instead of "git rebase master" to pull changes from master into their branch. When we run "git rebase master" at the end, this creates a lot of conflicts.
Instead, optionally run "git merge master". This should be equivalent in our case (where we always rebase) and much better in their case (where they sometimes merge).
We're both going to use it for a bit and see if it creates problems. If it improves the "sometimes-merge" workflow without affecting the "always rebase" workflow, we can make it a default. If it improves theirs but damages ours, we can keep it a flag/option. If it's just terrible, we can figure out something else.
Test Plan:
Ran `arc land --mergeup --trace <feature> --keep-branch --hold`, see P624 for output. Observed merge update strategy and general success.
Also verified the conflict:
$ arc land --mergeup --merge
Usage Exception: Arguments '--mergeup' and '--merge' are mutually exclusive: The --merge strategy does not update the feature branch.
Reviewers: btrahan, vrana, DurhamGoode
Reviewed By: btrahan
CC: aran, keir
Differential Revision: https://secure.phabricator.com/D4080
Test Plan: Added a debug output there and ran.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4147
Summary:
arc browse will open a browser to the Diffusion view of a file. Convenient if you like
Diffusion for reading source. Naturally, it fixes relative filenames.
Combined with git-grep it can be an easy replacement for server-side search functions.
Test Plan:
use feature with and without 'browser' configured.
I've only tested this on Linux, because that's all I have right now, but the principle is sound.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4127
Test Plan: Will test it by closing this revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4122
Summary:
Fixes the exception:
Exception
Command 'git commit -a --author=Whatever Long Name <whatever@email.com> -F -' failed with error #1
Test Plan: Tested with full git name
Reviewers: epriestley, aurelijus
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3979
Summary:
Allow authors to publish a new or updated revision to Phabricator
without requesting code reviews. The revision will have status
"Needs Revision" instead of "Needs Review".
In order to avoid a change of Conduit API, this is done by adding
a comment with the "plan changes" action immediately after the
revision is published.
Test Plan:
Using my local repository, run "./bin/arc diff --plan-changes"
Check the resulting diff in Phabricator.
Reviewers: vrana
Reviewed By: vrana
CC: aran, epriestley
Maniphest Tasks: T2024
Differential Revision: https://secure.phabricator.com/D4084
Summary:
If you run `arc diff` in a repository which:
- has uncommitted or untracked changes; and
- has a .arcconfig with a never-before-seen project ID;
- we fatal: http://pastebin.com/raw.php?i=ykpfr4MT
This patch is a bit iffy, open to alternatives. The "right" patch is probably an `arcanistproject.query` which behaves more sensibly.
I return array() directly since we'll later create the project.
Test Plan: Ran `arc diff` in a repository with untracked files or uncommitted changes and a new project ID.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4057
Summary: Should have been part of D3934.
Test Plan:
$ arc diff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4076
Summary:
Makes arc land support hg repositories. Both bookmarks and named branches can be landed. For the most part all the arc land options work, but there are a few caveats:
- bookmarks can only be landed on bookmarks
- branches can only be landed on branches
- landing a named branch with --merge creates a commit to close the branch before the merge.
- since mercurial doesn't start with a default master bookmark, landing a bookmark requires specifying --onto or setting arc.land.onto.default
Test Plan:
Tested arc land with all permutations of --merge, --keep-branch on both bookmark branches and named branches. Also tested --hold, --revision, --onto, --remote.
See https://secure.phabricator.com/P619
Also tested git arc land with --merge and --keep-branch.
Reviewers: dschleimer, sid0, epriestley, bos
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4068
Summary:
See discussion in D4056. In `findRevision()` we call `loadWorkingCopyDifferentialRevisions()`. However, this method depends upon the state of the working copy, because the end of the commit range it examines is HEAD. Prior to D4056 we checked out the target branch before calling `findRevision()`; after D4056 we call it earlier.
This isn't problematic in the `arc land` case, but in the `arc land <branch>` case it means we may fail to identify a revision, or identify the wrong revision, because HEAD isn't where we expect it to be.
Instead, unconditionally check out the target branch before finding the revision.
See <http://dl.dropbox.com/u/116385/Slingshot/Pictures/Screen%20Shot%202012-12-03%20at%203.43.45%20PM.png> for a transcript of the issue.
Test Plan: Reproduced issue as per link above. Ran `arc land --keep-branch --hold somebranch` successfully after this patch.
Reviewers: DurhamGoode, zeeg
Reviewed By: DurhamGoode
CC: aran
Differential Revision: https://secure.phabricator.com/D4072
Summary:
We can add `GRANULARITY_DIRECTORY` and `GRANULARITY_REPOSITORY` later.
Repository granularity may use current commit + changes.
Directory would need to use hashes of all files in dir which would be quite expensive.
Test Plan:
$ echo '<?php class A extends B {}' > A.php
$ arc lint --cache 1
$ arc lint --cache 1
$ echo '<?php class B {}' > B.php
$ arc lint --cache 1
$ arc lint --cache 1
$ rm B.php
$ arc lint --cache 1
$ arc lint --cache 1
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4021
Summary:
Refactor the arc land code into several
functions so it's easier to maintain. This is in
preparation for adding hg support to arc land
in my next commit.
Without this refactor, adding hg support makes the run()
function too big.
Test Plan:
Set up a git repo and clone with a branch scenario.
Example: https://secure.phabricator.com/P614
Ran and verified:
arc land
arc land --keep-branch
arc land --merge
arc land --merge --keep-branch
arc land --hold
arc land --revision <another phabricator rev>
Reviewers: epriestley
Reviewed By: epriestley
CC: dschleimer, sid0, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4056
Summary:
There is one big decision: How to get linters version.
I've created `getCacheVersion()` which is supposed to be bumped every time engine or any linter is changed.
This is not very nice but the other alternative (detect this automatically) seems worse:
- The engine may be outside repo and may or may not be under version control so getting its version through something like `git log` may not be even possible.
- If it is in the same repo then every rebase will obsolete the whole cache.
Even though bumping the version manually is PITA I still think it's a better solution.
Test Plan:
$ time arc lint --cache 1
# verified file
$ arc arc lint --cache 1
# added some debug output to see the cached results
# also observed better time (.57 s instead of 2.19 s)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2036
Differential Revision: https://secure.phabricator.com/D4006
Summary: Also delete extra newlines.
Test Plan:
$ arc diff # on top of my commit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2025
Differential Revision: https://secure.phabricator.com/D3996
Summary: pragmatism wins the day, though I think eventually we might want something really fancy to deal with arcanist and conduit not being up to date with respect to one another
Test Plan: php -l
Reviewers: epriestley
Reviewed By: epriestley
CC: zeeg, aran, Korvin
Maniphest Tasks: T2088
Differential Revision: https://secure.phabricator.com/D3988
Summary:
Some users want be stopped even if there are lint advices.
NOTE: Deleted by D3364.
Test Plan:
On diff with lint advice:
$ arc diff --preview # advice just printed
$ arc diff --preview --advice # excuse required
Reviewers: epriestley
Reviewed By: epriestley
CC: akramer, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3982
Summary:
There's quite some logic in here:
- It automatically decides whether to create a new commit or amend.
- It partially respects 'default-relative-commit'.
- However if it points to a closed revision then it creates a new commit.
Resolves T2025.
Test Plan:
`arc diff` on:
- Clean committed repository.
- Dirty repository without commit since 'default-relative-commit'.
- Dirty repository with non-revision commit since 'default-relative-commit'.
- Dirty repository with revision commit since 'default-relative-commit'.
- `arc diff HEAD^` on dirty repository on top of closed revision.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2025
Differential Revision: https://secure.phabricator.com/D3967
Summary: assumes D3917 (or something like it that populates 'author' value from conduit call) exists in production
Test Plan: stubbed out 'author' value and verified checkins as author worked
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T479
Differential Revision: https://secure.phabricator.com/D3918
Summary:
It's inside `.git/` for some time.
It also ignored changes in `test.arc/`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3900
Summary: Maybe I will need it on other places.
Test Plan:
$ arc lint --output json # on file with lint error
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3898
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in unit tests and LICENSE file.
Reviewers: epriestley, btrahan, edward
Reviewed By: epriestley
CC: aran, Korvin, davidrecordon
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3881
Summary: fix for T2011, first option in list of possible fixes
Test Plan: ...do I really have to setup mercurial with mq? :D
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2011
Differential Revision: https://secure.phabricator.com/D3869
Summary: I broke this in D3750. Calling `getRepositoryAPI()` triggers a check for workflow requirement of the repository API. Instead, we should just check if we alreayd have one.
Test Plan: Ran `cat x | arc diff --raw`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1998
Differential Revision: https://secure.phabricator.com/D3848
Summary: turns out retina has files like image.png and image@2x.png. The latter breaks since '@' is a special command telling svn to "look for image at revision 2x.png" -- nonsensical garbage. If we add it to the end every time this error goes way.
Test Plan: touch 2@2.png; svn add '2@2@'; arc diff; <fill out form, accept commit>; arc commit -- observe commit working
Reviewers: epriestley
Reviewed By: epriestley
CC: mbishopim3, aran, Korvin
Maniphest Tasks: T1999
Differential Revision: https://secure.phabricator.com/D3845
Summary:
Some users assume they can update anything, not just revisions they own (see https://github.com/facebook/arcanist/issues/54).
Currently, if you `arc patch` or `arc amend` and get a commit message, then `arc diff` for a revision you don't own, we:
- Fail early with a very confusing message ("You can not review a revision you own!") if you are on the "Reviewers" line, until D3820.
- Or fail very very late with a good error message, but after lint, unit and update messages.
Instead, check that you own the revision as early as we can.
Test Plan: Tried to update revisions I didn't own, got good error messages early on (with D3820).
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3821
Test Plan: Made lint error, slept in test, verified that tests are finished when I confirm `arc diff --excuse`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3803
Summary: This diff obsoletes D3385.
Test Plan: Made lint error, explained it, verified that unit already finished before I finished explaining (by adding `sleep(3)` to test).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3786
Summary: Make this harder to get wrong. Instead of requiring a separate call for synthetic data, automatically load it if we can.
Test Plan: Unit tests; `arc diff`.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T866
Differential Revision: https://secure.phabricator.com/D3750
Summary:
- I caused $parser to be reused in D3732 which I belived was safe, but actually isn't. We end up writing to the same changes. We should make it safe but there's some mess in Phabricator that needs to be cleaned up first.
- One minor error code thing, variable is undefined.
Test Plan: Ran `arc export --git` on a moved file, got a better result. Ran some command which made me hit the other case and didn't get a fatal anymore.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3749
Summary:
When some linter throws then we don't print any result.
This is bad in case when the linter threw e.g. because of syntax error in some file which some other linter will tell us about.
Test Plan: Threw from a linter, made lint error in a file, saw error then exception.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3756
Summary:
This is horrible and git specific, but fixes a case where people are using "arc
patch --nobranch ..." when they're not currently on a branch.
The old code assumed you were on a branch and used getBranchName() to record
this, in order to return to that branch later and cherry-pick the patch.
When not on a branch, and using arc patch --nobranch, this was trying to return
to the branch '(no branch)'.
Now, I detect that we're not on a branch and just record what HEAD is instead.
Test Plan:
Checkout the SHA of master (so I'm on master, but not on a branch) then try to
patch it with a feature diff:
€ git checkout e7a3ec68159d6847372cab5ad913f2f15aa7c249
Warning: you are leaving 1 commit behind, not connected to
any of your branches:
ac1ad39 Updating a-file
If you want to keep them by creating a new branch, this may be a good time
to do so with:
git branch new_branch_name ac1ad392350a51edd10343f12b9713f5e5b3707c
HEAD is now at e7a3ec6... Fix arcconfig
€ arc patch --nobranch D7
Created and checked out branch arcpatch-D7.
OKAY Successfully committed patch.
€ git branch
* (no branch)
feature
haddock
master
€ git log --oneline | head -2
38c0235 Updating a-file
e7a3ec6 Fix arcconfig
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3743
Summary:
We need to run new Arcanist over old code in Perflab.
We also need to run new Arcanist over new code (with already deleted custom `buildAllWorkflows()`).
This will work because old code overwrites `buildAllWorkflows()`.
Test Plan:
$ arc help
$ arc help # after deleting getWorkflowName() from one workflow
Reviewers: edward, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3741
