Summary: Allow users to run a command to determine the base revision of the commit range.
Test Plan: Ran stuff like `arc which --show-base --base 'arc:exec(ls)'` and got the expected results.
Reviewers: dschleimer, csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2830
Summary: We will extract them at some point, lint before it's too late.
Test Plan:
New test.
Linted all callsites.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2825
Summary: Allow the default to be set in global config, not just project config.
Test Plan: `arc set-config arc.land.onto.default derp; arc land`
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2686
Test Plan: Show Full Unit Results on this diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2777
Summary:
New optional mode. If you set 'base' in local, project or global config or pass '--base' to 'arc diff' or 'arc which', it switches to DSL mode.
In DSL mode, lists of rules from args, local, project and global config are resolved, in that order. Rules can manipulate the rule machine or resolve into actual commits. Provides support for some 'arc' rules (mostly machine manipulation) and 'git' rules (symbolic ref and merge-base).
Test Plan:
Ran unit tests. Also:
```$ arc which --show-base --base 'arc:prompt'
Against which commit? HEAD
HEAD
$ arc which --show-base --base 'git:HEAD'
HEAD
$ arc which --show-base --base 'git:fake'
Usage Exception: None of the rules in your 'base' configuration matched a valid commit. Adjust rules or specify which commit you want to use explicitly.
$ arc which --show-base --base 'git:origin/master'
origin/master
$ arc which --show-base --base 'git:upstream'
Usage Exception: None of the rules in your 'base' configuration matched a valid commit. Adjust rules or specify which commit you want to use explicitly.
$ arc which --show-base --base 'literal:derp'
derp
$ arc which --show-base --base 'arc:halt'
Usage Exception: None of the rules in your 'base' configuration matched a valid commit. Adjust rules or specify which commit you want to use explicitly.
$ arc set-config --local base git:origin/master
Set key 'base' = 'git:origin/master' in local config.
$ arc which --show-base
origin/master
$ arc which --show-base --base 'git:HEAD^'
HEAD^
$ arc which --show-base --base 'arc:yield, git:HEAD^'
origin/master
$ arc which --show-base --base 'arc:global, git:HEAD^'
HEAD^
$ arc which --show-base --base 'arc:global, git:merge-base(origin/master)'
3f4f8992fba8d1f142974da36a82bae900e247c0```
Reviewers: dschleimer, vrana
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2748
Summary: Otherwise we'll get a cached result from fileperms() if we end up here again.
Test Plan: `chmod 644 ~/.arcrc ; arc help` no longer double prompts when run from outside of a .arcconfig working copy.
Reviewers: csilvers
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1359
Differential Revision: https://secure.phabricator.com/D2752
Summary:
We currently use the language "relative commit" or "relative local commit" to refer to the head of a commit range. I want to move toward callign this a "base commit", since I think that makes more sense. This moves us a small step in that direction.
The DSL stuff in T1233 also needs access to this stuff, so move it up to the base. This is mostly a bite-sized piece of the change in that diff.
Test Plan: Ran "arc which" in a couple of circumstances, read explanations.
Reviewers: dschleimer, vrana
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2747
Summary:
This adds the concept of a local (i.e. working copy local) config file
to arc. This config file has the highest precedence for config
settings that may come from any config file. The config is stored in
.(git|hg|sv)/arc/config, and is read ahead of the project config by
getConfigFromAnySource().
Test Plan:
#Testing arc set-config and arc get-config
[16:57:04 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19410 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
[16:57:12 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19410 $ ./bin/arc set-config foo bar
Set key 'foo' = 'bar' in global config.
[16:57:23 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19411 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
(global) foo = bar
[16:57:26 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19411 $ ./bin/arc set-config --local foo baz
Set key 'foo' = 'baz' in local config.
[16:57:35 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19412 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
(global) foo = bar
(local) foo = baz
[16:57:39 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19412 $ ./bin/arc set-config foo ''
Deleted key 'foo' from global config (was 'bar').
[16:57:49 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19413 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
(global) foo =
(local) foo = baz
[16:57:51 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19413 $ ./bin/arc set-config --local foo ''
Deleted key 'foo' from local config (was 'baz').
[16:58:05 Tue Jun 12 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19414 $ ./bin/arc get-config
(global) default = https://phabricator.fb.com/conduit
#testing getConfigFromAnySource by means of lint
[11:26:57 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19612 $ ./bin/arc set-config lint.engine BogusLintEngine
Set key 'lint.engine' = 'BogusLintEngine' in global config.
[11:30:04 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19613 $ ./bin/arc set-config --local lint.engine DummyLintEngine
Set key 'lint.engine' = 'DummyLintEngine' in local config.
[11:30:19 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19587 $ ./bin/arc lint
Exception
Failed to load symbol 'DummyLintEngine'. If this symbol was recently added or moved, your library map may be out of date. You can rebuild the map by running 'arc liberate'. For more information, see: http://www.phabricator.com/docs/phabricator/article/libphutil_Libraries_User_Guide.html
(Run with --trace for a full exception trace.)
[11:30:25 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19586 $ ./bin/arc set-config --local lint.engine ''
Deleted key 'lint.engine' from local config (was 'DummyLintEngine').
[11:30:34 Wed Jun 13 2012] dschleimer@dev4022.snc6 ~/devtools/arcanist
arcanist local_config 19587 $ ./bin/arc lint
OKAY No lint warnings.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2739
Summary:
We currently remove an invalid user from Reviewers and similar fields and print a note in the template. There are several problems:
# If I only made a typo in the name then it takes some effort to fix this typo and put the name on the original place (this information is lost).
# The notice is almost invisible and most users will just confirm the message without noticing it.
# If I fill the data in the commit message (and not in the template) then I probably want to treat that as authoritative so I want it to be correct.
Test Plan:
`arc diff` with invalid reviewer in commit message.
`arc diff` on empty commit message.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, nh
Differential Revision: https://secure.phabricator.com/D2730
Summary:
Using .arc as the scratch and per-repository configuration directory
has some unfortunate consequenses in the real world. Among other
things, people forget to .gitignore it so it gets checked in.
Test Plan:
the only thing that seems to use this is the relative commit setting
for git. This diff consists of 2 commits, one for the .gitignore and
one for everything else.
Comment out the portion of my .git/config that defines the upstream
for the branch. Run arc diff --only with HEAD^ in
.arc/default-relative-commit. See that .gitignore is not included in
the resultant diff, that .arc no longer exists, and that .git/arc
exists and has HEAD^ in .git/arc/default-relative-commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1233
Differential Revision: https://secure.phabricator.com/D2725
Summary:
##arc amend## in a mercurial repo was failing with this message
Usage Exception: Commit 'HEAD^' is not a valid Mercurial commit identifier.
mercurial's .^ is about the same thing as git's HEAD^
Test Plan:
##arc amend## in a mercurial repo with ##"immutable_history" : "false"## in the
##.arcconfig##.
Reviewers: epriestley
Reviewed By: epriestley
CC: vrana, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2690
Summary:
I just hope that we will not create `ArcanistPhutilTestCaseTestCaseTestCase`.
Also fix a copy/paste error (@edward's this time).
Test Plan: `arc unit`
Reviewers: epriestley
Reviewed By: epriestley
CC: edward, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2692
Summary: Almost revert D2673 but leave the support for 'repeat'.
Test Plan: `arc help unit`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2689
Summary:
This need a bunch of work, but is a sort of minimum viable product for the hgdaemon.
The two ProtocolChannels implement the client and server halves of the protocol.
The Server implements the server logic, the Client implements the client logic.
Test Plan: Launched a server and client on the same repo, got hg output in ~2-3ms instead of 100ms+.
Reviewers: csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2665
Summary:
We run all tests before deploy but some of them are expected to fail.
We need to skip them.
It can be useful also for someone else.
I don't propagate this to `arc diff` as that would be much more complicated - we would need a new state 'partially skipped'.
The 'path' argument needs to be a file, skipping whole dirs is not supported.
The 'path' argument is in fact engine specific and engines can support format like 'Class::testMethod' but I didn't bother to document it to not confuse people.
Test Plan:
`arc unit --skip test.php`
`arc unit --skip ../src/test.php`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2673
Summary:
This adds basic support for mutable history for arc diff and arc amend for
mercurial. This is purely opt-in (so it shouldn't affect anyone who doesn't want
this feature) by explicitly setting
"immutable_history" : false
in arc configuration.
This also fixes another instance of weird behaviour for multiple heads - the
first instance was fixed here:
https://github.com/facebook/arcanist/pull/28
Test Plan:
without "immutable_history" turned on)>
When ##arc diff## produces an update diff, it should list only commits that are
ancestors of the current revision - not ones from other heads.
Reviewers: epriestley
CC: csilvers, aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2654
Summary:
We added "repeat" to PhutilArgumentParser a while ago, so we can modernize the rest of the argument parsing (allow multiple --load-phutil-library flags).
Add a "--conduit-timeout" flag to allow users to set the timeout. See D2602.
Test Plan:
$ arc diff --conduit-timeout=0.01
Exception
[cURL/28] <CURLE_OPERATION_TIMEOUTED> The request took too long to complete.
(Run with --trace for a full exception trace.)
$ arc diff --conduit-version 33
Exception
ERR-BAD-VERSION: Your 'arc' client version is '33', which is newer than the server version, '5'. Upgrade your Phabricator install.
(Run with --trace for a full exception trace.)
$ arc diff --conduit-uri http://derp.derp/
Usage Exception: YOU NEED TO INSTALL A CERTIFICATE TO LOGIN TO PHABRICATOR
You are trying to connect to 'http://derp.derp/api/' but do not have a certificate installed for this host. Run:
$ arc install-certificate
...to install one.
$ arc diff --load-phutil-library src --load-phutil-library ../phabricator/src --trace
Loading phutil library '0' from '/INSECURE/devtools/arcanist/src'...
Loading phutil library '1' from '/INSECURE/devtools/phabricator/src'...
Reviewers: phleet, vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2603
Summary:
It makes me nervous.
Also move them to the dir.
Test Plan:
`arc lint`
`arc lint --output json`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2644
Summary: We don't support `git rebase -i` or other magic in `arc amend` so we can examine just HEAD which we always amend.
Test Plan: `arc amend` on a branch with two commits.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2643
Summary:
If a message has only "line", or "line" and "char", we don't point out the location in the context block.
When a message includes "line", show chevrons on the line.
When a message includes "line" and "char", show chevrons on the line and a caret on the next line.
Test Plan: Ran regex linters to capture line, line+char, and normal linters to capture everything. Output appeared sane.
Reviewers: csilvers, vrana, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2642
Summary:
`$this->getPaths()` gives us paths relative to repository root.
We resolve them relative to cwd.
Test Plan: [src] `arc unit difference`
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2634
Summary:
The theory is that lint and unit errors are rare with no false positives.
Our reality is currently different.
This diff removes the question for providing explanation before actually providing it.
I hope that the wording is not confusing.
It also simplifies the code with less copy/paste errors (not me!) potential.
Test Plan:
`arc diff` on code with lint and unit errors.
Repeat with `--excuse .`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, meitros
Differential Revision: https://secure.phabricator.com/D2631
Summary: I wonder if this is not by purpose?
Test Plan: Modified two files, ran `arc lint`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2629
Summary:
Khan Academy is looking into lint configuration, but doesn't use ".arcconfig" because they have a large number of repositories. Making configuration more flexible generally gives us more options for onboarding installs.
- Currently, only project config (".arcconfig") can load libraries. Allow user config ("~/.arcrc") to load libraries as well.
- Currently, only project config can set lint/unit engines. Allow user config to set default lint/unit engines.
- Add some type checking to "arc set-config".
- Add "arc set-config --show".
Test Plan:
- **load**
- Ran `arc set-config load xxx`, got error about format.
- Ran `arc set-config load ["apple"]`, got warning on running 'arc' commands (no such library) but was able to run 'arc set-config' again to clear it.
- Ran `arc set-config load ["/path/to/a/lib/src/"]`, worked.
- Ran `arc list --trace`, verified my library loaded in addition to `.arcconfig` libraries.
- Ran `arc list --load-phutil-library=xxx --trace`, verified only that library loaded.
- Ran `arc list --trace --load-phutil-library=apple --trace`, got hard error about bad library.
- Set `.arcconfig` to point at a bad library, verified hard error.
- **lint.engine** / **unit.engine**
- Removed lint engine from `.arcconfig`, ran "arc lint", got a run with specified engine.
- Removed unit engine from `.arcconfig`, ran "arc unit", got a run with specified engine.
- **--show**
- Ran `arc set-config --show`.
- **misc**
- Ran `arc get-config`.
Reviewers: csilvers, btrahan, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2618
Summary:
Move away from setModule(), to setPathPrefix(). Also simplify test location/selection.
Depends on D2621.
Test Plan: Ran "arc unit".
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2622
Summary:
- The library linter works fine on v1 or v2 libraries, so switch to it for all libraries.
- "arc liberate" on v1 libraries will still invoke the Module linter, and thus generate __init__.php files.
Test Plan: Ran "arc lint". Grepped for module linter.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2620
Summary: Mechanical changes from D2588. No "Class.php" moves yet because they aren't necessary for libraries to function.
Test Plan: See D2588.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2589
Summary:
- "arc liberate" now works for v1 or v2 libraries.
- "arc liberate --upgrade" converts a v1 to a v2.
- We delete __init__.php files but do not move Class.php files, since this is vastly simpler. Authors can do this on their own if they want. We'll do it separately.
- v2 has less lint support than v1, but I think we can move first and migrate lint support later. Much of the v1 lint is bad anyway.
Test Plan: Upgraded libphutil, arcanist and phabricator to v2
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2588
Summary: We recover from Conduit exceptions, but not from transport exceptions or other general classes of error here. Recover from everything, and add an explicit flag to skip uploads.
Test Plan: Added a "throw", created a diff, skiped upload. Removed throw, created a diff with --skip-binaries. Created a diff normally.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2598
Summary:
Adds a linter for v2 libraries which raises the relevant errors.
NOTE: Not hooked up anywhere yet, so this diff has no effect.
Test Plan:
Switched the ModuleLinter to LibraryLinter and ran it with a junk block to trigger errors:
>>> Lint for src/lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php:
Error (PHL3) One Class Per File
File 'lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php' mixes
function (id) and class/interface (ArcanistPhutilLibraryLinter)
definitions in the same file. A file which declares a class or an
interface MUST declare nothing else.
190 }
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Error (PHL2) Duplicate Symbol
Definition of function 'id' in
'lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php' in library
'arcanist' duplicates prior definition in 'utils/utils.php' in library
'phutil'.
190 }
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Error (PHL1) Unknown Symbol
Use of unknown class 'XYZ'. This symbol is not defined in any loaded
libphutil library.
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2597
Summary:
We have a linter that needs to read data from the repo that isn't in the commit,
so that data isn't part of the svn transaction, thus not available in svnlook
cat --transaction. This change provides a method to get data from a committed
file.
Test Plan:
Successfully ran arc svn-hook-pre-commit with a linter that needs something
not in the transaction.
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2582
Summary:
One line is usually more than enough for these excuses.
The excuse is quite often the same as in past so history is very useful.
Prompting user right below the errors is better than writing them below prompt.
Test Plan:
Produce a lint error.
Provide an empty explanation.
Provide a non-empty explanation.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2580
Summary: I am trying to stop using `--verbatim` and miss this information.
Test Plan: This diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2575
Summary:
`arc diff` adds a space after each field in template.
It is later removed for the last field.
Test Plan: This diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2564
Summary:
Simple wrapper for PHP_CodeSniffer.
You need to have PHP_CodeSniffer and it's dependencies installed.
Test Plan: - Try running it with your custom lint engine
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2560
Summary: On windows paths are separated with \.
Test Plan: - Run filename linter on windows (or some path with \)
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2559
Summary: Git supports this feature (https://git.wiki.kernel.org/index.php/Aliases), it's fairly simple to implement, and gives us a little more ammunition to dissuade users with unusual workflow requirements from actually requesting features.
Test Plan: Defined the alias described in the documentation. Ran "arc ls", "arc ls -alh", etc.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2552
Summary:
there are two problems making the ArcanistSvnHookPreCommitWorkflow not working.
- pathExists($path) will always return false because it hasn't loaded the path yet. Because of this, PhutilLintEngine will unset the path at https://secure.phabricator.com/diffusion/ARC/browse/master/src/lint/engine/phutil/PhutilLintEngine.php;032b9b30b0721c3f$46, so ArcanistSvnHookPreCommitWorkflow will have no path to lint against and never detects a problem.
- In ArcanistSubversionHookAPI::getCurrentFileData(), the $path is already the full path in svnroot (path got from https://secure.phabricator.com/diffusion/ARC/browse/master/src/workflow/svn-hook-pre-commit/ArcanistSvnHookPreCommitWorkflow.php;032b9b30b0721c3f$72). Prepending the project root to it will make the file path to be wrong so that the file content is empty. Again, ArcanistSvnHookPreCommitWorkflow never detects a problem.
Test Plan:
changed --transaction to --revision in related code and manually ran the following command. It detected the syntaxt error correctly. 558937 is a revision with syntax error.
/usr/local/bin/php -f /home/jungejason/nfs_devtools/arcanist/bin/arc svn-hook-pre-commit --load-phutil-library=/tmp/testwww/www/lib/arcanist --load-phutil-library=/home/jungejason/nfs_devtools/facebook/src /svnroot 558937 2>&1
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, Girish, Koolvin
Differential Revision: https://secure.phabricator.com/D2485
Summary:
It's easy to forget to do this when writing a new lint engine (like I recently
just did), so I added it to the example to improve documentation on how to write
a lint engine. This code is copied from PhutilLintEngine (as well as many
others).
Test Plan: php -l
Reviewers: jungejason, vrana, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2553
Summary: If you develop in "master" and try to "arc land", you get an error. You can reasonably "arc amend" in this situation, most of the time. Give the user an explicit pointer.
Test Plan:
$ arc land master
Usage Exception: You can not land a branch onto itself -- you are trying to
land 'master' onto 'master'. For more information on how to push changes, see
'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the
documentation. You may be able to 'arc amend' instead.
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2547
Summary:
More & more use cases come up with empty suite names, guessing
and making test names nice is mess. Will leave it as it is, except
data set part, as it could be super long.
Test Plan: - Try running some phpunit tests with & without data sets
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2544
Summary: Request from @csilvers, whose team is alergic to $EDITOR.
Test Plan:
Adding reviewers and CCs to this diff via CLI. The initial commit message for this diff is:
Add "--reviewers" and "--ccs" flags to arc diff
Request from @csilvers, whose team is alergic to $EDITOR.
Tested: Adding reviewers and CCs to this diff via CLI. The initial commit message for this diff is:
(...infinite recursion omitted...)
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2538
Summary:
- When you run "arc diff" in Mercurial, we currently give you an empty template. Instead, prefill a likely template by parsing messages, as we do for Git.
- Unify Git and Mercurial logic for acquiring messages, since `getLocalCommitInformation()` now provides this information. This should improve Git performance, and allows us to delete some code.
- Merge messages more intelligently. Currently, we just overwrite fields. Instead, merge arrays (like ccs, reviewers, tasks) and concatenate strings (like summary and test plan). We need to special case "title", see inline.
- @csilvers: this is a precursor to implementing "--reviewers", etc.
Test Plan: See next post, test plan includes giant output.
Reviewers: btrahan, csilvers, Makinde, jungejason, vrana
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2536
Summary:
Better test name handling for tests with data sets.
Instead of showing test name with full data set, show it's id/name,
e.g. `testConstructor with data set #1`
Test Plan: - Try running tests with data sets
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2535
Summary:
PHPUnit wrapper for arc.
The idea here is simple - find the test case which is related to the updated
class file. Generate tmp files for json & clover reports, run phpunit
with provided arguments.
It supports phpunit configuration file setting in `.arcconfig`: `phpunit_config`.
Path should be relative to project root.
Test Plan:
- Set `unit_engine` to `PhpunitTestEngine`
- Try running tests with & without `phpunit_config` option.
Reviewers: epriestley, davidreuss
Reviewed By: epriestley
CC: aran, Koolvin, jungejason
Differential Revision: https://secure.phabricator.com/D2472
Summary:
Several related changes:
- Add a "--conduit-version" flag, so you can actually diff conduit version bumps. Otherwise, the server rejects you.
- Make "arc upgrade" upgrade both libphutil and arcanist, not just arcanist.
- Bump the version number to 5. See D2527.
Test Plan:
- Ran "arc upgrade".
- Ran "arc diff". Got told there was a version issue.
- Ran "arc diff --conduit-version=4" to create this diff.
Reviewers: indiefan, nh, vrana, btrahan, jungejason, Makinde
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2528
Summary:
Hive has custom integration which relies on prefilling a bunch of information from JIRA. This is currently broken and accomplished in a roundabout way. Instead, add an event that the integration can listen for. See next diff.
@Girish, I guess you're official Phabricator/Hive support now?
Test Plan: Ran the Hive/JIRA workflows in Git and SVN, see next diff.
Reviewers: Girish, btrahan, ashutoshc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1206
Differential Revision: https://secure.phabricator.com/D2449
Summary:
There are different options how to implement this:
We can also generate the warning in `validateField()` and handle it in all callsites.
This is sufficient for me and simple enough.
Test Plan: `arc diff` revision with reviewer away/not away.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D2493
Summary:
If you forget to provide an argument for --update and have another argument
following it (e.g. HEAD^), we should provide a nice error message instead
of passing that argument through to a conduit call and then printing the
conduit error.
Test Plan: ran 'arc diff --update HEAD^' and got a nice error message
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2468
Summary:
- "git log" still includes "\n", so we're currently generating nonsense hashes like "\nabd9879bab86ad78ab...".
- Correct the log range we use in Git. See comment. When users perform merges, their expectations about what the "included commits" and what the included changes are are different. Represent them with two different ranges.
- Same deal for Mercurial
Test Plan: Ran "arc which" in various contexts.
Reviewers: btrahan, aurelijus, Makinde
Reviewed By: Makinde
CC: aran, nh, jungejason
Maniphest Tasks: T873
Differential Revision: https://secure.phabricator.com/D2460
Summary: Provide far more information about what "arc diff" intends to do.
Test Plan: Ran "arc which" in a variety of circumstances.
Reviewers: btrahan, Makinde
Reviewed By: Makinde
CC: aran
Maniphest Tasks: T1183
Differential Revision: https://secure.phabricator.com/D2454
Summary:
When getting an encoding, we should query the server for the encoding of the
project that we're exporting from, not the project that we're running arc in
(arc might not be in a working copy).
Test Plan: ran arc export with --diff and didn't get a workflow exception
Reviewers: epriestley, jungejason, vrana, davidreuss
Reviewed By: jungejason
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2455
Summary:
- Allow users to delete the remote copy of a feature branch as well as the local one, for workflows that push feature branches. We test if the remote exists before trying to delete it.
- Raise a better warning when you misuse "arc land".
- I also wrote some documentation about this, see next diff.
Test Plan:
- Tried to land a branch onto itself.
- Ran "arc land --delete-remote" on feature branches with and without remote feature branches.
Reviewers: aurelijus, btrahan
Reviewed By: aurelijus
CC: aran
Maniphest Tasks: T1204
Differential Revision: https://secure.phabricator.com/D2445
Summary: I renamed this in D2437 for greater consistency with everything else, but missed this use of the old key.
Test Plan: idk lmk?
Reviewers: Makinde
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2452
Summary: Currently, we ship only the summary, but we need to ship the whole thing for T1189.
Test Plan: Added var_dump() + die, ran in git and hg working copies, verified 'message' included the whole message.
Reviewers: csilvers, btrahan, Makinde
Reviewed By: csilvers
CC: aran
Maniphest Tasks: T1189
Differential Revision: https://secure.phabricator.com/D2437
Summary:
- Try to limit the pain of //future// version bumps by making arc self-updating.
- When the server needs a newer version, prompt the user to update.
- (We need them to reissue their command because we may already have loaded classes which have changed in the update.)
- Make the message sound exciting!
Test Plan: Artifically bumped server forward, ran "arc list", got to upgrade!
Reviewers: Makinde, nh, jungejason, btrahan
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D2435
Summary:
ArcanistDiffWorkflow::getGitParentLogInfo() calls
ArcanistDifferentialCommitMessage::newFromRawCorpus() which may throw a
ArcanistUsageException if the parent commit message is malformed (specifically,
a bad "Differential Revision:" line); this should not stop arc diff.
Test Plan: successfully ran arc diff where the parent commit message was malformed.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2434
Summary:
There's no reason these should be exclusive: -m is used only on update for the
update message, and --verbatim doesn't affect the udpate message. It's also
useful to allow both of these, if say I want to update my test plan by editing
my git commit message and also provide a message for the update from the
command line.
Test Plan:
ran arc diff with --verbatim and -m and saw my message from -m was used, as
well as my updates in the commit message went through.
Reviewers: jungejason, epriestley, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2426
Summary:
This is mostly an onboarding thing, but also allows "arc upload", "arc download", and "arc paste" to work anywhere on the system.
- Try to read the Phabricator install URI from arc global config if we can't find ".arcconfig".
- Build a WorkingCopy anyway if we can't find ".arcconfig", as long as we can find ".svn", ".git", or ".hg".
- Make all the workflows handle "no project ID" at least somewhat gracefully.
Test Plan:
- Ran "arc diff" in .arcconfig-less Mercurial, Git, and Subversion working copies.
- Ran "arc upload" and "arc download" from my desktop.
- Ran "arc paste" from somewhere random.
- Cleared my config and hit the error, got useful instructions.
Reviewers: btrahan, csilvers
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2424
Summary: These are the unambiguously-good changes from D2388. Show commits included in a revision in the editor in "arc diff".
Test Plan: Ran "arc diff", saw which commits were being included.
Reviewers: nh, jungejason, vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1183
Differential Revision: https://secure.phabricator.com/D2406
Summary:
Always use the value in git.default-relative-commit when getting the relative
commit in git.
Test Plan:
Ran arc diff in a repo with git.default-relative-commit set to HEAD^ on a
branch tracking a remote (that is different from HEAD^), and checked that
the diff against HEAD^, not the remote, was published.
Reviewers: jungejason, epriestley, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2409
Summary:
Essentially D2391, but with, uh, more comments?
- I forgot that we already implemented shouldOverwriteWhenCommitMessageIsEdited(). This patch already behaves nearly correctly.
- Requires changes in D2412.
- Use `'edit' => 'edit'`, which does the same thing as `'edit' => true`, but is more correct after the "edit" / "create" split.
- Under "--verbatim", always get the message "from the user", which means "from the working copy" because verbtatim disables the editor part.
Test Plan:
- Created and updated revisions with `arc diff`.
- Created and updated revisions with `arc diff --verbatim`.
- Updated revisions with `arc diff --edit`.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: vrana, aran
Differential Revision: https://secure.phabricator.com/D2411
Summary:
Clean up the remaining odds-and-ends here -- move to "differential.close", get rid of the old constant, etc.
I'll wait a week or two to land this since "differential.close" just landed and all the other stuff is trivial.
Test Plan: Grepped for "committed".
Reviewers: btrahan, vrana, Makinde
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T909, T1182
Differential Revision: https://secure.phabricator.com/D2309
Summary:
The major thing I want to do here is allow you to set a default Phabricator URI, so we can make "arc paste", and "arc upload", "arc download" work anywhere.
We can also relax the .arcconfig requirements (request from @csilvers).
Test Plan:
Get/set some values?
iiam
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2400
Summary:
- We no longer need color options since we fake our way through parsing ANSI colorized diffs and use HGPLAIN (on Windows, too!). Drop 'em.
- In the case where you have nothing outgoing, we don't cache the relative commit and thus run "hg outgoing" too many times, which is fairly slow (even if you have nothing outgoing). Cache it.
Test Plan: Ran "arc diff --trace" in a mercurial working copy with nothing outgoing; verified we run "hg outgoing" only once.
Reviewers: Makinde, csilvers, btrahan
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2399
Test Plan:
Cancel `arc diff`.
Verify that the message is created.
Run `arc diff --verbatim` and see no reuse message question.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2390
Summary: With `--verbatim` flag, notes created from parse errors are never displayed to user resulting in blank fields.
Test Plan:
- `arc diff --verbatim` with invalid Cc
- `arc diff --verbatim` with all fields correct
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2395
Test Plan: Ran arc land locally with both the mutable default option and with the --merge flag to ensure that messages are set properly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2372
Summary: In Windows, you can't use `X=y cmd` syntax to set variables. Use "set X=y & cmd" instead.
Test Plan:
- Ran "arc diff" in a Mercurial repo in Windows, created D2367.
- Verified this does //not// cause 'HGPLAIN' to be set in the outer shell (where you type "arc diff").
Reviewers: Makinde, tido, indiefan, btrahan
Reviewed By: tido
CC: aran
Maniphest Tasks: T1179
Differential Revision: https://secure.phabricator.com/D2368
Test Plan: Run arc diff locally, verify via git log that the commit is amended afterwards (using the mutable history paradigm)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2369
Summary:
We do unnecessary working copy checks under "--show", even though the working copy isn't relevant.
Also, 'sourcePath' may not be set (e.g., "arc commit --show --revision X" where X is some "--only" revision).
Test Plan: Ran "arc commit --show --revision 1" against some test data, got clean output.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2353
Summary: Apparently my advice here was terrible, and `--no-decorate` and `--decorate=no` are both very recent additions to Git which a bunch of users don't have. Get rid of them since D2344 allows us to parse all decorate levels anyway.
Test Plan: Tried to google "git changelog", got a bunch of pages about managing changelogs with git.
Reviewers: zeeg, ehren
Reviewed By: ehren
CC: ehren, aran, NorthIsUp
Differential Revision: https://secure.phabricator.com/D2354
Summary:
"git diff -M -C" generates useful metadata (moves/copies) but (for a pure move) no diff text. Synthetically build the diff text after the fact so this information is available in Differential.
This patch is kind of nasty but I couldn't see a cleaner way to do it. :/
This also needs some UI changes in Differential: we get a full-green new file right now, but it would be better to default-hide it with "This file was moved. Show More" or similar.
Test Plan: Moved a file, ran "arc diff", got textual diff.
Reviewers: aran, tuomaspelkonen, jungejason, btrahan, vrana
Reviewed By: vrana
CC: aran, epriestley, vrana
Maniphest Tasks: T230
Differential Revision: https://secure.phabricator.com/D479
Summary:
Arcanist fails to find git's 'commit <hash>' header when log.decorate is
set in one of git's config files. git adds the named refs that point to
the commit in parentheses following the hash. This changes the regex
used by arcanist to match git commit headers to optionally match the
branch names in parentheses following the hash.
Test Plan:
Run `git config --global log.decorate short` and check that `arc diff`
runs successfully.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2344
Summary: Missed this when getting rid of all the 'file' calls.
Test Plan: Meta.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1159
Differential Revision: https://secure.phabricator.com/D2327
Summary:
Wrapper for Python 'nose' (http://readthedocs.org/docs/nose/en/latest/)
testing tool.
Test Plan:
Install latest 'nose' v1.1.3. Currently it is available through
Github only (``pep install git+https://github.com/nose-devs/nose.git``).
Create a Python project with following structure:
/package_name/module_name.py
/tests/package_name/test_module_name.py
Write some tests
Run ``arc unit``
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, zeeg
Differential Revision: https://secure.phabricator.com/D2322
Summary: Currently, if you change a symlink outside a libphutil library and the link target is something inside a libphutil library, we may enter an inifite loop in the "do { ... } while(...)" later. Just bail if the loop won't resolve.
Test Plan: Ran arc unit, Airtime reported the issue resolved by a similar fix.
Reviewers: cpiro, btrahan
Reviewed By: cpiro
CC: aran
Differential Revision: https://secure.phabricator.com/D2318
Summary:
Allow tests to be skipped by calling assertSkipped(). It's not really
an assertion of anything tangible; more like "assert that we can't
really assert anything right now".
Test Plan: Added a new test to the PhutilUnitTestEngineTestCase.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2312
Summary:
- Add branch name tab completion to "arc land".
- Default to landing the current branch.
- This is a little bit hacky but not too terrible. I'm planning to move the whole thing to PhutilArgumentParser at some point so that'll be an opportunity for a big refactor.
Test Plan: Hit tab, landed this branch.
Reviewers: zeeg, btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran, kdeggelman
Differential Revision: https://secure.phabricator.com/D2293
Summary:
Don't use abstract subclasses of ArcanistPhutilTestCase, only use
concrete ones.
This lets you put common functionality in an abstract BaseTestCase
(which itself is a subclass of ArcanistPhutilTestCase), then implement
concrete subclasses of the BaseTestCase.
Test Plan: Tested with a simple Base -> {Case1, Case2} setup.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2300
Summary:
- Replace SVN-specific language with VCS-agnostic language.
- Add new "arc close-revision", works exactly like "arc mark-committed" but with agnostic language.
- Use status constants, not status strings.
- Mark "arc mark-committed" deprecated.
- Remove deprecated "arc merge".
Test Plan: Ran "arc mark-committed", "arc close-revision".
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran, Makinde
Maniphest Tasks: T909
Differential Revision: https://secure.phabricator.com/D2244
Summary: See rage in T1117. Don't use the <project + branch> heuristic anymore..
Test Plan: Ran "arc diff --strict HEAD^" on a commit stacked on top of this one, got no matches.
Reviewers: btrahan, vrana, simpkins, beng
Reviewed By: btrahan
CC: aran, avive
Maniphest Tasks: T1117
Differential Revision: https://secure.phabricator.com/D2221
Summary:
- Historically, "--preview" was forbidden under SVN. No reason for that now.
- The "--auto" patch moved the "--preview" / "--only" checks later than they should be.
- Fix an issue with Conduit query construction in SVN.
Test Plan: Ran "arc diff --preview" in an SVN working copy. Ran "arc diff" in an SVN working copy.
Reviewers: svemir, btrahan, vrana, jungejason
Reviewed By: svemir
CC: aran
Differential Revision: https://secure.phabricator.com/D2218
Summary:
When people create the .arc/default-relative-commit scratchfile with $EDITOR of
choice, their editor usually puts a newline at the end, which breaks arc diff.
We should trim the newline before using the contents of the scratchfile.
Test Plan:
ran arc diff in a working copy that contained a .arc/default-relative-commit
with a newline
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2209
Summary: If you have two projects (say, libphutil and arcanist) and you prepare a patch for one of them on branch "master", run "arc diff", and then prepare a patch for the other one on the same branch, "arc diff" will try to update the first revision when you run it. Instead, make it smart enough to stay within arc projects.
Test Plan: Ran "arc which" in circumstances where it previously generated a false positive, no false positive.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T1100
Differential Revision: https://secure.phabricator.com/D2199
Summary: See discussion in D1861.
Test Plan: Ran "arc diff" on master, got an upstream-based relative commit. Ran "arc diff" on a feature branch, got a config-based relative commit. Ran "arc diff x", got an argument-based relative commit.
Reviewers: btrahan, vrana, davidreuss, elgenie
Reviewed By: davidreuss
CC: aran
Differential Revision: https://secure.phabricator.com/D2192
Summary:
For Objective-C repositories, we want to provide aliases to
arc diff --amend-autofixes by default.
This adds the ability to define aliases in .arcconfig (overridden
by any specified in the user config, of course).
Test Plan:
Tested defining alias with nothing in .arcconfig, with
an alias in .arcconfig. Tested arc alias outside of working
repository.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2191
Summary:
Xcode (a popular code editor on Mac OS X) has no facility
to trim trailing whitespace automatically.
This adds a new lint severity "AUTOFIX" that's between
WARNING and ERROR. When running the linter, any lint message
whose severity is AUTOFIX will automatically be patched.
Furthermore, if all lint messages returned from the engine are
AUTOFIX, we'll automatically amend HEAD with the patch.
Test Plan:
arc lint on files with and without trailing whitespace,
with and without UTF-8 contents to confirm those still error
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2125
Summary:
--auto doesn't work right now on the implicit --create pathway in SVN and HG because we hit these conditions.
Also improve a message.
Test Plan: Ran "arc diff" in unaffiliated working copies in HG and SVN.
Reviewers: svemir, btrahan, vrana, jungejason
Reviewed By: svemir
CC: aran
Differential Revision: https://secure.phabricator.com/D2187
Summary: See next diff for an explanation of this issue.
Test Plan: See next diff.
Reviewers: Makinde, btrahan, vrana, jungejason
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2174
Summary:
```arc close T1088 --status wontfix --message "I'm not going to fix this."```
T1088 is the test task to screw with, so feel free.
Test Plan: ```arc close T1088 --status [ resolved | wontfix | invalid | duplicate | spite | open ] -m "Message"```
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2162
Summary: Saw this in a diff somewhere; complain about it.
Test Plan: Unit coverage.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1060
Differential Revision: https://secure.phabricator.com/D2153
Summary:
Added `arc tasks`:
%%%arc tasks
arc tasks --view-all // View Open and Closed Tasks
arc tasks --by-status // Group By Status
arc tasks --by-priority // Group By Priority%%%
Test Plan: Connect to conduit and run arc tasks >> make sure you have tasks =p
Reviewers: epriestley, indiefan
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T749
Differential Revision: https://secure.phabricator.com/D1943
Summary:
NOTE: This is a disruptive change to how 'arc diff' behaves by default.
Many years ago, Differential (then DiffCamp) supported SVN. Someone added a "-i" mode to the "diffcamp" script so you could "git show | diffcamp -i", and thus we were
forever bound to storing metadata in commit messages.
But this isn't a common use case outside of Facebook + git-svn, and isn't very flexible. We now have a great deal of flexibility to identify revisions based on
hashes, branch names, etc, and to sync metdata from web to CLI and back. I want to jettison the commit-message-bound artifacts of the tool's history, and move to a
more flexible, modern workflow.
I added "--auto" a while ago, which figures out if you want to create or update a diff automatically, and then prompts you for whatever data it needs, reading
information if it can from commit messages in the range. This is a vastly better workflow in general, especially for SVN and Mercurial users (who currently need to
jump to the web UI to create revisions). It's better for git users too, since they don't need to use template commits and don't have to muck with or configure
templates. However, it's a nontrivial change to git users' core workflow that is clearly different in more ways than it is clearly better.
- This might be contentious, but probably not toooo much, I hope?
- I also deleted the (fairly ridiculous) workflow where we sync commit message changes from the working copy. I think two or three people will be REALLY upset about
this but I have no sympathy. "--edit" covers this and has been around for like two years now, and making the commit message and web dual-authoritative was always a bad
idea that we only ever half-accommodated anyway (see giant swaths of removed TOOD nonsense).
Test Plan:
- I've been using "--auto" exclusively for like a month, it seems to work well.
- Created/updated SVN, Git and HG diffs with "arc diff" under this workflow.
Reviewers: btrahan, jungejason, nh
Reviewed By: btrahan
CC: Makinde, vrana, zeeg, mbautin, aran, yairlivne, 20after4
Maniphest Tasks: T614
Differential Revision: https://secure.phabricator.com/D2080
Summary:
This should be a raw text block, not a parsed message object.
I broke this in rARC1f13e022cdc9bf4859274a83784bd615caf62ef9 when I improved Mercurial support.
See: https://github.com/facebook/arcanist/issues/23
Test Plan: (Will update this diff...)
Reviewers: vrana, jungejason, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2136
Summary: Allows you to set a default in .arcconfig. This default is overriden by any .arc/ setting.
Test Plan: Ran "arc diff" with a .arcconfig setting but no .arc/ setting, got a diff against the specified relative commit.
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D2093
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2084
Summary:
The default of "arc diff" to "arc diff HEAD^" in git is universally confusing to everyone not at Facebook.
Drive the default with configuration instead. Even at Facebook, "origin/trunk" (or whatever) is probably a better default than "HEAD^".
See D863 for the last attempt at this.
NOTE: This is contentious!
Test Plan: Ran "arc diff", got prompted to set a default. Ran "arc diff" from a zero-commit repo, got sensible behavior
Reviewers: btrahan, vrana, nh, jungejason, tuomaspelkonen
Reviewed By: btrahan
CC: aran, epriestley, zeeg, davidreuss
Maniphest Tasks: T651
Differential Revision: https://secure.phabricator.com/D1861
Summary:
See D2049, D2050. Identify reuses of locals as iterator variables. Before raising an error, we require:
- Variable is declared before the loop.
- Variable is used after the loop, ignoring uses as an iterator variable.
I think this identifies all problems with a very low false positive rate (the false positives are suspicious/unconventional code, but not necessarily errors).
Also fix an issue identified by the linter.
Test Plan:
- Verified this identified the bugs in D2049 and D2050.
- Ran linter against libphutil/, arcanist/ and phabricator/ (see D2051, this, and next diff).
- Ran unit tests.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley, jungejason
Differential Revision: https://secure.phabricator.com/D2052
Summary:
Addresses concerns in rARCefb8219196abf047f14b505959e54d078e1df6d3:
- As I recall, the intent of "generateFile" was that these warnings would replace all the other warnings for that file, under the theory that if one warning caused regeneration of
the file the other warnings were irrelevant.
- However, this code never had any effect and I haven't seen any issues with the behavior.
- So, just remove it.
Addresses concerns in rARC070e963d1c26879e47eab19a2377e388c2f166c5:
- Ran "arc liberate --all".
Test Plan: Ran "arc lint" after removing an "__init__.php" with and without a "fixed" generateFile block, there was no behavioral change. So I just nuked it.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2049
Summary:
D2016 changed the behavior of `phutil_console_wrap()`.
The new behavior is better so I am fixing callsites instead of the function.
Test Plan:
arc help help
Verify that the options descriptions is not indented with 28 spaces.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2047
Summary: When tests have a lot of output, show a diff of the expected/actual.
Test Plan: Used this when developing D2016 to examine large output usefully.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2017
Summary: Git can't apply filename case change patches on case-insensitive filesystems. Some day we could manually do this ourselves, but it's fairly rare and complicated -- just raise a useful warning.
Test Plan: Tried to apply a case-changing patch, got a good error.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T618
Differential Revision: https://secure.phabricator.com/D2015
