1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2025-02-21 19:19:11 +01:00
Commit graph

800 commits

Author SHA1 Message Date
epriestley
1ef9409817 Update "arcanist/" for "topological" API changes
Summary: Ref T13325.

Test Plan: Grepped for `topograph`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13325

Differential Revision: https://secure.phabricator.com/D20598
2019-06-20 16:05:21 -07:00
Asher Baker
7329bc7c32 Fix arc land on odd/modern git-svn checkouts
Summary:
The current code assumes git-svn is always working from a remote called
`trunk`, but if the repository is initialized without the `-T` option it
will instead be called `git-svn`, and if `--prefix` is used (which is
set by default to `origin/` in Git 2+) the remote name will have the
specified prefix as well.

Instead, look at the `fetch` target refspec set in the git-svn config.

Fixes T13293.

Test Plan:
`arc land` without errors (or manually creating a `trunk` branch) from a
checkout made with Git 2.18.0 (verified this manually on a non-`-T`
checkout as well).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T13293

Differential Revision: https://secure.phabricator.com/D19681
2019-05-23 10:58:42 +01:00
epriestley
9ccbbee336 Fix a potential double-prompt in "arc land" when landing with ongoing builds
Summary: The recently-added, build-plan-behavior-aware check here does all of its own prompting, so we should skip the other prompting if it doesn't throw.

Test Plan: Will `arc land` something sketchy sooner or later.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20494
2019-05-14 09:27:53 -07:00
epriestley
9830c9316d Make minor correctness changes to some "arc patch" command execution
Summary:
Since I'm in here for PHI1083:

  - Add some "--" so we get correct behavior when you have a file named "master", a branch named "README.txt", etc.
  - Stop using "%C" unnecessarily.
  - Fix some untranslatable strings.

Test Plan: Ran `arc patch` a couple of times, ran the variations of `git` commands to catch anything weird with "--" handling.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20254
2019-03-07 13:00:04 -08:00
epriestley
73804f0039 Fix a case where "arc patch" could skip submodule changes
Summary:
See PHI1083. Previously, see PHI648 and D19475.

When you apply a submodule patch in Git, it leaves you with a working copy that has the "submodule pointer" dirtied but the actual submodule untouched:

```
$ git status
On branch ...
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   philter

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   philter (new commits)
```

So, if you're applying `D123` and `submodule/` was previously pointed at commit "A" but `D123` updates it to point at commit "B", you get this after `git apply ...`:

  - Git index says "submodule/ = B".
  - On disk, "submodule/ = A".

Now, if you `git add --all` or `git commit --all`, git picks up the "change" on disk as an intended modification of the submodule. This puts the submodule back to "A" and overwrites/undoes the "pointer" update that's trying to make it point to "B".

To avoid this, update submodules after applying the patch.

Also, every time we modify the working copy, just update submodules.

Test Plan:
  - Add a submodule in branch "B1", pointed at commit "A".
  - Branch to create branch "B2". Update the submodule to point at commit "B". Commit this and `arc diff` it.
  - Go back to "B1". Use `arc patch D...` to apply the revision you just created.
  - Before change:
    - "arc patch" applies the submodule change, so "pointer = B", "disk = A".
    - "arc patch" runs "git commit --all", which looks at disk and sets "pointer = A".
    - This isn't a change, so we fail with an empty commit.
  - After change:
    - "arc patch" applies the submodule change, so "pointer = B", "disk = A".
    - "arc patch" updates submodules, so "pointer = B", "disk = B".
    - "arc patch" runs "git commit --all", which now has a change, and commits "submodule = B".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20253
2019-03-07 12:52:27 -08:00
epriestley
f6b8480adc Implement "Warn When Landing" behavior for Build Plans in Arcanist
Summary:
Ref T13258. This makes "arc land" respect the new "Warn When Landing" behavior.

This will only work if you have very up-to-date APIs. Just fall back to the older code if the new API calls fail.

Test Plan: Ran `arc land` on a revision with builds in various states and with the different "Warn When Landing" behaviors. Saw appropriate warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13258

