Summary:
When there is no active branch name, arc diff currently fails under PHP8 when we try to strlen(null).
This change is also credited to Evan from upstream Phabricator that applied the same change:
https://secure.phabricator.com/rARCc39ab20eb3717a15aed2467842bd77d9addce96a
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15412
Test Plan: Under PHP 8.1: ran git checkout <hash of head>, then arc diff to generate this revision.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15412
Differential Revision: https://we.phorge.it/D25237
Summary:
`array_fuse` in Arcanist is a wrapper for calling `array_combine($list, $list)`.
The latter doesn't accept passing `null` in PHP 8.2.
Going to `/conduit/method/project.create/`, entering a `name` but nothing as `members` (so we pass `null`), and calling this method, an exception is thrown.
Thus make `array_fuse` accept null and return an empty list in such cases.
Closes T15393
Test Plan: Applied this change; afterwards "Method Call Result" page at `/api/project.create` correctly displayed in the web browser.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15393
Differential Revision: https://we.phorge.it/D25228
Summary:
For some reason it may happen that a specific command line argument receives a null argument
from PHP, instead of just an empty string.
Nowadays, this null value probably breaks almost whatever GNU/Linux or FreeBSD or Microsoft Windows
etc. implementations since everyone expect to receive a string.
This used to work in the past since functions like strpos() or strlen() accepted null, but not
anymore. This generate a deprecation warning since PHP 8.1, that is elevated as exception from
Phabricator/Phorge and breaking features.
Without getting into implementation logics (which doesn't make sense to fix all of them) the
calling function should just be kind. So we normalize nonsense null values to an empty string.
Note: this was the expected behavior prior to PHP 8.1.
Now we do that normalization explicitly, in this early point.
After this fix, also T15368 should probably be fixed.
Closes T15367
Test Plan:
- run "arc patch <something valid>"
- to you it must continue to work
- (to @ton it starts working right now)
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno, ton
Maniphest Tasks: T15367
Differential Revision: https://we.phorge.it/D25205
Summary:
After PHP 8.1 the function `rawurlencode()` does not accept anymore the `null` value.
Thus return an empty string when the input parameter is null instead of passing the input parameter to `rawurlencode()`.
Closes T15263
Test Plan:
Applied this change on top of D25144, D25145, D25146, D25147, D25151,
D25152, D25153 and D25163 and already existing Workboard located at
`/project/view/1/` finally rendered in web browser.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15263
Differential Revision: https://we.phorge.it/D25164
Summary:
Passing `null` to the `$string` parameter of `mb_convert_case()` is deprecated in PHP 8.1.
This is one of the exceptions which block rendering the "Browse Projects" overlay dialog.
Closes part of T15335
Test Plan: Applied this change in Arcanist (plus the four changes in D25179 in Phorge) and the `Browse Projects` overlay dialog finally rendered in web browser and listed existing projects.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15335
Differential Revision: https://we.phorge.it/D25180
Summary:
Entering a malformed string (like `"pHoRgE rOcKs!!"`) into the "Raw Diff" field, Arcanist's `tryMatchHeader()` function is first called in `$ok = $this->tryMatchHeader($patterns, $line, $match)` with a non-null `$line` value (the first line entered in the "Raw Diff" field) being passed.
Afterwards, `tryMatchHeader()` is called for a second time after assigning `$line = $this->nextLineThatLooksLikeDiffStart()`.
This time `$line` is null and a RuntimeException is thrown, as `tryMatchHeader()` calls `preg_match()` which does not accept passing null as the $subject string parameter in PHP 8.1.
Thus add a `phutil_nonempty_string()` check if the `$subject` parameter (in this case, `$line`) is a non-empty string.
Arcanist's `tryMatchHeader()` function is not called outside of the file in which it is defined.
Thus catch the exception in the second call to `tryMatchHeader()` and not in the code of the `tryMatchHeader()` function itself.
Closes T15338
Test Plan: After adding the additional check, `/differential/diff/create/` showed the expected `Diff Parse Exception` instead.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15338
Differential Revision: https://we.phorge.it/D25183
Summary:
Conduit responds to requests with either `ERR-INVALID-SESSION` or `ERR-INVALID-AUTH` if the request wasn't sufficiently authenticated. Arcanist's `patch` workflow can automatically attempt to recover from situations in which Conduit responds to unauthenticated requests with `ERR-INVALID-SESSION` (by resending an authenticated version of the request), but not `ERR-INVALID-AUTH` - recover from `ERR-INVALID-AUTH` in the same way.
Closes T15333
Test Plan: The company I work for has been running a local clone of Arcanist containing this change in production for over 18 months now with no problems.
Reviewers: #blessed_committers, O1 Blessed Committers, valerio.bozzolan, avivey
Reviewed By: #blessed_committers, O1 Blessed Committers, valerio.bozzolan, avivey
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15333
Differential Revision: https://we.phorge.it/D25178
Summary:
This change fixes a RuntimeException for passing null to strlen() when setting up the DB host parameter
Closes T15260
Test Plan: I was able to run "./bin/config set mysql.host "localhost"" successfully
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: Ekubischta, goddenrich, Dylsss, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15260
Differential Revision: https://we.phorge.it/D25129
Summary:
In PHP 8.1, the strtolower() function no longer accepts NULL.
Without this change, getMatchSeverity() exits early if there is no severity found.
Closes T15257
Ref T15190
Test Plan: I ran this with an arc linter on a private repository which doesn't have a regex to match severity
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15257, T15190
Differential Revision: https://we.phorge.it/D25126
Summary:
This change fixes 'arc patch' in some circumstances.
Closes T15254
Test Plan: I was able to run "arc patch D25111" without issues
Reviewers: O1 Blessed Committers, Matthew
Reviewed By: O1 Blessed Committers, Matthew
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15187, T15254
Differential Revision: https://we.phorge.it/D25123
Summary:
Use the name "Phorge" as the defined platform.
Also prepare to rename the core library "phorge" rather then "phabricator" - see next diff.
T15006
Test Plan: Deployed change, tooltip for "Config" shows "Configure Phorge"
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew
Maniphest Tasks: T15006
Differential Revision: https://we.phorge.it/D25046
Summary:
Ref T13588. "arc-ls-markers" emits a "branch-state" marker so callers can identify which branch is active in the working copy.
This marker doesn't have an associated commit, so trying to generate a display name fails under stricter PHP 8.1 rules when we try to `substr(null, ...)`.
Don't attempt to generate a display name for markers with no commit hash.
Test Plan:
- Ran `arc branches` under PHP 8.1 in a Mercurial repository.
- Before: fatal.
- After: sensible output.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21825
Summary: Ref T13588. Additional PhutilURI fixes for PHP 8.1.
Test Plan: Ran "arc unit --everything" in Phabricator.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21822
Summary: Ref T13603. This is just a small piece of cleanup I've wanted to do for a while: different languages might have different list separators and repeating this implosion manually all over the place is a bit ugly even if the beahvior is never a function of translation language.
Test Plan: See next change.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21814
Summary:
Ref T13675. When a process daemonizes, it needs to close these actual file descriptors, so the calling context must provide them.
Elsewhere, modify the embedded "arc" to provide them. Then use them in place of reopening the streams.
Test Plan: Ran locally with embedded "arc", will deploy for production hangs.
Maniphest Tasks: T13675
Differential Revision: https://secure.phabricator.com/D21804
Summary:
Ref T13676. With increased PHP8.1 strictness around null, it seems reasonable to add some convenience parsing to "PhutilArgumentParser".
Add a helper function for parsing integers.
Test Plan:
See next change. Tried these arguments:
```
--count ''
--count asdf
--count 0
--count -3
--count 999999999999999999999999999999999999999999999234
--count 5
```
Got sensible parsing and appropriate feedback in all cases.
Maniphest Tasks: T13676
Differential Revision: https://secure.phabricator.com/D21799
Summary: Ref T13676. Ref T13588. These properties on PhutilURI have flexible types, just wrap them.
Test Plan: Built a new Drydock working copy with an HTTPS URI under PHP 8.1, see T13676.
Maniphest Tasks: T13676, T13588
Differential Revision: https://secure.phabricator.com/D21798
Summary:
Ref T13675. Ref T13556. The "STDOUT" and "STDERR" constants are defined by the PHP CLI SAPI, in `cli_register_file_handles()`.
The "native arc" embedded PHP wrapper doesn't define these, and there's no real reason to define them, since they're just defined in terms of the PHP stream wrappers ("php://stdin", etc) anyway.
This patch isn't exhaustive (and a subsequent change should add lint, rejecting these magic constants) but is just trying to make native `arc` functional.
Test Plan: Created this revision with a standalone native `arc` binary.
Subscribers: cspeckmim
Maniphest Tasks: T13675, T13556
Differential Revision: https://secure.phabricator.com/D21794
Summary:
Ref T13658. The lint rule called "getStringLiteralValue()", which produces string literals for fewer nodes than "evalStatic()".
Switch to "evalStatic()", then fix new warnings.
Test Plan:
This test plan is non-exhaustive.
- Ran "arc lint --everything --output summary" to generate new warnings.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21776
Summary:
Ref T13658. Remove all product name literals from "pht()" strings, by replacing them with generic text where that feels reasonably natural, or "PlatformSymbols" calls elsewhere.
These calls were identified with `arc lint --everything` after enabling the lint rule in D21763.
Test Plan: Read strings, ran "arc".
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21764
Summary:
Ref T13658. One challenge in forking Phabricator is product name references in user-visible strings, particularly in "pht()".
Add a linter to identify the use of product name literals in "pht()" text.
The linter could fix these automatically, but it looks like there are fewer than 200 across both Arcanist and Phabricator and some sampling suggests that many are probably clearer when rewritten into generic language anyway.
Test Plan: Ran linter, saw it pop out reasonable warnings.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21763
Summary: Ref T13588. Adds "phutil_nonempty_string()" and similar methods to support PHP8.1 compatibility.
Test Plan: Added unit tests, ran unit tests.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21762
Summary: Ref T13588. I pointed my local `php` at PHP 8.1 and this is what I've hit so far; all these cases seem very unlikely to have any subtle behavior.
Test Plan: Ran various `arc` workflows under PHP 8.1.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21742
Summary:
Ref T13588. "each()" has been a bad idea for a long time, and was formally deprecated in PHP 7.2 and removed in PHP 8.0.
Catch use of "each()" in lint and reject it.
Test Plan:
Added and ran unit tests.
{F10021268}
Subscribers: cspeckmim
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21741
Summary:
Ref T13588. See that task for discussion.
Improve behavior under PHP8.1, particularly the deprecation warning raised by calling `strlen(null)`.
Test Plan:
- Ran `arc help`, `arc branches`, `arc diff`, etc., under PHP 8.1 and PHP 7.4.
- Created this change with PHP8.1.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21740
Summary: Ref T13653. This entirely exists to support converging into the right swap state in deployment workflows.
Test Plan: Ran unit tests.
Maniphest Tasks: T13653
Differential Revision: https://secure.phabricator.com/D21733
Summary: Ref T13630. Piledriver cares about EBS error specifics; expand this class a bit to expose more error information.
Test Plan: Created and destroyed resource piles with Piledriver control code, elsewhere.
Maniphest Tasks: T13630
Differential Revision: https://secure.phabricator.com/D21732
Summary:
Author field is formatted with csprintf, which would be appropriate
if the resulting string was concatenated into a shell command as a
string -- but because the flags are passed as a vector of strings
and not parsed by the shell, this results in extraneous shell
quoting making it into to author field. In particular this
renders my name as D'\''Anna instead of D'Anna
Test Plan:
Performed 'arc patch' with and without these changes, confirmed
that my apostrophe was no longer mangled by shell quotes in the
resulting commit.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: MacFan4000, Ekubischta, speck, tobiaswiese, valerio.bozzolan
Differential Revision: https://we.phorge.it/D25026
Summary:
Refs T13665
In the update to D21716 the `str_starts_with` function was added which is only available in PHP 8. This changes it to use `strncmp()` which is compatible back to PHP 4.
Additionally fixed running into an error when trying to run `arc amend` on a commit which already matches the content known in Phabricator. Mercurial throws an error when running `amend` without file changes and using the exact same commit message it already has.
Test Plan:
I created a commit and revision then used `hg amend -e` to modify the commit message locally.
I used `arc amend` to update the commit message back to the appropriate message from Phabricator.
I ran `arc amend` again to verify that the command did not fail even though the commit message is unchanged.
I ran `arc inspect --explore -- 'commit(674492bb460)'` and verified it matched the appropriate commit, to verify the substring matching works properly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T13665
Differential Revision: https://secure.phabricator.com/D21723
Summary:
Mercurial does not have an implementation for querying commit symbol hardpoints, which is what the "arc amend" workflow uses.
This provides an implementation for Mercurial as well as updating `ArcanistMercurialAPI` to specify the current working directory symbol as `.`.
Additionally removed an erroneous early return in `ArcanistAmendWorkflow` which prevents a check against uncommitted changes.
Fixes T13665
Test Plan:
1. I created a diff on a Mercurial revision.
2. I updated the revisions summary in phabricator.
3. I ran `arc amend` and it successfully amended the local commit with the updated commit message.
4. I modified a file in the repository and left the change uncommitted.
5. I ran `arc amend` and verified that it reported an error due to uncommited commits.
I ran the following commands to verify that they resolved to the correct commits
1. `arc inspect --explore -- 'commit(674492bb460)'` properly matched the right commit as a commit hash prefix
2. `arc inspect --explore -- 'commit(674492bb4606666d5321feb38d2a467a8733c786)'` properly matched the right commit as a full commit hash
3. `arc inspect --explore -- 'commit(master)'` properly matched the right commit as a bookmark
4. `arc inspect --explore -- 'commit(tip)'` properly matched the right commit as a tag
5. `arc inspect --explore -- 'commit(.)'` properly matched the right commit as the working directory
6. `arc inspect --explore -- 'commit(cafe)'` properly matched the right commit as a commit hash prefix
7. I created a 'cafe' bookmark on a changeset
8. `arc inspect --explore -- 'commit(cafe)'` properly matched the right commit as a bookmark
9. `arc inspect --explore -- 'commit(67449)'` properly matched the right commit as a revision number
10. `arc inspect --explore -- 'commit(2147483648)'` properly did not match any revision (no python exception)
11. `arc inspect --explore -- 'commit(0)'` properly matched the first commit
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T13665
Differential Revision: https://secure.phabricator.com/D21716
Summary:
Reported at the phorge project (https://we.phorge.it/D25017), running `arc liberate` fails on PHP 8 due to the `log()` function using `fwrite()` incorrectly assuming a format pattern can be used.
This updates to remove most of these status messages are they are largely uninformative and instead we can report progress.
- Remove the `--quiet` argument
- Always display the progress
- Remove all informational/status log statements
Test Plan:
Tested using both PHP 7.3 and PHP 8:
1. I ran `arc liberate` and saw the standard output:
```lang=console
SCAN Searching for libraries in the current working directory...
WORK Updating library: src/
Done.
DONE Updated library.
```
2. I ran deleted `phabricator/src/.phutil_module_cache` and ran `arc liberate /src`, verifying that progress was displayed while the map was computed.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21718
Summary:
Ref T13666. Currently, "PhabricatorRepository->newConduitFuture()" sometimes returns a real "ConduitFuture" (when repository clustering is configured) and sometimes returns a degenerate "ImmediateFuture" (when repository clustering is not configured).
To populate the "ImmediateFuture", the Conduit method is called inline without any special exception handling. This means that the two pathways have significantly different exception behavior:
- On the "ImmediateFuture" pathway, the "newConduitFuture()" method itself may raise an exception related to resolving the call (e.g., invalid parameters).
- On the "ConduitFuture" pathway, exceptions are raised only once the future is "resolve()"'d.
The second behavior is the correct behavior (and the primary behavior of upstream test environments, since all my stuff and the Phacility stuff is clustered).
Prepare to put both pathways on the second behavior by introducing a "MethodCallFuture", which can move the `$conduit_call->execute()` call (and thus its exception handling) to future resolution time.
See also the followup diff in Phabricator to actually enact this change.
Test Plan: Added unit tests and made them pass, see also next change.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Maniphest Tasks: T13666
Differential Revision: https://secure.phabricator.com/D21720
Summary:
`arc liberate` has been broken for a while because the `log` function attempts to call `fwrite` (whose third argument is an integer length) instead of `fprintf`. I think maybe PHP 5 was more lenient about this? Anyway, this bug makes other development somewhat tricky.
The arcanist source in general seems to be split between `fwrite(STDERR, $message."\n")` and `fprintf(STDERR, "%\n", $message)`; I decided to go with the latter.
This is sort of a test balloon on submitting diffs to Phorge. Please feel free to let me know if there's anything you'd like done differently!
Test Plan: Ran `arc liberate` to regenerate source maps and didn't get any errors
Reviewers: O1 Blessed Committers, eax
Reviewed By: O1 Blessed Committers, eax
Subscribers: chris, speck, tobiaswiese
Differential Revision: https://we.phorge.it/D25017
Summary: Ref T13659. I don't think this name actually matters since Mercurial doesn't care what the extension name is, but be a little more consistent.
Test Plan: Ran "arc land ..." and saw it run "arc-hg" commands. There's no behavioral change here: Mercurial loads the extension and runs fine no matter what name we provide.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Maniphest Tasks: T13659
Differential Revision: https://secure.phabricator.com/D21711
Summary:
Refs T13659, D21697.
I suppose I missed these cases.
Test Plan:
- I ran `arc land --test` and verified the rebase used by `ArcanistMercurialLandEngine` was run with the flag to enable the extension.
```
$ hg --encoding utf-8 --config 'extensions.rebase=' rebase --dest 2fd68f4c48aaab9f6d5613b90e60b8219a9af0ae --rev 6132da63090c2c8a8d479aaae7b20497dda3e8fd..6132da63090c2c8a8d479aaae7b20497dda3e8fd --logfile - --keep --collapse
```
- I modified the merging to throw an exception before writing to the future's stdin and verified that after `arc` completed there was no rebase active.
- I set up 3 dependent diffs and landed the middle revision and verified the first and second revisions landed but not the tip revision and there were no errors and the resulting working directory was correct.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13659
Differential Revision: https://secure.phabricator.com/D21706
Summary: This short-lived function never did anything more interesting than "phlog()" and never found a role. The last callsite for this function was removed in D330 in 2011.
Test Plan: Grepped for "phutil_deprecated()", found no callsites.
Differential Revision: https://secure.phabricator.com/D21704
Summary: Fixes T13660. See also D21703. The most desirable modern API here is "resolve()", so deprecate the similar "execute()".
Test Plan:
- Grepped for callsites.
- Ran `arc patch --trace` in a Git working copy and saw the updated "git apply" in the trace output.
- Used this test script (changing the method and the command invoked) to confirm that success and error behavior is identical in "resolve()" and "execute()", except that "execute()" now emits a deprecation warning:
```
<?php
require_once 'support/init/init-script.php';
$err = id(new PhutilExecPassthru('lsx'))->execute();
var_dump($err);
```
Reviewers: cspeckmim
Reviewed By: cspeckmim
Maniphest Tasks: T13660
Differential Revision: https://secure.phabricator.com/D21705
Summary:
Currently there are a number of `--config extensions.blah=` sprinkled throughout the Mercurial APIs which aren't pleasant to look at. Additionally it turns out the `rebase` command requires the `rebase` extension which is not turned on by default. Uses of `rebase` should also be including this flag to enable the extension when it's in use.
This adds `ArcanistMercurialAPI::buildMercurialExtensionCommand()` which allows running a Mercurial command that requires an extension just by naming the extension instead of providing the full `--config..` argument.
It can be used e.g.
```lang=php
$api->execxLocalWithExtension(
'strip',
'strip --rev %s --',
$rev);
$api->execxLocalWithExtension(
'arc-hg',
'arc-amend --logfile %s',
$tmp_file);
```
Which produces
```lang=console
$ hg --encoding utf-8 --config 'extensions.strip=' strip --rev 82567759e2d703e1e0497f5f41363de3a1500188 --
$ hg --encoding utf-8 --config 'extensions.arg-hg=/Users/cspeckrun/Source/phacility/arcanist/support/hg/arc-hg.py' arc-amend --logfile /private/var/folders/cg/364w44254v5767ydf_x1tg_80000gn/T/6bwck66ccx0kwskw/89989-5F8JaL
```
Refs T13659
Test Plan: I ran through several scenarios of running `arc diff` and `arc land` with uncommitted changes on non-head commits, while not having the `evolve` extension enabled. Using the `--trace` argument I verified that `rebase`, `strip`, `arc-amend`, and `arc-ls-markers` were all invoked and the corresponding `arc diff` and `arc land` commands succeeded. I also ran `arc land --onto-remote default` while `arc-hg.py` raised an exception for the "remote" case and verified that the error was caught by Arcanist and displayed the error and prevented further execution. I removed the error from `arc-hg.py` and re-ran the command and verified it succeeded.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T13659
Differential Revision: https://secure.phabricator.com/D21697
Summary:
Update `PhutilInteractiveEditor` to allow specifying a "task message" which will be displayed just prior to launching the user's editor.
Refs T3271
Test Plan: I ran several `arc diff` commands in varying states to invoke my editor and verified that it printed out the text indicating that my editor was being launched.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T3271
Differential Revision: https://secure.phabricator.com/D21700
Summary:
After `arc diff` creates a revision in Phabricator it amends the commit to include a link to the revision in the commit message. For Mercurial this is done with `hg commit --amend --logfile` however this will fail when trying to create a diff for a non-head commit.
This updates `ArcanistMercurialAPI::amendCommit()` to allow amending a non-head commit in two ways, depending on whether `evolve` is in use:
No evolve:
1. Rebasing the current commit onto the current commit's parent, using the new commit message
2. Rebasing all children + descendants of the current commit onto the new resulting commit
3. Stripping the original commit
With evolve:
1. Amend the commit with `hg amend --logfile`
2. Run `hg evolve` to tidy up all commits
Test Plan:
I created 6 commits in a row placing a bookmark at commits 2 `bookmark1`, 4 `bookmark2`, and 6 `bookmark3`, and ensured I had `arc:bookmark` in my base ruleset.
No evolve, non-head changeset:
1. I verified I did not have `evolve` enabled by running `hg debugextensions` and did not see `evolve` in the listed active extensions.
2. I updated to `bookmark1` and modified a file to leave a dirty working state.
3. I ran `arc diff` and when prompted to amend my changes I said "yes", and verified a phab revision was created properly.
4. I checked the status of my repository and verified it was still linear and the bookmarks pointed to the proper commits.
5. I ran `hg log -r bookmark1 --template {desc}` to view the full commit message and verified it contained both `Summary: ...` and `Differential Revision: https://...`.
6. I ran `hg diff -c bookmark1` and verified the changes for that commit included the changes I made in step 2.
No evolve, head changeset:
1. I updated to `bookmark3` which is the head commit and modified a file to leave a dirty working state.
2. I ran `arc diff` and when prompted to amend my changes I said "yes", and verified a phab revision was created properly.
3. I checked the status of my repository and verified it was still linear and all the bookmarks pointed to the proper commits.
4. I ran `hg log -r bookmark3 --template {desc}` to view the full commit message and verified it contained both `Summary: ...` and `Differential Revision: https://...`.
5. I ran `hg diff -c bookmar3` and verified the changes for that commit included the changes I made in step 1.
With evolve:
1. I enabled `evolve` and verified it was enabled by running `hg debugextensions` and saw `evolve` in the listed active extensions.
2. I updated to `bookmark2` and modified a file to leave a dirty working state.
3. I ran `arc diff` and when prompted to amend my changes I said "yes", and verified a phab revision was created properly.
4. I checked the status of my repository and verified it was still linear and all the bookmarks pointed to the proper commits.
5. I ran `hg log -r bookmark2 --template {desc}` to view the full commit message and verfieid it contained both `Summary: ...` and `Differential Revision: https://...`.
6. I ran `hg diff -c bookmark2` and verified the changes for that commit included the changes I made in step 2.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21686
Summary:
Refs T13546
**Behavior Changes**
1. Currently, after landing the `--onto` bookmark will not be advanced to the newly pushed commit(s)
- This updates so that after pushing commits upstream, onto-marker bookmarks are pulled back from the server to retrieve their updated state
2. Currently, after landing the working directory will typically be the old `--onto` commit
- This updates the behavior in the following ways
1. If the starting working state is a commit that is part of a revision being landed, the post-land state will be the newly published commit for that revision
2. If the starting working state is a commit for a tip revision being landed and there is only a single `--onto` target, the post-land state will be the newly published commit and the `--onto` bookmark will be activated, if applicable. If there are multiple `--onto` targets defined (uncommon) then the resulting working state will be the same behavior as before, as the desired behavior for this case is ambiguous.
3. If the starting working state is a commit that is not part of any revision being landed, then the post-land state will be that same commit, activating the same previous bookmark if applicable.
**Bugs Fixed**
1. When landing a diff that includes multiple commits, where the non-tip commit adds a file and later commit modifies that file, the land process would fail during rebasing.
2. When landing, the display of what commits are going to be landed only includes the commit hash but should include part of the commit message for easy identification.
3. If errors occur during a rebase while landing, the resulting state is an in-progress rebase. Users will typically not be able to resolve this in-progress rebase and possibly confuse them further as they have to first abort the rebase before trying to clean up. These rebases will now be aborted by arcanist before exiting.
4. If using evolve, landing a non-tip revision would leave behind orphaned commits.
Test Plan:
Bookmark test
- I created a diff with bookmark `test`, as a branch from before the `master` commit
- I updated working state to activate `test`
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit
Non-bookmark test
- I created a diff with bookmark `test`, as a branch from before the `master` commit
- I updated working state to be the new commit, but not activate `test` even though it points to that same commit
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit
Not-working state test
- I created a diff with bookmark `test`, as a branch from before the `master` commit
- I updated working state to an older commit not related to the one being landed
- I landed that diff
- I verified the ending resulting working state was on the older commit I was on prior to landing
Multiple commits test
- Using `test` bookmark I created a commit that added a new file, made a diff, made another new commit on top of it which modified that file, then updated the diff
- I updated my working state to the first commit for the diff (non-head)
- I ran `arc land test` to land the diff
- I verified that the resulting working state was on the newly published `master` commit
Landing while on `master`
- I created a diff with a bookmark `test`, as a branch from before the `master` commit
- I updated working state to the `master` bookmark
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit
Landing with no local `master` bookmark
- I created a diff consisting of two commits with bookmark `test`, as a branch from before the `master` commit
- I left working state on `test` and deleted my local `master` bookmark
- I landed that diff
- I verified that the resulting working state was on the newly published `master` commit and the `master` bookmark was pulled and active
Cascading diffs while on an earlier diff changeset
- I setup two diffs, each consisting of two commits, one depending on the other
- I put the working state onto a commit from the earlier diff
- I landed the later diff
- I verified that the resulting working state was on the newly published commit associated with the earlier diff, which is not the new `master` commit so there was no active bookmark
Cascading diffs while on the later diff changeset
- I setup two diffs, each consisting of two commits, one depending on the other
- I put the working state into a commit from the later diff
- I landed the later diff
- I verified that the resulting working state was on the newly published commit for the later diff which was `master` and `master` was active
Landing a non-tip revision, with evolve
- I setup three diffs, each consisting of two commits, creating a dependency chain
- I verified I had the evolve extension enabled via `hg debugextensions`
- I put the working state on a commit in the first revision
- I landed the second revision
- I verified that the resulting working state was clean, that revisions 1 and 2 were properly landed and published, and the resulting working state was on the published commit for the first revision.
Landing a non-tip revision, without evolve
- I setup three diffs, each consisting of two commits, creating a dependency chain
- I verified I did not have the evolve extension enabled via `hg debugextensions`
- I put the working state on a commit in the first revision
- I landed the second revision
- I verified that the resulting working state was clean, that revisions 1 and 2 were properly landed and published, and the resulting working state was on the published commit for the first revision.
Landing a non-tip revision, while the non-landing revision is active
- I setup three diffs, each consisting of two commits, creating a dependency chain
- I put the working state on the latest tip revision commit and its bookmark
- I landed the second revision
- I verified that revisions 1 and 2 were properly landed and published, and the resulting working state was still on the latest tip revision commit and its bookmark was active.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T13546
Differential Revision: https://secure.phabricator.com/D21680
Summary:
Refs D21679 (phabricator changes)
This updates Arcanist to be able to check whether the version of Mercurial supports using `{p1.node}` template format vs. `{p1node}`.
Test Plan: Tested under coverage from D21679
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21681
Summary:
When non-ascii characters appear in revision titles/summaries the `patch` and `diff` (to update) commands will fail on Windows systems. This often occurs due to “smart quotes” or "em—dash" characters being inserted into commit messages by editors on "user-friendly" operating systems like macOS.
This can be worked around by forcing all mercurial commands to use the global option `--encoding utf-8` which applies for any mercurial command. This option was [[ https://www.mercurial-scm.org/repo/hg/rev/a88e02081a88 | added in ~2006 ]] so this should work across all supported versions of mercurial.
Refs T13649
Test Plan:
I created a diff on a mercurial repository using smart quotes in the "Title" and "Summary" fields as well as in the content of a file being changed. Then on macOS, Windows (PowerShell), and Windows (cmd.exe) I was able to `patch` down the revision, make a modification, and `diff` the change back up to Phabricator, as well as `land` the change. I verified the commit and content looked correct on macOS as well as on Windows by using `nvim` which seems to properly detect and render the encoding, whereas mercurial displays the smart quotes and em-dashes with odd characters instead.
I did a grep through Arcanist codebase to find other places where `--encoding` might be specified for mercurial commands and could not find any. In the event that somehow this argument is added elsewhere I verified that multiple specifications of `--encoding utf-8` does not cause any issues and the later specification of `--encoding` appears to "win".
```lang=console
$ hg --encoding utf-8 --encoding utf-8 log -r tip
# prints out results in UTF-8 without issue
$ hg --encoding utf-8 log --encoding latin-1 -r tip
# prints out results in latin-1 without issue
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13649
Differential Revision: https://secure.phabricator.com/D21676
Summary:
Ref T13649. Currently, "arc" may leave stdin nonblocking after showing a prompt. This can cause various odd behaviors down the line.
I can't immediately reproduce this behavior on macOS in "zsh" or "bash" (I'm unable to get stdin to remain nonblocking beyond the process lifespan), and also don't have pcntl locally so there's a fair amount of handwaving here.
Test Plan: This is somewhat speculative since I can't immediately reproduce the behavior. I tested the locally-reachable paths (no pcntl) but they're not interesting.
Maniphest Tasks: T13649
Differential Revision: https://secure.phabricator.com/D21666
Summary:
Ref T13562. Currently, "Filesystem::copyFile()" uses "copy", which doesn't work now that we no longer invoke "cmd.exe" by default.
Use "copy()" instead.
Note that this whole function is probably nonsense, but I'll follow up on T13562.
Test Plan:
- Created a standalone script which runs "Filesystem::copyFile()".
- Before: failed to copy any file.
- After: succesfully copied normal files.
- After: failed to copy a file over an existing directory with a reasonable error.
- After: failed to copy a file over itself with a reasonable error.
Maniphest Tasks: T13562
Differential Revision: https://secure.phabricator.com/D21643
Summary: See PHI2027. This example command is missing "--", but it's required.
Test Plan: Ran the new command, no longer got an error about "--".
Differential Revision: https://secure.phabricator.com/D21623
Summary: See PHI2022. This link is missing the `/u/` part and currently 404's.
Test Plan: Followed the new link, got documentation.
Differential Revision: https://secure.phabricator.com/D21611
Summary:
Ref T13635. PHP native JSON functions sometimes represent JSON objects as PHP "stdClass" objects.
Accept this representation and emit it correctly in "PhutilJSON".
Test Plan: Added a test, made it pass.
Maniphest Tasks: T13635
Differential Revision: https://secure.phabricator.com/D21604
Summary: The ".git/arc" directory may need to be created when writing to working copy configuration.
Test Plan:
- Tried to store an answer to a prompt in a new working copy.
- Before: error that ".git/arc" does not exist.
- After: prompt saved to working copy configuration.
Differential Revision: https://secure.phabricator.com/D21588
Summary:
Ref T13576. See that task for discussion.
When a user runs `arc land --pick A`, they may be selecting a range of commits ("X..Y") which have ancestors ("V..W") that should NOT land.
We must slice "X..Y" out of history before we can merge it, to avoid landing changes from "V..W".
When "X..Y" is simple and linear, we can rebase the range to pick our desired slice out of history.
When "X..Y" includes merge commits, we frequently can not, and I could not identify any simple alternative. The best alternative I came up with is this "reduce" operation:
- squash "into" onto Y, producing S, to guarantee there are no natural conflicts;
- squash S onto X^, producing T, to get rid of the merge commits;
- rebase T onto "into", producing R, to slice "X..Y" out of history;
- squash R onto "into", producing Q. (R and Q will be the same, but this simplies the code.)
This feels flimsy and fragile, but I can't immediately find a way to break it. See T13576 for more discussion.
Test Plan:
- Applied conflicting changes to `example.txt` in `master` and `feature1`.
- Ran `arc land`, got a merge conflict.
- Resolved the conflict with `git merge master`.
- Ran `arc land`.
- Before: merge conflict.
- After: `arc land` resolves the merge correctly.
- Stacked `feature2` on `feature1`, and made various mutations to `feature1` and `feature2`, then ran `arc land --pick feature2`. Changes made in `feature1` should not land, and they mostly do not. See T13576.
Maniphest Tasks: T13576
Differential Revision: https://secure.phabricator.com/D21590
Summary:
Ref T13576. The "implicit commits" prompt in "arc land" shows a list of implicit and non-implicit commits.
The implicit commits are marked with a background color, but this doesn't survive if you copy/paste the output into a support ticket.
Make my life easier by also marking commits so the marker survives copy/paste.
Test Plan: Ran "arc land" with implicit commits, saw a copy-pastable indicator.
Maniphest Tasks: T13576
Differential Revision: https://secure.phabricator.com/D21589
Summary:
Ref T13624. If we want to send PHP errors to a log, using the "error_log" configuration option catches the broadest set of errors across versions of PHP.
Configuring this disables errors on `stderr`, since they're sent to the log instead. We'd like them to go to both places; provide a simple wrapper for this. Also do a bit of writability testing.
Test Plan: Wrote errors to a new log, see followup changes.
Maniphest Tasks: T13624
Differential Revision: https://secure.phabricator.com/D21578
Summary: Ref T13608. Ref T13100. Ref T13586. Properly checking "preg_match()" and similar calls for failure and raising useful exceptions is complicated and error-prone. Provide wrapper functions with an API that's more consistent with the rest of the codebase: matches are returned; and errors raise detailed exceptions.
Test Plan: See next change.
Maniphest Tasks: T13608, T13586, T13100
Differential Revision: https://secure.phabricator.com/D21561
Summary:
These are the instructions from https://github.com/golang/lint.
The fetch location changed to `golang.org/x/lint/golint` from `github.com/golang/lint/golint`.
`-u` tells go to update the package and its deps if they exist.
`-u` Shouldn't strictly be necessary, but figured we might as well follow the instructions from `golint`.
Test Plan:
Enable golint without having it installed.
Ensure that the install instructions now show the new location.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21552
Summary:
Ref T13588. For consistency of behavior between versions on either side of PHP7, any "catch (Exception)" block should generally have a "catch (Throwable)" block in the same catch list.
Raise a lint warning when "Exception" is caught without also catching "Throwable", since this is almost certainly not desired.
Test Plan: Added tests.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21542
Summary: Ref T13588. See D21500. This syntax is unusual and there are some hidden complexities involved; annotate them. See D21500 for more discussion.
Test Plan: Read text, reviewed D21500.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21541
Summary:
Ref T13588. Marking a method "final private" has never been meaningful, and is an error in PHP8.
Add static analysis to detect (and correct) this issue.
Test Plan: Added unit tests, will lint "phabricator/".
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21539
Summary: Ref T13598. This builds on D21537 and adds support for correcting the capitalization of builtin systems.
Test Plan:
- Linted a file that uses "ExCePtIoN", got a lint correction.
Maniphest Tasks: T13598
Differential Revision: https://secure.phabricator.com/D21538
Summary:
Ref T13598. If you spell a symbol like "Polygon" as "PoLyGoN", you currently get an "unknown symbol" lint message. However, provided "Polygon" is a valid symbol, we can unambiguously correct the spelling of the symbol.
Note that this patch can only correct the spelling of application symbols, not builtin symbols (since none of the library maps contain builtin symbols).
Test Plan: {F8374599}
Maniphest Tasks: T13598
Differential Revision: https://secure.phabricator.com/D21537
Summary: Ref T13589. See that task for discussion.
Test Plan:
- Created this diff, ran most commands in isolation.
- This change is difficult to test extensively.
Maniphest Tasks: T13589
Differential Revision: https://secure.phabricator.com/D21509
Summary:
Since 2.1.0 (commit 75bc0c03c145), pyflakes has included the Python
version and platform in its version output, so ignore it if present.
Since 2.2.0 (commit 6ba3f8e0b59b), pyflakes has included the column
number in its messages, so update the parser to include it and drop the
column number from the (only) test in order to work with both old and
new versions. Whilst here, assign names to the capture groups to make
the code clearer.
Test Plan: Ran arc unit
Reviewers: epriestley, joshuaspence, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21504
Summary:
Recent JSHint improves the warning and attributes it to the equals sign
rather than the end of the expression (changed in 897e0359ce19, first
released in 2.11.0-rc1).
Test Plan: Ran arc unit with JSHint 2.12.0
Reviewers: epriestley, joshuaspence, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21503
Summary:
PHP 8 deprecates openssl_free_key as the key is automatically freed, so
silence the warning in PhutilOAuth1Future::signString.
Test Plan: Ran arc lint --everything
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21502
Summary:
In PHP 8 passing an invalid encoding to mb_convert_encoding raises a
ValueError (which extends Error not Exception), so fix the test to also
catch Throwable (but leave the explicit Exception case for PHP 5, which
lacks Throwable).
Test Plan: Ran arc unit --everything
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21501
Summary:
Calling 'Foo::bar' is only supported since PHP 7, whereas the array form
is supported since PHP 5.4, which is below our PHP 5.5 baseline.
Test Plan: No regressions under PHP 8 and snippet tested on 3v4l
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21505
Summary:
PHP 8's sprintf raises a ValueError when encountering unknown format
specifiers (previously it would eat the argument and print nothing), so
linting format strings like %Ls dies with an uncaught ValueError.
Fix this by using a custom callback during linting to turn all format
specifiers into %s and replace the dummy null argument with the original
format specifier, ensuring we always end up providing valid input to the
sprintf at the end. This has the nice property that the output of the
call to xsprintf is the original format string, though any
transformation into valid input would do.
Test Plan: Ran arc lint
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21500
Summary:
In previous versions, passing the wrong type to preg_match would give a
warning that could be suppressed by @ and caught by set_error_handler,
but as of PHP 8 this raises a TypeError and so remains uncaught. Thus
check up-front whether the provided value is a string.
This fixes linting arc itself when run with PHP 8, as includes and
excludes use "optional regex | list<regex>", so would previously try to
pass an array to preg_match for the first alternative and die.
Test Plan: Ran arc lint
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21499
Summary:
PHP 7.2.0 deprecated the 5th parameter and PHP 8 removed it, so stop
using it and provide a default value to avoid erroring with:
```
Too few arguments to function PhutilErrorHandler::handleError(), 4 passed and exactly 5 expected
```
Test Plan: Used to create this revision with PHP 8 on macOS
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21498
Summary:
This combination does not make sense and PHP 8 errors with:
```
Private methods cannot be final as they are never overridden by other classes
```
Thus remove the redundant final from all such functions.
Test Plan: Used to create this revision with PHP 8 on macOS
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21496
Summary:
See <https://discourse.phabricator-community.org/t/search-by-name-in-files-doesnt-support-number/4300>.
I can't exactly reproduce the original issue, but when a query like "quack 1234" is tokenized, we end up calling "phutil_utf8v(1234)", where the argument is an integer.
At least in recent versions of PHP, this fatals ("trying to access an offset of an integer"). Cast the argument first.
Test Plan: Searched for "quack 1234" in Files. Before: fatal accessing offset of integer; after: correct results.
Differential Revision: https://secure.phabricator.com/D21477
Summary: Fix tests to work with rubocop 0.92.0 released on September 25, 2020
Test Plan: Unit tests pass
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21474
Summary:
We are having issues where people run out of file descriptors and the first `git push` will succeed, but the
second one will not. We'd like the diff to not be created in this case as it leads to weird behavior like our tests
running against 0 changed files.
Test Plan: none
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21471
Summary:
See PHI1862. This code calls "execute()" on the future directly, but that skips some steps -- notably, ServiceProfiler hooks.
Call "resolve()", which has the same effect but includes desirable/expected side effects.
Test Plan: Changed a workflow to run "phutil_passthru('ls')", ran it with "--trace". Before: no execution in trace; after: execution in trace.
Differential Revision: https://secure.phabricator.com/D21470
Summary: Ref T13582. When this code is reached with a raw HTTP exception, it currently fatals.
Test Plan:
- Ran `arc branches --conduit-uri=http://example.org` (a bad Conduit URI).
- Before: hard fatal with a bad method call.
- After: non-Conduit exception raised to user. Not ideal, but a step forward.
Maniphest Tasks: T13582
Differential Revision: https://secure.phabricator.com/D21467
Summary:
See T13572. FutureIterator does not release futures, so long-lived iterators (like the one that FuturePool may build) can end up leaking memory.
This affects the FuturePool used by the daemon overseer.
See T13572 for more discussion.
Test Plan:
- Ran the simple FutureIterator script from T13572. Before: memory held during iteration, script grows without bound. After: memory released, script uses stable memory.
- Ran the overseer with memory logging and an immediate wakeup from hibernation. Before: saw memory usage grow without bound at a rate of ~300MB/day. After: saw memory usage stable.
Differential Revision: https://secure.phabricator.com/D21466
Summary: Ref T13581. This message can be slightly more helpful in some cases by showing which method call failed.
Test Plan: Ran `arc branches` under the error condition in D21462, got a more useful error.
Maniphest Tasks: T13581
Differential Revision: https://secure.phabricator.com/D21463
Summary:
Ref T13577. After the lint rule fix in D21453, it can identify more errors. Fix the errors it identifies in "arcanist/".
These all seem fairly obscure/benign.
Test Plan: Ran `arc lint` on the files before and after these changes. Did not specifically re-test these particular messages, but they mostly very obscure.
Maniphest Tasks: T13577
Differential Revision: https://secure.phabricator.com/D21456
Summary:
Ref T13577. I'd like to `arc lint --everything` to find other bad calls to `pht()` and similar functions, but `n_HEREDOC` nodes currently can not generate a response to "getStringLiteralValue()".
Support literal extraction from heredocs.
Test Plan: Added a test, made it pass. Will lint everything.
Maniphest Tasks: T13577
Differential Revision: https://secure.phabricator.com/D21455
Summary: Ref T13577. This call is missing a parameter. After D21453, this is detected properly by lint. Provide the parameter.
Test Plan: Ran `arc lint` on HTTPSFuture before and after the change.
Maniphest Tasks: T13577
Differential Revision: https://secure.phabricator.com/D21454
Summary:
Ref T13577. This lint rule correctly detects the error in `pht('x %s y')` but the narrow test for `n_STRING_SCALAR` prevents it from detecting the error in `pht('x %s y'.'z')`.
Make the test broader.
Test Plan:
- Ran `arc lint` on `HTTPSFuture.php`, got a detection of the issue in T13577.
- Added a failing test and made it pass.
Maniphest Tasks: T13577
Differential Revision: https://secure.phabricator.com/D21453
Summary:
Ref T13555. Currently:
- If an exception is raised in "start()", the exception state is not set on the future.
- Futures do not always call "startFuture()" before starting, and do not always call "endFuture()" once they become resolvable.
- If you start an ExecFuture which immediately fails and then call "getPID()" on it, you get an unclear exception.
Simplify these behaviors:
- In FutureIterator, only start futures which have not already started.
- When starting a future on any pathway, run start code.
- When a future becomes resolvable on any pathway, run end code.
- Raise a more clear exception when calling "getPID()" on a future with no subprocess.
Test Plan: Faked a failing subprocess with "$proc = null", ran "bin/phd debug taskmaster" etc. Got clearer errors and more consistent future lifecycle workflows.
Maniphest Tasks: T13555
Differential Revision: https://secure.phabricator.com/D21423
Summary:
See PHI1808. Currently, "arc land <some bookmark>" does not destroy the bookmark in Mercurial. There are three issues here:
- "hg rebase --keep --collapse" moves bookmarks to the rewritten commits;
- "hg strip" moves bookmarks backwards;
- "hg prune" moves bookmarks backwards.
To get around "hg rebase", save and restore bookmark state.
To get around "hg strip" and "hg prune", explicitly destroy bookmarks pointing at commits before we strip/prune those commits.
Test Plan:
- Ran "arc land <some bookmark> --trace". Saw arc reset the bookmark position after rebasing, and destroy the bookmark explicitly before stripping.
- When the workflow exited, saw no more bookmark (previously: bookmark existed and pointed at a possibly-intermediate state).
Differential Revision: https://secure.phabricator.com/D21397
Summary:
Ref PHI1808. In Mercurial, we must save and restore bookmark state explicitly.
- Save and restore bookmarks.
- Clean up concepts in "arc-ls-markers" slightly, so we don't need separate "isCurrent" and "isActive" flags, hopefully.
I believe the totality of Mercurial state is:
- A (non-bare) working copy points at exactly one commit (which might be the empty/null commit, in an empty repository).
- A working copy has exactly one active branch.
- Each branch has zero or more heads.
- Each head may be closed.
- Each (non-null) commit belongs to exactly one branch.
- Note that the active branch may have zero heads and zero commits which belong to it!
- A working copy has zero or one active bookmark.
To capture this, we now emit:
- A list of branch heads. If a branch head is a working copy commit, that head is flagged as active.
- A list of bookmarks. If a bookmark is the current bookmark, that bookmark is flagged as active.
- A single "branch-state" virtual marker. This covers the case where you have run "hg branch X" to create X, but no objects in the working copy actually correspond to X yet. It also covers the case where you are on a concrete branch, but not any head of that branch.
- A single "commit-state" virtual marker. This always shows the current commit in the working copy.
Test Plan:
- Useful states to test are:
- Empty repository (not all commands currently work here).
- Normal repository, on a bookmark.
- Normal repository, no bookmark.
- "hg up 123" to update to somewhere in history.
- "hg branch X", to start a new branch with no commits.
- Ran "arc branches" and "arc bookmarks" in various states. Saw generally sensible output.
- Ran "arc land --hold ..." in various states against a failing remote. Saw generally sensible output, and saw working properly restored to the original state.
Differential Revision: https://secure.phabricator.com/D21396
Summary: Ref PHI1808. Currently, push failures are messaged awkwardly. Make this exception handling more selective and the user-facing behavior more readable.
Test Plan: Ran "arc land" against a failing remote, saw a human-readable message instead of a stack trace.
Differential Revision: https://secure.phabricator.com/D21395
Summary: See PHI1808. Some refactoring of the "passthru" API resulted in error conditinos here being dropped. Instead, raise them as exceptions.
Test Plan: Forced "hg push" to fail, used "arc land" against a failed push, saw error behavior instead of "success" feedback.
Differential Revision: https://secure.phabricator.com/D21394
Summary:
Ref PHI1805. Under some combination of versions (Python 3.8?), "arc-ls-markers" is running into additional Python runtime issues.
Sprinkle more "b" around to resolve them? Also clean up a couple of plain "arc" issues.
Test Plan:
Landed a change in Mercurial.
Some of this works fine without changes in Python 3.7/2.7 against Mercurial 4.7/5.4, so this may not be exhaustive.
Differential Revision: https://secure.phabricator.com/D21393
Summary: See PHI1805. This call is constructed improperly and can lead to a fatal in `arc patch` under Mercurial.
Test Plan: In Mercurial, ran a valid `arc patch` operation.
Differential Revision: https://secure.phabricator.com/D21391
Summary:
Ref T13546. Try using unicode box drawing characters to render a more obvious tree struture in "arc branches".
Unclear if this has enough support to use, but seems okay so far.
Test Plan: Ran "arc branches", saw a nicer tree display.
Maniphest Tasks: T13546
Differential Revision: https://secure.phabricator.com/D21390
