Summary:
I like this abstraction better.
Result of `phutil_implode_html()` may be also used as a param of `hsprintf()`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4904
Summary:
This resolves lots of double escaping.
We changed most of `phutil_render_tag(, , $s)` to `phutil_tag(, , $s)` which means that `$s` is now auto-escaped.
Also `pht()` auto escapes if it gets `PhutilSafeHTML`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4889
Summary: Show a more specific exception when the local clone cannot be read because of permission issues.
Test Plan: Create a repository in an unreadable location and check for the right exception.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2368
Differential Revision: https://secure.phabricator.com/D4868
Summary:
Lots of killed `phutil_escape_html()`.
Done by searching for `AphrontTableView` and then `$rows` (usually) backwards.
Test Plan:
Looked at homepage.
echo id(new AphrontTableView(array(array('<'))))->render();
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4884
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: do so via event engine. note different order now...
Test Plan: toggled "show beta applications" to off and noted that Conpherence disappeared. Otherwise noted that links showed.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2424
Differential Revision: https://secure.phabricator.com/D4708
Summary:
See D4812.
- This preference disables the file tree completely.
- It defaults off, so users who want it will have to go turn it on.
- Maybe slightly cleaner would be doing this if the tree was hidden and then ajaxing it in if you press "F", but that's complicated and I don't want to bother.
- Generally, I think this element is useful to something like 5% of users and not useful to 95%.
Test Plan: Enabled and disabled file tree. Looked at commits and revisions; verified they reflected the setting correctly.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4813
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: @nh, does this do something reasonable on merges? We can refine the behavior ('click to show all 92 million files'), but I want to make sure it's at least feasible before we pursue it.
Test Plan: Set 1000 to "3" and looked at a change which touched 6 files.
Reviewers: nh, vrana, zjwsoft
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D4730
Summary: I'm going to stop showing changes for commits which touch 30,000 files, but still want to show the comment panel.
Test Plan: Looked at commits, saw comments. Mashed "Z"; haunted mode worked.
Reviewers: nh, vrana
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D4729
Summary: Converts various callsites from render_tag variants to tag variants.
Test Plan: See inlines.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4689
Summary: Proper fix is to do some layout work in Diffusion. Short of that, make this escape properly.
Test Plan: Viewed various crumbs, no more overescaping for non-diffusion crumbs.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4641
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:
Created with spatch:
lang=diff
- phutil_render_tag
+ phutil_tag
(X, Y, '...')
Then searched for `&` and `<` in the output and replaced them.
Test Plan: Loaded homepage.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4503
Summary:
In D4359 I fixed an error with 'lint' in SVN repositories, but created an error with the 'lint' column in Javascript. Specifically, when we load the column information over Ajax, we now always include a 'lint' key, even if there is no lint column.
Instead, access the 'lint' property conditionally (so SVN works) but don't include the key if there's no data (so Javascript works).
Test Plan: Loaded SVN, non-SVN non-lint, non-SVN+lint repositories. Everything appeared to work correctly.
Reviewers: asherkin, codeblock
Reviewed By: codeblock
CC: aran
Differential Revision: https://secure.phabricator.com/D4578
Summary: Fixes T2243. We recently added the FileTreeView to Diffusion commits. However, if the page doesn't have any changesets (e.g., it has an error message instead, like "this commit hasn't imported yet"), we fail to build a file tree. In this case, don't try to build one.
Test Plan: Looked at not-imported and imported commits in Diffusion, saw proper rendering/crumbs and no exceptions.
Reviewers: btrahan, chad, vrana
Reviewed By: chad
CC: aran
Maniphest Tasks: T2243
Differential Revision: https://secure.phabricator.com/D4562
Summary: Fixes T2339
Test Plan: Close Audit button does not appear if audit.can-author-close-audit option is disabled
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2339
Differential Revision: https://secure.phabricator.com/D4525
Summary: Ref T2298. This seems like the least complicated reasonable order to implement.
Test Plan: Looked at repositories, saw them ordered by name.
Reviewers: vrana, btrahan, brennantaylor
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2298
Differential Revision: https://secure.phabricator.com/D4395
Summary: Implement Diffusion MetaMTA options. Also make the fake '{{config.option}}' rule work, and use Remarkup to render summaries as well as descriptions.
Test Plan: Looked at Diffusion rules, edited some, looked at setup issues, verified '{{config.option}}' linked to the right option.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4466
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: Still working through basic re-design. Adds the ability to re-use panel view without the background.
Test Plan: Viewed Diffusion and Differential in Chrome, FF, Safari.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4430
Summary: If the repository has no lint information, we don't set a 'lint' key, but try to access it unconditionally later on.
Test Plan: Looked at an SVN repository browse view, saw no errors.
Reviewers: vrana, btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2281
Differential Revision: https://secure.phabricator.com/D4359
Summary:
If you go to `/rXnnnn` in Git, we expand the hash. If you go to `/rXnnnn` in Mercurial, we give you a confusing error message.
Reconcile Mercurial behavior with Git. Fixes T2265.
Test Plan: Viewed partial hash, full hash commit in Diffusion. Viewed very short hash, got reasonable behaviors.
Reviewers: btrahan, tido
Reviewed By: tido
CC: aran
Maniphest Tasks: T2265
Differential Revision: https://secure.phabricator.com/D4330
Summary:
The search indexing API has several problems right now:
- Always runs in-process.
- It would be nice to push this into the task queue for performance. However, the API currently passses an object all the way through (and some indexers depend on preloaded object attributes), so it can't be dumped into the task queue at any stage since we can't serialize it.
- Being able to use the task queue will also make rebuilding indexes faster.
- Instead, make the API phid-oriented.
- No uniform indexing API.
- Each "Editor" currently calls SomeCustomIndexer::indexThing(). This won't work with AbstractTransactions. The API is also just weird.
- Instead, provide a uniform API.
- No uniform CLI.
- We have `scripts/search/reindex_everything.php`, but it doesn't actually index everything. Each new document type needs to be separately added to it, leading to stuff like D3839. Third-party applications can't provide indexers.
- Instead, let indexers expose documents for indexing.
- Not application-oriented.
- All the indexers live in search/ right now, which isn't the right organization in an application-orietned view of the world.
- Instead, move indexers to applications and load them with SymbolLoader.
Test Plan:
- `bin/search index`
- Indexed one revision, one task.
- Indexed `--type TASK`, `--type DREV`, etc., for all types.
- Indexed `--all`.
- Added the word "saboteur" to a revision, task, wiki page, and question and then searched for it.
- Creating users is a pain; searched for a user after indexing.
- Creating commits is a pain; searched for a commit after indexing.
- Mocks aren't currently loadable in the result view, so their indexing is moot.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T1991, T2104
Differential Revision: https://secure.phabricator.com/D4261
Summary: This is used in every other view.
Test Plan: Browsed around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4248
Summary: most awesome is that differential-primary-pane no longer has a place in diffusion. less awesome is fixing the zebra striping on differential "Local Commits" view and making the font size of one of the table headers match the others.
Test Plan: looks good!
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4177
Summary: we don't always have a diff so instead set an explicit title in the controller.
Test Plan: no more fatals. grepped carefully for every call site and tested them all
Summary:
- Remove "Diffusion" crumb (redundant with application icon)
- Put "All Repositories" in its place on the landing page.
- Render the repository crumb as "rX" instead of "X Repository".
Test Plan: Looked at various Diffusion pages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4173
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
Summary:
Issues here:
- Need an application-sized "eye", or a "home" icon for "Phabricator Home".
- Some of the "apps_lb_2x" sliced images are the "_dark_" versions, not the light versions.
- If you slice an application-sized "logout" (power off) icon and application-sized "help" (questionmark in circle) icon I can replace the current menu icons and nearly get rid of "autosprite".
- To replace the icons on /applications/, the non-retina size is "4x", so we'd need "8x" for retina. Alternatively I can reduce the icon sizes by 50%.
- The "Help", "Settings" and "Logout" items currently have a "glowing" hover state, which needs a variant (or we can drop it).
- The /applications/ icons have a white hover state (or we can drop it).
- The 1x application (14x14) icons aren't used anywhere right now, should they be? Maybe in the feed in the future, etc?
- The "apps-2x" and "apps-large" sheets are the same image, but getting them to actually use the same file is a bit tricky, so I just left them separate for now.
Test Plan:
{F26698}
{F26699}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4108
Summary:
AphrontSideNavView is an old class which required you to do a lot of work; it was obsoleted by AphrontSideNavFilterView. Remove all direct callsites so I can clean it up.
This is a precursor to letting me render a filter menu as a dropdown menu for T1960.
Test Plan:
Examined each interface for correct filter construction and selection:
- Browsed Diffusion
- Browsed Differential
- Browsed Files
- Browsed Slowvote
- Browsed Phriction
- Browsed repo edit interface
Grepped for `AphrontSideNavView`. The only remaining instances are in `AphrontSideNavView` itself and `AphrontSideNavFilterView` (which currently uses it).
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4034
Summary:
I want to add search per owner, this is a prerequisity for it.
There's no link to this page yet, I didn't find a good place for it.
Test Plan: Displayed it, clicked around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, scottmac
Maniphest Tasks: T2038
Differential Revision: https://secure.phabricator.com/D4067
Summary: make sure we only print the fancy tool tip thing if we have all the data we need. (fixes T2136). additionally, default $branches to array() to prevent errors from reset on null.
Test Plan: made a checkin for a mercurial repo with user "foo@bar" with no branch. verified name showed up in all views. also noted on commit detail view there was no more error about reset() on null for $branches.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2136
Differential Revision: https://secure.phabricator.com/D4065
Summary:
Some users like monospaced textareas and others don't.
This introduces an option to set this as a user preference.
Test Plan: Enabled and saw monospaced textareas, disabled and saw non-monospaced textareas.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2114
Differential Revision: https://secure.phabricator.com/D4037
Summary: D3533 changed the path for files at root from, e.g., "README" to "/README", which fatals when we try to `git cat-file` it.
Test Plan:
This no longer happens:
{F24874}
Viewed a directory in browse without a trailing slash in the URL.
Reviewers: vrana, nh
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4016
Summary:
If you try to load a directory in diffusion in a git repo that has a readme
file and you don't include a trailing slash in the path, you get an error
because we don't assemble the full paths of files correctly. This fixes that.
Test Plan: load such a directory in diffusion and see no fatal
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3533
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: we do this by passing the "seenOnBranches" commit data detail through the stack
Test Plan: browse in diffusion link worked for non-master checkins under git
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1949
Differential Revision: https://secure.phabricator.com/D3853
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: Followup to D3804. Makes Diffusion main comments (not just inlines) render properly with the modern markup pipeline.
Test Plan: Created previews and inline previews. Edited inlines. Saved comment, viewed comment. Verified caches were read and written using "Services" tab.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3805
Summary:
See T1963 for discussion of the Facebook-specific hack.
Differential currently uses a one-stage cache (render -> postprocess -> save in cache) rather than the two-stage cache (render -> save in cache -> postprocess) offered by `PhabricatorMarkupInteface`. This breaks Differential comments coming out of cache for the lightbox, and makes various other things suboptimal (status of handles like @mentions and embeds are not displayed accurately).
Instead, use the modern stuff.
Test Plan:
- Created preview comments and inlines in Differential.
- Edited a Differential inline.
- Submitted main and inline Differential comments.
- Viewed and edited Differential summary and test plan.
- Created preview comments and inlines in Diffusion.
- Submitted comments and inlines in Diffusion.
- Verified Differential now loads and saves to the generalized markup cache (Diffusion is close, but main comments still hold a single-stage cache).
- Verified old Differential comments work correctly with the lightbox.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1963
Differential Revision: https://secure.phabricator.com/D3804
Summary: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.
Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, vrana
Maniphest Tasks: T1676
Differential Revision: https://secure.phabricator.com/D3645
Summary:
When directories are added (e.g., on the `hg` initial commit, "/" is added) we don't render any diff for them but try to link to it in the table of contents.
Possibly we should render a diff saying "this directory was added", but stop it from complaining for now.
Test Plan: Looked at several hg and git commits which add directories to verify this gives us generally sensible behvior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3663
Summary: no parents - no problem - just diff that ish against "null"
Test Plan: initial commits were viewable in my test repos
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1689
Differential Revision: https://secure.phabricator.com/D3660
Summary: So they're maybe a little easier to deal with? I'm going to take this formally to "plz @chad plz help" land.
Test Plan: {F20329}
Reviewers: btrahan, vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3609
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
Summary: ...also makes Maniphest Task Edit Controller use this when its not appropriate to upsell email.
Test Plan: played around with each tool and verified the Remarkup reference was present
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1756
Differential Revision: https://secure.phabricator.com/D3468
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:
This is the last Paste UI element that doesn't work properly on tablets/phones. Make it flexible.
Also add empty states to Paste.
Test Plan: Viewed various errors, and `/uiexample/errors/`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3429
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 replaced all `id(new PhabricatorObjectHandleData(...))->loadHandles()` by `$this->loadViewerHandles(...)`.
Lint caught one usage in a static method.
Test Plan: Displayed revision with sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3432
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:
Adds a flexible navigation menu to diffs that shows you your current position in the diff.
Anticipating some "this is the best thing ever" and some "this is the wosrt thing ever" on this, but let's see how much pushback we get? It seems pretty good to me.
Test Plan: Will attach screenshots.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1633, T1591
Differential Revision: https://secure.phabricator.com/D3355
Summary:
Rendering method shouldn't load data.
The view probably shouldn't load data either because it is a job for component (object that both loads data and displays them) but we don't have that concept in Phabricator.
This at least improves the architecture a little bit.
Test Plan: /differential/
Reviewers: epriestley
Reviewed By: epriestley
CC: alanh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D3325
Summary: See T1665. If you have a directory named 'readme', we try to read it as a README.
Test Plan: Created a directory named 'readme', hit a similar fatal to the one in T1665, applied this patch, everything worked great.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1665
Differential Revision: https://secure.phabricator.com/D3312
Summary:
It's kind of nice to type `s explode` in jump nav and have it
just work. This involves weakening a bunch of the request parameter
checks, but the only real extra assumption is that language defaults to
PHP... which is not that big of a stretch, and it's not like we know
about any other languages' documentation.
It'd be better to have builtins be more first-class and less awkward
hack, but that seems hard.
Test Plan: Search for symbols.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3302
Summary:
We will need to process them.
Maybe it will fit better in Repository application but we don't have it yet.
Test Plan:
$ ./fact cursors
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, royw
Differential Revision: https://secure.phabricator.com/D3287
Summary:
- Add getHelpURI() to PhabricatorApplication for application user guides.
- Add a new "help" icon menu item and skeletal Diviner application.
- Move help tabs to Applications where they exist, document the other ones that don't exist yet.
- Grep for all tab-related stuff and delete it.
Test Plan: Clicked "help" for some apps. Clicked around randomly in a bunch of other apps.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3267
Summary:
If a repository is configured with a custom encoding, it wasn't respected by DiffusionGitFileContentQuery making all views with
non-UTF8 characters fail. Check if we have a custom encoding and encode if any it set.
NOTE: This only works for Git repositories.
Test Plan: Browsed a repository with custom encoding before and after this patch.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T452
Differential Revision: https://secure.phabricator.com/D3251
Summary:
Add a new left-side application menu. This menu shows which application you're in and provides a quick way to get to other applications.
On desktops, menus are always shown but the app menu can be collapsed to be very small.
On tablets, navigation buttons allow you to choose between the menus and the content.
On phones, navigation buttons allow you to choose between the app menu, the local menu, and the content.
This needs some code and UI cleanup, but has no effect yet so I think it's okay to land as-is, I'll clean it up a bit as I start integrating it. I want to play around with it a bit and see if it's good/useful or horrible anyway.
Test Plan: Will include screenshots.
Reviewers: vrana, btrahan, chad
Reviewed By: btrahan
CC: aran, alanh
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3223
Summary: rather than showing an erroneous "we still parsing" message.
Test Plan: for each version control system, typed in a garbage URL and got a 4oh4. (note this actually fails for SVN -- see comment about how my code fails atm -- and DiffusionGitRequest seems to pick off this error in advance and returns an AphrontUsageException.)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1624
Differential Revision: https://secure.phabricator.com/D3201
Summary:
Put the function in the base class so all the Diffusion views
can use it. Also use shinier tooltips.
Test Plan: Browse Diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3206
Summary:
- Commit detail view
- List of projects
- "edit" action which takes the user to a simple form where they can only add / remove projects.
- Integrated the project relationship into the commit search indexer
- fixed a bug from D790; it seems you must select the column if you're going to join against it later. Without this change searching for author or projectfails 100% for me.
Test Plan: added and removed projects. verified appropriate projects showed up in detail and edit view. searched for commits by project and found the ones I was supposed to...!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1614
Differential Revision: https://secure.phabricator.com/D3189
Summary:
Behave like Differential.
Also save one path ID query.
Test Plan: Displayed very large commit, clicked in ToC, clicked on Load.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3176
Summary:
- import_project_symbols supports an optional extra field, which is the
context of the symbol.
- Symbol query can take a symbol argument, either as a parameter or a
URL component (so you can now jump nav to `s Zerg/rush`, for
example).
- Conduit method not yet updated. Will do that later.
NOTE: Not providing a context is distinct from providing an empty
context, because an empty context stands for top-level context, i.e.
functions and classes for PHP. It will not find class methods, etc. It
is possible that we should use some weird token that could not normally
be a context name to stand in for empty context.
Test Plan: Do a bunch of symbol searches.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3148
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
Summary:
When we match an application route, select it as the current application and store it on the controller.
Move routes for the major applications into their PhabricatorApplication classes so this works properly.
Test Plan: Added a var_dump() and made sure we picked the right app for all these applications.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3144
Summary:
- Adds a new "Applications" application.
- Builds an application list via application config instead of via hard-coding, so we can move toward better concepts of installing/uninstalling applications, etc.
- Applications indicate that they need attention with notice counts and brief status messages rathern than 50 giant tables of all sorts of app data.
I want to try replacing the home screen with this screen, pretty much. Not sure if this is totally crazy or not. What does everyone else think?
Test Plan: Will add screenshots.
Reviewers: btrahan, chad, vrana, alanh
Reviewed By: vrana
CC: aran, davidreuss, champo
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3129
Summary:
If the user has an editor configured, an Edit link appears next
to the History link.
Somewhat suboptimally, the column is still there if there are no edit
links, it's just empty. I don't know if it matters...
Test Plan: Load Diffusion pages while changing editor setting in preferences.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, avivey
Maniphest Tasks: T1558
Differential Revision: https://secure.phabricator.com/D3124
Summary:
The tables are currently kind of wide, and the emails for
non-recognized users seem like the least useful parts, so I put them in
tooltips so they're only visible if you want them. On the other hand, it
means you can only view one at a time, and if you're on mobile you can't
see them at all. Overall, not sure whether this is a good idea.
Test Plan:
Load Diffusion pages with some recognized and some
unrecognized users. Hover over the latter and see that emails appear.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3123
Summary:
- Include symbols in main typeahead results.
- Simplify the symbol query a bit and extend PhabricatorOffsetPagedQuery. There was some stuff around language ranking that I got rid of, I think the theory there was that mapping file extensions to languages might not work in general but I think it works fine in practice, and we have more config stuff now around guessing languages and getting the mappings right.
- Make it easier to debug the typeahead by showing the results in page format for non-ajax requests.
- When we have too many results, show only the top few of each type.
Depends on D3116, D3117
Test Plan: Used typeahead, got symbols in results. Hit endpoint with non-ajax, got useful debug view.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3118
Summary:
Each query takes over 2 seconds in FBCODE.
I didn't find a way how to speed them up.
There's also no easy way how to parallelize them at least.
So AJAX is the last instance.
Test Plan: Loaded commit with one branch and no tag.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3112
Summary: helping noobs help themselves
Test Plan: set $rows = array() and verified the txt. also threw a false && for my isAdmin conditional to check the other txt
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1086, T1360
Differential Revision: https://secure.phabricator.com/D3100
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: I will need this for tracking line number in Blame previous revision.
Test Plan:
$ hg diff --rev 0:1
$ svn diff -r 64382:64383 README
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3083
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: created a PhabricatorInlineCommentPreviewController so controllers in Diffusion and Differential respectively just have to handle the URI mapping and data loading like good little controllers.
Test Plan:
left inline comments on commits, deleted inline commits, submitted inline comments -- all worked well
did the same on some diffs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1176
Differential Revision: https://secure.phabricator.com/D3034
Summary: ...basically by pounding the DOM a bit to be a little closer to differential. I also make the "add comment" UI show up if and only if the commit is rendering properly.
Test Plan: prezzed "Z" on diffusion and noted it was like differential now
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1177
Differential Revision: https://secure.phabricator.com/D3027
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:
This doesn't indicate which path is part of which package - I think it would be too heavy.
It just highlights the paths in a similar way as audits are highlighted.
Maybe we can use different colors for highlighting different packages and use them also in paths. We can mix the colors if one path is part of more packages :-).
Test Plan:
Viewed commit with 9 files and 4 packages where I am responsible only for one of them.
Verified that the only file in my package is highlighted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, haugen
Maniphest Tasks: T1226
Differential Revision: https://secure.phabricator.com/D2982
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: Paths autocompleter sometimes omits `/`.
Test Plan:
# Go to https://secure.phabricator.com/owners/new/.
# Write `/specs/h` to path.
# Delete the second slash resulting in `/specsh`.
# Don't find `/specshistorytest.html` in autocomplete.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2981
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:
- Old versions of Mercurial give different output for `hg log -- ''` and `hg log`. Just use `hg log`.
- Branch names with spaces can't be specified in `--rev`. I talked with hstuart in #mercurial and apparently am not crazy.
Test Plan:
- Viewed history of a repository.
- Viewed history of a file.
- Viewed branch `m m m m m 2:ffffffffffff (inactive)`
- Learned that you checkout this branch with `hg checkout ':m m m m m 2:ffffffffffff (inactive)'`
Reviewers: dschleimer, btrahan
Reviewed By: dschleimer
CC: aran
Maniphest Tasks: T1268
Differential Revision: https://secure.phabricator.com/D2950
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
Summary:
See T1448. If this file isn't present, just move on instead of failing, since it's a (sort of) legitimate repository state.
Also fix some silliness a little later that got introduced in refactoring, I think.
Test Plan: Added an external to my test repo and removed ".gitmodules". Verified that the directory is now viewable after this patch.
Reviewers: btrahan, davidreuss, jungejason
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1448
Differential Revision: https://secure.phabricator.com/D2922
Summary:
binary_safe_diff is needed in arcanist too. Moved it over to
arcanist. See D2915.
Test Plan: diffusion page rendered correctly on binary file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2916
Summary:
Oh man, after the explode/map, the output when no references is actually
array(1) {
[0]=> string(0) ""
}
.. making the falsey check fail to work as expected.
Test Plan: same as D2892, but with a little more scrutiny :p
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2893
Summary:
When there are no other references (and there will be none ususally,
if D2891 is accepted), we would show "References:" with empty contents.
Avoid that by testing for empty output after applying stupid filtering.
Test Plan: Looked in a standard repository with no special refs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2892
Summary: They just beg to be clicked.
Test Plan: clicked furiously with my mouse until i got tired - went expected places.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2889
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:
Diffusion page is sharing the keyboard shortcuts code with
Differential page. But since the toc (Changes) panel doesn't have id
'differential-review-toc', the 'jumping to toc' doesn't work. The fix is
to add the ID. I don't like adding 'Differential' to the Diffusion page.
Later we should refactor the code to extract the shared components out of Differential.
Test Plan:
verified that 't' worked on the diffusion commit page.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: hwang, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2500
Summary:
It's possible to have a tag with no tagger (or git used to allow this),
so some tags (like 26791a8bcf0e6d33f43aef7682bdb555236d56de in the linux
kernel) were causing trouble.
Test Plan:
opened diffusion to a page where I was previously getting a red box complaining
about being unable to parse the output of git for-each-ref.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, vrana
Differential Revision: https://secure.phabricator.com/D2711
Summary: These columns holds the same value in most cases which irritates me.
Test Plan: Displayed history with same author and committer, emulated different committer.
Reviewers: hsb, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2671
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
Summary:
This adds a link to [Closed] e-mail if it detects some changes.
It compares added and removed lines with 3 lines context.
The subtle form of informing is permissive to false negatives and positives.
I have an e-mail filter for [Closed] e-mails so I wouldn't personally notice this change - we should probably promote this feature a little bit.
Test Plan:
Reparse a diff with a change after last update.
Reparse a diff without a change after last update.
Reviewers: jungejason, epriestley
Reviewed By: jungejason
CC: aran, Koolvin, btrahan
Maniphest Tasks: T201
Differential Revision: https://secure.phabricator.com/D2540
Summary:
Git and hg (supposedly..) differentiate between an author (who wrote the patch)
and a committer (who applied the patch).
This patch allows Phabricator to note when a patch is committed
by someone other than the Author.
Test Plan:
Created 2 accounts,
- U (Account with a PHID)
- U' (Account without a PHID)
and had them create and commit commits
testing if their username/real name would be displayed correctly in Diffusion,
- BrowserTable
- HistoryTable
- Code revision
Teztz,
A(uthor)/C(ommitter)
If it's A/A then Author committed
UL = User link (<a href="/p/username">username</a>)
UN = User name ("Firstname Lastname")
Tezt | Expected in table | Got
-------------------------------------------
A/A | UL/UL | UL/UL
A'/C | UN/UL | UN/UL
A/C' | UL/UN | UL/UN
A'/C' | UN/UN | UN/UN
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T688
Differential Revision: https://secure.phabricator.com/D2541
Summary:
Mercurial renamed "--only-branch" to "--branch" about two years ago. "-b" exists in both versions.
http://www.selenic.com/pipermail/mercurial-devel/2010-April/020469.html
We have a few other cases where we use features that exist only in recent Mercurial (notably, 'ancestors' in log) but we can work around this one easily.
Test Plan: Looked at a Mercurial repo in Diffusion, verified that "log -b" commands issued and that the output was correct.
Reviewers: btrahan, Makinde, ipalaus
Reviewed By: ipalaus
CC: aran
Maniphest Tasks: T1264
Differential Revision: https://secure.phabricator.com/D2533
Summary: as title
Test Plan: no
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2526
Summary:
we were parsing the git log output slightly incorrectly and over-exploding on spaces. we also needed to escape the path %20 stuff`.
Not sure if there's something fancy to do given folks should reparse their repos if they are impacted by this issue.
Test Plan:
made a directory with spaces and some dummy revisions. observed diffs wouldn't load and links broken.
with patch, ran scripts/reparse.php for pertinent revisions and diffs loaded and links weren't broken.
Reviewers: floatinglomas, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1252
Differential Revision: https://secure.phabricator.com/D2510
Summary: If you have an empty value saved in the "default branch" field, we default to empty string (or null, or whatever) instead of the correct default.
Test Plan:
- Looked at a Git repo with an empty default branch, got a default to "master" instead of an error.
- Looked at a Mercurial repo with an empty default branch, got a default to "default" instead of an error.
Reviewers: btrahan, csilvers
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1237
Differential Revision: https://secure.phabricator.com/D2512
Summary: Prelminary, I want to test this a bit more when I'm better rested. Provide a "bulk" import mode for Mercurial so we can do initial discovery more quickly.
Test Plan: Imported the Mercurial repository in 2m45s, blocked on MySQL I/O rather than Mercurial I/O.
Reviewers: csilvers, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2457
Summary:
- Support offset/limit as added by D2442, for Mercurial.
- We just list everything and slice, no clear way to do better and this isn't a major perf issue.
- No clear way to easily get by-update sorting, we can implement this by looking up revs if someone asks.
- Bury some of the Git implementation details inside the Git query.
Test Plan:
- Looked at a Git repo, clicked "View All Branches".
- Looked at a Hg repo, clicked "View All Branches".
- Limited page size to 1, made sure it limited properly.
Reviewers: aurelijus, btrahan, vrana
Reviewed By: aurelijus
CC: aran
Differential Revision: https://secure.phabricator.com/D2453
Summary:
Sorting by last commit date
Branch view limit to 25 branches
All branches table page with pagination on Git
Test Plan:
* Check repository view for expected behavior on branch table view
* Check all branches page & test pagination
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1200
Differential Revision: https://secure.phabricator.com/D2442
Summary:
- When you have an un-cloned repository, we currently throw random-looking Git/Hg exception. Instead, throw a useful error.
- When you have a cloned but undiscovered repository, we show no commits. This is crazy confusing. Instead, show commits as "importing...".
- Fix some warnings and errors for empty path table cases, etc.
Test Plan:
- Wiped database.
- Added Mercurial repo without running daemons. Viewed in Diffusion, got a good exception.
- Pulled Mercurial repo without discovering it. Got "Importing...".
- Discovered Mercurial repo without parsing it. Got "Importing..." plus date information.
- Parsed Mercurial repo, got everything working properly.
- Added Git repo without running daemons, did all the stuff above, same results.
- This doesn't improve SVN much but that's a trickier case since we don't actually make SVN calls and rely only on the parse state.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T776
Differential Revision: https://secure.phabricator.com/D2439
Summary: Add history column & history link per branch on branch table view, also later add some more features like last commit date, etc.
Test Plan: Checked if History column is in browser table view & history links are properly linked.
Reviewers: epriestley
Reviewed By: epriestley
CC: davidreuss, aran, Koolvin
Maniphest Tasks: T1201, T1202, T1200
Differential Revision: https://secure.phabricator.com/D2432
Summary:
See D2418. This merges the commit discovery daemon into the same single daemon, and applies all the same rules to it.
There are relatively few implementation changes, but a few things did change:
- I simplified/improved Mercurial importing, by finding full branch tip hashes with "--debug branches" and using "parents --template {node}" so we don't need to do separate "--debug id" calls.
- Added a new "--not" flag to exclude repositories, since I switched to real arg parsing anyway.
- I removed a web UI notification that you need to restart the daemons, this is no longer true.
- I added a web UI notification that no pull daemon is running on the machine.
NOTE: @makinde, this doesn't change anything from your perspective, but it something breaks this is the likely cause.
This implicitly resolves T792, because discovery no longer runs before pulling.
Test Plan:
- Swapped databases to a fresh install.
- Ran "pulllocal" in debug mode. Verified it correctly does nothing (fixed a minor issue with min() on empty array).
- Added an SVN repository. Verified it cloned and discovered correctly.
- Added a Mercurial repository. Verified it cloned and discovered correctly.
- Added a Git repository. Verified it cloned and discovered correctly.
- Ran with arguments to verify behaviors: "--not MTEST --not STEST", "P --no-discovery", "P".
Reviewers: btrahan, csilvers, Makinde
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T792
Differential Revision: https://secure.phabricator.com/D2430
Summary: The PHID list is a list, not a map -- I must have broken this in refactoring or something, since everything else works fine. See D2013.
Test Plan: Viewed a resignable revision, saw "Resign" (new after this commit), resignd.
Reviewers: 20after4, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2417
Summary: This allows writing inline comments and reduces different behavior between normal and very large diffs.
Test Plan:
Verify that normal diff works.
Verify that very large diff works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2361
Summary: NOTE: `renderViewOptionsDropdown()` adds unnecessary parameters to URL but the link just redirects anyway.
Test Plan:
Show Raw File (Left and Right) in SVN and Git.
Verify also Added and Deleted files.
Reviewers: epriestley, aran
Reviewed By: epriestley
CC: Koolvin
Differential Revision: https://secure.phabricator.com/D2370
Summary:
- This is only slightly useful for updating Differential, since DiffQuery (vs RawDiffQuery) already gets you most of what you need. The only thing is that DiffQuery returns the diff for one path only right now(and the SVN version is very "special"). Should be easy to fix in the Git/HG cases at least, though (or maybe just use RawDiffQuery to avoid the SVN mess).
- Added a "download raw diff" link.
Test Plan: Viewed Diffusion and raw commits for SVN, Mercurial and Git repositories.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2350
Summary:
- Show README on the repository screen.
- Move README to the bottom of the page for both repository and browse screens.
- Support "README.rainbow".
Test Plan: Looked at repository, browse screens. Made a "README.rainbow".
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1104
Differential Revision: https://secure.phabricator.com/D2336
Summary: We now allow symbolic commits, so let them through the pipeline.
Test Plan: {F10571}
Reviewers: davidreuss, btrahan, vrana
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2315
Summary:
- When viewing a commit, show its tags.
- For commits with many tags, show a list of all tags on the tag list interface.
- Improve some handling of symbolic references.
- When tags contain content, show it on the browse view reached by clicking the tag name.
Test Plan: Looked at commits with and without tags, clicked "More tags...", clicked tag names.
Reviewers: btrahan, vrana, davidreuss, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2290
Summary:
- Track + message through file moves.
- Stop + message on file create.
- Stop + message on first commit.
Test Plan:
- Tested blaming through a move, through a create, and through the first commit.
- Verified this doesn't break anything in SVN / Mercurial.
Reviewers: vrana, btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1091
Differential Revision: https://secure.phabricator.com/D2295
Test Plan:
Added CC's/Auditors, clicked the form elements, and saw correct
behaviour. Verified that metadata was present in the detail table.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, 20after4, Koolvin
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2002
Summary: Lists the 25 most recent tags on the "Repository" page.
Test Plan: Looked at a git repository with a tag, saw it. Looked at HG/SVN repos, they didn't break.
Reviewers: davidreuss, 20after4, btrahan, vrana, jungejason
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2255
Summary:
- For line numbers, use "user-select: none" to make them unselectable. This provides a stronger visual cue that copy/paste is enchanted.
- In Paste, make it look sensible again after the blame-on-blame refactor in Diffusion. See also TODO to share this code formally.
- In Diffusion, use the "phabricator-oncopy" behavior.
NOTE: I left blame/commit columns selectable in Diffusion, since you might reasonably want to copy/paste them?
NOTE: In Differential, the left side of the diff still highlights, even though it will be copied only if you select part of a line on the left and nothing else. But this seemed like a reasonable behavior, so I left it.
Test Plan:
- Looked at Paste. Saw a nice line number column. Selected text, got the expected selection. Copied text, got the expected copy.
- Looked at Diffusion. Saw a nice line number column, still. Selected text, got expected selection. Copied text, got expected copy.
- Looked at Differential. Highlighted stuff, got expected results. Copied stuff, got expected results.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1123
Differential Revision: https://secure.phabricator.com/D2242
Summary:
Otherwise useless query is executed:
lang=sql
SELECT c.*
FROM `repository_commit` c
ORDER BY c.epoch DESC
Test Plan: /diffusion/X/browse/x
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2186
Test Plan:
/rX1
Browse in Diffusion
Open in Editor
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2180
Summary:
PHP arrays have an internal "current position" marker. (I think because foreach() wasn't introduced until PHP 4 and there was no way to get rid of it by then?)
A few functions affect the position of the marker, like reset(), end(), each(), next(), and prev(). A few functions read the position of the marker, like each(), next(), prev(), current() and key().
For the most part, no one uses any of this because foreach() is vastly easier and more natural. However, we sometimes want to select the first or last key from an array. Since key() returns the key //at the current position//, and you can't guarantee that no one will introduce some next() calls somewhere, the right way to do this is reset() + key(). This is cumbesome, so we introduced head_key() and last_key() (like head() and last()) in D2161.
Switch all the reset()/end() + key() (or omitted reset() since I was feeling like taking risks + key()) calls to head_key() or last_key().
Test Plan: Verified most of these by visiting the affected pages.
Reviewers: btrahan, vrana, jungejason, Koolvin
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2169
Summary:
- Make some effort to simplify the code.
- Make "Skip Past This Commit" work in Git and Mercurial.
- Make blame work in Mercurial.
- Add tooltip hover state to show more information about commits.
Test Plan: Viewed blame views in SVN, Git, Hg. Clicked line numbers, hovered/clicked commits, hovered/clicked "blame past..."
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T378
Differential Revision: https://secure.phabricator.com/D2142
Summary:
I've considered that user may have set editor but not checked out Phabricator repositories.
But stack trace is useful mainly for developers.
Test Plan:
Click on path in Unhandled Exception.
Repeat with disabled editor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2107
Summary: Use Edges to attach Commits and Tasks. Note, no "edit attached commits" interface from tasks yet since the search backend needs a little work to list commits in a sensible way.
Test Plan: Attached commits to tasks. Looked at commits, looked at tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2105
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2103
Test Plan:
Jump to head.
Go to doctor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2097
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2094
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2085
Summary:
- Differential, Maniphest and Diffusion use slightly different styles for the object detail panels.
- Instead, use the same styles and CSS.
- Add object actions to Diffusion, including "Flag".
Test Plan: Looked at revisions, tasks and commit. Flagged and unflagged commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2062
Summary:
I think this feature is probably good, but Differential is also really starting to get a lot of stuff which is not the diff in it. Not sure how best to deal with that.
The mixed table styles are also pretty ugly.
So I guess this is more feedback / proof-of-concept, I think I want to try to improve it somehow before I land it.
Test Plan: Looked at some diffs, some had an awkward, ugly list of diffs affecting the same files.
Reviewers: bill, aran, btrahan
Reviewed By: aran
CC: aran, epriestley
Maniphest Tasks: T829
Differential Revision: https://secure.phabricator.com/D2027
Summary:
These are the issues identified by the linter in D2052. I don't think any cause bugs, but they are all reasonable errors to raise and the linter correctly
detected that they are suspicious.
Test Plan: Mostly inspection.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2053
Summary:
The full path field of the DiffusionRepositoryPath object is used by the
DiffusionBrowseController when viewing a directory with a readme file, so
we should set this field.
Test Plan: loaded a directory containing a readme in a svn repo
Reviewers: epriestley, jungejason, emiraga
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2045
Summary:
If a file was last modified at revision 50 and you look at revision 55, we currently 404. Instead, always identify the last modification.
Also simplify some of the query objects.
Test Plan: Viewed after-modification revisions for several files in SVN.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Maniphest Tasks: T851
Differential Revision: https://secure.phabricator.com/D2028
Summary: Show parent commit information to make it easier to understand merges.
Test Plan: Looked at commits in SVN, hg, git.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T961
Differential Revision: https://secure.phabricator.com/D2021
Summary: COMPLETELY ORIGINAL IDEA
Test Plan: Browsed around Phabricator, got helpful readmes in some cases.
Reviewers: davidreuss, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2022
Summary:
See some discussion in D2002. Add two new actions:
- Resign: (auditor only) closes your open request (user request ONLY) by putting it in a "resigned" state.
- Close: (author only) closes all open requests by putting them in a "closed" state.
@davidreuss, this is probably conflict-city with D2002 -- I'll wait for you to land first and then handle the merge on my end.
Test Plan: Resigned from and closed audits.
Reviewers: 20after4, davidreuss, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2013
Summary:
$stdout from the previous run would be reused if an exception
occurred
Test Plan: that's a negative, ghostrider.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2008
Summary:
When a commit is a merge, show what it merged.
Also fix some bugs:
- Mercurial queries may contain ":", but mercurial rev ranges may also contain ":". A rev range with a branch that has a ":" in it is ambigiuous, e.g. branch "a:b" might appear in a rev range like "a🅱️0", which can not be parsed. Use stable commit names instead.
- Mercurial stable commit name implementation was broken, fix it.
- Extend DiffusionHistoryQuery from DiffusionQuery to share code.
- Fix a bug where Mercurial's main browse list would not show the most recent commit if it was a merge commit.
Test Plan: Generated a bunch of mercurial/git merge commits and looked at them, they seemed to accurately represent the repository state.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T961
Differential Revision: https://secure.phabricator.com/D2005
Summary: We need to build a request in order to pick up an appropriate default branch name, instead of using the raw static generator.
Test Plan: Clicked a symbol link, got /master/path/blahblah
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1982
Summary:
- Feature request from Airtime that I missed in the feedback notes, came up yesterday.
- Identify git submodules as "FILE_SUBMODULE", not "FILE_NORMAL".
- Link git submodules to an external resolver endpoint, which tries to find commits in tracked repositories.
- Identify git symlinks as "FILE_SYMLINK", not "FILE_NORMAL".
- Add folder, file, symlink and externals icons.
Test Plan:
- externals/javelin is now identified as a submoudule and links to Javelin, not identified as a file and links to error.
- bin/phd is now identified as a symlink.
- Interfaces have pretty icons.
Reviewers: btrahan, cpiro, ddfisher, keebuhm, allenjohnashton
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1975
Test Plan:
Search for some symbol. Click on the result. Verify that there is not // in URL.
Click on the link from generated exception.
View history in Diffusion, click on Browse.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1979
Test Plan: Clicked on `Exception` in `throw new Exception`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1978
Summary:
Depends on D1921. Depends on D1899.
Add the "View Options" dropdown menu to Diffusion, with options like "show standalone", "show raw file", "show all", etc.
Test Plan: Viewed commits in Differential and Diffusion.
Reviewers: davidreuss, nh, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1932
Summary: Depends on D1928. Uses the new UI element to display inlines in Diffusion.
Test Plan: Looked at a commit with inline comments, saw them in the summaries.
Reviewers: davidreuss, nh, btrahan
Reviewed By: davidreuss
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1929
Summary:
This is in preparation for getting the "View Options" dropdown working on audits.
- Use Files to serve raw data so we get all the security benefits of the alternate file domain. Although the difficulty of exploiting this is high (you need commit access to the repo) there's no reason to leave it dangling.
- Add a "contentHash" to Files so we can lookup files by content rather than adding some weird linker table. We can do other things with this later, potentially.
- Don't use 'data' URIs since they're crazy and we can just link to the file URI.
- When showing a binary file or an image, don't give options like "show highlighted text with blame" or "edit in external editor" since they don't make any sense.
- Use the existing infrastructure to figure out if things are images or binaries instead of an ad-hoc thing in this class.
Test Plan: Looked at text, image and binary files in Diffusion. Verified we reuse existing files if we've already generated them.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1899
Summary:
Diffusion request/uri handling is currently a big, hastily ported mess. In particular, it has:
- Tons and tons of duplicated code.
- Bugs with handling unusual branch and file names.
- An excessively large (and yet insufficiently expressive) API on DiffusionRequest, including a nonsensical concrete base class.
- Other tools were doing hacky things like passing ":" branch names.
This diff attempts to fix these issues.
- Make the base class abstract (it was concrete ONLY for "/diffusion/").
- Move all URI generation to DiffusionRequest. Make the core static. Add unit tests.
- Delete the 300 copies of URI generation code throughout Diffusion.
- Move all URI parsing to DiffusionRequest. Make the core static. Add unit tests.
- Add an appropriate static initializer for other callers.
- Convert all code calling `newFromAphrontRequestDictionary` outside of Diffusion to the new `newFromDictionary` API.
- Refactor static initializers to be sensibly-sized.
- Refactor derived DiffusionRequest classes to remove duplicated code.
- Properly encode branch names (fixes branches with "/", see <https://github.com/facebook/phabricator/issues/100>).
- Properly encode path names (fixes issues in D1742).
- Properly escape delimiter characters ";" and "$" in path names so files like "$100" are not interpreted as "line 100".
- Fix a couple warnings.
- Fix a couple lint issues.
- Fix a bug where we would not parse filenames with spaces in them correctly in the Git browse query.
- Fix a bug where Git change queries would fail unnecessarily.
- Provide or improve some documentation.
This thing is pretty gigantic but also kind of hard to split up. If it's unreasonably difficult to review, let me know and I can take a stab at it though.
This supplants D1742.
Test Plan:
- Used home, repository, branch, browse, change, history, diff (ajax), lastmodified (ajax) views of Diffusion.
- Used Owners typeaheads and search.
- Used diffusion.getrecentcommitsbypath method.
- Pushed a change to an absurdly-named file on an absurdly-named branch, everything worked properly.
{F9185}
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1921
Summary:
See <https://github.com/facebook/phabricator/issues/102>. Commit data may not be available for unpared commits, but we'll raise a warning about $commit_data in that case (the UI correctly handles missing $commit_data).
Also some minor cleanup / UI fixes.
Test Plan: Browsed around hg / git repos, including unparsed commits.
Reviewers: btrahan, killermonk
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1961
Summary:
Split from D1921.
- DifferentialChangesetParser doesn't have this property declared.
- We weren't providing a markup engine, which caused some warnings.
- This would cause failures if comment caches weren't present. Currently, they always will be though unless someone has wiped them explicitly in the DB.
Test Plan: Viewed a diff with inline comments, didn't get any warnings in the log.
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1925
Summary:
Split from D1921. We'll explode each line into too many parts currently, if the filename contains spaces.
Also use -z to get \0 newlines.
Test Plan: Browsed a directory containing files with spaces in their names, links etc were correct.
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1924
Summary: This diff is really complicated, only a master programmer like myself could have accomplished it.
Test Plan: derrrrrp
Reviewers: davidreuss, nh, btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1936
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
Summary:
It is misplaced from the beginning but it got worse after some CSS tweaks.
I was also thinking about using the same DropdownMenu as in Differential but I don't feel strongly about it to do it myself.
Test Plan:
Display file in Diffusion.
Repeat with disabled Editor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1892
Summary: svn cat returns a non-zero exit status when trying to cat a directory.
Test Plan: Browsed diffusion in my sandbox
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1882
Summary: No big surprises here, delted the unused "DarkConsole" class.
Test Plan: Ran 'testEverythingImplemented' to verify I wasn't finalizing anything we extend.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1876
Summary:
These are all unambiguously unextensible. Issues I hit:
- Maniphest Change/Diff controllers, just consolidated them.
- Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
- D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.
Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1843
Summary:
We can drive this query better from the Audit tool now; get rid of the Diffusion
version.
Preserve usernames in URIs as per T900.
Test Plan: Clicked "Commits" from profile. Browsed audit commit filters.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1713
Summary:
- Add a proper mailKey field to make these things mailable. Backfill all
existing objects.
- Denormalize authorPHID to the commit object so we can query by it
efficiently in a future diff. We currently use the search engine to drive
"commits by author" but that's not so good for audit, which needs more
constraints.
- Add an overall audit status field so we can efficiently query "commits that
needs your attention".
- Add enough code to convince myself that these fields are basically
reasonable and work correctly.
Test Plan:
- Ran schema upgrades. Checked database state afterward.
- Ran "reparse.php --owners --herald" to verify worker changes.
- Looked at a commit, altered aggregate status via audits / reparse.php,
verified it responded correctly.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley, nh
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1706
Summary: Add comment previews and saved drafts to audits, like Maniphest /
Differential.
Test Plan: Typed stuff into the box. Got a preview. Reloaded page. Stuff was
still there. Submitted comment. Stuff is gone.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1699
Summary:
Allows you to write a commit rule that triggers an audit by a user (personal
rules) or a project (global rules).
Mostly this is trying to make auditing more lightweight and accessible in
environments where setting up Owners packages doesn't make sense.
For instance, Disqus wants a rule like "trigger an audit for everything that
didn't have a Differential revision". While not necessarily scalable, this is a
perfectly reasonable rule for a small company, but a lot of work to implement
with Owners (and you'll get a lot of collateral damage if you don't make every
committer a project owner).
Instead, they can create a project called 'Unreviewed Commits' and write a rule
like:
- When: Differential revision does not exist
- Action: Trigger an Audit for project: "Unreviewed Commits"
Then whoever cares can join that project and they'll see those audits in their
queue, and when they approve/raise on commits their actions will affect the
project audit.
Similarly, if I want to look at all commits that match some other rule (say,
XSS) but only want to do it like once a month, I can just set up an audit rule
and go through the queue when I feel like it.
NOTE: This abuses the 'packagePHID' field to also store user and project PHIDs.
Through the magic of handles, this (apparently) works fine for now; I'll do a
big schema patch soon but have several other edits I want to make at the same
time.
Also:
- Adds an "active" fiew for /audit/, eventually this will be like the
Differential "active" view (stuff that is relevant to you right now).
- On commits, highlight triggered audits you are responsible for.
Test Plan: Added personal and global audit triggers to Herald, reparsed some
commits with --herald, got audits. Browsed all audit interfaces to make sure
nothing exploded. Viewed a commit where I was responsible for only some audits.
Performed audits and made sure the triggers I am supposed to be responsible for
updated properly.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1690
Summary:
Current approach has several problems:
- if there is no link in the cell then it still shows a link cursor
- if there is a link then it is clickable only on the text
Test Plan:
Display file in Differential, hover over cell with link.
Repeat for Paste.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1701
Summary:
I added multiline highlighting with the syntax:
http://site/path/to/file$from-to
NOTE: you can reverse the from and to
Test Plan: Open a file in diffusion and attempt to highlight multiple lines
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1693
Summary:
This is intended to supplant the existing "audit edit" interface. I've changed
them to both drive down the same write pathway, but the UIs are still different.
I'll fully merge them in a future diff.
Add a comment box (like Maniphest and Differential) to Diffusion. When users
make comments, their comments appear on the commit. Any audits triggers they are
responsible for are updated to reflect actions they take, as well.
Currently, audits can only be triggered by packages, but I intend to allow them
to be triggered by users and projects (via herald rules) in an upcoming diff.
Thus some of the language like "projects, users or packages" when the code is
clearly dealing only with "packagePHID".
Test Plan: Made audit updates via commit interface and via existing edit
interface. Verified both interfaces updated correctly, and that audit
responsibility rules were applied properly.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1688
Summary: We already allow you to create comments, but we don't show them on the
commit page. After style / view unification this is easy; show comments on the
commit page.
Test Plan: Made comments on a commit using the audit too, saw them show up in
Diffusion.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1687
Summary:
Currently, audits are only accessible through the Owners tool. Start moving them
to their own first-class tool in preparation for broader audit integration.
- Lay some infrastructure groundwork (e.g. AuditQuery).
- Build a basic /audit/ view.
- Show audits on the commit page in Diffusion.
This has some code duplication with stuff we've already got, but I'll merge
everything together as we move forward on this.
Test Plan: Looked at /audit/ and a commit.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1685
Summary:
DifferentialRevisionListView requires setFields to be called before
calling getRequiredHandlePHIDs; this adds that call for DiffusionController
Test Plan:
loaded diffusion and saw the "Pending Differential Revisions" section
populated, and no errors in the darkconsole
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1665
Summary: Add a "Search for ... in (document group)" thing that picks the current
scope based on the current application.
Test Plan: Conducted searches in several browsers.
Reviewers: btrahan, skrul
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T858
Differential Revision: https://secure.phabricator.com/D1610
Summary: I think that these are the only links that are useful - commit which
deleted the path and last version of the path.
Test Plan: Display deleted path, click on links.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1536
Summary:
It is possible to open a file in editor by registering a custom URI scheme
(pseudo-protocol). Some editors register it by default.
Having links to open the file in external editor is productivity booster
although it is a little bit harder to set up.
There are several other tools using file_link_format configuration directive
(XDebug, Symfony) to bind to this protocol.
I've added the example with editor: protocol which can be used as a proxy to
actual editor (used by Nette Framework:
http://wiki.nette.org/en/howto-editor-link).
Test Plan:
Configure Editor Link in User Preferences.
Register URI scheme in OS.
Open a file in Diffusion. Click on the Edit button.
Open a revision in Differential. Click on the Edit button.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1422
Summary:
They are present in the document so there is no reason to omit the links to
them.
Similar to D1412.
Test Plan: Display commit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1464
Summary:
We show the contextual branch (always the repository default branch) when
viewing a commit. Instead, show all branches the commit appears on.
Also pull some of the duplicated DiffusionXQuery stuff into a DiffusionQuery
base class, I'll do a followup to reduce more duplication.
Test Plan: Looked at a commit in Git. My HG and SVN setups are a little borked
so I kind of faked tests in them -- I'm fixing them now.
Reviewers: btrahan, jungejason, fratrik
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T768
Differential Revision: https://secure.phabricator.com/D1458
Summary:
/diffusion/X/history/?copies=0 is same as /diffusion/X/history/
/countdown/1/?chrome=1 is same as /countdown/1/
Test Plan:
Visit /diffusion/X/history/, click on Show/Hide Copies/Branches twice.
Visit /countdown/1/, click on Disable/Enable Chrome twice.
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1424
Summary:
Provide an easy way to jump to Diffusion from Differential if we have
the data we need to connect them.
Test Plan: Tested menu in linked and unlinked diffs. Used menu item.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T309
Differential Revision: https://secure.phabricator.com/D1326
Summary:
Some installs use Git as the backbone of a CI framework or use a Git remote to
share patches. The tracker scripts currently recognize associated revisions as
"Committed" when they appear in any branch, even if that branch is
"alincoln-personal-development_test_hack" or whatever.
To address the broadest need here, allow Git repositories to be configured to
track only certain branches instead of all branches.
This doesn't allow you to import a branch into Diffusion but ignore it in
Differential. Supporting that is somewhat technically complicated because the
parser currently goes like this:
- Look at HEAD of all branches.
- For any commits we haven't seen before, follow them back to something we
have seen (or the root).
- "Discover" everything new.
Since this doesn't track <branch, commit> pairs, we currently don't have enough
information to tell when a commit appears in a branch for the first time, so we
don't have anywhere we can put a test for whether that branch is tracked and do
the Differential hook only if it is.
However, I think this cruder patch satisfies most of the need and is simple and
obvious in its implementation.
See also D1263.
Test Plan:
- Updated a Git repository with various filters: "", "master, remote", "derp",
" ,,, master ,,,,,"
- Edited SVN and Mercurial repositories to verify they didn't get caught in
the crossfire.
- Ran daemon in debug mode on libphutil with filter "derp", got exception
about no tracked branches. Ran with filter "master", got tracking. Ran with no
filter, got tracking.
- Looked at Diffusion with "derp" and "master", saw no branches and "master"
respectively.
- Added unit tests to cover filtering logic.
Reviewers: btrahan, jungejason, nh, fratrik
Reviewed By: fratrik
CC: aran, fratrik, epriestley
Maniphest Tasks: T270
Differential Revision: https://secure.phabricator.com/D1290
Summary:
See T624. I originally wrote this to require an explicit remote, but this
creates an ugly "origin:" in all the URIs and makes T270 more difficult.
Treat all branch names as implying 'origin/'.
Test Plan:
- Pulled and imported a fresh copy of libphutil without issues.
- Browsed various git repositories.
- Browsed Javelin's various branches.
- Ran upgrade script, got a bunch of clean 'origin/master' -> 'master'
conversions.
- Tried to specify an explicit remote in a default branch name.
- Unit tests.
Reviewers: nh, jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T624
Differential Revision: https://secure.phabricator.com/D1269
Summary: I think we only hit this because I mucked around with the database to
recover from the runaway parse of the Diviner repository (now prevented by
D1253), but be more robust against missing data in this interface.
Test Plan: After applying this patch, no longer received a fatal on the commit
history page for users linked to nonexistant/bogus commits.
Reviewers: jack, btrahan, jungejason, aran
Reviewed By: aran
CC: aran
Maniphest Tasks: T701
Differential Revision: https://secure.phabricator.com/D1264
Summary: I want to add a command like "where is ArcanistUnitTestEngine" to
phabot. I also want to add a symbol typeahead to Diffusion and generally finish
up that feature since it's useful but only half-implemented. Consolidate the
query logic and expose the data over Conduit.
Test Plan: Used /symbol/ and Conduit to lookup symbols.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T315
Differential Revision: https://secure.phabricator.com/D1260
Summary: See T547. One of these I just missed in D1000; the comment change just
makes it easier to audit use of hash functions by cleaning up "grep" output.
Test Plan: Ran isolation unit test.
Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1124
Summary:
This adds an encoding detail to the repository, so we can attempt to
convert hunks previously detected as binary.
We also add the encoding information to the arcanist projectinfo
API so we can pull the information if we have it when uploading changes
via arc.
Test Plan:
Changed encoding through the edit UI, and saw "This is binary file", and
changed it back and saw the correct output from the diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1009
Summary:
make sure all symboles can be loaded to avoid issues like missing
methods in descendants of abstract base class.
Test Plan:
ran it and verified it passes; remove a method in a descendant class
and verified that the test failed.
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, nh, jungejason
Differential Revision: 1023
Summary: It looked stupid.
Test Plan: It looks better now and other options still work.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 1017
Summary:
This makes symbol cross-references work in Differential. You need to do a little
legwork but I'll document that once the change has baked for a little while.
Basically:
- Projects are annotated with indexed languages, and "shared library" projects
(for example, symbols in Phabricator should be searched for in Arcanist and
libphutil).
- When we render a changeset, we check if its language is an indexed one. If
it is, we invoke the decorator Javascript.
- The Javascript takes you to a lookup page, which either gives you a list of
matching symbols (if several match) or redirects you instantly to the
definition.
Test Plan: Clicked class and function symbols in a diff, got jumped into
sensible sorts of places in Diffusion.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 980
Summary:
Still some rough edges, but this adds a table of open revisions to Diffusion.
See T262.
I'll make this a little better (e.g., "see all.." instead of arbitrary 10 cap,
or maybe move to top-level nav?) but I think I have to refactor some other stuff
first. This should let us root out any major issues, at least.
NOTE: You must associate Arcanist Projects with Repositories (in Repositories ->
Arcanist Projects -> Edit) for this to work!
Also made paths include all parent paths so that browse views of directories
will work.
Test Plan: Uploaded a diff which affected "/blah", it appeared when browsing "/"
and "/blah".
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 979
Summary: See T507. Since you can't do "xxxxxxxx^" where "xxxxxxxx" is the first
commit in a repository, fall back to diffing against the empty tree if we fail
to diff against the parent commit.
Test Plan: Looked at the first commit in libphutil on my local.
Reviewers: edward, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, edward, epriestley, nh
Differential Revision: 953
Summary: Change import script plus almost all the view stuff. Still some rough
edges but this seems to mostly work. Blame is currently unsupported but I think
everything else works properly.
Test Plan:
Imported the hg repository itself. It doesn't immediately seem completely
broken. Here are some screens:
https://secure.phabricator.com/file/view/PHID-FILE-1438b71cc7c4a2eb4569/https://secure.phabricator.com/file/view/PHID-FILE-3cec4f72f39e7de2d041/https://secure.phabricator.com/file/view/PHID-FILE-2ea4883f160e8e5098f9/https://secure.phabricator.com/file/view/PHID-FILE-35f751a36ebf65399ade/
All the parsers were able to churn through it without errors.
Ran the new "reparse.php" script in various one-commit and repository modes.
Browsed/imported some git repos for good measure.
NOTE: The hg repository is only 15,000 commits and around 1,000 files.
Performance is okay but hg doesn't provide performant, native APIs to get some
data efficiently so we have to do some dumb stuff. If some of these interfaces
are cripplingly slow or whatever, let me know and we can start bundling some
Mercurial extensions with Arcanist.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde, epriestley
Differential Revision: 960
Summary:
Just breaking D960 into some smaller parts, this is a standalone method used in
Mercurial parsing.
(There's a bad version of this function in the SVN stuff but I'll get rid of it
the next time I'm in there.)
Test Plan: See D960.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 965
Summary:
We currently rely on "remote_hooks_enabled" in .arcconfig to determine whether
commands like "arc amend" and "arc merge" should imply "arc mark-committed".
However, this is a historical artifact that is now bad for a bunch of reasons:
- The option name is confusing, it really means 'repository is tracked'.
- The option is hard to discover and generally sucks.
- We can empirically determine the right answer since we now know if a project
is in a tracked repository.
Add a call which arcanist can make on these workflows to figure out if it is
interacting with a project in a tracked repository or not.
Also added an "isTracked()" convenience method to reduce the number of magic
strings all over the place.
Test Plan: Ran "arcanist.projectinfo" for nonexistent, untracked and tracked
projects.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, epriestley, Makinde
Differential Revision: 945
Summary:
dirname('x') returns '.', not '/'; this caused some issues for repositories with
files at the root.
There are some cases in the parsers where I should probably swap this out too
but I'll wait until I'm doing some more rigorous testing since that stuff is a
bit fragile and this fixes an immediate issue.
Test Plan: Ran unit tests. Viewed a file at root level in a test repository.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 932
Summary: See T262. This creates the index on the Differential side which we need in order to execute this query efficiently on the Diffusion side.
Also renames "DiffusionGitPathIDQuery" to "DiffusionPathIDQuery", this query object has nothing to do with git.
Test Plan: Attached top-level and sub-level diffs to revisions and verified they populated the table with sensible data.
Reviewers: bmaurer, aravindn, fmoo, jungejason, nh, tuomaspelkonen, aran
CC:
Differential Revision: 931
Summary: This will get fancier, but here's a basic interface for doing symbol
lookups. Still all pretty tentative.
Test Plan: Looked up various things, got some sensible results.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 900
Summary:
When Diffusion encounters an image file, it displays it as an
image, but when it encounters a PDF file, it currently shows only some
gibberish. This fixes that.
Test Plan:
I tried it. Embedding a large PDF in a data URL is a little
bit slow, but it works.
Reviewers: tuomaspelkonen, epriestley, gc3, waltermundt, jungejason, nh
Reviewed By: epriestley
CC: aran, tuomaspelkonen, epriestley, jaapweel
Differential Revision: 915
- Removed irrelevant csprintf(..)
- Updated code to use $repository->getRemoteURI()
- Updated code to use getRemoteCommandFuture(..) in Diffusion code
- Updated code to use $repository->getRemoteURI()
Summary:
Give users better errors and UI:
- For subpath SVN repositories, default the path to the subdirectory, not to
"/". This makes the home screen useful and things generally less confusing.
- For unparsed commits, show a more descriptive error message without the
"blah blah" silliness.
- For paths outside of the subpath parse tree, short circuit into an
appropriate error message.
- For foreign SVN stub commits (see D892), show an explicit message.
Test Plan: Looked at unparsed commits, subpath repositories, foreign stub
commits, and paths outside of the subpath parse tree. Received sensible error
messages.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 894
Summary:
When selecting children of a directory, it is possible that none of its
children exist anymore even though the directory still exists. After fetching
the children but before returning them, we should check whether there are any,
and if there are no children, set the reason as empty directory.
Test Plan:
In sandbox, browsed in diffusion to a directory that exists but has no
files and saw that it has a useful message instead of a vague exception.
Reviewers: epriestley, tuomaspelkonen, jungejason
Reviewed By: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 846
Summary:
In preparation for adding another search engine (see T355):
- Rename "executor" to "engine".
- Move all engine-specific operations into the engine. Specifically, this
means that indexing moves out of the document store and into the engine (it was
sort of silly where it was before).
- Split choice of an engine into an overridable "selector" class, a base API,
and a concrete MySQL implementation (just like storage engine selection).
- Make all callers go through the indirection layer.
The default selector just unconditionally selects the MySQL engine, but now
(with D786) I can build an Elastic Search engine and you guys can build a
multi-target engine if you want and I don't get there fast enough.
Test Plan:
- Created a new document (task).
- Searched for and found it.
- Viewed index reconstruction.
Reviewed By: jungejason
Reviewers: jungejason, amckinley, tuomaspelkonen, aran
CC: aran, jungejason, epriestley
Differential Revision: 788
Summary:
See T372. Always render commit messages on one display line, so the table
doesn't jump around as they AJAX in on browse views.
The goal here is to have the cell choose a size naturally and for its content to
render with "overflow: hidden" if the natural size isn't large enough to contain
the content. "white-space: pre" or "white-space: nowrap" would prevent wrapping
but potentially make the table exceed the display width when a better behavior
is to hide some of the commit message.
Also use utf8-aware shortening, now that we have a function for it.
Casting a wide net in case anyone has a better way to do the CSS here. It's kind
of nasty that we have to use so many DOM nodes.
Test Plan:
- Resized window while viewing browse and history views in Safari, Chrome and
Firefox. Table exhibited described behavior.
- Verified summaries render sensibly and are properly truncated to 100
characters.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, tomo, mroch, cpojer
CC: aran, epriestley
Differential Revision: 750
Summary:
We just weren't handling these at all reasonably, must have dropped the logic
when they got ported.
This still isn't perfect: we have some display glitches around file names, so
the 'away' part renders as "This file was moved to .". I'll see if I can follow
up and fix that, but this resolves the more immediate issue of the interface
just not working at all.
Test Plan: Moved and copied files in my test repository, verified they rendered
somewhat correctly.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, jungejason, epriestley
Differential Revision: 699
Summary: create the page by getting data from the search result.
Test Plan:
load page with url /author/, /author/valid_username, and
/uathor/invalid_username, and verified that it works as expected.
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
Commenters: tuomaspelkonen
CC: hwang, aran, tuomaspelkonen, epriestley, jungejason
Differential Revision: 723
Summary:
Share code with the new PhabricatorDifferenceEngine, which handles diffs with no
changes correctly.
(This isn't the same issue as file moves, but I ran into it while generating a
repro case.)
Test Plan: Previously, changes which didn't change file content (e.g., property
changes) would throw. Now they work.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 698
Summary:
This is really rough and needs work (particularly, there's some diff code I
really need to refactor since I sort-of-copy-pasted it) but basically
functional.
Show text changes between diffs and allow users to revert to earlier versions.
Differential's line-oriented diff style isn't ideal for large blocks of text but
I'm betting this is probably good enough in most cases. We can see how bad it is
in practice and then fix it if needbe.
I added a bunch of support for "description" but didn't add the feature in this
diff, I'll either follow up or task it out since it should be a pretty
straightforward change.
Test Plan: Looked at history for several Phriction documents, clicked "previous
change" / "next change", clicked revert buttons.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 687
