Summary: I broke this in D3750. Calling `getRepositoryAPI()` triggers a check for workflow requirement of the repository API. Instead, we should just check if we alreayd have one.
Test Plan: Ran `cat x | arc diff --raw`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1998
Differential Revision: https://secure.phabricator.com/D3848
Summary: turns out retina has files like image.png and image@2x.png. The latter breaks since '@' is a special command telling svn to "look for image at revision 2x.png" -- nonsensical garbage. If we add it to the end every time this error goes way.
Test Plan: touch 2@2.png; svn add '2@2@'; arc diff; <fill out form, accept commit>; arc commit -- observe commit working
Reviewers: epriestley
Reviewed By: epriestley
CC: mbishopim3, aran, Korvin
Maniphest Tasks: T1999
Differential Revision: https://secure.phabricator.com/D3845
Summary:
D3748 attempted to improve the behavior of `arc diff` when dealing with files merged from another branch, but had the side effect of marking all normal edits and deletes as adds. Revert this side effect, at least. This likely degrades the merging case, but it's comparatively rare, and editing/deleting files is very common.
I'll make an effort to fix this properly (and back it with DirectoryFixture tests) when I deal with T1947.
Test Plan: Ran `arc diff --preview` for a change that edits or removes files.
Reviewers: btrahan, vrana, svemir
Reviewed By: svemir
CC: aran
Differential Revision: https://secure.phabricator.com/D3836
Summary:
Some users assume they can update anything, not just revisions they own (see https://github.com/facebook/arcanist/issues/54).
Currently, if you `arc patch` or `arc amend` and get a commit message, then `arc diff` for a revision you don't own, we:
- Fail early with a very confusing message ("You can not review a revision you own!") if you are on the "Reviewers" line, until D3820.
- Or fail very very late with a good error message, but after lint, unit and update messages.
Instead, check that you own the revision as early as we can.
Test Plan: Tried to update revisions I didn't own, got good error messages early on (with D3820).
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3821
Summary:
See T1973. In T1675, we addressed parsing of diffs with `--no-prefix` or custom `--src-prefix` and `--dst-prefix` flags. However, this inadvetently broke diffing of files with spaces in their names, which Git does not quote. They look like this normally:
diff --git a/old file b/new file
Prior to D3744, we accidentally got this right by looking for the `a/` and `b/`. However, we no longer do, and instead produce nonsense results.
This problem is difficult because for files with spaces, `git diff --no-prefix` may generate an ambiguous line like:
diff --git a b c d e f g
From this line, we have no way to deterine if this moves "a" to "b c d e f g", or "a b c d" to "e f g", or anything in between. In some diffs we have more information later on, but in some cases we do not, e.g. for binary diffs without `--binary`.
Try to get this right in as many cases as possible:
- If there are quotes, we can unambiguously get it right. This only happens for filenames with quotes or unicode characters, however.
- If there is exactly one space, we can unambiguously get it right.
- Interpret the common case of `a/<anything> b/<anything>` in the most-likely-correct way again.
- Interpret the rare case of `<anything> <that same thing>` in the most-likely-correct way.
- Complain about any `a b c d e f g` garbage.
Test Plan: Ran unit tests. Created a diff of a file called "File With Spaces".
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, ReturnZero
Maniphest Tasks: T1973
Differential Revision: https://secure.phabricator.com/D3818
Summary: This is a BC break but it was introduced recently.
Test Plan:
$ arc unit x
No more:
> Fatal error: Argument 2 passed to ArcanistConfiguration::didAbortWorkflow() must be an instance of ArcanistBaseWorkflow, null given
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3812
Test Plan: Made lint error, slept in test, verified that tests are finished when I confirm `arc diff --excuse`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3803
Summary: This diff obsoletes D3385.
Test Plan: Made lint error, explained it, verified that unit already finished before I finished explaining (by adding `sleep(3)` to test).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3786
Summary:
We start Sandcastle push when `arc diff` starts.
If `arc diff` throws then HHVM waits for finishing the futures.
We need to kill them sooner.
Test Plan: Will implement the hook.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3713
Summary: Currently, a shutdown exception ("script exited with open transactions!") overwhelms the actual test failure exception, which is the one that needs to be fixed.
Test Plan: Ran a fixture test which opens a transaction and then throws. Got diagnostically useful output after this patch.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3780
Summary: We need to tweak a few patterns to accommodate the possibility that lines end in "\r\n".
Test Plan: Added failing unit test and made it pass.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, mbishopim3
Maniphest Tasks: T1944
Differential Revision: https://secure.phabricator.com/D3772
Summary: The way we represent some move/copy stuff is a bit messed up, but it mostly works, so add coverage before I mess with it.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T866
Differential Revision: https://secure.phabricator.com/D3752
Summary:
We have coverage for generating patches, applying them, and making sure they reproduce the original repository state. However:
- The code uses simplified patch generation which omits some flags like `-M` and `-C`, and generally produces less rich patches than we really produce. Instead, produce patches the same way `arc diff` does.
- We don't test the intermediate change representation. In theory it's not too important because if we get it wrong the output should be wrong, but in practice it makes it easier to nail down issues. We can also generate less-rich patches which still apply correctly, but would prefer not to.
- Similarly, we don't test the intermediate patch representation. This is almost entirely redundant with simply applying the patch, but easier to visualize.
Add coverage for all that stuff and fix some bugs with `-M` / `-C` patch generation that weren't caught under the simpler patch generation.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T866
Differential Revision: https://secure.phabricator.com/D3751
Summary: Make this harder to get wrong. Instead of requiring a separate call for synthetic data, automatically load it if we can.
Test Plan: Unit tests; `arc diff`.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T866
Differential Revision: https://secure.phabricator.com/D3750
Summary: This information may be quite useful.
Test Plan: Threw exception from license linter.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3764
Summary:
This code is much more readable to me.
It should also be faster as building the array at once should be faster than one by one.
Test Plan: Made license and XHPAST errors in PHP file, made spelling error in JS file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3758
Summary:
- I caused $parser to be reused in D3732 which I belived was safe, but actually isn't. We end up writing to the same changes. We should make it safe but there's some mess in Phabricator that needs to be cleaned up first.
- One minor error code thing, variable is undefined.
Test Plan: Ran `arc export --git` on a moved file, got a better result. Ran some command which made me hit the other case and didn't get a fatal anymore.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3749
Summary:
Currently, ArcanisSingleLintEngine lints deleted paths and directories. These are sometimes appropriate, but SingleLintEngine is a less-sophisticated linter and should have more safe defaults.
Also fix an error where JSHint reported useless messages on failure.
Test Plan:
Reproduced the problem:
$ git show
commit d71efe2b13770c8861bcd3415c15503fc377339f
Author: epriestley <git@epriestley.com>
Date: Fri Oct 19 12:22:50 2012 -0700
WIP
diff --git a/test.js b/test.js
deleted file mode 100644
index 8bd6648..0000000
--- a/test.js
+++ /dev/null
@@ -1 +0,0 @@
-asdf
$ arc set-config --local lint.engine.single.linter ArcanistJSHintLinter
Set key 'lint.engine.single.linter' = "ArcanistJSHintLinter" in local config (was null).
$ arc lint --engine ArcanistSingleLintEngine --rev HEAD^
Usage Exception: JSHint returned output we can't parse. Check that your JSHint installation.
Output:
Applied the error message fix:
$ arc lint --engine ArcanistSingleLintEngine --rev HEAD^
Usage Exception: JSHint returned unparseable output.
stdout:
stderr:
node.js:181
throw e; // process.nextTick error, or 'error' event on first tick
^
Error: ENOENT, No such file or directory '/INSECURE/repos/git-working-copy/test.js'
at Object.statSync (fs.js:400:18)
at _collect (/usr/lib/node_modules/jshint/lib/hint.js:77:12)
at /usr/lib/node_modules/jshint/lib/hint.js:93:13
at Array.forEach (native)
at Object.hint (/usr/lib/node_modules/jshint/lib/hint.js:92:17)
at Object.interpret (/usr/lib/node_modules/jshint/lib/cli.js:137:21)
at Object.<anonymous> (/usr/lib/node_modules/jshint/bin/hint:2:25)
at Module._compile (module.js:420:26)
at Object..js (module.js:426:10)
at Module.load (module.js:336:31)
Applied the remove paths fix:
$ arc lint --engine ArcanistSingleLintEngine --rev HEAD^
Usage Exception: No paths are lintable.
Reviewers: magazovski, btrahan, vrana
Reviewed By: btrahan
CC: aran, Korvin, vissi
Differential Revision: https://secure.phabricator.com/D3735
Summary:
When some linter throws then we don't print any result.
This is bad in case when the linter threw e.g. because of syntax error in some file which some other linter will tell us about.
Test Plan: Threw from a linter, made lint error in a file, saw error then exception.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3756
Summary:
After a reintegration merge, "Copied From URL" will be different and current approach will result in a wrong path. If the path does not match, just mark it as a new file.
moved the comment before if so lines stay at 80 chars
Test Plan: in trunk, svn merge --reintegrate ^/branches/foo and arc diff - without this change it will say "Copied from es/foo/..."
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3748
Summary: See T1675 for discussion. We currently strip `[abicwo12]/` from Git patches, but the user can provide arbitrary prefixes with `--src-prefix` and `--dst-prefix`, or strip prefixes entirely with `--no-prefix`. In these cases, trust they know what they're doing rather than rejecting the diff.
Test Plan: Added a bunch of tests. We have existing tests for `diff.mnemonicprefix` and normal prefixes.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1675
Differential Revision: https://secure.phabricator.com/D3744
Summary:
This is horrible and git specific, but fixes a case where people are using "arc
patch --nobranch ..." when they're not currently on a branch.
The old code assumed you were on a branch and used getBranchName() to record
this, in order to return to that branch later and cherry-pick the patch.
When not on a branch, and using arc patch --nobranch, this was trying to return
to the branch '(no branch)'.
Now, I detect that we're not on a branch and just record what HEAD is instead.
Test Plan:
Checkout the SHA of master (so I'm on master, but not on a branch) then try to
patch it with a feature diff:
€ git checkout e7a3ec68159d6847372cab5ad913f2f15aa7c249
Warning: you are leaving 1 commit behind, not connected to
any of your branches:
ac1ad39 Updating a-file
If you want to keep them by creating a new branch, this may be a good time
to do so with:
git branch new_branch_name ac1ad392350a51edd10343f12b9713f5e5b3707c
HEAD is now at e7a3ec6... Fix arcconfig
€ arc patch --nobranch D7
Created and checked out branch arcpatch-D7.
OKAY Successfully committed patch.
€ git branch
* (no branch)
feature
haddock
master
€ git log --oneline | head -2
38c0235 Updating a-file
e7a3ec6 Fix arcconfig
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3743
Summary: This also changes how we treat `@generated` and `@nolint` - after this diff, we will still lint their path.
Test Plan:
Created directory `a.php` and symlink `b.php` pointing to directory.
`arc lint` previously printed:
> Requested path `/data/users/jakubv/devtools/arcanist/_.php' is not a file.
Now it prints:
> No lint warnings.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3737
Summary:
We need to run new Arcanist over old code in Perflab.
We also need to run new Arcanist over new code (with already deleted custom `buildAllWorkflows()`).
This will work because old code overwrites `buildAllWorkflows()`.
Test Plan:
$ arc help
$ arc help # after deleting getWorkflowName() from one workflow
Reviewers: edward, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3741
Summary:
Currently, adding a new workflow requires you to override ArcanistConfiguration, which is messy. Instead, just load everything that extends ArcanistBaseWorkflow.
Remove all the rules tying workflow names to class names through arcane incantations.
This has a very small performance cost in that we need to load every Workflow class every time now, but we don't hit __init__ and such anymore and it was pretty negligible on my machine (98ms vs 104ms or something).
Test Plan: Ran "arc help", "arc which", "arc diff", etc.
Reviewers: edward, vrana, btrahan
Reviewed By: edward
CC: aran, zeeg
Differential Revision: https://secure.phabricator.com/D3691
Summary:
This changes what --nobranch does, though not what it means. In git, the patch
is applied to the git commit it is based off where it will commit cleanly, and
if you specify --nobranch then arc-patch will then switch back to your original
branch and try to cherry-pick the commit there.
If this fails, you end up with merge-conflict markers in the file which can be
handy.
Test Plan: https://secure.phabricator.com/P572
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3712
Summary:
We currently pull message from Conduit twice in `arc diff` update.
Once in `buildCommitMessage()` and once in `buildRevisionFromCommitMessage()`.
Remeber that we already pulled it (and so it is authoritative) to save this call.
Even faster solution would be to not pull and update the message at all in common (non-`--edit`, non-`--verbatim` and such) update workflows but it's more involved.
Test Plan:
$ arc diff --trace
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3716
Summary: We want to use them in event.
Test Plan: Will use it in event.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3690
Summary:
I was thinking about creating a method `differential.setdiffproperties` updating all properties at once or adding 'properties' to `differential.creatediff` (better) but it will require bumping Conduit version.
This looks simpler and with similar effect.
We could postpone resolving properties more but I don't want to risk not resolving them after an error.
Test Plan:
This diff for that it works.
Benchmark: 0.55 s before, 0.25 s after (with three properties, the difference will be bigger with more).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3689
Summary:
The 'review its own revision' check is now handled by Differential (bug T1879)
Previous behavior:
arc threw an "You can not be a reviewer for your own revision." exception
if an users adds itself as reviewer, even when this configuration is
allowed on the Differential remote install's configuration.
New behavior:
Arc doesn't check that anymore. It still will be checked by the server.
Test Plan: Tested locally pushing revisions with "arc diff" to a Phabricator server with differential.allow-self-accept at true or false with myself or not as reviewer.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1879
Differential Revision: https://secure.phabricator.com/D3674
Test Plan: A lot of spew before. Not so much now.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3681
Summary:
Hunk may be missing newline at end of file. It produces exports like this:
lang=diff
--- a/third-party
+++ b/third-party
@@ -1 +1 @@
-/mnt/gvfs/third-party/90cb1654197e56261b1733c704b387285f36208e
\ No newline at end of file
+/mnt/gvfs/third-party/7097083d10d37251218531da398545658872a47a
\ No newline at end of filediff --git a/ti/proxygen/TARGETS b/ti/proxygen/TARGETS
Test Plan:
$ arc export --git --diff 1
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, lesliepc16
Differential Revision: https://secure.phabricator.com/D3672
Summary: I use it more often without second parameter than with it.
Test Plan:
Lint of:
preg_quote('');
preg_quote('', '/');
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3647
Summary: cd && cmd won't work so use chdir; -m $multiline_message won't work so use -F $tmp_file
Test Plan: this is actually un-tested at the moment.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1617
Differential Revision: https://secure.phabricator.com/D3592
Summary:
Currently, we run `runDiffSetupBasics()` //after// splitting off background lint and unit tests. However, this means `--base` and any explicit revision name (like "HEAD^") will not be parsed, so the call to `getRelativeCommit()` in order to generate `arc lint --rev XXX` will fail or not work as expected, because it will ignore any arguments.
Instead, parse `--base`, explicit revisions, and other repository API arguments before doing lint and unit tests.
Test Plan:
- Set global config for `base` to `arc:amended, git:branch-unique(origin/master)`.
- Created a commit on master.
- Ran `arc diff HEAD^`.
- Before this change, the command fails with "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." when it attempts to run lint, because the `HEAD^` argument is never parsed. After
- After this change, the command succeeds.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3574
Summary:
We store the message to a scratch file.
But if I need to switch context, commit something else and then go back then I'll lose the message.
Amend the repository commit by it instead.
This also removes the annoying question "Do you want to use this message?".
Test Plan: Made an error in message, verified Git log.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3561
Summary:
Currently, we print `arc tasks` in a console-agnostic and unicode-unaware way, so:
- Tasks with multibyte characters get aligned incorrectly (see T1831); and
- narrow terminals plus long task names results in a broken display.
Test Plan: Ran `arc tasks`. Will post screenshots.
Reviewers: btrahan, vrana, Korvin
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T749, T1831
Differential Revision: https://secure.phabricator.com/D3568
Summary:
See https://github.com/facebook/arcanist/pull/53
- Work correctly for directories with `%` in their name; this breaks under sprintf().
- Search for `src/` -> `tests/` style directories.
- Add coverage for search paths.
Test Plan:
Ran unit tests.
I don't have a working test case for PHPUnit tests, can one of you guys apply this and verify I didn't break your setups?
Reviewers: quard, aurelijus
Reviewed By: aurelijus
CC: aran
Differential Revision: https://secure.phabricator.com/D3558
Summary: `arc patch` currently warns about "This diff is against commit svn+ssh://... but the commit is nowhere in the working copy" in Git SVN repository.
Test Plan:
$ arc patch # in Git SVN repository
$ svn find-rev r121212112 # invalid revision
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3539
Summary: Patch created on Git contains 'unix:filemode' property which is useless in SVN.
Test Plan: Exported a bundle changing a file to executable in Git, applied it in SVN, verified that the file is executable.
Reviewers: epriestley
Reviewed By: epriestley
CC: ptarjan, aran, Korvin, digoangeline
Differential Revision: https://secure.phabricator.com/D3554
Summary:
Running `a.php` from command line doesn't work on Windows, we need to run `php a.php`.
This shouldn't break other OSes.
Test Plan:
$ arc diff --background 1
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3544
Summary:
I am using it for about a month.
It works even in Facebook www.
Test Plan:
$ arc diff --background 1 # hundreds of times
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3495
Summary: $param is null here. it should be $node.
Test Plan: arc lint no longer barfed on me!
Reviewers: vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3540
Summary:
Adds a test which goes through a git repository commit by commit and applies them (in effect) via "arc patch", verifying that the results match the actual commit.
See D3439 for the fixture stuff.
The git repo archive has a couple of trivial commits in it:
$ ../libphutil/scripts/utils/directory_fixture.php src/parser/__tests__/bundle.git.tgz
Spawning an interactive shell. Exit when complete.
sh-3.2$ git log
commit f19fb9fa1385c01b53bdb6d8842dd154e47151ec
Author: epriestley <git@epriestley.com>
Date: Wed Sep 5 14:30:28 2012 -0700
Edit a text file.
commit 228d7be4840313ed805c25c15bba0f7b188af3e6
Author: epriestley <git@epriestley.com>
Date: Wed Sep 5 14:30:11 2012 -0700
Add a text file.
Test Plan: Ran tests.
Reviewers: edward
Reviewed By: edward
CC: aran
Maniphest Tasks: T866
Differential Revision: https://secure.phabricator.com/D3440
Summary:
If the file has no changes (probably because it has been moved or copied) then we fail.
Fixes T1708, fixes T1559.
Test Plan:
$ svn mv a b
$ arc diff --only
$ svn revert a b
$ arc patch --diff # of created diff
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Maniphest Tasks: T1559, T1708
Differential Revision: https://secure.phabricator.com/D3526
Summary:
This problem shows very far away.
One of the symptomps is that the contents of a moved file is displayed as added in Differential but it is not a big deal.
The real trouble happens when you try to `arc patch` this diff.
It tries to both copy the file and to add a new contents (which fails).
Fixes T1709.
Test Plan:
$ git mv a b
$ git commit -m.
$ arc diff --only
$ git reset --hard HEAD^
$ arc patch --diff # of the created diff
$ arc unit src/parser/__tests__
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin, boris, mroch, slawekbiel
Maniphest Tasks: T1709
Differential Revision: https://secure.phabricator.com/D3524
Summary: We don't store old file PHID for binary file moves here.
Test Plan: `arc patch` on revision with binary file moves which was failing earlier.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3521
Summary: We call it from `arcanist.php`.
Test Plan: This diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3496
Summary: We currently insert background parameters to end which causes ignoring them if there is '--'.
Test Plan:
$ arc diff --background 1 -- HEAD^
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3497
Summary: I want to use it from outside.
Test Plan:
$ arc unit src/lint/linter/__tests__/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3493
Summary: I don't offer a replacement because `f() ?: 1` converted to `f() ? f() : 1` can cause side effects and whitespace issues.
Test Plan: New test.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3489
Summary:
`pht()` can be some random function.
Better solution would be to separate this linter to its own class but it would be slower.
Also use `PhutilLintEngine` as `lint.engine`.
Test Plan:
$ arc lint # on file with pht($a)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3480
Summary: Add `ruby -wc` as a linter and raise Error whenever there's a syntax error
Test Plan: Just a few dumb unit tests.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3447
Test Plan:
Wrote "Posible" in the linter, let it change to "Possible".
New unit test.
Reviewers: jack, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3469
Summary: See D3449, comments, unit tests.
Test Plan: Unit tests.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3463
Summary:
I tried to fixed it but I've given up.
See rP958e6cd109f3.
Test Plan:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3449
Summary: This is Arcanist part of D3434.
Test Plan: None.
Reviewers: epriestley, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3450
Git commit messages must have 2 newlines between the subject and body
If used to rewrite a commit message then the commit message will be
incorrectly reformatted.
Summary:
- See D3422.
- Also improve some event configuration/debugging stuff.
Test Plan: Ran `arc list --trace`, set bogus/valid event handlers.
Reviewers: vrana, nh
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3423
Summary:
HPHP now autoloads in `is_subclass_of()`.
I've left the `class_exists()` check as the code is more explicit.
Test Plan:
// under HPHP
function __autoload($c) {
echo "$c\n";
}
is_subclass_of('C', 'stdClass');
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3435
Summary:
This object is highly useful in many client event handlers (particularly for access to the CLI) and allows them to be implemented less intrusively.
This also slightly reduces code duplication.
Test Plan: Ran "arc diff".
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1753
Differential Revision: https://secure.phabricator.com/D3421
Summary:
w00t!
Created new functions getBookmarkName, createBookmark that use mercurial
commands to create a new bookmark and apply the commit message when
running arc patch. Like with git, it checks if the bookmark already
exists. If it does, creates arcpatch-DXXX-1, -2 etc
Updated the mercurial section of run() to include applying the commit
message.
Pretty new to programming in general still, so I wasn't sure if I should
have just modified getBranchName and createBranch to work with Mercurial
(instead of creating new functions). This was easier for me to make sure
I didn't break the git functionality. Let me know I can merge the
functions.
Test Plan:
Tested in my www-hg repository. Probably need to test further (suggestions?)
Ran the following trials with arc patch D539987
1) the bookmark arcpatch-D539987 already existed
2) the bookmark did not exist
3) tried an invalid commit, arc patch R539987
4) --nobookmark flag (bookmark was not created, but the commit applied)
5) --nocommit flag (boomark was created, and the commit was not applied)
6) --nocommit and --nobokmark
Here's the summary of the results:
https://secure.phabricator.com/P493
Reviewers: dschleimer
Reviewed By: dschleimer
CC: aran, epriestley, phleet, bos, csilvers
Differential Revision: https://secure.phabricator.com/D3334
Summary:
Adds an event prior to creation of a new revision so installs can muck around with titles, etc.
I'll also update the docs.
Test Plan: Ran "arc diff --trace" and observed event dispatch. Added `var_dump()` and verified $revision is a reasonable object.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, geoffberger
Differential Revision: https://secure.phabricator.com/D3408
Summary:
We log time of running `arc diff`. It's easy with `--verbatim` or with users just confirming the message built in commit template.
With `--background`, I want to log only waiting time, not message editing time. This event should allow it.
Test Plan:
$ arc diff
Haven't created the listener yet.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3394
Summary:
This is how it looks like now:
> What do you want to name this library? x
> Writing '__phutil_library_init__.php' to 'x/__phutil_library_init__.php'...
> Usage Exception: This library is using libphutil v1, which is no longer supported. Run 'arc liberate --upgrade' to upgrade to v2.
Test Plan:
$ arc liberate # in new dir
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3368
Summary:
We want to have a class of lint problems which are displayed to author and attached to revision but don't require excuse in diff workflow.
Lint advices already serve this purpose but no linters emit them because they need `--advice` flag to be processed.
By always enabling advices, we can switch more linters from warnings to advices and don't stop the diff workflow for them.
This diff also bumps down default severity of TODO rule.
Test Plan: Made lint advice mistake, ran `arc lint`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, ide, Korvin
Differential Revision: https://secure.phabricator.com/D3364
Summary:
- Add zlib functions to extension functions.
- Provide a better error if the extension is actually missing.
Test Plan: Eyeballed it.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D3321
Summary: See D3296#1.
Test Plan:
New test.
Linted Arcanist, libphutil, Phabricator repositories, found no false positive and one real positive.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3297
Summary:
I'm not sure if we are interested in this kind of linters ("Don't use slow function if there is a fast alternative").
I didn't find a case where `strpos() === 0` could be useful except [[ http://www.php.net/manual/en/mbstring.overload.php | mbstring.func_overload ]] craziness.
Test Plan:
New unit test.
Linted Arcanist, libphutil, Phabricator repositories, found no false positive and one real positive.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3296
Summary:
See discussion on T1630.
extraData provides more scope for extensions to piggy-back
more data on the test results and have that pulled up to the UI.
We're using keys like "facebook:complexity" to store additional
data as part of the test results.
Test Plan:
Nothing in the codebase touches extraData at the moment, so
you'll just have to have faith/prove by inspection.
Reviewers: vrana, nh, tuomaspelkonen, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T1630
Differential Revision: https://secure.phabricator.com/D3276
Summary: XHPAST doesn't currently parse most PHP 5.4 stuff, but it does parse this. Warn about it.
Test Plan:
Unit tests, and:
Error (XHP35) Use Of PHP 5.4 Features
The f()[...] syntax was not introduced until PHP 5.4, but this codebase
targets an earlier version of PHP. You can rewrite this expression using
idx().
365 public function lintPHP54Features($root) {
366
367 if (false) {
>>> 368 id()[0];
^
369 }
370
Reviewers: alanh, vrana
Reviewed By: alanh
CC: aran
Differential Revision: https://secure.phabricator.com/D3291
Summary:
I need to run some jobs only if the tests hasn't been skipped.
I know that this could end up by passing more and more data to the event but this is all I need so far.
Test Plan: Dumped `unitResult` from the listener.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3259
Summary: See T1635 and the giant inline comment.
Test Plan: Ran unit tests on 32-bit and 64-bit machines.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1635
Differential Revision: https://secure.phabricator.com/D3250
Summary: See T1635. I'm going to make an effort to rewrite this in a way that's safe in 32-bit PHP, i.e. without bcmath. Add test coverage to limit the chance I screw it up.
Test Plan: Ran unit tests.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1635
Differential Revision: https://secure.phabricator.com/D3249
Summary: See D3252. Reduces code duplication a little bit. Also remove some dire warnings about impending doom -- this has been in use in the wild for a long time.
Test Plan: Added a file in ISO-8859-1, ran `arc diff --encoding ISO-8859-1` to generate this revision, got an encoding note in output.
Reviewers: davidreuss, vrana, btrahan
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D3253
Summary: I will also commit fixes in other repos.
Test Plan: LinkChecker
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3242
Summary: We always do this on --recon now, see D3213.
Test Plan: Ran `arc diff --background 1` to generate this diff.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3241
Summary:
The only purpose of "Arcanist Overview" is to tell people they shouldn't be here, but we bury the lede.
Make it clear that this is not user documentation.
After T988 we can improve the organization here, but some recent users found this pretty confusing.
Test Plan: Generated and read documentation.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3235
Summary: We emit a confusing error if there's no ".arcconfig" in the local right now.
Test Plan:
$ arc patch D3185
This patch is for the 'phabricator' project, but the working copy
belongs to the 'phabricatox' project. Still try to apply the patch? [Y/n]
$ arc patch D3185
This patch is for the 'phabricator' project, but the working copy does
not have an '.arcconfig' file to identify which project it belongs to.
Still try to apply the patch? [Y/n]
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3231
Summary:
We shove alias parameters onto the front of the arg list so if you make an alias like "qdiff" = "diff x y z" and then run "qdiff a b c", we end up with "diff x y z a b c". However, currently we reverse alias parameters, so you actually get "diff z y x a b c".
This is a problem for `arc alias bdiff -- diff --background 1`, which evaluates to `arc diff 1 --background` and fails.
Test Plan: Created a `bdiff` alias and ran it successfully.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3196
Summary: Depends on D2614.
Test Plan:
Updated a diff with no lint errors.
Updated a diff with lint errors, verified that the previous message is not lost.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3174
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: 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
