1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-12-05 13:16:13 +01:00
Commit graph

1056 commits

Author SHA1 Message Date
vrana
21530fa459 Change severity of PEP8 errors to warnings
Summary: None of these are that serious that I would like to be informed about them on unmodified lines.

Test Plan: Linted Python file with lots of PEP8 errors, now warnings.

Reviewers: zeeg, epriestley

Reviewed By: zeeg

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3884
2012-11-05 13:05:13 -08:00
vrana
66d204be81 Delete license headers from files
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
2012-11-05 11:16:24 -08:00
vrana
d4a97d87c8 Ask for explanation for skipping lint and unit
Summary: Fixes T2023.

Test Plan:
  $ arc diff --nounit # Abort
  $ arc diff --nounit
  $ arc diff --nounit --excuse 'Move fast and break things.'

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2023

Differential Revision: https://secure.phabricator.com/D3872
2012-11-03 22:34:12 -07:00
vrana
317cf6fc36 Specify config for text editors
Summary: This is sort of documentation.

Test Plan:
Used the plugin for Notepad++ with default indent of 4 spaces.
Opened file from `src/`, verified that it indents 2 spaces.
Opened file from `externals/`, verified that it indents 4 spaces.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3865
2012-11-03 00:13:53 -07:00
vrana
851e579e94 Declare undeclared variable in lint engine
Test Plan:
  $ arc lint

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3876
2012-11-02 16:53:12 -07:00
Bob Trahan
027483f29a modify arc diff workflow to only do amend optimization if git repository
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
2012-11-01 16:47:08 -07:00
vrana
5c4d2ae765 Fix copy/paste error in comment 2012-11-01 16:21:12 -07:00
vrana
90417fbda8 Advice user to set up stripping trailing white space in editor
Test Plan: Triggered it.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2022

Differential Revision: https://secure.phabricator.com/D3864
2012-11-01 11:23:31 -07:00
epriestley
24405e0e86 Fix arc diff --raw
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
2012-10-31 11:50:43 -07:00
vrana
475a576fdc Require defining getWorkflowName()
Test Plan:
  $ arc help

Reviewers: epriestley, edward

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3742
2012-10-31 10:52:26 -07:00
Bob Trahan
522c35c1ed escape files properly in arc commit
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
2012-10-30 18:26:58 -07:00
epriestley
44842aeb25 Don't try to run didAbortWorkflow() if the user never provided a command
Summary:
If you type `arc`, you currently get a fatal since $config isn't defined. Don't try to run the hook if we never built a config.

(We could build the config earlier and then run the hook anyway, but $workflow and $command would not be defined. It seems unlikely that executing the hook here is useful, since it affects only the case that the user types `arc` on its own.)

Test Plan: Typed `arc`. Typed `arc diff`, etc.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D3841
2012-10-30 10:44:49 -07:00
epriestley
d96eaed097 Partially revert D3748
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
2012-10-30 07:06:05 -07:00
epriestley
519e23f3a4 In arc diff, always check that you own a revision before trying to do anything with it
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
2012-10-25 16:27:57 -07:00
epriestley
6be5cfd104 Split paths on "diff --git" lines correctly in more cases
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
2012-10-25 14:46:44 -07:00
epriestley
b8ead97637 Minor, fix obvious method name typo.
Test Plan: Ran `arc export --git`.

Auditors: btrahan
2012-10-25 13:38:23 -07:00
vrana
fa75a03b72 Fix didAbortWorkflow() without workflow
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
2012-10-24 14:09:02 -07:00
vrana
2d226f3110 Disallow using DD1 as revision ID
Test Plan:
  $ arc inlines --revision 1
  $ arc inlines --revision D1
  $ arc inlines --revision DD1

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3811
2012-10-24 11:33:12 -07:00
vrana
448e820eb6 Run unit on background also in waiting for confirmation with arc diff --excuse
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
2012-10-23 15:26:23 -07:00
vrana
0b02170723 Ask for lint and unit excuses asynchronously
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
2012-10-22 16:25:35 -07:00
vrana
1b51b74135 Provide a hook for aborted workflow
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
2012-10-22 12:55:39 -07:00
vrana
47a036a535 Fix comprehensive lint engine 2012-10-22 12:47:27 -07:00
epriestley
538115bd0e Use PhutilAggregateException to aggregate unit test exceptions
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
2012-10-22 11:41:33 -07:00
epriestley
10653d7ff3 Fix some regexp issues for parsing SVN diffs with \r\n newlines
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
2012-10-22 11:15:36 -07:00
epriestley
6114d7cf9c Add a large number of move/copy diff/patch tests
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
2012-10-20 08:43:31 -07:00
epriestley
4b03a3fa0d Improve test coverage of diff/patch generation
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
2012-10-20 08:43:13 -07:00
epriestley
5981aa1df4 Make ArcanistDiffParser automatically load synthetic changes if the working copy is available
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
2012-10-20 08:42:16 -07:00
vrana
835f2afb5d Pass key to aggregate exception
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
2012-10-20 07:33:56 -07:00
vrana
77af6fb35d Set all lint paths at once
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
2012-10-20 06:49:54 -07:00
vrana
262423b5a8 Fix lint tests after D3756
Test Plan:
Threw normal exception, saw failed test.
Threw usage exception, saw skipped test.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3761
2012-10-20 06:19:00 -07:00
epriestley
7cb3551dc7 Fix two arc issues
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
2012-10-20 06:13:12 -07:00
epriestley
3853483c64 Fix ArcanistSingleLintEngine for deleted paths
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
2012-10-20 06:12:24 -07:00
vrana
d09eb881a3 Print lint results before throwing
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
2012-10-20 05:03:22 -07:00
Svemir Brkic
134e9e5b4d Treat a file merged from another svn branch as an addition
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
2012-10-19 15:55:42 -07:00
epriestley
f1bca1b7cb Make parser more flexible in the face of Git diff prefix flags
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
2012-10-19 14:07:34 -07:00
vrana
4fb2b48d06 Don't unset linted paths from subdirectory and in commit hook mode
Summary: D3737#6

