Updated the forked version #1
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…orts after recent changes Summary: Ref T13303. I upgraded this to a vector-based sort but forgot to type a "v", which means the sort has different stability under PHP 5.5. See D20582 for a root cause fix. Test Plan: Locally, on PHP7, not much changes. I expect this to fix the odd selection of title stories in mail and notification stories on `secure`, which is running PHP 5.5. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13303 Differential Revision: https://secure.phabricator.com/D20583
Summary: Ref T13303. See B22967. This should be "msortv()" but didn't get updated properly. Test Plan: The system works! Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13303 Differential Revision: https://secure.phabricator.com/D20585
Summary:
Fixes T13313. The "Download Raw Diff" workflow in Differential currently uses an older way of interacting with Files that doesn't engage the chunk engine and can't handle 8MB+ files.
Update to `IteratorFileUploadSource` -- we're still passing in a single giant blob, but this approach can be chunked.
This will still break somewhere north of 8MB (it will break at 2GB with the PHP string limit if nowhere sooner, since we're putting the entire raw diff in `$raw_diff` rather than using a rope/stream) but will likely survive diffs in the hundreds-of-megabytes range for now.
Test Plan:
- Added `str_repeat('x', 1024 * 1024 * 9)` to the `$raw_diff` to create a 9MB+ diff.
- Configured file storage with no engine explicitly configured for >8MB chunks (i.e., "reasonably").
- Clicked "Download Raw Diff".
- Before: misleading file storage engine error ("no engine can store this file").
- After: large, raw diff response.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13313
Differential Revision: https://secure.phabricator.com/D20579
…users with no Spaces can log out Summary: Fixes T13310. Use cases in the form "users with no access to any spaces can not <do things>" are generally unsupported (that is, we consider this to mean that the install is misconfigured), but "log out" is a somewhat more reasonable sort of thing to do and easy to support. Drop the requirement that users be logged in to access the Logout controller. This skips the check for access to any Spaces and allows users with no Spaces to log out. For users who are already logged out, this just redirects home with no effect. Test Plan: - As a user with access to no Spaces, logged out. (Before: error; after: worked). - As a logged-out user, logged out (was redirected). - As a normal user, logged out (normal logout). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13310 Differential Revision: https://secure.phabricator.com/D20578
…ommits Summary: Ref T13311. We currently don't use committer identity mappings when triggering audits, so if a user is only associated with an identity via manual mapping we won't treat them as the author. Instead, use the identity and manual mapping if they're available. Test Plan: - Pushed a commit as `xyz <xyz@example.org>`, an address with no corresponding user. - In the UI, manually associated that identity with user `@alice`. - Ran `bin/repository reparse --publish <hash>` to trigger audits and publishing for the commit. - Before: observed the `$author_phid` was `null`. - After: observed the `$author_phid` is Alice. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13311 Differential Revision: https://secure.phabricator.com/D20580
Summary: Depends on D20580. Fixes T13311. When we choose which actions to show a user, we can either show them "auditor" actions (like "raise concern") or "author" actions (like "request verification"). Currently, we don't show "author" actions if you're the author of the commit via an identity mapping, but we should. Use identity mappings where they exist. (Because I've implemented `getEffectiveAuthorPHID()` in a way that requires `$data` be attached, it's possible this will make something throw a "DataNotAttached" exception, but: probably it won't?; and that's easy to fix if it happens.) Test Plan: See D20580. As `@alice`, viewed the commit in the UI. - Before: got auditor actions presented to me. - After: got author actions. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13311 Differential Revision: https://secure.phabricator.com/D20581
Summary: Ref T13305. See that task for discussion. This old migration may indirectly cause search index worker tasks to queue by loading handles. They'll fail since we later added `dateCreated` to the worker task table. Use `needHandles(false)` (since we don't use them) to disable loading handles and avoid the problem. Test Plan: - Ran `bin/storage upgrade -f` on an older instance (late 2016) and hit this issue. - Applied the patch, got a clean migration to modernity. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13305 Differential Revision: https://secure.phabricator.com/D20570
…ailureException" Summary: Fixes T13315. See that task for discussion. Without `--background`, we currently treat this as a catastrophic failure, but it's relatively routine for some repository states. We can safely continue reparsing other steps. Test Plan: Ran `bin/repository reparse --all X --message` with commits faked to all be unreachable. Got warnings instead of a hard failure on first problem. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13315 Differential Revision: https://secure.phabricator.com/D20588
Summary: See <https://discourse.phabricator-community.org/t/unhandled-exception-when-logging-in-with-mfa/2828>. The recent changes to turn `msort()` on a vector an error have smoked out a few more of these mistakes. These cases do not meaningfully rely on sort stability so there's no real bug being fixed, but we'd still prefer `msortv()`. Test Plan: Viewed MFA and External Account settings panels. Did a `git grep 'msort(' | grep -i vector` for any more obvious callsites, but none turned up. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20587
…ad of "| gzip > X" Summary: Fixes T13304. Shell pipes and redirects do not have robust behavior when errors occur. We provide "--compress" and "--output" flags as robust alternatives, but do not currently recommend their use. - Recommend their use, since their error handling behavior is more robust in the face of issues like full disks. - If "--compress" is provided but won't work because the "zlib" extension is missing, raise an explicit error. I believe this extension is very common and this error should be rare. If that turns out to be untrue, we could take another look at this. - Also, verify some flag usage sooner so we can exit with an error faster if you mistype a "bin/storage dump" command. Test Plan: Read documentation, hit affected error cases, did a dump and spot-checked that it came out sane looking. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13304 Differential Revision: https://secure.phabricator.com/D20572
Summary: See <https://discourse.phabricator-community.org/t/view-task-from-maniphest-e-mail-doesnt-have-url/2827>. I added "View Task" / "View Commit" buttons recently but the logic for generating URIs isn't quite right. Fix it up. Test Plan: - Commented on a task. - Used `bin/mail show-outbound --id ... --dump-html > out.html` to dump the HTML. - Previewed the HTML in a browser. - This time, actually clicked the button to go to the task. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20586
Summary: Fixes T13309. If you void the warranty on a repository on disk and turn it into a shallow clone, Phabricator currently can't serve it. We don't support hosting shallow working copies, but we should still parse and proxy the protocol rather than breaking in a mysterious way. Test Plan: - Created a shallow working copy with `mv X X.full; git clone --depth Y file://.../X.full X` in the storage directory on disk. - Cloned it with `git clone <uri>`. - Deleted all the refs inside it so the wire only has "shallow" frames; cloned it. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13309 Differential Revision: https://secure.phabricator.com/D20577
Summary: Ref T13302. Currently, if you enable Quicksand (by clicking "Persistent Chat"), open a dialog with links in it (like "Create Subtask" with multiple available subtypes), and then follow a navigation link, the page content reloads behind the dialog but the dialog stays in the foreground. Fix this by closing dialogs when users click navigation links inside them. Test Plan: With Quicksand enabled and disabled, clicked a subtask type in the "Create Subtask" dialog. - Before, Quicksand Disabled: Dialog stays on screen, then navigation occurs. - After, Quicksand Disabled: Dialog vanishes, then navigation occurs. - Before, Quicksand Enabled: Dialog stays on screen, navigation occurs behind it. - After, Quicksand Enabled: Dialog vanishes, then navigation occurs. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13302 Differential Revision: https://secure.phabricator.com/D20573
…th a link to the main branch list
Summary:
Fixes T13312. Currently, {nav Manage > Branches} has a list of branches on the same page. This has a few minor issues:
- Pager is at the top (see T13312), which is weird.
- "Default" icon is mystery meat.
- Table is kind of pointless/redundant in general?
Previously, this table had more information about technical status of each branch (autoclose/track/publish) but most of these details have been simplified/eliminated, and the main "Branches" view now has more information than it did before.
Get rid of this and just link to the main view.
Test Plan: Viewed "Branches" in UI, saw a link to the main view instead of a weird table.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13312
Differential Revision: https://secure.phabricator.com/D20584
…ser magic Summary: Ref T13302. In at least some browsers (including Safari and Chrome), when you write this: ``` <a href="#">...</a> ``` ...and then access `<that node>.href`, you get `http://local-domain-whatever.com/path/to/current/page#` back. This is wonderful, but not what we want. Access the raw attribute value instead, which is `#` in all browsers. Test Plan: - In Safari, Chrome, and Firefox: - Clicked "Edit Subtasks" from a task. - Clicked "Select" buttons to select several tasks. - Before: Clicking these button incorrectly closed the dialog (because of D20573). - After: Clicking these buttons now selects tasks without closing the dialog. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13302 Differential Revision: https://secure.phabricator.com/D20590
…as "x@y.com" attached to their account has not verified the address
Summary:
Fixes T13317. On `admin.phacility.com`, an enterprising user added `noreply@admin.phacility.com` to their account. This caused them to become CC'd on several support issues over the last year, because we send mail "From" this address and it can get CC'd via reply/reply all/whatever else.
The original driving goal here is that if I reply to a task email and CC you on my reply, that should count as a CC in Phabricator, since this aligns with user intent and keeps them in the loop.
This misfire on `noreply@` is ultimately harmless (being CC'd does not grant the user access permission, see T4411), but confusing and undesirable. Instead:
- Don't allow reserved addresses ("noreply@", "ssladmin@", etc) to trigger this subscribe-via-CC behavior.
- Only count verified addresses as legitimate user recipients.
Test Plan:
- Added a `bin/mail receive-test --cc ...` flag to make this easier to test.
- Sent mail as `bin/mail receive-test --to X --as alice --cc bailey@verified.com`. Bailey was CC'd both before and after the change.
- Sent mail as `bin/mail receive-test --to X --as alice --cc unverified@imaginary.com`, an address which Bailey has added to her account but not verified.
- Before change: Bailey was CC'd on the task anyway.
- After change: Bailey is not CC'd on the task.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13317
Differential Revision: https://secure.phabricator.com/D20593
… to at least have all the information
Summary:
Ref T13319. Currently, transactions about changes to a default form value use a raw internal key for the affected field and don't show the actual value change.
An ideal implementation will likely require us to specialize a great deal of rendering, but we can do much better than we currently do without too much work:
- Try to pull the actual `EditField` object for the key so we can `getLabel()` it and get a human-readable label (like `Visible To` instead of `policy.view`).
- Add a "(Show Changes)" action that dumps the raw values as more-or-less JSON, so you can at least figure out what happened if you're sophisticated enough.
Test Plan:
Before:
{F6516640}
After:
{F6516642}
The quality of "Show Details" varies a lot. For some fields, like "Description", it's pretty good:
{F6516645}
For others, like "Assigned To", it's better than nothing but pretty technical:
{F6516647}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13319
Differential Revision: https://secure.phabricator.com/D20594
… effects Summary: Fixes T13324. Ref PHI1288. Currently, if you edit an Owners package that has some paths with no trailing slashes (like `README.md`) so their internal names and display names differ (`/README.md` display, vs `/README.md/` internal), the "Show Details" in the transaction log shows the path as re-normalized even if you didn't touch it. Instead, be more careful about handling display paths vs internal paths. (This code on the whole is significantly less clear than it probably could be, but this issue is so minor that I'm hesitant to start ripping things out.) Test Plan: - In a package with some paths like `/src/` and some paths like `/src`: - Added new paths. - Removed paths. - Changed paths from `/src/` to `/src`. - Changed paths from `/src` to `/src/`. In all cases, the "paths" list and the transaction record identically reflected the edit in the way I expected them to. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13324 Differential Revision: https://secure.phabricator.com/D20596
Summary: Ref T13325. Test Plan: - Grepped for `topograph`. - Viewed a task graph since that's easy, looked fine. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13325 Differential Revision: https://secure.phabricator.com/D20599
Summary: Ref T13319. Ref PHI1302. Migrate `PhabricatorEditEngineConfigurationTransaction` to modular transactions and add some additional transaction rendering to make these edits less opaque. Test Plan: Hit all the form edit controllers, viewed resulting transaction timeline. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T13319 Differential Revision: https://secure.phabricator.com/D20595
…ecial way (with Doorkeeper)
Summary:
Ref T13291. See PHI1312. Currently, if you link to a JIRA or Asana issue with an anchor (`#asdf`) or query parameters (`?a=b`), we:
- treat the link as an external object reference and attempt a lookup on it;
- if the lookup succeeds, we discard the fragment or parameters when re-rendering the rich link (with the issue/task title).
Particularly, the re-rendering part uses the canonical URI of the object, and can discard these parameters/fragments, which is broken/bad.
As a first pass at improving this, just don't apply special behavior for links with anchors or parameters -- simply treat them as links.
In some future change, we could specialize this behavior and permit certain known parameters or anchors or something, but these use cases are likely fairly marginal.
Test Plan:
Before:
{F6516392}
After:
{F6516393}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291
Differential Revision: https://secure.phabricator.com/D20592
…th audit actions not applying properly Summary: See <https://discourse.phabricator-community.org/t/cannot-audit-a-git-commit/2848>. In D20581, I made some audit behavior dependent upon identities, but the actual edit flow doesn't load them. This can cause us to raise an "attach identities first" exception in the bowels of the edit workflow and trigger unexpected behavior at top level. Load identities when editing a commit so that the transaction flows have access to identity information and can use it to figure out if a user is an author, etc. Test Plan: - As an auditor, applied an "Accept Commit" action to an open audit after D20581. - Before patch: accept no-ops internally since the preconditions throw. - After patch: accept works properly. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20612
Summary: Ref T13321. Depends on D20600. Make `bin/phd stop` mean: - `bin/phd stop`: Stop all processes which have daemon process titles. If we're instanced, only stop daemons for the current instance. - `bin/phd stop --force`: Stop all processes which have deamon process titles for any instance. We no longer read or care about PID files on disk, and this moves us away from PID files. This makes unusual flag `--gently` do nothing. A followup will update the documentation and flags to reflect actual usage/behavior. This also removes the ability to stop specific PIDs. This was somewhat useful long, long ago when you might explicitly run different copies of the `PullLocal` daemon with flags to control which repositories they updated, but with the advent of clustering it's no longer valid to run custom daemon loadouts. Test Plan: Ran `bin/phd start`, then `bin/phd stop`. Saw instance daemons stop. Ran `bin/phd stop --force`, saw all daemons stop. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13321 Differential Revision: https://secure.phabricator.com/D20601
Summary: Ref T13321. Previously, the behavior was: - `bin/phd stop --gently`: Stop all daemons with PID files that belong to the current instance. - `bin/phd stop`: Stop all daemons with PID files that belong to the current instance. Complain if there are more processes. - `bin/phd stop --force`: Stop all processes that look like daemons, ignoring instances. The new behavior is: - `bin/phd stop`: Stop all processes that look like daemons and belong to the current instance. - `bin/phd stop --force`: Stop all processes that look like daemons, period. Test Plan: Grep / documentation only. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13321 Differential Revision: https://secure.phabricator.com/D20602
… a mess of local/remote information Summary: Ref T13321. Fixes T11037. Realign "bin/phd status" to just mean "show daemon processes on this host". The value of `bin/phd status` as a mixed remote/local command isn't clear, and the current output is a confusing mess (see T11037). This also continues letting us move away from PID files. Test Plan: Ran `bin/phd status`, saw sensible local process status. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13321, T11037 Differential Revision: https://secure.phabricator.com/D20604
Summary: Ref T13321. This gets rid of the last pidfile readers in Phabricator; we just use the process list instead. These commands always only work on the current instance since they don't make much sense otherwise. Test Plan: Ran `bin/phd start` and `bin/phd reload` with and without daemons running. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13321 Differential Revision: https://secure.phabricator.com/D20606
… daemons Summary: Ref T13321. The daemons no longer write PID files, so we no longer need to pass any of this stuff to them. Test Plan: Grepped for affected symbols. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13321 Differential Revision: https://secure.phabricator.com/D20608
… when billing a Phortune subscription
Summary:
Fixes T13327. Currently, when we try to bill an account and all members are disabled, we fail temporarily and the task retries forever.
At least for now, just treat this as a permanent failure.
Test Plan:
- Used `bin/phortune invoice` to generate a normal invoice for a regular subscription.
- Disabled all the account members, then tried again. Got a helpful permanent failure:
```
$ ./bin/phortune invoice --subscription PHID-PSUB-kbedwt5cyepoc6tohjq5 --auto-range
Set current time to Mon, Jun 24, 2:47 PM.
Preparing to invoice subscription "localb.phacility.com" from Fri, May 31, 10:14 AM to Sun, Jun 30, 10:14 AM.
WARNING
Manually invoicing will double bill payment accounts if the range overlaps an
existing or future invoice. This script is intended for testing and
development, and should not be part of routine billing operations. If you
continue, you may incorrectly overcharge customers.
Really invoice this subscription? [y/N] y
[2019-06-24 14:47:57] EXCEPTION: (PhabricatorWorkerPermanentFailureException) All members of the account ("PHID-ACNT-qp54y3unedoaxgkkjpj4") for this subscription ("PHID-PSUB-kbedwt5cyepoc6tohjq5") are disabled. at [<phabricator>/src/applications/phortune/worker/PhortuneSubscriptionWorker.php:88]
arcanist(head=experimental, ref.master=d92fa96366c0, ref.experimental=db4cd55d4673), corgi(head=master, ref.master=6371578c9d32), instances(head=stable, ref.master=ba9e4a19df1c, ref.stable=37fb1f4917c7), libcore(), phabricator(head=master, ref.master=65bc481c91de, custom=11), phutil(head=master, ref.master=7adfe4e4f4a3), services(head=master, ref.master=5424383159ac)
#0 PhortuneSubscriptionWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:124]
#1 PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:163]
#2 PhabricatorWorker::scheduleTask(string, array, array) called at [<phabricator>/src/applications/phortune/management/PhabricatorPhortuneManagementInvoiceWorkflow.php:169]
#3 PhabricatorPhortuneManagementInvoiceWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:457]
#4 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:349]
#5 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/setup/manage_phortune.php:21]
$
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13327
Differential Revision: https://secure.phabricator.com/D20613
Summary: Fixes T13326. In D20571, I slightly generalized construction of an iterator over a set of files, but missed some code in other "bin/files ..." commands which was also affected. Today, basically all of these workflows define their own "--all" and "names" flags. Pull these definitions up and implement them more consistently. Test Plan: Ran multiple different `bin/files` commands with different combinations of arguments, saw consistent handling of iterator construction. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13326 Differential Revision: https://secure.phabricator.com/D20614
Summary:
Ref T13328. Currently, we read from `mysqldump` something like this:
```
until (done) {
for (100 ms) {
mysqldump > in-memory-buffer;
}
in-memory-buffer > disk;
}
```
This general structure isn't great. In this use case, where we're streaming a large amount of data from a source to a sink, we'd prefer to have a "select()"-like way to interact with futures, so our code is called after every read (or maybe once some small buffer fills up, if we want to do the writes in larger chunks).
We don't currently have this (`FutureIterator` can wake up every X milliseconds, or on future exit, but, today, can not wake for readable futures), so we may buffer an arbitrary amount of data into memory (however much data `mysqldump` can write in 100ms).
Reduce the update frequency from 100ms to 10ms, and limit the buffer size to 32MB. This effectively imposes an artificial 3,200MB/sec limit on throughput, but hopefully that's fast enough that we'll have a "wake on readable" mechanism by the time it's a problem.
Test Plan:
- Replaced `mysqldump` with `cat /dev/zero` as the source command, to get fast input.
- Ran `bin/storage dump` with `var_dump()` on the buffer size.
- Before change: saw arbitrarily large buffers (300MB+).
- After change: saw consistent maximum buffer size of 32MB.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13328
Differential Revision: https://secure.phabricator.com/D20617
…lect a DocumentEngine Summary: Ref T13425. The changes in D20865 could incorrectly lead to selection of a DocumentEngine that can not generate document diffs if a file was added or removed (for example, when a source file is added). Move the engine pruning code to be shared -- we should always discard engines which can't generate a diff, even if we don't have both documents. Test Plan: Viewed an added source file, no more document ref error arising from document engine selection. Maniphest Tasks: T13425 Differential Revision: https://secure.phabricator.com/D20866
Summary:
Fixes T13435. If you move Phabricator or copy data from one environment to another, the repository URI index currently still references the old URI, since it writes the URI as a plain string. This may make "arc which" and similar workflows have difficulty identifying repositories.
Instead, store the "phabricator.base-uri" domain and the "diffusion.ssh-host" domain as tokens, so lookups continue to work correctly even after these values change.
Test Plan:
- Added unit tests to cover the normalization.
- Ran migration, ran daemons, inspected `repository_uriindex` table, saw a mixture of sensible tokens (for local domains) and static domains (like "github.com").
- Ran this thing:
```
$ echo '{"remoteURIs": ["ssh://git@local.phacility.com/diffusion/P"]}' | ./bin/conduit call --method repository.query --trace --input -
Reading input from stdin...
>>> [2] (+0) <conduit> repository.query()
>>> [3] (+3) <connect> local_repository
<<< [3] (+3) <connect> 555 us
>>> [4] (+5) <query> SELECT `r`.* FROM `repository` `r` LEFT JOIN `local_repository`.`repository_uriindex` uri ON r.phid = uri.repositoryPHID WHERE (uri.repositoryURI IN ('<base-uri>/diffusion/P')) GROUP BY `r`.phid ORDER BY `r`.`id` DESC LIMIT 101
<<< [4] (+5) <query> 596 us
<<< [2] (+6) <conduit> 6,108 us
{
"result": [
{
"id": "1",
"name": "Phabricator",
"phid": "PHID-REPO-2psrynlauicce7d3q7g2",
"callsign": "P",
"monogram": "rP",
"vcs": "git",
"uri": "http://local.phacility.com/source/phabricator/",
"remoteURI": "https://github.com/phacility/phabricator.git",
"description": "asdf",
"isActive": true,
"isHosted": false,
"isImporting": false,
"encoding": "UTF-8",
"staging": {
"supported": true,
"prefix": "phabricator",
"uri": null
}
}
]
}
```
Note the `WHERE` clause in the query normalizes the URI into "<base-uri>", and the lookup succeeds.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13435
Differential Revision: https://secure.phabricator.com/D20872
Summary: Ref T13436. Historically, this script could be used with a forked copy of "sshd" to do lower-cost per-key auth. Relatively modern "sshd" supports "%f" to "AuthorizedKeysCommand", which effectively moots this. Users have never been instructed to use this script for anything, and we moved away from this specific patch to "sshd" some time ago. Test Plan: Grepped for "ssh-auth-key", no hits. Maniphest Tasks: T13436 Differential Revision: https://secure.phabricator.com/D20873
…%k" from modern sshd Summary: Depends on D20873. Ref T13436. Allow callers to configure "bin/ssh-auth --sshd-key %k" as an "AuthorizedKeysCommand"; if they do, and we recognize the key, emit just that key in the output. Test Plan: - Used `git pull` locally, still worked fine. - Instrumented things, saw the public key lookup actually work and emit a single key. - Ran without "--sshd-key", got a full key list as before. Maniphest Tasks: T13436 Differential Revision: https://secure.phabricator.com/D20874
Summary: Ref T13436. There's no real security value to doing this comparison, it just wards off evil "security researchers" who get upset if you ever compare two strings with a non-constant-time algorithm. In practice, SSH public keys are pretty long, pretty public, and have pretty similar lengths. This leads to a relatively large amount of work to do constant-time comparisons on them (we frequently can't abort early after identifying differing string length). Test Plan: Ran `bin/ssh-auth --sshd-key ...` on `secure` with ~1K keys, saw runtime drop by ~50% (~400ms to ~200ms) with `===`. Maniphest Tasks: T13436 Differential Revision: https://secure.phabricator.com/D20875
Summary:
Fixes T13437. This URI construction was just missing URI encoding.
Also, trim the symbol because my test case ended up catching "#define\n" as symbol text.
Test Plan:
- Configured a repository to have PHP symbols.
- Touched a ".php" file with "#define" in it.
- Diffed the change.
- Command-clicked "#define" in the UI, in Safari/MacOS, to jump to the definition.
- Before: taken to a nonsense page where "#define" became an anchor.
- After: taken to symbol search for "#define".
Maniphest Tasks: T13437
Differential Revision: https://secure.phabricator.com/D20876
Summary: Ref T13438. This is a sort of minimal plausible implementation. Test Plan: Used "harbormaster.artifact.search" to query information about artifacts. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13438 Differential Revision: https://secure.phabricator.com/D20878
…tasks, not total tasks Summary: Ref T13279. See PHI1491. Currently, the top-level "Burnup Rate" chart in Maniphest shows total created tasks above the X-axis, without adjusting for closures. This is unintended and not very useful. The filtered-by-project charts show the right value (cumulative open tasks, i.e. open minus close). Change the value to aggregate creation events and status change events. Test Plan: Viewed top-level chart, saw the value no longer monotonically increasing. Maniphest Tasks: T13279 Differential Revision: https://secure.phabricator.com/D20879
Summary: On fresh installation which doesn't have yet any task closed you will not be able to open charts because of error below: Fixes: ``` [Mon Oct 21 15:42:41 2019] [2019-10-21 15:42:41] EXCEPTION: (TypeError) Argument 1 passed to head_key() must be of the type array, null given, called in ..phabricator/src/applications/fact/chart/PhabricatorFactChartFunction.php on line 86 at [<phutil>/src/utils/utils.php:832] [Mon Oct 21 15:42:41 2019] #0 phlog(TypeError) called at [<phabricator>/src/aphront/handler/PhabricatorAjaxRequestExceptionHandler.php:27] [Mon Oct 21 15:42:41 2019] #1 PhabricatorAjaxRequestExceptionHandler::handleRequestThrowable(AphrontRequest, TypeError) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:797] [Mon Oct 21 15:42:41 2019] #2 AphrontApplicationConfiguration::handleThrowable(TypeError) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:345] [Mon Oct 21 15:42:41 2019] #3 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:214] [Mon Oct 21 15:42:41 2019] #4 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:35 ``` To fix issue - lets return empty data set instead Test Plan: 1) Create fresh phabricator installation 2) Create fresh project 3) Try viewing charts Reviewers: epriestley, Pawka, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, yelirekim Differential Revision: https://secure.phabricator.com/D20861
… hovercards
Summary:
Ref T12164. Ref T13439. Commit hovercards don't currently show the repository. Although this is sometimes obvious from context, it isn't at other times and it's clearly useful/important.
Also, use identities to render author/committer information and show committer if the committer differs from the author.
Test Plan: {F6989595}
Maniphest Tasks: T13439, T12164
Differential Revision: https://secure.phabricator.com/D20881
Summary: Ref T13440. This feature is used in only one interface which I'm about to rewrite, so throw it away. Test Plan: Grepped for all affected symbols, didn't find any hits anywhere. Maniphest Tasks: T13440 Differential Revision: https://secure.phabricator.com/D20882
…le lists
Summary: Depends on D20882. Ref T13440. Instead of lists of "Differential Revisions" and "Commits", show all changes related to the task in a tabular view.
Test Plan: {F6989816}
Maniphest Tasks: T13440
Differential Revision: https://secure.phabricator.com/D20883
…iders for changes on tasks
Summary: Depends on D20883. Ref T13440. In most cases, all changes belong to the same repository, which makes the "Repository" column redundant and visually noisy. Show repository information in a section header.
Test Plan: {F6989932}
Maniphest Tasks: T13440
Differential Revision: https://secure.phabricator.com/D20884
Summary: Ref T13440. Give the table more obvious visual structure and get rid of the largely useless header columns. Test Plan: Viewed table, saw a slightly cleaner result. Maniphest Tasks: T13440 Differential Revision: https://secure.phabricator.com/D20885
…depth" in the API Summary: Fixes T13441. Internally, projects can be queried by depth, but this is not exposed in the UI. Add a "Is root project?" contraint in the UI, and "minDepth" / "maxDepth" constraints to the API. Test Plan: - Used the UI to query root projects, got only root projects back. - Used "project.search" in the API to query combinations of root projects and projects at particular depths, got matching results. Maniphest Tasks: T13441 Differential Revision: https://secure.phabricator.com/D20886
…de none of"
Summary: Fixes T13445. Make the meaning of this condition more clear, since the current wording is ambiguous between "any of" and "all of".
Test Plan: Edited a Herald rule with a PHID list field ("Project tags"), saw text label say "include none of".
Maniphest Tasks: T13445
Differential Revision: https://secure.phabricator.com/D20889
Summary:
Fixes T13446. Currently, the validation logic here rejects a rename like "alice" to "ALICE" (which changes only letter case) but this is a permissible rename.
Allow collisions that collide with the same user to permit this rename.
Also, fix an issue where an empty rename was treated improperly.
Test Plan:
- Renamed "alice" to "ALICE".
- Before: username collision error.
- After: clean rename.
- Renamed "alice" to "orange" (an existing user). Got an error.
- Renamed "alice" to "", "!@#$", etc (invalid usernames). Got sensible errors.
Maniphest Tasks: T13446
Differential Revision: https://secure.phabricator.com/D20890
…mprove "Fetch Refs" behavior Summary: Ref T13448. When "Fetch Refs" is configured: - We switch to a narrow mode when running "ls-remote" against the local working copy. This excludes surplus refs, so we'll incorrectly detect that the local and remote working copies are identical in cases where the local working copy really has surplus refs. - We rely on "--prune" to remove surplus local refs, but it only prunes refs matching the refspecs we pass "git fetch". Since these refspecs are very narrow under "Fetch Only", the pruning behavior is also very narrow. Instead: - When listing local refs, always list everything. If we have too much stuff locally, we want to get rid of it. - When we identify surplus local refs, explicitly delete them instead of relying on "--prune". We can just do this in all cases so we don't have separate "--prune" and "manual" cases. Test Plan: - Created a new repository, observed from a GitHub repository, with many tags/refs/branches. Pulled it. - Observed lots of refs in `git for-each-ref`. - Changed "Fetch Refs" to "refs/heads/master". - Ran `bin/repository pull X --trace --verbose`. On deciding to do something: - Before: since "master" did not change, the pull declined to act. - After: the pull detected surplus refs and deleted them. Since the states then matched, it declined further action. On pruning: - Before: if the pull was forced to act, it ran "fetch --prune" with a narrow refspec, which did not prune the working copy. - After: saw working copy pruned explicitly with "update-ref -d" commands. Also, set "Fetch Refs" back to the default (empty) and pulled, saw everything pull. Maniphest Tasks: T13448 Differential Revision: https://secure.phabricator.com/D20892
…tch Refs" operate more narrowly
Summary:
Ref T13448. The default behavior of "git fetch '+refs/heads/master:refs/heads/master'" is to follow and fetch associated tags.
We don't want this when we pass a narrow refspec because of "Fetch Refs" configuration. Tell Git that we only want the refs we explicitly passed.
Note that this doesn't prevent us from fetching tags (if the refspec specifies them), it just stops us from fetching extra tags that aren't part of the refspec.
Test Plan:
- Ran "bin/repository pull X --trace --verbose" in a repository with a "Fetch Refs" configuration, saw only "master" get fetched (previously: "master" and reachable tags).
- Ran "git fetch --no-tags '+refs/*:refs/*'", saw tags fetched normally ("--no-tags" does not disable fetching tags).
Maniphest Tasks: T13448
Differential Revision: https://secure.phabricator.com/D20893
…lone"
Summary:
Fixes T13448. We currently "git clone" to initialize repositories, but this will fetch too many refs if "Fetch Refs" is configured.
In modern Phabricator, there's no apparent reason to "git clone"; we can just "git init" instead. This workflow naturally falls through to an update, where we'll do a "git fetch" and pull in exactly the refs we want.
Test Plan:
- Configured an observed repository with "Fetch Refs".
- Destroyed the working copy.
- Ran "bin/repository pull X --trace --verbose".
- Before: saw "git clone" pull in the world.
- After: saw "git init" create a bare empty working copy, then "git fetch" fill it surgically.
Both flows end up in the same place, this one is just simpler and does less work.
Maniphest Tasks: T13448
Differential Revision: https://secure.phabricator.com/D20894
…nches" menu item in the Diffusion Management UI Summary: Ref T13448. Minor UI issue: setting a "Fetch Refs" value does not highlight the associated menu item, but should. Test Plan: Set only "Fetch Refs", now saw menu item highlighted. Maniphest Tasks: T13448 Differential Revision: https://secure.phabricator.com/D20895
Summary: Ref T13425. When a change adds or removes a block (vs adding or removing a document, or changing a block), we currently try to render `null` as cell content in the unified view. Make this check broader to catch both "no opposite document" and "no opposite cell".
Test Plan: {F7008772}
Subscribers: artms
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20897
…text Summary: Fixes T13452. We currently give users mixed signals about the interaction mode of this text: the cursor says "text" but the behavior is "grab". Make the behavior "text" to align with the cursor. An alternate variation of this change is to remove the cursor, but this is preferable if it doesn't cause problems, since copying the task ID is at least somewhat useful. Test Plan: In Safari, Firefox, and Chrome: selected and copied object names from workboard cards; and dragged workboard cards by other parts of their UI. Maniphest Tasks: T13452 Differential Revision: https://secure.phabricator.com/D20898
Summary: Ref T13453. The Asana API has changed, replacing all "id" fields with "gid", including the "users/me" API call result. Test Plan: Linked an Asana account. Before: error about missing 'id'. After: clean link. Maniphest Tasks: T13453 Differential Revision: https://secure.phabricator.com/D20899
Summary: Ref T13453. Some of the Asana integrations also need API updates. Depends on D20899. Test Plan: - Viewed "asana.workspace-id" in Config, got a sensible GID list. - Created a revision, saw the associated Asana task get assigned. - Pasted an Asana link I could view into a revision description, saw it Doorkeeper in the metadata. Maniphest Tasks: T13453 Differential Revision: https://secure.phabricator.com/D20900
…details and hovercards Summary: Ref T13442. Ref T13157. There's a secret URI to look at an object's hovercard in a standalone view, but it's hard to remember and impossible to discover. In developer mode, add an action to "View Hovercard". Also add "View Handle", which primarily shows the object PHID. Test Plan: Viewed some objects, saw "Advanced/Developer...". Used "View Hovercard" to view hovercards and "View Handle" to view handles. Maniphest Tasks: T13442, T13157 Differential Revision: https://secure.phabricator.com/D20887
…saction performs moves on multiple boards Summary: See <https://discourse.phabricator-community.org/t/unhandled-exception-rendering-maniphest-task/3234>. If a single transaction performs column moves on multiple different boards (which is permitted in the API), the rendering logic currently fails. Make it render properly. Test Plan: {F7011464} Differential Revision: https://secure.phabricator.com/D20901
…angeset" queries as not suitable for panel generation Summary: Fixes T13443. When a panel raises an exception during edit action generation, it currently escapes to top level. Instead, catch it more narrowly. Additionally, mark "ChangesetSearchEngine" as not being a suitable search engine for use in query panels. There's no list view or search URI so it can't generate a sensible panel. Test Plan: - Added a changeset panel to a dashboard. - Before: entire dashboard fataled. - After: panel fataled narrowly, menu fatals narrowly, dashboard no longer permits creation of another Changeset query panel. Maniphest Tasks: T13443 Differential Revision: https://secure.phabricator.com/D20902
tavasja
pushed a commit
that referenced
this pull request
Jan 29, 2020
…t failure when billing a Phortune subscription
Summary:
Fixes T13327. Currently, when we try to bill an account and all members are disabled, we fail temporarily and the task retries forever.
At least for now, just treat this as a permanent failure.
Test Plan:
- Used `bin/phortune invoice` to generate a normal invoice for a regular subscription.
- Disabled all the account members, then tried again. Got a helpful permanent failure:
```
$ ./bin/phortune invoice --subscription PHID-PSUB-kbedwt5cyepoc6tohjq5 --auto-range
Set current time to Mon, Jun 24, 2:47 PM.
Preparing to invoice subscription "localb.phacility.com" from Fri, May 31, 10:14 AM to Sun, Jun 30, 10:14 AM.
WARNING
Manually invoicing will double bill payment accounts if the range overlaps an
existing or future invoice. This script is intended for testing and
development, and should not be part of routine billing operations. If you
continue, you may incorrectly overcharge customers.
Really invoice this subscription? [y/N] y
[2019-06-24 14:47:57] EXCEPTION: (PhabricatorWorkerPermanentFailureException) All members of the account ("PHID-ACNT-qp54y3unedoaxgkkjpj4") for this subscription ("PHID-PSUB-kbedwt5cyepoc6tohjq5") are disabled. at [<phabricator>/src/applications/phortune/worker/PhortuneSubscriptionWorker.php:88]
arcanist(head=experimental, ref.master=d92fa96366c0, ref.experimental=db4cd55d4673), corgi(head=master, ref.master=6371578c9d32), instances(head=stable, ref.master=ba9e4a19df1c, ref.stable=37fb1f4917c7), libcore(), phabricator(head=master, ref.master=65bc481c91de, custom=11), phutil(head=master, ref.master=7adfe4e4f4a3), services(head=master, ref.master=5424383159ac)
#0 PhortuneSubscriptionWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:124]
#1 PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:163]
#2 PhabricatorWorker::scheduleTask(string, array, array) called at [<phabricator>/src/applications/phortune/management/PhabricatorPhortuneManagementInvoiceWorkflow.php:169]
#3 PhabricatorPhortuneManagementInvoiceWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:457]
#4 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:349]
#5 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/setup/manage_phortune.php:21]
$
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13327
Differential Revision: https://secure.phabricator.com/D20613
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.