Page MenuHomePhabricator

Plans: Hefty Differential revisions, draft state transitions, and exotic interactions
Open, NormalPublic

Description

See PHI431. See PHI426. Multiple installs would like more sophisticated reviewer behavior. Current requests are roughly:

  • "Accept this diff on behalf of packages I have authority for, but revert those packages to blocking reviewers when the revision is updated."
  • "Add me as a blocking reviewer, but only for a short period of time."

I suspect these lead to T731 and T10574, practically.

See PHI523. When a revision auto-updates with a commit, reviewers become "Accepted Prior Diff". We should probably retain their status relative to the last human diff, ignoring the auto-update.

The output of arc diff when a revision affects a very large number of files is not especially useful, since it ends with a list of every file which can leave the actual URI to the revision lost many pages back in your scroll buffer. Better would probably be the first 100 files and then "And 7389 more files", or moving the URI to the bottom.


Largely Resolved?

See PHI483. There's currently a bad interaction between builds completing and drafts unpublishing, described in greater detail in T13108.

See PHI283. When a draft revision is abandoned, it should retain its "draftness" in some sense and not send email. With the above change, revisions should also probably be able to demote into a "Changes Planned + But, Still a Draft" state.

See PHI230, where I plan to explore the [----++ ] element described in D16322 and see how it works.

See PHI485 and PHI489. An install would like better behavior from Differential when reviewing revisions which change 5,000+ files and have 1,000+ reviewers. See also T8612.

