Summary: Ref T6569. If a lease is activated with an expiration date, schedule a task to try to clean it up after that time.
Test Plan:
- Used `bin/drydock lease ... --until ...` to activate a lease in the near future.
- Waited for a bit.
- Saw it expire and get destroyed at the scheduled time.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T6569
Differential Revision: https://secure.phabricator.com/D14148
Summary:
Ref T9252. This simplifies some Drydock code.
Most of this code relates to the old notion of Drydock being able to enumerate all the tasks it needs to complete in order to acquire a lease. The code has stepped back from this, since it's unnecessary, the queue is more powerful than it used to be, and it would be a lot of work to keep track of.
The ~only thing that should ever wait for leases in modern code is `bin/drydock lease`, and it's fine for it to just sit there sleeping, so this just does that.
This reduces the granularity of logging, but I'll address that separately in future logging-focused changes.
Test Plan: Used `bin/drydock lease` to acquire a lease, saw it acquire cleanly.
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14147
Summary:
Ref T9253. For resources and leases that need to do something which takes a lot of time or requires waiting, allow them to allocate/acquire first and then activate later.
When we allocate a resource or acquire a lease, the blueprint can either activate it immediately (if all the work can happen quickly/inline) or activate it later. If the blueprint activates it later, we queue a worker to handle activating it.
Rebuild the "working copy" blueprint to work with this model: it allocates/acquires and activates in a separate step, once it is able to acquire a host.
Test Plan: With some power of imagination, brought up a bunch of working copies with `bin/drydock lease --type working-copy ...`
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14127
Summary:
Ref T9253. See discussion in D13843.
I want to let Drydock blueprints for Almanac services choose those services from a typeahead, but only list appropriate services in the typeahead. To do this:
- Provide a StandardCustomField for an arbitrary datasource.
- Adjust the AlmanacServiceDatasource to allow filtering by service class.
This implementation is substantially the same as the one in D13843, with some adjustments:
- I lifted most of the code in the `Users` standard custom field into a new `Tokenizer` standard custom field.
- The `Users` and `Datasource` custom fields now extend the `Tokenizer` custom field and can share most of the code it uses.
- I exposed this field fully as a configurable field. I don't think anyone will ever use it, but this generality costs us nearly nothing and improves consistency.
- The code in D13843 didn't actually pass the parameters over the wire, since the object that responds to the request is not the same object that renders the field. Use the "parameters" mechanism in datasources to get things passed over the wire.
Test Plan:
- Created a custom "users" field in Maniphest and made sure it still wokred.
- Created a custom "almanc services" field in Maniphest and selected some services for a task.
- With additional changes from D13843, selected an appropriate Almanac service in a new Drydock blueprint.
Reviewers: hach-que, chad
Reviewed By: hach-que, chad
Maniphest Tasks: T9253
Differential Revision: https://secure.phabricator.com/D14111
Summary:
Ref T7785. Makes Figlet available without installing the `figlet` package.
The PEAR Text_Figlet code is really sketchy and includes this API, which is quite marvelous:
```
function loadFont($filename, $loadgerman = true)
```
At some point, this should probably be rewritten into a modern style, but it's not trivial since the figlet file format and rendering engine are somewhat complicated. I made some adjustments:
- Broke the dependency on the PEAR core.
- Prevented it from doing any wrong HTML escaping.
- Looked through it for any glaring security or correctness problems.
This code isn't very pretty or modern, but as far as I can tell it's safe and does render Figlet fonts in a reasonable way.
Test Plan: {F803268}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9408, T7785
Differential Revision: https://secure.phabricator.com/D14102
Summary:
Ref T7785. Convert the Cowsay Remarkup rule to use a PHP implementation so we don't have to execute an external `cowsay` binary.
I removed some of the default ".cow" files that come with Cowsay because they:
- include Perl code which we can not interpret; or
- are primarily in-jokes or standalone visual puns or artwork rather than usable actors on the grand stage of cowsay; or
- offended my delicate sensibilities.
Users can add new cows to `resources/cows/custom/` if they want to make new cows available.
I have included a majestic original artwork depicting the "Companion Cube" character from //Portal//.
Test Plan: {F802535}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9408, T7785
Differential Revision: https://secure.phabricator.com/D14100
Summary: Ref T9408. This rule is unsafe in principle, and a practical vulnerability has been found by a security researcher.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9408
Differential Revision: https://secure.phabricator.com/D14103
Summary:
Fixes T9126. In particular:
- Add "Browse" links to all history views.
- Use icons to show "Browse" and "History" links, instead of text.
- Use FontAwesome.
- Generally standardize handling of these elements.
This might need a little design attention, but I think it's an improvement overall.
Test Plan:
- Viewed repository history.
- Viewed branch history.
- Viewed file history.
- Viewed table of contents on a commit.
- Viewed merged changes on a merge commit.
- Viewed a directory containing an external.
- Viewed a deleted file.
{F788419}
{F788420}
{F788421}
{F788422}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9126
Differential Revision: https://secure.phabricator.com/D14096
Summary: Fixes T8572. Ideally we would probably just permit this, but clean up the behavior until the day arrives when inline code is actually rewritten.
Test Plan:
- Tried to launch editors in Differential and Diffusion while comments were already open.
- Verified that "Jump to inline" works in both cases.
{F788008}
{F788009}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8572
Differential Revision: https://secure.phabricator.com/D14094
Summary: See D14025. In all cases where we compare hashes, use strict, constant-time comparisons.
Test Plan: Logged in, logged out, added TOTP, ran Conduit, terminated sessions, submitted forms, changed password. Tweaked CSRF token, got rejected.
Reviewers: chad
Reviewed By: chad
Subscribers: chenxiruanhai
Differential Revision: https://secure.phabricator.com/D14026
Summary: Fixes T9135. This is (probably) never intended and can be confusing.
Test Plan: Saw no hide button on unpublished inlines. Saw hide button on published inlines. Clicked hide button.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9135
Differential Revision: https://secure.phabricator.com/D14014
Summary:
Email is so exciting I can't wait 30 days for initial results.
ref T9161
Test Plan:
* `./bin/mail volume --days 60` took longer and gave plausibly larger
results.
* `./bin/mail volume --days 0` quickly told me no mail had been sent.
* `./bin/mail volume` Said it was still looking 30 days back.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T9161
Differential Revision: https://secure.phabricator.com/D13901
Summary:
We currently detect tab panels embedding themselves, but do not detect text panels embedding themselves with `{Wxx}`.
Detect these self-embedding panels.
I had to add a bit of a hack to pass the parent panel PHIDs to the rule. Generally, I got the Markup API kind of wrong and want to update it, I'll file a followup with details about how I'd like to move forward.
Test Plan:
Created a text panel embedding itself, a tab panel embedding a text panel embedding itself, a tab panel embedding a text panel embedding the tab panel, etc.
Rendered all panels standalone and as `{Wxx}` from a different context.
{F761158}
{F761159}
{F761160}
{F761161}
{F761162}
Reviewers: chad, jbeta
Reviewed By: chad, jbeta
Differential Revision: https://secure.phabricator.com/D13999
Summary:
Fixes T9268. Currently, we try to match any string like "a2f313f1" as a commit/revision, so short hashes will get picked up.
However, we don't require a word boundary or terminal after the match, so for input like "aaa...aaaaz" the engine can get stuck trying to split the string into sub-matches.
That is, in the original case, the input "aaaz" had valid matches against `[rA-Z0-9a-f]+` up to "z" of:
aaa
aa a
a aa
a a a
All of these will fail once it hits "z", but it has to try them all. This complexity is explosive with longer strings.
Instead, require a word boundary or EOL after the match, so this is the only valid match:
aaa
Then the engine sees the "z", says "nope, no match" and doesn't have to backtrack across all possible combinations.
Test Plan: Added a failing unit test, applied patch, clean test.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9268
Differential Revision: https://secure.phabricator.com/D13997
Summary:
Ref T8672. Ref T9187. Root issue in at least one case is:
- User makes a commit including a file with some non-UTF8 text (say, a Japanese file full of Shift-JIS).
- We pass the file to the TransactionEditor so it can inline or attach the patch if the server is configured for these things.
- When inlining patches, we convert them to UTF8 before inlining. We must do this since the rest of the mail is UTF8.
- When attaching patches, we send them in the original encoding (as file attachments). This is correct, and means we need to give the worker the raw patch in whatever encoding it was originally in: we can't just convert it to utf8 earlier, or we'd attach the wrong patch in some cases.
- TransactionEditor does its thing (e.g., creates the commit), then gets ready to send mail about whatever it did.
- The publishing work now happens in the daemon queue, so we prepare to queue a PublishWorker and pass it the patch (with some other data).
- When we queue workers, we serialize the state data with JSON.
So far, so good. But this is where things go wrong:
- JSON can't encode binary data, and can't encode Shift-JIS. The encoding silently fails and we ignore it.
Then we get to the worker, and things go wrong-er:
- Since the data is bad, we fatal. This isn't a permanent failure, so we continue retrying the task indefinitely.
This applies several fixes:
# When queueing tasks, fail loudly when JSON encoding fails.
# In the worker, fail permanently when data can't be decoded.
# Allow Editors to specify that some of their data is binary and needs special handling.
This is fairly messy, but some simpler alternatives don't seem like good ways forward:
- We can't convert to UTF8 earlier, because we need the original raw patch when adding it as an attachment.
- We could encode //only// this field, but I suspect some other fields will also need attention, so that adding a mechanism will be worthwhile. In particular, I suspect filenames //may// be causing a similar problem in some cases.
- We could convert task data to always use a serialize()-based binary safe encoding, but this is a larger change and I think it's correct that things are UTF8 by default, even if it makes a bit of a mess. I'd rather have an explicit mess like this than a lot of binary data floating around.
The change to make `LiskDAO` will almost certainly catch some other problems too, so I'm going to hold this until after `stable` is cut. These problems were existing problems (i.e., the code was previously breaking or destroying data) so it's definitely correct to catch them, but this will make the problems much more obvious/urgent than they previously were.
Test Plan:
- Created a commit with a bunch of Shift-JIS stuff in a file.
- Tried to import it.
Prior to patch:
- Broken PublishWorker with distant, irrelevant error message.
With patch partially applied (only new error checking):
- Explicit, local error message about bad key in serialized data.
With patch fully applied:
- Import went fine and mail generated.
Reviewers: chad
Reviewed By: chad
Subscribers: devurandom, nevogd
Maniphest Tasks: T8672, T9187
Differential Revision: https://secure.phabricator.com/D13939
Summary:
Ref T9218. See discussion there for rationale; I think this is the right behavior to pursue.
The screenshot below is pretty ugly. I think it's a lot worse than most real-world cases will be, since you have to sort of opt-in to having crazy levels of overlapping packages, and it's perfectly normal/reasonable for files owned by one package. Owners is powerful enough to let you specify sub-packages with exclusive ownership.
That said, this may be more typical than I hope. I don't think we can reduce the complexity here much for free, but it would might be reasonable to add some view options (e.g.: group by package?, show only packages I own?, show packages as icons with a tooltip?) if it's an issue.
Test Plan: {F734956}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9218
Differential Revision: https://secure.phabricator.com/D13940
Summary:
Fixes T8004.
- For paths which are part of a package, show the package.
- Highlight paths which are part of a package you (the viewer) have authority over.
Test Plan:
{F725418}
- Viewed owned and unowned chagnes in Diffusion and Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8004
Differential Revision: https://secure.phabricator.com/D13923
Summary:
Fixes T2183. We now use the same rendering element in both places.
Intentional changes:
- Package highlighting is out, coming back to both apps in next diff.
- removed redundant-feeling "Change" link. The information is now shown with a character ("M", "V", etc.) and the page is a click away under "History". Clicking the path also jumps you to substantially similar content. (We could restore it fairly easily, I just think it's probably the least useful thing in the table right now.)
Test Plan: Viewed a bunch of commits in Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13910
Summary:
Ref T2183. Introduces a new View which can (in theory) unify the Revision, Diff and Commit table of contents views.
This has the same behavior as before, but accepts slightly more general primitives and parameters and has somewhat cleaner code.
I've made one intentinoal behavior change: removing the "Open All in Editor" button. I suspect this is essentially unused, and is a pain to keep around. We can look at restoring it if anyone notices.
Test Plan: Looked at a bunch of revisions, no changes from before.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2183
Differential Revision: https://secure.phabricator.com/D13908
Summary: Fixes T9099, I think this is as much as I can come up with for unbeta. Cleans up the answer header (profile image, smaller font, smaller header). Cleans up voting (new, with color), and makes it a bit more readable.
Test Plan:
Review a number of answers in ponder with and without votes, comments.
{F720189}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T9099
Differential Revision: https://secure.phabricator.com/D13907
Summary: Use `PhutilClassMaQuery` instead of `PhutilSymbolLoader`, mostly for consistency. Depends on D13588.
Test Plan: Poked around a bunch of pages.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13589
Summary:
Fixes T7370. Two changes:
- Make the default to show nothing, instead of showing all the data. This is a better default because the data is sometimes sensitive. Workers should have to opt in to revealing it.
- For TargetWorkers, link to the target (technically the build, for now, since there's no dedicated target detail page).
Test Plan: {F698325}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7370
Differential Revision: https://secure.phabricator.com/D13845
Summary: Ref T3578 Allows display of projects if available on individual ponder search results.
Test Plan: Added projects to my questions, see them display on search results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T3578
Differential Revision: https://secure.phabricator.com/D13793
Summary: Ref T8726. Convert these to be modular.
Test Plan:
- Created rules using these actions.
- Upgraded them.
- Verified they still work.
{F659266}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13705
Summary: Ref T8726. No surprises.
Test Plan:
Created rules using both action variants, applied upgrade, saw rules still work correctly.
{F658842}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13701
Summary: Ref T8726. Converts these actions to be modular. No real surprises in this change.
Test Plan:
{F658709}
- Wrote some rules.
- Migrated them forward.
- Used a bunch of these rules.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13699
Summary: DRAFT - throw together Phurl skeleton.
Test Plan: The idea is that `some/long/url` will become `install/Udet4d` and can be viewed and edited at `install/Udet4d/view` and `install/Udet4d/edit`, respectively?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, chad, epriestley, Korvin
Maniphest Tasks: T6049
Differential Revision: https://secure.phabricator.com/D13681
Summary:
Basic plumbing for Badges application.
- You can make Badges.
- You can look at a list of them.
- They can be edited.
- They can be assigned to people.
- You can revoke them from people.
- You can subscribe to them.
Test Plan: Make Badges with various options. Give them to people. Take them away from people.
Reviewers: lpriestley, epriestley
Reviewed By: epriestley
Subscribers: tycho.tatitscheff, johnny-bit, epriestley, Korvin
Maniphest Tasks: T6526
Differential Revision: https://secure.phabricator.com/D13626
Summary:
Fixes T8917. Prior to T2618, deleting inlines prompted users, then really deleted the rows.
After T2618, we delete immediately and offer "Undo". However, some interactions with drafts were missed, and we were only clearing the "this revision has a draft" flag on one of the delete pathways (when you delete all the comment text, then save the comment).
Make both the "Delete" action and the "Delete All Comment Text + Save" workflows do the same thing: mark the row as deleted, and clear any relevant drafts.
Test Plan:
- Made an inline comment on a clean revision with no "draft comments" marker in the list view.
- Used "delete" to delete it.
- After applying the patch, verified that no "draft commetns" marker appears in the list view.
- Used Delete and Edit + Remove Text + Save to delete comments in Differential and Diffusion.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8917
Differential Revision: https://secure.phabricator.com/D13665
Summary: I assume this is correct, it fixes my test case and was inconsistent with the rest of the file.
Test Plan: Add and Remove a CustomFieldPHID in Maniphest, no longer get errors.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8866
Differential Revision: https://secure.phabricator.com/D13642
Summary:
Ref T8726. Some adapters now have a large number of fields, and we lost the sort-of-human-readable implicit ordering when fields were modularized.
Instead, group and sort fields.
Test Plan: {F603066}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13619
Summary: Ref T8726. This gets rid of all the `VALUE_*` constants and lets Fields provide arbitrary typeaheads without upstream/JS changes.
Test Plan: Used all tokenizers. Used "Another Herald Rule". Grepped for all removed constants.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13616
Summary: Use `PhutilClassMapQuery` where appropriate.
Test Plan: Browsed around the UI to verify things seemed somewhat working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13429
Summary: Ref T8726. The existing implementation is largely made redundant by the newer modular implementation.
Test Plan: Created a custom field rule, set custom field to various values until it matched, saw effect and proper transcripts.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, joshuaspence, epriestley
Maniphest Tasks: T8726
Differential Revision: https://secure.phabricator.com/D13497
Summary: Return `$this` from setter methods for consistency. I started writing a [[https://secure.phabricator.com/differential/diff/32506/ | linter rule]] to detect this, but I don't think it is trivial to do this properly.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13422
Summary: Move some `PhabricatorSearchField` subclasses to be adjacent to the application to which they belong. This seems generally better to me than lumping them all together in the `src/applications/search/field/` directory. I was also wondering if it makes sense to rename these subclasses as `PhabricatorXSearchField` rather than `PhabricatorSearchXField` (as per T5655), but wasn't really sure if these objects are meant to be search-fields, or just fields belonging to the #search application.
Test Plan: N/A.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13374
Summary: Ref T8750, Adds a maxwidth class for Graphviz images.
Test Plan: Generate a Graphviz image, really big, see it scale to the viewport.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8750
Differential Revision: https://secure.phabricator.com/D13548