Differential Revision: https://secure.phabricator.com/D20236
2019-03-06 06:47:53 -08:00
epriestley
96fde137a1 Improve performance of "arc diff" updates for changes with large diff text
Summary:
See PHI1104. The older "differential.querydiffs" method includes the entire raw diff text for all the diffs associated with a revision in its response, but we: only care about the most recent diff; and don't care about the text at all.

For reasonably large changes with several updates, this can be significantly slow.

We can get this same information more efficiently from the modern "differential.diff.search", since D19386 (April 2018). The only trick is that we need a "revisionPHID", which we don't have on hand.

For now, just fetch the revision PHID. In the future, we can likely make adjustments so that we have the revision PHID already by the time we get here.

This may slow down the normal case very slightly (since we now do two calls instead of one), but it speeds up the bad cases dramatically.

Test Plan:
Ran `arc diff` to update a change in a local repository. `var_dump()`'d the old and new algorithm results, saw the same outcome.

Used `arc diff --trace` on an update to a change to verify that `differential.diff.search` is called but `differential.querydiffs` is not.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20221
2019-02-28 08:00:01 -08:00
epriestley
07a208d8fc In "arc diff", warn when some reviewers are away even if not everyone is away
Summary:
Ref T13249. See PHI810. We currently warn you when //all// reviewers are away, but not when only some reviewers are away.

This makes some amount of sense under the "anyone can accept anything" rules we sort of recommend, but a lot of installs realistically have tons of owner/package rules now.

Instead, if any reviewers are away, show the user exactly who is away and until when, then make sure they don't want to make any adjustments.

(We can do a better job of this after the toolsets change when we can use the new APIs, but this is an easy fix for now.)

Test Plan: Created a revision with multiple reviewers, either some or all of whom were away. Got appropriate output and prompt behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20172
2019-02-15 14:44:37 -08:00
epriestley
df7313bdf2 In "arc patch", update submodules slightly later
Summary:
Ref T13151. See PHI648. With `arc patch --nobranch`, we update submodules a little too early.

I //believe// it is safe to just update them a little later, after the intermediate branch management logic runs.

Test Plan: Ran `arc patch --nobranch`, saw submodule update run later. Not 100% sure this doesn't cause weird issues, but I can't anticipate any.

Reviewers: amckinley, jmeador

Reviewed By: jmeador

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19475
2018-06-07 12:03:08 -07:00
epriestley
73f5afd441 Remove "very large change" warning from Arcanist
Summary: Ref T13110. We now degrade very large changes and I'm not convinced any user ever entered "n" at this prompt.

Test Plan: Ran `arc diff` to create this very revision.

Maniphest Tasks: T13110

Differential Revision: https://secure.phabricator.com/D19299
2018-04-05 06:50:57 -07:00
epriestley
e44a2d3ac0 Improve argument parsing for "arc patch --revision Dxxx"
Summary: See PHI527. Ref T13116. The `--revision` flag currently fails if the argument is in the form `D123` instead of `123`. Normalize monogram arguments.

Test Plan: Ran `arc patch --revision Dxxx`, `arc patch --revision xxx`, `arc patch --revision xxx --diff yyy`, `arc patch`; got good behavior on the good ones and sensible error messages on the other ones.

Maniphest Tasks: T13116

Differential Revision: https://secure.phabricator.com/D19292
2018-04-03 10:56:46 -07:00
Alex Vandiver
bf3d32e34e Remove accidental sprintf injection in error reporting
Summary:
STDERR output with `%`s in it could cause:

```
ERROR 2: fprintf(): Too few arguments at [/usr/local/arcanist/src/workflow/ArcanistFeatureWorkflow.php:170]
```

Test Plan: Untested.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D19261
2018-03-26 13:56:50 -07:00
epriestley
be1dd7e2ba Robustly fuse files together with arc weld
Summary: Fixes T8236. I played around with a lot of variations of this but in the end it felt like the simple version was best.

Test Plan: Ran `arc weld a.txt b.txt`, observed very robust fusion of materials.

Maniphest Tasks: T8236

