Summary:
I usually write commit messages 1-2 minutes.
`arc lint` in our repository usually runs for around 30 seconds, `arc unit` another minute or two.
Even Phabricator unit tests sometimes runs long (because `CREATE DATABASE` and `DROP DATABASE` is slow in our setup for some reason).
Waiting for the results is boring and unnecessary.
This diff presents two different concepts how to run them on background:
# Lint is run with `--output json`, results are parsed and presented back to user. It isn't perfect - there's no context in printed lint errors which is a serious problem.
# Unit tests are run normally and the results are written to a scratch file. It also isn't perfect - colors are lost during the process.
I'll probably choose one approach and use it on both places. Let me know your thoughts about them.
This can be further improved to resolve the futures also after inputting the update message but it can be done in a separate diff.
Test Plan:
- Remove lint engine.
- Remove unit engine.
- Make lint errors.
- Make unit errors.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, beng, tuomaspelkonen, alanh
Differential Revision: https://secure.phabricator.com/D2614
Summary:
According to @epriestley, it's nasty and kind of crazy: D2933#1.
It also stands in my way for D2614.
Test Plan: Rewrote our callsite to event listener and verified that it still works.
Reviewers: tuomaspelkonen, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D3171
Summary:
If two branches have the same HEAD, they currently race to overwrite each other in $commit_map.
We don't need to return a map indexed by commit since nothing ever reads the keys out of it. Just update $branches in place.
Test Plan: With two branches at the same HEAD, ran "arc branch". Saw both branches in output.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3188
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: I originally excluded `mysql` these under the theory that it would be impossible to test anything without it, but `arc` doesn't need it and you could have only one of the mysql-flavored extensions without having the other one. Whitelist all the mysql and mysqli extension functions as far as depenencies are concerned.
Test Plan: @floatinglomas, let me know if this fixes things for you?
Reviewers: btrahan, vrana, floatinglomas
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3119
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: Just like we ship with a list of extension functions, add a list of extension classes so people stop getting lint errors about DOMDocument just because some linter uses it.
Test Plan: Ran "arc lint".
Reviewers: vrana, btrahan, codeblock
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3082
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: The mbstring extension is common but not part of the PHP core, and we have a soft dependency on it but generally guard calls with function_exists(). Add the mbstring functions to the whitelist of known extension functions so lint doesn't raise errors about them if you don't have mbstring installed.
Test Plan: @ffx, can you verify this fixes the lint issue for you? Probably faster than recompiling my PHP without mbstring.
Reviewers: ffx, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2961
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: I think @nh needs this at some point?
Test Plan: Added bogus and valid "load" to system config, got warning and load respectively.
Reviewers: nh, dschleimer, btrahan
Reviewed By: dschleimer
CC: aran
Differential Revision: https://secure.phabricator.com/D2931
Summary: We currently print //additional// libraries, but not the core libraries, which makes diagnosing include problems more difficult than necessary.
Test Plan: ran `arc derp --trace`, got libphutil/arcanist locations
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2929
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: Using /usr/bin/env in #! to allow the arc script to run as is in every UNIX OSes, like FreeBSD where bash is installed in /usr/local/bin/bash.
Test Plan: Tested on Solaris, Ubuntu, kFreeBSD/Debian and FreeBSD.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T1440
Differential Revision: https://secure.phabricator.com/D2913
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
