Summary:
Fixes T13307. We currently require "CAN_EDIT" to sign actions, but it's fine to sign a comment with only "CAN_INTERACT".
Since the actions like "Accept Revision" already work like this, the fix is one line.
Test Plan: {F6488135}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13307
Differential Revision: https://secure.phabricator.com/D20574
Summary:
Ref T13306. Currently, there's no easy way to import a third-party local-disk file dump into a Phacility instance.
Add some more options to `bin/files migrate` to support this. In particular, this enables:
```
$ ./bin/files --from-engine local-disk --engine amazon-s3 --local-disk-source path/to/backup
```
...to import these files into S3 directly.
These are general-purpose options and theoretically useful in other use cases, although realistically those cases are probably very rare.
Test Plan: Used `bin/files` with the new options to move files in and out of local disk storage in an arbitrary backup directory. Got clean exports/imports.
Reviewers: amckinley
Maniphest Tasks: T13306
Differential Revision: https://secure.phabricator.com/D20571
Summary:
Ref T13298. Add a simple profiler as a starting point to catch any egregiously expensive rules or conditions.
This doesn't profile rule actions, so if "Add subscriber" (or whatever) is outrageously expensive it won't show up on the profile. Right now, actions get evaluated inside the Adapter so they're hard to profile. A future change could likely dig them out without too much trouble. I generally expect actions to be less expensive than conditions.
This also can't pin down a //specific// condition being expensive, but if you see that `H123` takes 20s to evaluate you can probably guess that the giant complicated regex is the expensive part.
Test Plan: {F6473407}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13298
Differential Revision: https://secure.phabricator.com/D20566
Summary:
Depends on D20566. Ref T13298. See PHI1280. Currently, there's no clean way to disable problematic personal rules. This comes up occasionally and sometimes isn't really the best approach to solving a problem, but is a generally reasonable capability to provide.
Allow Herald rules (including personal rules) to be disabled/enabled via `bin/herald rule ... --disable/--enable`.
Test Plan: Used the CLI to disable and enable a personal rule.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jmeador
Maniphest Tasks: T13298
Differential Revision: https://secure.phabricator.com/D20567
Summary:
Fixes T13300. Currently, if you create a revision and then immediately land it (either using `--draft` or just beating Harbormaster to the punch) it can be stuck in "Draft" forever.
Instead, count landing changes like this as a publishing action.
Test Plan:
- Used `arc diff --hold` to create a revision, then pushed the commit immediately.
- Before change: revision closed, but was stuck in draft.
- After change: revision closed and was promoted out of draft.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13300
Differential Revision: https://secure.phabricator.com/D20565
Summary:
See PHI1118. That issue may describe more than one bug, but the recent ordering changes to the import pipeline likely make this at least part of the problem.
Previously, commits would always close associated revisions before we made it to the "publish" step. This is no longer true, so we might be triggering audits on a commit before the associated revision actually closes.
Accommodate this by counting a revision in either "Accepted" or "Published (Was Previously Accepted)" as "reviewed".
Test Plan:
- With commit C affecting paths in package P with "Audit Unreviewed Commits and Commits With No Owner Involvement", associated with revision R, with both R and C authored by the same user, and "R" in the state "Accepted", used `bin/repository reparse --publish <hash>` to republish the commit.
- Before change: audit by package P triggered.
- After change: audit by package P no longer triggered.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20564
Summary:
Ref T13303. In D20525 I fixed an issue where transaction rendering could use cached values with the wrong viewer by reloading transactions.
However, reloading transactions may also reorder them as a side effect, since `withPHIDs(...)` does not imply an order. This can make transaction rendering order in mail wrong/inconsistent.
Instead, reorder the transactions before continuing so mail transaction order is consistent.
Test Plan: Applied a group of transactions to a task, saw a more consistent rendering order in mail after the change.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13303
Differential Revision: https://secure.phabricator.com/D20563
Summary:
See downstream <https://phabricator.wikimedia.org/T1050>. Some time ago, we added a "View Revision" button to Differential mail. This hasn't created any problems and generally seems good / desirable.
It isn't trivial to just add everywhere since we need a translation string in each case, but at least add it to Maniphest for now. Going forward, we can fill in more applications as they come up.
Test Plan:
Used `bin/mail show-outbound --id <x> --dump-html`:
{F6470461}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20561
Summary:
See downstream <https://phabricator.wikimedia.org/T88655>. This is very marginal, but we currently allow comments consisting of //only// whitespace.
These are probably always mistakes, so treat them like completely empty comments.
(We intentionally do not trim leading or trailing whitespace from comments when posting them becuase leading spaces can be used to trigger codeblock formatting.)
Test Plan:
- Posted empty, nonempty, and whitespace-only comments.
- Whitespace-only comments now have the same behavior as truly empty comments (e.g., do not actually generate a transaction).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20562
Summary: These instructions are fairly old and can be a little fancier and more clear in the context of modern Phabricator. Drop the reference to "HPHP", link the actual timezone list, wordsmith a little.
Test Plan: d( O_o )b
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20560
Summary:
See downstream <https://phabricator.wikimedia.org/T902>. Currently, timezones are rendered with their raw internal names (like `America/Los_Angeles`) which include underscores.
Replacing underscores with spaces is a more human-readable (and perhaps meaningfully better for things like screen readers, although this is pure speculation).
There's some vague argument against this, like "administrators may need to set a raw internal value in `phabricator.timezone` and this could mislead them", but we already give a pretty good error message if you do this and could improve hinting if necessary.
Test Plan: Viewed timezone list in {nav Settings} and the timezone "reconcile" dialog, saw a more-readable "Los Angeles".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20559
Summary:
Ref T13289. See D20551. In D20551, I implemented some "CAN_INTERACT" checks against certain edits, but these checks end up testing "CAN_INTERACT" against objects like Conpherence threads which do not support a distinct "CAN_INTERACT" permission. I misrembered how the "CAN_INTERACT" fallback to "CAN_VIEW" actually works: it's not fully automatic, and needs some explicit "interact, or view if interact is not available" checks.
Use the "interact" wrappers to test these policies so they fall back to "CAN_VIEW" if an object does not support "CAN_INTERACT". Generally, objects which have a "locked" state have a separate "CAN_INTERACT" permission; objects which don't have a "locked" state do not.
Test Plan: Created and edited comments in Conpherence (or most applications other than Maniphest).
Reviewers: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20558
Summary:
Ref T13289. If you do this:
- Subscribe to a task (so we don't generate a subscribe side-effect later).
- Prepare a transaction group: sign with MFA, change projects (don't make any changes), add a comment.
- Submit the transaction group.
...you'll get prompted "Some actions don't have any effect (the non-change to projects), apply remaining effects?".
If you confirm, you get MFA'd, but the MFA flow loses the "continue" confirmation, so you get trapped in a workflow loop of confirming and MFA'ing.
Instead, retain the "continue" bit through the MFA.
Also, don't show "You can't sign an empty transaction group" if there's a comment.
See also T13295, since the amount of magic here can probably be reduced. There's likely little reason for "continue" or "hisec" to be magic nowadays.
Test Plan:
- Went through the workflow above.
- Before: looping workflow.
- After: "Continue" carries through the MFA gate.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20552
Summary: Ref T11741. See PHI1276. After the switch to "Ferret", this table has no remaining readers or writers.
Test Plan:
- Ran `bin/storage upgrade -f`, no warnings.
- Grepped for class name, table name, `stemmedCorpus` column; got no relevant hits.
- Did a fulltext search.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11741
Differential Revision: https://secure.phabricator.com/D20549
Summary:
Ref T13289. This tightens up a couple of corner cases around locked threads.
Locking is primarily motivated by two use cases: stopping nonproductive conversations on open source installs (similar to GitHub's feature); and freezing object state for audit/record-keeping purposes.
Currently, you can edit or remove comments on a locked thread, but neither use case is well-served by allowing this. Require "CAN_INTERACT" to edit or remove a comment.
Administrators can still remove comments from a locked thread to serve "lock a flamewar, then clean it up", since "Remove Comment" on a comment you don't own is fairly unambiguously an administrative action.
Test Plan:
- On a locked task, tried to edit and remove my comments as a non-administrator. Saw appropriate disabled UI state and error dialogs (actions were disallowed).
- On a locked task, tried to remove another user's comments as an administrator. This works.
- On a normal task, edited comments normally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20551
Summary:
Ref T13289. See <https://discourse.phabricator-community.org/t/fatal-error-in-pagination-in-drydock-resources-host-logs-all-logs/2735>.
`bin/drydock lease` and the web UI for reviewing all object logs when there is more than one page of logs didn't get fully updated to the new cursors.
- Use a cursor pager in `bin/drydock lease`.
- Implement `withIDs()` in `LeaseQuery` so the default paging works properly.
Test Plan:
- Ran `bin/drydock lease`, got a lease with log output along the way.
- Set page size to 2, viewed host logs with multiple pages, paged to page 2.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20553
Summary:
Depends on D20546. Ref T13283. Currently, if you do something (transactions "A", "B") and Herald does some things in response (transaction "C"), Herald acts only on the things you did ("A", "B") since the thing it did ("C") didn't exist yet, until it ran.
However, if you use the test console to test rules against the object we'll pick up all three transactions since they're all part of the same group. This isn't ideal.
To fix this, skip transactions which Herald applied, since it obviously didn't consider them when it was evaluating.
Test Plan:
- Created a revision, in the presence of a Herald rule that adds reviewers.
- Then, ran the revision through the test console.
- Before: saw the "Herald added reviewers: ..." transaction in the transaction group Herald evaluated.
- After: saw only authentic human transactions.
{F6464064}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20547
Summary:
Ref T13283. Currently, each Editor sets its own group ID, so if you create a revision and then Herald does some stuff, the two groups of transactions get different group IDs.
This means the test console is slightly misleading (it will only pick up the Herald transactions). It's going to be misleading anyway (Herald obviously can't evaluate Herald transactions) but this is at least a little closer to reality and stops Herald actions from masking non-Herald actions.
Test Plan:
- Created a revision. Herald applied one transaction.
- Used the test console.
- Before: The test console only picked up the single most recent Herald transaction.
- After: The test console picked up the whole transaction group.
{F6464059}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20546
Summary:
Ref T13289. When you create a Phriction document, you currently get an email with the whole new content as a "diff".
You also get extra transactions in the email and on the page.
This is because Phriction isn't on EditEngine and doesn't mark "create" transactions in a modern way. Get them marked properly to fix these obviously-broken behaviors. This can all go away once Phriction switches to EditEngine, although I don't have any particular plans to do that in the immediate future.
Test Plan:
- Created a new document, viewed email, no longer saw redundant "edited content" transaction or "CHANGES TO CONTENT" diff.
- Updated a document, viewed email, got interdiff.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20548
Summary:
Ref T13290. Prior to recent changes, if we parsed some commit C which was associated with a revision R, but R was already closed, we'd skip the whole set of updates because the "close the revision" transaction would fail and we'd throw because we did not `setContinueOnNoEffect()`.
We now continue on no effect so we can get the edge ("commit has revision" / "revision has commit"), since we want it in all cases, but this means we may also apply an extra "Updated revision to reflect committed changes" transaction and new diff. This can happen even if we're careful about not trying to apply this transaction to closed revisions, since two workers may race. (Today, we aren't too careful about this.)
To fix this, just make this transaction no-op itself if the revision is already closed by the time it tries to apply.
This happened on D20451 because a merge commit with the same hash as the last diff was pushed, but it's easiest to reproduce by just running `bin/repository reparse --message <commit>`, which updates related revisions with a new diff every time.
Test Plan:
- Ran `bin/repository reparse --messsage <commit>` several times, on a commit with an associated revision.
- Before: each run attached a new diff and created a new "updated to reflect committed changes" transaction.
- After: repeated runs had no effects.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13290
Differential Revision: https://secure.phabricator.com/D20545
Summary:
Ref T13290. Ref T13291. Now that a full URI is a "mention", the full URI in "Differential Revision: ..." also triggers a mention.
Stop it from doing that, since these mentions are silly/redundant/unintended.
The API here is also slightly odd; simplify it a little bit to get rid of doing "append" with "get + append + set".
Test Plan: Used `bin/repository reparse --publish` to republish commits with "Differential Revision: ..." and verified that the revision PHID was properly dropped from the mention list.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291, T13290
Differential Revision: https://secure.phabricator.com/D20544
Summary:
See PHI1222. When we publish several transactions to feed at once, we sort them by "action strength" to figure out which one gets to be the title story.
This sort currently uses `msort()`, which uses `asort()`, which is not a stable sort and has inconsistent behavior across PHP versions:
{F6463721}
Switch to `msortv()`, which is a stable sort. Previously, see also T6861.
If all transactions have the same strength, we'll now consistently pick the first one.
This probably (?) does not impact anything in the upstream, but is good from a consistency point of view.
Test Plan:
Top story was published after this change and uses the chronologically first transaction as the title story.
Bottom story was published before this change and uses the chronologically second transaction as the title story.
Both stories have two transactions with the same strength ("create" + "add reviewer").
{F6463722}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20540
Summary:
Ref T13279. See that task for some discussion.
The accumulations of some of the datasets may be negative (e.g., if more tasks are moved out of a project than into it) which can lead to negative area in the stacked chart.
Introduce `min(...)` and `max(...)` to separate a function into points above or below some line, then mangle the areas to pick the negative and positive regions apart so they at least have a plausible physical interpretation and none of the areas are negative.
This is presumably not a final version, I'm just trying to produce a chart that isn't a sequence of overlapping regions with negative areas that is "technically" correct but not really possible to interpret.
Test Plan: {F6439195}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20506
Summary:
Ref T13279. Currently, we store a fairly low-level description of functions and datasets in a chart. This will create problems with (for example) translating function labels.
If you view a chart someone links you, it should say "El Charto" if you speak Spanish, not "The Chart" if the original viewer speaks English.
To support this, store a slightly higher level version of the chart: the chart engine key, plus configuration parameters. This is very similar to how SearchEngine works.
For example, the burndown chart now stores a list of project PHIDs, instead of a list of `[accumulate [sum [fact task.open <project-phid>]]]` functions.
(This leaves some serialization code with no callsites, but we may eventually have a "CustomChartEngine" which stores raw functions, so I'm leaving it for now.)
As a result, function labels provided by the chart engine are now translatable.
(Note that the actual chart is meaningless since the underlying facts can't be stacked like they're being stacked, as some are negative in some areas of their accumulation.)
Test Plan: {F6439121}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20504
Summary:
Ref T13279. Replace the hard-coded default range with a range computed by examining the chart data.
Instead of having a "Dataset" return a blob of wire data, "Dataset" now returns a structure with raw wire data plus a range. I expect to add more structured data here in future changes (tooltip/hover event data, maybe function labels).
Test Plan: {F6439101}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20503
Summary: Ref T13279. Slightly simplify domain handling by putting all the "[x, y]" stuff in an Interval class. I'm planning to do something similar for ranges next, so this should make that easierr.
Test Plan: Viewed chart, saw same chart as before.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20502
Summary: Ref T13279. Makes charts incrementally more useful by allowing the server to provide labels and colors for functions.
Test Plan: {F6438872}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20501
Summary:
Ref T13279. Adds client-side support for rendering function labels on charts, then labels every function as important data.
Works okay on mobile, although I'm not planning to target mobile terribly heavily for v0.
Test Plan: {F6438860}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20500
Summary:
Ref T13279. This adds support for:
- Datasets can have types, like "stacked area".
- Datasets can have multiple functions.
- Charts can store dataset types and datasets with multiple functions.
- Adds a "stacked area" dataset.
- Makes D3 actually draw a stacked area chart.
Lots of rough edges here still, but the result looks slightly more like it's supposed to look.
D3 can do some of this logic itself, like adding up the area stacks on top of one another with `d3.stack()`. I'm doing it in PHP instead because I think it's a bit easier to debug, and it gives us more options for things like caching or "export to CSV" or "export to API" or rendering a data table under the chart or whatever.
Test Plan: {F6427780}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20498
Summary:
Ref T13279. For now, we need to render burndowns from both Maniphest (legacy) and Projects (new prototype).
Consolidate this logic into a "BurndownChartEngine". I plan to expand this to work a bit like a "SearchEngine", and serve as a UI layer on top of the raw chart features.
The old "ChartEngine" is now "ChartRenderingEngine".
Test Plan:
- Viewed burndowns ("burnups") in Maniphest.
- Viewed burndowns in Projects.
- Saw the same chart.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20496
Summary:
Ref T13279. Since the use cases that have made it upstream are all for relatively complex charts (e.g., requiring aggregation and composition of multiple data series in nontrivial ways) I'm currently looking at an overall approach like this:
- At least for now, Charts provides a low-level internal-only API for composing charts from raw datasets.
- This is exposed to users through pre-built `SearchEngine`-like interfaces that provide a small number of more manageable controls (show chart from date X to date Y, show projects A, B, C), but not the full set of composition features (`compose(scale(2), cos())` and such).
- Eventually, we may put more UI on the raw chart composition stuff and let you build your own fully custom charts by gluing together datasets and functions.
- Or we may add this stuff in piecemeal to the higher-level UI as tools like "add goal line" or "add trend line" or whatever.
This will let the low-level API mature/evolve a bit before users get hold of it directly, if they ever do. Most requests today are likely satisfiable with a small number of chart engines plus raw API data access, so maybe UI access to flexible charting is far away.
Step toward this by adding a "Reports" section to projects. For now, this just renders a basic burnup for the current project. Followups will add an "Engine" layer above this and make the chart it produces more useful.
Test Plan: {F6426984}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20495
Summary:
Depends on D20488. Ref T13279. When installs run `bin/phd start`, start the fact daemon alongside other daemons.
Since "Reports" in Maniphest now relies on Facts data, populate it.
Test Plan: Ran `bin/phd start`, saw the Fact daemon start.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20489
Summary: Depends on D20487. If you `min(1, 2, null)`, you get `null`. We want `1`.
Test Plan: Viewed a "burnup for project X" chart where one dataseries had no datapoints. Saw a sensible domain selected automatically.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Differential Revision: https://secure.phabricator.com/D20488
Summary:
Depends on D20485. Ref T13279. This removes the ad-hoc charting in Maniphest and replaces it with a Facts-based chart.
(To do this, we build a dashboard panel inline and render it.)
Test Plan: {F6412720}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20486
Summary:
Depends on D20484. Ref T13279. Allows a chart to render as a panel.
Configuring these is currently quite low-level (you have to manually copy/paste a chart key in), but works well enough.
Test Plan: {F6412708}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20485
Summary:
Ref T13279. This changes the chart controller:
- if we have no arguments, build a demo chart and redirect to it;
- otherwise, load the specified chart from storage and render it.
This mostly prepares for "Chart" panels on dashboards.
Test Plan: Visited `/fact/chart/`, got redirected to a chart from storage.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20483
Summary: Depends on D20538. Ref T13291. We now recognize full source URIs, but encoding full URIs isn't super human-friendly and we can't do stuff like relative links with them. Add `{src ...}` as a way to get to this behavior that supports options and more flexible syntax.
Test Plan: {F6463607}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291
Differential Revision: https://secure.phabricator.com/D20539
Summary:
Depends on D20530. Ref T13291. When users paste links to files in Diffusion into remarkup contexts, identify them and specialize the rendering.
When the URIs are embedded with `{...}`, parse them in more detail.
This is a lead-up to a `{src ...}` rule which will use the same `View` but give users more options to customize presentation.
Test Plan: {F6463580}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291
Differential Revision: https://secure.phabricator.com/D20538
Summary:
Ref T13289. In Maniphest, you can currently search for "Owner: none()" to find tasks with no owner, but there's no way to search for "Reviewers: none()" in Differential right now.
Add support for this, since it's consistent and reasonable and doesn't seem too weird or niche.
Test Plan: Searched for "Reviewers: none()", found revisions with no reviewers. Searched for "Reviewers: alice, none()", "Reviewers: alice", and "Reviewers: <no constraint>" and got sensible results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20537
Summary: Depends on D20534. Ref T13294. Add export support so you can dump these out, print them on paper, notarize them, and store them in a box under a tree or whatever.
Test Plan: Exported transactions to a flat file, read the file.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13294
Differential Revision: https://secure.phabricator.com/D20535
Summary: Depends on D20533. Allow querying for transactions of a specific object type, so you can run queries like "Show all edits to Herald rules between date X and Y".
Test Plan: {F6463478}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20534
Summary: Depends on D20531. Ref T13294. Enable finding raw transactions in particular date ranges or with particular authors.
Test Plan: {F6463471}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13294
Differential Revision: https://secure.phabricator.com/D20533
Summary:
Ref T13294. An install is interested in a way to easily answer audit-focused questions like "what edits were made to any Herald rule in Q1 2019?".
We can answer this kind of question with a more granular version of feed that focuses on being exhaustive rather than being human-readable.
This starts a rough version of it and deals with the two major tricky pieces: transactions are in a lot of different tables; and paging across them is not trivial.
To solve "lots of tables", we just query every table. There's a little bit of sleight-of-hand to get this working, but nothing too awful.
To solve "paging is hard", we order by "<dateCreated, phid>". The "phid" part of this order doesn't have much meaning, but it lets us put every transaction in a single, stable, global order and identify a place in that ordering given only one transaction PHID.
Test Plan: {F6463076}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13294
Differential Revision: https://secure.phabricator.com/D20531
Summary:
See PHI1268. We currently do some weird width handling when rendering Diffusion readmes in a document directory view.
I think this came from D12330, which used `PHUIDocumentViewPro` to change the font, but we later reverted the font and were left with the `DocumentView`. Other changes after that modified `DocumentView` to have fixed-width behavior, but it doesn't make much sense here since the content panel is clearly rendered full-width.
Today, the `DocumentView` is a more structural element with methods like `setCurtain()`. Just get rid of it to simplify things, at least as a first step.
Test Plan:
Before:
{F6463493}
After:
{F6463492}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20536
Summary:
Depends on D20528. Ref T13291. Ref T13285. Currently, we don't put a timeout on external service calls when enriching URIs for external Asana/JIRA tasks.
Add a 15-second timeout so we'll do something reasonable-ish in the face of a downed service provider. Later, I plan to healthcheck Asana/JIRA providers in a generic way (see T13287) so we can stop making calls if they time out / fail too frequently.
Test Plan:
- Linked to JIRA and Asana tasks in comments.
- Set timeout to 0.0001 seconds, saw requests time out.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291, T13285
Differential Revision: https://secure.phabricator.com/D20530
Summary:
Depends on D20527. Ref T13291. Now that we have more flexible support for URI rewriting, use it for Doorkeeper URIs.
These are used when you set up Asana or JIRA and include the URI to an Asana task or a JIRA issue in a comment.
Test Plan:
- Linked up to Asana and JIRA.
- Put Asana and JIRA URIs in comments.
- Saw the UI update to pull task titles from Asana / JIRA using my OAuth credentials.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291
Differential Revision: https://secure.phabricator.com/D20528
Summary:
Ref T13291. Currently, `T123` is a mention and adds an "alice mentioned this on Txxx." to `T123`, but `https://install.com/T123` is not a mention.
Make the full URI a mention.
Test Plan: Commented a full URI, saw the target object get a mention story in its timeline.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291
Differential Revision: https://secure.phabricator.com/D20527
Summary:
See <https://phabricator.wikimedia.org/T179591>. Some time ago, all handle rendering preloaded handles: things emitted a list of PHIDs they'd need handles for, then later used only those PHIDs.
Later, we introduced `HandlePool` and lazy/on-demand handle loading. Modern transactions mostly use this to render object PHIDs.
When we build mail, many newer transactions use an on-demand load to fetch handles to render transactions. This on-demand load may use the original viewer (the acting user) instead of the correct viewer (the mail recipient): we fetch and reset handles using the correct viewer, but do not overwrite the active viewer for on-demand loading. This could cause mail to leak the titles of related objects to users who don't have permission to see them.
Instead, just reload the transactions with the correct viewer when building mail instead of playing a bunch of `setViewer()` and `clone` games. Until we're 100% on modular transactions, several pieces of the stack cache viewer or state information.
Test Plan:
- Created task A (public) with subtask B (private).
- Closed subtask B as a user with access to it.
- Viewed mail sent to subscribers of task A who can not see subtask B.
- Before change: mail discloses title of subtask B.
- After change: mail properly labels subtask B as "Restricted Task".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20525
Summary:
Depends on D20446. Currently, chart functions are both configured through arguments and evaluated through arguments. This sort of conflates things and makes some logic more difficult than it should be.
Instead:
- Function arguments are used to configure function behavior. For example, `scale(2)` configures a function which does `f(x) => 2 * x`.
- Evaluation is now separate, after configuration.
We can get rid of "sourceFunction" (which was basically marking one argument as "this is the thing that gets piped in" in a weird magical way) and "canEvaluate()" and "impulse".
Sequences of functions are achieved with `compose(u, v, w)`, which configures a function `f(x) => w(v(u(x)))` (note order is left-to right, like piping `x | u | v | w` to produce `y`).
The new flow is:
- Every chartable function is `compose(...)` at top level, and composes one or more functions. `compose(x)` is longhand for `id(x)`. This just gives us a root/anchor node.
- Figure out a domain, through various means.
- Ask the function for a list of good input X values in that domain. This lets function chains which include a "fact" with distinct datapoints tell us that we should evaluate those datapoints.
- Pipe those X values through the function.
- We get Y values out.
- Draw those points.
Also:
- Adds `accumluate()`.
- Adds `sum()`, which is now easy to implement.
- Adds `compose()`.
- All functions can now always evaluate everywhere, they just return `null` if they are not defined at a given X.
- Adds repeatable arguments for `compose(f, g, ...)` and `sum(f, g, ...)`.
Test Plan: {F6409890}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Differential Revision: https://secure.phabricator.com/D20454
Summary: Ref T13272. Since the move to EditEngine, these methods have no callsites.
Test Plan: `grep`
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20484
Summary:
Ref T13283. See PHI1202. See D20519. When we apply a group of transactions, label all of them with the same "group ID".
This allows other things, notably Herald, to figure out which transactions applied together in a faithful way rather than by guessing, even though the guess was probably pretty good most of the time.
Also expose this to `transaction.search` in case callers want to do something similar. They get a list of transaction IDs from webhooks already anyway, but some callers use `transaction.search` outside of webhooks and this information may be useful.
Test Plan:
- Ran Herald Test Console, saw faithful selection of recent transactions.
- Changed hard limit from 1000 to 1, saw exception. Users should be very hard-pressed to hit this normally (they'd have to add 990-ish custom fields, then edit every field at once, I think) so I'm just fataling rather than processing some subset of the transaction set.
- Called `transaction.search`, saw group ID information available.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20524
Summary:
See PHI1210. For certain large inputs, we spend more time than we need to replacing tabs with spaces. Add some fast paths:
- When a line only has tabs at the beginning of the line, we don't need to do as much work parsing the rest of the line.
- When a line has no unicode characters, we don't need to vectorize it to get the right result.
Test Plan:
- Added test coverage.
- Profiled this, got a ~60x performance increase on a 36,000 line 3MB text file.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20477
Summary:
See PHI1225. Ref T13277. In Diffusion, show "default", "permanent", or "not permanent" when looking at branches.
For repositories with 100 or fewer branches, put default and permanent branches on top.
Test Plan: {F6426814}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: leoluk
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20493
Summary:
Depends on D20507. See PHI1232. Previously, see T13255 and D20209.
Since nothing seems to have exploded after "projects" was exposed, give "subscribers" the same treatment.
Test Plan: Added, removed, and modified subscribers. Queried transactions with "transaction.search", saw sensible "type" and data.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20508
Summary:
Depends on D20510. Ref T5378. When remarkup includes a hyperlink to the current install in the form "/X123" (which is common), load the corresponding object and specialize the rendering.
This doesn't cover everything (notably, no handling for Diffusion paths yet), but does cover a lot of the most common cases.
The "uri" form preserves the URI as written, but adds an icon, tag, and hovercard.
The "{uri}" form is more similar to `{T123}` and shows the object name.
Test Plan: {F6440367}
Reviewers: amckinley, joshuaspence
Reviewed By: joshuaspence
Subscribers: joshuaspence
Maniphest Tasks: T5378
Differential Revision: https://secure.phabricator.com/D20512
Summary:
Depends on D20509. See PHI1224. Ref T5378. With some frequency, I paste URIs into the global search input (I am dumb).
When I do this dumb thing, redirect to the URI as though the global search was a URI bar.
Maybe only I am dumb like this, but I don't think it'll hurt anything.
Test Plan: pasted a URI and hit return; tried to eat a rock
Reviewers: amckinley, joshuaspence
Reviewed By: joshuaspence
Maniphest Tasks: T5378
Differential Revision: https://secure.phabricator.com/D20510
Summary: Ref T5378. This class was renamed more than a year ago, in D19087. Remove the leftover compatiblity layer.
Test Plan: `grep`
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5378
Differential Revision: https://secure.phabricator.com/D20509
Summary:
See PHI1232, which describes a reasonable use case for wanting information about the "draft" ("Hold as Draft / Do Not Auto-Promote") flag.
Also, flesh out "testPlan" and "summary". It's possible these "blob of remarkup" fields might have metadata some day (e.g., a rendered version or a list of PHIDs or something), but we could add more keys, and we already have some other transactions which work like this.
Test Plan: Used "transaction.search" to fetch these transaction types, saw type information and metadata.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20507
Summary:
See PHI1229. An install has a somewhat duct-taped registration flow which can dump users on the "Wait for Approval" screen without clear guidance. The desired guidance is something like "this is totally normal, just wait a bit for a bot to approve you".
Adding guidance here is generally reasonable and consistent with the intent of this feature.
Test Plan: {F6426583}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: kylec
Differential Revision: https://secure.phabricator.com/D20492
Summary: Depends on D20519. Ref T13283. See PHI1202. Add a new rule which triggers when the current/most-recent transaction group includes a "content" or "publish" transaction, which means the published document content has changed.
Test Plan:
- Wrote a Herald rule using this field.
- Created a document (rule matched).
- Edited a document (rule matched).
- Edited a document, saving as a draft (no match).
- Edited a draft, updating it (no match).
- Published a draft docuemnt (rule matched).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20520
Summary:
Depends on D20518. Ref T13283. When you use the "Test Console" in Herald to test rules, pass the most recent group of transactions to the Adapter.
This will make it easier to test rules that depend on edit state, since you can make the type of edit you're trying to test and then use the Test Console to actually test if it matches in the way you expect.
The transactions we select may not be exactly the group of transactions that most recently applied. For example, if you make edits A, B, and C in rapid succession and then run the Test Console on the object, it may select "A + B + C" as a transaction group. But we'll show you what we selected and this is basically sane/reasonable and should be fine.
(Eventually, we could include a separate "transaction group ID" on transactions if we want to get this selection to match exactly.)
Test Plan: Ran the Test Console on various objects, saw sensible transaction lists in the transcripts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20519
Summary:
Ref T13283. Since D14575, we already pass applied transactions to Herald, but they exist only as a backwards compatibility layer and have no upstream callsites.
Save the applied transaction PHIDs as part of the object transcript, and show them in the UI.
Test Plan:
- Viewed a modern transcript, saw a list of transactions.
- Viewed an older transcript, saw nothing (since there were no transactions in the transcript).
{F6456431}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20518
Summary:
Depends on D20516. See PHI1247. In D20331, I made the crumbs on workboards point at ancestor workboards.
However, this isn't a great destination if an ancestor doesn't actually have a workboard. In this case, point at the normal profile URI instead.
Test Plan:
- Viewed a milestone workboard with a parent that had no workboard. Saw a profile link instead of a workboard link (new behavior).
- Viewed a milestone workboard with a parent that also had a workboard. Saw a workboard link (existing old behavior still works).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20517
Summary:
Ref T13276. Previously, these edges were added directly with an `EdgeEditor`, so they did not generate transaction stories.
Now, they're added properly, but they aren't terribly useful in feed/mail. Hide them in those contexts, like we already do with other types of similar edges.
Test Plan: Will verify behavior on `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20491
Summary:
See PHI1209. When a task is in "Hard Lock" mode, it's still possible to apply some changes to it. Notably:
- You can subscribe/unsubscribe.
- You can mention it on another object.
- You can add a relationship from some other object to it (e.g., select it as a "Parent Task" for some other task).
Currently, these types of edits will show a "Lock Overridden" timeline emblem icon. However, they should not: you didn't override a lock to make these changes, they just bypass locks.
For now, special case these cases (self subscribe/unsubscribe + inverse edge edits) so they don't get the little icon, since I think this list is exhaustive today.
Some day we should modularize this, but we'd need code like this anyway (since TYPE_SUBSCRIBE is not modular yet), and this seems unlikely to cause problems even if it's a bit rough.
Test Plan:
- Hard-locked a task.
- Subscribed/unsubscribed, mentioned, relationship'd it as a non-author. No timeline emblems.
- Soft-locked a task.
- Subscribed/unsubscribed, mentioned, relationship'd it, no timeline emblems.
- Clicked "Edit", answered "yes" to the override prompt, edited it. Got a timeline emblem.
- Added some comments and stuff to a normal non-locked task, no emblems.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20513
Summary:
See PHI1247. If you remove the Workboard from a project profile menu, then navigate to the URI, we currently fatal when trying to select the "Workboard" item.
Instead, only try to select the item if some matching item is present.
Test Plan:
- Disabled the workboard on a project, navigated to `/board/`, got a sensible page with no navigation item selected instead of a fatal.
- Viewed a normal workboard, saw the correct selection.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20516
Summary: Ref PHI1166. I'm documenting our dependencies, and we have approximately 5,000 lines of external code to support WePay as a Phortune provider. We don't use it, I'm almost certain it doesn't work, and we have no plans to use it in the near future. If we did pursue it, I'd probably just wrap the API in a 100-line `WePayFuture` anyway since 5K lines of dependencies to make a couple method calls is ridiculous.
Test Plan: Grepped for `wepay`, `httpful`, `restful`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: aurelijus
Differential Revision: https://secure.phabricator.com/D20521
Summary: Ref T13276. This edge is pointed the wrong way. Point it the right way.
Test Plan: Will verify production works better.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20490
Summary: Depends on D20474. Ref T13272. Provide an easy way to rearrange tabs on a tab panel, by moving them left or right from the context menu.
Test Plan: Moved tabs left and right. Tried to move them off the end of the tab list, no luck.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20475
Summary:
Depends on D20473. Ref T13272. Fixes T7216. If you want to tweak the query a panel uses, you currently have to complete 7 Great Labors.
Instead, add a "Customize Query" action which lets you update the query inline.
Test Plan: {F6402171}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272, T7216
Differential Revision: https://secure.phabricator.com/D20474
Summary:
Depends on D20469. Ref T13276. See PHI1159. See PHI953. See PHI901.
Allow Herald to detect when "arc land" would (or did) warn users about failed or ongoing builds. This respects the "Warn on Landing" build plan behavior.
To accomplish this:
- When we close a revision, set a "wrong build state" flag if it lands in the wrong build state.
- If the revision is closed when we hit Herald, look for the flag.
- If not (common for push rules, can happen for commit rules if we race against the revision update worker), hit Harbormaster ourselves and check the current state.
Test Plan:
- Wrote a "Require Green" rule.
- Ran it against various commits with various build states (good, not good).
- Fiddled with "Warn on Landing" and saw the effect in rule evaluation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20470
Summary:
Depends on D20468. Ref T13276. See PHI1008.
When a commit or revision "reverts <hash>", and that's the hash of a commit which has a revision, also write a "reverts" edge to the revision.
Test Plan:
Created "reverts X" objects for:
- a revision;
- a commit;
- a commit with a revision (both got marked properly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20469
Summary:
Depends on D20467. Ref T13277. Currently, the "MessageParserWorker" writes this property on commits, then Herald and Audit both read it.
Make them share code so this property has one writer and one reader. This property isn't great, but at least now the badness is hidden.
Currently, we can't just use edges because they may not have been written yet. I am likely to just do this, soon:
- Just write the edges (in "MessageParserWorker").
- Hide the edges from mail.
However, we'll sort-of lose the "revisionMatchData" explanation thing if I do that. Maybe this is fine? But when commits match because hashes match, it legitimately isn't obvious.
For now, just reduce the amount of harm/badness here.
Test Plan:
- Ran `bin/repository reparse --publish ...`.
- Ran a Herald "Audit" rule using the "Accepted Differential revision" field.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20468
Summary: See PHI1220. Ref T13272. I accidentally left the ability to set a query limit behind when updating this.
Test Plan: Edited a query panel, set/removed the limit, tried to set an invalid limit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20472
Summary:
Depends on D20472. Ref T13272. Currently, when you edit a panel from a dashboard, we try to add the panel to the dashboard. This always works since dashboards no longer enforce panel uniqueness, and you can end up with duplicate panels.
Instead, only add panels if we're creating them.
Test Plan:
- Edited an existing panel, no duplication.
- Created a new panel, saw it added to the dashboard.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20473
Summary:
If you edit an auth message in Auth > Customize Messages, then click "Show Details" in the transaction record, the resulting dialog uses the object's handle's URI to generate a "cancel" button.
Since these handles currently have no URI, the dialog currently has no cancel/done button to close it.
Test Plan: Edited an auth message, clicked "Show Details", was now able to click "Done" to close the dialog.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20471
Summary:
See PHI985. I think we pretty much need to start applying language-specific rules, but we can apply at least one more relatively language-agnostic rule: don't match lines which are indented 3+ levels.
In C++, we may have symbols like this:
```
class X {
public:
int m() { ... }
}
```
..but I believe no mainstream language puts symbol definitions 3+ levels deep.
Also clean up some of the tab handling very slightly.
Test Plan: Tests pass, looked at some C++ code and got slightly better (but still not great) matches.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20479
Summary:
See PHI1218. When rendering "A vs B", we currently show the properties of diff A without modification.
Instead, take properties from the same place we're taking change details.
See T12664 for a followup.
Test Plan:
- In diff A, removed "+x" from a file.
- In diff B, changed the file but did not remove "+x" from it.
- Diffed B vs A.
- Before change: UI incorrectly shows "+x" removed (both sides incorrect, just showing the change from diff A).
- After change: UI shows 100644 -> null, which is half right.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20478
Summary:
See <https://discourse.phabricator-community.org/t/unable-to-reload-object-that-hasnt-been-loaded/2677>.
When editing "Config" objects, they currently get a PHID set outside of the TransactionEditor. They probably should not, but fixing that is likely an involved change.
This causes us to incorrectly fail to detect `$is_new` correctly and try to `reload()` and object with no ID.
To work around this, test for new objects with `getID()` instead of `getPHID()`.
Test Plan: Edited any config value with the web UI.
Reviewers: amckinley
Differential Revision: https://secure.phabricator.com/D20482
Summary:
Depends on D20466. Ref T13277. Currently:
- The "Owners" worker writes ownership relationships (e.g., commit X affects package Y, because it touches a path in package Y) -- these are just edges.
- It also triggers audits.
- Then it queues a "Herald" worker.
- This formally publishes the commit and triggers Herald.
These aren't really separate steps and can happen more easily in one shot. Merge them.
Test Plan:
- Ran `bin/repository reparse --publish` to republish various commits, got sensible behavior.
- Grepped for "IMPORTED_OWNERS", "IMPORTED_HERALD", "--herald", "--owners", and "--force-local" flags.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20467
Summary:
Depends on D20465. Ref T13277. Currently, when a commit is unpublished, we put a single line about it on the "Edit Commit" page. This is pretty much impossible to find.
Move it to the main page. This treatment is more big/bold than I'd probably like to end up, but we should probably overshoot on the explanatory text until users get used to this behavior.
Also, allow searching for only published / unpublished commits.
Test Plan: {F6395705}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20466
Summary:
Depends on D20464. Ref T13277. Broadly:
- Move all the "should publish X" and "why aren't we publishing X" stuff to a separate class (`PhabricatorRepositoryPublisher`).
- Rename things to be more consistent with modern terminology ("Publish", "Permanent Refs").
Test Plan:
This could use some trial-by-fire on `secure`, but:
- Grepped for all symbols.
- Viewed various commits.
- Reparsed commits.
- Here's a commit with an explanation:
{F6394569}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20465
Summary: Depends on D20463. Ref T13277. This flag was added some time before 2015 and I don't think I've ever used it. Just get rid of it.
Test Plan: Grepped for `force-autoclose`, `forceAutoclose`, `AUTOCLOSE_FORCED`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20464
Summary:
Depends on D20462. Ref T13276. Currently, the "Message" parser also updates related tasks and revisions when a commit is published.
For PHI1165, which ran into a race with message parsing, I originally believed we needed to separate this logic and lock + yield to avoid the race. D20462 provides what is probably a better approach for avoiding the race.
Still, I think separating these "update related revisions" and "updated related tasks" chunks into separate workers is a net improvement. There may still be some value in doing lock + yield in the future to deal with other issues, and when we occasionally run into problems with pulling a diff out of the repository to update the revision (usually because the diff is too big) this isolates the problem better and allows the commit to import.
I think the only thing to watch out for here is that Herald may now run before the revision and commit are attached to one another. This is fine for all current Herald rules, we just need to be mindful in implementing new rules.
Test Plan: Used `bin/repository reparse --message` on various commits, including commits that close revisions and close tasks.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20463
Summary:
Depends on D20461. Ref T13276. Ref T13054.
Currently, we acquire the transaction read lock after populating "old values" in transactions and filtering transactions with no effect.
This isn't early enough to prevent all weird chaotic races: if two processes try to apply a "close revision" transaction at the same time, this can happen:
```
PROCESS A PROCESS B
Old Value = Open Old Value = Open
Transaction OK: Yes Transaction OK: Yes
Acquire Read Lock Acquire Read Lock
Got Read Lock! Wait...
Apply Transactions Wait...
New Value = Closed Wait...
Release Lock Wait...
Got Read Lock!
Apply Transactions
New Value = Closed
Release Lock
```
That's not great: both processes apply an "Open -> Closed" transaction since this was a valid transaction from the viewpoint of each process when it did the checks.
We actually want this:
```
PROCESS A PROCESS B
Acquire Read Lock Acquire Read Lock
Got Read Lock! Wait...
Old Value = Open Wait...
Transaction OK: Yes Wait...
Apply Transactions Wait...
New Value = Closed Wait...
Release Lock Wait...
Got Read Lock!
>>> Old Value = Closed
>>> Transaction Has No Effect!
>>> Do Nothing / Abort
Release Lock
```
Move the "lock" part up so we do that.
This may cause some kind of weird second-order effects, but T13054 went through pretty cleanly and we have to do this to get correct behavior, so we can survive those if/when they arise.
Test Plan:
- Added a `sleep(10)` before the lock.
- Ran `bin/repository message --reparse X` in two console windows, where X is a commit that closes revision Y and Y is open.
- Before patch: both windows closed the revision and added duplicate transactions.
- After patch: only one of the processes had an effect.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jmeador
Maniphest Tasks: T13276, T13054
Differential Revision: https://secure.phabricator.com/D20462
Summary: Depends on D20459. Ref T13276. I'll file a followup to actually destroy the table.
Test Plan:
- Grepped for `TABLE_COMMIT`.
- Ran `bin/storage upgrade -f`, got a clean bill of health.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20461
Summary:
Depends on D20458. Ref T13276. Although I'm not thrilled about "needCommitPHIDs()", it has a few callers, including custom fields. Allow "need + attach + get" to survive for now since they're reasonably modern, at least.
However, use edges instead of "TABLE_COMMIT" and require `need...()` + `get...()`, removing the direct `load...()`.
Also remove `RevisionQuery->withCommitPHIDs(...)`, which has no callers.
Test Plan:
- Grepped for `loadCommitPHIDs` (only two hits, the private `RevisionQuery` method).
- Called "differential.getrevision", got commits.
- Viewed a revision, saw "Commits: ...".
- Grepped for `withCommitPHIDs()`, no callers on `RevisionQuery` (some other query classes have methods with this name).
- Called "differential.query", got commits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20459
Summary: Depends on D20457. Ref T13276. Kill all remaining callers to this method and delete it.
Test Plan:
- Grepped for `loadIDsByCommitPHIDs`.
- Viewed blame again to make sure I didn't break it.
- Viewed "History" view for commits with revisions.
- Viewed "Graph" view for commits with revisions.
- Viewed "Merged Commits" table for commits with revisions.
- Viewed "Compare" table for commits with revisions.
- Viewed "Repository" main page history table for commits with revisions.
- Grepped for `linkRevision`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20458
Summary:
Ref T13276. Differential has a pre-edge "TABLE_COMMIT" with about a half-dozen weird callers I'd like to get rid of.
Move blame to use edges instead. (Bonus: this makes blame respect edge edits in the UI.)
Since there are some more callers to clean up this code may move into some "RelatedObjectQueryThing" class or something, but I'm taking it one step at a time for now.
Test Plan: {F6394106}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20457
Summary:
Depends on D20445. Ref T13279. I'm not sure what the class tree of functions actually looks like, and I suspect it isn't really a tree, so I'm hesitant to start subclassing. Instead, try adding some `isSomethingSomething()` methods.
We have some different types of functions:
# Some functions can be evaluated anywhere, like "constant(3)", which always evaluates to 3.
# Some functions can't be evaluated anywhere, but have values everywhere in some domain. This is most interesting functions, like "number of open tasks". These functions also usually have a distinct set of interesting points, and are constant between those points (any count of anything, like "open points in project" or "tasks closed by alice", etc).
# Some functions can be evaluated almost nowhere and have only discrete values. This is most of the data we actually store, which is just "+1" when a task is opened and "-1" when a task is closed.
Soon, I'd like to be able to show ("all tasks" - "open tasks") and draw a chart of closed tasks. This is somewhat tricky because the two datasets are of the second class of function (straight lines connecting dots) but their "interesting" x values won't be the same (users don't open and close tasks every second, or at the same time).
The "subtract X Y" function will need to be able to know that `subtract "all tasks" 3` and `subtract "all tasks" "closed tasks"` evaluate slightly differently.
To make this worse, the data we actually //store// is of the third class of function (just the "derivative" of the line chart), then we accumulate it in the application after we pull it out of the database. So the code will need to know that `subtract "derivative of all tasks" "derivative of closed tasks"` is meaningless, or the UI needs to make that clear, or it needs to interpret it to mean "accumulate the derivative into a line first".
Anyway, I'll sort that out in future changes. For now, simplify the easy case of functions in class (1), where they're just actual functions.
Add "shift(function, number)" and "scale(function, number)". These are probably like "mul" and "add" but they can't take two functions -- the second value must always be a constant. Maybe these will go away in the future and become `add(function, constant(3))` or something?
Test Plan: {F6382885}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20446
Summary:
Depends on D20444. Ref T13279. Instead of ad-hoc parsing and messages, formalize chart function arguments.
Also, add a whole lot of extra type checking.
Test Plan: Built and charted various functions with various valid and invalid argument lists, got sensible-seeming errors and results.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20445
Summary:
See PHI1213. If we don't have identities for "revision X closed by commit Y" stories, just do the plain non-attribution rendering rather than trying to fall back. Falling back won't work since we don't load the data, which should be OK now since identities seem like they're in generally good shape.
(We could probably just throw out the fallback behavior instead, but we can always clean things up later.)
Test Plan: Forced no commit identity on a revision, loaded it, saw reasonable story rendering.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20460
Summary:
Ref T13266. See <https://discourse.phabricator-community.org/t/notification-page-throws-unrecoverable-fatal-error/2651/>.
The "notifications" query currently uses offset paging for no apparent reason (just a legacy issue?), so some of the paging code is only reachable internally.
- Stop it from using offset paging, since modern cursor paging is fine here (and Feed has used cursor paging for a long time).
- Fix the non-offset paging to work like Feed.
Also:
- Remove a couple of stub methods with no callsites after cursor refactoring.
Test Plan:
- Set things up so I had more than 100 notifications and some in the first 100 were policy filtered, to reproduce the issue (I just made `FeedStory` return `NO_ONE` as a visibility policy).
- Applied the patch, notifications now page cleanly.
- Verified that "Next Page" took me to the right place in the result list.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: hskiba
Maniphest Tasks: T13266
Differential Revision: https://secure.phabricator.com/D20455
Summary:
Ref T13280. In D20421, I changed our observe strategy to `git fetch <uri>` in all cases.
This doesn't work in an ancient, non-bare repository if `master` is checked out and `master` is also fetch: `git` refuses to overwrite the local ref unless we pass `--update-head-ok`. Pass this flag.
Also, remove some code which examines branches and tags in a special way for non-bare working copies. The old `git fetch <origin>` code without explicit revsets meant that `refs/remotes/orgin/heads/xyz` got updated instead of `refs/heads/xyz`. We now update our local refs in all cases (bare and non-bare) so we can throw away this special casing.
Test Plan:
- Replaced a modern bare working copy with a non-bare working copy by explicitly using `git clone` without `--bare`.
- Ran `bin/repository update`, hit the `--update-head-ok` error. Applied the patch, got a clean update.
- Used the "repository.branchquery" API method...
- ...with "contains" to trigger the "git branch" case. Got identical results after removing the special casing.
- ...without "contains" to trigger the "low level ref" case. Got identical results after removing the special casing.
- Grepped for `isWorkingCopyBare()`. The only remaining callsites deal with hook paths, and genuinely need to be specialized.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13280
Differential Revision: https://secure.phabricator.com/D20450
Summary:
Ref T13279. Charting changes alter how the "line-chart" behavior works, but the "Burnup Chart" still relies on the old behavior.
Although I'm intending to remove "Maniphest > Reports" once Facts is a minimally sufficient replacement, copy this behavior to keep it working until we're ready to pull the trigger.
Also fix a leftover typo from D20435.
Test Plan: Viewed a legacy Maniphest burnup rate report.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20449
Summary: This always bugs me when I'm going through `secure` looking for the spiciest macros.
Test Plan: Forced a date to be null, saw reasonable text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20453
Summary:
Depends on D20443. Ref T13279. This is probably not terribly useful on its own, but is mostly a function which takes another function as an argument, and a step toward more useful functions like arithmetic and drawing a picture of an owl.
The only structural change here is that functions now read data parameters (domain, sample limit) using a more tailored "ChartDataQuery" instead of reading the actual axis. Mostly, I want a more cohesive representation of query state that can be easily passed to sub-functions, as here.
Test Plan: {F6382432}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20444
Summary:
Depends on D20442. Ref T13279. Add basic support for drawing chart functions that are not based on Facts first-party ETL datasets. Some general goals:
- This might be useful to draw a line like "goal" or "profitability".
- This might be useful to pull data from an external source.
- For composable functions like "add" or "subtract", which are useful in manipulating ETL datasets, these value functions will make testing easier.
Test Plan:
Added a `constant(256)` function:
{F6382408}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20443
Summary:
Depends on D20441. Ref T13279. Currently, we pull all the data, then decide what the X-axis should look like.
Since users will reasonably want to do stuff like "show me march-april 2018" in the future, we need to move toward flipping this around so that we can support cases where the domain is specified by the user.
For actual chart functions (like "constant(3)" or "cos(x)"), we must also know the domain before we pull data, since there are an infinite number of places where we can evaluate the function "constant(3)".
See note in T13279 about continunity.
Test Plan: {F6382356}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20442
Summary:
Depends on D20440. Ref T13279. Create a class to represent a chartable function: something we can get some data points out of.
Then, make the chart chart two functions.
For now, the only supported function is "fact(key)", which pulls data from the Facts ETL pipeline, identified by "key", and takes no other arguments.
In future changes, I plan to support things like "fact(tasks.open.project, PHID-PROJ-xyz)", "constant(1000)" (e.g. to draw a goal line), "sum(fact(...), fact(...))" (to combine data from several projects), and so on.
The UI may not expose this level of power for a while (or maybe ever) but until we get close enough to the UI that these features are a ton of extra work I'm going to try to keep things fairly flexible/modular.
Test Plan: {F6382286}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20441
Summary:
Depends on D20439. Ref T13279. Some day, charts will probably need to reload themselves or do a bunch of defer/request-shaping magic when they're on a dashboard with 900 other charts.
Give the controller separate "HTML placeholder" and "actual data" modes, and make the placeholder fetch the data in a separate request.
Then, make the chart redraw if you resize the window instead of staying at whatever size it started as.
Test Plan:
- Loaded a chart, saw it load data asynchronously.
- Resized the window, saw the chart resize.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20440
Summary:
Ref T13279. I think I'm going to fling some stuff at the wall for a bit here and hope most of it sticks, so this series of changes may not be terribly cohesive or focused. Here:
The range of the chart is locked to "[0, 105% of max]". This is trying to make a pleasing extra margin above the maximum value, but currently just breaks charts with negative values. Later:
- I'll probably let users customize this.
- We should likely select 0 as the automatic minimum for charts with no negative values.
- For charts with positive values, it would be nice to automatically pick a pleasantly round number (25, 100, 1000) as a maximum by default.
We don't have any storage for charts yet. Add some. This works like queries, where every possible configuration gets a short URL slug. Nothing writes or reads this yet.
Rename `fn()` to `css_function()`. This builds CSS functions for D3. The JS is likely to get substantial structural rewrites later on, `fn()` was just particularly offensive.
Test Plan: Viewed a fact series with negative values. Ran `bin/storage upgrade`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20438
Summary:
Depends on D20434. Fixes T5963. Broadly, the issue here is that when:
- You create a new, empty repository.
- Then, you work on some branch other than `master`, without ever creating `master`.
...you get a warning on `git clone`:
> warning: remote HEAD refers to nonexistent ref, unable to checkout
To fix this, point the symbolic-ref HEAD at `refs/heads/<default-branch>` after installing commit hooks.
This fixes the warning, and also means that `git clone` will check out the repository default branch by default, which is nice.
There are a few caveats about this behavior (see T5963 for discussion) but nothing too substantial.
The only real issue is that Git prevents deletion of the default branch without a config setting. Just set that settting.
Test Plan:
See T5963.
In a repository, set `HEAD` to point somewhere invalid. Ran `bin/repository update ...`. Saw HEAD pointed back at the repository default branch.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5963
Differential Revision: https://secure.phabricator.com/D20435
Summary:
Depends on D20433. Ref T13277. Since "Autoclose" no longer exists, update the documentation.
Currently, this documentation focuses a lot on troubleshooting because users historically had a lot of trouble with figuring out why things were or were not autoclosing. I haven't seen any real confusion about this in years, so I suspect we may have improved the import pipeline and/or UI to make this less of a problem.
It's also possible that this document "fixed" the problem, but usually I expect a documentation fix to not affect the frequency of reports, just make them easier to resolve, so I doubt it.
If unclear things remain //and// documentation really did fix it, maybe we can fix the issues. Or we can just put the troubleshooting documentation back.
Test Plan: Read documentation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20434
Summary:
Depends on D20432. Ref T13277. Fixes T12967. Removes some "Track Only" hints and warns that the feature is deprecated in favor of "Permanent Refs" and "Fetch Only".
(This "fixes" T12967 by mooting it.)
Test Plan: Viewed "branches" sectino of the manage UI, edited "braches" section of a repository.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277, T12967
Differential Revision: https://secure.phabricator.com/D20433
Summary:
Depends on D20427. Ref T13277. As an optimization, when we discover that a commit which was previously only on a non-permanent ref ("tmp-epriestley-123") is now reachable from a permanent ref ("master"), we currently queue only a new "message" parse step.
This is an optimization because these commits previously got the full treatment (feed, publish, audit, etc) as soon as they were discovered. Now, those steps only happen once a commit is reachable from a permanent ref, so we need to run everything.
Test Plan:
- Pushed local "tmp-123" branch to remote tag "tmp-123".
- Updated repository with "bin/repository update", saw commit import as a "not on any permanent ref" commit, with no herald/audit/etc.
- Merged "tmp-123" tag into "master".
- Pushed new "master".
- Updated repository with "bin/repository refs ... --trace --verbose", saw commit detected as now reachable from a permament ref.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20428
Summary:
Depends on D20426. Ref T13277. The new behavior is to fire Herald only once a commit becomes reachable from a permanent ref (previously, an "Autoclose" branch).
That means that every "Commit" Herald rule implicitly has this field as a condition, and it no longer does anything.
Test Plan: Wrote a Herald rule, saw this as an option in the "Deprecated" section.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20427
Summary:
Depends on D20425. Ref T13277. See PHI1067. There's currently no way to retrieve branch/ref rules over the API, which makes some management operations against a large number of repositories difficult.
Expose these rules to the API.
Test Plan: Called `diffusion.repository.search`, got rules in the result set.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20426
Summary:
Depends on D20424. Ref T13277. Now that the "Actions" panel only has one item ("Publishing"), just move it to the "Basics" panel.
Update the UI to show active/publishing status more clearly and relate them to one another and importing state.
Test Plan: {F6378087}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20425
Summary:
Depends on D20423. Ref T13277. Repositories currently have separate toggles for "Autoclose" and "Publishing".
Merge the "Autoclose" toggle into the "Publishing" toggle. I'm unaware of any valid use case for enabling one but not the other.
(This doesn't fix all the documentation, yet.)
Test Plan: Edited a repository, saw only one publishing option.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20424
Summary:
Depends on D20422. Ref T13277. Currently, "track only", "publish", and "autoclose" are three separate ideas. I'd like to generally merge them into a more natural idea called "permanent refs".
Since "Autoclose" effectively now controls both "autoclose" and "publish", rename it.
This doesn't rename all the methods or internals, and the documentation needs an update, but it renames most of the UI-facing stuff.
(You also can only specify branches as "Permanent Refs" today, but we may let you specify tags and other arbitrary refs in the future.)
Test Plan: Grepped, poked around the UI, saw UI show "Permanent" / "Permanent Refs" more often and "Autoclose" less.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20423
Summary:
Depends on D20421. Ref T13277. I'd generally like to move away from "Track Only".
Some of the use cases for "Track Only" (or adjacent to "Track Only") are better resolved with "Fetch Rules" -- basically, rules to fetch only some subset of refs from the observed remote.
Add configurable "Fetch Rules" for Git repositories. For example, if you only want to fetch `master`, you can now speify:
```
refs/heads/master
```
If you only want to fetch branches and tags, you can use:
```
refs/heads/*
refs/tags/*
```
In theory, this is slightly less powerful in the general case than "Track Only", but gives us better behavior in some cases (e.g., when the remote has 50K random temporary branches). In practice, I think this and a better "Autoclose Only" will let us move away from "Track Only", get default behavior which is better aligned with what users actually expect, and dodge all the "track tags/refs" questions.
Test Plan: Configured repositories with "Fetch Refs" rules, used `bin/repository pull --verbose --trace ...` to run pulls, saw expected pull/fetch behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20422
Summary:
Depends on D20420. Ref T13277. We currently spend substantial effort trying to detect and correct the URL of the "origin" remote in Git repositories.
I believe this is unnecessary, and we can always `git fetch <url> ...` to get the desired result instead of `git muck-with-origin + git fetch origin ...`. We already do this in the more recent parts of the codebase (e.g., intracluster sync) and it works correctly in every case I'm aware of.
Test Plan:
- Grepped for `origin`, ` origin `.
- Ran `bin/repository update ...` to fetch a mirrored repository.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20421
Summary:
Depends on D20418. Ref T13277. Fixes T11314.
Currently, when you push commits to some arbitrary ref or tag (like `refs/pull/123` on GitHub, `refs/tags/phabricator/diff/123` on Phabricator, or `refs/changes/whatever` on Gerrit), we do not "autoclose" related objects. This means that we don't process `Ref T123` to create links to tasks, and don't process `Differential Revision: xyz` to close revisions.
However, we //do// still publish these commits. "Publish" means: trigger audits, publish feed stories, and run Herald rules.
- Stop publishing these commits.
- In the UI, show these commits as "Not Permanent" with a note that they are "Not [on any permanent branch]."
These commits will publish and autoclose if they ever become reachable from an "autoclose" ref (most commonly, if they are later merged to "master").
Test Plan:
- Pushed a commit to `refs/tags/quack`.
- Before: got a feed story.
- After: no feed story, UI shows commit as "Not Permanent".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277, T11314
Differential Revision: https://secure.phabricator.com/D20419
Summary:
Depends on D20381. Ref T8093. This makes minor improvements to the protocol proxy to handle cases where we add, remove, or replace refs and may need to move the "capabilities" section.
Rather than invoking a callback on every ref: parse the whole ref list into a data structure, mutate it if necessary (in a future diff), then dump it back into wire format.
This allows us to shift the capabilities data (which needs to be coupled with the first ref) around if we modify the first ref, and reorder the reflist alphabetically like git does.
When the server has no refs, Git sends no capabilities data. This is easy to emulate, just surprising.
Test Plan:
Tested the cases not covered by D20381:
- Fetching where the fetch actually fetches data.
- `ls-remote` when we hide the first ref (capabilities data properly moves to the first visible ref).
- `ls-remote` when the remote is empty (we just drop the capabilities frame completely).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8093
Differential Revision: https://secure.phabricator.com/D20436
Summary:
Depends on D20380. Ref T8093. When prototypes are enabled, inject a (hopefully?) no-op proxy into the Git wire protocol.
This proxy decodes "git upload-pack" and allows the list of references to be rewritten, in a similar way to how we already proxy the Subversion protocol to rewrite URIs and proxy the Mercurial protocol to distinguish between read and write operations.
The piece we care about comes at the beginning, and looks like this:
```
<frame-length><ref-hash> <ref-name>\0<server-capabilities>\n
<frame-length><ref-hash> <ref-name>\n
<frame-length><ref-hash> <ref-name>\n
...
<0000>
```
We can add, remove, or modify this section to make it appear that the server has different refs than the refs that exist on disk.
Things I have tried:
- `git ls-remote`
- `git ls-remote` where the server hides some refs.
- `git fetch` where the fetch is a no-op.
Things I have not tried:
- `git fetch` where the fetch is not a no-op.
- Tricking things into doing protocol v2. Or: I tried this, I wasn't successful. In v2, additional "\0" tricks are used to hide data in the capabilities, I think?
- `git ls-remote` where we rewrite/hide the first ref in the list, and need to move the capabilities frame elsewhere.
- `git ls-remote` where the server has no refs at all, or we remove every ref.
So the "interesting" piece of this works, but it almost certainly needs some cleanup to survive interaction with the real world.
Test Plan: See above.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8093
Differential Revision: https://secure.phabricator.com/D20381
Summary:
Ref T8093. Support dumping the protocol bytes to a side channel logfile, as a precursor to parsing the protocol and rewriting protocol frames to virtualize refs.
The protocol itself is mostly ASCII text so the raw protocol bytes are pretty comprehensible.
Test Plan:
{F6363221}
{F6363222}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8093
Differential Revision: https://secure.phabricator.com/D20380
Summary: Ref T7667. Also one text fix.
Test Plan: Ran `bin/auth lock`, didn't get a replacement setup issue.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20447
Summary:
See PHI1134. Generally, "alice added a dependent revision: ..." isn't a very interesting story. This relationship itself is valuable, but the creation of the relationship is usually pretty obvious from context.
In the specific case of PHI1134, various scripts are racing one another, but I don't think this story is of much value in the general case anyway.
Test Plan: Edited parent/child revisions, no more feed stories.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20437
Summary:
Depends on D20419. Ref T13277. Fixes T8936. Fixes T9383. Fixes T12300. When you push arbitrary refs to Phabricator, the push log currently complains if those refs are not tags or branches.
Upstream Git now features "notes", and there's no reason to prevent writes to arbitrary refs, particularly beause we plan to start using them soon (see T13278).
Allow these writes as affecting raw refs.
Test Plan:
- Pushed an arbitrary ref.
- Pushed some Git notes.
- Wrote a Herald ref rule, saw "ref" in the dropdown.
{F6376492}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277, T8936, T9383, T12300
Differential Revision: https://secure.phabricator.com/D20420
Summary:
Depends on D20416. Ref T13269. See D20329.
If you try to save an "Assign to" rule with no assignee, we currently replace the control with an "InvalidRule" control that isn't editable. We'd prefer to give you an empty field back and let you pick a different value.
Differentiate between "bad record format" (i.e., we can't really do anything with this) and "bad record value" (i.e., everything is structurally fine, you just typed the wrong thing). In the latter case, we still build a properly typed rule for the UI, we just refuse to update storage until you fix the problem.
Test Plan:
First, hit the original issue and got a nicer UI with a more consistent control width (note full-width error):
{F6374205}
Then, applied the rest of the patch and got a normal "fix the issue" form state instead of a dead-end:
{F6374211}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20417
Summary:
Ref T12164. Ref T13276. Currently, the parsing pipeline copies the author and committer names and PHIDs into the transcaction record as metadata. They are then rendered directly from the metadata.
This makes planned changes to the parsing pipeline (to prevent races when multiple commits matching a single revision are pushed simultaneously) more difficult, and generally won't work with repository identities.
Instead, load the commit and use its identities.
Test Plan: Loaded a revision, saw the same story rendering for a "Closed by commit" story.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276, T12164
Differential Revision: https://secure.phabricator.com/D20418
Summary:
Ref T13269. See D20329. When we switch trigger rule control types, reset the rule value.
Also, pick slightly nicer defaults for status/priority.
Test Plan:
- Created a "Change Status To: X" rule.
- Saved it.
- Edited it.
- Selected "Assign to" for the existing action's dropdown.
- Before: tokenizer filled with nonsense.
- After: tokenizer cleared.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13269
Differential Revision: https://secure.phabricator.com/D20416
Summary:
Depends on D20414. Ref T13272. Several minor things here:
- Currently, you can drag panels underneath the invisible "there are no items in this column" div and the "Create Panel / Add Existing Panel" buttons. This is silly; stop it.
- Currently, when viewing a tab panel on a dashboard, you can drag the panels inside it. This is extremely silly. Make "movable" off by default and pass it through the async flow only when we actually need it.
- Make the whole "Add Tab..." virtual tab clickable to open the dropdown. This removes the rare exception/todo combo I added earlier. {key F}
- Add or remove some icons or something.
Test Plan: Moved panels around on dashboards. Tried to drag panels inside tab panels. Added tab. Things were less obviously broken.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20415
Summary:
Depends on D20413. Ref T13272. When you search for stuff, you can "Use Results > Add to Dashboard" to generate a query panel.
This needs some updating after the recent refactoring. All the changes are pretty straightforward. Swaps a giant `<select />` for a tokenizer with a datasource.
Test Plan: Used the "Use Results > Add to Dashboard" flow to create a panel on a dashboard using a query.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20414
Summary:
Depends on D20412. See PHI1147.
- Index the targets of "Add Reviewer", "Add Blocking Reviewer", "Add Auditor", "Add Subscriber", and "Remove Subscriber" Herald rules. My major goal is to get Owners packages. This will also hit projects/users, but we just don't read those edges (for now, at least).
- Add a "Related Herald Rules" panel to Owners Package pages.
- Add a migration to reindex Herald rules for the recent build plan stuff and this, now that such a migration is easy to write.
Test Plan:
Ran migration, verified all rules reindexed.
{F6372695}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20413
Summary:
Depends on D20411. Ref T13272. Dashboards and panels have new indexes (Ferret and usage edges) that need a rebuild.
For large datasets like commits we have the "activity" flow in T11932, but realistically these rebuilds won't take more than a few minutes on any realistic install so we should be able to just queue them up as migrations.
Let migrations insert a job to basically run `bin/search index --type SomeObjectType`, then do that for dashboards and panels.
(I'll do Herald rules in a followup too, but I want to tweak one indexing thing there.)
Test Plan: Ran the migration, ran `bin/phd debug task`, saw everything get indexed with no manual intervention.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20412
Summary:
Ref T7667. On the road to locking the auth config, also clean up some minor UI issues:
* Only show the warning about not Phacility instance auth if the user isn't a manager (see next diff).
* When rendering more than one warning in the guidance, add bullets.
* I didn't like the text in the `auth.config-lock` config setting.
Test Plan: Loaded the page, saw more reasonable-looking guidance: {F6369405}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20400
Summary:
Depends on D20410. Ref T13272. Dashboards/Panels currently use older "ngram" indexing, which is a less-powerful precursor to Ferret. Throw away the ngram index and provide a Ferret index instead. Also:
- Remove the NUX state, which links to the wrong place now and doesn't seem terribly important.
- Add project tags to the search result list.
- Make the "No Tags" tag a little less conspicious.
Test Plan:
- Indexed dashboards and panels.
- Searched for dashboards and panels via SearchEngine using Ferret "query" field.
- Searched for panels via "Add Existing Panel" datasource typeahead.
- Searched for dashboards via "Add Menu Item > Dashboard" on a ProfileMenu via typeahead.
- Viewed dashboard NUX state (no special state, but no more bad link to "/create/").
- Viewed dashboard list, saw project tags.
- Viewed dashboards with no project tags ("No Tags" is now displayed but less visible).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20411
Summary: Depends on D20409. Ref T13272. Before "ProfileMenu", dashboards were installed on specific objects using this table. Installs are now handled via ProfileMenu and this table no longer has any meaningful readers. Remove references to the table and destroy it.
Test Plan: Grepped for `DashboardInstall`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20410
Summary: Depends on D20408. Ref T13272. The actual JS is still a little bit iffy, but this makes the server side "move" operation work correctly by updating it to use the same code as everything else.
Test Plan: Moved panels around on single-column and multi-column dashboards, saw them move to reasonable places and stay there when I reloaded the page.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20409
Summary:
Depends on D20407. Ref T13272. This updates the "add panel" (which has two flavors: "add existing" and "create new") and "remove panel" flows to work with the new duplicate-friendly storage format.
- We now modify panels by "panelKey", not by panel PHID, so one dashboard may have multiple copies of the same panel and we can still figure out what's going on.
- We now work with "contextPHID", not "dashboardID", to make some flows with tab panels (or other nested panels in the future) easier.
The only major remaining flow is the Javascript "move panels around with drag-and-drop" flow.
Test Plan:
- Added panels to a dashboard with "Create New Panel".
- Added panels to a dashboard with "Add Existing Panel".
- Removed panels from a dashboard.
- Added and removed duplicate panels, got a correctly-functioning dashboard that didn't care about duplicates.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20408
Summary:
Depends on D20406. Ref T13272. This gets about half of Dashboards working with the new "duplicate panel friendly" storage format. Followups will fix the write pathways.
Collateral damage here includes:
- Remove the old Dashboard/Panel edge type. We have a new, more general edge type for "container X uses panel Y", and we don't need this edge type for anything else.
- Remove "attachPanels()" from Dashboard. Only rendering actually needs this, and it can just load the panels.
- Remove "attachPanelPHIDs()" from Dashboard. We can look at the panel refs to figure this out.
- Remove "attachProjects()" from Dashboard. Nothing uses this and it's not a very modern approach.
- `getPanelPHIDs()` just looks at the config now.
- Deleted some `LayoutConfig`-related code which is broken/obsolete.
Test Plan:
- Viewed various dashboards which were created before the changes, saw them render correctly.
- Viewed a dashboard with two of the same panel! AMAZING!
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20407
Summary:
Depends on D20405. Ref T13272. Currently, the `PhabricatorDashboardLayoutConfig` class uses a lot of `switch()` statements to define layout modes.
Although I'm not planning to add thousands of new layout modes, this (and upcoming changes) can be made substantially cleaner by using a standard modular approach.
(This doesn't fully remove `PhabricatorDashboardLayoutConfig` yet, but that will happen soon.)
Test Plan: Edited a dashboard, saw the same layout modes as before.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20406
Summary:
Depends on D20402. Ref T13272. Replaces an old-school hard-coded "EditController" with a more modern one.
The actual panel stuff is still using a weird mix of legacy manual `save()` calls, but that's up next.
Test Plan: Created Dashboards, edited all dashboard fields via "Edit Dashboard".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20403
Summary:
Depends on D20399. Ref T13272. I'm moving toward fixing all the "moving panels around on Dashboards breaks the entire world" problems.
On the way there, modularize Dashboard transactions.
Test Plan:
- Created a new Dashboard.
- Edited all fiedls of a dashboard.
- Archived/restored a dashboard.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20402
Summary:
Depends on D20398. Ref T13272. Fixes T6018. Previously, panels showed "used on dashboards: x, y", but this did not include cases where a panel was used by another container panel (today, a tab panel).
Do edge indexing when a dashboard or panel is saved, then pull the edges on the Panel page so we can provide a full list of uses.
Test Plan: {F6369289}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272, T6018
Differential Revision: https://secure.phabricator.com/D20399
Summary:
Depends on D20397. Ref T13272. Similar to the recent "where are Herald rules used" stuff, show which menus Dashboards are installed in.
This is mostly straightforward, except that I pulled some of the Herald logic into a parent class so it could be shared.
Test Plan: {F6369164}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20398
Summary:
Depends on D20396. Ref T13272. Currently, using the dropdowns to edit a tab panel from a dashboard redirects you to the tab panel page.
Instead, redirect back to the context page (usually, a dashboard -- but theoretically a containing tab panel, since you can put tab panels inside tab panels).
Also, fix some JS issues with non-integer panel keys. I've moved panel keys from "0, 1, 2, ..." to having arbitrary keys to make some operations less flimsy/error-prone, but this needs some JS tweaks.
Test Plan: Edited a tab panel from a dashboard, got sent sensibly back to the dashboard.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20397
Summary:
Ref T13272. In edit mode, tab panels now have a dropdown menu. However, this sort of overrlaps with the actual action of clicking the tab to select it.
Separate these into different click targets so that "select tab X" and "open dropdown menu for X" are different operations.
This is more work than it appears because:
- We have an "action icon" already, used when you put a dashboard on a portal/home to create an "Edit" link. It makes sense to attach dropdowns to this, but it has some hard-coded stuff.
- In applications with a "Create <thing>" in the crumbs (like Maniphest), we may use a dropdown menu if there are multiple create forms available. However, this menu renders in a weird way by reading all the properties out of an actual "View" object and building something else.
- The "list of tabs" stuff shares code with different "list of tabs" navigation used by Diffusion and Instances.
..but I think I fixed everything and didn't break anything.
Test Plan:
- Clicked "select tab" and "open dropdown menu" as separate actions.
- Viewed Diffusion, Maniphest with multiple create forms, Instances.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20396
Summary:
Ref T13276. This was introduced in D2586 to power a "trigger audits when the committed change does not match the reviewed change" feature. It was removed without ceremony in D15939. Broadly, rebases mean that this sort of feature can't really work like this and this approach is inherently unreliable; see also T182.
This property no longer has readers, and is unlikely to get any in the future since my planned pathway for "committed code must match reviewed code, modulo an automated rebase" is automating the rebase via "Land Revision", not comparing the diff text.
Remove this to simplify the flow of data here so that things in T13276 can be fixed more easily.
Test Plan: Grepped for `vsDiff`, no hits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13276
Differential Revision: https://secure.phabricator.com/D20395