Differential Revision: https://secure.phabricator.com/D19081
2018-02-13 15:15:36 -08:00
epriestley
349109426c Clarify what "--everything" means in "arc lint" and "arc unit"
Summary:
Fixes T13061. Both `arc lint` and `arc unit` accept an `--everything` flag, but the documentation isn't quite clear about what these flags do.

They act as though every //tracked// file in the repository (`git ls-files`, `hg manifest`, or `svn list -R`) is included in the argument list.

They do not lint/test ignored files (and I think almost all users would be very surprised if they did).

They also don't lint/test untracked files (files you have not yet used `git add`, `svn add`, or `hg add` on). This is slightly more contentious but we have good reasons for doing it (e.g., `git ls-files` often outperforms `find .` by a large margin) and I believe users very rarely use `--everything` in a situation where they have untracked files. The only real exception I can come up with is linter configuration/development, as in PHI343, and it seems okay to have a slightly surprising behvaior here.

Make the documentation more clear about what is in scope.

We could also rename these to `--nearly-everything` or whatever, but I think the name is probably clear enough given current information about how confusing this is (specifically: only rarely, in unusual cases).

Test Plan:
  - Grepped for documentation about these flags.
  - Ran `arc help lint`, `arc help unit`, `arc unit --everything x`, `arc lint --everything x` and read all the new messages.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13061

Differential Revision: https://secure.phabricator.com/D18989
2018-02-05 12:26:41 -08:00
epriestley
3d06bd4c56 Fix a minor 'buildableStatus' error in "arc land"
See D18837. I made a mistake here with rigging this for testing; this line was
changed incorrectly.
2017-12-26 08:33:35 -08:00
epriestley
c784b56920 Make "arc land" accommodate a minor API change in Harbormaster build statuses
Summary:
See PHI261. Currently "arc land" shows every build staus (passed, failed, building, etc) as yellow. Intended behavior is that passed builds are green, failed builds are red, and so on.

This is because of an unintended API change a while ago in D16356. Since the only impact was a cosmetic color issue, this escaped notice until now.

Additionally, try to use the modern `harbormaster.build.search` if it is available.

Test Plan:
  - Ran `arc land` with running builds, got reasonable coloration.
  - Faked the new method not being available, still got sensible behavior from the old method.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18837
2017-12-23 11:39:19 -08:00
Alex Vandiver
249f3a80fe Default the prompt for "Amend HEAD with these patches" to true
Summary:
Most users, if they have gone through the trouble of
accepting the auto-fixes, are most likely going to want to take those
changes and attempt to land with them.  Assuming "Y" for this prompt
streamlines for the more likely flow.

Test Plan: `arc lint`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D18824
2017-12-12 01:43:16 -08:00
epriestley
f4c80a114d Make "arc land" prompt on "Changes Planned" revisions more explicit
Summary: Fixes T10233. See PHI231. Users sometimes believe this warning is a bug and/or don't understand how they're supposed to resolve it.

Test Plan: Ran `arc land` on a revision in "Changes Planned", got a sensible prompt. Ran `arc land` on a revision in another non-accepted state, got more or less the old prompt.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10233

Differential Revision: https://secure.phabricator.com/D18807
2017-11-30 13:51:50 -08:00
epriestley
76b54ce0a9 Fix parsing of Git branches with common and useful name "0"
Summary: See <https://discourse.phabricator-community.org/t/arc-has-a-spurious-failure-on-arc-diff/521>. Oh, PHP!

Test Plan: Created a branch named "0", ran `arc diff`. Before: fatal. After: this beautiful revision.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18678
2017-10-04 10:20:48 -07:00
epriestley
d9cb5b18fb Cheat our way through arc version on Windows for the moment
Summary: Fixes T8291. See PHI52. This is papering over the real issue (T8298) but it's a 10-second patch so just improve things slightly for now.

Test Plan: Ran `arc version` locally; patch confirmed on a Windows system by an affected user.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8291

