Summary: Maybe I will need it on other places.
Test Plan:
$ arc lint --output json # on file with lint error
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3898
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in unit tests and LICENSE file.
Reviewers: epriestley, btrahan, edward
Reviewed By: epriestley
CC: aran, Korvin, davidrecordon
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3881
Summary: fix for T2011, first option in list of possible fixes
Test Plan: ...do I really have to setup mercurial with mq? :D
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2011
Differential Revision: https://secure.phabricator.com/D3869
Summary:
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
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: 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:
- 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, 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:
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
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:
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:
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 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: 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
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:
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:
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:
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:
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 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: 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: 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: 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
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:
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 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:
- 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