See PHI598. Very large revisions still build $packageChangesetMap and $pathPackageMap, but these are mostly not useful. The path map is unused (since we don't render a ToC) and the changeset map is used only to render "(Owns no Paths)" in Reviewers, which we can reasonably just degrade out of without a significant functional impact.

Revisions and Commits

rARC Arcanist
D19299
rP Phabricator
D19418
D19416
D19298
D19296
D19295
D19294
D19293
D19291
D19290
D19289
D19288
D19287
D19286
D19285
D19284
D19283
D19281
D19280
D19279
D19278

Event Timeline

epriestley triaged this task as Normal priority.Mar 21 2018, 3:27 PM
epriestley created this task.

See PHI483. There's currently a bad interaction between builds completing and drafts unpublishing, described in greater detail in T13108.

T13108 lays this out with richer narrative context. This is going to be something like:

  • New Engine for publishing build status changes (or SCOPE EXPLOSION into a new Engine for publishing all changes to all objects?!).
  • The Diffusion version of this engine does exactly what it currently does for now (publish a boring transaction to the timeline).
  • The Differential version of this engine becomes fancier.
  • New bin/harbormaster publish to try to combat the complexity this introduces.
  • DifferentialRevision->loadActiveBuilds() becomes more like DifferentialRevision->getKnownFinalBuildStatus() which returns "pass", "fail", or "not yet known". The response logic changes accordingly. This is now decoupled somewhat from build status in Harbormaster when aggregating the results of autoplans.
  • Maybe a little later, start sending email to authors on build failures.

See PHI283. When a draft revision is abandoned, it should retain its "draftness" in some sense and not send email. With the above change, revisions should also probably be able to demote into a "Changes Planned + But, Still a Draft" state.

Then:

  • Make isSortOfADraftStill a separate flag from the "Draft" status. Maybe PROPERTY_HAS_BROADCAST can represent this exactly, already.
  • Condition broadcasting on this flag only, not the actual draft flag.
  • When a revision with this flag is abandoned, don't clear the flag.
  • Update the UI and API to distinguish between "abandoned" and "abandoned + draft" clearly.
  • When a build failure occurs on a non-held draft, demote to "Changes Planned" and retain the flag.

See PHI356, which has some reasonable suggestions for minor improvements to the filetree view.

  • When a folder in the filetree contains exactly one child, fold the child into the parent.
  • Show which files the viewer has authority over, like the table of contents.
  • Maybe some minor CSS tweaks since I'm not entirely happy with where this is at the moment.

See PHI230, where I plan to explore the [----++ ] element described in D16322 and see how it works.

  • Put it on the dashboard.
  • Maybe put it in email now; maybe wait and see how it goes on dashboards. I'd ideally like to replace "[238 lines]" in email with this element.

See PHI485 and PHI489. An install would like better behavior from Differential when reviewing revisions which change 5,000+ files and have 1,000+ reviewers. See also T8612.

It's no secret that I think these use cases are mostly bad and that these revisions do not actually receive review. However, there are increasingly good reasons to send them through Differential anyway (build integrations, webhooks, submit queue integrations, "regulatory/compliance" reasons, triggering Herald side effects, cohesion-of-process/plain-human-laziness, etc.) fully realizing that no one will really look at the textual changes of the diff.

The strategy I plan to pursue here is to just accept that the changes are not important for these revisions and degrade them to a separate page, while retaining the /Dxyz page for discussion.

These tasks also identify some more-legitimate cases where performance could improve. One example is that if you add 1,000 reviewers to a revision, we render "X added reviewers: ..." and lists all 1,000 reviewers in the timeline. Likewise, the entire set of reviewers is shown on the dashboard. Although adding 1,000 reviewers isn't really "legitimate review", it also isn't absurdly abusive, and our behavior isn't very good and these interfaces should reasonably degrade like Maniphest and Subscribers already do.

  • Build a standalone, pagingated page for the table of contents. This just lists changes in a huge table with links to the corresponding standalone views. This can be a SearchEngine page with some filtering options.
  • Provide a link from the normal table of contents.
  • Above 1K changes, replace the table of contents and changesets with a link to the paginated view.
  • Remove the "large changes" warning prompt in arc, which I suspect no human has ever answered "n" to. This will moot T10852.
  • Remove the "Differential User Guide: Large Changes" article from the documentation.
  • Above 1K changes, probably degrade some other elements like "Other open revisions" and the filetree.
  • Summarize large reviewer lists in the timeline and on the dashboard (and, ideally, in the primary list).
  • Take any other easy optimization opportunities from the profiles in PHI485.

See PHI431. See PHI426. ... I suspect these lead to T731 and T10574, practically.

These still need a bit more planning but are probably mostly like:

  • Add a new ReviewRulesetEngine.
  • Revisions default to the normal ruleset ("At least one human reviewer"); Herald global rules can change them to use a different ruleset ("At least 2 human reviewers", "all human reviewers", etc).
  • DifferentialTransactionEditor->updateReviewStatus(), which decides whether a review should transition to "Accepted" or not, becomes a callback into the new Engine.
  • DifferentialTransactionEditor->expandTransaction(), which includes logic to decide when reviews should be voided, becomes a callback into the new Engine. VoidTransaction probably becomes simpler and the recent logic it gained moves to the Engine instead. This callback should also be able to upgrade reviewers (e.g., accepted -> blocking), force them to resign, etc., not just void them.
  • DifferentialReviewer gets more support for primitive behaviors, including timeouts, to allow custom Herald and Review actions to do zanier things.
  • The new RulesetEngine gets more UI hooks so it can render "Waiting for 7 more humans and at least 1 representative of the Code Style Review Authority Consulate to accept this revision." and customize how the "Accept" aggregation and "Void/downgrade/upgrade" transitions are rendered.

Compatibility:

  • Implementing BuildableInterface now requires implementing newBuildableEngine().
  • Implementing BuildableInterface no longer requires implementing getHarbormasterPublishablePHID(). Publishing indirection is now handled by BuildableEngine.
  • PhabricatorTransactions::TYPE_BUILDABLE no longer exists. Instead, applications should implement application-specific modular transactions to report build status information. DiffusionCommitBuildableTransaction may be useful as a template.

Compatibility:

  • getHasBroadcast(), setHasBroadcast(), and PROPERTY_HAS_BROADCAST are now getShouldBroadcast(), etc.

Compatibility:

  • shouldBroadcast() has been removed; getShouldBroadcast() now means the same thing.

Junk:

  • Existing drafts need to be migrated to setShouldBroadcast(false) explicitly. This wasn't populated by default before, only on transition.

where I plan to explore the [----++ ] element

Here's what this looks like after D19294 on my local test data:

Screen Shot 2018-04-03 at 12.41.55 PM.png (688×411 px, 55 KB)

Briefly:

  • Revisions are rated from 1-7 stars.
  • Scale is logarithmic. You get another star at 20, 50, 150, 375, 1000 and 2500 lines (total lines changed).
  • Each star is either a "-" or a "+"; these are linear by how many lines of each type you have relative to one another.
  • If you have at most one "+", the element is green.
  • If you have 4 or more "+", the element is red.
  • All these numbers and all of the behavior are subject to change.
  • Mail subject lines now say [Request] [++-- ] instead of [Request, 123 lines].

This script can back-populate revision sizes that haven't been updated since D18832:

update-revision-sizes.php
<?php

require_once 'scripts/init/init-script.php';

$table = new DifferentialRevision();
$conn = $table->establishConnection('w');

foreach (new LiskMigrationIterator($table) as $revision) {
  $diff = $revision->loadActiveDiff();
  if (!$diff) {
    continue;
  }

  $row = queryfx_one(
    $conn,
    'SELECT SUM(addLines) A, SUM(delLines) D FROM %T
      WHERE diffID = %d',
    id(new DifferentialChangeset())->getTableName(),
    $diff->getID());

  $properties = $revision->getProperties();

  $properties[DifferentialRevision::PROPERTY_LINES_ADDED] = (int)$row['A'];
  $properties[DifferentialRevision::PROPERTY_LINES_REMOVED] = (int)$row['D'];

  queryfx(
    $conn,
    'UPDATE %T SET properties = %s WHERE id = %d',
    id(new DifferentialRevision())->getTableName(),
    phutil_json_encode($properties),
    $revision->getID());
}

It's a little slow-ish and D18832 landed in December 2017 so I'm not necessarily going to make it an upstream migration.