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
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:
When we hit a diff which is missing file context, we try to pull it synthetically later. This works for moves and copies, but currently fails for property changes. Since it failed, we didn't have context, so we'd try to pull it again...
The general problem this creates is that when you mark a file "+x" without changing it, we can't show you the content in Differential. Not a huge deal. In some future diff, I'll build the content synthetically.
Adds commits to cover this behavior:
commit 1830a13adf764b55743f7edc6066451898d8ffa4
Author: epriestley <git@epriestley.com>
Date: Tue Nov 6 17:11:18 2012 -0800
Mark koan2 +x and edit it.
commit 8ecc728bcc9b482a9a91527ea471b04fc1a025cf
Author: epriestley <git@epriestley.com>
Date: Tue Nov 6 17:08:44 2012 -0800
Move 'text' to 'executable' and mark it +x.
commit 39c8e7dd3914edff087a6214f0cd996ad08e5b3d
Author: epriestley <git@epriestley.com>
Date: Tue Nov 6 16:36:59 2012 -0800
Mark koan as +x.
Test Plan: Ran unit tests. Previously, they looped indefinitely. Now, they pass.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3909
Test Plan:
$ arc lint a.py # with too long line
Reviewers: zeeg, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3882
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:
Makes lots of noise:
{F22758}
Test Plan:
Linted file with several bad characters per line.
Linted file with one bad character per line.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3896
Summary: We could also inject the value from the test case config but this is simpler.
Test Plan:
$ arc unit src/lint/linter/ArcanistLicenseLinter.php
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3895
Summary: None of these are that serious that I would like to be informed about them on unmodified lines.
Test Plan: Linted Python file with lots of PEP8 errors, now warnings.
Reviewers: zeeg, epriestley
Reviewed By: zeeg
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3884
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: This is sort of documentation.
Test Plan:
Used the plugin for Notepad++ with default indent of 4 spaces.
Opened file from `src/`, verified that it indents 2 spaces.
Opened file from `externals/`, verified that it indents 4 spaces.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3865
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:
If you type `arc`, you currently get a fatal since $config isn't defined. Don't try to run the hook if we never built a config.
(We could build the config earlier and then run the hook anyway, but $workflow and $command would not be defined. It seems unlikely that executing the hook here is useful, since it affects only the case that the user types `arc` on its own.)
Test Plan: Typed `arc`. Typed `arc diff`, etc.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3841
Summary:
D3748 attempted to improve the behavior of `arc diff` when dealing with files merged from another branch, but had the side effect of marking all normal edits and deletes as adds. Revert this side effect, at least. This likely degrades the merging case, but it's comparatively rare, and editing/deleting files is very common.
I'll make an effort to fix this properly (and back it with DirectoryFixture tests) when I deal with T1947.
Test Plan: Ran `arc diff --preview` for a change that edits or removes files.
Reviewers: btrahan, vrana, svemir
Reviewed By: svemir
CC: aran
Differential Revision: https://secure.phabricator.com/D3836