Test Plan:
  phabricator/src/ $ arc lint __phutil_library_init__.php

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3746
2012-10-19 12:57:46 -07:00
Edward Speyer
f0c0245957 Support --nobranch when not currently on a branch
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
2012-10-19 12:12:57 -07:00
vrana
4585ab7363 Don't lint file content of directories
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
2012-10-19 11:38:44 -07:00
vrana
b52ee2fdee Temporarily unrequire defining getWorkflowName()
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
2012-10-19 11:02:20 -07:00
vrana
94d283a453 Require defining workflows' synopses and help
Test Plan: Deleted `getCommandSynopses()` from workflow, ran `arc help`.

Reviewers: epriestley, edward

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3739
2012-10-19 11:01:57 -07:00
epriestley
ffac8d5c4c Improve arc handling of various binary file operations in Git
Summary:
See T866, D3521. Additional things this fixes:

  - `arc export` now exports binary data correctly.
  - `ArcanistBundle` unit tests now load and apply binary data correctly.
  - `arc patch` no longer relies on `base` configuration.
  - Adds tests to the tarball:

  commit df340e88d8aba12e8f2b8827f01f0cd9f35eb758
  Author: epriestley <git@epriestley.com>
  Date:   Wed Oct 17 15:46:11 2012 -0700

      Remove binary image.

  commit 3f5c6d735e64c25a04f83be48ef184b25b5282f0
  Author: epriestley <git@epriestley.com>
  Date:   Wed Oct 17 15:45:58 2012 -0700

      Copy binary image.

  commit b454edb3bb29890ee5b3af5ef66ce6a24d15d882
  Author: epriestley <git@epriestley.com>
  Date:   Wed Oct 17 15:45:35 2012 -0700

      Move binary image.

  commit 5de5f3dfda1b7db2eb054e57699f05aaf1f4483e
  Author: epriestley <git@epriestley.com>
  Date:   Wed Oct 17 15:45:09 2012 -0700

      Add a binary image.

Test Plan: Ran unit tests, `arc patch`, `arc export`, `arc diff`, `arc upgrade`.

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T866

Differential Revision: https://secure.phabricator.com/D3732
2012-10-19 10:10:25 -07:00
epriestley
4c476c0201 Make Arcanist workflow names explicit
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
2012-10-17 08:35:03 -07:00
Edward Speyer
2d269aefed Use git-cherry-pick for arc patch
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
2012-10-16 16:46:21 -07:00
vrana
a850f1c6ef Don't pull message twice from Conduit in arc diff
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
2012-10-16 12:53:59 -07:00
vrana
485660831e Fix typo in comment 2012-10-16 10:01:38 -07:00
Stanislav Vorobyev
9c2348f2f4 Fix parsing of PHPUnit JSON log format for PHPUnit 3.7.7
Summary: See https://github.com/facebook/arcanist/pull/55

Reviewed by: epriestley
2012-10-15 11:07:50 -07:00
vrana
eb5bdb03d0 Provide getter for test results properties in arc diff
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
2012-10-12 18:15:27 -07:00
vrana
3d00e0448d Resolve diff property updates in parallel
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
2012-10-12 15:18:56 -07:00
Dereckson
646b37a343 (T1889) arc cover supports Mercurial too
Summary: improving documentation: add cover supports Mercurial

Test Plan: ./bin/arc help cover

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T1889

Differential Revision: https://secure.phabricator.com/D3683
2012-10-12 07:41:24 -07:00
Dereckson
5725585f2d Allow to push revisions to review by himself to Differential (bug T1879, change 2/2, Arcanist part)
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
2012-10-12 07:40:59 -07:00