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:
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:
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:
- 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: 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:
- 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:
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:
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: 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:
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:
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: 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:
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:
- 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: 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: 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:
- 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
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:
- 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:
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: 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:
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: 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:
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: 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
