Summary:
Ref T13202. See PHI889. If the lock log is enabled, we can try to offer more details about lock holders.
When we fail to acquire a lock:
- check for recent acquisitions and suggest that this is a bottleneck issue;
- if there are no recent acquisitions, check for the last acquisition and print details about it (what process, how long ago, whether or not we believe it was released).
Test Plan:
- Enabled the lock log.
- Changed the lock wait time to 1 second.
- Added a `sleep(10)` after grabbing the lock.
- In one window, ran a Conduit call or a `git fetch`.
- In another window, ran another operation.
- Got useful/sensible errors for both ssh and web lock holders, for example:
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=12609, host=orbital-3.local, sapi=apache2handler, controller=PhabricatorConduitAPIController, method=diffusion.rawdiffquery) 3 second(s) ago. There is no record of this lock being released.)
> PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=65251, host=orbital-3.local, sapi=cli, argv=/Users/epriestley/dev/core/lib/phabricator/bin/ssh-exec --phabricator-ssh-device local.phacility.net --phabricator-ssh-key 2) 2 second(s) ago. There is no record of this lock being released.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19702
Summary:
Ref T13202. See PHI889. Update the read and write locks to the modern parameterized verison, which handles hashing/normalization and can store better logs.
This parameterized mode was added in D19173 and has been used successfully for some time, but not all locks have switched over to it yet.
Test Plan:
- Added an `fprintf(STDERR, $full_name)` to the lock code.
- Pulled a repository.
- Saw sensible lock name on stdout before "acquired read lock...".
- Additional changes in this patch series will vet this more completely.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19701
Summary:
Ref T13202. In D19660, I added comments to Phriction and tweaked some CSS.
One of these tweaks was getting rid of an extra border which was rendering under the comment area. However, I took off too much and ended up removing borders from other applications.
I think we don't actually need this `setNoBorder()` stuff after all -- a later change was sufficient to stop the actual border I was trying to get rid of from rendering. So this mostly just reverts part of D19660.
This rendering still isn't perfect, but I'm fine leaving that for another day for now.
Test Plan:
- Viewed comment areas in Phriction. Saw correct number of borders (1).
- Viewed comment areas in Maniphest. Saw correct number of borders (1).
- Grepped for extraneous removed classs, no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19684
Summary:
Depends on D19682. Ref T13202. We currently fatal when trying to render a timeline if:
- an install is fresh, so there are no pages yet, and you look at "/w/"; or
- you're looking at a Phriction page which doesn't exist (yet) like "/w/aadsflknadsflnf/".
Rendering a timeline and comment area doesn't make sense in these cases, so don't render them.
Test Plan: Hit both cases described above, got "new/empty page" prompts instead of fatals.
Reviewers: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19683
Summary:
Ref T13202. See PHI881. These stories have bad rendering methods, but they didn't previously render into the timeilne (since Phriction documents didn't have a timeline).
Update the rendering to work.
The rendered outcome isn't great (it isn't very clear or explicit about exactly what moved where), but I'll fix that in a followup. This is a net improvement since it doesn't fatal the page, at least.
Test Plan:
- Moved page "X" to "Y".
- Viewed the old page "X".
- Before patch: bad timeline story would fatal rendering.
- After patch: story renders, at least, just not great.
Reviewers: amckinley
Maniphest Tasks: T13202
Differential Revision: https://secure.phabricator.com/D19682
Summary:
Depends on D19672. Ref T13197. See PHI873. This writes a trivial log when we begin acting on a working copy and makes it look reasonable in the UI.
This is mostly just to prove that logging works properly.
Test Plan: {F5887697}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19673
Summary:
Depends on D19671. Ref T13197. See PHI873.
Expose logs in the RepositoryOperation UI. Nothing writes the logs yet, so these interfaces are currently always empty.
Test Plan:
{F5887102}
{F5887103}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19672
Summary: Ref T13197. See PHI873. I want to give RepositoryOperation objects access to Drydock logging like leases, resources, and blueprints currently have. This just does the schema/query changes, no actual UI or new logging yet.
Test Plan: Ran storage upgrade, poked around the UI looking for anything broken.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19671
Summary: Depends on D19668. Ref T13197. See PHI840. This updates the documentation to describe how drafts work in more detail.
Test Plan: Read documentation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19669
Summary:
Ref T13077. Ref T13197. See PHI840.
- In the "History > Diff/Compare" view, the button language wasn't draft-aware.
- Revise language to avoid the word "Revert", since this can be ambiguous.
- "Edit this page, starting with an older version of the text" is now "Edit Older|Current|Draft Version X".
- "Mark this older version of the page as the current published version" is now "Publish Older Version".
- Let the user edit the current published version, too, since this is a reasonable operation if there are drafts.
Test Plan: Navigated the history diff view, saw better button and action text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197, T13077
Differential Revision: https://secure.phabricator.com/D19668
Summary: Depends on D19663. Ref T13077. When you edit a Phriction draft, don't publish a feed story. (The eventual "Publish" event gets a story.)
Test Plan: Made draft / non-draft / publish edits, only saw feed stories for non-draft and publish edits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19664
Summary:
Depends on D19662. Ref T13077. See PHI840.
- If you're looking at the published version of a document, but a draft version exists and you can edit it, add a hint/link.
- Fix an issue where the "draft" transaction would complain when you created a document since the initial content is empty and no "draft" transaction is adding any content.
Test Plan: Created new documents, viewed documents with current published versions and unpublished drafts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19663
Summary:
Depends on D19661. Ref T13077. See PHI840.
When a user edits a page normally, add a "Save as Draft" button. Much of this change is around making that button render and behave properly: it needs to be an `<input type="submit" ...>` so browsers submit it and we can figure out which button the user clicked.
Then there are a few minor rules:
- If you're editing a page which is already a draft, we only give you "Save as Draft". This makes edits to update/revise a draft more natural.
- Highlight "Publish" if it's a likely action that you might want to take.
Internally, there are two types of edits. Both types create a new version with the new content. However:
- A "content" edit sets the version shown on the live page to the newly-created version.
- A "draft" edit does not update the version shown on the live page.
Test Plan: Edited a published document, edited the draft. Published documents. Reverted documents.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19662
Summary:
Depends on D19660. Ref T5811. Ref T13077.
Long ago, if you started editing a Phriction document but didn't save it, we'd save the draft in the background as part of the preview.
D11169 updated the preview to use shared infrastructure and broke this function, since we never save drafts.
Since this doesn't work right now, I want to add another thing called "draft", and the future of this feature should be more integrated with modern drafts and EditEngine (which fixed some bugs related to versioning), just get rid of this code for the moment.
Test Plan: Edited documents. This code doesn't do anything since D11169, so no behavior changed.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077, T5811
Differential Revision: https://secure.phabricator.com/D19661
Summary:
Depends on D19659. Fixes T1894. Ref T13077. See PHI840.
- Add an EditEngine, although it currently supports no fields.
- Add (basic, top-level-only) commenting (we already had the table in the database).
This will probably create some issues. I'm most concerned about documents accumulating a ton of old, irrelevant comments over time which are hard to keep track of and no longer relevant. But I think this is probably a step forward in almost all cases, and a good thing on the balance.
This also moves us incrementally toward putting all editing on top of EditEngine.
Test Plan: {F5877347}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077, T1894
Differential Revision: https://secure.phabricator.com/D19660
Summary:
Ref T13197. See PHI873. Record when a user has MFA'd and add a little icon to the transaction, similar to the exiting "Silent" icon.
For now, this just makes this stuff more auditable. Future changes may add ways to require MFA for certain specific transactions, outside of the ones that already always require MFA (like revealing credentials).
Test Plan: {F5877960}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19665
Summary:
Depends on D19657. Ref T13197. See PHI841.
This enriches the results from `diffusion.commit.search` with information similar to the information returned by the "commits" attachment from `differential.diff.search`.
Also include unreachable, imported, message, audit status, and repository PHID.
Test Plan: Called `diffusion.commit.search` and reviewed the results, which looked sensible.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19658
Summary:
Depends on D19656. Ref T13197. See PHI851.
- This class is now a real object, so get rid of the "Constants" part of the name.
- Rename it for greater consistency with other modern objects.
- Get rid of the `MODERN_` tag now that the old constants are gone.
Test Plan: Bunch of `grep`, browsed around.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19657
Summary: Depends on D19655. Ref T13197. These no longer have callers.
Test Plan: Grepped for these constants.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19656
Summary: Depends on D19652. Ref T13197. See PHI851. This migrates the actual `auditStatus` on Commits, and older status transactions.
Test Plan:
- Ran migrations.
- Spot-checked the database for sanity.
- Ran some different queries, got unchanged results from before migration.
- Reviewed historic audit state transactions, and accepted/raised concern on new audits. All state transactions appeared to generate properly.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19655
Summary: Depends on D19651. Ref T13197. The application now accepts either numeric or string values. However, for consistency and to reduce surprise in the future, migrate existing saved queries to use string values.
Test Plan: Saved some queries on `master` with numeric constants, ran the migration, saw string constants in the database and equivalent behavior in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19652
Summary:
Ref T13077. See PHI840. Ref T1894. I'm planning to just let you comment on Phriction documents. I think this will create a few problems (e.g., around popular documents which collect long comment threads that are eventually obsolete) but nothing should be too terribly critical (e.g., we handle it gracefully when objects have very large number of comments/transactions) and for most documents this is likely just a net improvement.
"Just enable comments" is probably not the final iteration on this, but I think it's probably a step forward on the balance, not a step sideways or a slippery slope down into a dark hole or anything.
Test Plan: {F5877316}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13077, T1894
Differential Revision: https://secure.phabricator.com/D19659
Summary: See PHI871. Ref T13197. These sections are only divided visually and don't have textual headers. Add aural headers.
Test Plan: {F5875471}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19654
Summary:
Depends on D19650. Ref T13197. Allow `SearchCheckboxesField` to have a "deprecated" map of older aliases, then convert them to modern values.
On the API method page, show all the values.
This technically resolves the issue in PHI841, although I still plan to migrate behind this.
Test Plan:
{F5875363}
- Queried audits, fiddled with `?status=1,audited`, etc.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19651
Summary: Ref T13197. We're almost ready to migrate: let the Query accept either older integer values or new string values. Then move some callsites to use strings.
Test Plan: Called `audit.query`, browsed audits, audited commits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13197
Differential Revision: https://secure.phabricator.com/D19650
Summary: Ref T13195. See PHI851. Continuing down the path toward replacing these legacy numeric constants with more modern string constants.
Test Plan:
- Raised concern, requested verification, verified.
- Looked at commit hovercard with audit status.
- Viewed header on a commit page.
- (Didn't test the Doorkeeper stuff since it requires linking to Asana and seems unlikely to break.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19647
Summary:
Ref T13195. If a Phriction page begins with a code block, the `clear: both;` currently makes it clear the action list.
Instead, use table-cell layout on desktops.
Test Plan: Viewed a Phriction page with an initial code block on desktop/tablet/mobile/printable layouts. Now got more sensible layouts in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: GoogleLegacy
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19649
Summary:
Ref T13195. Fixes T8573. When you're adding inlines to your own stuff, mark them "Done" by default. You can unmark them as "Done" if you're legitimately leaving TODOs for yourself, although I think this is unusual.
(If this turns out to be less unusual than I think, we could consider an alternate rule: mark replies by the author as "Done" by default.)
Test Plan: Added some inlines as an author and a non-author. Saw my author inlines marked as "Done" by default. Submitted them; unmarked and submittted them.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19635
Summary:
Ref T13195. Ref T8573. This allows reviewers to mark their own inline comments as "Done" before they submit them.
If you're leaving a non-actionable comment like "this is good", you can pre-check "Done" to give the author a hint that you don't expect any response.
Test Plan: On revisions and commits, added inlines as the author and a reviewer/auditor. Marked them done/not-done before submitting. As author, marked the not-done ones done after submitting. Checked preivews, toggled done/not done states.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19634
Summary:
Ref T13195. See PHI861. The "+ Draft" flag is not currently exposed over the API, but seems stable enough to expose.
Also expose the "hold as draft" flag, normally `arc diff --draft`.
Today, you can get "+ Draft" with some other state by:
- abandoning a draft revision ("Abandoned + Draft"); or
- using `arc diff --plan-changes` with an older `arc` version ("Changes Planned + Draft").
Test Plan: Called `differential.revision.search` via Conduit and got sensible-looking results for revisions in various states.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19648
Summary:
Ref T13195. See PHI851. Add an object, analogous to the `DifferentialRevisionStatus` object, to handle audit status management.
This will primarily make it easier to swap storage over to strings later, but also cleans things up a bit.
Test Plan: Viewed audit/commit lists, saw sensible state icons. Ran `bin/audit synchronize`, got sensible output.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19646
Summary:
See PHI859. Ref T13195. The regexp for replacing variables currently does not include underscores.
Include underscores.
I also made a note in T13088: we should (almost certainly?) throw immediately if you try to pass a bogus variable name as a custom parameter, but this is a slightly larger change.
Test Plan:
- Made an "http request" build plan with `?x=${initiator.phid}&y={$some_variable}`.
- Added `some_variable` as a parameter to the parameter collection.
- Before patch: `initiator.phid` was replaced, but `some_variable` was not.
- After patch: both variables are replaced.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19645
Summary: See D19632. Agreed that this is more clear.
Test Plan: Read carefully.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19644
Summary:
Ref T13195. See PHI851. Audits currently have older integer status constants. We've moved almost all object types away from this to string constants (which are better in basically every way, and particularly way better for exposing over the API).
Commits/audits are currently accessible over the API and expose these constants via a "statuses" constraint.
Prepare to move toward modern string constants by defining a new, more modern map of status details and defining the existing methods in terms of it.
Test Plan: Browsed audits checking for icons/names/open-ness, saw no changes. This change should have no user-visible effects, as it just reorganizes code.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19642
Summary:
Ref T13195. Ref PHI851. Currently, checkbox constraints don't tell you what values are supported on the API page.
You can figure this out with "Inspect Element" or by viewing the source code, but just render a nice table instead.
Test Plan: {F5862969}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19641
Summary:
Ref T13195. See PHI851. Start by making some minor improvements here:
- Clarify that the example of what constraints look like is just an example.
- Add descriptions for parameters missing descriptions.
Test Plan: Looked at API method page, saw more helpful/complete instructions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19640
Summary:
Ref T13195. See PHI842. Alternative to D19638.
Instead of doing all the stuff in D19638, //just// remove the `rebuild_summaries.php` script. This script is outdated, copy/pastes the rebuild logic, and doesn't understand unreachable commits.
If we had some use for it it should move to `bin/repository rebuild-summary ...` or similar, but it's not clear there's any use for it. The incremental summary rebuilds seem to work fine as-is.
Test Plan: Grepped for callers or documentation referencing this script, found nothing.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19643
Summary: Ref T13195. See PHI845. For custom OperationTypes, provide access to the Interface and Operation via getters. This is just for convenience, since passing these around everywhere can be a bit of a pain if you have a deep-ish stack of things or love using callbacks or whatever.
Test Plan: Landed a revision via upstream Drydock operations.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19636
Summary:
Ref T13195. An install had a user apply a transaction which added about 1,000 inline comments. Rendering the email for this transaction took a very long time because the context section for each comment must be highlighted separately.
We can make the highlighting faster (in this case, by porting the lexer to PHP) but it's also sort of silly to include more than 100 inlines in an email. These emails are likely to be truncated by outbound size rules at some point anyway. Instead, limit inlines rendered directly into email to the first 100 per transaction group.
Test Plan:
Set limit to 2, added 4 comments, viewed text and HTML emails:
{F5859967}
{F5859968}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195
Differential Revision: https://secure.phabricator.com/D19632
Summary:
Ref T13195. Ref T8573. The inline comment controllers currently use outdated `$user = $this->getRequest()->getUser()` calls.
Instead, use `$viewer = $this->getViewer()`.
This is just a small consistency update with no behavioral changes.
Test Plan: Viewed and added inlines in Differential and Diffusion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13195, T8573
Differential Revision: https://secure.phabricator.com/D19633
The unique key on <documentPHID, version> may fail to apply if any content
rows don't have a valid document. This is rare, but we have some old random
garbage rows on "secure.phabricator.com" which prevent the next patch from
applying. Just toss these rows, they're junk.
Summary: Ref T13077. This is currently a little too confusing to go out into the world, mostly because there's no way to edit documents without auto-publishing them. Keep it out of the spotlight for this release.
Test Plan: Viewed Phriction, saw publish operation marked as a prototype.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19627
Summary: Ref T13077. Updates the "History" view to be slightly better organized and draft-aware.
Test Plan: Viewed page history in Phriction.
Reviewers: amckinley
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19626
Summary:
Ref T13077. We need to know the maximum version of a document in several cases, so denormalize it onto the Document object.
Then clean up some behaviors where we edit a document with, e.g., 7 versions but version 5 is currently published. For now, we: edit starting with version 7, save as version 8, and immediately publish the new version.
Test Plan:
- Ran migration.
- Edited a draft page without hitting any weird version errors.
- Checked database for sensible `maxVersion` values.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19625
Summary:
See T13193. See T13077. If we drop a column which is part of a UNIQUE KEY, MariaDB raises an error.
This is probably a bad idea on our side anyway, but in this case it wasn't an obviously bad idea.
To get around this:
- Drop the unique key, if it exists, before dropping the column.
- Explicitly add the new unique key afterward.
Test Plan: Ran `bin/storage upgrade` locally without issue, but I'm on MySQL. Will follow up on T13193.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19624
Summary:
See PHI844. Ref T13189.
Add "Revision test plan" as an available field for Herald. This is a little niche -- and a little odd because it sticks around even if you fully disable test plans -- but probably broadly reasonable.
The existing "Revision summary" field counterintuitively included the test plan. Separate this out since it's now a separate field and the behavior was weird historic nonsense. I'll note this in the changelog.
Test Plan: Wrote a rule using both fields, verified they generated the expected values.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19623
