Properly display number of lines for paste list view
Needs RevisionPublic
Actions

Authored by michaeljs1990 on Aug 28 2016, 6:32 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T11547: Paste query results do not display actual length of listed Pastes (but always "5 lines" if Paste is longer)

Summary

The number of lines in the list view of paste previously displayed the paste was 5 lines long if it was 5 lines or over. This correctly shows the snippet length.

This seems a little hacky to me since it loads in the entire file just to figure out the number of lines in it. From a performance standpoint this seems really bad but in practice it seems to be ok. Some other solutions would be don't display the line numbers at all as I'm not sure how useful they are although it is kinda nice to see. Change the schema to keep track of the number of lines in a file when you create/edit it so you can easily retrieve it from the PhabricatorPaste DAO.

Closes T11547

Test Plan

Looks at UI and saw the correct number of lines.

Diff Detail

Repository

rP Phabricator

Branch

arcpatch-D16465

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 13483
Build 17338: Run Core Tests
Build 17337: arc lint + arc unit

Event Timeline

michaeljs1990 updated this revision to Diff 39606.Aug 28 2016, 6:32 AM

michaeljs1990 retitled this revision from to Properly display number of lines for paste list view.

michaeljs1990 updated this object.

michaeljs1990 edited the test plan for this revision. (Show Details)

michaeljs1990 added a reviewer: Blessed Reviewers.

michaeljs1990 set the repository for this revision to rP Phabricator.

michaeljs1990 added a task: T11547: Paste query results do not display actual length of listed Pastes (but always "5 lines" if Paste is longer).

Herald edited edge metadata. · View Herald TranscriptAug 28 2016, 6:32 AM

Herald added subscribers: yelirekim, epriestley. · View Herald Transcript

Although this diff may be completely useless I did stumble upon something else. Since I was being extra lazy today I created this diff with git diff and made it through the UI. The harbormaster plan requires it to be pushed to the staging repository however that was not done so it's throwing an error as seen here https://secure.phabricator.com/drydock/lease/5342/.

Push to staging

michaeljs1990 updated this object.Aug 28 2016, 3:03 PM

This will fix the issue, but at a potentially substantial performance cost, since it needs to load all the content. This can be slow, particularly if a higher-latency datastore (like Amazon S3) is in use.

A better fix would be to store the length of the paste in the "snippet", which is a little cache of just the first part of the content. I think you can do that like this:

Add a property to PhabricatorPasteSnippet like contentLineCount or originalLineLength or whatever strikes your fancy.
In PhabricatorPasteQuery->buildSnippet() add a setYourNewProperty(...) call on the snippet before returning it, counting the lines in the full raw content.
The Snippet is converted to/from JSON in the Query class. These will need to be aware of the new property. Introducing Snippet->toDicationary() and Snippet::newFromDicationary() to put this logic inside Snippet might be cleaner.
Change the display code to use the length stored on the snippet instead of counting anything.
Tweak getSnippetCacheKey to add a v2 or something so we flush all the existing caches.
Along the way bin/cache purge --purge-general should let you nuke the cache on-demand.