Differential Revision: https://secure.phabricator.com/D18518
2017-09-01 18:06:00 -07:00
Alex Vandiver
5eda40337b Fix missing whitespace in arc linters --help message
Test Plan: Ran `arc linters --help`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D18344
2017-08-04 13:12:59 -07:00
Stephanie Ren
165df12046 Consistently use phutil_console_confirm().
Summary:
`PhutilConsole->confirm()` is vestigial as noted in efcd70c, as
`PhutilConsole` may be deprecated sometime in the future. `PhutilConsole`
was developed as a tool for T4281 but the feature has been removed
since.

Replace existing occurences of the `PhutilConsole->confirm()` pattern with
`phutil_console_confirm()`. There should be no change in functionality
since the two functions are interchangeable.

Test Plan: Manually tested by running `bin/arc lint`, `bin/arc diff --preview`, `bin/arc land --preview`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, alexmv

Differential Revision: https://secure.phabricator.com/D18185
2017-07-06 16:27:36 -07:00
epriestley
c04f141ab0 Remove obsolete "arc land" flags: --update-with-merge/rebase, --delete-remote
Summary:
Fixes T12815. During the last update to "arc land", some flags were disabled but remained in place in case we needed to retain them.

It now seems reasonably clear that we do not. The "rebase" and "merge" strategies for landing were replaced by a better "headless" strategy which seems to avoid the original issues, so these flags no longer do anything or reasonably could do anything.

`--delete-remote` is slightly more ambiguous (e.g., see T12650 and maybe others) but the only real use case is "git push = save changes".

Test Plan:
Ran `arc land --update-with-rebase`, was told the flag does not exist.
Grepped for affected flags/symbols.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12815

Differential Revision: https://secure.phabricator.com/D18108
2017-06-09 08:12:37 -07:00
Alex Vandiver
129d51fa09 If the base commit for arc patch does not exist locally, try to fetch it
Summary:
If the commit does not exist locally, aborting still leaves
the user checked out on the branch.  In nearly all cases, all that is
necessary is a fetch -- but the branch must also be cleaned up.  This
leads to the pattern of:
```
arc patch D12345
[...base commit does not exist...]
^C
git checkout master
git branch -D arcpatch-D12345
git fetch
arc patch D12345
```

Solve this common problem by simply trying to fetch once if the commit does not
exist locally.

Test Plan: Ran `arc patch` on a recent diff.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17949
2017-05-18 13:13:22 -04:00
epriestley
27b51e6192 Fix two minor issues with "arc download"
Summary:
Ref T12651. Ran into these during D17799:

  - Use `getStatusCode()` to put the actual status code into the message.
  - If we fail but wrote an empty file to reserve the filename, clean it up.

Test Plan:
  - Faked the error, `phlog()`'d the exception.
  - Saw sensible exception message.
  - Saw empty file get cleaned up.

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12651

Differential Revision: https://secure.phabricator.com/D17800
2017-04-28 07:45:25 -07:00
epriestley
82b7cd778a Make "arc download" use "file.search" if available
Summary: Fixes T8348. Just use normal HTTP GET to download files if the server is sufficiently modern.

Test Plan: Downloaded various files with `--as`, `--show`, large files, small files, old server, new server.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8348

Differential Revision: https://secure.phabricator.com/D17614
2017-04-04 16:16:11 -07:00
Jakub Vrana
3b6b523c2b Fix errors found by PHPStan
Test Plan: None.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D17376
2017-02-18 09:24:19 +00:00
Jakub Vrana
d0353d2381 Fix errors found by PHPStan
Test Plan:
Ran `phpstan analyze -a autoload.php arcanist/src` with `autoload.php` containing:

  <?php
  require_once 'libphutil/src/__phutil_library_init__.php';
  require_once 'arcanist/src/__phutil_library_init__.php';

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17367
2017-02-16 13:54:43 +00:00
Aviv Eyal
13596cd10f Add back pht()s and tsprintf()s to arg set-config
Summary: See D17357

Test Plan: invoke and still see output in English?

Reviewers: #blessed_reviewers, joshuaspence, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17362
2017-02-16 11:08:45 +00:00
Aviv Eyal
bc9b70508e arc set-config: warn about unknown keys
Summary: See T12266. Warn when the user tries to set a value we will probably not read.

