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:
See discussion in D4049.
The getWorkingCopyStatus() method gets called from requireCleanWorkingCopy() in a lot of places, which triggers resolution of the base of the commit range. This is unnecessary; we do not need to examine the base commit in order to determine whether the working copy is dirty or not. This causes problems, in D4049 and elsewhere (we currently have a lot of fluff calls to setDefaultBaseCommit() in workflows which need to call requireCleanWorkingCopy() but do not ever use commit ranges, such as `arc patch`). This is mostly an artifact of SVN, where the "commit range" and "uncommitted stuff in the working copy" are always the same.
- Split the method into two status methods: getUncommittedStatus() (uncommitted stuff in the working copy, required by requireCleanWorkingCopy()) and getCommitRangeStatus() (committed stuff in the commit range).
- Lift caching out of the implementations into the base class.
- Dirty the cache after we commit.
This doesn't do anything useful on its own and creates one caching problem (`commitRangeStatusCache` is not invalidated when the commit range changes because of `setBaseCommit()` or similar) but I wanted to break things apart here. I won't land it until there's a more complete picture.
This creates a minor performance regression in git and hg (we run less stuff in parallel than previously) but all the commands should be disk-bound anyway and the regression should be minor. It prevents a larger regression in `hg` in D4049, and lets us do less work to arrive at common error states (dirty working copy). We can examine perf at the end of this change sequence.
Test Plan: Ran unit tests, various `arc` commands.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4095
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:
This was causing the following error in environments that didnt have scala configured:
Some linters failed:
- ArcanistScalaSBTLinter: ArcanistUsageException: This directory does not appear to be maintained by SBT, as we can't seem to find a working build file (project/Build.scala or build.sbt).
Test Plan: .
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4070
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:
SBT is the most common Scala buildsystem. This adds an extremely basic and
slightly horrible linter to check SBT's output for warnings and errors.
Test Plan:
Tested this with a Scala project I've been working on for some time.
It seemed relatively sane.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4064
Summary: I plan to use it in `save_lint.php`.
Test Plan:
$api->getUnderlyingWorkingCopyRevision(); // In Git SVN repo
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4054
Summary: `svn info` is the slowest command for discovering repository (up to 300 ms) and Subversion is probably the least used repository type with Arcanist. Let's discover it last.
Test Plan:
$ arc lint --trace
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4039
Summary:
We delegate parsing to `PhutilArgumentParser` but then skip the parts of the code which do something with this flag.
In particular, `arc --help list` just runs `arc list`.
See https://github.com/facebook/arcanist/issues/58
Test Plan: Ran `arc --help`, `arc list --help`, `arc --help list`, `arc --help help --help`. Got expected help in all cases.
Reviewers: vrana, chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4023
Summary:
- Rename some very old variables.
- Wrap some contributed lines.
Test Plan: `arc lint` / `arc unit`. Viewed a diff in an uncacheable mode to verify intraline behavior.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4018
Summary:
Let's try this.
It may be useful to see this together in Differential overview.
Test Plan: Linted a file with TODO.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T744
Differential Revision: https://secure.phabricator.com/D4014
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: See D3963. Instead, parse these diffs so they'll work with `--raw`, etc.
Test Plan:
Generated a failing diff, added it as a test case. Fixed issue. Ran test suite. Ran `arc` against it:
$ git -c diff.suppress-blank-empty=true diff HEAD | arc diff --raw --only --conduit-uri=http://local.aphront.com:8080/
Reading diff from stdin...
Created a new Differential diff:
Diff URI: http://local.aphront.com:8080/differential/diff/103/
Included changes:
M things
Reviewers: vrana, jiiix, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3969
Summary:
accommodate git's diff.suppress-blank-empty=true setting
Without this change, if you were to set diff.suppress-blank-empty=true
in your .gitconfig (as I do), then "arc diff" would always fail with the
cryptic diagnostic, "Diff Parse Exception: Found the wrong number of
hunk lines."
Test Plan:
Put this in ~/.gitconfig or .git/config
[diff]
suppress-blank-empty = true
and run "arc lint". It should pass. Without this chnage,
it would fail as described above.
Reviewers: vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3963
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:
The variable $old_phid was not being set in a certain situation in
buildBinaryChange(), and that was causing the following error, during
`arc patch <revision>`:
"the patch applies to <file> (<hash>), which does not match the
current contents."
and hence it was failing to download/apply the patch.
Signed-off-by: Sergio Correia <sergio@correia.cc>
Test Plan:
I spotted the problem in a revision where I was renaming
some images, which are binary.
Reviewers: epriestley, vrana, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3925
Summary:
Run `hg status` as a Future while getting the diff. Add a cache for
getRawDiffText().
Test Plan:
Run `arc lint --trace` on a HG repo and confirm that `diff` is only called once
and `status` is run in the background.
Reviewers: epriestley
Reviewed By: epriestley
CC: yliang, dpepper, aran, Korvin
Maniphest Tasks: T2016
Differential Revision: https://secure.phabricator.com/D3913
Summary:
Installations extend this.
Another solution would be to extend `ArcanistLinterTestCase` from `ArcanistArcanistLinterTestCase` and return null in `getLink()` to avoid code duplication but I prefer clean class hierarchy.
Test Plan:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3878
