Summary:
When previously known commits have been destroyed in a Mercurial repository, Phabricator does not end up marking the commits as unreachable. This results in daemon tasks which continuously fail and retry.
This updates `PhabricatorRepositoryDiscoveryEngine` and `PhabricatorManagementRepositoryMarkReachableWorkflow` to include support of marking commits as unreachable for Mercurial repositories.
The `PhabricatorMercurialGraphStream` also needed updated to support a stream with no starting commit.
Refs T13634
Test Plan:
1. I set up a hosted Mercurial repository.
2. I removed the head commit from the on-disk repository state.
3. I attempted to load the repository page and saw an exception due to a missing commit.
4. I went to `/manage` for the repository and scheduled an update of the repository.
5. After an updated performed, I went to the repository main page and saw there was no exception and the history view properly did not have the commit I had removed.
6. I checked the phd logs and verified there were no exceptions related to the repository.
7. I ran the `./bin/repository mark-reachable` command on the Mercurial repository and it reported that it marked the commit as unreachable.
8. I pushed the same commit back upstream and verified that the commit was found and displayed in the history view of the repository page and `mark-unreachable` did not identify it as being unreachable.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T13634
Differential Revision: https://secure.phabricator.com/D21715
Summary:
The history query for the repository page isn't actually used to display any content. It looks like it was previously used to display the last user which modified a file however this looks to be removed in D21404. This removes the history query from happening as well as updates `DiffusionBrowseTableView` to remove the parameters for passing this information in, resulting in also updating `DiffusionBrowseController` to no longer need to put this information together.
Refs T13666
Test Plan:
1. I removed commits from a repository on the local state.
2. I navigated to the repository's landing page and saw that the landing page attempted to render content and only failed to load the browse files section.
3. I navigated to the history tab and verified that it showed an exception about failing to query commit information.
4. I restored the repository working state to function properly.
5. I navigated to a repository's landing page and verified it loaded properly, including showing the last modified date for each file.
6. I navigated to the Code, Branches, Tags, and History tabs to verify each tab page loaded properly.
7. I verified on the Code tab that the last modified date for each file displayed properly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T13666
Differential Revision: https://secure.phabricator.com/D21717
Summary:
Depends on D21720. Ref T13666. See D21720 for additional discussion.
Use "MethodCallFuture", introduced in D21720, so that exceptions raised in "execute()" are thrown when the future is resolved, not when the future is created.
This makes exception behavior for clustered and non-clustered setups consistent, and chooses the intended (clustered) behavior in both cases, which currently deals with errors better.
Test Plan:
- Applied both parts of the patch in T13666 (break history queries, force immediate futures) to reproduce the issue in T13666.
- Loaded a Diffusion landing page, reproduced the error described in that task.
- Applied this patch, verified landing page works again.
- Removed the "break history queries" change, verified landing page works in forced-immediate mode.
- Removed the "force immediate" change, verified landing page works in "actual future" mode.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Maniphest Tasks: T13666
Differential Revision: https://secure.phabricator.com/D21721
Summary: With this change, links to documentation will point to we.phorge.it rather then secure.phabricator,com
Test Plan: No testing needed - simple URL update
Reviewers: O1 Blessed Committers, deadalnix
Reviewed By: O1 Blessed Committers, deadalnix
Subscribers: taavi, speck, tobiaswiese
Differential Revision: https://we.phorge.it/D25014
Summary:
This diff adds conduit methods for searching for legalpad documents and signatures. This is very helpful for auditing who's actually signed a document. It also fixes the "contributorPHIDs" constraint in the existing search engine.
In order to expose legalpad signatures through Conduit, this adds a `phid` column to the `legalpad_documentsignature` table. It includes a migration (in the style of many previous phid-adding migrations) to actually populate the column.
Test Plan: We run this on my company's internal fork and it seems to work okay. I don't think any other conduit methods anywhere have tests (???), but if you can point me at one I'm glad to write a unit test!
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: 20after4, speck, tobiaswiese
Differential Revision: https://we.phorge.it/D25018
Summary: Fixes T13663. `supportsSubtypes` tries to create an editable object, but this isn't always valid for `PhabricatorCalendarImport`. Use `instanceof` instead.
Test Plan:
- Edited calendar import, tasks (2 different subtypes), and projects (2 different subtypes).
- Changed task subtypes using {nav Change Subtype} action and batch editor.
- Changed task and project subtypes using Conduit.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13663
Differential Revision: https://secure.phabricator.com/D21714
Summary:
Fixes T13662. Phriction currently passes a map as a "context object", but this code is ancient and predates the modern meaning of a "context object". In modern code, context objects should be real objects.
Provide a real object as a context object. We do this by either loading the actual document or constructing a synthetic version of it.
Test Plan:
- Edited an existing document, observing the preview:
- Used a mention rule, saw a preview.
- Used `[[ a ]]` and `[[ ./a ]]` absolute and relative reference rules, saw accurate previews.
- Edited a new document, observing the preview:
- Used a mention rule, saw a preview.
- Used absolute/relative references, saw accurate previews.
- Grepped for other references to the removed properties (`phriction.isPreview` and `phriction.slug`), found none remaining.
Reviewers: 0
Reviewed By: 0
Maniphest Tasks: T13662
Differential Revision: https://secure.phabricator.com/D21709
Summary:
Ref T13662. I ran into this while trying to reproduce the mention issue discussed there.
Currently, the root document (with slug "/") attempts to preview using the URI `/phriction/preview//` (with two `//` at the end). This is collapsed into "/phriction/preview/" by Apache if "MergeSlashes On" is configured, which is the default behavior. The route then 404s.
Instead, just use "/phriction/preview/?slug=/" so this endpoint functions properly regardless of the "MergeSlashes" configuration.
Test Plan:
- Configured Apache with "MergeSlashes On" (which is the default behavior).
- Tried to preview a content edit of the root document in Phriction, which didn't work and generated 404s for "/phriction/preview//" in the console log.
- Applied patch.
- Previwed content in Phriction (which now worked properly).
- Accessed `/a//b///c////` and similar with "MergeSlashes On" and "MergeSlashes Off", confirmed that this option controls whether PHP receives a URI with or without merged slashes in "__path__" after rewriting.
Reviewers: 0
Reviewed By: 0
Maniphest Tasks: T13662
Differential Revision: https://secure.phabricator.com/D21708
Summary:
Ref T13658. This adds a simple expression evaluator to Remarkup and supports platform name expressions. The syntax is:
```
${{{strings.platform.server.name}}}
```
Note that this won't work inside code blocks (or literal blocks, or other block-level literal elements) right now, although it could be made to selectively (the ".path" expressions might be useful in documentation codeblocks).
Test Plan: {F9391006}
Reviewers: cspeckmim
Reviewed By: cspeckmim
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21713
Summary:
See PHI498. This should be initialized to "self::ATTACHABLE" like other attachable properties, but is currently initialized to "array()".
Initialize it the normal way and try to catch all code paths which may have accessed it without actually loading and attaching it.
Also, remove UI for the very old "excuse" property, which "arc" has not written for well more than a year.
Test Plan: Grepped for affected symbols, loaded various revision pages. Somewhat tricky to be 100% sure that every pathway is caught, but it should be obvious if I missed anything once someone hits the code path.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D21710
Summary:
Fixes T13648. If a package recipient has been destroyed, this query currently fails to return an expanded recipient value.
Instead, make sure all input PHIDs get an output. For destroyed packages, the output will just be an empty list.
Test Plan:
- Added a package to a revision as a reviewer.
- Destroyed the package.
- Commented on the revision.
- Processed the publishing worker with `bin/worker execute`.
- Before: fatal after expanding the destroyed package.
- After: clean publish.
Maniphest Tasks: T13648
Differential Revision: https://secure.phabricator.com/D21707
Summary: Ref T13072. This exception is now raised by all of the message-sending code. Pretty straight find/replace.
Test Plan: Grepped for old class name, no hits.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21699
Summary: Ref T13072. Expand the role of "harbormaster.sendmessage" and allow it to send control messages to Builds and Buildables.
Test Plan: Read documentation, sent commands to Builds and Buildables, hit a bunch of error cases, will deploy to catch full-lifecycle Build Target use cases.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21698
Summary: Ref T13072. Make large Conduit doc pages a bit more navigable. This prepares for updating "harbormaster.sendmessage" to support sending messages to builds.
Test Plan: Viewed various Conduit API documentation pages, clicked links.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21696
Summary: Ref T13072. These don't do anything useful yet, but get the skeletons in.
Test Plan: Loaded documentation pages without fataling.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21695
Summary: Ref T13072. Trivially convert this into a modular transaction type.
Test Plan: Issued commands to a buildable.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21694
Summary: Ref T13072. This transaction type has no writers and is mooted by EditEngine.
Test Plan: Grepped for transaction constant.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21693
Summary: Ref T13072. Update the last few constant references to this class and remove it.
Test Plan: Grepped for "HarbormasterBuildCommand", got no hits.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21692
Summary: Ref T13072. Push nearly all Harbormaster build message logic into the new per-message transaction classes.
Test Plan:
- Issued every message to Buildables.
- Issued every message to Builds.
- Looked at a big pile of error messages, couldn't find any typos.
- Grepped for affected symbols, etc.
- Ran `bin/harbormaster restart ...`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21691
Summary:
Ref T13072. Further modularize build messages by applying each one in a separate transaction type.
This makes it easier to add new types of messages (although I have no particular plans to do this, offhand) and reduces the amount of switch-boilerplate.
This will probably also simplify validating "harbormaster.sendmessage".
Test Plan:
- Applied all commands.
- Ran migration, saw transactions render properly
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21690
Summary: Ref T13072. Update "HarbormasterBuild" to use modern modular transactions.
Test Plan:
- Aborted, restarted, paused, and resumed a build.
- Used `bin/harbormaster restart`.
- Grepped for use of old "::TYPE_COMMAND" constant, didn't find any hits.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21689
Summary: Ref T13072. No callers currently generate these transactions, and they probably never have. Remove them.
Test Plan: Grepped for "HarbormasterBuildTransaction::TYPE_CREATE" and "self::TYPE_CREATE" in the class, found no hits.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21688
Summary:
Ref T13072. Currently, Harbormaster builds react to messages by applying a transaction inline (which can race) that has no real effect.
Later, the BuildEngine picks up the mesasge and applies a real effect, but this isn't transactional.
This is backwards, and makes it more difficult to transition to ModularTransaction and EditEngine. The desired workflow is:
- sending a message //just// writes to the message table (and queues a worker to process the message);
- the BuildEngine processes the message and applies effects in a transactional way.
Force this into at least roughly the right sequence of behaviors. This paves the way for porting to ModularTransaction, which should allow further cleanup.
Test Plan: Paused, resumed, aborted, and restarted a build. Ran BuildWorkers to process the commands, saw builds update appropriately.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21687
Summary:
Ref T13072. Currently, Builds have basic states (like "passed" and "failed") and pending states where a command has been issued but not yet executed (pausing, resuming, restarting, and aborting).
These are handled in a bit of an ad-hoc way, and not everything treats them the same way. In particular, the build page can concurrently report a build as "Aborting" and "Pausing", with different icons and colors.
Make everything use the same logic so that a Build can only be in exactly zero or one pending state, and use the same icons and colors.
Also tighten up which transitions are allowed: for example, it doesn't make sense to pause an aborting build.
The tighter rules don't all produce great UX right now (like "You can't pause this build.", when it would be better as "You can't pause a build which is already aborting." or similar), but just leave that alone for now.
Test Plan: Viewed builds, applied various state changes, ran BuildWorker to effect the state changes, grepped for affected methods, tried to issue various out-of-sequence build commands.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21685
Summary:
Ref T13072. These two similar tables don't make sense to keep separate. Instead, make Build a valid receiver for BuildMessage objects.
These tables are practically the same, so this is straightforward: just copy the rows in and then drop the old table.
(This table was trivial and ephemeral anyway, so I'm not bothering to do the usual "keep it around for a couple years just in case".)
Test Plan:
- Populated BuildCommand table, ran migration, saw Builds end up in the proper transitional state (e.g., pausing, aborting, restarting) with appropriate queued messages.
- Queued new messages by clicking UI buttons.
- Ran BuildWorkers, saw them process messages and mark them as consumed.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21684
Summary:
Ref T13072. Rename various "command" properties to "message" properties, to prepare for merging "HarbormasterCommand" into "HarbormasterMessage".
This change only renames variables and methods and should not affect program behavior.
Test Plan: Grepped for affected symbols, found no unmodified hits.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13072
Differential Revision: https://secure.phabricator.com/D21683
Summary: Ref T13660. Clean up callsites to "PhutilExecPassthru->execute()" to prepare to deprecate it.
Test Plan:
- Grepped for "PhutilExecPassthru" and looked for callsites.
- Ran `GIT_SSH=.../ssh-connect git ls-remote origin` to execute the "ssh-connect" code.
- The two passthru future methods have no callers and could possibly be removed, but I'm just letting sleeping dogs lie for now.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Maniphest Tasks: T13660
Differential Revision: https://secure.phabricator.com/D21703
Summary:
Refer to discussion on D21677#275541
Refs D21681 (arcanist changes)
Phabricator has several uses of the `--debug` flag being used with Mercurial. Use of this flag causes additional output to be added which Phabricator needs, however the behavior of `--debug` is not guaranteed to be stable, and in newer versions of Mercurial there have been additional output that has caused Phabricator to choke on parsing the output. This change removes several uses of `--debug` in favor of using `--template` with the `hg log` or `hg annotate` commands in combination with the `{p1.node}` or `{p2.node}` template format.
The use of `{p1node}` format in templates was added in [[ https://www.mercurial-scm.org/wiki/WhatsNew/Archive#Mercurial_2.4_.282012-11-01.29 | Mercurial 2.4 (2012) ]]. This format was deprecated in [[ https://www.mercurial-scm.org/wiki/WhatsNew#Mercurial_4.9_.282019-02-01.29 | Mercurial 4.9 (2019) ]] in favor of using `{p1.node}` format which is unclear when this new format was added (presumably earlier than Mercurial 4.9).
The use of `--template` with `hg annotate` is only officially supported in [[ https://www.mercurial-scm.org/wiki/Release4.6 | Mercurial 4.6 (2018) ]], though does appear to work in 4.5 but is not documented.
Since the `{p1node}` format was introduced in 2.4 this bumps the required version of `hg` to 2.4 (from 1.9). Since the `annotate --template` feature wasn't added until 4.6 (which is still fairly recent), the use of it is gated on a capability test, but still preferred for use where possible to avoid extraneous output from `--debug` flag.
Test Plan:
I verified I could do the following in a mercurial repository, while having mercurial 5.8 installed:
1. Navigate and view files in Diffusion under e.g. `/source/test-repo/`.
2. While viewing a file in Diffusion verified that I could view the blame of the file and the history/annotations looked accurate for the files I was browsing.
3. From the blame sidebar, select to view a commit which loaded and displayed changes properly.
4. View the history of the repository under e.g. `/source/test-repo/history/default/`. I verified the history looked correct and the tree-like structure showing relationship of commits also looked accurate.
I setup mercurial to run version 4.4, created a new repository, added some commits, and verified all the above behavior still works properly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21679
Summary:
Ref T13658. This just scrubs some of the simple references from the codebase.
Most of what's left is in documentation which won't be relevant for a fork and/or which I need to separately revise (or more-or-less delete) at some point anyway.
I removed the "install RHEL" and "install Ubuntu" scripts outright since I don't have any reasonable way to test them and don't plan to maintain them.
Test Plan: Grepped for "phacility", "epriestley"; ran unit tests.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21678
Summary:
With newer versions of Mercurial come newer debug messages which need filtered out.
1. In the scenario of Phabricator observing a hosted Mercurial repository which exists on a server in a multi-user environment it's possible that a repository computes branch cache at a tip revision which is not present. When this happens Mercurial will include in the debug output this information. This message indicates that the cache is going to be re-computed. See https://www.mercurial-scm.org/pipermail/mercurial/2014-June/047239.html.
2. Likely in some version with added or improved support for `pager` the debug info seems to indicate when a pager is being invoked for a command. This seems to print out regularly despite piping the stdout.
3. If the repository on Phabricator ever had the `largefiles` extension enabled then some additional details about "updated patterns" will print out.
Test Plan:
I verified an observed repository's history could be browsed, specifically the history of files which previously resulted in "Undefined offset: 1".
Added a unit test to check the results of `filterMercurialDebugOutput()`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21677
Summary: Found a few typos which could be updated.
Test Plan:
I tested the Configuration page change by navigating to `/config` and verifying the page title set in the browser as well as the page title text on the page
|Before|After|
|---|---|
|{F9013208}|{F9013210}|
|{F9013300}|{F9013301}|
I verified the Conduit error message by navigating to `/auth/start/?__conduit__=1`
{F9013289}
The CircleCI error message was not verified due to the involvement of testing with CircleCI however the change is very minor and has very little risk of impacting any functionality.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21675
Summary: See T13657. An install has "watcher" packages which should not allow owners to "Force Accept" other packages.
Test Plan:
- Created package A, which I own, on "/", with "Weak" authority.
- Created package B, which I do not own, on "/src".
- Created a revision which touches "/src" and added package B as a reviewer.
- Attempted to accept the revision...
- Before patch: permitted to "Force Accept" for package B.
- After patch: not allowed to "Force Accept" for package B.
- Verified that setting package "A" back to "Strong" authority allows a force-accept for package B.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D21674
Summary: Ref PHI2071. This path is incorrect; the correct path is `local.json`.
Test Plan: Looked in my `conf/local/` directory.
Differential Revision: https://secure.phabricator.com/D21663
Summary: This makes the set of hooks easily extensible, as a first step toward integrating more 3rd party CI in phorge.
Test Plan: Send requests to `/harbormaster/hook/circleci/` and `/harbormaster/hook/buildkite/` and check they run the proper handler.
Reviewers: O1 Blessed Committers, Matthew
Reviewed By: O1 Blessed Committers, Matthew
Subscribers: Matthew, speck, tobiaswiese
Maniphest Tasks: T15018
Differential Revision: https://we.phorge.it/D25005
Summary: This makes the whole setup easier, future proof and reproducible.
Test Plan:
cd support/aphlict/server/
npm install
See that ws gets installed as expected.
Reviewers: O1 Blessed Committers, Matthew
Reviewed By: O1 Blessed Committers, Matthew
Subscribers: Matthew, Ekubischta, speck, tobiaswiese
Maniphest Tasks: T15019
Differential Revision: https://we.phorge.it/D25006
Summary: This commit also removes references to support pacts and updates links to point to the new upstream.
Test Plan: Generated Diviner documentation on a local install and verified that the changes look good.
Reviewers: O1 Blessed Committers, chris
Reviewed By: O1 Blessed Committers, chris
Subscribers: chris, speck, tobiaswiese
Maniphest Tasks: T15012
Differential Revision: https://we.phorge.it/D25007
Summary: Ref T13614. Provide "bin/repository lock" to temporarily lock repositories for manual maintenance.
Test Plan:
- Read instructions.
- Used `bin/repository lock` according to the instructions.
- Saw Storage tab in Diffusion report lock held during maintenance, released after it completes.
- Saw "maintenance" push log generated and repository version bump.
- Tried to lock some invalid repositories.
Maniphest Tasks: T13614
Differential Revision: https://secure.phabricator.com/D21671
Summary:
Ref T13614. When a script holds the write lock but modifies the repository directly (rather than by pushing), the repository version won't change when the script releases the write lock. Thus, the writes may not propagate to other nodes (it depends which node lucks out and accepts the next write).
To guarantee that writes propagate, allow these scripts to pretend they pushed the repository. These are bare-bones valid events flagged as "Maintenance".
Test Plan:
- Wrote a script to hold the write lock, wait (or pretend to do something), then release the write lock.
- Applied patches, modified script to use new APIs ("newMaintenanceEvent()").
- Ran script, saw repository verison bump and relevant push logs:
{F8814923}
Maniphest Tasks: T13614
Differential Revision: https://secure.phabricator.com/D21670
Summary:
Ref T13614. When an omnipotent user calls "synchronizeWorkingCopyBeforeWrite()", we record a WorkingCopyVersion record with a null "userPHID". The UI then renders this as "Unknown Object (????)".
Improve this behavior:
- When no PHID is available, just render nothing in the UI (this doesn't seem meaningfully different from no version existing at all).
- Allow callers to provide an acting user PHID, similar to Editor.
There's currently no way to perform this kind of write legitimately in the upstream, but T13614 is providing one.
Test Plan:
- Wrote a script that calls "synchronizeWorkingCopyBeforeWrite()" as the omnipotent user.
- Ran script, saw "Unknown Object (????)" in the UI.
- Applied UI fix, saw empty UI.
- Applied "acting as" fix, modified script to act as the Diffusion application, ran script, saw "Diffusion" attribution in UI.
{F8814806}
Maniphest Tasks: T13614
Differential Revision: https://secure.phabricator.com/D21669
Summary:
Ref T13650. Currently, viewing the API console help page for this method fatals because it constructs a generic, untyped panel.
As a step toward improving this, generate a concrete panel type instead. This isn't the best possible fix, see T13650 for discussion.
Test Plan: Viewed "dashboard.panel.edit" API page, now saw a usable page.
Maniphest Tasks: T13650
Differential Revision: https://secure.phabricator.com/D21668
Summary:
As backstory: I accidentally added the subscriber `PHID-USER-abcd` to `T1` on this install by calling `maniphest.edit`. I intended to edit `T1` on my local install.
This edit is permitted for messy technical reasons, described in T13429. It's not valid, but it's hard to prevent.
The state we reach is also possible even if the edit is rejected (i.e., someone can go manually update the database).
Regardless of how we get into this state, the state (a non-user subscriber) breaks the UI on the task page when it attempts to test if the subscriber can see the task.
To prevent this, only claim that a Handle can have capabilities if the handle is complete. If the handle is incomplete (an invalid or restricted object), it either can't be meaningfully tested for capabilities or the viewer isn't allowed to know them.
Test Plan:
Viewed `T1` on this install, saw a fatal. Applied the same edit to `T1` locally, got the same fatal. Applied patch, no more fatal. Now saw "Unknown Object (User)" in subscriber curtain.
Specifically, the fatal is:
> Attempting to test capability "view" for handle of type "USER", but this capability has not been attached.
Differential Revision: https://secure.phabricator.com/D21662
Summary: Ref T13559. Substantially correct the client logic for "Save" and "Cancel" actions to handle unusual cases.
Test Plan:
Quoting behavior:
- Quoted a comment.
- Cancelled the quoted comment without modifying anything.
- Reloaded page.
- Before changes: quoted comment still exists.
- After changes: quoted comment is deleted.
- Looked at comment count in header, saw consistent behavior (before: weird behavior).
Empty suggestion behavior:
- Created a new comment on a suggestable file.
- Clicked "Suggest Edit" to enable suggestions.
- Without making any text or suggestion changes, clicked "Save".
- Before changes: comment saves, but is empty.
- After changes: comment deletes itself without undo.
General behavior:
- Created and saved an empty comment (deletes itself).
- Created and saved a nonempty comment (saves as draft).
- Created and saved an empty comment with an edit suggestion (saves).
- Created and saved an empty comment with a suggestion to entirely delete lines -- that is, no suggestion text (saves).
- Edited a comment, saved without changes (save).
- Edited a comment, save deleting all text (saves -- note that this is intentionally without undo, since this is a lot of steps to do by accident).
- Cancel editing an unchanged comment (cancels without undo).
- Cancel editing a changed comment (cancels with undo).
- Undo'd, got text back.
- Cancel new comment with no text (deletes without undo).
- Cancel new comment with text (deletes with undo).
- Undo'd, got text back.
- Saved a quoted comment with no changes (saves -- note that this is intentionally not a "delete", since just quoting someone seems fine if you click "Save" -- maybe you want to come back to it later).
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21654
Summary:
Ref T13559. Various client decisions depend on the "initial" or "committed" states of inline comments. Previously, these were informally constructed from "mostly similar" available values, or glossed over in some cases.
On the server, save the initial state when creating a comment. Save the committed state when applying a "save" operation. Send all three states to the client.
On the client, load and track all three states explicitly.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21653
Summary:
Ref T13559. Currently, the default text for inline comment side-loads in a bizarre way. Instead, when a user creates an inline comment, load the inline context and set it as part of the initial content state.
This allows the side channel (and the code that puts the text in place at the last second on the client) to be removed.
Test Plan: Created inlines, clicked "Suggest Edit". See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21652
Summary:
Ref T13559. If you create comments A and B, then delete comments A and B, then undo the deletion of A, the UI undoes the deletion of B instead.
This is becasue the undo rows are shipped down with a static scalar metadata reference. When copied multiple times to create multiple undo rows, they reference the same data object.
Preventing this in the general case is a problem with greater scope. For now, just avoid rendering these rows with any metadata so they don't alias a single data object.
Test Plan:
- Created comments A, B.
- Deleted comments A, B.
- Clicked "Undo" on A.
- Before: Deletion of "B" undone.
- After: Deletion of "A" undone.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21650
Summary:
Ref T13559. When the user clicks the "Cancel" button, we sometimes take it to mean "delete" (when the comment is empty).
Both the client and server make a decision about this, and they may not agree, which causes the client to fall out of sync.
Make the client responsible for deciding whether it wants to interpret a click on the "Cancel" button as a "revert" or a "delete".
Test Plan: Cancelled empty and nonempty comments, etc. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21649
Summary:
Ref T13559. When you click "Save" on an inline comment and it's empty, we may actually delete the comment.
Currently, the client and server both make decisions about whether the comment should be deleted. These decisions may not agree, causing the client state to fall out of sync.
Make the client authoritative about whether it wants to handle the user clicking the "Save" button as an intent to save or an intent to delete.
Test Plan: Saved empty and nonempty inlines. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21648
Summary:
Ref T13647. The ref discovery process prunes commits that no longer exist in the repository before executing "git log <new heads> --not <old heads>" to identify newly published commits.
If we don't do this, the "git log" command will fail if any old head has been pruned from the repository.
Currently, this test for missing commits starts with a call to "git for-each-ref" to attempt to resolve symbols as tag or branch names, but:
- this is painfully slow in repositories with many refs; and
- this is incorrect (not consistent with "git" behavior) for 40-character hex strings, which Git will never resolve as symbolic names.
Instead, when a symbol is a 40-character hex string, skip "git for-each-ref" and jump directly to "git cat-file --batch-check".
Test Plan:
- Ran `bin/repository update` in a repository with 65K refs and extra debugging info.
- Before: took ~30s, three calls to `git for-each-ref`.
- After: took ~20s, two calls to `git for-each-ref`. Same resolution result on queries.
Maniphest Tasks: T13647
Differential Revision: https://secure.phabricator.com/D21658
Summary: Ref T13641. These conditions are swapped, and "activeBindings" loads more data than necessary while "bindings" doesn't load enough.
Test Plan: Called method with each attachment, got good results instead of an exception.
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21657
Summary:
Ref T13644. Ref T13638.
- Double-encode the symbol that is used as a path component, similar to Diffusion.
- Fix an outdated reference to ".path", which provided context for symbol lookup.
- Prevent command-clicking headers from looking up the path as a symbol.
Test Plan:
- Command-clicked headers, no longer got a symbol.
- Command-clicked stuff with "/", saw it double-encoded and decoded properly.
- Command-clicked normal symbols, saw "path" populate correctly.
Maniphest Tasks: T13644, T13638
Differential Revision: https://secure.phabricator.com/D21641
Summary:
Ref T9764. These stars are inconsistent, not accessible, and generally weird. They predate icons.
Update them to use icons instead.
Test Plan:
{F8545721}
{F8545722}
{F8545723}
Maniphest Tasks: T9764
Differential Revision: https://secure.phabricator.com/D21640
Summary:
Ref T9764. These "star" icons are unclear, inconsistent, and not friendly to colorblind users.
They date from a time long ago when the product didn't have icons.
Modernize them and make them more consistent with the similar statuses in Harbormaster.
Test Plan:
{F8545690}
{F8545691}
{F8545692}
Maniphest Tasks: T9764
Differential Revision: https://secure.phabricator.com/D21639
Summary: Ref T9499. When using the manual "Update Diff" workflow on the web, the "Repository" field isn't pre-filled properly. This can lead to revisions losing their repository after a manual update.
Test Plan: Did a manual update of a revision with a repository, saw it stick.
Maniphest Tasks: T9499
Differential Revision: https://secure.phabricator.com/D21638
Summary:
See PHI2032. The Conduit UI shows some "Example Custom Constraints" that are intended to be generic, but use of "statuses" is misleading since many objects have a status and most of them don't support these specific values.
Make it more clear that these are generic values.
Test Plan: Read new text.
Differential Revision: https://secure.phabricator.com/D21637
Summary: Ref T13641. Now that we can provide an "Active Devices" query, provide it and make it the default.
Test Plan: Viewed Almanac devices, got a list of active devices. Clicked "All Devices" to get all devices.
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21636
Summary:
Ref T13641. Phabricator sometimes makes intracluster requests that authenticate as a device.
Forbid these requests from authenticating as a disabled device.
Test Plan:
- Ran `bin/ssh-exec --phabricator-ssh-device ...` as an enabled/disabled device (worked; sensible error).
- Made Conduit calls as an enable/disabled device (worked; sensible error).
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21635
Summary: Ref T13065. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean upgrade, saw migrated values in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065
Differential Revision: https://secure.phabricator.com/D21634
Summary: Ref T13065. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report, saw migrated data in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065
Differential Revision: https://secure.phabricator.com/D21633
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report, saw keys migrate in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21632
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report, saw mail keys migrated to mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21631
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report and saw binding mail keys in the mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21630
Summary: Ref T13641. Ref T13065. Migrate and drop the onboard "mailKey" column for Almanac Services.
Test Plan: Ran storage migration, got clean report, saw migrated value in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21629
Summary: Ref T13641. Make "active bindings" a real query and make callers that only care about active bindings only query for active bindings.
Test Plan:
- Queried for "bindings" and "activeBindings" via Conduit.
- Disabled/enabled devices, saw binding status update in UI.
- Loaded Diffusion cluster layout.
- Grepped for `needBindings()`, `getActiveBindings()`, etc.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21628
Summary:
Ref T13641. Add a "status" property with most of the relevant support code.
This currently has no impact on use of the device or bindings by Diffusion or Drydock: they ignore the status of devices bound to services.
Test Plan:
- Created a new device.
- Changed the status of a device via web and API.
- Queried for devices via API.
- Searched for active and disabled devices.
- Viewed UI in list view, detail view.
- Used typeahead to add a new binding to an interface on a disabled device, got disabled hint in typeahead UI.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21627
Summary: Ref T13641. Provide minor modernizations before adding a "Disabled" state.
Test Plan: Browsed devices, created a new device.
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21626
Summary: Ref T13065. See similar changes attached to that task.
Test Plan: Ran migration, got a clean database state, saw mail keys populate in mail property table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065, T13641
Differential Revision: https://secure.phabricator.com/D21625
Summary:
Ref T13395. "libphutil/" was stripped for parts, but some documentation still references it. This is mostly minor corrections, but:
- Removes "Javelin at Facebook", long obsolete.
- Removes "php FPM warmup", which was always a prototype and is obsoleted by PHP preloading in recent PHP.
Test Plan: `grep` / reading
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D21624
Summary: Ref T13639. Move Diffusion to use the new API and get rid of the old API now that it no longer has any callers.
Test Plan:
Grepped for remaining callers.
{F8539335}
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21620
Summary:
Ref T13639. There's currently a hard-to-hit bug where editing the "Repository" of a revision doesn't update this index.
Instead: update the index on repository change, not just diff update.
Test Plan:
- Updated a revision, used debug view to see index update.
- Changed repository on a revision, used debug view to see index update.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21618
Summary:
Ref T13639. Make schema changes:
- Make repositoryID nullable, for revisions with no repository.
- Remove "epoch", which has no readers and no clear use.
- Change the ordering of the key, since "pathID" has more unique values and no queries ever issue without it.
Test Plan:
- Ran `bin/storage upgrade`, got a clean schema.
- Reindexed all revisions with an external script.
- Reviewed index via debug UI, saw appropriate index for non-repositoy revisions.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21617
Summary:
Ref T13639. Move operations related to updating the "AffectedPath" index to a dedicated class.
This change has no functional effect and only moves code.
Test Plan: Used an external script to rebuild every revision index; destroyed a revision with `bin/remove destroy`.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21616
Summary:
Ref T13639. The "Affected Path" table is currently hard to inspect: there's no UI, and using MySQL just gives you a bunch of IDs.
Add a simple UI and a debug-mode link to it.
Test Plan:
{F8539098}
{F8539099}
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21615
Summary:
Ref T13639. Updating the affected path table has a peculiar side effect from D19426, which is a simplification of a peculiar side effect from earlier.
Don't condition Owners behavior on path index behavior.
Test Plan: Created a revision.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21614
Summary: See T13639. This change simplifies providing a more modern approach to querying this data via "differential.revision.search".
Test Plan: Called "differential.query" with paths (got an error) and without paths (got a valid query result).
Differential Revision: https://secure.phabricator.com/D21613
Summary:
Ref T13639. In D17754, this:
> OPEN REVISIONS
> Recently updated open revisions affecting this file.
...was simplified into:
> RECENTLY OPEN REVISIONS
This is a bit misleading, since the panel doesn't contain "recently open" results. Use "Recent Open" instead, which is a bit more consistent with other product text. This is still slightly misleading, but probably close enough.
Test Plan: Read text.
Maniphest Tasks: T13639
Differential Revision: https://secure.phabricator.com/D21612
Summary:
Ref T13631. This option has a behavior other than the behavior implied by the name and documented.
Document the correct behavior, at least. This can likely be removed after T10574.
Test Plan: Read config option help in Config.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21610
Summary: Ref T13623. In D21603, I made the "partial object" this query returns a raw row, which paging keys can no longer be extracted from properly.
Test Plan: Paged notifications to page 2, no longer saw an error.
Maniphest Tasks: T13623
Differential Revision: https://secure.phabricator.com/D21609
Summary:
Fixes T13622. Figuring out what permissions we have seems difficult, so address this a bit more narrowly:
- Make the "access denied" error message a bit more helpful.
- Tailor error handling for the "CREATE TEMPORARY TABLE" statement.
Test Plan:
- Created a new user, granted them "SELECT ON *.*" but not "CREATE TEMPORARY TABLE", ran `bin/storage upgrade --force --apply phabricator:20210215.changeset.02.phid-populate.php`.
- Before: fairly opaque error.
- After: fairly useful error.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13622
Differential Revision: https://secure.phabricator.com/D21608
Summary: Ref T13636. Add routing for "/robots.txt", "favicon.ico", and "/status/" on the ShortSite and BlogSite.
Test Plan: Visted all resources (and 404 pages) on Short and Blog sites, and Platform site.
Maniphest Tasks: T13636
Differential Revision: https://secure.phabricator.com/D21607
Summary:
Fixes T12919. Fixes T13636. Prior to this change, some well-known resource paths don't route on sites like ResourceSite.
- `/robots.txt`: Make it route on ResourceSite and just deny the whole site.
- `/favicon.ico`: Make it route on ResourceSite.
- `/status/`: Make it route on ResourceSite.
- 404: Make it render a 404 on ResourceSite.
Test Plan:
- Visited all URIs on ResourceSite, got sensible responses.
- Visited all URIs on main site.
- Visited 404 while logged out, got login page.
Maniphest Tasks: T13636, T12919
Differential Revision: https://secure.phabricator.com/D21606
Summary:
Ref T13635. Currently, the JSON DocumentEngine uses "phutil_json_decode()", but this can confuse "{}" and "[]".
Be more careful about how the JSON value is decoded, to preserve the distinction.
Test Plan: {F8520479}
Maniphest Tasks: T13635
Differential Revision: https://secure.phabricator.com/D21605
Summary:
Ref T13623. The change in D21577 could lead to a case where we try to access stories the user can't see.
Move the story-loading piece to "willFilterPage()" to make our way thorugh this.
Test Plan:
- Made FeedStory return nothing to simulate invisible notifications, loaded page.
- Before: index access fatal.
- After: clean "no notifications".
- Loaded notifications normally, saw normal notifications.
Maniphest Tasks: T13623
Differential Revision: https://secure.phabricator.com/D21603
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
Summary: Ref T13552. Currently, various callers read raw properties off "CommitData" directly. Wrap these in accessors to support storage changes which persist "CommitRef" information instead.
Test Plan:
- Ran "diffusion.querycommits", saw the same data before and after.
- Looked at a commit, saw authorship information and date.
- Viewed tags in a repository, saw author information.
- Ran "rebuild-identities", saw no net effect.
- Grepped for callers to "getCommitDetail(...)".
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21448
Summary: Ref T13552. The internal caller for this now uses "internal.commit.search", which is always authority-reading. No legitimate external caller should rely on the behavior of "bypassCache"; no-op it to simplify behavior.
Test Plan: Called "diffusion.querycommits", saw the same data as before.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21447
Summary: Ref T13552. Swap the call we're using to build "CommitRef" objects here to the recently-introduced "internal.commit.search" method.
Test Plan: Used "bin/repository reparse --message ..." to reparse commits, added "var_dump()" to inspect results. Saw sensible CommitRef and CommitData objects get built.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21446
Summary:
Ref T13552. This is one of two callsites to "diffusion.querycommits". It's an old debugging workflow which I haven't used in years and which is likely obsoleted by identities and other changes.
I believe the root problem here was also ultimately user error (a user has misconfigured their local Git author email as another user).
Test Plan: Grepped for "lookup-users", got no hits.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21444
Summary:
Ref T13552. Commit parsers currently invoke a special mode of "diffusion.querycommits", which is an older frozen method.
The replacement, "diffusion.commit.search", is not really appropriate for low-level access. This mode of having a single method which operates in "cache" or "non-cache" modes also ends up in a lot of unnecessary field shuffling.
Provide "internal.commit.search" as a modern equivalent that returns a "DiffusionCommitRef"-compatible structure.
Test Plan: Executed "internal.commit.search", got sensible low-level commit results.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21443
Summary:
Ref T13552. Some Diffusion conduit calls may only be served by a node which hosts a working copy on disk, so they're proxied if received by a different node.
This capability is currently bound tightly to "DiffusionRequest", which is a bundle of context parameters used by some Diffusion calls. However, call proxying is not fundamentally a Diffusion behavior.
I want to perform proxying on a "*.search" call which does not use the "DiffusionRequest" parameter bundle. Lift proxying to the root level of Conduit.
Test Plan: Browsed diffusion in a clusterized repsository.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21442
Summary: Ref T13552. Neither "$hashes" or "$user" are used, and constructing them has no side effects.
Test Plan: Searched for these symbols.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21441
Summary:
Ref T13570. Fixes T13235. In most cases, we use modern (v4) signatures for almost all AWS API calls, and have for several years.
However, sending email via SES currently uses an older piece of external code which uses the older (v3) signature method.
AWS is retiring v3 signatures on October 1 2020, so this pathway will stop working.
Update the pathway to use `PhutilAWSFuture`, which provides v4 signatures.
T13235 discusses poor error messages from SES. Switching to Futures fixes this for free, as they have more useful error handling.
Test Plan:
- Configured an SES mailer, including the new `region` parameter.
- Used `bin/mail send-test` to send mail via SES.
- Sent invalid mail (from an unverified address); got a more useful error message.
- Grepped for removed external, no hits.
Maniphest Tasks: T13570, T13235
Differential Revision: https://secure.phabricator.com/D21461
Summary: Ref T13577. After the fix in D21453, lint identifies additional static errors in Phabricator; fix them.
Test Plan: Ran `arc lint`; these messages are essentially all very obscure.
Subscribers: hach-que, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13577
Differential Revision: https://secure.phabricator.com/D21457
Summary: Ref T13579. This property was removed in D21425, but I missed this usage site. Remove the assignment; this class no longer tracks the subprocess PID directly.
Test Plan: Searched for "->pid", no further hits.
Maniphest Tasks: T13579
Differential Revision: https://secure.phabricator.com/D21452
Summary:
Ref T13573. Using the browser "Print" feature on pages produces "Thu, Aug 4, 12:22" timestamps which require context to interpret precisely (they don't have a year and don't have a timezone).
Instead, retain these timestamps in "screen" contexts but use "YYYY-MM-DD HH:MM:SS (UTC+X)" timestamps when printing.
Test Plan: Printed Maniphest tasks and other pages in Safari and Chrome using "?__print__=1" and "Print to PDF", saw absolute timestamps after this chagne in the printed documents.
Maniphest Tasks: T13573
Differential Revision: https://secure.phabricator.com/D21451
Summary:
See PHI1809, which identified a bug in Project search where queries with a large number of slugs could paginate improperly.
This change detects problems in this category: cases where multiple rows with the same ID are passed to "loadAllFromArray()". It's likely that all cases it detects are cases where a GROUP BY is missing.
Since this might have some false positives or detect some things which aren't fundamentally problematic, I'm planning to hold it until the next release.
Test Plan:
- Reverted D21399, then created a project with multiple slugs and queried for one of them via "project.search". Hit this new exeception.
- Browsed around a bit, didn't immediately catch any collateral damage.
Differential Revision: https://secure.phabricator.com/D21400
Summary: Ref T13552. There are currently some content overflow issues on the graph view where the menu height can exceed the content height and the frame is drawn on a sub-element. Make the frame draw around all the content.
Test Plan: Viewed commit graph history view, saw more sensible UI.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21440
Summary: Ref T13552. Provide a richer handle/status list item for commit lists.
Test Plan: Viewed commits in various interfaces, saw richer information.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21431
Summary:
Ref T13552. Build the "commit list" elements so that the menu action items collapse under the element on mobile.
Also change the mobile breakpoint to 512px because my Safari window can't go any narrower than 508px. Future changes to responsive design will be more content-aware anyway.
Test Plan: Looked at commits in various interfaces, at desktop and mobile widths.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21430
Summary:
Ref T13552. The current layout doesn't work particularly well on desktops or devices.
We have some device/desktop table layout code, but it isn't generic. We also have property list layout code, but it isn't generic either.
Provide generic layout elements ("Fuel", from "Phabricator UI Layout" to "PHUIL"?) and narrowly specialize their display behavior. Then swap the ListItemView stuff to use it.
Test Plan:
Saw slightly better responsive behavior:
{F7637457}
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21418
Summary:
Ref T13552. In unifying the various Graph/List/Table commit views, some information was dropped -- particularly, audit status.
Restore most of it. The result isn't very pretty, but has most of the required information.
Test Plan: {F7637411}
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21417
Summary: Ref T13552. Some of the CSS can be removed or simplified now that essentially all lists of commits are on a single rendering pathway.
Test Plan: Grepped for affected CSS, viewed commit graph.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21416
Summary: Ref T13552. Remove yet another way to render a list of commits, and unify it with "CommitGraphView".
Test Plan:
- Viewed commit search results.
- Viewed owners package detail page.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21415
Summary:
Ref T13552.
Currently, the "Browse" page shows a snippet of unmerged changes if you're looking at a non-default branch. Remove this for consistency with the simplified main "Browse" page. This is reachable via "Compare".
Update the "Compare" page to use the new "CommitGraphView".
Test Plan:
- Looked at the "Browse" page of "stable".
- Looked at the "Compare" page for "stable vs master".
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21414
Summary: Ref T13552. When viewing a merge commit, merged changes are currently shown inline. Update this view to use the new "GraphView" rendering pipeline.
Test Plan:
- Viewed a merge commit, saw merges.
- Viewed history, profile page, etc.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21413
Summary:
Ref T13552. This older view mostly duplicates other code and has only two callsites:
- The "Commits" section of user profile pages.
- The "Ambiguous Hash" page when you visit a commit hash page which is an ambiguous prefix of two or more commit hashes.
Replace both with "DiffusionCommitGraphView".
Test Plan:
- Visited profile page, clicked "Commits".
- Visited an ambiguous hash page (`rPbd3c23`).
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21412
Summary: Ref T13552. In the new combined "table/list" graph view, tidy up the graph rendering.
Test Plan: {F7633504}
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21411
Summary:
Ref T13552. Currently, commit lists are sometimes rendered as an object list and sometimes rendered as a table. There are two separate views for table rendering.
Add a fourth view ("list, with a graph") with the eventual intent of unifying all the other views. For now, this only replaces "HistoryListView" -- and needs some more work to really be a convincing replacement.
Test Plan:
- Looked at "History" in Diffusion, saw an ugly view with all the information we want.
- Grepped for "HistoryListView", no hits.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21410
Summary:
Ref T13552. Currently, Diffusion has two effectively identical history views, the "Graph" view and the "History" view.
These arose out of product uncertainty about the importance of the graph, but I think we can just put the graph on the "object item list" view and merge these views.
Test Plan: Looked at repositories in Diffusion, no longer saw a "Graph" tab. Grepped for "graph"-related symbols.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21409
Summary:
Ref T13552. Currently, the repository landing page has a panel with recent commits. This is accessible by clicking "History" and usually below the fold, so it's not clearly useful.
Since I'm consolidating this code anyway to fix an issue with the import pipeline, just get rid of this history view.
Test Plan: Viewed a repository landing page, no longer saw a history panel.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21408
Summary: Ref T13552. This older class has no callers; tag and branch listings were replaced with an "ObjectList" view.
Test Plan: Grepped for "DiffusionTagTableView", got no hits.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21407
Summary:
Ref T13552. I'm trying to reduce the number of direct callers to commit authorship metadata. This header seems low-value enough to simply remove; this information is shown more clearly and prominently in the "Provenance" UI.
In particular, commits have multiple dates (authored, committed, pushed) but this header shows only one. It currently shows the author identity and the commit date, which isn't entirely correct. And it potentially uses an "Identity" as a timeline actor, which is conceptually fine but not entirely firm ground.
Test Plan: Viewed a commit, saw no more subheader.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21406
Summary:
Ref T13552. Give "Commit" objects a more modern, identity-aware way to render author and committer information.
This uses handles in a more modern way and gives us a single read callsite for raw author and committer names.
Test Plan:
- Grepped for callers to the old methods, found none. (There are a lot of "renderAuthor()" callers in transactions, but this call takes no parameters.)
- Viewed some commits, saw sensible lists of authors and committers.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21405
Summary:
Ref T13552. When viewing a directory in Diffusion, we make an Ajax call to get the last commit for each path.
This call currently pulls author information, since an older version of this UI showed author information.
The current UI does not show author information, so this parameter is unused. Delete the code which builds it.
Test Plan: Grepped for `'author'` and references to the "pull-lastmodified" behavior. This behavior is invoked in only one place, which never generates an author placeholder.
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21404
Summary:
See PHI1834. It's not obvious why this "+1" is present in the code, but it causes inlines to be adjusted incorrectly when a file is not modified across changes. See D21435.
Remove it, which appears to produce accurate adjustment behavior.
Test Plan:
- See D21435 for instructions to build a change, where a file with lines "A-Z" is unmodified across Diff 1 and Diff 2.
- Left inlines on lines 14, 17-19, and 16-26 (end of the file) on Diff 1.
- Before: saw inlines incorrectly adjusted to lines 15, 18, and 17 on Diff 2. Before D21435, the last inline was culled by the rendering engine.
- After: saw inlines correctly adjusted to lines 14, 17, and 16 (the same lines as the original), render properly, and highlight the correct lines when hovered.
Differential Revision: https://secure.phabricator.com/D21436
Summary:
See PHI1834. Currently, the inline adjustment engine can sometime "adjust" an inline off the end of a diff. If it does, we lay it out on an invalid display line here and never render it.
Instead, make sure that layout never puts a comment on an invalid line, so the UI is robust against questionable decisions by the adjustment engine: no adjustment should be able to accidentally discard an inline.
Test Plan:
- Created a two diff revision, where Diffs 1 and 2 have "alphabet.txt" with A-Z on one line each. The file is unchanged across diffs; some other file is changed.
- Added a comment to lines P-Z of Diff 1.
- Before: comment is adjusted out of range on Diff 2 and not shown in the UI.
- After: comment is still adjusted out of range internally, but now corrected into the display range and shown.
Differential Revision: https://secure.phabricator.com/D21435
Summary:
Ref PHI1835. Generally, Jupyter notebooks in the wild may store source and markdown content as either a single string or a list of strings.
Make the renderer read these formats more consistently. In particular, this fixes rendering of code blocks stored as a single string.
This also fixes an issue where cell labels were double-rendered in diff views.
Test Plan:
Created a notebook with a code block represented on disk as a single string, rendered a diff from it.
{F7696071}
Differential Revision: https://secure.phabricator.com/D21434
Summary:
See PHI1839. Currently, the "No newline at end of file" text is dropped in the 1-up diff view for changes that affect a file with no trailing newline.
Track it through the construction of diff primitivies more carefully.
Test Plan: {F7695760}
Differential Revision: https://secure.phabricator.com/D21433
Summary:
Although I'm not entirely thrilled about doing flow control like this (as an actual action in a build plan), I believe this build step works correctly and there's no fancy replacement mechanism on the immediate horizon, and this didn't send us down a slippery slope of Turing-complete builds encoded without real structure or context. Just kick it out of prototype.
(Other approaches which might be better in the long run are things like "this is a top-level behavior on the build plan itself" and/or "build plans are written in a DSL, not a Javascript UI".)
Test Plan: Added a new build step, saw this as an option in the "Flow Control" section.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D21432
Summary:
Reverts D21419. See PHI1814. Previously, I used "user-select: all" to group sequences of spaces for selection.
However, this has a side effect: the sequence is now selected with a single click. I didn't read the docuementation on the CSS property thoroughly and missed this in testing, since I was focused on drag-selection behavior.
This behavior is enough of a net negative that I think we're in a worse state overall; revert it.
Test Plan: Straight revert.
Differential Revision: https://secure.phabricator.com/D21429
Summary:
Ref T13556. These options are very old and effectively obsoleted by "bin/phd debug [--trace]". I haven't used either option diagnostically in many years, and they aren't mentioned in the documentation.
Remove them to simplify configuration, and because "phd.trace" doesn't work anyway and likely hasn't for a long time -- it has specific issues with TTY detection (see T13556).
Test Plan: Grepped for "phd.trace" and "phd.verbose". Ran "bin/phd debug [--trace]" and saw verbose/trace output.
Maniphest Tasks: T13556
Differential Revision: https://secure.phabricator.com/D21426
Summary:
Ref T13555. Currently, the daemon future may resolve into a failure state immediately inside "start()", and not have a valid PID when we read it.
Instead, read PIDs from the current active future in all cases, using "hasPID()" to test for the presence of a valid PID.
Since we don't query the PID immediately, we no longer need to explicitly start the future.
Also fix an issue where the same future could be added to the overseer pool more than once if it threw on "resolve()". In general:
- Before we "resolve()" a future, detach it from the DaemonHandle: we're always done with it.
- Catch exceptions on resolution and treat them the same way as subprocess resolution errors. These aren't common, but are possible in the general case.
- Have DaemonHandle add futures to the future pool directly when they're created.
Test Plan:
- Ran daemons with intentional subprocess creation failures, saw clean recovery.
- Ran daemons with intentional resolution exceptions, saw clean recovery.
Maniphest Tasks: T13555
Differential Revision: https://secure.phabricator.com/D21425
Summary:
Ref T13555. Although these callsites may not actually impact anything, it's possible for an active handle to have no PID (e.g., if the subprocess failed to start).
Handle these cases more carefully.
Test Plan: Started daemons, saw them run fine. See also next change.
Maniphest Tasks: T13555
Differential Revision: https://secure.phabricator.com/D21424
Summary:
Fixes T13554. For certain prose diff inputs and PCRE backtracking limits, this regular expression may back track too often and fail.
A characteristic input is "x x x x ...", i.e. many sequences where `(.*?)\s*\z` looks like it may be able to match but actually can not.
I think writing an expression which has all the behavior we'd like without this backtracking issue isn't trivial (at least, I don't think I know how to do it offhand); just use a strategy based on "trim()" insetad, which avoids any PCRE complexities here.
Test Plan: Locally, this passes the "x x x ..." test which the previous code failed. I'm not including that test because it won't reproduce across values of "pcre.backtrac_limit", PCRE versions, etc.
Maniphest Tasks: T13554
Differential Revision: https://secure.phabricator.com/D21422
Summary: See PHI1819. This structure may have `null` elements.
Test Plan: Will confirm user reproduction case.
Differential Revision: https://secure.phabricator.com/D21420
Summary:
Ref T2495. See PHI1814. Currently, Phabricator replaces tabs with spaces when rendering diffs.
This may or may not be the best behavior in the long term, but it gives us more control over expansion of tabs than using tab literals.
However, one downside is that you can use your mouse cursor to select "half a tab", and can't use your mouse cursor to distinguish between tabs and spaces. Although you probably shouldn't be doing this, this behavior is less accurate/correct than selecting the entire block as a single unit.
A specific correctness issue with this behavior is that the entire block is copied to the clipboard as a tab literal if you select any of it, so two different visual selection ranges can produce the same clipboard content.
This particular behavior can be improved with "user-select: all", to instruct browsers to select the entire element as a single logical element. Now, selecting part of the tab selects the whole thing, as though it were really a tab literal.
(Some future change might abandon this approach and opt to use real tab literals with "tab-size" CSS, but we lose some ability to control alignment behavior if we do that and it doesn't have any obvious advantages over this approach other than cursor selection behavior.)
Test Plan:
- In Safari and Firefox, dragged text to select a whitespace-expanded tab literal. Saw browsers select the whole sequence as though it were a single tab.
- In Chorme, this also mostly works, but there's some glitchiness and flickering. I think this is still a net improvement, it's just not as smooth as Safari and Firefox.
Maniphest Tasks: T2495
Differential Revision: https://secure.phabricator.com/D21419
Summary:
See PHI1810. In situations where:
- An author submits an urgent change for review.
- The author pings reviewers to ask them to look at it.
...the reviewers may not be able to move the review forward if the review is currently a "Draft". They can only "Commandeer" or ask the author to "Request Review" as ways forward.
Although I'm hesitant to support review actions (particularly, "Accept") on draft revisions, I think there's no harm in allowing reviewers to skip tests and promote the revision out of draft as an explicit action.
Additionally, lightly specialize some of the transaction strings to distinguish between "request review from draft" and other state transitions.
Test Plan:
- As an author, used "Request Review" to promote a draft and to return a change to reviewers for consideration. These behaviors are unchanged, except "promote a draft" has different timeline text.
- As a non-author, used "Begin Review" to promote a draft.
Differential Revision: https://secure.phabricator.com/D21403
Summary:
Currently, adding subscribers to a draft revision raises a warning that they won't get an email/notification.
This warning has some false positives:
- it triggers on any subscriber change, including removing subscribers; and
- it triggers if you're only adding yourself as a subscriber.
Narrow the scope of the warning so it is raised only if you're adding a subscriber other than yourself.
Test Plan:
- Added a non-self subscriber, got the warning as before.
- Added self as a subscriber, no warning (previously: warning).
- Removed a subscriber, no warning (previously: warning).
Differential Revision: https://secure.phabricator.com/D21402
Summary:
See PHI1810. Build toward support for "Request Review" by non-authors on drafts, to forcefully pull a revision out of draft.
Currently, some action strings can't vary based on revision state or the current viewer, so this "pull out of draft" action would have to either: say "Request Review"; or be a totally separate action.
Neither seem great, so allow the labels and messages to vary based on the viewer and revision state.
Test Plan: Grepped for affected symbols, see followup changes.
Differential Revision: https://secure.phabricator.com/D21401
Summary:
Modern Mercurial may emit some more patterns under "--debug".
This whole list is gross and can likely now be eliminated by increasing the minimum required Mercurial version (as `arc` has), but just paper over it for now.
Test Plan:
Locally, saw some views return to functional behavior that weren't previously working on a modern version of Mercurial.
The reproduction case is likely something in the vein of "repository is not writable by webserver, look at history view".
Differential Revision: https://secure.phabricator.com/D21398
Summary:
See PHI1809. This query may join the "slug" table, but each project may have multiple slugs, and the query does not "GROUP BY" when this join occurs.
This may lead to partial result sets and unusual paging behavior.
This could likely be caught categorically in `loadAllFromArray()`; I'll adjust this in a followup.
Test Plan:
A minimal reproduction case is something like:
- Give project P slugs: a, b, c.
- Give project Q slugs: d.
- Query for slugs: a, b, c, d; with limit 2.
- Order the query so P returns first.
- Expect: P and Q.
- Actual: P generates 3 raw rows and the final result is just P with no pagination cursor.
Differential Revision: https://secure.phabricator.com/D21399
Summary:
A handful of Phacility production shards have run into memory pressure issues recently. Although there's no smoking gun, and at least two other plausible contributors, one possible concern is that the Fact daemon was written before hibernation and can not currently hibernate. Even if there's no memory leak, this creates unnecessary memory pressure by holding the processes in memory.
Allow the Fact daemon to hibernate, like other daemons do.
Test Plan: Ran "bin/phd debug fact", saw the Fact daemon hibernate.
Subscribers: yelirekim
Differential Revision: https://secure.phabricator.com/D21389
Summary: Ref T13546. Companion change to D21372. Move URI normalization code to Arcanist to we can more-often resolve remote URIs correctly.
Test Plan: Grepped for affected symbols.
Maniphest Tasks: T13546
Differential Revision: https://secure.phabricator.com/D21373
Summary:
See PHI1794, which describes a connection exhaustion issue with a large number of webhook tasks in queue.
The "GlobalLock" mechanism manages a separate connection pool from the main pool, and webhook workers immediately try to grab a webhook lock with a 0-second wait when they start. So far, this is fine.
Prior to this change, good connections which fail to acqiure a lock are discarded. This can lead to connection exhaustion as the worker rapidly cycles through lock attempts: the connections will remain open for at least 60 seconds (since D16389) in an effort to avoid outbound port exhaustion, but they're effectively orphaned because they aren't part of the main pool and aren't part of the lock pool. We're basically leaking a connection every time we fail to lock.
Failing to lock doesn't mean we need to discard the connection: it's a completely suitable connection for reuse. Instead of dropping it on the floor, put it into the lock pool.
Test Plan:
- Used "bin/webhook call ... --count 10000 --background" to queue a large number of webhook calls against a slow ("sleep(15);") webhook.
- Used "bin/phd launch 32 taskmaster" to start taskmasters.
- Observed MySQL connection behavior:
- Before change: 2048 configured connections immediately exhausted.
- After change: connections stable at ~160ish.
- Ran queue for a while, saw expected single-threaded calls to webhook.
Differential Revision: https://secure.phabricator.com/D21369
Summary:
See PHI1794, which reports an issue where a large number of queued webhook calls led to connection exhaustion. To make this easier to reproduce and test, add "--count" and "--background" flags to "bin/webhook call".
This primarily supports "bin/webook call ... --background --count 10000" to quickly fill the queue with a bunch of calls.
Test Plan: Ran `bin/webhook call` in foreground and background modes, with and without counts. Saw appropriate console and queue behavior.
Differential Revision: https://secure.phabricator.com/D21368
Summary:
The "Export Data" workflow incorrectly uses the "Policy Favorites" setting to choose a default export format. This is just a copy/paste error; the correct setting exists and is unused.
If the setting value is an array (as the "Policy Favorites" value often is), we try to use it as an array index. This generates a runtime exception after D21044.
```
[2020-06-16 06:32:12] EXCEPTION: (RuntimeException) Illegal offset type in isset or empty at [<arcanist>/src/error/PhutilErrorHandler.php:263]
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/search/controller/PhabricatorApplicationSearchController.php:460]
```
- Use the correct setting.
- Make sure the value we read is a string.
Test Plan:
- Used "Export Data" with a nonempty, array-valued "Policy Favorites" setting.
- Before: runtime exception.
- After: clean export.
- Used "Export Data" again, saw my selection from the first time persisted.
Differential Revision: https://secure.phabricator.com/D21361
Summary: Ref T13546. This makes some "arc" tasks a little easier, and will make them more correct if "arc" ever switches to using SSH.
Test Plan: Ran "harbormaster.buildable.search" from the web UI, saw URIs in the result set.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13546
Differential Revision: https://secure.phabricator.com/D21346
Summary:
See <https://discourse.phabricator-community.org/t/bad-regex-in-prose-diff-logic/3969>.
The prose splitting rules normally guarantee that newlines appear only at the beginning or end of blocks. However, if a prose sentence ends with text like "...x\n.", we can end up with a newline inside a "sentence".
If we do, the regular expression that breaks it into pieces will fail.
Arguably, this is an error in how sentences are split apart (we might prefer to split this into two sentences, "x\n" and ".", rather than a single "x\n." sentence) but in the general case it's not unreasonable for blocks to contain newlines, so a simple fix is to make the pattern more robust.
Test Plan: Added a failing test which includes this behavior, made it pass.
Differential Revision: https://secure.phabricator.com/D21295
Summary:
Currently, Phortune attempts to prevent users from removing themselves as account managers. It does this by checking that the new list includes them.
Usually this is sufficient, because you can't normally edit an account unless you're already a manager. However, we get the wrong result (incorrect rejection of the edit) if the actor is omnipotent and the acting user was not already a member.
It's okay to edit an account into a state which doesn't include you if you have permission to edit the account and aren't already a manager.
Specifically, this supports more formal tooling around staff modifications to billing accounts, where the actor has staff-omnipotence and the acting user is a staff member and only used for purposes of leaving a useful audit trail.
Test Plan: Elsewhere, ran staff tooling to modify accounts and was able to act as "alice" to add "bailey", even though "alice" was not herself a manager.
Differential Revision: https://secure.phabricator.com/D21288
Summary:
Ref T13513. An inline is not considered empty if it has a suggestion, but some of the shared transaction code doesn't test for this properly.
Update the shared transaction code to be aware that application comments may have more complex emptiness rules.
Test Plan:
- Posted an inline with only an edit suggestion, comment went through.
- Tried to post a normal empty comment, got an appropriate warning.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21287
Summary:
Ref T13541. The passthru future does not have time limit behavior, so if we reach this code we currently fail.
Phabricator never reaches this code normally, but this code is reachable during debugging if you try to foreground a slow fetch to inspect it.
Passthru commands generally only make sense to run interactively, and the caller or control script can enforce their own timeouts (usually by pressing "^C" with their fingers).
Test Plan: Used a debugging script to run ref-by-ref fetches in the foreground.
Maniphest Tasks: T13541
Differential Revision: https://secure.phabricator.com/D21284
Summary:
See PHI1752.
- Early exit of document layout can cause us to fail to populate available rows.
- Some Jupyter documents have "markdown" cells with plain strings, apparently.
Test Plan: Successfully rendered example diff from PHI1752.
Differential Revision: https://secure.phabricator.com/D21285
Summary:
Ref PHI1749. Instead of opening files to the last unchanged line on either side of the change, open files to the "simple" line number of the selected block.
For inlines, this is the inline line number.
For blocks, this is the first new-file line number, or the first old-file line number if no new-file line number exists in the block.
This may not always be what the user is hoping for (we can't know what the state of their working copy is) but should produce more obvious behavior.
Test Plan:
- In Diffusion, used "Open in Editor" with and without line selections. Saw same behavior as before.
- Used "n" and "r" to leave an inline with the keyboard, saw same behavior as before.
- Used "\" and "Open in Editor" menu item to open a file with:
- Nothing selected or changeset selected (line: 1).
- An inline selected (line: inline line).
- A block selected (line: first line in block, per above).
Differential Revision: https://secure.phabricator.com/D21282
Summary: Ref T13276. Ref T13513. All readers and writers were removed more than a year ago; clean up the last remnants of this table.
Test Plan: Grepped for table references, found none.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13513, T13276
Differential Revision: https://secure.phabricator.com/D21281
Summary:
Ref T13513. Syntax highlighting is potentially expensive, and the changeset rendering pipeline can cache it. However, the cache is currently keyed ONLY by Differential changeset ID.
Destroy the existing cache and rebuild it with a more standard cache key so it can be used in a more ad-hoc way by inline suggestion snippets.
Test Plan: Used Darkconsole, saw cache hits and no more inline syntax highlighting for changesets with many inlines.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21280
Summary: Ref T13513. Inline comment context information is somewhat expensive to construct and can be cached. Add a readthrough cache on top of it.
Test Plan: Loaded a source code changeset with many inline comments, used Darkconsole to inspect query activity. Saw caches get populated. Updated cache key, saw caches regenerate. Browsed Diffusion, nothing looked broken.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21279
Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before.
Test Plan:
- Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior.
- Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21278
Summary: Ref T13513. When rendering an inline suggestion for display, use highlighting and diffing.
Test Plan: {F7495053}
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21277
Summary:
Ref T13513. This still has quite a few rough edges and some significant performance isssues, but appears to mostly work.
Allow reviewers to "Suggest Edit" on an inline comment and provide replacement text for the highlighted source.
Test Plan: Created, edited, reloaded, and submitted inline comments in various states with and without suggestion text.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21276
Summary: Ref T13513. Introduce a formal server-side content state object so the whole state can be saved and restored to the drafts table, read from the request, etc.
Test Plan: Created and edited inlines. Reloaded drafts with edits. Submitted normal and editing comments. Grepped for affected symbols.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21275
Summary:
Ref T13513. If your 10 most recently authored inlines have all been deleted, these queries can fail by overheating. This is silly and probably rarely happens outside of development.
For now, just let them overheat. This may create a false negative (incorrect "no draft" signal when the real condition is "drafts, but 10 most recent comments were deleted"). This could be sorted out later with a query mode like "executeAny()", perhaps.
Test Plan:
- Created and deleted 10 inlines.
- Submitted comments.
- Before: overheating fatal during draft flag generation.
- After: clean submission.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21274
Summary:
Ref T13513. Currently, all the inline code passes around strings to describe content. I plan to add background music, animation effects, etc., soon. To prepare for this change, make content a state object.
This does not change any user-visible behavior, it just prepares for content to become more complicated than a single string.
Test Plan: Created, edited, submitted, cancelled, etc., comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21273
Summary: See PHI1745. Under PHP7, errors raised as Throwable miss this "generic exception" logic and don't increment their failure count. Instead, treat any "Throwable" we don't recognize like any "Exception" we don't recognize.
Test Plan:
- Under PHP7, caused a worker task to raise a Throwable (e.g., call to undefined method, see D21270).
- Ran `bin/worker execute --id ...`.
- Before: worker failed, but did not increment failure count.
- After: worker fails and increments failure count as it would for other types of unknown error.
Differential Revision: https://secure.phabricator.com/D21271
Summary: See PHI1745. This callsite for "ChangesetParser" was not properly updated for recent changes.
Test Plan:
- Set `metamta.differential.inline-patches` to 100.
- Created a new revision with a small (<100 line) diff, with at least one reviewer.
- Ran `bin/phd debug` and observed outbound mail queue with `bin/mail list-outbound`.
- Before: fatal when trying to generate the inline changes for mail.
- After: clean mail generation.
Differential Revision: https://secure.phabricator.com/D21270
Summary: See PHI1743. If a build has no initiator PHID, the rendering pathway incorrectly tries to access a handle for it anyway.
Test Plan:
- Set a build to have no initiator PHID.
- Viewed the build plan for the build.
- Before: fatal when trying to access the `null` handle.
- After: clean build plan rendering.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D21269
Summary:
Fixes T13539. See that task for discussion and a reproduction case.
This algorithm currently counts "\ No newline at end of file" lines as though they were normal source lines. This can cause offset issues in the rare case that a diff contains two of these lines (for each side of the file) and has changes between them (because the last line of the file was modified between the diffs).
Instead, don't count "\" as a display line.
Test Plan:
- See T13539 and PHI1740.
- Before: got fatals on the "wild" diff and the synthetic simplified version.
- After: clean intradiff rendering in both cases.
Maniphest Tasks: T13539
Differential Revision: https://secure.phabricator.com/D21267
Summary:
Ref T13538. See PHI1739. Synthetic Git commits with no author and/or no commit message currently extract `null` and then fail to parse.
Ideally, we would carefully distinguish between `null` and empty string. In practice, that requires significant schema changes (these columns are non-nullable and have indexing requirements) and these cases are degenerate. These commits are challenging to build and can not normally be constructed with `git commit`.
At least for now, merge the `null` cases into the empty string cases so we can survive import.
Test Plan:
- Constructed a commit with no author and no commit message using the approach described in T13538; pushed and parsed it.
- Before: fatals during identity selection and storing the commit message (both roughly NULL inserts into non-null columns).
- After: clean import.
This produces a less-than-ideal UI in Diffusion, but it doesn't break anything:
{F7492094}
Maniphest Tasks: T13538
Differential Revision: https://secure.phabricator.com/D21266
Summary:
Fixes T13536. See that task for discussion.
Older versions of MySQL (roughly, prior to 8.0.19) emit "int(10)" types. Newer versions emit "int" types. Accept these as equivalent.
Test Plan: Ran `bin/storage upgrade --force` against MySQL 8.0.11 and 8.0.20. Got clean adjustment lists on both versions.
Maniphest Tasks: T13536
Differential Revision: https://secure.phabricator.com/D21265
Summary: On the "New User" web workflow, if you use an invalid email address, you get a failure with an empty message.
Test Plan:
- Before: Tried to create a new user with address "asdf". Got no specific guidance.
- After: Got specific guidance about email address formatting and length.
Differential Revision: https://secure.phabricator.com/D21264
Summary:
Ref T13529. Now that instances can be renamed, an instance may have multiple valid SSH usernames and the preferred SSH username may not be the intenal instance name.
`PhacilitySiteSource` should already always set `diffusion.ssh-username` correctly, to the current preferred SSH username (which may be "new-name" after a rename from "old-name"), so we should never be able to reach this code without an accurate `diffusion.ssh-username` value available.
The code to resolve names into instances also already works for both "ssh old-name@..." and "ssh new-name@...".
So I believe this code has no beneficial effects and only causes harm: it may force us to return "old-name" when falling through would correctly return "new-name".
Test Plan:
- Previously: renamed an instance, then SSH'd to it using both the old and new names. Both work.
- Previously: verified that `diffusion.ssh-username` is set correctly after a rename.
- Verified that Diffusion "Clone" UI now shows "new-name" after an instance rename.
- The real question here is: does this break something I'm not thinking of? And the change probably has to go to production to answer that.
Maniphest Tasks: T13529
Differential Revision: https://secure.phabricator.com/D21259
Summary:
Ref T13513. The way I'm highlighting lines won't work for Jupyter notebooks or other complex content blocks, and I don't see an obvious way to make it work that's reasonably robust.
However, we can just ignore the range behavior for complex content and treat the entire block as selected. This isn't quite as fancy as the source behavior, but pretty good.
Also, adjust unified diff behavior to work correctly with highlighting and range selection.
Test Plan:
- Used range selection in a Jupyter notebook, got reasonable behavior (range is treated as "entire block").
- Used range selection in a unified diff, got equivalent behavior to 2-up diffs.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21257
Summary:
Ref T13513. Give selected inlines a selection state and visual cues which are similar to the changeset selection state.
Also fix a couple of minor issues with select interactions and offset comments.
Test Plan: Selected inlines, saw obvious visual cues.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21256
Summary:
Ref T13513. When a user selects a text range and uses "New Inline Comment" to create a comment directly from a range, store the offset information alongside the comment.
When hovering the comment, highlight the original range.
Test Plan: {F7480926, size=full}
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21250
Summary: Ref T13513. Support direct text selection for inlines. This is currently just an alternate way to get to the same place as using line numbers, but can preserve offset/range information in the future.
Test Plan:
- Selected some text, hit "c", clicked "New Inline Comment", got sensible comments on both sides of a diff in Safari, Chrome, and (with limitations) Firefox.
- Caveats: no unified support, doesn't work across lines in Firefox.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21248
Summary: Ref T11401. Ref T13513. This paves the way for more comment actions, particularly an edit-after-submit action.
Test Plan: Took all actions from menus, via mouse and via keyboard (where applicable).
Maniphest Tasks: T13513, T11401
Differential Revision: https://secure.phabricator.com/D21244
Summary:
Ref T13454. See <https://discourse.phabricator-community.org/t/newly-created-ssh-private-keys-with-passphrase-not-working-anymore/3883>.
After changes to distinguish between invalid and passphrase-protected keys, SSH private key management code incorrectly uses "-y ..." ("print public key") when it means "-p ..." ("modify input file, removing passphrase"). This results in the command having no effect, and Passphrase stores the raw input credential, not the stripped version.
We can't recover the keys because we don't store the passphrase, so no migration here is really possible. (We could add more code to detect this case, but it's presumably rare.)
Also, correct the behavior of the "Show Public Key" action: this is available for users who can see the credential and does not require edit permission.
Test Plan:
- Created a new credential with a passphrase, then showed the public key.
Maniphest Tasks: T13006, T13454
Differential Revision: https://secure.phabricator.com/D21245
Summary:
Ref T13513. Currently, viewing a Jupyter document, hidden context just gets a plain "* * *" facade with no way to expand it.
Support click-to-expand, like source changes.
Test Plan:
- Clicked to expand various Jupyter diffs.
- Clicked to expand normal source changes.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21243
Summary:
Ref T13513. If you leave an inline on line 20 of a Jupyter document, we currently render context around *raw* line 20, which is inevitably some unrelated piece of JSON.
Instead, drop this context. (Ideal behavior would be to render context around Jupyter block 20, but that's a whole lot of work.)
Test Plan:
- On Jupyter changes and normal source changes, made and submitted inline comments, then viewed text and HTML mail.
- Saw no context on Jupyter comments (instead of bad context), and unchanged behavior (useful context) on normal source changes.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21242
Summary:
Ref T13513. Currently, "View as Document Type..." lists every available engine.
This is hard to get completely right because we can't always rebuild the document ref accurately in the endpoint, but try harder to fake something reasonable.
Test Plan: Used "View as Document Type..." on Jupyter notebooks, was given "Jupyter" and "Source" as options.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21241
Summary:
Ref T13513. As part of inline metadata, save the document engine the change is being rendered with.
This will allow other parts of the UI to detect that an inline was created on a Jupyter notebook but is being rendered on raw source, or whatever else.
The immediate goal is to fix nonsensical inline snippet rendering in email on Jupyter notebooks.
Test Plan:
- Created inlines and replies on normal soure code, saw no document engine annotated in the database.
- Created inlines and replies on a Jupyter notebook rendered in Jupyter mode, saw "jupyter" annotations in the database.
- Swapped document engines between Jupyter and Source, etc.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21240
Summary:
Ref T13513. If an intradiff has at least one unchanged file ("hasSameEffectAs()") or more than 100 files ("Large Change"), we hit this block and don't upcast storage inlines to runtime inlines. I missed this in testing.
Add the conversion step.
Test Plan: Viewed an intradiff with at least one unchanged file and at least one inline comment, saw correct rendering instead of fatal.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21239
Summary:
Ref T13513. Currently, clicking "View" from the inline comment preview (below the "add comment" area at the bottom of the page) only works if the inline isn't being edited.
Update this behavior so it works on inlines in either "Viewing" or "Editing" states.
Test Plan:
- Clicked "View" on a normal inline, got jumped/selected.
- Clicked "View" on an editing inline, got jumped/selected.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21237
Summary:
Ref T13513. Currently:
- If you click the "Show Changeset" button, your state change doesn't actually get saved on the server.
- It's hard to select a changeset path name for copy/paste because the "highlight the header" code tends to eat the event.
Instead: persist the former event; make the actual path text not be part of the highlight hitbox.
Test Plan:
- Clicked "Show Changeset", reloaded, saw changeset visibility persisted.
- Selected changeset path text without issues.
- Clicked non-text header area to select/deselect changesets.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21236
Summary:
Ref T13513. Currently, if you:
- click a line to create an inline;
- type some text;
- wait a moment; and
- close the page.
...you don't get an "Unsubmitted Draft" marker in the revision list.
Lift all the draft behavior to "InlineController" and make saving a draft dirty the overall container draft state.
Test Plan:
- Took the steps described above, got a draft state marker.
- Created, edited, submitted, etc., inlines in Diffusion and Differential.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21235
Summary: Ref T13513. All queries now go through a reasonably minimal set of pathways and should have consistent behavior.
Test Plan:
- Loaded a revision with inlines.
- Created a new empty inline, reloaded page, saw it vanish.
- Created a new empty inline, typed draft text, did not save, reloaded page, saw draft present.
- Created a new empty inline, typed draft text. Submitted feedback, got prompt, answered "Y", saw draft text submit.
- Created a new empty inline, typed draft text, scrolled down to bottom of page, typed non-draft text, saw preview include draft text.
- Marked and submitted "Done".
- Used hide/show on inlines, verified state persisted.
- Did much of the same stuff in Diffusion, where it all works the same way (except: there's no prompt when submitting draft is-editing inlines).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21234
Summary: Ref T13513. Replaces "DifferentialInlineCommentQuery" with the similar but more modern "DifferentialDiffInlineCommentQuery".
Test Plan: Viewed comments in timeline, changesets. Created, edited, and submitted comments. Hid and un-hid comments, reloading (saw state preserved).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21233
Summary: Ref T13513. Continue removing usage sites for the obsolete "DifferentialInlineCommentQuery".
Test Plan: Viewed the inline list in Differential, saw sensible inlines.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21232
Summary: Ref T13513. Move querying to "DiffInlineCommentQuery" classes and lift them into the base Controller.
Test Plan: In Differential and Diffusion, created, edited, and submitted inline comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21231
Summary: Ref T13513. Another step closer to the light.
Test Plan: Created, edited, deleted, replied to, and submitted inline comments in Diffusion.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21230
Summary: Ref T13513. Continue marching toward coherent query pathways for all access to inline comments.
Test Plan:
- Viewed a commit and a path within that commit, as a user with unpublished inlines and a different user.
- Saw appropriate inlines in all cases (published inlines, plus undeleted unpublished inlines authored by the current viewer).
- Grepped for "loadDraftAndPublishedComments()".
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21228
Summary:
Ref T13513. Improve consistency and robustness of the "InlineComment" queries.
The only real change here is that these queries now implicitly add a clause for selecting inlines ("pathID IS NULL" or "changesetID IS NULL").
Test Plan: Browed, created, edited, and submitted inlines.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21227
Summary:
Ref T13513. Currently, inline storage objects ("TransactionComment") can't directly generate a runtime object ("InlineComment").
Allow this transformation to be performed in a genric way so clunky code which does it per-object-type can be removed, lifted, or simplified.
Simplify an especially gross callsite in preview code.
Test Plan: Previewed inline comments.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21226
Summary: Ref T13513. See that task for some discussion. This prepares to lift "loadUnsubmittedInlineComments(...)" into shared code.
Test Plan: Grepped for callers, found none in the upstream. This is a backward compatibilty break. See T13513.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21225
Summary: Ref T13513. This controller was obsoleted by EditEngine and appears unreachable without explicitly typing the URL.
Test Plan:
- Grepped for the route, didn't find any hits.
- Deleted the controller, successfully previewed comments in Diffusion.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21224
Summary:
Ref T13519. This is a little fuzzy, but I think the workflow here is:
- View an intradiff, generating an ephemeral comparison changeset with no changeset ID. This produces a state key of "*".
- Apply "hidden" state changes to the changeset.
- View some other intradiff and/or diff view.
- The code attempts to use "*" as a changset ID?
I'm not entirely sure this is accurate; this was observed in production and I couldn't get a clean reproduction case locally.
Optimistically, try making changeset IDs explicit rather than relying on state keys to be "usually changeset-ID-like".
Test Plan: Used "hidden" locally across multiple intradiffs, but I wasn't cleanly able to reproduce the initial issue.
Maniphest Tasks: T13519
Differential Revision: https://secure.phabricator.com/D21223
Summary: Ref T13523. If a file hasn't been touched in the newer changeset, we can currently hit an error in the interdiff.
Test Plan:
- Touched "moo.txt" in Diff 1.
- Reverted the changes to "moo.txt" in Diff 2.
- Diffed 2 vs 1.
- Before patch: fatal (call to getFilename() on null).
- After patch: clean interdiff.
Maniphest Tasks: T13523
Differential Revision: https://secure.phabricator.com/D21220
Summary:
Ref T13513. Overloading "original text" to get "edit-on-load" comments into the right state has some undesirable side effects.
Instead, provide the text when the editor opens. This fixes a cancel interaction.
Test Plan:
- Create an inline, type text, don't save.
- Reload page.
- Cancel.
- Before: cancelled into empty state.
- After: cancelled into deleted+undo state.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21219
Summary: Ref T13513. When users choose to publish inlines, we want to publish the visible text, not the last "checkpointed" state.
Test Plan:
- Created an inline ("AAA").
- Edited it into "BBB", did not save.
- Submitted.
- Confirmed that I want to publish the unsaved inline.
- Saw "BBB" publish.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21218
Summary:
Ref T13513. As users type text into inline comments, save the comment state as a draft on the server.
This has some rough edges, particularly around previews, but mostly works. See T13513 for notes.
Test Plan: Started an inline, typed some text, waited a second, reloaded the page, saw an editing inline with the saved text.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21216
Summary: Ref T13513. When computing whether a revision has draft comments or not, ignore empty inlines.
Test Plan: Added empty inlines to a revision, no longer saw a yellow "draft" bubble in the list UI.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21215
Summary: Ref T13513. When you load a changeset, discard all empty inlines. This is likely a more desirable behavior than keeping empty editors around, even though the rest of the pipeline generally handles them fairly well now.
Test Plan:
- Started an inline, didn't type any text or save, reloaded page.
- Before: page restores empty editor in the same place.
- After: we just discard this likely-pointless empty inline.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21214
Summary: Ref T13513. Refine some inline behaviors, see test plan.
Test Plan:
- Edit a comment ("A"), type text ("AB"), cancel, edit.
- Old behavior: edit and undo states (wrong, and undo does not function).
- New behavior: edit state only.
- Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
- Old behavior: "AB" (wrong: you never submitted this text).
- New behavior: "A".
- Create a comment, type text, cancel.
- Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
- New behavior: no counter.
- Cancel editing an empty comment with no text.
- Old behavior: Something buggy -- undo, I think?
- New behavior: it just vanishes (correct behavior).
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21212
Summary:
Ref T13513. Currently, if you start an inline and then submit overall comments, we publish an empty inline. This is literally faithful to what you did, but almost certainly not the intent.
Instead, simply ignore empty inlines at publishing time (and ignore "done" state changes for those comments).
We could delete them outright, but if we do, they'll break if you have another window open with the empty inline (since the stored comment won't exist anymore). At least for now, leave them in place.
Test Plan: Created empty inlines, submitted comments, no longer saw them publish.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21211
Summary: Ref T13513. This slightly expands the existing-but-hacky "warning" workflow to cover both "mentions on draft" and "submitting inlines being edited".
Test Plan:
- Submitted changes to a revision with mentions on a draft, inlines being edited, both, and neither.
- Got sensible warnings in the cases where warnings were appropriate.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21191
Summary:
Ref T13513. If you submit top-level comments while an inline comment editor is open, kick the comment out of the editing state.
(An improvement to this behavior would be to warn the user that we're going to do this first, but this is currently less straightforward.)
Test Plan:
- Clicked a line number to create an inline.
- Type text, save, click edit.
- (Optional: reload page.)
- Save changes overall using the form at the bottom of the page.
- Outcome: published inline is no longer in an "editing" state.
Weirdness:
- If you click a line number (and, optionally, type text), then submit without using "Save", the server-side version of the inline has no content.
- This gives you a no-effect warning. Instead, these inlines should probably just be marked as deleted somewhere in the pipeline.
- This saves the last "Saved" copy of the inline. That's (probably?) desired, but somewhat destructive without a warning.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21188
Summary:
Ref T13513. Now that the "currently being edited" state of inlines is saved on the server side, clear the flag when the user clicks "Cancel" to leave the "editing" state on the client.
This also serves to delete empty comments.
Test Plan:
- Clicked a line number to create a new comment. Then:
- Clicked "Cancel". Reloaded page, saw no more comment.
- Typed text, saved. Reloaded page, saw non-editing draft. Clicked "Edit", reloaded page, saw editing draft. Clicked "Cancel", reloaded page, saw non-editing draft.
- Typed text, saved. Clicked "Edit", deleted text, saved. Reloaded page, saw no more comment.
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21187
Summary:
Ref T13513. This is mostly an infrastructure cleanup change.
In a perfect world, this would be a series of several changes, but they're tightly interconnected and don't have an obvious clean, nontrivial partition (or, at least, I don't see one). Followup changes will exercise this code repeatedly and all of these individual mutations are "obviously good", so I'm not too worried about the breadth of this change.
---
Inline comments are stored as transaction comments in the `PhabricatorAuditTransactionComment` and `DifferentialTransactionComment` classes.
On top of these two storage classes sit `PhabricatorAuditInlineComment` and `DifferentialInlineComment`. Historically, these were an indirection layer over significantly different storage classes, but nowadays both storage classes look pretty similar and most of the logic is actually the same. Prior to this change, these two classes were about 80% copy/pastes of one another.
Part of the reason they're so copy/pastey is that they implement a parent `Interface`. They are the only classes which implement this interface, and the interface does not provide any correctness guarantees (the storage objects are not actually constrained by it).
To simplify this:
- Make `PhabricatorInlineCommentInterface` an abstract base class instead.
- Lift as much code out of the `Audit` and `Differential` subclasses as possible.
- Delete methods which no longer have callers, or have only trivial callers.
---
Inline comments have two `View` rendering classes, `DetailView` and `EditView`. They share very little code.
Partly, this is because `EditView` does not take an `$inline` object. Historically, it needed to be able to operate on inlines that did not have an ID yet, and even further back in history this was probably just an outgrowth of a simple `<form />`.
These classes can be significantly simplified by passing an `$inline` to the `EditView`, instead of individually setting all the properties on the `View` itself. This allows the `DetailView` and `EditView` classes to share a lot of code.
The `EditView` can not fully render its content. Move the content rendering code into the view.
---
Prior to this change, some operations need to work on inlines that don't have an inline ID yet (we assign an ID the first time you "Save" a comment). Since "editing" comments will now be saved, we can instead create a row immediately.
This means that all the inline code can always rely on having a valid ID to work with, even if that ID corresponds to an empty, draft, "isEditing" comment. This simplifies more code in `EditView` and allows the "create" and "reply" code to be merged in `PhabricatorInlineCommentController`.
---
Client-side inline events are currently handled through a mixture of `ChangesetList` listeners (good) and ad-hoc row-level listeners (less good). In particular, the "save", "cancel", and "undo" events are row-level. All other events are list-level.
Move all events to list-level. This is supported by all inlines now having an ID at all stages of their lifecycle.
This allows some of the client behavior to be simplified. It currently depends on binding complex ad-hoc dictionaries into event handlers in `_drawRows()`, but it seems like almost all of this code can be removed. In fact, no more than one row ever seems to be drawn, so this code can probably be simplified further.
---
Finally, save an "isEditing" state. When we rebuild a revision on the client, click the "edit" button if it's in this state. This is a little hacky, but simpler to get into a stable state, since the row layout of an inline depends on a "view row" followed by an "edit row".
Test Plan:
- Created comments on either side of a diff.
- Edited a comment, reloaded, saw edit stick.
- Saved comments, reloaded, saw save stick.
- Edited a comment, typed text, cancelled, "unedited" to get state back.
- Created a comment, typed text, cancelled, "unedited" to get state back.
- Deleted a comment, "undeleted" to get state back.
Weirdness / known issues:
- Drafts don't autosave yet.
- Fixed in D21187:
- When you create an empty comment then reload, you get an empty editor. This is a bit silly.
- "Cancel" does not save state, but should, once drafts autosave.
- Mostly fixed in D21188:
- "Editing" comments aren't handled specially by the overall submission flow.
- "Editing" comments submitted in that state try to edit themselves again on load, which doesn't work.
Subscribers: jmeador
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21186
Summary: Ref T13513. This plans for "currently editing", character range comments, code suggestions, document engine tracking. And absolutely nothing else.
Test Plan:
- Ran `bin/storage upgrade -f`, got a clean upgrade.
- Created and submitted some inline comments; nothing exploded.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13513
Differential Revision: https://secure.phabricator.com/D21184
Summary:
At some point, the highlighting behavior for the timeline broke. When you follow a link to a particular timeline story, the story should be highlighted.
Prior to this change, the `<a />` tag itself highlights, but there's no associated CSS and it's too deep in the tree to do anything useful.
(Since this change is fairly straightforward, I gave up digging for the root cause before finding it.)
Test Plan:
- Clicked a timeline story anchor, saw the story highlight.
Differential Revision: https://secure.phabricator.com/D21213
Summary:
Ref T13523. In the caching layer, there's a tricky clause about filetypes that skips some body rendering behavior.
Provide file type information which at least has a better chance of representing all changes (e.g., an image file may be replaced with a text file, but this can not be represented by a single file type).
Formalize "hasSourceTextBody()", to mean the changeset parser should engage the change as source text.
Test Plan: Intradiffed text changes, saw the body render properly.
Maniphest Tasks: T13523
Differential Revision: https://secure.phabricator.com/D21210
Summary: Fixes T13530. The block parser could match too many lines in an unterminated "%%%" literal block. Adjust the logic to stop doing this (and hopefully be a little easier to read).
Test Plan: Added a failing test, made it pass.
Maniphest Tasks: T13530
Differential Revision: https://secure.phabricator.com/D21208
Summary:
See PHI1722, which requests transaction details about reviewer changes.
This adds them; they're structured to be similar to "projects" and "subscribers" transactions and the "reviewers" attachment on revisions.
Test Plan: {F7410675}
Differential Revision: https://secure.phabricator.com/D21207
Summary: This supports the IntelliJ IDEA editor.
Test Plan:
- Looked at the editor settings panel, saw "idea://".
- Set my editor pattern to "idea://a?b".
- (Did not actually install IntelliJ IDEA.)
Differential Revision: https://secure.phabricator.com/D21206
Summary: Ref T13528. Now that we're hinting users into Files, put the content first and move the detail panel under it. Move the most-useful details (author, size, dimensions) into the curtain.
Test Plan: {F7409925}
Maniphest Tasks: T13528
Differential Revision: https://secure.phabricator.com/D21201
Summary:
Ref T13528. The original rationale here was that it's easier to find items at the bottom of the curtain than somewhere in the middle, since they're in a more clearly predictable visual location.
This might be true in some sense, but user feedback about this has fairly consistently indicated that the layout is surprising. Try the other order. See also D20967 for some discussion.
In practice, this primarily moves "Author / Assigned" above other panel elements in Maniphest.
Test Plan: Looked at tasks, aw "Author / Assigned" above "Tags / Subscribers".
Maniphest Tasks: T13528
Differential Revision: https://secure.phabricator.com/D21200
Summary:
Ref T13528. Paste data is stored in files, but the files are always named "raw.txt".
Now that Paste provides a hint to use Files for "DocumentEngine" rendering, try to use the same name as the paste instead.
Test Plan:
- Created a paste named "staggering-insight.ipynb".
- Clicked "View as Jupyter Notebook" from Paste.
- Saw a file named "staggering-insight.ipynb", not "raw.txt".
- Created a paste with no name, saw a file named "raw-paste-data.txt" get created.
Maniphest Tasks: T13528
Differential Revision: https://secure.phabricator.com/D21197
Summary: Ref T13528. When a file in Paste (like a Jupyter notebook) has a good/useful document engine, provide a link to Files.
Test Plan: {F7409881}
Maniphest Tasks: T13528
Differential Revision: https://secure.phabricator.com/D21196
Summary:
See PHI1719. User agents making hard-coded requests to "/favicon.ico" currently 404. This is a mild source of log noise, and we can reasonably route this request.
Limitations:
- This only routes the "PlatformSite". Other sites (custom Phame blogs, third-party sites, Phurl redirectors) won't route here for now.
- This returns a "Location:" redirect to the correct resource rather than icon data directly. This produces the right icon with the right caching behavior, and returning icon data directly is difficult in the general case. However, it won't perform/cache as well as a direct response would.
Test Plan:
- Visted `/favicon.ico`.
- Before: 404.
- After: redirect to favicon.
Differential Revision: https://secure.phabricator.com/D21195
Summary:
Ref T13526. Currently, if a build plan is restricted, viewers may fatal when trying to view related builds.
The old behavior allowed them to see the build even if they can not see the build plan. This is sort of incoherent, but try to stabilize things before fixing this.
Test Plan:
This is a muddy change.
- Created a build with a build plan that Alice can't see.
- As Alice, viewed the build page (restricted before, restricted after); the buildable page (fatal before, works after).
- Also viewed a revision page (works before and after, but user-reported fatal).
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13526
Differential Revision: https://secure.phabricator.com/D21194
Summary:
See PHI1714. This code is incorrectly rendering the chart panel twice, sort of, and passing a non-View object to rendering.
After D21044, this fatals by raising an exception in rendering.
Test Plan: Loaded page, no more exception.
Differential Revision: https://secure.phabricator.com/D21185
Summary:
Ref T13520. Generally, make the table of contents look and more like the paths panel:
- Show a hierarchy, with compression for single-sibling children.
- Use the same icons, instead of "M D" and "(img)" stuff.
- Use EditDistanceMatrix to do a piece-by-piece diff of paths changes.
- Show path changes within the path list.
I'm not entirely sold on this, but it was complicated to write and I've never heard the term "sunk cost fallacy". I think this is mostly a net improvement, but may need some adjustments and followup.
Test Plan: Viewed various changes in Differential and Diffusion, saw a more usable table of contents.
Maniphest Tasks: T13520
Differential Revision: https://secure.phabricator.com/D21183
Summary:
See <https://discourse.phabricator-community.org/t/runtimeexception-during-import-of-commit/3801>. When importing commits with "Auditors:", a raw transaction new value (with an edge edit map using a "+" key) may be passed as an unmentionable PHID list.
Instead, pass an actual PHID list.
Test Plan:
- Pushed a commit with "Auditors: duck".
- Ran daemons.
- Before patch: umentionable PHID exception.
- After patch: clean commit import.
- Verified "duck" was added as an auditor.
Differential Revision: https://secure.phabricator.com/D21181
Summary:
Ref T13523. Currently, when building a "comparison" changeset, metadata is taken from the left changeset. This is somewhat arbitrary.
This means that intradiffs of images don't work properly because the rendered changeset has only the left (usually "old") information.
Later, some of the code attempts to ignore the file data stored on the changeset and reconstruct the correct file data, which is how the result ends up not-completely-wrong.
Be more careful about building sensible-ish metadata, and then just use it directly later on. This fixes the "spooky" code referencing D955 + D6851.
There are some related issues, where "change type" and "file type" are selected arbitrarily and then used to determine whether the change has an "old/new" state or not (i.e., is the left side of the diff empty, since the change creates the file)?
In many cases, neither of the original changesets have a "change type" which will answer this question correctly. Separate this concept from "has state" from "change type", and make more of the code ask narrower questions about the specific conditions or states it cares about, rather than "change type".
Test Plan:
- Created a revision with Diff 1, Diff 2, and Diff 3. Diff 1 takes an image from "null -> A". Diff 2 takes the same image from "null -> B". Diff 3 takes the same image from "A -> B'.
- Intradiffed 1v2 and 1v3.
- Before patch:
- Left side usually missing, which is incorrect (should always be "A").
- Change properties are a mess ("null -> image/png" for MIME type, e.g.)
- Uninteresting/incorrect "unix:filemode" stuff.
- After patch;
- Left side shows state "A".
- Change properties only show size changes (which is correct).
{F7402012}
Maniphest Tasks: T13523
Differential Revision: https://secure.phabricator.com/D21180
Summary:
Fixes T13525. Since D21044, the intermittent GC list warnings are treated more severely and can become user-visible errors.
Silence them, since this seems to be the only realistic response in most versions of APC/APCu.
Test Plan: Will deploy.
Maniphest Tasks: T13525
Differential Revision: https://secure.phabricator.com/D21179
Summary:
Ref T13524. If a Harbormaster lint message has no line number (which is permitted), we try to access an invalid index here. This is an exception after D21044.
Treat comments with no line number as unchanged. These comments do not have "ghost" behavior and do not port across diffs.
Test Plan:
- Used "harbormaster.sendmessage" to submit lint with no line number on a changeset.
- Viewed changeset.
- Before patch: "Undefined index: <null>" error.
- After patch: Clean changeset with lint message.
{F7400072}
Maniphest Tasks: T13524
Differential Revision: https://secure.phabricator.com/D21178
Summary:
See PHI1710. Python encodes `True` as `True` (with an uppercase "T") when building URLs.
We currently do not accept this as a "truthy" value, but it's reasonable and unambiguous. Accept "True", "TRUE", "tRuE", etc.
Test Plan: Made a cURL conduit call with "True" and "tRuE". Before patch: failure to decoded booleans; after patch: successful interpretation of "true" variations.
Differential Revision: https://secure.phabricator.com/D21177
Summary: See PHI1710. Until D21044, some transactions could omit "value" and apply correctly. This now throws an exception when accessing `$xaction['value']`. All transactions are expected to have a "value" key, so require it explicitly rather than implicitly.
Test Plan: Submitted a transaction with a "type" but no "value". After D21044, got a language-level exception. After this change, got an explicit exception.
Differential Revision: https://secure.phabricator.com/D21176
Summary: Ref T13522. When changesets update an image, we currently compute no effect hash. A content hash of the image (or other binary file) is a reasonable effect hash, and enalbes effect-hash-based behavior, including hiding files in intradiffs.
Test Plan:
- Created a revision affecting `cat.png` and `quack.txt` (currently, there must be 2+ changesets to trigger the hide logic).
- Updated it with the exact same changes.
- Viewed revision:
- Saw the image renderered in the interdiff.
- Applied patch.
- Ran `bin/differential rebuild-changesets ...`.
- Viewed revision:
- Saw both changesets collapse as unchanged.
Maniphest Tasks: T13522
Differential Revision: https://secure.phabricator.com/D21174
Summary: Ref T13518. See <https://discourse.phabricator-community.org/t/more-exceptions-when-viewing-diffs/3789/>. Under PHP 7.4, accessing an array index of values like `false` and `null` is no longer valid. This is great, but we occasionally do it.
Test Plan:
- Upgraded to PHP 7.4.
- Loaded revisions with added/changed lines, inlines, and Asana support configured.
- Before patch: saw various fatals around accessing indexes of booleans and nulls.
- After patch: clean revision.
Maniphest Tasks: T13518
Differential Revision: https://secure.phabricator.com/D21172
Summary:
Ref T13493. At time of writing, the old API method no longer functions: `1/session` does not return an `accountId` but all calls now require one.
Use the modern `3/myself` API instead. The datastructure returned by `2/user` (older appraoch) and `3/myself` (newer approach) is more or less the same, as far as I can tell.
Test Plan: Linked an account against modern-at-time-of-writing Atlassian-hosted JIRA.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21170
Summary:
Ref T13517. See that task for details about the underlying issue here.
Currently, we may decode a compressed response, then retransmit it with leftover "Content-Encoding" and "Content-Length" headers. Instead, strip these headers.
Test Plan:
- In a clustered repository setup, cloned a Git repository over HTTP.
- Before: Error while processing content unencoding: invalid stored block lengths
- After: Clean clone.
Maniphest Tasks: T13517
Differential Revision: https://secure.phabricator.com/D21167
Summary:
Ref T4369. During T13507, I set my "max_post_size" to a very small value, like 7 (i.e., 7 bytes). This essentially disables "enable_post_data_reading" even if the setting is technically on.
This breaks forms which use "multipart/form-data", which are rare but not nonexistent. Notably, forms in Config use this setting (because of `ui.header` stuff?) although perhaps they should not or no longer need to.
This can be fixed by parsing the raw input.
Since the only reason we don't parse the raw input is concern that we may not be able to read it (per documentation, but never actually observed), and we do a `strlen()` test anyway, just read it unconditionally.
This should fix cases where POST data wasn't read because of "max_post_size" without impacting anything else.
Test Plan: With very small "max_post_size", updated "ui.footer-items" in Config. Before: form acted as a no-op. After: form submitted.
Maniphest Tasks: T4369
Differential Revision: https://secure.phabricator.com/D21165
Summary: Ref T13516. This isn't terribly clean, but get the page footer into the bottom of the content page on FormationView pages so it doesn't overlap into the side panel.
Test Plan: With and without a footer, viewed normal and FormationView pages. Saw footers in appropriate places at appropriate times.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21166
Summary:
Ref T13455. Viewstates are fairly small and will probably grow less quickly than the changeset table, but the data is also not important to retain in the long term: if you revisit a change several months after hiding some files, it's fine if we've forgotten that you adjusted the view parameters.
Add a GC with a long default collection policy (180 days) so installs can manage the size of this table if it becomes necessary.
Test Plan: Ran via `bin/garbage` to adjust the GC policy and collect viewstates.
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21164
Summary: Ref T13516. Hide this UI on devices without the screen width to reasonably support it.
Test Plan: Viewed a revision at various window widths, saw the elements vanish at device widths and reappear at desktop widths.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21162
Summary:
Ref T13516.
- Add an "Add Comment" navigation anchor.
- Make selection state more clear.
- Make hidden state tidier and more clear.
- Hide "View Options" in the hidden state to dodge all the weird behaviors it implies.
- Click to select/deselect changesets.
- When you open the view dropdown menu, then press "h", close the dropdown menu.
Test Plan: Fiddled with all these behaviors.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21161
Summary:
Ref T13516. Minor improvements here:
- Show key commands in the "View Options" dropdown.
- Organize it slightly better.
- Improve disabled item behaviors a little bit.
- Add a "Browse Directory" action.
- Rename "...in Diffusion" to "...in Repository".
- Make "d", "D", and "h" use the same targeting rules as "\".
- When you hide a file with the "h" menu item, select it.
Test Plan: Poked at the menu a lot, ran into less questionable behavior.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21160
Summary: See PHI1707, which has a Jupyter notebook which fails to diff nicely when modified. The root cause seems to be that the document does not end in a newline.
Test Plan: Applied patch, diffed the file, got a Jupyter diff out of it.
Differential Revision: https://secure.phabricator.com/D21159
Summary:
Ref T13455. Make "hidden" a changeset property similar to other changeset properties.
We don't need to render this on the server, so we make a request (to update the setting) and just discard the response.
Test Plan: {F7375468}
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21158
Summary: Ref T13516. Mark low-importance changes (generated code, deleted files) and owned-with-authority changes in the filetree.
Test Plan: {F7375327}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21157
Summary:
Ref T13516. Deletes all old filetree / flex / active / collapse nav code in favor of the new code.
Restores the inline tips in the path tree.
Test Plan: {F7374175}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21154
Summary: Ref T13516. Apply basic UI styling to the new UI and make some more interaction work.
Test Plan: {F7374096}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21153
Summary: Ref T13516. Generate a tree structure based on the page changesets. Still missing styles and a whole lot of behavior.
Test Plan: {F7373967}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21152
Summary: Ref T13516. This glues "FormationView" to "ChangesetList". The actual tree is not functional in any meaningful way yet.
Test Plan: {F7373838}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21151
Summary:
Ref T13516. Currently, the "File Tree" element is a semi-dynamic side panel that's implemented as a special mode of a side nav panel.
This implementation is fairly clunky, and arose from organic growth out of the side nav. As such, it has some weird behaviors, doesn't have builtin support for show/hide, and can't generalize easily.
Introduce a "FormationView" which supports loading a page up with piles of side panels in various modes.
Test Plan: No callers and no user-visible impact.
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21150
Summary:
Ref T13515. This restores the "Open in Editor" behavior to Diffusion, and makes "\" work there.
The URI pattern is now sent as a structured template to the client, so the code will work properly if a file path contains "%l".
Test Plan:
- Clicked "Open in Editor" and pressed "\" in Diffusion when viewing a file.
- Clicked a line, hit "\", got the file opened to that line.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21149
Summary: Ref T13515. External editor URIs currently depend on repositories having callsigns, but callsigns are no longer required. Add some variables to support configuring this feature for repositories that do not have callsigns.
Test Plan: Changed settings to use new variables, saw links generate appropriately.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21147
Summary:
Ref T13515.
- Previously valid editor URIs may become invalid without being changed (if an administrator removes a protocol from the list, for example), but this isn't explained very well. Show an error on the settings page if the current value isn't usable.
- Generate a list of functions from an authority in the parser.
- Generate a list of protocols from configuration.
Test Plan: {F7370872}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21146
Summary:
Ref T13515. Settings currently has some highly specialized code for rendering "Changes saved." messages. The "saved" state is communicated across a redirect-after-POST by adding `/saved/` to the end of the URI.
This isn't great. It needs a lot of moving pieces, including special accommodations in routing rules. It's user-visible. It has the wrong behavior if you reload the page or navigate directly to the "saved" URI.
Try this scheme, which is also pretty sketchy but seems like an upgrade on the balance:
- Set a cookie on the redirect which identifies the form we just saved.
- On page startup: if this cookie exists, save the value and clear it.
- If the current page started with a cookie identifying the form on the page, treat the page as a "saved" page.
This supports passing a small amount of state across the redirect-after-POST flow, and when you reload the page it doesn't keep the message around. Applications don't need to coordinate it, either. Seems somewhat cleaner?
Test Plan: In Firefox, Safari, and Chrome: saved settings, saw a "Saved changes" banner without any URI junk. Reloaded page, saw banner vanish properly.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21144
Summary:
Ref T13515. Currently, opening a file to a particular line in an external editor relies on replacing "%l" with "%l" (which is escaped as "%25l") on the server, and then replacing "%25l" with the line number on the client. This will fail if the file path (or any other variable) contains "%l" in its unencoded form.
The parser also can't identify invalid variables.
Pull the parser out, formalize it, and make it generate an intermediate representation which can be sent to the client and reconstituted.
(This temporarily breaks Diffusion and permanently removes the weird, ancient integration in Dark Console.)
Test Plan:
- Added a bunch of tests for the actual parser.
- Used "Open in Editor" in Differential.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21143
Summary: Ref T13515. No callsites actually use this, most editors don't support it, it doesn't seem terribly useful for the ones that do, it makes template-based APIs for line-number substitution complicated, and we can probably just loop on `window.open()` anyway.
Test Plan: Grepped for affected symbols, found no more references. Loaded settings page, saw no more setting.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21142
Summary:
Ref T13515. Adding "\" ("Open in External Editor") made this slighlty worse, but it was already pretty bad.
Long ago the keys had a special style on them, but this got changed and dropped somewhere around D16568 -- although at the time, I think they still had a grey background (see T11654).
Some later change removed this background.
Put the background back and separate the keystrokes into groups.
Test Plan: {F7370615}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21141
Summary:
Ref T13515. It's not intuitive that these settings are "Display Preferences", even thought they're intenrally related to some of the other display preferences.
Give them a separate group.
Test Plan: {F7370500}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21140
Summary:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.
Some complexity here arises from these concerns:
- In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
- In all other cases, we render the changeset with AJAX.
So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.
Then, some bookkeeping issues:
- At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
- Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".
Test Plan:
- Viewed changes in Differential, Diffusion, and in standalone mode.
- Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
- Started typing a comment, cancelled it, hit the undo UI.
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21138
Summary:
Ref T13455. Add container-level storage for persistent view state, and persist "Highlight As..." inside it.
The storage generates a "PhabricatorChangesetViewState" configuration object as an output.
When preferences are expressed on a diff and that diff is later attached to a revision, we attempt to copy the preferences.
The internal storage tracks per-changeset settings, but currently always uses "last update wins" to apply the settings in the UI.
Test Plan:
- Viewed revisions, changed highlighting, reloaded. Saw highlighting stick in revision view and standalone view.
- Viewed commits, changed highlighting, reloaded. Saw highlighting stick.
- Created a diff, changed highlighting, turned it into a revision, saw highlighting persist.
Subscribers: jmeador, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21137
Summary:
Ref T13515. We "shield" some changesets, including generated code and intradiffs with no intermediate changes.
These files don't get shielded if they have inline comments.
But, if the viewer has collapsed all the comments, we can shield the file again.
Test Plan:
- Created a change affecting files A and B, with three diffs:
- Touch A and B.
- Touch B only.
- Touch nothing.
- Added an inline to A and collapsed it.
- Viewed Diff 1 vs Diff 2:
- Saw A collapse with a note about inlines.
- Saw B changes, normally.
- Viewed Diff 2 vs Diff 3:
- Saw A collapse with a note about inlines.
- Saw B collapse normally.
- Uncollapsed the inline, viewed 1v2 and 2v3, saw A expand in both cases.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21136
Summary: Ref T13515. See PHI1661. If a file is selected, add a keystroke to click the "Open in External Editor" link.
Test Plan: In Safari, Chrome, and Firefox: used "J" to select a file, then "\" to open it in an external editor. (In Safari and Chrome, this prompts.)
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21135
Summary: Fixes T13512. Archived packages in Owners are missing hinting, but should have it.
Test Plan:
Before:
{F7369122}
After:
{F7369128}
Maniphest Tasks: T13512
Differential Revision: https://secure.phabricator.com/D21134
Summary:
Ref PHI1292. Enable fulltext searchs in paste. Maybe this should only index a snippet instead of the entire content?
Also updates table names in `PhabricatorPasteQuery`.
Test Plan: Created some pastes, indexed them, searched for them.
Reviewers: amckinley
Subscribers: codeblock, Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20650
Summary: Ref T13511. Currently, Ferret fulltext field functions (like "title:") are hard-coded. Modularize them so extensions may define new ones.
Test Plan: Added a new custom field which emits data for the indexer, searched for "animal-noises:moo", "animal-noises:-", etc., in global search and application search.
Maniphest Tasks: T13511
Differential Revision: https://secure.phabricator.com/D21131
Summary:
Ref T13511. Ferret functions currently define "aliases", and some applications override the default aliases.
This probably isn't really the right model, since it means the available function aliases in global search depend on the types of documents you're searching for. This isn't fundamentally unworkable but is kind of weird.
Regardless, these don't actually work. Searching for "description:x" is a syntax error.
Since they don't work, it's a good bet no one is relying on them. Just get rid of them until there's a clearer argument for the feature.
Test Plan: Grepped for "getFunctionMap", got no other hits. Ran some queries with the alias functions, got syntax errors.
Maniphest Tasks: T13511
Differential Revision: https://secure.phabricator.com/D21130
Summary:
Ref T13501. Depends on D21127. With the "prefix" behavior removed in D21127, we now have two virtually identical copies of the same code.
The newer one in Ferret is better: it slices utf8 correctly and is slightly more efficient on large inputs. Pull it out and make all callers call into it.
Test Plan:
- Grepped for all affected symbols.
- Ran `bin/search index --force ...` to reindex various objects (tasks, files).
- Searched for things in the UI.
Maniphest Tasks: T13501
Differential Revision: https://secure.phabricator.com/D21128
Summary:
Ref T13501. The older ngram code has some "prefix" behavior that tries to handle cases where a user issues a very short (one or two character) query.
This code doesn't work, presumably never worked, and can not be made to work (or, at least, I don't see a way, and am fairly sure one does not exist).
If the user searches for "xy", we can find trigrams in the form "xy*" using the index, but not in the form "*xy". The code makes a misguided effort to look for " xy", but this will only find "xy" in words that begin with "xy", like "xylophone".
For example, searching Files for "om" does not currently find "random.txt".
Remove this behavior. Without engaging the trigram index, these queries fall back to an unidexed "LIKE" table scan, but that's about the best we can do.
Test Plan: Searched for "om", hit "random.txt".
Maniphest Tasks: T13501
Differential Revision: https://secure.phabricator.com/D21127
Summary: Ref T13511. This function does nothing interesting and has no callers.
Test Plan: Grepped for callers.
Maniphest Tasks: T13511
Differential Revision: https://secure.phabricator.com/D21126
Summary:
Ref T13507. We currently compress normal responses, but do not compress file data responses because most files we serve are images and already compressed.
However, there are some cases where large files may be highly compressible (e.g., huge XML files stored in LFS) and we can benefit from compressing responses.
Make a reasonable guess about whether compression is beneficial and enable compression if we guess it is.
Test Plan:
- Used `curl ...` to download an image with `Accept-Encoding: gzip`. Got raw image data in the response (as expected, because we don't expect images to be worthwhile to recompress).
- Used `curl ...` to download a text file with `Accept-Encoding: gzip`. Got a compressed response. Decompressed the response into the original file.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21125
Summary:
See <https://hackerone.com/reports/850114>.
An attacker with administrator privileges can configure "notification.servers" to connect to internal services, either directly or with chosen parameters by selecting an attacker-controlled service and having it issue a "Location" redirect.
Generally, we allow this attack to occur. The same administrator can use an authentication provider or a VCS repository to perform the same attack, and we can't reasonably harden these workflows without breaking things that users expect to be able to do.
There's no reason this particular variation of the attack needs to be allowable, though, and the current behavior isn't consistent with how other similar things work.
- Hide the "notification.servers" configuration, which also locks it. This is similar to other modern service/server configuration.
- Don't follow redirects on these requests. Aphlict should never issue a "Location" header, so if we encounter one something is misconfigured. Declining to follow this header likely makes the issue easier to debug.
Test Plan:
- Viewed configuration in web UI.
- Configured a server that "Location: ..." redirects, got a followed redirect before and a failure afterward.
{F7365973}
Differential Revision: https://secure.phabricator.com/D21123
Summary:
Ref T13507. Now that we handle processing of "Content-Encoding: gzip" headers by default, this setup check can get a decompressed body back. Since it specifically wants a raw body back, disable this behavior.
Also, "@" a couple things which can get in the way if they fail now that error handling is more aggressive about throwing on warnings.
Test Plan: Ran setup check after other changes in T13507, got clean result.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21122
Summary: Ref T13507. If we believe the server can accept "Content-Encoding: gzip" requests, make the claim in an "X-Conduit-Capabilities" header in responses. Clients can use request compression on subsequent requests.
Test Plan: See D21119 for the client piece.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21120
Summary: Ref T13507. See that task for discussion.
Test Plan: Faked different response behaviors and hit both variations of this error.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21116
Summary:
See PHI1692. Currently, the Aphlict log is ridiculously verbose. As an initial pass at improving this:
- When starting in "debug" mode, pass "--debug=1" to Node.
- In Node, separate logging into "log" (lower-volume, more-important messages) and "trace" (higher-volume, less-important messages).
- Only print "trace" messages in "debug" mode.
Test Plan: Ran Aphlict in debug and non-debug modes. Behavior unchanged in debug mode, but log has more sensible verbosity in non-debug mode.
Differential Revision: https://secure.phabricator.com/D21115
Summary:
See PHI1692. Currently, it's hard to get a local profile or "--trace" of some Diffusion API methods, since they always proxy via HTTP -- even if the local node can serve the request.
This always-proxy behavior is intentional (so we always go down the same code path, to limit surprises) but inconvenient when debugging. Allow an operator to connect to a node which can serve a request and issue a `--local` call to force in-process execution.
This makes it straightforward to "--trace" or "--xprofile" the call.
Test Plan: Ran `bin/conduit call ...` with and without `--local` using a Diffusion method on a clustered repository. Without `--local`, saw proxy via HTTP. With `--local`, saw in-process execution.
Differential Revision: https://secure.phabricator.com/D21114
Summary: Ref T13509. In `title:big "red" dog`, keep "title" sticky across all three terms, since this seems like it's probably the best match for intent.
Test Plan: Added unit tests; ran unit tests.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21111
Summary:
Ref T13509. Now that the compiler can parse these queries, actually implement them.
These are fairly easy to implement:
- For present, just "JOIN". If it works, the field is present.
- For absent, we "LEFT JOIN" and then "WHERE any_column IS NULL".
Test Plan: Searched for various documents with and without fields present, got sensible results in Maniphest. For example, "body:-" finds tasks with no body, "body:- duck" finds tasks with no body and "duck" elsewhere in the content, and so on.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21110
Summary:
Ref T13509. Since `title:- cat` is now ambiguous, forbid spaces after operators.
Also, forbid spaces inside operators, although this has no effect today.
Test Plan: Added unit tests, ran unit tests.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21109
Summary:
Ref T13509. Currently, functions are "sticky", but this stickness is in the query execution layer.
Instead:
- move stickiness to the query compiler; and
- make it so that functions are not sticky if their arguments are quoted.
For example:
- `title:x y` previously meant `title:x title:y` (and still does). The "title:" is sticky.
- `title:"x" y` previously meant `title:x title:y`. It now means `title:x all:y`. The "title:" is not sticky because the argument is quoted.
Test Plan: Added unit tests, ran unit tests.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21108
Summary: Ref T13509. Parse "xyz:-" as "xyz is absent" and "xyz:~" as "xyz is present". These are new operators which the compiler emits separately from "not" and "substring".
Test Plan: Added unit tests, ran unit tests.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21107
Summary:
Ref T13509. Certain query tokens like `title:=""` are currently accepted by the parser but discarded, and have no impact on the query. This isn't desirable.
Instead, require that tokens making an assertion about field content must be nonempty.
Test Plan: Added unit tests, made them pass.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21106
Summary: Ref T13490. This simplifies some client behavior in the general case.
Test Plan: Called API method, saw URIs.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21105
Summary:
See PHI1697. If a diff is not attached to a revision (for example, if it was created with "arc diff --only"), but is attached to a repository, it is supposed to be visible only to users who can see that repository.
It currently skips this extended policy check and may incorrectly be visible to too many users.
(Once a diff is attached to a revision, this rule is enforced properly via the revision policy.)
Test Plan:
- Set repository R to be visible only to Alice.
- As Alice, created a diff from a working copy of repository R with "arc diff --only".
- As Bailey, viewed the diff.
- Before: visible diff.
- After: policy exception (as expected).
Differential Revision: https://secure.phabricator.com/D21103
Summary: Ref T13490. This simplifies mostly-theoretical cases where you're accessing Phabricator via arc-over-ssh and the Conduit protocol + domain may differ from the production protocol + domain.
Test Plan: Called API via web UI, saw sensible URI values in results.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21102
Summary: Ref T13505. See that task for details. When a class has exactly one "@task" block, this API returns a string. Some day, this should be made more consistent.
Test Plan: Viewed a class with exactly one "@task", no more fatal. Viewed classes with zero and more than one "@task" attributes, got clean renderings.
Maniphest Tasks: T13505
Differential Revision: https://secure.phabricator.com/D21062
Summary: Ref T13505. See that task for discussion.
Test Plan: Ran `diviner generate` locally, found a page fataling on this `strlen()`, applied patch, got a sketchy but not-broken page.
Maniphest Tasks: T13505
Differential Revision: https://secure.phabricator.com/D21061
Summary: See PHI1684. Expose the published state of the "Done" checkbox to the API.
Test Plan: Made API calls on a comment in all four states, got correct published states via the API in all cases.
Differential Revision: https://secure.phabricator.com/D21059
Summary:
See <https://discourse.phabricator-community.org/t/upgrade-from-sep-30-2016/3702/>. A user performing an upgrade from 2016 to 2020 ran into an issue where this setup query is overheating.
This is likely caused by too many rows changing state during query execution, but the particulars aren't important since this setup check isn't too critical and will catch the issue eventually. It's fine to just move on if this query fails for any reason.
Test Plan: Forced the query to overheat, loaded setup issues, got overheating fatal. Applied patch, no more fatal.
Differential Revision: https://secure.phabricator.com/D21057
Summary:
See PHI1688. If many refs with a large amount of shared ancestry are deleted from a repository, we can spend much longer than necessary marking their mutual ancestors as unreachable over and over again.
For example, if refs A, B and C all point near the head of an obsolete "develop" branch and have about 1K shared commits reachable from no other refs, deleting all three refs will lead to us performing 3,000 mark-as-unreachable operations (once for each "<ref, commit>" pair).
Instead, we can stop exploring history once we reach an already-unreachable commit.
Test Plan:
- Destroyed 7 similar refs simultaneously.
- Ran `bin/repository refs`, saw 7 entries appear in the `oldref` table.
- Ran `bin/repository discover` with some debugging statements added, saw sensible-seeming behavior which didn't double-mark any newly-unreachable refs.
Differential Revision: https://secure.phabricator.com/D21056
Summary:
Depends on D21053. Ref T11968. Three things have changed:
- Overseers can no longer use FutureIterator to continue execution of an arbitrary list of futures from any state. Use FuturePool instead.
- Same with repository daemons.
- Probably (?) fix an API change in the Harbormaster exec future.
Test Plan:
- Ran "bin/phd debug task" and "bin/phd debug pull", no longer saw Future-management related errors.
- The Harbormaster future is easiest to test by just seeing if production works once this change is deployed there.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21054
Summary:
We write some synthetic HTTP responses inside unit tests. Some responses have an indirect side effect of adjusting "zlib.output_compression", but this adjustment fails if headers have already been output. From a CLI context, headers appear to count as already-output after we write anything to stdout:
```
<?php
echo headers_sent() ? "Y" : "N";
echo "\n";
echo headers_sent() ? "Y" : "N";
echo "\n";
```
This script prints "N", then "Y".
Recently, the default severity of warnings was increased in libphutil; this has been a long-standing warning but now causes test failures.
This behavior is sort of silly but the whole thing is kind of moot anyway. Just skip it if "headers_sent()" is true.
Test Plan: Ran "arc unit --everything", got clean results.
Differential Revision: https://secure.phabricator.com/D21055
Summary: Jira allows creating projects which contain number in names, phabricator will not allow such projects but it should
Test Plan: Pasted URL with Jira project which contain number in project name and it was parsed and resolved properly in phabricator
Reviewers: epriestley, Pawka, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21040
Summary:
Ref T13493. Google returns a lower-quality account identifier ("email") and a higher-quality account identifier ("id"). We currently read only "email".
Change the logic to read both "email" and "id", so that if Google ever moves away from "email" the transition will be a bit easier.
Test Plan: Linked/unlinked a Google account, looked at the external account identifier table.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21028
Summary:
Fixes T5028. Older versions of Git (apparently, from before 2010) did not provide a way to extract the raw body of a commit message from "git log", so we approximate it with "subject" and "wrapped body".
In newer versions of Git, the raw body can be extracted exactly.
Adjust how we extract messages based on the version of Git, and try to be more faithful to edge cases: particularly, be more careful to extract the correct number of trailing newlines.
Test Plan:
- Added "var_dump()" + "die(1)" later in this method, then pushed various commit messages. Used "&& false" to force execution down the old path (either path should work in modern Git).
- Observed more faithful extraction of messages, including a more faithful extraction of the number of trailing newlines. Extraction is fully faithful if we can go down the "%B" path, which we should be able to in nearly all modern cases.
- Not all messages extract faithfully or consistently across the old and new versions, but the old extraction is destructive so this is likely about as close as we can realistically ever get.
Maniphest Tasks: T5028
Differential Revision: https://secure.phabricator.com/D21027
Summary:
Depends on D21022. Ref T13493. The JIRA API has changed from using "key" to identify users to using "accountId".
By reading both identifiers, this linkage "just works" if you run against an old version of JIRA, a new version of JIRA, or an intermediate version of JIRA.
It also "just works" if you run old JIRA, upgrade to intermediate JIRA, everyone refreshes their link at least once, then you upgrade to new JIRA.
This is a subset of cases and does not include "sudden upgrade to new JIRA", but it's strictly better than the old behavior for all cases it covers.
Test Plan: Linked, unlinked, and logged in with JIRA. Looked at the "ExternalAccountIdentifier" table and saw a sensible value.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21023
Summary: Depends on D21019. Ref T13493. There are no more barriers to removing readers and writers of "accountID"; the new "ExternalAccountIdentity" table can replace it completely.
Test Plan: Linked and unlinked OAuth accounts, logged in with OAuth accounts, tried to double-link OAuth accounts, grepped for affected symbols.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21022
Summary:
Depends on D21018. Ref T13493. Ref T6703. The "ExternalAccount" table has a unique key on `<accountType, accountDomain, accountID>` but this no longer matches our model of reality and changes in this sequence end writes to `accountID`.
Remove this key.
Then, remove all readers of `accountType` and `accountDomain` (and all nontrivial writers) because none of these callsites are well-aligned with plans in T6703.
This change has no user-facing impact today: all the rules about linking/unlinking/etc remain unchanged, because other rules currently prevent creation of more than one provider with a given "accountType".
Test Plan:
- Linked an OAuth1 account (JIRA).
- Linked an OAuth2 account (Asana).
- Used `bin/auth refresh` to cycle OAuth tokens.
- Grepped for affected symbols.
- Published an Asana update.
- Published a JIRA link.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13493, T6703
Differential Revision: https://secure.phabricator.com/D21019
Summary: Depends on D21017. Ref T13493. Update the Asana integration so it reads the "ExternalAccountIdentifier" table instead of the old "accountID" field.
Test Plan: Linked an Asana account, used `bin/feed republish` to publish activity to Asana.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21018