Test Plan: `arc set-config foo bar`, see fancy warning.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D17357
2017-02-15 13:38:57 +00:00
Josh Cox
224986af63 Provide a better error message when an invalid ID is given to arc patch
Summary:
Fixes T8937. Previously when running `arc patch D9999999999` or `arc export --revision 99999999` with a non-existent diff or revision ID you would get a rather unhelpful error message. Now you'll get a slightly more helpful error message:
```
$ arc patch D99999999
Exception
Couldn't find a revision or diff that matches the given ID
(Run with `--trace` for a full exception trace.)
```

Test Plan: Ran arc patch with a valid revision and saw it patch successfully. Ran again with an invalid revision, saw the error message.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley, yelirekim

Maniphest Tasks: T8937

Differential Revision: https://secure.phabricator.com/D17325
2017-02-08 04:53:41 -05:00
epriestley
ade25facfd Fix a bad interaction between "arc diff --reviewers" and "the first line of a message is always a title"
Summary:
Fixes T12069. We implement "arc diff --reviewers" (and "--cc") by parsing a faux message with "Reviewers: ...".

After D17122, the first line of the message is always interpreted as a title, so the text ends up in the message body.

Instead, use a placeholder title so these fields are never initial fields.

Test Plan: Ran `arc diff --reviewers dog`, got only one "Reviewers" field.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12069

Differential Revision: https://secure.phabricator.com/D17147
2017-01-05 14:35:39 -08:00
Josh Cox
9e82ef979e Added a warning prompt if the user tries to use an API cert instead of a CLI cert
Summary: Fixes T9692. Instead of disallowing API tokens entirely, we're going to just warn the user that they might not want to do that. After that, they can proceed if they want to.

Test Plan:
Run arc install-certificate.
Manually go to `Settings → Conduit API Tokens` in the web UI.
Generate an API token explicitly, which should have the form api-******.
Paste that into the prompt on the CLI.
It will give you a warning prompt then ask if you'd like to proceed anyway (defaults to No).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T9692

Differential Revision: https://secure.phabricator.com/D16448
2016-08-25 11:34:34 -04:00
Luke081515
3ae0cc8d41 Avoid double [y/N] when updating a not owned revision
Summary:
Since 737f5c0df9 arcanist shows [y/N] two times, when arcanist asks you, if you want to proceed, if you want to update a not owned revision. Ths patch fixes
that, so that Arcanist shows [y/N] only once, like at other situations, when Arcanist asks you a question.
Ref T11489

Test Plan: Updated a not owned revision.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Tags: #arcanist

Maniphest Tasks: T11489

Differential Revision: https://secure.phabricator.com/D16415
2016-08-17 10:36:16 -07:00
epriestley
06c641f92c Remove command spelling correction from Arcanist
Summary: Fixes T7489. Depends on D16332, which moved this code to libphutil.

