Denormalize added and removed line counts for the current diff onto revisions
ClosedPublic
Actions

Authored by epriestley on Dec 13 2017, 3:40 PM.

Details

Reviewers

amckinley

Commits

rPa1ad184ddd8c: Denormalize added and removed line counts for the current diff onto revisions

Summary

See PHI230. Currently, we denormalize raw line counts onto diffs and revisions, but not added/removed line counts.

I'd like to try a [---+ ] sort of size hint element (see D16322 for more) as a general approach to conveying size information at a glance and see how it feels, since I think the raw size number isn't very scannable/useful and it may be a significant improvement to hint about how much of a change is throwing stuff out vs adding new stuff.

This just makes the data available without any subquerying and doesn't actually change the UI.

Test Plan

Created a revision, saw detailed change information populate in the database.

mysql> select * from differential_revision where id = 292\G
*************************** 1. row ***************************
              id: 292
           title: WIP
   originalTitle: WIP
            phid: PHID-DREV-ux3cxptibn3l5pxsug3z
          status: draft
         summary: asdf
        testPlan: asdf
      authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
lastReviewerPHID: NULL
       lineCount: 41
     dateCreated: 1513179418
    dateModified: 1513179418
        attached: []
         mailKey: h4mn6perdio47o4beomyvu75zezwvredx3mbrlgz
      branchName: NULL
      viewPolicy: users
      editPolicy: users
  repositoryPHID: PHID-REPO-wif5lutk5gn3y6ursk4p
      properties: {"lines.added":40,"lines.removed":1}
  activeDiffPHID: PHID-DIFF-ixjphpunpkenqgukpmce
1 row in set (0.00 sec)

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Dec 13 2017, 3:40 PM

Harbormaster completed remote builds in B18951: Diff 45176.Dec 13 2017, 3:42 PM

epriestley requested review of this revision.Dec 13 2017, 3:42 PM

Are we ever going to deploy a migration to populate this info for old revisions? Or just not show the change hint?

This revision is now accepted and ready to land.Dec 14 2017, 7:12 PM

Probably just not show the change hint unless I get it written Real Soon and Everyone Loves It. Migrating revisions is a bit of a pain for larger installs (since it's like 10+ minutes of migration downtime), and the relevance of the change hint is probably pretty short-lived (i.e., doesn't matter if older accepted/landed/abandoned revisions show it or not). We could also say "here's an optional migration script if you want the UI thing retroactively" but don't want to force everyone through a migration.

(We don't currently have much support for ad-hoc, online migrations -- there are "Manual Activities" in T11932 but they're pretty heavy.)

Closed by commit rPa1ad184ddd8c: Denormalize added and removed line counts for the current diff onto revisions (authored by epriestley). · Explain WhyDec 18 2017, 5:18 PM

This revision was automatically updated to reflect the committed changes.

epriestley mentioned this in T13110: Plans: Hefty Differential revisions, draft state transitions, and exotic interactions.Apr 3 2018, 8:08 PM