Page MenuHomePhabricator

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

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

Details

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

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.)

This revision was automatically updated to reflect the committed changes.