Test Plan:
```
$ arc banch --bystatus
(Assuming 'banch' is the British spelling of 'branch'.)
(Assuming '--bystatus' is the British spelling of '--by-status'.)
...
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7489

Differential Revision: https://secure.phabricator.com/D16333
2016-07-27 09:35:28 -07:00
Rabah Meradi
1bedfdccd7 Fixes help message for stop command
Test Plan: arc help stop will display 'Stop tracking work...'

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: avivey, chad, epriestley

Differential Revision: https://secure.phabricator.com/D16310
2016-07-27 05:42:07 -07:00
epriestley
4d4d16f259 Validate Arcanist install-certificate URIs more carefully
Summary: Fixes T11222. This was lazy-future-proofed for Conduit SSH support, but users are boundlessly creative. Check protocols explicitly.

Test Plan:
```
$ arc install-certificate a.b:1/
Usage Exception: Server URI "a.b:1/" must include the "http" or "https" protocol. It should be in the form "https://phabricator.example.com/".
```

  - Also went through a successful workflow with a URI in the form provided in the example.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11222

Differential Revision: https://secure.phabricator.com/D16188
2016-06-28 15:20:06 -07:00
epriestley
2374403e8f Remove the "you have not specified reviewers" prompt from the arc client
Summary:
Ref T4631. Ref T10939. I don't have any good solutions here; this is perhaps the least-bad one.

  - This prompt is misleading/confusing in the presence of Herald/Owners.
  - This prompt is likely of very little value for experienced reviewers.
  - When it works, this prompt may be of some value for new reviewers, but getting it wrong is probably more confusing than getting it right is helpful, and there is a more accurate version of the warning in the web UI that new users are likely to see.
  - In the long run, this code should not live in the client.

Test Plan: Created this revision without specifying reviewers, probably didn't get prompted.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4631, T10939

Differential Revision: https://secure.phabricator.com/D16139
2016-06-17 08:04:08 -07:00
Aviv Eyal
ca33240942 Don't specify size 0 for deleted files
Summary: See D15828 - arc is reporting file size as `0` for unexisting files - make it stop.

Test Plan: `arc diff` with empty, deleted, added files - see size reported as `null` when appropriate.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: chad, Korvin

Differential Revision: https://secure.phabricator.com/D16059
2016-06-06 20:51:16 +00:00
epriestley
7891df6f25 Read arc aliases from all available configuration files
Summary: Fixes T10431. Updates the getAliases() method to read every `arc` configuration file.

Test Plan:
Added an `arc duck` alias to `/etc/arcconfig`, ran `arc duck`, saw "quack".

Added an `arc pig` alias to `~/.arcrc` via `arc alias`, ran `arc pig`, saw "oink oink".

Reviewers: nevogd, chad, #blessed_reviewers

Reviewed By: chad, #blessed_reviewers

Subscribers: eadler, epriestley

Tags: #twitter

Maniphest Tasks: T10431

Differential Revision: https://secure.phabricator.com/D15342
2016-06-06 13:15:59 -07:00
epriestley
a2ab38df78 Improve performance of arc branch in Git with many branches
Summary:
This is mostly just a personal quality-of-life fix. I run this command fairly often and having it return a little faster is nice.

This replaces a `git show` for each individual branch with a big `git for-each-ref` which we were already running anyway. This is quite a bit faster.

This command also occasionally hangs or segfaults for me while executing the huge pile of subprocesses. This is unreliable to reproduce, probably some bug in some PHP extension I have, and likely hard to narrow down, and this approach is better in every way anyway.

Test Plan:
  - Ran `arc branch` in Git, observed faster output (in my `phabricator/`, about 2000ms -> 1200ms).
  - Ran `arc feature` in Mercurial.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D15735
2016-04-16 16:39:32 -07:00
Mukunda Modell
737f5c0df9 Allow amending revisions without commandeering first
Summary:
It is common practice in Wikimedia's projects to amend a contributor's
change without taking over authorship of the change. We found that
the only enforcement of commandeering before amending is in arcanist,
not validated server-side. While it would be fairly straightforward to
maintain this as a patch to arcanist, I thought I would see if upstream
is willing to support making this optional.

With this change, amending without commandeering is enabled by a flag in
`.arcconfig` and it defaults to the old behavior.

For background see [wmf T121751](https://phabricator.wikimedia.org/T121751)

Test Plan:
* ran `arc patch D146` to locally apply a revision that I did not author,
* made a trivial change and amended the commit.
* ran `arc diff --update D146 HEAD^` to send the update to differential
* Saw that https://phabricator.wikimedia.org/D146 updated as it should.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: greggrossmeier, aklapper, Luke081515.2, Korvin, dereckson

Maniphest Tasks: T10584

Differential Revision: https://secure.phabricator.com/D15468
2016-04-09 11:57:42 -05:00
epriestley
8701e6c1f3 Strip tips out of commit messages from arc backout
Summary:
Fixes T10707. Currently, `arc backout` creates a commit message which includes questionably-helpful "tips" in the message itself.

Strip these out.

Test Plan:
Used `arc backout` to revert any commit, then `git show` to see the generated message.

  - Before patch: included tips.
  - After patch: no tips.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10707

Differential Revision: https://secure.phabricator.com/D15573
2016-04-02 07:26:02 -07:00
Aviv Eyal
3d7ac867f5 Make callsigns optional in arcanist
Summary:
Remove couple of references to callsigns:
- `arc which` now prints repository name
- `getShouldAmend()` can now use new format of commit name

a quick git-grep looks like the remaining references are all about `repository.callsign` config.

Ref T4245

Test Plan:
- `arc which` on a repository with no callsign
- trigger `requireCleanWorkingCopy()`, see both "Do you want to amend this change" and "Do you want to create a new commit" prompts.
- fire this diff with new code.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D15472
2016-03-15 03:58:46 +00:00
epriestley
ccbaee585e When arc pushes to the staging area, tell Phabricator what we did
Summary:
Ref T10093. Right now, Phabricator kind of guesses that `arc` probably pushed stuff to the staging area.

This can cause confusing/misleading errors later, if it didn't actually push.

Instead, tell Phabricator that we pushed, so we can raise more tailored messages in the web UI (e.g., make "Land Revision" say "this wasn't pushed to the staging area" instead of "whoops, error!!~").

Test Plan:
Ran `arc diff` a few times, then looked in the database for properties.

{F1161655}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10093

Differential Revision: https://secure.phabricator.com/D15426
2016-03-07 07:24:16 -08:00
epriestley
4a1160e0c3 When pushing changes to staging, also push the base commit
Summary:
Fixes T10509. Pushing changes to staging can be inefficient. What happens, roughly, is:

  - Master is at commit "W" -- "W" is the most recent published commit in the main repository.
  - The local working copy has one change on top of that, "X", so its history is commits "A, B, C, D, E, F, ..., U, V, W, X".
  - The remote has some other previous changes that I or other users have made, maybe like "A, B, C, ..., S, T, U, Y" and "A, B, C, ..., T, U, V, Z", from previous pushes to staging areas.
  - "X", "Y" and "Z" will never actually make it to master, because they'll be squash-merged/amended by `arc land`.

So the local says "I want to push 'X'", and the remote says "I know about 'Y' and 'Z', are those in the history of 'W'? You only need to send me new stuff if they are".

But they aren't, so the local says "nope, so here's the whole history for you". This is slow and sends a ton of data that the remote already has over the network.

In theory, Git could use a slightly different algorithm to tell the local about more commits, but this is hard, rarely useful, and not the kind of thing I'd be excited about changing if I was the Git upstream.

Instead, when pushing "X", also push "W", to trick Git into telling future clients about it.

Now, the remote should say "I know about 'W', 'Y' and 'Z'", and the local will say "oh, great, 'W' is in history, here's just the changes since then".

Also, fail `arc diff` if the push to staging fails, and tell users to use `--skip-staging`. This code has been in production for a while and doesn't seem to have any issues, and a failed push to staging prevents builds, lands, etc.

Test Plan:
  - Ran `arc diff`, saw two changes push.
  - Ran `arc diff --base arc:empty`, saw only one change push.
  - Ran `arc diff` with an intentionally broken staging area, saw an error.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10509

Differential Revision: https://secure.phabricator.com/D15424
2016-03-07 06:53:49 -08:00
epriestley
3876d93583 Properly URL encode branches in arc browse --branch ...
Summary: Fixes T10511. If you `arc browse --branch x/y/z`, we do not encode the URI properly.

Test Plan:
Ran `arc browse --branch x/y/z/ something.c`.

Before, got an error about "x" does not exist. This is wrong; the error should be about "x/y/z".

After, got the proper error:

{F1141096}

Reviewers: chad, avivey

Reviewed By: avivey

Maniphest Tasks: T10511

Differential Revision: https://secure.phabricator.com/D15397
2016-03-04 15:52:58 -08:00
epriestley
b1de04aa68 Report unit test details from Arcanist to Harbormaster
Summary: Ref T10457. Provides information for D15363.

Test Plan: See D15363.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9951, T10457

Differential Revision: https://secure.phabricator.com/D15364
2016-03-04 15:49:46 -08:00
epriestley
98d71571e4 Fix arc diff --raw with "onto" target properties
Summary:
Currently, `git show | arc diff --raw` and similar doesn't work because we try to figure out what the "Branch: feature (branched from whatever)" value is, which doesn't make sense.

```
$ git show | arc diff --raw --trace
 ARGV  '/Users/epriestley/dev/core/lib/arcanist/bin/../scripts/arcanist.php' 'diff' '--raw' '--trace'
 LOAD  Loaded "phutil" from "/Users/epriestley/dev/core/lib/libphutil/src".
 LOAD  Loaded "arcanist" from "/Users/epriestley/dev/core/lib/arcanist/src".
