Summary:
We are loading blame on background which confuses me a bit - I'm not sure if it's still loading or it's already loaded and the commit changed lots of lines (so I don't see the author) and it was a long time ago (so I don't see green).
Provide a visual feedback even for very old commits.
I want to point out that I really enjoy this kind of work.
Also, this diff is my masterpiece at least for today.
Test Plan: ?view=blame - verified that the gray changed to a decent shade of green even for very old commits.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5292
Test Plan:
Applied the patch.
Looked at blame and plain blame of SVN and Git file.
Ran the lint saver.
Looked at lint messages list.
/diffusion/lint/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5218
Summary:
I have blame enabled by default and displaying files with long history takes easily over 10 seconds.
Load the blame data by AJAX instead.
This is actually doing more work and the total response time is longer but it's worth it for me as I am interested just in the file contents quite often.
I know you were talking about building blame cache but until we have it...
I'm not sure if the AJAX loading indicator in bottom right corner is enough to inform the user that we are loading it on background.
Test Plan:
?view=highlighted
?view=plainblame
?view=blame
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5244
Summary: Added ttl field to files. Gabage collect files with expired ttl
Test Plan: created file with a ttl. Let garbage collector run
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4987
Summary: Done by searching for `AphrontErrorView` and then `appendChild()`.
Test Plan:
Looked at Commit Detail.
Looked at Revision Detail.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4843
Summary:
We are doing a better job in 1) of D3069#3 and 2) is just dumb.
Let's see if someone notices this change.
Test Plan: /settings/panel/display/
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4789
Test Plan:
Looked at file with lint errors in Diffusion.
I've also tried inline comments in Differential but it failed.
I'll try it again after you land all your diffs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4755
Summary: It's a little bit confusing that you couldn't use %d but kind of expected taken that the number will be formatted so it's not a number anymore.
Test Plan: /paste/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4764
Summary:
- Implements `javelin_tag()`, which is `javelin_render_tag()` on top of `phutil_tag()` instead of `phutil_render_tag()`.
- Manually converts all or almost all of the trivial callsites.
Test Plan:
- Site does not seem any more broken than before.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4639
Summary:
Log of some FB paths takes over 10 seconds.
We query two logs only to get accurate message about lint info which is not that important.
Test Plan: Displayed and clicked on it.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4429
Summary:
upgrades are CrumbsView, HeaderView, PropertyListView, and ActionListView. I had to modify CrumbsView stuff a bit to handle the "advanced" diffusion crumbs.
Quirks fixed include making file tree view show up in diffusion, the page not have extra space when the file tree is hidden, links no longer breaking once you visit files (since without the change the files always got "/" appended and thus 404'd), and a differential quirk where it read "next step:" and that colon is a no no,
Test Plan: played around in diffusion and differential
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T2048, T2178
Differential Revision: https://secure.phabricator.com/D4169
Test Plan: Set `lintCommit` to 'x' and browsed a file.
Reviewers: nh, epriestley
Reviewed By: nh
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3976
Summary: Also remove some columns.
Test Plan: Looked at SVN dir in Diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, vsuba
Maniphest Tasks: T2038
Differential Revision: https://secure.phabricator.com/D3949
Test Plan:
/diffusion/ARC/lint/master/src/, clicked on count link.
/diffusion/ARC/browse/master/src/difference/?lint=XHP9, clicked on file name.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=, verified that all messages are displayed.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=XHP9.
/diffusion/ARC/browse/master/src/difference/ArcanistDiffUtils.php?lint=TXT3, verified that 0 messages are displayed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3929
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary: D3499 introduced some new hotness but broke these less common cases. add slightly updated ui for this. Note we need a 'download' icon for this UI to not be horrendous.
Test Plan: viewed a binary file and an image in diffusion browse view
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Maniphest Tasks: T1997
Differential Revision: https://secure.phabricator.com/D3849
Summary:
It always bothered me that adding/removing blame view takes two clicks.
Showing it like this saves a click for almost all transformations, and I think it feels nicer too.
I'm open to adding a user-setting for this (This form or the drop-down), but figured I'd ask first.
Test Plan:
Use buttons to switch modes - make sure text on button matches action.
Repeate with lines highlighted.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: jungejason, Two9A, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3499
Summary: I didn't notice that D3494 is revert of D3453.
Test Plan: Checked both line and Blame previous links.
Reviewers: epriestley, fdoemges
Reviewed By: fdoemges
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3566
Summary:
We already have this information for all VCSes, there's no point in parsing it again.
We currently support it only for Git and I remember there were some bugs in it.
It should also be faster.
Test Plan: Blamed previous revision of a file which was moved in SVN.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3564
Test Plan:
# Set default view to non-blame.
# Visit URL with '?view=blame'.
# Click //Skip Past This Commit//.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3494
Summary:
Including view in permalink seemed like the correct solution to me.
Instead, I delete it all the time when copying somewhere else.
Test Plan:
Clicked on the link.
Changed view on permalink.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3453
Summary: I've done D3432 in the hope that it will fix also this...
Test Plan: Blamed sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3433
Summary: Nobody knows that it's possible to highlight more lines because there's no interface for it.
Test Plan:
Highlighted:
- single line
- top to bottom
- bottom to top
- inside to outside
Reviewers: Korvin, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D3150
Test Plan: Jumped on correct line in SVN, Git and HG repos.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3084
Summary: Some people don't like these, so they should be able to turn them off.
Test Plan:
Toggled the setting on and off; loaded a page in diffusion and differential
that should have symbol cross-references, and saw that they weren't linked
when I had the setting disabled. I also checked that the symbols are still
linked when the setting hasn't been touched.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3069
Summary: This returns a list of PHIDs, not objects which need to be pulled.
Test Plan:
Repro'd fatal locally, verified it was fixed.
Repro steps are:
- Create an arc project associated with a repository, with indexed language(s) and subprojects.
- View a file in that repository.
Reviewers: nh, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3038
Summary:
We do this in differential. To do this in diffusion, we need to know the
arcanist project (which I do by loading all possible projects for the
repository) and the language.
Test Plan:
load a php file in diffusion to see crossreferences; load a text file and
check darkconsole that it didn't try to crossreference.
Reviewers: epriestley, vrana, jungejason
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3020
Summary: I spend most time in software development by being lazy.
Test Plan: Changed view, reloaded page, viewed the file without sending the form.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3012
Summary: Blame can be slow.
Test Plan:
Viewed a file with no preference, saw blame.
Changed view, saw it.
Viewed a file, saw the changed view.
Viewed a file as raw document.
Reviewers: Two9A, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin, btrahan
Maniphest Tasks: T1278
Differential Revision: https://secure.phabricator.com/D3000
Summary:
Still not 100% sure what the repro case here is, but we do something similar on line 438 above. One case could be an SVN repo with a subpath specified, I think.
In any case, don't fatal if we're missing the commit.
Test Plan: Loaded some browse views, although I don't have a repro.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2959
Summary:
We have a bit more copy-paste than we need, consolidate a bit.
(Also switch Mercurial to download git diffs, which it handles well; we use them in "arc patch".)
Test Plan:
- Downloaded a raw diff from Differential.
- Downloaded a raw change from Diffusion.
- Downloaded a raw file from Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2942
Summary: Currently, Diffusion supports highlighting of line ranges passed in the URI. It would be helpful to be able to highlight multiple line ranges.
Test Plan: Accessed directly via URL in my sandbox. Seems to work. I'm not sure what other components use this functionality, but this change should be backwards compatible.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D2921
Test Plan:
Displayed repository.
Displayed repository history.
Wondered that we actually have bunch of commits without a revision.
Displayed blame.
Didn't display merge commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2840
Summary: I need this much more than commit but it can be useful too.
Test Plan: Displayed blame, clicked on the link.
Reviewers: epriestley
Reviewed By: epriestley
CC: john, aran, Korvin, phunt
Differential Revision: https://secure.phabricator.com/D2820
Summary: As per title: when browsing files in Diffusion, set "Highlighted with blame" as the default view instead of falling back to "Highlighted".
Test Plan: Tested view with a local Diffusion setup, defaults correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1278
Differential Revision: https://secure.phabricator.com/D2669
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
2012-06-01 12:32:44 -07:00
Renamed from src/applications/diffusion/controller/file/DiffusionBrowseFileController.php (Browse further)
