Summary: This was accidentally disabled with some Mercurial changes that allowed dirty working copies.
Test Plan: Ran `arc diff` with staged changes.
Reviewers: nh
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D3165
Test Plan:
`arc lint` with OK result and with patchable error.
`arc unit` with passes and errors.
`arc diff` with patchable lint error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3151
Summary:
Allow querying and modifying flags from arcanist. Currently
supports only printing and deleting flags for Differential revisions,
but it should be straightforward to add more capabilities (given Conduit
support).
Test Plan:
Run arc flag, passing it various revisions. Flags are
modified appropriately.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1556
Differential Revision: https://secure.phabricator.com/D3133
Summary: On windows there is no 'which', only 'where'
Test Plan: Run JSHint linter on windows and unix-like system.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3096
Summary: This diff format is used by de-facto mercurial GUI called "TortoiseHG".
It is available as the only way to copy diff into clipboard (right click commit,
select "export", select "copy patch". I added this format support into arcanist
so revisions in Differential can be created from TortoiseHG via simple copy-
paste. Unit test added, manually tested.
See: https://github.com/facebook/arcanist/pull/46
Reviewed by: epriestley
Summary:
We've had these in our library names and they're quite useful as word
separators. Allowing them shouldn't cause any trouble.
Test Plan:
ran arc liberate in an empty directory, and it didn't complain when I gave
it a name with a hyphen.
Reviewers: epriestley, vrana, jungejason
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3070
Summary:
See <https://github.com/facebook/arcanist/issues/45>
Currently, when the user types `arc set-config x false`, we set it as the string "false", which is usually not desirable. We have some steps toward typed config already, but expand on what we have and move as much stuff as possible into it, including all the config settings that aren't currently documented (there are still some lint-specific and project-specific settings not present here, but this is most of it).
Also make the `phutil_libraries` key a legacy name for `load`, and `immutable_history` a legacy name for `history.immutable`. Generally the goal here is to make config simpler and bring it more in-line with Git/Mercurial, which use dotted hierarchies.
I'll add some documentation here but I think most of the changes should be fairly straightforward.
Test Plan:
- `arc set-config history.immutable on` (And similar -- sets to boolean true.)
- `arc set-config history.immutable off` (And similar -- sets to boolean false.)
- `arc set-config history.immutable derp` (And similar -- raises exception.)
- `arc set-config history.immutable ''` (And similar -- removes setting value.)
- `arc set-config --show`
- `arc get-config`
- `arc get-config base`
Reviewers: dschleimer, bos, btrahan, vrana
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1546
Differential Revision: https://secure.phabricator.com/D3045
Summary: When you run `hg diff -r x:y`, we get two "-r" arguments in the diff header. Currently, we parse this incorrectly.
Test Plan: Added unit test which previously failed; test now passes.
Reviewers: dschleimer, btrahan
Reviewed By: dschleimer
CC: aran, cakoose
Maniphest Tasks: T1550
Differential Revision: https://secure.phabricator.com/D3061
Summary:
Phabricator, as we all know, is marketed as a fun adventure game. However, while it is occasionally fun and often an adventure, it's so far been sorely deficient in the game aspect. This patch aims to rectify that oversight. (Presence of the first two qualities is not guaranteed.)
Note: In case there's any doubt, this is not a serious suggestion. I was bored.
Test Plan: Seriously?
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3026
Summary:
We have a lint rule checking if some string is too long.
This string can span multiple lines.
If we report a warning at the first line then it is muted if the first line wasn't modified.
We need to say that this whole block is wrong and report it when at least one line from the block was modified.
Test Plan: Changed a lint rule to call `raiseLintAtLines()` and verified that the warning is reported even if the changed line isn't first.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3029
Summary: Currently, if you have a branch named "docs" and a local file named "docs", `git show -s docs` complains because it's ambiguous. Use `--` to unambiguously mark branches as revisions, not files.
Test Plan: Ran `arc branch` in a working copy with a "docs" branch and a "docs" file, got expected results.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3030
Summary: 'arc todo' now logs a message with the task title and URI when run.
Test Plan: Run 'arc todo test' and see that it logs a message with the form 'Created task <task number>: '<task title' at <task URI>
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3016
Summary:
This reduces time of `arc branch` from 13s to 7s in my repo which is faster than `git branch --verbose` (10s).
The price for this speedup is that we loose the information [ahead 1, behind 21242] but we showed it only in branches with no revision so it's not a big deal.
Test Plan:
$ arc branch
Reviewers: epriestley
Reviewed By: epriestley
CC: ahupp, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3005
Summary:
On Windows, a diff may have "\n" newlines (from the file itself) but "\r\n" blocks (from svn).
NOTE: indents are funky since I edited this with Notepad++, I'll fix before landing.
Test Plan: Diffed an edit to a "\n" newline file on Windows in SVN.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2998
Summary: I'm trying to get a repro for a Windows + SVN patch issue. Dump patches which fail to a temp file so there's less bewilderment in getting the right patch handed over for analysis.
Test Plan: Forced a parse failure, ran "arc diff", inspected temp file.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2997
Summary:
- In "arc which", we recommend "--rev x --rev ." to show changes. This is not accurate if there are uncommitted changes in the working copy. Just "--rev x" shows the correct changes (implicitly, the other end of the range is the working copy state).
- When you diff only working copy changes, we currently incorrectly identify all your open revisions as belonging to the working copy. Instead, correctly identify none of them as belonging to the working copy (in theory, we could go farther than this and do path-based identification like SVN, but with --amend in hg 2.2+ this workflow should be going away in the long run).
- If you have uncommitted working copy changes, never try to amend.
Test Plan: Ran "arc which .", "arc diff ." in a working copy with dirty changes, got better results than before.
Reviewers: dschleimer, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1507
Differential Revision: https://secure.phabricator.com/D2980
Summary: From "cmd.exe" with, e.g. SilkSVN, there are some issues getting arc to do anything useful. Resolve enough of them so that it's at least usable.
Test Plan: Created a revision from Windows / cmd.exe / arc / SVN.
Reviewers: btrahan, jungejason, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2984
Summary:
This uses a similiar approach as with postponed unittests, allowing
the lint workflow/engine to report postponed linter names. After
the lint engine is run, a separate method is used to collect any
postponed linters and these are reposted to the diff via the
"arc:lint-postponed" property.
Also, a ##diff.wasCreated## was added allowing hooks to be called
immediately after the call ##differential.creatediff## with the
returned diff ID.
Test Plan:
Created diffs with a dummy lint engine which always reports a
postponed linter.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1332
Differential Revision: https://secure.phabricator.com/D2933
Summary: See discussion in T1467. This `log` logs everything in the repo. The old command was `show -s`, I just unthinkingly converted it and it doesn't matter for non-Facebook-sized repositories.
Test Plan: Ran "arc branch".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1467
Differential Revision: https://secure.phabricator.com/D2963
Summary: See D2955. Allow "arc set-config editor ..." to override all other editor settings.
Test Plan: Ran "arc set-config editor 'mate -w'", am typing this in textmate.
Reviewers: btrahan, ezfoxie
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1309
Differential Revision: https://secure.phabricator.com/D2956
Summary:
Run `arc todo o rly?` to create a Maniphest task
titled 'o rly?'. Pass --cc to add CCs besides yourself.
Test Plan: Ran `arc todo o rly?`. Behaved as expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T749, T1325
Differential Revision: https://secure.phabricator.com/D2952
Summary:
when a commit message contains error which fails the parsing, we 'arc branch' fails. One sample commit message we saw contains
Differential Revision: 363812, 367983, 370452
The author manually committed the code without a real revision.
Test Plan: ran on the repo with problem commit and it succeeded.
Reviewers: epriestley
Reviewed By: epriestley
CC: malmond, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2944
Summary: Otherwise svn diff fails when the file is binary but the "svn:mime-type = application/x-shellscript" is missing in it.
Test Plan: arc diff succeeded against deleting a binary file which doesn't have svn:mime-type = application/x-shellscript.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2915
Summary:
- Implement "arc:amended", a base commit DSL rule which always selects HEAD (git) or `.` (hg) if it has "differential revision:" in the commit message. This is unambiguously correct in amend workflows, and can cover holes in other rules like "git:branch-unique(*)".
- Fix a bunch of Mercurial stuff:
- Our use of '.' is wrong, and based on a misunderstanding on my part of the behavior of `hg diff --rev . --rev .`, which means "ignore the second --rev flag", not ". means working directory state". As far as I know there's no explicit way to say "the working copy plus all its changes".
- The `--prune` argument to "hg log" does not support symbolic names like ".^". Use revsets instead.
- Reduce the number of times we need to run `hg branch`.
- We can safely use "." to mean "the working copy revision", and do not need to do "hg --debug id" or similar.
- Generally simplify some of the nonsense in the implementation left over from me having no idea how Mercurial works.
Test Plan:
Ran "arc which" in various scenarios in a mercurial working copy. I //think// I exercised all the changes.
Ran "arc which --base arc:amended" in hg and git working copies without "Differential Revision:" in head/. (no match) and with it (matched head/.).
Reviewers: dschleimer
Reviewed By: dschleimer
CC: Makinde, tido, phleet, aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2876
Summary:
This increases the amount of information arc diff collects in
Mercurial repositories. IN particular, it collects the active
bookmark, if there is one, and svn information in hgsubversion
repositories.
The Phabricator half of this is https://secure.phabricator.com/D2897
Test Plan:
[14:06:59 Sat Jun 30 2012] dschleimer@dev4022.snc6 ~/www-hg
www-hg 2941 $ ~/devtools/arcanist/bin/arc diff --only --conduit-uri http://phabricator.dschleimer.dev4022.facebook.com
HipHop Notice: Undefined index: 1 in /data/users/dschleimer/devtools/arcanist/src/repository/api/ArcanistMercurialAPI.php on line 708
Created a new Differential diff:
Diff URI: http://phabricator.dschleimer.dev4022.facebook.com/differential/diff/126/
Included changes:
A foo
HipHop Notice: Undefined index: 1 in /data/users/dschleimer/www-hg/lib/arcanist/arcanist/FacebookArcanistConfiguration.php on line 81
mcproxy on this server is out of date
version: , expected version:
please restart (http://fburl.com/1787362)
[14:07:46 Sat Jun 30 2012] dschleimer@dev4022.snc6 ~/www-hg
www-hg 2941 $ echo '{"diff_id": 126}' | ~/devtools/arcanist/bin/arc call-conduit differential.getdiff --conduit-uri http://phabricator.dschleimer.dev4022.facebook.com | json_pretty | egrep -i 'bookmark|sourcecontrol'
"bookmark": "bar",
"sourceControlBaseRevision": "svn+ssh://tubbs/svnroot/tfb/trunk/www@583442",
"sourceControlPath": "svn+ssh://tubbs/svnroot/tfb/trunk/www",
"sourceControlSystem": "hg",
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1331
Differential Revision: https://secure.phabricator.com/D2896
Summary: We currently error if a user types "--revision D123".
Test Plan: `arc export --git --revision D123`, got a diff.
Reviewers: vrana, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2880
Summary:
Add a DSL command to select the first commit between HEAD and (the merge-base of some target with HEAD) that's on more than one branch.
This is similar to @csilvers' suggestion in <https://secure.phabricator.com/T1233#comment-8>. A specific problem this address is that, currently, if you use this workflow:
$ git checkout -b feature
$ git commit
$ git checkout -b subfeature
$ git commit
..i.e., develop dependent features in sub-branches, "arc diff" will try to send up (feature + subfeature). If you use the rule 'git:branch-unique(origin/master)' instead, diffing from 'subfeature' will correctly select only the commit(s) on 'subfeature'.
Test Plan: Constructed feature/subfeature branches, verified that we select the correct base commit.
Reviewers: dschleimer, csilvers, btrahan, jungejason
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2866
Summary:
This adds a new base option that Does the Right Thing (or as close to
it as I can get) for someone using Mercurial bookmarks as lightweight
branches, and where each branch should be one differential revision.
Specifically, it walks backwards through the ancestors of the working
copy until it finds a revision that is either not outgoing (ie already
in the remote) or that is bookmarked. This means that bookmarks
effectively act as delimiters, and point at the last revision that
will be included in an arc diff.
Test Plan:
[14:40:54 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1226 $ hg log -r '(first(outgoing())^)::' --template='node: {node}\nbookmark: {bookmarks}\n\n' -G
@ node: c8379ef32b0d0e6cf94fe636751ea4fe1353e157
| bookmark:
|
| o node: 14f03139049cbda339190b814e52f4ec8b05c431
| | bookmark: more
| |
| o node: 6970e9263ab8c6da428420606d1f15c9980da183
| | bookmark: something
| |
| o node: 433a93023f03d5f3eddaa243fa973d32a1566aee
|/ bookmark:
|
o node: f47ccfe34267592dd2e336174a3a311b8783c024
| bookmark:
|
[14:41:00 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1226 $ ~/devtools/arcanist/bin/arc which --show-base --base 'arc:bookmark'
f47ccfe34267592dd2e336174a3a311b8783c024
[14:41:05 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1226 $ hg up 14f03139049cbda339190b814e52f4ec8b05c431
3 files updated, 0 files merged, 1 files removed, 0 files unresolved
[14:41:10 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1226 $ ~/devtools/arcanist/bin/arc which --show-base --base 'arc:bookmark'
6970e9263ab8c6da428420606d1f15c9980da183
[14:41:14 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1226 $ hg up 6970e9263ab8c6da428420606d1f15c9980da183
0 files updated, 0 files merged, 1 files removed, 0 files unresolved
[14:41:44 Tue Jun 26 2012] dschleimer@dev4022.snc6 ~/hg-dummy
hg-dummy 1227 $ ~/devtools/arcanist/bin/arc which --show-base --base 'arc:bookmark'
f47ccfe34267592dd2e336174a3a311b8783c024
Reviewers: epriestley, bos
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1331
Differential Revision: https://secure.phabricator.com/D2863
Summary:
I use `//~` for marking places which shouldn't be committed.
It also looks ugly.
Maybe it can be just a warning but we provide a patch so error is OK?
Test Plan: New test.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2824
Summary:
Delete all code related to v1 libraries in arcanist.
When users liberate a v1 library, prompt them to upgrade.
Test Plan: Reverted phabricator/ to a couple of months ago and liberated it. Got prompted to upgrade. Upgraded.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2861
Summary:
- Add flags to exit after an idle time or client count.
- Add flags to control daemonization.
- Add flags to control output.
- Add flags to skip the "hello" frame of the protocol.
- Make the client launch a server if one does not exist.
The one-time overhead to launch a server and run a command through it looks to be ~130% of the overhead to run the command directly with "hg", so even if we never run a second command we're not paying too much.
The incremental overhead to run subsequent command appears to be less than 3% of the overhead to run the command directly with "hg" (and maybe less than 1%, I'm not sure how long the computation part of a command like 'hg log' "actually" takes).
The overhead to launch a PHP client, connect to an existing server, run a command, and then print it and exit is roughly 50% of the overhead to run the command directly with "hg". So theoretically a user can achieve an amortized 2x performance increase for all 'hg' commands by aliasing 'hg' to the PHP client in their shell.
Test Plan:
- Ran servers with idle and client count limits, let them idle and/or hit their connection limits, saw them exit.
- Ran foreground and background servers.
- Ran a daemon server with redirected stdout/stderr. Verified logs appeared.
- Ran with --quiet.
- Ran clients and servers with and without --skip-hello, things work if they agree and break if they disagree. The throughput gain on this is fairly small (maybe 5%?) but it seems simple enough to keep for the moment.
- Ran serverless clients and verified that servers launched the first time, were available subsequently, and relaunched after 15 seconds idle.
Reviewers: csilvers, vrana, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2680
Summary:
- Move "arc branch"-specific code to the branch workflow.
- Instead of doing "git rev-parse", just do "git branch --verbose --abbrev=40".
- Use revision owners to identify ownership, not working copy identity. Particularly with the advent of "Commandeer", you might not own commits you made.
- Do a batch lookup for commits by hash (depends on D2859, but doesn't break without it).
- Use PhutilConsole for console stuff.
- Removed color from "arc list" for the moment.
- The "--by-status" flag has a slightly different output format now.
Test Plan: Ran "arc branch" in various circumstances, verified it identifies branches in immutable history repositories.
Reviewers: btrahan, vrana, jungejason, nh, slawekbiel
Reviewed By: slawekbiel
CC: aran
Maniphest Tasks: T693
Differential Revision: https://secure.phabricator.com/D2860
Summary:
svn 1.7 got rid of the per-direcotry .svn dirs in favor of a single
.svn directory under the root of the working copy. Unfortunately, we
relied on ther being a .svn directory at the same level as .arcconfig,
which may or may not be at the root of the working copy. We now walk
up the directory tree until we find a .svn directory that we can use
for scratch files.
Test Plan:
[16:27:52 Mon Jun 25 2012] dschleimer@dev4022.snc6 ~/data/admin/facebook/scripts/db
db 21298 $ ls -la .arcconfig .svn ../../../.svn/arc/config
ls: .svn: No such file or directory
ls: ../../../.svn/arc/config: No such file or directory
-rw-r--r-- 1 dschleimer users 239 Jun 25 16:15 .arcconfig
[16:27:54 Mon Jun 25 2012] dschleimer@dev4022.snc6 ~/data/admin/facebook/scripts/db
db 21298 $ ~/devtools/arcanist/bin/arc set-config --local foo bar
Set key 'foo' = 'bar' in local config.
[16:29:40 Mon Jun 25 2012] dschleimer@dev4022.snc6 ~/data/admin/facebook/scripts/db
db 21298 $ ls -la .arcconfig .svn ../../../.svn/arc/config
ls: .svn: No such file or directory
-rw-r--r-- 1 dschleimer users 239 Jun 25 16:15 .arcconfig
-rw-r--r-- 1 dschleimer users 20 Jun 25 16:29 ../../../.svn/arc/config
[16:29:43 Mon Jun 25 2012] dschleimer@dev4022.snc6 ~/data/admin/facebook/scripts/db
db 21298 $ cat ../../../.svn/arc/config
{
"foo" : "bar"
}
[16:29:48 Mon Jun 25 2012] dschleimer@dev4022.snc6 ~/data/admin/facebook/scripts/db
db 21299 $ ~/devtools/arcanist/bin/arc get-config foo
(global) foo =
(local) foo = bar
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1411
Differential Revision: https://secure.phabricator.com/D2856
Summary: This displays all inline comments attached to a revision in a format consumable by editors.
Test Plan: Ran it, opened the file on the line.
Reviewers: epriestley
Reviewed By: epriestley
CC: vii, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2845
Summary: Allow users to run a command to determine the base revision of the commit range.
Test Plan: Ran stuff like `arc which --show-base --base 'arc:exec(ls)'` and got the expected results.
Reviewers: dschleimer, csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2830
Summary: We will extract them at some point, lint before it's too late.
Test Plan:
New test.
Linted all callsites.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2825
Summary: Allow the default to be set in global config, not just project config.
Test Plan: `arc set-config arc.land.onto.default derp; arc land`
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2686
Test Plan: Show Full Unit Results on this diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2777
Summary:
New optional mode. If you set 'base' in local, project or global config or pass '--base' to 'arc diff' or 'arc which', it switches to DSL mode.
In DSL mode, lists of rules from args, local, project and global config are resolved, in that order. Rules can manipulate the rule machine or resolve into actual commits. Provides support for some 'arc' rules (mostly machine manipulation) and 'git' rules (symbolic ref and merge-base).
Test Plan:
Ran unit tests. Also:
```$ arc which --show-base --base 'arc:prompt'
Against which commit? HEAD
HEAD
$ arc which --show-base --base 'git:HEAD'
HEAD
$ arc which --show-base --base 'git:fake'
Usage Exception: None of the rules in your 'base' configuration matched a valid commit. Adjust rules or specify which commit you want to use explicitly.
$ arc which --show-base --base 'git:origin/master'
origin/master
$ arc which --show-base --base 'git:upstream'
Usage Exception: None of the rules in your 'base' configuration matched a valid commit. Adjust rules or specify which commit you want to use explicitly.
$ arc which --show-base --base 'literal:derp'
derp
$ arc which --show-base --base 'arc:halt'
Usage Exception: None of the rules in your 'base' configuration matched a valid commit. Adjust rules or specify which commit you want to use explicitly.
$ arc set-config --local base git:origin/master
Set key 'base' = 'git:origin/master' in local config.
$ arc which --show-base
origin/master
$ arc which --show-base --base 'git:HEAD^'
HEAD^
$ arc which --show-base --base 'arc:yield, git:HEAD^'
origin/master
$ arc which --show-base --base 'arc:global, git:HEAD^'
HEAD^
$ arc which --show-base --base 'arc:global, git:merge-base(origin/master)'
3f4f8992fba8d1f142974da36a82bae900e247c0```
Reviewers: dschleimer, vrana
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2748
Summary: Otherwise we'll get a cached result from fileperms() if we end up here again.
Test Plan: `chmod 644 ~/.arcrc ; arc help` no longer double prompts when run from outside of a .arcconfig working copy.
Reviewers: csilvers
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1359
Differential Revision: https://secure.phabricator.com/D2752
Summary:
We currently use the language "relative commit" or "relative local commit" to refer to the head of a commit range. I want to move toward callign this a "base commit", since I think that makes more sense. This moves us a small step in that direction.
The DSL stuff in T1233 also needs access to this stuff, so move it up to the base. This is mostly a bite-sized piece of the change in that diff.
Test Plan: Ran "arc which" in a couple of circumstances, read explanations.
Reviewers: dschleimer, vrana
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2747
Summary:
This adds the concept of a local (i.e. working copy local) config file
to arc. This config file has the highest precedence for config
settings that may come from any config file. The config is stored in
.(git|hg|sv)/arc/config, and is read ahead of the project config by
getConfigFromAnySource().
Test Plan:
#Testing arc set-config and arc get-config
[16:57:04 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19410 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
[16:57:12 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19410 $ ./bin/arc set-config foo bar
Set key 'foo' = 'bar' in global config.
[16:57:23 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19411 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
(global) foo = bar
[16:57:26 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19411 $ ./bin/arc set-config --local foo baz
Set key 'foo' = 'baz' in local config.
[16:57:35 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19412 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
(global) foo = bar
(local) foo = baz
[16:57:39 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19412 $ ./bin/arc set-config foo ''
Deleted key 'foo' from global config (was 'bar').
[16:57:49 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19413 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
(global) foo =
(local) foo = baz
[16:57:51 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19413 $ ./bin/arc set-config --local foo ''
Deleted key 'foo' from local config (was 'baz').
[16:58:05 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19414 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
#testing getConfigFromAnySource by means of lint
[11:26:57 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19612 $ ./bin/arc set-config lint.engine BogusLintEngine
Set key 'lint.engine' = 'BogusLintEngine' in global config.
[11:30:04 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19613 $ ./bin/arc set-config --local lint.engine DummyLintEngine
Set key 'lint.engine' = 'DummyLintEngine' in local config.
[11:30:19 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19587 $ ./bin/arc lint
Exception
Failed to load symbol 'DummyLintEngine'. If this symbol was recently added or moved, your library map may be out of date. You can rebuild the map by running 'arc liberate'. For more information, see: http://www.phabricator.com/docs/phabricator/article/libphutil_Libraries_User_Guide.html
(Run with --trace for a full exception trace.)
[11:30:25 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19586 $ ./bin/arc set-config --local lint.engine ''
Deleted key 'lint.engine' from local config (was 'DummyLintEngine').
[11:30:34 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19587 $ ./bin/arc lint
OKAY No lint warnings.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2739
Summary:
We currently remove an invalid user from Reviewers and similar fields and print a note in the template. There are several problems:
# If I only made a typo in the name then it takes some effort to fix this typo and put the name on the original place (this information is lost).
# The notice is almost invisible and most users will just confirm the message without noticing it.
# If I fill the data in the commit message (and not in the template) then I probably want to treat that as authoritative so I want it to be correct.
Test Plan:
`arc diff` with invalid reviewer in commit message.
`arc diff` on empty commit message.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, nh
Differential Revision: https://secure.phabricator.com/D2730
Summary:
Using .arc as the scratch and per-repository configuration directory
has some unfortunate consequenses in the real world. Among other
things, people forget to .gitignore it so it gets checked in.
Test Plan:
the only thing that seems to use this is the relative commit setting
for git. This diff consists of 2 commits, one for the .gitignore and
one for everything else.
Comment out the portion of my .git/config that defines the upstream
for the branch. Run arc diff --only with HEAD^ in
.arc/default-relative-commit. See that .gitignore is not included in
the resultant diff, that .arc no longer exists, and that .git/arc
exists and has HEAD^ in .git/arc/default-relative-commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2725
Summary:
##arc amend## in a mercurial repo was failing with this message
Usage Exception: Commit 'HEAD^' is not a valid Mercurial commit identifier.
mercurial's .^ is about the same thing as git's HEAD^
Test Plan:
##arc amend## in a mercurial repo with ##"immutable_history" : "false"## in the
##.arcconfig##.
Reviewers: epriestley
Reviewed By: epriestley
CC: vrana, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2690
Summary:
I just hope that we will not create `ArcanistPhutilTestCaseTestCaseTestCase`.
Also fix a copy/paste error (@edward's this time).
Test Plan: `arc unit`
Reviewers: epriestley
Reviewed By: epriestley
CC: edward, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2692
Summary: Almost revert D2673 but leave the support for 'repeat'.
Test Plan: `arc help unit`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2689
Summary:
This need a bunch of work, but is a sort of minimum viable product for the hgdaemon.
The two ProtocolChannels implement the client and server halves of the protocol.
The Server implements the server logic, the Client implements the client logic.
Test Plan: Launched a server and client on the same repo, got hg output in ~2-3ms instead of 100ms+.
Reviewers: csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2665
Summary:
We run all tests before deploy but some of them are expected to fail.
We need to skip them.
It can be useful also for someone else.
I don't propagate this to `arc diff` as that would be much more complicated - we would need a new state 'partially skipped'.
The 'path' argument needs to be a file, skipping whole dirs is not supported.
The 'path' argument is in fact engine specific and engines can support format like 'Class::testMethod' but I didn't bother to document it to not confuse people.
Test Plan:
`arc unit --skip test.php`
`arc unit --skip ../src/test.php`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2673
Summary:
This adds basic support for mutable history for arc diff and arc amend for
mercurial. This is purely opt-in (so it shouldn't affect anyone who doesn't want
this feature) by explicitly setting
"immutable_history" : false
in arc configuration.
This also fixes another instance of weird behaviour for multiple heads - the
first instance was fixed here:
https://github.com/facebook/arcanist/pull/28
Test Plan:
without "immutable_history" turned on)>
When ##arc diff## produces an update diff, it should list only commits that are
ancestors of the current revision - not ones from other heads.
Reviewers: epriestley
CC: csilvers, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2654
Summary:
We added "repeat" to PhutilArgumentParser a while ago, so we can modernize the rest of the argument parsing (allow multiple --load-phutil-library flags).
Add a "--conduit-timeout" flag to allow users to set the timeout. See D2602.
Test Plan:
$ arc diff --conduit-timeout=0.01
Exception
[cURL/28] <CURLE_OPERATION_TIMEOUTED> The request took too long to complete.
(Run with --trace for a full exception trace.)
$ arc diff --conduit-version 33
Exception
ERR-BAD-VERSION: Your 'arc' client version is '33', which is newer than the server version, '5'. Upgrade your Phabricator install.
(Run with --trace for a full exception trace.)
$ arc diff --conduit-uri http://derp.derp/
Usage Exception: YOU NEED TO INSTALL A CERTIFICATE TO LOGIN TO PHABRICATOR
You are trying to connect to 'http://derp.derp/api/' but do not have a certificate installed for this host. Run:
$ arc install-certificate
...to install one.
$ arc diff --load-phutil-library src --load-phutil-library ../phabricator/src --trace
Loading phutil library '0' from '/INSECURE/devtools/arcanist/src'...
Loading phutil library '1' from '/INSECURE/devtools/phabricator/src'...
Reviewers: phleet, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2603
Summary:
It makes me nervous.
Also move them to the dir.
Test Plan:
`arc lint`
`arc lint --output json`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2644
Summary: We don't support `git rebase -i` or other magic in `arc amend` so we can examine just HEAD which we always amend.
Test Plan: `arc amend` on a branch with two commits.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2643
Summary:
If a message has only "line", or "line" and "char", we don't point out the location in the context block.
When a message includes "line", show chevrons on the line.
When a message includes "line" and "char", show chevrons on the line and a caret on the next line.
Test Plan: Ran regex linters to capture line, line+char, and normal linters to capture everything. Output appeared sane.
Reviewers: csilvers, vrana, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2642
Summary:
`$this->getPaths()` gives us paths relative to repository root.
We resolve them relative to cwd.
Test Plan: [src] `arc unit difference`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2634
Summary:
The theory is that lint and unit errors are rare with no false positives.
Our reality is currently different.
This diff removes the question for providing explanation before actually providing it.
I hope that the wording is not confusing.
It also simplifies the code with less copy/paste errors (not me!) potential.
Test Plan:
`arc diff` on code with lint and unit errors.
Repeat with `--excuse .`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, meitros
Differential Revision: https://secure.phabricator.com/D2631
Summary: I wonder if this is not by purpose?
Test Plan: Modified two files, ran `arc lint`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2629
Summary:
Khan Academy is looking into lint configuration, but doesn't use ".arcconfig" because they have a large number of repositories. Making configuration more flexible generally gives us more options for onboarding installs.
- Currently, only project config (".arcconfig") can load libraries. Allow user config ("~/.arcrc") to load libraries as well.
- Currently, only project config can set lint/unit engines. Allow user config to set default lint/unit engines.
- Add some type checking to "arc set-config".
- Add "arc set-config --show".
Test Plan:
- **load**
- Ran `arc set-config load xxx`, got error about format.
- Ran `arc set-config load ["apple"]`, got warning on running 'arc' commands (no such library) but was able to run 'arc set-config' again to clear it.
- Ran `arc set-config load ["/path/to/a/lib/src/"]`, worked.
- Ran `arc list --trace`, verified my library loaded in addition to `.arcconfig` libraries.
- Ran `arc list --load-phutil-library=xxx --trace`, verified only that library loaded.
- Ran `arc list --trace --load-phutil-library=apple --trace`, got hard error about bad library.
- Set `.arcconfig` to point at a bad library, verified hard error.
- **lint.engine** / **unit.engine**
- Removed lint engine from `.arcconfig`, ran "arc lint", got a run with specified engine.
- Removed unit engine from `.arcconfig`, ran "arc unit", got a run with specified engine.
- **--show**
- Ran `arc set-config --show`.
- **misc**
- Ran `arc get-config`.
Reviewers: csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2618
Summary:
Move away from setModule(), to setPathPrefix(). Also simplify test location/selection.
Depends on D2621.
Test Plan: Ran "arc unit".
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2622
Summary:
- The library linter works fine on v1 or v2 libraries, so switch to it for all libraries.
- "arc liberate" on v1 libraries will still invoke the Module linter, and thus generate __init__.php files.
Test Plan: Ran "arc lint". Grepped for module linter.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2620
Summary: Mechanical changes from D2588. No "Class.php" moves yet because they aren't necessary for libraries to function.
Test Plan: See D2588.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2589
Summary:
- "arc liberate" now works for v1 or v2 libraries.
- "arc liberate --upgrade" converts a v1 to a v2.
- We delete __init__.php files but do not move Class.php files, since this is vastly simpler. Authors can do this on their own if they want. We'll do it separately.
- v2 has less lint support than v1, but I think we can move first and migrate lint support later. Much of the v1 lint is bad anyway.
Test Plan: Upgraded libphutil, arcanist and phabricator to v2
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2588
Summary: We recover from Conduit exceptions, but not from transport exceptions or other general classes of error here. Recover from everything, and add an explicit flag to skip uploads.
Test Plan: Added a "throw", created a diff, skiped upload. Removed throw, created a diff with --skip-binaries. Created a diff normally.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2598
Summary:
Adds a linter for v2 libraries which raises the relevant errors.
NOTE: Not hooked up anywhere yet, so this diff has no effect.
Test Plan:
Switched the ModuleLinter to LibraryLinter and ran it with a junk block to trigger errors:
>>> Lint for src/lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php:
Error (PHL3) One Class Per File
File 'lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php' mixes
function (id) and class/interface (ArcanistPhutilLibraryLinter)
definitions in the same file. A file which declares a class or an
interface MUST declare nothing else.
190 }
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Error (PHL2) Duplicate Symbol
Definition of function 'id' in
'lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php' in library
'arcanist' duplicates prior definition in 'utils/utils.php' in library
'phutil'.
190 }
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Error (PHL1) Unknown Symbol
Use of unknown class 'XYZ'. This symbol is not defined in any loaded
libphutil library.
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2597
Summary:
We have a linter that needs to read data from the repo that isn't in the commit,
so that data isn't part of the svn transaction, thus not available in svnlook
cat --transaction. This change provides a method to get data from a committed
file.
Test Plan:
Successfully ran arc svn-hook-pre-commit with a linter that needs something
not in the transaction.
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2582
Summary:
One line is usually more than enough for these excuses.
The excuse is quite often the same as in past so history is very useful.
Prompting user right below the errors is better than writing them below prompt.
Test Plan:
Produce a lint error.
Provide an empty explanation.
Provide a non-empty explanation.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2580
Summary: I am trying to stop using `--verbatim` and miss this information.
Test Plan: This diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2575
Summary:
`arc diff` adds a space after each field in template.
It is later removed for the last field.
Test Plan: This diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2564
Summary:
Simple wrapper for PHP_CodeSniffer.
You need to have PHP_CodeSniffer and it's dependencies installed.
Test Plan: - Try running it with your custom lint engine
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2560
Summary: On windows paths are separated with \.
Test Plan: - Run filename linter on windows (or some path with \)
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2559
Summary: Git supports this feature (https://git.wiki.kernel.org/index.php/Aliases), it's fairly simple to implement, and gives us a little more ammunition to dissuade users with unusual workflow requirements from actually requesting features.
Test Plan: Defined the alias described in the documentation. Ran "arc ls", "arc ls -alh", etc.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2552
Summary:
there are two problems making the ArcanistSvnHookPreCommitWorkflow not working.
- pathExists($path) will always return false because it hasn't loaded the path yet. Because of this, PhutilLintEngine will unset the path at https://secure.phabricator.com/diffusion/ARC/browse/master/src/lint/engine/phutil/PhutilLintEngine.php;032b9b30b0721c3f$46, so ArcanistSvnHookPreCommitWorkflow will have no path to lint against and never detects a problem.
- In ArcanistSubversionHookAPI::getCurrentFileData(), the $path is already the full path in svnroot (path got from https://secure.phabricator.com/diffusion/ARC/browse/master/src/workflow/svn-hook-pre-commit/ArcanistSvnHookPreCommitWorkflow.php;032b9b30b0721c3f$72). Prepending the project root to it will make the file path to be wrong so that the file content is empty. Again, ArcanistSvnHookPreCommitWorkflow never detects a problem.
Test Plan:
changed --transaction to --revision in related code and manually ran the following command. It detected the syntaxt error correctly. 558937 is a revision with syntax error.
/usr/local/bin/php -f /home/jungejason/nfs_devtools/arcanist/bin/arc svn-hook-pre-commit --load-phutil-library=/tmp/testwww/www/lib/arcanist --load-phutil-library=/home/jungejason/nfs_devtools/facebook/src /svnroot 558937 2>&1
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, Girish, Koolvin
Differential Revision: https://secure.phabricator.com/D2485
Summary:
It's easy to forget to do this when writing a new lint engine (like I recently
just did), so I added it to the example to improve documentation on how to write
a lint engine. This code is copied from PhutilLintEngine (as well as many
others).
Test Plan: php -l
Reviewers: jungejason, vrana, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2553
Summary: If you develop in "master" and try to "arc land", you get an error. You can reasonably "arc amend" in this situation, most of the time. Give the user an explicit pointer.
Test Plan:
$ arc land master
Usage Exception: You can not land a branch onto itself -- you are trying to
land 'master' onto 'master'. For more information on how to push changes, see
'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the
documentation. You may be able to 'arc amend' instead.
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2547
Summary:
More & more use cases come up with empty suite names, guessing
and making test names nice is mess. Will leave it as it is, except
data set part, as it could be super long.
Test Plan: - Try running some phpunit tests with & without data sets
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2544
Summary: Request from @csilvers, whose team is alergic to $EDITOR.
Test Plan:
Adding reviewers and CCs to this diff via CLI. The initial commit message for this diff is:
Add "--reviewers" and "--ccs" flags to arc diff
Request from @csilvers, whose team is alergic to $EDITOR.
Tested: Adding reviewers and CCs to this diff via CLI. The initial commit message for this diff is:
(...infinite recursion omitted...)
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2538
Summary:
- When you run "arc diff" in Mercurial, we currently give you an empty template. Instead, prefill a likely template by parsing messages, as we do for Git.
- Unify Git and Mercurial logic for acquiring messages, since `getLocalCommitInformation()` now provides this information. This should improve Git performance, and allows us to delete some code.
- Merge messages more intelligently. Currently, we just overwrite fields. Instead, merge arrays (like ccs, reviewers, tasks) and concatenate strings (like summary and test plan). We need to special case "title", see inline.
- @csilvers: this is a precursor to implementing "--reviewers", etc.
Test Plan: See next post, test plan includes giant output.
Reviewers: btrahan, csilvers, Makinde, jungejason, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2536
Summary:
Better test name handling for tests with data sets.
Instead of showing test name with full data set, show it's id/name,
e.g. `testConstructor with data set #1`
Test Plan: - Try running tests with data sets
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2535
Summary:
PHPUnit wrapper for arc.
The idea here is simple - find the test case which is related to the updated
class file. Generate tmp files for json & clover reports, run phpunit
with provided arguments.
It supports phpunit configuration file setting in `.arcconfig`: `phpunit_config`.
Path should be relative to project root.
Test Plan:
- Set `unit_engine` to `PhpunitTestEngine`
- Try running tests with & without `phpunit_config` option.
Reviewers: epriestley, davidreuss
Reviewed By: epriestley
CC: aran, Koolvin, jungejason
Differential Revision: https://secure.phabricator.com/D2472
Summary:
Several related changes:
- Add a "--conduit-version" flag, so you can actually diff conduit version bumps. Otherwise, the server rejects you.
- Make "arc upgrade" upgrade both libphutil and arcanist, not just arcanist.
- Bump the version number to 5. See D2527.
Test Plan:
- Ran "arc upgrade".
- Ran "arc diff". Got told there was a version issue.
- Ran "arc diff --conduit-version=4" to create this diff.
Reviewers: indiefan, nh, vrana, btrahan, jungejason, Makinde
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2528
Summary:
Hive has custom integration which relies on prefilling a bunch of information from JIRA. This is currently broken and accomplished in a roundabout way. Instead, add an event that the integration can listen for. See next diff.
@Girish, I guess you're official Phabricator/Hive support now?
Test Plan: Ran the Hive/JIRA workflows in Git and SVN, see next diff.
Reviewers: Girish, btrahan, ashutoshc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1206
Differential Revision: https://secure.phabricator.com/D2449
Summary:
There are different options how to implement this:
We can also generate the warning in `validateField()` and handle it in all callsites.
This is sufficient for me and simple enough.
Test Plan: `arc diff` revision with reviewer away/not away.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D2493
Summary:
If you forget to provide an argument for --update and have another argument
following it (e.g. HEAD^), we should provide a nice error message instead
of passing that argument through to a conduit call and then printing the
conduit error.
Test Plan: ran 'arc diff --update HEAD^' and got a nice error message
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2468
Summary:
- "git log" still includes "\n", so we're currently generating nonsense hashes like "\nabd9879bab86ad78ab...".
- Correct the log range we use in Git. See comment. When users perform merges, their expectations about what the "included commits" and what the included changes are are different. Represent them with two different ranges.
- Same deal for Mercurial
Test Plan: Ran "arc which" in various contexts.
Reviewers: btrahan, aurelijus, Makinde
Reviewed By: Makinde
CC: aran, nh, jungejason
Maniphest Tasks: T873
Differential Revision: https://secure.phabricator.com/D2460
Summary: Provide far more information about what "arc diff" intends to do.
Test Plan: Ran "arc which" in a variety of circumstances.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran
Maniphest Tasks: T1183
Differential Revision: https://secure.phabricator.com/D2454
Summary:
When getting an encoding, we should query the server for the encoding of the
project that we're exporting from, not the project that we're running arc in
(arc might not be in a working copy).
Test Plan: ran arc export with --diff and didn't get a workflow exception
Reviewers: epriestley, jungejason, vrana, davidreuss
Reviewed By: jungejason
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2455
Summary:
- Allow users to delete the remote copy of a feature branch as well as the local one, for workflows that push feature branches. We test if the remote exists before trying to delete it.
- Raise a better warning when you misuse "arc land".
- I also wrote some documentation about this, see next diff.
Test Plan:
- Tried to land a branch onto itself.
- Ran "arc land --delete-remote" on feature branches with and without remote feature branches.
Reviewers: aurelijus, btrahan
Reviewed By: aurelijus
CC: aran
Maniphest Tasks: T1204
Differential Revision: https://secure.phabricator.com/D2445
Summary: I renamed this in D2437 for greater consistency with everything else, but missed this use of the old key.
Test Plan: idk lmk?
Reviewers: Makinde
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2452
Summary: Currently, we ship only the summary, but we need to ship the whole thing for T1189.
Test Plan: Added var_dump() + die, ran in git and hg working copies, verified 'message' included the whole message.
Reviewers: csilvers, btrahan, Makinde
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1189
Differential Revision: https://secure.phabricator.com/D2437
Summary:
- Try to limit the pain of //future// version bumps by making arc self-updating.
- When the server needs a newer version, prompt the user to update.
- (We need them to reissue their command because we may already have loaded classes which have changed in the update.)
- Make the message sound exciting!
Test Plan: Artifically bumped server forward, ran "arc list", got to upgrade!
Reviewers: Makinde, nh, jungejason, btrahan
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D2435
Summary:
ArcanistDiffWorkflow::getGitParentLogInfo() calls
ArcanistDifferentialCommitMessage::newFromRawCorpus() which may throw a
ArcanistUsageException if the parent commit message is malformed (specifically,
a bad "Differential Revision:" line); this should not stop arc diff.
Test Plan: successfully ran arc diff where the parent commit message was malformed.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2434
Summary:
There's no reason these should be exclusive: -m is used only on update for the
update message, and --verbatim doesn't affect the udpate message. It's also
useful to allow both of these, if say I want to update my test plan by editing
my git commit message and also provide a message for the update from the
command line.
Test Plan:
ran arc diff with --verbatim and -m and saw my message from -m was used, as
well as my updates in the commit message went through.
Reviewers: jungejason, epriestley, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2426
Summary:
This is mostly an onboarding thing, but also allows "arc upload", "arc download", and "arc paste" to work anywhere on the system.
- Try to read the Phabricator install URI from arc global config if we can't find ".arcconfig".
- Build a WorkingCopy anyway if we can't find ".arcconfig", as long as we can find ".svn", ".git", or ".hg".
- Make all the workflows handle "no project ID" at least somewhat gracefully.
Test Plan:
- Ran "arc diff" in .arcconfig-less Mercurial, Git, and Subversion working copies.
- Ran "arc upload" and "arc download" from my desktop.
- Ran "arc paste" from somewhere random.
- Cleared my config and hit the error, got useful instructions.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2424
Summary: These are the unambiguously-good changes from D2388. Show commits included in a revision in the editor in "arc diff".
Test Plan: Ran "arc diff", saw which commits were being included.
Reviewers: nh, jungejason, vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1183
Differential Revision: https://secure.phabricator.com/D2406
Summary:
Always use the value in git.default-relative-commit when getting the relative
commit in git.
Test Plan:
Ran arc diff in a repo with git.default-relative-commit set to HEAD^ on a
branch tracking a remote (that is different from HEAD^), and checked that
the diff against HEAD^, not the remote, was published.
Reviewers: jungejason, epriestley, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2409
Summary:
Essentially D2391, but with, uh, more comments?
- I forgot that we already implemented shouldOverwriteWhenCommitMessageIsEdited(). This patch already behaves nearly correctly.
- Requires changes in D2412.
- Use `'edit' => 'edit'`, which does the same thing as `'edit' => true`, but is more correct after the "edit" / "create" split.
- Under "--verbatim", always get the message "from the user", which means "from the working copy" because verbtatim disables the editor part.
Test Plan:
- Created and updated revisions with `arc diff`.
- Created and updated revisions with `arc diff --verbatim`.
- Updated revisions with `arc diff --edit`.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: vrana, aran
Differential Revision: https://secure.phabricator.com/D2411
Summary:
Clean up the remaining odds-and-ends here -- move to "differential.close", get rid of the old constant, etc.
I'll wait a week or two to land this since "differential.close" just landed and all the other stuff is trivial.
Test Plan: Grepped for "committed".
Reviewers: btrahan, vrana, Makinde
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T909, T1182
Differential Revision: https://secure.phabricator.com/D2309
Summary:
The major thing I want to do here is allow you to set a default Phabricator URI, so we can make "arc paste", and "arc upload", "arc download" work anywhere.
We can also relax the .arcconfig requirements (request from @csilvers).
Test Plan:
Get/set some values?
iiam
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2400
Summary:
- We no longer need color options since we fake our way through parsing ANSI colorized diffs and use HGPLAIN (on Windows, too!). Drop 'em.
- In the case where you have nothing outgoing, we don't cache the relative commit and thus run "hg outgoing" too many times, which is fairly slow (even if you have nothing outgoing). Cache it.
Test Plan: Ran "arc diff --trace" in a mercurial working copy with nothing outgoing; verified we run "hg outgoing" only once.
Reviewers: Makinde, csilvers, btrahan
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2399
Test Plan:
Cancel `arc diff`.
Verify that the message is created.
Run `arc diff --verbatim` and see no reuse message question.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2390
Summary: With `--verbatim` flag, notes created from parse errors are never displayed to user resulting in blank fields.
Test Plan:
- `arc diff --verbatim` with invalid Cc
- `arc diff --verbatim` with all fields correct
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2395
Test Plan: Ran arc land locally with both the mutable default option and with the --merge flag to ensure that messages are set properly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2372
Summary: In Windows, you can't use `X=y cmd` syntax to set variables. Use "set X=y & cmd" instead.
Test Plan:
- Ran "arc diff" in a Mercurial repo in Windows, created D2367.
- Verified this does //not// cause 'HGPLAIN' to be set in the outer shell (where you type "arc diff").
Reviewers: Makinde, tido, indiefan, btrahan
Reviewed By: tido
CC: aran
Maniphest Tasks: T1179
Differential Revision: https://secure.phabricator.com/D2368
Test Plan: Run arc diff locally, verify via git log that the commit is amended afterwards (using the mutable history paradigm)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2369
Summary:
We do unnecessary working copy checks under "--show", even though the working copy isn't relevant.
Also, 'sourcePath' may not be set (e.g., "arc commit --show --revision X" where X is some "--only" revision).
Test Plan: Ran "arc commit --show --revision 1" against some test data, got clean output.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2353
Summary: Apparently my advice here was terrible, and `--no-decorate` and `--decorate=no` are both very recent additions to Git which a bunch of users don't have. Get rid of them since D2344 allows us to parse all decorate levels anyway.
Test Plan: Tried to google "git changelog", got a bunch of pages about managing changelogs with git.
Reviewers: zeeg, ehren
Reviewed By: ehren
CC: ehren, aran, NorthIsUp
Differential Revision: https://secure.phabricator.com/D2354
Summary:
"git diff -M -C" generates useful metadata (moves/copies) but (for a pure move) no diff text. Synthetically build the diff text after the fact so this information is available in Differential.
This patch is kind of nasty but I couldn't see a cleaner way to do it. :/
This also needs some UI changes in Differential: we get a full-green new file right now, but it would be better to default-hide it with "This file was moved. Show More" or similar.
Test Plan: Moved a file, ran "arc diff", got textual diff.
Reviewers: aran, tuomaspelkonen, jungejason, btrahan, vrana
Reviewed By: vrana
CC: aran, epriestley, vrana
Maniphest Tasks: T230
Differential Revision: https://secure.phabricator.com/D479
Summary:
Arcanist fails to find git's 'commit <hash>' header when log.decorate is
set in one of git's config files. git adds the named refs that point to
the commit in parentheses following the hash. This changes the regex
used by arcanist to match git commit headers to optionally match the
branch names in parentheses following the hash.
Test Plan:
Run `git config --global log.decorate short` and check that `arc diff`
runs successfully.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2344
Summary: Missed this when getting rid of all the 'file' calls.
Test Plan: Meta.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1159
Differential Revision: https://secure.phabricator.com/D2327
Summary:
Wrapper for Python 'nose' (http://readthedocs.org/docs/nose/en/latest/)
testing tool.
Test Plan:
Install latest 'nose' v1.1.3. Currently it is available through
Github only (``pep install git+https://github.com/nose-devs/nose.git``).
Create a Python project with following structure:
/package_name/module_name.py
/tests/package_name/test_module_name.py
Write some tests
Run ``arc unit``
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, zeeg
Differential Revision: https://secure.phabricator.com/D2322
Summary: Currently, if you change a symlink outside a libphutil library and the link target is something inside a libphutil library, we may enter an inifite loop in the "do { ... } while(...)" later. Just bail if the loop won't resolve.
Test Plan: Ran arc unit, Airtime reported the issue resolved by a similar fix.
Reviewers: cpiro, btrahan
Reviewed By: cpiro
CC: aran
Differential Revision: https://secure.phabricator.com/D2318
Summary:
Allow tests to be skipped by calling assertSkipped(). It's not really
an assertion of anything tangible; more like "assert that we can't
really assert anything right now".
Test Plan: Added a new test to the PhutilUnitTestEngineTestCase.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2312
Summary:
- Add branch name tab completion to "arc land".
- Default to landing the current branch.
- This is a little bit hacky but not too terrible. I'm planning to move the whole thing to PhutilArgumentParser at some point so that'll be an opportunity for a big refactor.
Test Plan: Hit tab, landed this branch.
Reviewers: zeeg, btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran, kdeggelman
Differential Revision: https://secure.phabricator.com/D2293
Summary:
Don't use abstract subclasses of ArcanistPhutilTestCase, only use
concrete ones.
This lets you put common functionality in an abstract BaseTestCase
(which itself is a subclass of ArcanistPhutilTestCase), then implement
concrete subclasses of the BaseTestCase.
Test Plan: Tested with a simple Base -> {Case1, Case2} setup.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2300
Summary:
- Replace SVN-specific language with VCS-agnostic language.
- Add new "arc close-revision", works exactly like "arc mark-committed" but with agnostic language.
- Use status constants, not status strings.
- Mark "arc mark-committed" deprecated.
- Remove deprecated "arc merge".
Test Plan: Ran "arc mark-committed", "arc close-revision".
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran, Makinde
Maniphest Tasks: T909
Differential Revision: https://secure.phabricator.com/D2244
Summary: See rage in T1117. Don't use the <project + branch> heuristic anymore..
Test Plan: Ran "arc diff --strict HEAD^" on a commit stacked on top of this one, got no matches.
Reviewers: btrahan, vrana, simpkins, beng
Reviewed By: btrahan
CC: aran, avive
Maniphest Tasks: T1117
Differential Revision: https://secure.phabricator.com/D2221
Summary:
- Historically, "--preview" was forbidden under SVN. No reason for that now.
- The "--auto" patch moved the "--preview" / "--only" checks later than they should be.
- Fix an issue with Conduit query construction in SVN.
Test Plan: Ran "arc diff --preview" in an SVN working copy. Ran "arc diff" in an SVN working copy.
Reviewers: svemir, btrahan, vrana, jungejason
Reviewed By: svemir
CC: aran
Differential Revision: https://secure.phabricator.com/D2218
Summary:
When people create the .arc/default-relative-commit scratchfile with $EDITOR of
choice, their editor usually puts a newline at the end, which breaks arc diff.
We should trim the newline before using the contents of the scratchfile.
Test Plan:
ran arc diff in a working copy that contained a .arc/default-relative-commit
with a newline
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2209
Summary: If you have two projects (say, libphutil and arcanist) and you prepare a patch for one of them on branch "master", run "arc diff", and then prepare a patch for the other one on the same branch, "arc diff" will try to update the first revision when you run it. Instead, make it smart enough to stay within arc projects.
Test Plan: Ran "arc which" in circumstances where it previously generated a false positive, no false positive.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T1100
Differential Revision: https://secure.phabricator.com/D2199
Summary: See discussion in D1861.
Test Plan: Ran "arc diff" on master, got an upstream-based relative commit. Ran "arc diff" on a feature branch, got a config-based relative commit. Ran "arc diff x", got an argument-based relative commit.
Reviewers: btrahan, vrana, davidreuss, elgenie
Reviewed By: davidreuss
CC: aran
Differential Revision: https://secure.phabricator.com/D2192
Summary:
For Objective-C repositories, we want to provide aliases to
arc diff --amend-autofixes by default.
This adds the ability to define aliases in .arcconfig (overridden
by any specified in the user config, of course).
Test Plan:
Tested defining alias with nothing in .arcconfig, with
an alias in .arcconfig. Tested arc alias outside of working
repository.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2191
Summary:
Xcode (a popular code editor on Mac OS X) has no facility
to trim trailing whitespace automatically.
This adds a new lint severity "AUTOFIX" that's between
WARNING and ERROR. When running the linter, any lint message
whose severity is AUTOFIX will automatically be patched.
Furthermore, if all lint messages returned from the engine are
AUTOFIX, we'll automatically amend HEAD with the patch.
Test Plan:
arc lint on files with and without trailing whitespace,
with and without UTF-8 contents to confirm those still error
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2125
Summary:
--auto doesn't work right now on the implicit --create pathway in SVN and HG because we hit these conditions.
Also improve a message.
Test Plan: Ran "arc diff" in unaffiliated working copies in HG and SVN.
Reviewers: svemir, btrahan, vrana, jungejason
Reviewed By: svemir
CC: aran
Differential Revision: https://secure.phabricator.com/D2187
Summary: See next diff for an explanation of this issue.
Test Plan: See next diff.
Reviewers: Makinde, btrahan, vrana, jungejason
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2174
Summary:
```arc close T1088 --status wontfix --message "I'm not going to fix this."```
T1088 is the test task to screw with, so feel free.
Test Plan: ```arc close T1088 --status [ resolved | wontfix | invalid | duplicate | spite | open ] -m "Message"```
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2162
Summary: Saw this in a diff somewhere; complain about it.
Test Plan: Unit coverage.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1060
Differential Revision: https://secure.phabricator.com/D2153
Summary:
Added `arc tasks`:
%%%arc tasks
arc tasks --view-all // View Open and Closed Tasks
arc tasks --by-status // Group By Status
arc tasks --by-priority // Group By Priority%%%
Test Plan: Connect to conduit and run arc tasks >> make sure you have tasks =p
Reviewers: epriestley, indiefan
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T749
Differential Revision: https://secure.phabricator.com/D1943
Summary:
NOTE: This is a disruptive change to how 'arc diff' behaves by default.
Many years ago, Differential (then DiffCamp) supported SVN. Someone added a "-i" mode to the "diffcamp" script so you could "git show | diffcamp -i", and thus we were
forever bound to storing metadata in commit messages.
But this isn't a common use case outside of Facebook + git-svn, and isn't very flexible. We now have a great deal of flexibility to identify revisions based on
hashes, branch names, etc, and to sync metdata from web to CLI and back. I want to jettison the commit-message-bound artifacts of the tool's history, and move to a
more flexible, modern workflow.
I added "--auto" a while ago, which figures out if you want to create or update a diff automatically, and then prompts you for whatever data it needs, reading
information if it can from commit messages in the range. This is a vastly better workflow in general, especially for SVN and Mercurial users (who currently need to
jump to the web UI to create revisions). It's better for git users too, since they don't need to use template commits and don't have to muck with or configure
templates. However, it's a nontrivial change to git users' core workflow that is clearly different in more ways than it is clearly better.
- This might be contentious, but probably not toooo much, I hope?
- I also deleted the (fairly ridiculous) workflow where we sync commit message changes from the working copy. I think two or three people will be REALLY upset about
this but I have no sympathy. "--edit" covers this and has been around for like two years now, and making the commit message and web dual-authoritative was always a bad
idea that we only ever half-accommodated anyway (see giant swaths of removed TOOD nonsense).
Test Plan:
- I've been using "--auto" exclusively for like a month, it seems to work well.
- Created/updated SVN, Git and HG diffs with "arc diff" under this workflow.
Reviewers: btrahan, jungejason, nh
Reviewed By: btrahan
CC: Makinde, vrana, zeeg, mbautin, aran, yairlivne, 20after4
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D2080
Summary:
This should be a raw text block, not a parsed message object.
I broke this in rARC1f13e022cdc9bf4859274a83784bd615caf62ef9 when I improved Mercurial support.
See: https://github.com/facebook/arcanist/issues/23
Test Plan: (Will update this diff...)
Reviewers: vrana, jungejason, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2136
Summary: Allows you to set a default in .arcconfig. This default is overriden by any .arc/ setting.
Test Plan: Ran "arc diff" with a .arcconfig setting but no .arc/ setting, got a diff against the specified relative commit.
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D2093
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2084
Summary:
The default of "arc diff" to "arc diff HEAD^" in git is universally confusing to everyone not at Facebook.
Drive the default with configuration instead. Even at Facebook, "origin/trunk" (or whatever) is probably a better default than "HEAD^".
See D863 for the last attempt at this.
NOTE: This is contentious!
Test Plan: Ran "arc diff", got prompted to set a default. Ran "arc diff" from a zero-commit repo, got sensible behavior
Reviewers: btrahan, vrana, nh, jungejason, tuomaspelkonen
Reviewed By: btrahan
CC: aran, epriestley, zeeg, davidreuss
Maniphest Tasks: T651
Differential Revision: https://secure.phabricator.com/D1861
Summary:
See D2049, D2050. Identify reuses of locals as iterator variables. Before raising an error, we require:
- Variable is declared before the loop.
- Variable is used after the loop, ignoring uses as an iterator variable.
I think this identifies all problems with a very low false positive rate (the false positives are suspicious/unconventional code, but not necessarily errors).
Also fix an issue identified by the linter.
Test Plan:
- Verified this identified the bugs in D2049 and D2050.
- Ran linter against libphutil/, arcanist/ and phabricator/ (see D2051, this, and next diff).
- Ran unit tests.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley, jungejason
Differential Revision: https://secure.phabricator.com/D2052
Summary:
Addresses concerns in rARCefb8219196abf047f14b505959e54d078e1df6d3:
- As I recall, the intent of "generateFile" was that these warnings would replace all the other warnings for that file, under the theory that if one warning caused regeneration of
the file the other warnings were irrelevant.
- However, this code never had any effect and I haven't seen any issues with the behavior.
- So, just remove it.
Addresses concerns in rARC070e963d1c26879e47eab19a2377e388c2f166c5:
- Ran "arc liberate --all".
Test Plan: Ran "arc lint" after removing an "__init__.php" with and without a "fixed" generateFile block, there was no behavioral change. So I just nuked it.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2049
Summary:
D2016 changed the behavior of `phutil_console_wrap()`.
The new behavior is better so I am fixing callsites instead of the function.
Test Plan:
arc help help
Verify that the options descriptions is not indented with 28 spaces.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2047
Summary: When tests have a lot of output, show a diff of the expected/actual.
Test Plan: Used this when developing D2016 to examine large output usefully.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2017
Summary: Git can't apply filename case change patches on case-insensitive filesystems. Some day we could manually do this ourselves, but it's fairly rare and complicated -- just raise a useful warning.
Test Plan: Tried to apply a case-changing patch, got a good error.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T618
Differential Revision: https://secure.phabricator.com/D2015
Summary:
- We have "--merge", added similar "--squash" for completeness.
- Switched away from (cd .. && ...) pattern which is not functional on Windows.
- Allow a default --onto to be specified, if master isn't the best default for a project.
Test Plan:
- Did --hold --keep dry runs, changes landed properly.
- Set a default onto branch in .arcconfig.
- Ran --merge and --squash together to verify conflict configuration.
Reviewers: btrahan
CC: aran, epriestley
Maniphest Tasks: T124, T1033
Differential Revision: https://secure.phabricator.com/D2001
Summary:
In Mercurial, we figure out if the working copy is dirty with "hg diff", and do a somewhat expensive merge-base / outgoing operation if the relative commit isn't set. We can set the relative commit earlier and avoid some extra work.
We also may incorrectly cache this state (diff from merge-base of outgoing to tip) and pass the wrong rev and file dirty list to the linters.
Test Plan: Made commits which changed (A, B) and then (A). Ran "arc diff tip^". Before this change, observed full outgoing + merge base resolve and both "A" and "B" passed to lint. Observed execution of fewer commands and lint executing against "A" only after this change.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1998
Summary: Obvious error in refactoring code around getCanonicalRevisionName() in D1954.
Test Plan: Ran "arc diff" without looping. Can you verify this fixes your case?
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T1025
Differential Revision: https://secure.phabricator.com/D1970
Summary:
See task. Allow mercurial users to diff with uncommitted changes.
- By default, commit range is merge-base of `hg outgoing` to `.` (dirstate).
- You can get JUST dirstate with `arc diff tip` or similar.
- This ended up being a giant mess various other changes to deal with empty `hg outgoing` and empty dirstate.
Test Plan: Diffed with uncommitted changes, got sensible prompts and results.
Reviewers: Makinde, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T998
Differential Revision: https://secure.phabricator.com/D1954
Summary: We crash and burn right now, trying to launch an interactive editor against a non-tty stdin. Raise a helpful error message instead.
Test Plan: Will run "arc diff --raw --update".
Reviewers: btrahan, yairlivne
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1949
Summary:
- `git rev-parse --verify` "verifies" very valid-looking commit name, not just valid commit names.
- Currently, if we can't find the base rev we'll incorrectly "verify" it and then fail on "git checkout -b <branch> <some bogus commit>".
- Instead, use `git cat-file -t`.
- See similar fix in D1590.
Example:
$ git rev-parse --verify aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
Test Plan: Ran "arc patch" in a mismatched local, hit "Y" to branch, got a branch off HEAD instead of an error.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1947
Summary:
In some workflows, we don't have Conduit at all, so we'll fail with a raw exception. Test for conduit presence before trying to make the encoding call.
Also move some "instanceof" logic for updates into RepositoryAPI (factoring, windows compat).
Test Plan: Ran "arc patch --patch some.patch".
Reviewers: 20after4, davidreuss, nh, btrahan
Reviewed By: 20after4
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1935
Summary:
We currently detect the "\" as a change, and may generate a hunk like this, without changes.
@@ -97,4 +98,4 @@
mmm
mmm
mmm
\ No newline at end of file
While "git apply" is OK with this, "patch" is not. Instead, don't detect this as a change (it is always accompanied by - / + if it's a real change).
Test Plan: Successfully applied a previously-failing SVN patch to a file without a terminal newline that had nonlocal changes.
Reviewers: 20after4, nh, btrahan
Reviewed By: 20after4
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1934
Summary:
In cases where a codebase is not UTF-8, we will attempt an conversion,
if an alternative encoding is given/configured.
This is now possible in two ways:
- by configuring one under repository tracking in diffusion
- by passing an --encoding option to the workflow
If the first is not available we will make a conduit call
to do an extra check and see if an encoding is configured directly with
phabricator.
Test Plan:
Tried various diffs with known encodings (mostly ISO-8859-1), and passed
it in, via stdin, or downloaded a known problematic revision from
phabricator, and they applied where they otherwise failed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D1880
Summary:
- When users pipe in colorized diffs, strip the colors instead of failing.
- When showing context, show line numbers (we do show 3 lines around the failure, the failure was just on the first line).
- Remove an irrelevant TODO comment (we handle this elsewhere now).
Test Plan: Unit tests.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1887
Summary:
`svn rm $directory` removes the directory and its contents in svn 1.7, whereas
svn 1.6 only removes the directory's contents, so arc diff needs to not try
to read the directory.
Test Plan:
ran arc diff in a svn working copy that had a directory removed (with both
svn 1.6 and svn 1.7) and confirmed that the command did not throw an exception,
and that the removed directory was marked as directory (not a file).
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1889
Summary: svn changed the format of property changes for svn1.7.
Test Plan: arc diff on a working copy with svn property changes
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1885
Summary: @koolvin ran into this and was justifiably confused.
Test Plan: Put some random .php file in src/, ran arc liberate src/, got warned.
Reviewers: btrahan, Koolvin
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1878
Summary:
svn 1.7 reports a rev number of -1 for added files; previous versions
reported a rev number of 0. Interpret nonpositive rev numbers to mean
the file was added.
Test Plan: ran arc diff on an svn1.7 working copy with added files
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1867
Summary: Wanted to double-check that this works properly; it does, but might as well keep the test case.
Test Plan: Ran tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T970
Differential Revision: https://secure.phabricator.com/D1869
Summary: We incorrectly merge array() into empty string, which is later interpreted as "this file is entirely not-executable". Instead, show no coverage information in the UI.
Test Plan: Looked at a README diff with no coverage information, got no UI render.
Reviewers: tuomaspelkonen, btrahan, zeeg
Reviewed By: tuomaspelkonen
CC: aran, epriestley
Maniphest Tasks: T965
Differential Revision: https://secure.phabricator.com/D1863
Summary: Give "arc cover" --rev and paths parameters like "arc lint" and "arc unit".
Test Plan: Ran 'arc cover' with various rev/path things. Ran in SVN to verify it works/fails correctly.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T707
Differential Revision: https://secure.phabricator.com/D1860
Summary:
Resolves crazy stuff like 'HEAD' or '{2001-01-01T01:01:01}' into an
actual revision, with a provided implementation for git.
Test Plan: Tested in a hacky script.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1849
Summary:
svn 1.7 no longer has a .svn directory in every subdirectory of the working
copy, only one in the root of the working copy (like git, except you can still
check out subtrees). Thus, we can't check whether we're in an svn repo by
looking for a .svn directory alongside the .arcconfig file.
Test Plan: ran arc diff in an svn 1.7 working copy where it previously wasn't working
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1848
Summary:
- Add a lint check for PHP 5.3 features (namespaces, anonymous functions).
- Add unit tests.
- Disable it by default.
- Enable it for us.
Test Plan: Ran unit tests.
Reviewers: btrahan, edward
Reviewed By: edward
CC: aran, epriestley
Maniphest Tasks: T962
Differential Revision: https://secure.phabricator.com/D1846
Summary:
Make lint output look like gcc errors / grep output; a format used by
editors to jump from a compiler error to a file+line of code which
triggered the error.
Test Plan:
In vim, I ran:
:set makeprg=~/checkouts/devtools/arcanist/bin/arc\ lint\ --output\ compiler
Then:
:make
And was able to jump directly to problematic lines of code.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran, edwardspeyer, jungejason, codeblock, tuomaspelkonen, epriestley
Differential Revision: https://secure.phabricator.com/D550
Summary:
This allows for disabling certain PEP8 linter errors by calling
setCustomSeverityMap on an ArcanistPEP8Linter. However, any custom severities
besides disabled will be ignored.
Test Plan: arc lint
Reviewers: epriestley, andrewjcg
Reviewed By: epriestley
CC: jungejason, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1839
Summary: `arc which` and related workflows have supplanted these now-unused mechanisms.
Test Plan: Grepped for chooseRevision() and DifferentialRevisionRef.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1841
Summary: Similar to rARC298ed9cda55d90b755e4dc45d202213e91631888, work around "differential.anonymous-access" issues until we get proper permissions up.
Test Plan: Ran "arc patch" against a "differential.anonymous-acccess" install with a revision argument
Reviewers: btrahan, mbautin
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T848
Differential Revision: https://secure.phabricator.com/D1835
Summary: Simpler assert function for asserting a type of exception was raised.
Test Plan: Wrote this for (and tested it with) D1836.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1837
Summary: If a case does not end with break, continue, throw, exit or return and does not have a "fallthrough" comment, raise a warning.
Test Plan: Ran test case; ran linter against all of Phabricator (no hits).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1824
Summary: Linter flipped out on D1817; reign it in.
Test Plan: Ran "arc lint" on D1817, got one message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1818
Summary:
- When you try to "arc patch" a change which adds or removes a trailing newline or alters a file with a trailing newline near the newline, the patch fails because we fail to reconstuct the "\ No newline at end of file" line.
- We don't currently record enough information about these diffs to reconstruct them with a sensible amount of effort. Store the "\ No newline at end of file" line instead of just a flag.
- There are some special rules for these lines; implement them.
NOTE: This causes some display glitching in Differential, but nothing major; I'll fix that in a followup patch.
Test Plan:
- Created a diff which added a trailing newline, removed a trailing newline, and altered a file without a trailing newline near the end of the file.
- Verified that the git version of this patch and the 'arc export --git' version of this patch are (essentially) identical.
- Verified that "arc diff" + "arc patch" can apply these changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T533
Differential Revision: https://secure.phabricator.com/D1810
Summary: good title
Test Plan: ran "arc patch DX" a bunch
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T892
Differential Revision: https://secure.phabricator.com/D1789
Summary: This may raise other types of exceptions, e.g. http/network exceptions, where getErrorCode() is not defined.
Test Plan: Reasoned about beahvior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1804
Summary:
See <https://github.com/facebook/phabricator/issues/97>. Mercurial branch names may contain any characters, but we currently reject branch names with spaces.
NOTE: Mercurial allows you to name branches things like ` ` (five spaces). We do not parse this correctly. Die in a well fire.
Test Plan:
- Added some horrible-but-possible test cases for crazy branch names.
- Unit tests now pass.
Reviewers: btrahan, Makinde, killermonk
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1795
Summary:
On Windows, the PHP function escapeshellarg() replaces '%' with ' ' (space). This is apparently because there is no safe way to escape % inside of strings.
cmd.exe does use "^" as an escape character, so I think replacing "xyz" with "^x^y^z" might work for arbitrary strings (maybe?), or at least some subset of strings, but I don't know cmd.exe well enough to make that call without being concerned I'm introducing a security issue.
Although this patch is dumb, it's certinaly safe, and can only do something wrong if the user has environmental variables like H, P, T, or x01, in which case they're sort of asking for it.
cmd.exe also truncates output on \0, so use \1 as a delimiter instead.
Seriously it's like this thing was written in 1982 and never ever changed.
Test Plan: Created D1783 successfully after applying this patch.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T124
Differential Revision: https://secure.phabricator.com/D1785
Summary: This use of "(cd ..)" is outside of the RepositoryAPI proper (it's when we're trying to figure out which VCS a directory uses) and explodes on Windows.
Test Plan: @koolvin applied this patch manually and got farther than before.
Reviewers: btrahan, Koolvin, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T124
Differential Revision: https://secure.phabricator.com/D1779
Summary:
`arc help` currently prints more than 500 lines and it is hardly usable without ##> arc.help##, ##| less##, ##| grep##, ##| wc -l## or such.
It is even worse with custom commands and options.
This diff changes `arc help` to only list commands with no description (under 100 lines).
It also adds `arc help --full` which maintains the original behavior.
It doesn't change `arc help <command>`.
NOTE: BC break on different levels.
Test Plan:
arc help
arc help --full
arc help help
arc help lint
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1769
Summary:
There's no reason this can't work now, we're just not quite careful enough about definitions.
NOTE: If you use a message which has a revision ID in it already, this will fail in an obtuse way when launching the update editor fails because stdin isn't a tty. I'll clean this all up after T614 is fully sorted.
Test Plan: Ran "arc diff --raw -C HEAD".
Reviewers: btrahan, yairlivne
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1765
Summary:
We currently raise a very confusing error when we hit this case:
Exception: The phutil library '' has not been loaded!
Because of the trickiness of init-order stuff, it's difficult to detect this more explicitly earlier -- instead, just raise a more descriptive error.
Test Plan: Ran "arc unit" in a copy of libphutil other than the one arc loads.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T580
Differential Revision: https://secure.phabricator.com/D1760
Summary:
- When altering the include_path(), use PATH_SEPARATOR (";" on Windows, ":" elsewhere) instead of hard-coded ":".
- Detect missing php_curl.dll extension.
- Use APPDATA instead of HOME for storing .arcrc (the internet implies this is correct?)
- Don't try to do chmod() stuff on Windows; it's not critical and I don't want to figure out how it works.
Test Plan: Was able to run part of some arc commands on Windows.
Reviewers: btrahan, Makinde, Koolvin
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T124
Differential Revision: https://secure.phabricator.com/D1756
Summary:
Otherwise it says after hitting Tab everytime:
> Warning: Invalid argument supplied for foreach() in arcanist/src/workflow/shell-complete/ArcanistShellCompleteWorkflow.php on line 99
Test Plan: arc h<Tab>
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1766
Summary:
- This is simpler and reuses more code than doing "(cd %s && ...)" every time.
- If we have an issue like HGPLAIN, we have one place to fix it now.
- On Windows, the construct "(cd %s && ...)" does not mean what we'd like it to.
Test Plan: Ran "arc diff" in a git repo with this change.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T124
Differential Revision: https://secure.phabricator.com/D1761
Summary: Allow the user to pick a revision explicitly if they think they know what they're doing. Similar to "arc amend --revision", etc.
Test Plan: Obviously compells a meta-test.
Reviewers: btrahan, fratrik
Reviewed By: fratrik
CC: aran, epriestley
Maniphest Tasks: T928
Differential Revision: https://secure.phabricator.com/D1753
Summary:
If a user reverts HEAD in the index or working copy, "git diff" shows no changes
and we explode.
Instead, we should simply return no changes; the rest of the lint pipeline
accommodates this correctly.
Test Plan: Reverted HEAD in a working copy, ran "arc lint", got an error.
Applied patch, ran "arc lint", got accurate lint.
Reviewers: Koolvin, btrahan
Reviewed By: Koolvin
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1729
Summary: Missed this in review (D1715), idx() does not operate on strings (maybe
it does in HPHP/i?).
Test Plan: Faked an editable lint warning, ran "arc lint".
Reviewers: Koolvin, andrewjcg, btrahan
Reviewed By: Koolvin
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1728
Summary:
When the user runs "arc diff --create" or triggers it via "arc diff
--auto", prefill the template as best we can.
Test Plan:
Ran "arc diff --auto" with a template commit message in the working
copy under various configurations, results seemed reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, dmi
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D1719
Summary:
When a user invokes the update flow from Mercurial, prefill the update message
like we do for Git.
Also add a little caching to improve performance since "hg" execs cost ~100ms.
Test Plan: Updated a Mercurial diff repeatedly, got sensible default message
fills.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T28
Differential Revision: https://secure.phabricator.com/D1718
Summary: See comments. Skip [defaults] in .hgrc when executing commands.
Test Plan: Ran "arc diff" in a mercurial working copy.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T922
Differential Revision: https://secure.phabricator.com/D1707
Summary:
Not too familiar with the patch rendering code, but diff should fix
a few issues:
1) The current didn't seem to handle the case of an insertion only
diff (where the original text is empty). Specifically, it would
still print a replacement line and would merge the line immediately
after the patch location into the patch application.
2) Patches with trailing newlines were rendered with an additional
newline. This appears to be due to using ##explode## to break the
patch into lines.
Test Plan:
1) Verified that this fixed apache license linting for files that had
no license.
2) Checked that this didn't affect trailing-whitespace patch application.
Reviewers: epriestley, aran
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1715