Config: Reading user configuration file "/Users/epriestley/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/Users/epriestley/dev/core/lib/arcanist/.arcconfig".
Working Copy: Path "/Users/epriestley/dev/core/lib/arcanist" is part of `git` working copy "/Users/epriestley/dev/core/lib/arcanist".
Working Copy: Project root is at "/Users/epriestley/dev/core/lib/arcanist".
Config: Reading local configuration file "/Users/epriestley/dev/core/lib/arcanist/.git/arc/config"...
Loading phutil library from '/Users/epriestley/dev/core/lib/arcanist/src'...
>>> [0] <conduit> conduit.connect() <bytes = 489>
>>> [1] <http> https://secure.phabricator.com/api/conduit.connect
<<< [1] <http> 211,217 us
<<< [0] <conduit> 212,001 us
>>> [2] <event> diff.didCollectChanges <listeners = 0>
<<< [2] <event> 140 us
>>> [3] <event> diff.didBuildMessage <listeners = 0>
<<< [3] <event> 46 us
Reading diff from stdin...
>>> [4] <conduit> differential.creatediff() <bytes = 10,542>
>>> [5] <http> https://secure.phabricator.com/api/differential.creatediff
<<< [5] <http> 120,215 us
<<< [4] <conduit> 120,411 us
>>> [6] <event> diff.wasCreated <listeners = 0>
<<< [6] <event> 41 us
 SKIP STAGING  Raw changes can not be pushed to a staging area.
