Summary: Ref T13632. Users searching for `__FILE__`, etc., almost certainly mean to perform a substring search.
Test Plan: Added tests and made them pass. Searched for various tokens, saw compiler interpretation in UI.
Maniphest Tasks: T13632
Differential Revision: https://secure.phabricator.com/D21602
Summary:
Ref T13631. This supports a more robust version of "poll for updates by using dateModified window queries" that uses transactions as a logical clock.
This is particularly relevant for commits, since they don't have a "dateModified" at time of writing.
Test Plan:
- Queried for transactions by type and object.
- Issued various invalid transaction queries, got appropriate errors.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21601
Summary: Ref T13631. These strings were a little inconsistent; make them more consistent.
Test Plan: Called `diffusion.commit.search` with the appropriate attachment, saw slightly more consistent statuses.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21600
Summary:
Ref T13631. Move "PhabricatorAuditStatusConstants" to a more modern object ("PhabricatorAuditRequestStatus").
Expose the status value via Conduit.
Test Plan:
- Ran `bin/audit delete`.
- Viewed a commit with auditors in the web UI.
- Grepped for affected symbols.
- Called Conduit with the "auditors" attachment, saw auditor statuses.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21599
Summary:
Ref T13631. See that task for discussion.
- "NONE": Probably never used?
- "CC": Obsoleted by subscribers.
- "AUDIT_NOT_REQUIRED": For Owners packages, obsoleted by edges.
- "CLOSED": For "Close Audit", obsoleted by "Request Verification".
Test Plan:
- Grepped for constants, browsed Diffusion.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21598
Summary: Ref T13631. For now, this only shows the auditor PHID. The current status constants could use some cleanup before they're exposed.
Test Plan: Queried with "auditors" attachment, saw basic auditor information.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21597
Summary:
Ref T13629.
- Allow files to have custom alt text.
- If a file doesn't have alt text, try to generate a plausible default alt text with the information we have.
Test Plan:
- Viewed image files in DocumentEngine diffs, files, `{Fxxx}` embeds, and lightboxes.
- Saw default alt text in all cases, or custom alt text if provided.
- Set, modified, and removed file alt text. Viewed timeline and feed.
- Pulled alt text with "conduit.search".
Maniphest Tasks: T13629
Differential Revision: https://secure.phabricator.com/D21596
Summary:
Ref T13628. When creating a revision, hide the recently introduced "Author" field.
It seems exceedingly unlikely that anyone would ever want to adjust this field when creating a revision, and this workflow already has a lot of fields and complexity, so this likely adds more noise than signal on the balance.
Test Plan:
- Created revisions via copy/paste on the web workflow, no longer saw "Author".
- Used "Edit Revision" to adjust the author normally.
Maniphest Tasks: T13628
Differential Revision: https://secure.phabricator.com/D21595
Summary:
See PHI2015. Diffusion attempts to prevent a commit's author from being made an auditor, but currently uses an out-of-date method for identifying the author.
Use the modern ("Repository Identity" aware) method instead.
Test Plan:
- Authored a commit as user "X", mapped to my account.
- Pushed/imported/discovered it.
- Changed the identity mapping for "X" from my account to a different account.
- Tried to add myself as an auditor.
- Before: error, "author can't be an auditor".
- After: succeeds.
- Tried to add the newly mapped user as an auditor. This correctly fails with the "author can't be an auditor" error.
It's possible to put commits into a wonky state by remapping the author identity to a user who is already an auditor, but I think that isn't important and we can't do much about it, realistically.
Differential Revision: https://secure.phabricator.com/D21594
Summary:
See PHI2015. Currently, "resigned" reviewers/auditors get different icons in Differential and Diffusion.
The Diffusion icon is exceptionally poor and confusing: it does not communicate "resigned" and it is similar to other icons.
For clarity and consistency, use the Differential icon (a grey "X") in both applications.
Test Plan: {F8492303}
Differential Revision: https://secure.phabricator.com/D21593
Summary:
Ref T13628. If you "Foist Upon", then delete the value in the tokenizer, then scroll down to the preview, you currently get an ugly "Unknown Object (???)" rendering.
This is technically correct in some vague sense and the transaction won't apply, but provide a more aesthetic rendering instead.
Test Plan: {F8487050}
Maniphest Tasks: T13628
Differential Revision: https://secure.phabricator.com/D21592
Summary:
Ref T13628. Currently, Differential has a "Commandeer" action, but no way to edit the author otherwise.
This is largely archaic: there is no reason to prevent editing the author, and this makes it difficult to undo mistakes if you commandeer by accident.
Instead, provide a normal "Author" field and a "Foist Upon" action, similar to the "Owner" and "Claim/Assign" fields in Maniphest.
Test Plan:
- Applied author transactions as the old author ("foisted"), the new author ("commandeerd"), and an arbitrary third party ("changed author").
- Tried to unassign author, etc.
- Viewed stories in feed and transaction timeline.
- Saw sensible automatic reviewer changes.
- Used existing "Commandeer" action, which is unchanged.
- Called "transaction.search" and saw reasonable transaction values.
Maniphest Tasks: T13628
Differential Revision: https://secure.phabricator.com/D21591
Summary: T13578
Test Plan:
This method uses the existing transaction. As such, most of the testing focused on the integration between the workflow and transaction. The only change made to the transaction was to allow an omnipotent user to make the change in addition to an admin.
Other than that, I removed the "approved" flag from the user, then ran the command-line utilty until the user was successfully approved.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T13578
Differential Revision: https://secure.phabricator.com/D21587
Summary:
Ref T13627. If a lock fails, the connection may be returned to the pool, even if the connection is an external connection. Under old versions of MySQL, connection reuse can release other locks on the same connection.
Don't return external connections to the pool.
This issue was introduced in D21369.
Test Plan: Added a failing test and made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21585
Summary:
Ref T13627. MySQL versions older than 5.7 release held locks when a new lock is acquired.
Prevent acquisition of a second lock to prevent this.
Test Plan: Added a failing unit test, made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21586
Summary: Ref T13627. This makes the API harder to misuse: setting an external connection on a held lock isn't a meaningful operation. Prevent it.
Test Plan: Added a failing test, made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21584
Summary:
Ref T13627. Currently, global locks always return connections (even external connections) to the connection pool when unlocked.
This code is obviously buggy: `isExternalConnection` is set to false immediately before it is tested. This bug has existed since this code was introduced, in D15792.
- Instead of storing a flag, store the actual connection.
- Don't clear it when unlocking.
- Don't return external connections to the pool.
Test Plan:
- Added a failing test, made it pass.
Maniphest Tasks: T13627
Differential Revision: https://secure.phabricator.com/D21583
Summary:
Ref T13625. See that task for discussion.
Currently, the Viewer when performing revision updates in response to commits may be an arbitrary low-privilege user (an Application, a disabled User, a bot, a mailing list, etc).
Today, this leads to an exception when trying to make API calls.
Ideally, we probably would not perform the update in these cases. However, performing the update isn't a policy violation and is generally less surprising than not performing it, so continue performing it for now: just use the omnipotent user to interact with the API.
Test Plan:
- Authored a commit as a bot user without permission to view the repository or revision.
- Commented out a couple of caches, and used `bin/repository reparse --publish ...` to republish the commit.
- Before: exception when trying to interact with the API.
- After: clean publish.
Maniphest Tasks: T13625
Differential Revision: https://secure.phabricator.com/D21582
Summary:
Ref T13623. When paginating notifications, we may currently construct a query which:
- loads from non-unique rows; and
- returns multiple results.
In particular, `chronologicalKey` isn't unique across the whole table (only for a given viewer). We can get away with this because no user-facing view of notifications is truly "every notification for every viewer" today.
One fix would be to implicitly force the paging query to include `withUserPHIDs(viewerPHID)`, but puruse a slightly more general fix:
- Load only unique stories.
- Explictly limit the pagination subquery to one result.
Test Plan:
- Set page size to 1, inserted duplicate notifications of all stories for another user, clicked "Next", got the GROUP BY error.
- Applied the "only load unique stories" part of the change, got a "expected one row" error instead.
- Applied the "limit 1" part of the change, got a second page of notifications.
Maniphest Tasks: T13623
Differential Revision: https://secure.phabricator.com/D21577
Summary: Ref T13624. Depends on D21578. In "sshd" subprocess contexts, use "PhutilErrorLog" to direct errors to both stderr and, if configured, a logfile on disk.
Test Plan:
- Confiugured an error log.
- Forced `ssh-auth` to fatal.
- Saw errors on stderr and in log.
Maniphest Tasks: T13624
Differential Revision: https://secure.phabricator.com/D21579
Summary: Ref T13611. This property worked correctly when implemented in D19357. The behavior was broken by D20775, which tested node-level routing but did not specifically re-test the "writable" property. This was difficult to spot because ref query outcomes weren't observable in the UI, and the ref itself had the correct property value.
Test Plan:
See D21575. After this change, the UI shows the correct state, rather than showing a read-only service ref as writable:
{F8465865}
Maniphest Tasks: T13611
Differential Revision: https://secure.phabricator.com/D21576
Summary: Ref T13611. Currently, the "writable" property on service bindings has no effect because of a trivial bug. Provide more information in the UI to make this kind of problem observable.
Test Plan:
Viewed "Storage" section of management UI, saw a more-obvious problem with ref management (a non-writable ref is listed as writable).
{F8465851}
Maniphest Tasks: T13611
Differential Revision: https://secure.phabricator.com/D21575
Summary:
Ref T13620.
- Make generic edge stories render links with hovercards. Other story types (like subscriptions) already do this so I'm fairly certain this is just old code from before hovercards.
- Include a longer commit message snippet in hovercards.
Test Plan: {F8465645}
Maniphest Tasks: T13620
Differential Revision: https://secure.phabricator.com/D21574
Summary:
Ref T13617. When an inline comment is added inside a block of added lines, it currently ends up off-by-one when porting forward.
This is a disagreement between the mapping engine and the display engine about what "offset" means. Choose the simpler of the two interpretations.
Test Plan:
- Created a revision with the diff in T13617.
- Added an inline in the middle of the added block.
- Updated the revision with the same diff.
- Before: inline incorrectly moves up by one line.
- After: inline maps correctly.
Maniphest Tasks: T13617
Differential Revision: https://secure.phabricator.com/D21572
Summary: Ref T13586. In the footsteps of D21563, make Herald rule results more formal and structured to support meaningful exception reporting.
Test Plan:
Ran various Herald rules and viewed transcripts, including rules with recursive dependencies and condition exceptions.
{F8447894}
Maniphest Tasks: T13586
Differential Revision: https://secure.phabricator.com/D21565
Summary: Ref T13586. Lift the behavioral core of "HeraldConditionResult" into a new abstract base "HeraldTranscriptResult", with the intent to introduce a "HeraldRuleResult".
Test Plan:
- Ran Herald rules, reviewed transcripts.
- This change should have no behavioral effect.
Maniphest Tasks: T13586
Differential Revision: https://secure.phabricator.com/D21564
Summary:
Ref T13586. Currently, Herald condition logs encode "pass" or "fail" robustly, "forbidden" through a sort of awkward side channel, and can not properly encode "invalid" or "exception" outcomes.
Structure the condition log so results are represented unambiguously and all possible outcomes (pass, fail, forbidden, invalid, exception) are clearly encoded.
Test Plan:
{F8446102}
{F8446103}
Maniphest Tasks: T13586
Differential Revision: https://secure.phabricator.com/D21563
Summary: Ref T13586. The Herald transcript page has become more and more complicated over time, and recently added "Transactions" and "Profiler" sections. Split these across separate navigation tabs to limit the maximum complexity of any single view and make it easier to navigate to particular sections, like the profiler section.
Test Plan: Viewed various transcripts, saw nice digestible sections.
Maniphest Tasks: T13586
Differential Revision: https://secure.phabricator.com/D21493
Summary: Ref T13615. This property was removed from the Facebook API at some point, perhaps November 2020. Stop relying no it.
Test Plan: Created a local Facebook OAuth app, registered a new account locally.
Maniphest Tasks: T13615
Differential Revision: https://secure.phabricator.com/D21571
Summary: Ref T13609. Add the Object PHID (object being built), Container PHID (container of the object being built), Build PHID, and Buildable PHID to Harbormaster build variables.
Test Plan:
{F8448191}
{F8448192}
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13609
Differential Revision: https://secure.phabricator.com/D21569
Summary:
Ref T13608. When searching for bare URIs in remarkup text, don't look for URIs with a protocol string longer than 32 characters.
This avoids a case where the regexp engine may be tricked into executing at `O(N^2)` or some similar complexity.
Test Plan:
- Applied remarkup to "AAAA..." (512KB).
- Before: 64 seconds to process.
- After: <10ms to process.
- Ran unit tests.
Maniphest Tasks: T13608
Differential Revision: https://secure.phabricator.com/D21562
Summary:
Ref T13587. D21495 has significant changes to the ngram indexer, which might possibly contain bugs.
Make it easier to reindex a subset of documents (based on the date when the index was built, and/or the software version which generated the index).
This is in addition to the existing versioning, which is focused on object versions.
Test Plan: Ran `bin/search index` with various old and new arguments. Spot-checked the `IndexVersion` table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13587
Differential Revision: https://secure.phabricator.com/D21560
Summary:
Ref T13587. Currently, when a document is reindexed by Ferret, the old document is completely discarded and a new version is inserted to replace it.
This approach is simple to implement, but can lead to exhaustion of the ngram AUTO_INCREMENT id column in reasonable circumstances.
Conceptually, this approach "should" be fine and this exhaustion is an awkard implementation detail. However, since it's easy to be less wasteful when performing document updates and all the other approaches are awkward or leaky in other ways that are probably worse, use a more complex implementation to avoid executing unnecessary INSERT statements.
Test Plan:
- Created and indexed a new document, searched for it.
- Updated a document, indexed it with `bin/search index ... --force --trace`, saw only modifications updated in the index.
- Searched for newly added terms (got hits) and removed terms (no longer got hits) to verify add/delete index behavior.
Maniphest Tasks: T13587
Differential Revision: https://secure.phabricator.com/D21495
Summary: Ref T13607. Add some time-oriented constraints to this API method to support compiling build statistics.
Test Plan:
- Called "harbormaster.target.search" with all new constraints.
- Viewed documentation in API console.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13607
Differential Revision: https://secure.phabricator.com/D21559
Summary: Ref T13605. Support selecting a diff's changesets (to get a list of affected file paths) via the API.
Test Plan: Called API with no arguments, diffPHIDs, PHIDs, IDs. Got sensible output.
Maniphest Tasks: T13605
Differential Revision: https://secure.phabricator.com/D21558
Summary:
Ref T13605. Changesets currently have no PHID, which limits their ability to use standard API infrastructure.
Give them a PHID, since there's no reason they don't have one other than their age.
Test Plan:
- Ran migrations, saw PHIDs populated.
- Created new changesets, saw PHIDs.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13605
Differential Revision: https://secure.phabricator.com/D21557
Summary: Ref T13602. Warn when a reivison has at least one human reviewer, no non-human reviewers, and no human reviewers can view it.
Test Plan: {F8430683}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21556
Summary: Ref T13602. Similar to subscriber and mention treatments, make it clear when a user doesn't have view permission.
Test Plan: {F8430595}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21555
Summary:
Ref T13602. When rendering a user hovercard, pass the object on which the reference appears. If the user can't see the object, make it clear on the hovecard.
Restyle the "nopermission" markup in mentions to make it more obvious what the style means: instead of grey text, use red with an explicit icon.
Test Plan: {F8430398}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21554
Summary:
Ref T13602. Currently, Hovercards are functions only of the object they represent (and the viewer, etc).
Recent changes to how users who can't see an object are rendered motivate making them a function of both the object they represent //and// the context in which they are being viewed. In particular, this enables a hovecard for a user to explain "This user can't see the thing you're lookign at right now.", so visual "exiled" markers can have a path forward toward discovery.
Test Plan:
- This change isn't expected to affect any behavior.
- Viewed hovercards, moused over/out, resized windows, viewed standalone cards, viewed debug cards, saw no behavioral changes.
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21553
Summary:
Ref T13602. Currently, timeline comment rendering does not (by default) propagate the context object to the rendering layer.
This means that `@mentions` of users who can't see the object aren't rendered properly (currently: they show up as blue, but should show up as grey).
Pass the context down the stack and into the remarkup engine.
Test Plan: {F8382905}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21548
Summary:
Ref T13602. When a subscriber can't see an object, it's currently hard to figure it out.
Show this status clearly in the curtain UI.
Test Plan: {F8382865}
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21547
Summary:
Ref T13602. Currently, the policy framework can not execute "test if many users can see one object" particluarly efficiently. This test must be executed more broadly to implement the changes in T13602.
To avoid making this any worse than it already is, lift this block into a wrapper class that has a bulk queue + fetch API and could eventually be optimized.
Test Plan: Viewed a task with an `@mention` of a user without permission to see it in the summary, saw it rendered in a disabled style.
Maniphest Tasks: T13602
Differential Revision: https://secure.phabricator.com/D21546
Summary:
Ref T13395. Libphutil has merged into Arcanist and no longer needs to be installed or upgraded. Additionally:
- The minimum PHP version is now PHP 5.5.
- Although older versions of PHP should still install APC, modern versions come with Opcache and do not need APC. Setup issues guide administrators thorugh the correct install procedure now.
Test Plan: Read documentation.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D21550
Summary:
Ref T13600. When migrating observed repositories between cluster services, impact can be better controlled by fetching a copy of the repository on the target host before clusterizing it.
In particular, in the Phacility cluster, migrations are generally from one shared shard to one dedicated shard. It's helpful to perform these migrations synchronously without waiting for the cluster to sync in the background (helpful in the sense that there are fewer steps and fewer commands to run).
This supports an "--observe" mode to the internal "bin/services load-repository" workflow, which transfers repository data by refetching it from the remote rather than by getting it from the older host. This fetch occurs before cluster configuration is adjusted.
Test Plan: Ran locally as a sanity check, will apply in production.
Maniphest Tasks: T13600
Differential Revision: https://secure.phabricator.com/D21544
Summary:
See <https://discourse.phabricator-community.org/t/i-cant-create-almanac-space/4424/>.
Almanac namespaces have never really had property support, but they implemented the interface in the original implementation.
At the time, this had no effect. Later changes integrated properties into the edit flows and broke this no-op integration.
Remove the interface for now. They could be given property support later, but need a bit of support code.
This feature is very rarely used and primarily useful for Phacility instances.
Test Plan: Created new namespaces and edited namespaces, browsed namespace UI.
Differential Revision: https://secure.phabricator.com/D21543
Summary:
Ref T13588. This has never been meaningful, but a "final private" method is specifically forbidden in PHP8.
Remove meaningless "final" from these methods, per new lint checks.
Test Plan: Ran `arc lint --everything` to identify affected methods, then `... | xargs -n1 arc lint --apply-patches`.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21540
Summary: Ref T13591. Provide some guidance on the most common cases for wanting to interact with the worker queue.
Test Plan: Read documentation.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21536
Summary: Ref T13591. Support delaying selected tasks until a later time and bulk-adjustment of task priority.
Test Plan: Ran `bin/worker delay` and `bin/worker priority` to delay and reprioritize tasks. Confirmed outcomes with daemon console.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21535
Summary:
Ref T13591. Add more selector flags to let "bin/worker" commands operate on tasks by container PHID, object PHID, priority, etc.
This anticipates adding "bin/worker reprioritize" and "bin/worker delay" workflows, to provide more tools for handling repository imports.
Test Plan:
- Ran `bin/worker execute`, `cancel`, `retry`, and `free` with various sets of selector flags.
- Used `--min-priority`, `--max-priority`, `--object`, `--container`, `--archived`, `--max-failure-count` to select tasks.
- Specified invalid, duplicate, aliased objects with "--object".
- Specified invalid range priority selectors.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21534
Summary: Ref T13591. This is a minor consistency change to use PHIDs instead of IDs in the commit import processing pipeline. PHIDs are generally more powerful in more contexts and it would be unusual for a modern worker to use an ID here.
Test Plan:
- Made the "accept either ID or PHID" part of the change only.
- Pushed a commit, parsed and reparsed it step by step (this tests that "commitID" tasks can still process normally).
- Made the "write PHIDs" part of the change.
- Pushed a commit, parsed and reparsed it step by step.
- Looked at the task row in the database, saw PHID data.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21533
Summary:
Ref T13591. Improve how parameters are passed between commit worker tasks:
- Always pass "via", to track where tasks came from.
- Always provide "objectPHID" (with the commit PHID).
- Always provide "containerPHID" (with the repository PHID).
Test Plan:
- Pushed a new commit.
- Ran `bin/repository pull` + `bin/repository discover`, saw commit with all parameters.
- Ran `bin/worker execute ...`, saw a Change worker and then a Publish worker with appropriate parameters.
- Ran `bin/repository reparse ... --background`, saw workers queue with appropriate parameters.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21532
Summary:
Ref T13591. Worker queue tasks which affect commits currently (mostly) store the commit as an "objectPHID", but do not directly reference the repository the commit belongs to.
This can make certain operations (like "change the priority of all tasks affecting repository Y") more difficult than it needs to be.
Support a "containerPHID", similar to the field of the same name on builds, that can store a parent object like a repository and better support operations against subsets of tasks.
See also D11044 for the genesis of "objectPHID".
This depends on the introduction of storage patch phases (in D21529) so that earlier migrations which queue worker tasks don't try to insert this column before it actually exists.
Test Plan:
- Ran `bin/storage upgrade`.
- No callers yet, see further changes for usage.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21531
Summary:
Ref T13591. Some storage patches queue worker tasks, currently always to rebuild search indexes.
These patches can not execute in creation order if a later patch modifies the worker task table, since they'll try to perform a modern INSERT against an out-of-date table schema. Such a modification is desirable in the context of T13591, but making it causes these patches to fail.
Patches have an existing "after" mechanism which allows them to have explicit dependencies. This mechanism could be used to resolve this issue, but all patches with a dependency like this would need to be updated every time the queue table changes.
Instead, introduce "phases" to provide broader ordering rules. There are now two phases: "default" and "worker". Patches in the "worker" phase execute after patches in the "default" phase.
Phases may eventually be further separated, but
Test Plan:
- Ran `bin/storage status`, saw patches annotated with phases.
- Will apply `containerPHID` changes on top of this.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21529
Summary:
Ref T13596. See that task for discussion. Executing "INSERT ... SELECT" at default isolation levels requires more locking than executing "SELECT" + "INSERT" separately.
Decompose this "INSERT ... SELECT" into "SELECT + INSERT", and reformat it to execute a minimal set of changes instead of wiping everything out and then writing all of it back. In most cases, this means we write 1 row instead of `O(number of project members)` rows.
Test Plan:
- Created a project. Added and removed members, looked at database and saw a consistent membership/materialization list.
- Created a subproject. Added and removed members, looked at database and saw a consistent membership/materialization list.
I wasn't successful in reproducing the LOCK WAIT issue locally by trying various concurrent SELECT / INSERT / INSERT ... SELECT strategies. It may depend on the "DELETE + INSERT ... SELECT" structure used here, or versions/config/etc, so we'll have to see how that fares in production.
Maniphest Tasks: T13596
Differential Revision: https://secure.phabricator.com/D21527
Summary:
See PHI1983. Ref T13599. Ref T13589. Currently, if you browse to a path browse URI in Diffusion without a trailing slash (`/browse/master/src`), you get a nonsensical view (the directory as a single item).
Be more precise in how "git ls-tree" arguments are constructed.
Test Plan: Visited files and directories in the browse view, with and without trailing slashes. Saw improved behavior for directories with no trailing slash and reasonable behavior in all other cases.
Maniphest Tasks: T13599, T13589
Differential Revision: https://secure.phabricator.com/D21528
Summary:
Ref T13590. By default, PHP kills execution after web scripts run for 30 seconds. If this occurs in the locked section of a repository write while we're holding the durable write lock, the lock will get stuck.
Use "set_time_limit(0)" to prevent this mechanism from interrupting execution while the durable lock is held.
Test Plan:
- Added "set_time_limit(1)" before the lock and "while (1);" in the critical section of the lock.
- Pushed, got the lock stuck.
- Cleared the lock, applied this patch, pushed.
- Got an infinite hang instead. (Normally, we expect the script to take more than 30 seconds to execute because there is a large push that executes in finite time, not because there's an infinte loop.)
Maniphest Tasks: T13590
Differential Revision: https://secure.phabricator.com/D21526
Summary:
Ref T13590. Currently, errors arising from cluster locking (like the "stuck write lock" exception) are not caught and converted into VCS responses on the HTTP VCS workflow.
Catch a broader range of exceptions and convert them into appropriate responses.
Test Plan:
- Forced a "stuck write lock" exception, pushed to a Git repository over HTTP.
- Before: generic fatal.
- After: VCS-specific fatal with a useful message in the "X-Phabricator-Message" response header.
Maniphest Tasks: T13590
Differential Revision: https://secure.phabricator.com/D21525
Summary:
Ref T13590. User objects have some inline caches that don't do readthrough generation by default because it may be indicative of high-impact performance problems in code.
During a VCS request, these caches are normally unnecessary, but they may be hit on some unusual pathways (like error handling).
Flag VCS users as okay for inline generation. This does not indicate a performance problem and access to these caches is very rare, at least today.
Test Plan:
- Executed a Git HTTP request which hit an unhandled exception (stuck write lock).
- Before: got a second-level exception while handling the first exception, when trying to access user preferences to render a standard uncaught exception page.
- After: no second-level exception.
Maniphest Tasks: T13590
Differential Revision: https://secure.phabricator.com/D21524
Summary:
Ref T13590. Currently, when you encounter a HTTP error in Git, there is no apparent way to make the client show any additional useful information. In particular, the response body is ignored.
We can partially get around this by putting the information in an "X-Phabricator-Message: ..." HTTP header, which is visible with "GIT_CURL_VERBOSE=1 git ...". Users won't normally know to look here, but it's still better than nothing.
Test Plan:
- Ran "GIT_CURL_VERBOSE=1 git fetch" against a Phabricator HTTP URI that returned a HTTP/500 error.
- Before: no clue what happened on the client.
- After: client shows useful message in the "X-Phabricator-Message" header in debug output.
Maniphest Tasks: T13590
Differential Revision: https://secure.phabricator.com/D21523
Summary:
Ref T13593. The commit cache in this Engine has a maximum fixed size (currently 65,535 entries).
If we execute discovery in a repository with more refs than this (e.g., 180K), we get fast lookups for the first 65,535 refs and slow lookups for the remaining refs.
Instead, divide the refs into chunks no larger than the cache size, and perform an explicit cache fill before each chunk is processed.
Test Plan:
- Created a repository with 1K refs. Set cache size to 256. Ran discovery.
- Before patch: saw one large cache fill and then ~750 single-gets.
- After patch: saw four large cache fills.
- Compared `bin/repository discover ... --verbose` output before and after patch for overall effect; saw no differences.
Maniphest Tasks: T13593
Differential Revision: https://secure.phabricator.com/D21521
Summary:
Ref T13595. See that task for discussion.
D21511 renamed the iteration variable here (previously "$path") but did not rename this use of it.
Test Plan:
- In Diffusion, browsed a directory with a submodule.
- Before: "setExternalURI()" fatal in conduit call.
- After: directory listing including submodule.
Maniphest Tasks: T13595
Differential Revision: https://secure.phabricator.com/D21520
Summary:
Ref T13589. The output for "git ls-tree commit:path" (the old invocation) and "git ls-tree commit -- path" (the new invocation) differs: the latter emits absolute paths.
Update the code to account for this difference in behavior.
Test Plan:
- Browsed a non-root directory in a Git repository in Diffusion.
- Before: saw absolute paths.
- After: saw relative paths.
Maniphest Tasks: T13589
Differential Revision: https://secure.phabricator.com/D21519
Summary: Ref T13591. Fixes a few issues with the recent updates here discovered in more thorough testing.
Test Plan:
- Stopped the daemons.
- Created a new copy of Phabricator in Diffusion.
- Pulled it with `bin/repository pull ...`.
- Got 17,278 commits on disk with `git log --all --format=%H`.
- Set permanent refs to "master".
- Discovered it with `bin/repository discover ...`.
- This took 31.5s and inserted 17,278 tasks.
- Verified that all tasks have priority 4,000 (PRIORITY_IMPORT).
- Observed that 16,799 commits have IMPORTED_PERMANENT and 479 commits do not.
- This matches `git log master --format=%H` exactly.
- Ran `bin/repository refs ...`. Expected no changes and saw no changes.
- Ran `bin/worker execute --active` for a minute or two. It processed all the impermanent changes first (since `bin/worker` is LIFO and these are supposed to process last).
- Ran `bin/repository refs`. Expected no changes and saw no changes.
- Marked all refs as permanent.
- Starting state: 16,009 message tasks, all at priority 4000.
- Ran `bin/repository refs`, expecting 479 new tasks at priority 4000.
- Saw count rise to 16,488 as expected.
- Saw all the new tasks have priority 4000 and all commits now have the IMPORTED_PERMANENT flag.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21518
Summary: Ref T13591. This is mostly a workaround for Big Sur not having pcntl/posix installed by default and the mess with M1 / Homebrew / SIP / Code Signing (see T13232) so I can't easily run actual daemons and need to fake them with `bin/worker execute --active`, but it's a reasonable flag on its own.
Test Plan:
- Ran `bin/worker execute --active` and `bin/worker cancel --active`.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21517
Summary:
Ref T13591. There are currently two pathways to queue an import task for a commit: via repository discovery, or via a ref becoming permanent.
These pathways duplicate some logic and have behavioral differences: one does not set `objectPHID` properly, one does not set the priority correctly.
Unify these pathways, make them both set `objectPHID`, and make them both use the same priority logic.
Test Plan:
- Discovered refs.
- See later changes in this series for more complete test cases.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21516
Summary:
Ref T13591. Currently, the "IMPORTED_PERMANENT" flag (previously "IMPORTED_CLOSEABLE", until D21514) flag is set by using the result of "shouldPublishRef()".
This method returns the wrong value for the flag when there is a repository-level reason not to publish the ref (most commonly, because the repository is currently importing).
Although it's correct that commits should not be published in an importing repository, that's already handled in the "PublishWorker" by testing "shouldPublishCommit()". The "IMPORTED_PERMANENT" flag should only reflect whether a commit is reachable from a permanent ref or not.
- Move the relevant logic to a new method in Publisher.
- Fill "IMPORTED_PERMANENT" narrowly from "isPermanentRef()", rather than broadly from "shouldPublishRef()".
- Deduplicate some logic in "PhabricatorRepositoryRefEngine" which has the same intent as the logic in the Publisher.
Test Plan:
- Ran discovery on a new repository, saw permanent commits marked as permanent from the beginning.
- See later changes in this patch series for additional testing.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21515
Summary:
Ref T13591. This is an old flag with an old name, and there's an import bug because the outdated concept of "closable" is confusing two different behaviors.
This flag should mean only "is this commit reachable from a permanent ref?". Rename it to "IMPORTED_PERMANENT" to make that more clear.
Rename the "Unpublished" query to "Permanent" to make that more clear, as well.
Test Plan:
- Grepped for all affected symbols.
- Queried for all commmits, permament commits, and impermanent commits.
- Ran repository discovery.
- See also further changes in this change series for more extensive tests.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21514
Summary:
Ref T13591. Since D8781, this flag does not function correctly in Git and Mercurial repositories, since ref discovery pre-fills the cache.
Move the "don't look at the database" behavior the flag enables into the cache lookup. D8781 should have been slightly more aggressive and done this, it was just overlooked.
Test Plan:
- Ran `bin/repository discover --help` and read the updated help text.
- Ran `bin/repository discover --repair` in a fully-discovered Git repository.
- Before: no effect.
- After: full rediscovery.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21513
Summary:
Ref T13589. In D21510, not every ref selector got touched, and this isn't a valid construction in Git:
```
$ git ls-tree ... -- ''
```
Thus:
- Disambiguate more (all?) ref selectors.
- Correct the construction of "git ls-tree" when there is no path.
- Clean some stuff up: make the construction of some flags and arguments more explicit, get rid of a needless "%C", prefer "%Ls" over acrobatics, etc.
Test Plan: Browsed/updated a local Git repository. (This change is somewhat difficult to test exhaustively, as evidenced by the "ls-tree" issue in D21510.)
Maniphest Tasks: T13589
Differential Revision: https://secure.phabricator.com/D21511
Summary: Ref T13589. See that task for discussion.
Test Plan: Executed most commands via "bin/conduit" or in isolation.
Maniphest Tasks: T13589
Differential Revision: https://secure.phabricator.com/D21510
Summary: These characters are missing support in `{key ...}` but are reasonable to include.
Test Plan: {F8302969}
Differential Revision: https://secure.phabricator.com/D21508
Summary: Ref T13575. Particularly with the new Apple silicon, I think there are enough domain collisions for `M1`, `M2`, `P1`, etc., to justify adding them to the default ignore list.
Test Plan: Created a mock, then wrote a comment referencing an object on the list (`M1`) and an object not on the list (`T1`). Got text and a link respectively.
Maniphest Tasks: T13575
Differential Revision: https://secure.phabricator.com/D21507
Summary:
Ref T13575. Since PHP builtin webserver support was added, the pathway for parsing request parameters became more complex. We now rebuild "$_REQUEST" later, and this rebuild will destroy any mutations made to it here, so the assignment to "__path__" is lost.
Instead of "validating" the request path, make this method "read" the request path and store it explicitly, so it will survive any later request mutations.
Test Plan:
- Submitted any POST form while running Phabricator under the builtin PHP webserver. Old behavior was an error when accessing "__path__"; new behavior is a working application.
- Loaded normal pages, etc.
Maniphest Tasks: T13575
Differential Revision: https://secure.phabricator.com/D21506
Summary: Without this change, the landing page for the Packages app is https://secure.phabricator.com/packages, which is a 404. There's probably a better way to fix this, but this was the fewest characters.
Test Plan: doitlive
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21494
Summary: Ref T13585. Provide a minimal but technically functional "harbormaster.step.edit" API method.
Test Plan: Used the web console to modify the URI for a "Make HTTP Request" build step.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13585
Differential Revision: https://secure.phabricator.com/D21489
Summary: Ref T13585. This isn't particularly useful (notably, it does not include custom field values and isn't searchable by build plan PHID) but get the basics into place.
Test Plan: Used the web UI to make API calls, reviewed results.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13585
Differential Revision: https://secure.phabricator.com/D21488
Summary:
Ref T2312. Numeric strings are read out of arrays as integers, and modern PHP raises appropriate warnings when they're then treated as strings.
For now, cast the keys to strings explicitly (we know we inserted only strings). In the future, introduction of a `StringMap` type or similar might be appropriate.
Test Plan:
- Added "abc.12345.xyz" to the blocklist, changed my VCS password.
- Before: fatal when trying to "strpos()" an integer.
- After: password change worked correctly.
Maniphest Tasks: T2312
Differential Revision: https://secure.phabricator.com/D21487
Summary:
Changes the heuristic method by which non-zero exit statuses from git-http-backend are found to be due to packfile negotiation during shallow fetches, etc.
Instead of checking git-http-backend stderr for a generic "hung up" error message, see if the pack-result response contains a terminating flush packet ("0000"). This should give a greater assurance that the request was handled correctly and the response is complete.
Test Plan: Run `GIT_CURL_VERBOSE=1 git fetch --depth 1 https://host.example/source/repo.git HEAD` to ensure it completes and includes two successful POST requests during packfile negotiation (the last one actually receives the packfile).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, dzduvall
Tags: #diffusion
Differential Revision: https://secure.phabricator.com/D21484
Summary:
See <https://discourse.phabricator-community.org/t/mail-details-view-broken/4315>. The change in D21400 detects a missing "GROUP BY" in some variations of this query.
Specifically, we may join multiple recipient rows (since mail may have multiple recipients) and then fail to group the results.
Fix this by adding the "GROUP BY". Additionally, remove the special-cased behavior when no authors or recipients are specified -- it's complicated and not entirely correct (e.g., may produce a "no object" instead of a policy error when querying by ID), and likely predates overheating.
Test Plan:
- Disabled `metamta.one-mail-per-recipient` in Config.
- Generated a message to 2+ recipients.
- Viewed the message detail; queried for the message by specifying 2+ recipients.
- Viewed the unfiltered list of messages, saw the query overheat.
Differential Revision: https://secure.phabricator.com/D21486
Summary:
See PHI1876. Normally, deleted inlines are undeleted with an "undelete" operation, which clears the "isDeleted" flag.
However, when an inline is deleted implicitly by using "Cancel" without first saving it, the flag currently isn't cleared properly. This can lead to cases where inlines seem to vanish (they are shown to the user in the UI, but treated as deleted on submission).
Test Plan:
There are two affected sequences here:
- Create a new inline, type text, cancel, undo.
- Create a new inline, type text, cancel, undo, save.
The former sequence triggers an "edit" operation. The subsequent "Save" in the second sequence triggers a "save" operation.
It's normally impossible in the UI to execute a "save" without executing an "edit" first, but "save" clearly should undelete the comment if you get there somehow, so this change clears the deleted flag in both cases for completeness.
- Executed both sequences, saw comment persist in preview, on reload, and after submission.
Differential Revision: https://secure.phabricator.com/D21483
Summary: See PHI1912. Ref T13491. "arc" now requires "--" when stdin is not a TTY; provide this argument for users.
Test Plan: Viewed example in console, saw "--". Executed example.
Maniphest Tasks: T13491
Differential Revision: https://secure.phabricator.com/D21482
Summary:
See PHI1900. Recent changes to how commit graphs are drawn made the height automatic in most cases, but it fails in Differential because the element isn't initially visible so the computed height is 0.
Just give them an explicit height so they show up again.
Test Plan: Viewed graphs in Maniphest, Differential, and Diffusion; saw them all render properly.
Differential Revision: https://secure.phabricator.com/D21481
Summary:
See PHI1901. An install would like improved support for identifying files related to an object (like a task or revision) for retention/archival/backup/migration/snapshotting purposes.
The "attachment" edge is not really user-level: it just means "if you can see the object, that allows you to see the file". This set includes files that users may not think of as "attached", like thumbnails and internal objects which are attached for technical reasons.
However, this is generally an appropriate relationship to expose for retention purposes.
Test Plan: Used "edge.search" to find files attached to a revision and objects attached to a file.
Differential Revision: https://secure.phabricator.com/D21480
Summary: Ref T13583. To improve support for making it harder to improperly mix data retention policies, allow Herald to act on comment content.
Test Plan:
- Wrote comment content Herald rules in Maniphest and Differential.
- Submitted non-matching comments (no action) and matching comments (Herald action).
- In Differential, triggered rules by submitting non-matching main content and a matching inline comment.
Maniphest Tasks: T13583
Differential Revision: https://secure.phabricator.com/D21479
Summary:
See PHI1896. If you do this:
- Create an inline comment over a wide range of lines.
- Suggest an edit.
- Make a change near the beginning of the block.
- Make a change near the end of the block.
- Save the inline.
...you get a rendering which includes a "Show More Context" fold in the middle.
Currently, this element renders in a visually broken way and consumes too many columns.
However, this element isn't ever desirable inside inline comment suggestions. Stop it from rendering entirely.
Test Plan:
- Made an inline comment suggestion across lines 1-50 with edits at the beginning and end, saw a contiguous diff.
- Made smaller inline comment suggestions (one line, a few lines).
Differential Revision: https://secure.phabricator.com/D21476
Summary:
Ref T13552. The behavior of "RepositoryQuery" with ambiguous identifiers under "withRepositoryPHIDs()" is tricky. This leads to failure to load commits in Subversion in some cases.
Use "withRepository()", which gives us the correct identifier resolution behavior.
Test Plan: Viewed a subversion repository history in Diffusion, saw commit details after change.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21469
Summary:
Ref T13552. The Herald field "Accepted Differential revision" (and similar fields) depend on the task/revision update steps running before Herald executes.
Herald currently executes first, so it never sees associated revisions. Swap this order.
Test Plan: Published a commit, got a clean parse/import. Will test with production rules ("Cowboy Commits").
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21468
Summary: See PHI1885. Repository operations are queryable by state and author, but neither column has a usable key. Add usable keys.
Test Plan: Ran EXPLAIN on a state query. Ran `bin/storage upgrade`. Ran EXPLAIN again, saw query go from a table scan to a `const` key lookup.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D21465
Summary:
Ref T13581. Currently, unexpected exceptions inside Conduit calls are passed to the client, but not logged on the server.
These exceptions should generally be unexpected, and producing a server-side trace is potentially useful.
Test Plan: Simulated a during-execution exception, saw it get logged on the server.
Maniphest Tasks: T13581
Differential Revision: https://secure.phabricator.com/D21464
Summary:
Ref T13581. If you query for revisions by hash and provide multiple hashes (A, B) which match a single revision (e.g., older and newer diffs for that revision), the query omits a GROUP BY clause but should contain one.
Add a GROUP BY clause in this case.
Test Plan:
With a working copy that has multiple hashes corresponding to a single revision, ran `arc branches` before and after the change. Before, got this error:
```
[2020-09-15 17:02:07] EXCEPTION: (ConduitClientException) ERR-CONDUIT-CORE: Rows passed to "loadAllFromArray(...)" include two or more rows with the same ID ("130"). Rows must have unique IDs. An underlying query may be missing a GROUP BY. at [<arcanist>/src/conduit/ConduitFuture.php:65]
```
After, clean execution.
Maniphest Tasks: T13581
Differential Revision: https://secure.phabricator.com/D21462
Summary:
Ref T13552. When a previously discovered commit becomes reachable from a permanent ref, we re-queue workers to update it. However, the commit may already be marked as "published", so the publish worker may do nothing.
It would perhaps be simpler to not mark the commit as published when it isn't reachable from a permanent ref, but this is tricky because the flag is also part of the "imported / all steps" state (see T13580).
Until that can be cleaned up, just clear the flag.
Test Plan:
- Pushed a commit with "fixes X" to a non-permanent branch.
- Pushed it to a permanent branch.
- Before change: task failed to close.
- After change: task closes properly.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21460
Summary:
Ref T13552. Fixes T13569. Currently, if a process uses in-process tasks (usually, a debugging/diagnostic workflow) and those tasks (or tasks those tasks queue) fail permanently, the exception escapes to top level and the process exits.
This isn't desirable; catch the exception and fail them locally instead.
Test Plan:
With a failing Asana integration and misconfigured Webhook, ran `bin/repository reparse --publish ...`.
- Before: fatals on each substep.
- After: warnings emitted for failed substep, but process completes.
Maniphest Tasks: T13569, T13552
Differential Revision: https://secure.phabricator.com/D21459
Summary:
Ref T13552. Now that these steps can build their own "CommitRef" object from storage on the "CommitData" object, move them from the "Message" step to the "Publishing" step.
This should resolve the root issue in T13552, where a commit moved from a non-permanent branch to a permanent branch does not publish closures properly.
Test Plan: Used "bin/repository reparse --publish ..." to republish changes.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21450
Summary:
Ref T13552. Turn "CommitData" into an application-level layer on top of the repository-level "CommitRef" object.
For older commits which will not have a "CommitRef" record on disk, build a synthetic one at runtime. This could eventually be migrated.
Test Plan: Ran "bin/repository reparse --message", browsed Diffusion.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21449