>>> [7] <conduit> harbormaster.queryautotargets() <bytes = 290>
>>> [8] <http> https://secure.phabricator.com/api/harbormaster.queryautotargets
<<< [8] <http> 217,717 us
<<< [7] <conduit> 217,944 us
>>> [9] <conduit> harbormaster.sendmessage() <bytes = 274>
>>> [10] <http> https://secure.phabricator.com/api/harbormaster.sendmessage
>>> [11] <conduit> harbormaster.sendmessage() <bytes = 274>
>>> [12] <http> https://secure.phabricator.com/api/harbormaster.sendmessage
<<< [10] <http> 123,821 us
<<< [9] <conduit> 134,329 us
<<< [12] <http> 227,580 us
<<< [11] <conduit> 227,787 us

[2016-01-05 10:08:58] EXCEPTION: (Exception) This workflow ('ArcanistDiffWorkflow') requires a Repository API, override requiresRepositoryAPI() to return true. at [<arcanist>/src/workflow/ArcanistWorkflow.php:804]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=stable, ref.master=adb8a9c074ba, ref.stable=7b8d38cd2d4e)
  #0 ArcanistWorkflow::getRepositoryAPI() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2421]
  #1 ArcanistDiffWorkflow::getDiffOntoTargets() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2411]
  #2 ArcanistDiffWorkflow::updateOntoDiffProperty() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:534]
  #3 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:392]
```

Test Plan: Ran `arc diff --raw` in `phabricator/`.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14946
2016-01-05 10:16:39 -08:00
epriestley
8762e3f367 Use "--whitespace nowarn" in arc patch to respect trailing whitespace
Summary: Fixes T10008. Git tries to fix some issues by default (apparently? empirically; not consistent with documentation, I think?), but patches from `arc patch` are "always" accurate (disregarding other bugs we might have -- basically, they haven't been emailed or copy/pasted or anything like that) so we can just tell it to apply the patch exactly as-is.

Test Plan: {F1029182}

Reviewers: chad, joshuaspence

Reviewed By: chad, joshuaspence

Maniphest Tasks: T10008

Differential Revision: https://secure.phabricator.com/D14816
2015-12-17 17:36:26 -08:00