Summary: The "line count" is always the same as count($this->getOldLines()), and somewhat misleading since it's really "number of rows in the two-up view". D4421 removed the only (or only remaining?) call.
Test Plan: looked at some diffs
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4422
Summary:
This is a half-step toward one-up and test renderers. This is mostly a structural change, and has no user-facing impact. It splits the rendering hierarchy like this:
- Renderer (more methods are abstract than before)
- HTML Renderer (most HTML stuff has moved down from the base to here)
- HTML 1-up (placeholder only -- not yet a functional implementation)
- HTML 2-up (minimal changes, uses mostly old code)
- Test Renderer (unit-testable renderer base, implements text versions of the HTML stuff)
- Test 1-up (selects 1-up mode for test rendering)
- Test 2-up (selects 2-up mode for test rendering)
Broadly, I'm trying to share as much code as possible by splitting rendering into more, smaller stages. Specifically, we do this:
- Combine the various sorts of inputs (changes, context, inlines, etc.) into a single, relatively homogenous list of "primitives". This happens in the base class.
- The primitive types are: old (diff left side), new (diff right side), context ("show more context"), no-context ("context not available") and inline (inline comment).
- Possibly, apply a filtering/reordering step to the primitives to get them ready for 1-up rendering. This mostly removes information, and does a small amount of reordering. This also happens in the base class.
- Pass the primitives to the actual renderer, to convert them into HTML, text, or whatever else. This happens in the leaf class.
The primitive implementation is not yet complete (it doesn't attach as much information to the primitives as it should -- stuff like coverage and copies), but covers the basics.
The existing HTMLTwoUp renderer does not use the primitive path; instead, it still goes down the old path.
Test Plan: Ran unit tests, looked at a bunch of diffs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4421
Summary:
Fixes T181. I actually have no idea what the original issue in T181 was, but this fixes several problems:
- The code for figuring out whitespace-only changes was extremely confusing and probably buggy (the code for figuring out unchanged files was equally confusing but probably less buggy).
- When rendering a whitespace shield, we did not offer a "Show Changes" link. Instead, show the "Show Changes" link: we can always render the content beneath this link.
- When clicking "Show Changes", we used the current whitespace mode, which might result in a diff with no changes. Instead, force "show all" whitespace mode.
- We never offered a "show changes" link for unchanged files. Instead, offer this link if we can service it.
- When clicking "Show Changes", we pierced the shield but didn't force file content, which would fold the entire file even if it was available. Instead, force the file content.
Test Plan: Generated whitespace-only and no-change diffs. Clicked shield links, or verified no shield link available in no-change-with-no-content. Generated an "@generated" change, verified shield worked correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T181, T2009
Differential Revision: https://secure.phabricator.com/D4407
Summary:
Simplify whitespace-ignoring diffing:
- Currently, we generate two diffs (one ignoring whitespace, one normal) and then copy the //text content// from the normal one to the whitespace-ignoring one.
- Instead, copy the //change types// from the ignoring one to the normal one.
- This is cheaper and much simpler.
- It means that we have the right change types in reparseHunksForSpecialAttributes(), and can simplify some other things.
Simplify whitespace changes, unchanged files, and deleted file detections:
- Currently, we do this inline with a bunch of other stuff, in the reparse step.
- Instead, do it separately. This is simpler.
Simplify intraline whitespace handling:
- Currently, this is a complicated mess that makes roughly zero sense.
- Instead, do it in reparse in a straightforward way.
Partially fix handling of files changed only by changing whitespace.
Partially fix handling of unchanged files.
Test Plan:
- Ran unit tests.
- Created context-less diffs, verified they rendered reasonably.
- Generated a diff with prefix whitespace, suffix whitespace, intraline whitespace, and non-whitespace changes. Verified changes rendered correctly in "ignore most" and "show all" modes.
- Verified unchanged files and files with only whitspace changes render with the correct masks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4398
Summary:
- Remove 'missingNew', etc. It's impossible for a diff to popluate these, as far as I can tell (I can't generate such a diff, or find any which generate it).
- Add unit tests.
Test Plan: Unit tests, viewed a diff with some missing context.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4393
Summary: D4351 swapped this from a map to a list, and then stopped it from getting to the ChangesetParser so it didn't make it to the Renderer.
Test Plan: Ran "arc diff", copy/pasted it into the web UI. Saw a diff with "context not available" blocks in between missing contexts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4384
Summary: I forgot to "set it" from the changeset parser.
Test Plan: made a diff with silly whitespace changes. toggled the various whitespace modes and got appropriate results
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4378
Summary: introducing a new friend called DifferentialHunkParser. Sort of like the DifferentialChangesetParser but works with hunks only. tried to grab hunk parsing type things from across the code base and move them into this new class.
Test Plan: unit tests and played around in Differential a bit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4351
Summary: in the re-factor quest this got broken. Turns out NewLines (aka $this->new in the parser) gets updated right up until the very end for showing text changes so instead pass over the total line count to the renderer to get this codepath right.
Test Plan: showed content of some generated files in diffusion and all was well
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2204
Differential Revision: https://secure.phabricator.com/D4237
Summary: pull out some more stuff from the TwoUp renderer that's generically useful. also clean up $xhp variable and add a get method for the $coverage. Finally, fix T2177 while I'm in here.
Test Plan: played around with Differential. Also opened things up in Firefox to verify T2177.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009, T2177
Differential Revision: https://secure.phabricator.com/D4161
Summary: Minor issue from D4117, user doesn't get passed down so inline comments are missing their "reply" link.
Test Plan: Looked at Differential, saw reply link.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4145
Summary:
basically did my darnedest to pull out a TwoUp rendering view. Made a base class for the rendering views with "old" and "new" terminology rather than "left" and "right.
Future revisions will finish cleaning up the terminology within the DifferentialChangesetParser itself and more of the ideas within T2009.
Test Plan: been playing with differential all day
Reviewers: epriestley
Reviewed By: epriestley
CC: vrana, chad, aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4117
Summary: inline comments eat up all 3 tds and no code coverage should be shown.
Test Plan: verified in FIREFOX that inline comments looked good
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4115
Summary: when we had a change that had new data and uncommitted changes the colspan could get off. make sure to only decrement the colspan if we are actually on a new line
Test Plan: http://phabricator.dev/rP1a3bf098ae4ab4f8add4af744a6b93a257851fb0 now looks good on Firefox
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4109
Summary:
assume at least 360px for a given code pane. that's about when the comment box starts fighting back anyway. we'll use the yet-to-be-built one page render for the narrow viewport cases.
This address the cases as laid out in T2005. It fails the "MMMMM" case pretty horribly. However, if there is a space it works just fine and presumably folks are stretching out their windows on big glorious monitors at 160 characters wide or whatever.
Re-factored things just a tad but figure I'll take a nice big chunk of "renderer" to move forward T2009
Test Plan: looked at all sorts of funky diffs
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4083
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:
all sorts of stuff
- made comment form width flexible
- made margins element specific rather than part of differential-primary-pane
- made box elements all veritically align left and right until code stuff
- re-factored width calculaton stuff a bunch so only the code section has to suffer from max-width calculations; everything else can flex
- made colspan 3 for rightmost table header element. this is so the "View Options" UI element ends up lining up correctly with the "Show All Lines" element just below
Test Plan: looked at revision view and changeset view and it all looked hot. note I did not test what things looked like with different word wrap values; that should still work given the re-factoring and not re-design here. also toggled haunted panel mode and it looked good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2006
Differential Revision: https://secure.phabricator.com/D3866
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: This should all go away at some point when we move to fluid layout, but don't be more annoying than necessary in the meantime.
Test Plan: Meta.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3788
Summary:
Same as D3759 with a fix:
Previously, we would insert `null` to indicate a line that doesn't exist on one side of a diff, and then implode on "\n", so we'd get "\n" as a result.
In D3759, we'd insert `null` but implode on empty string, and get nothing as a result.
When a placeholder null is present, explicitly insert a newline.
Test Plan: D3759 plus examined a diff with removed lines on the left side prior to new lines on the right side.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3765
This reverts commit f6cb51562e.
This has some bugs in normal diffs that I haven't immediately been able to figure out. I'll reapply it once I sort them out.
Auditors: vrana
Summary:
- We currently treat "\r" as a newline, but should not because VCSes do not.
- We get an extra empty line at the end of diffs created after D3442 because we now retain newlines.
- Historically we've converted tab pre-cache, but do it post-cache instead so we can add prefs about it, as we should handle it better than we do (e.g., let the user set it to a different width, infer width from comments in the file, expand it to actual tab stops, or show it visually in some way).
Test Plan:
- Verified diffs no longer have an empty line at the end.
- Created a diff of a "\r" file and verified it displayed somewhat reasonably. All browsers treat "\r" as a real newline so it's not necessarily perfect, but we can clean that up later. Hopefully these files are exceedingly rare.
- Created a file with tabs and verified it came out reasonably.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1857
Differential Revision: https://secure.phabricator.com/D3759
Summary:
sometimes we show moved files as unchanged, sometimes deleted,
and sometimes inconsistent with what actually happened at the
destination of the move. Rather than make a false claim,
omit the 'not changed' notice if this is the source of a move.
Background: one of our users was confused by the source of a move
claiming to be unmodified when the destination of the move had
extensive modifications.
There may be a question about the quality of the data we keep/compute
about the changes, so maybe we could do a better or more consistent
job there (may just be nuances of the SCM in use); this approach
just smoothes that out and doesn't require fixing up the status
in pre-existing diffs.
Test Plan:
In the facebook phabricator, D594195#739ace1a and D590020#b97cfcfd
For others, find a diff that has moved files.
For the source of a move, we should now only show the "X moved to Y"
header and not the "unchanged" shield.
Reviewers: nh, vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3669
Summary: Show First 20 Lines doesn't display gap context and emits error.
Test Plan: Showed first 20 lines, last 20 lines, middle 20 lines - saw context and no error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3616
Summary:
We have some complaints on this feature:
- It's not clear that the displayed code is context from gap.
- It would be better if the context would be displayed with its real indentation.
- It's not clear how far the context is from the displayed code.
- Links revealing gap aren't on consistent place.
This solves all these problems and introduces new one:
It now seems that the reveal links works only with the left side.
Anyway, I think that this is better overall.
I don't want to put the context on a separate line to not waste space.
Test Plan: Displayed various contexts, revealed context.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, bh, jwatzman
Differential Revision: https://secure.phabricator.com/D3404
Summary:
We have `max_allowed_packet` 1 GiB but our replication dies if the query is longer than unknown value (it dies with 293 MB long query).
Anyway, there's no reason why we should not save the cache if you have small `max_allowed_packet`.
Test Plan: Lowered `$size` to 100, deleted cache from DB, displayed changeset, verified issued queries in DarkConsole, verified DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3390
Summary:
See T1602#15.
I don't intend to land this right away, because I'm not sure I won't
need another change or two to the rendering code. Keeping it here so I
won't forget about it.
Test Plan: iiam
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1602
Differential Revision: https://secure.phabricator.com/D3240
Summary:
I quite often wonder what's inside these gaps displayed in changeset. In which method I am? Is it a while loop or a foreach? Maybe I'm in a class but the project doesn't have a sctrict policy of one class per file so what's the name of the class?
I've experimented with bunch of rules:
- Always display 0 indentation: useless for one class per file.
- Always display 1 indentation: weird inside global functions.
- Display closest lower indentation: works best.
I'm not sure about highlighting the context. I like highlighting but maybe in this case subtler monochrome text will work better.
Test Plan: Browsed bunch of diffs, loved it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3371
Summary:
Many images in Differential changesets are icons designed for
use on dark backgrounds. This makes them invisible on Differential's
white background. This adds an option to use a darker background
instead so you can see the images.
Currently this behavior is triggered on hover. (Also, it's a rather
garish fuchsia.) It seems fine UX-wise but I'm not totally sure of it.
Test Plan:
Load diff containing grippy_texture. Marvel at the grippy
fuchsia.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3272
Summary: This is a tax for internationalization.
Test Plan: Displayed a revision with added and moved files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3166
Summary:
We need `$this->old` and `$this->new` in `renderShield()`.
Broken since D2358.
Test Plan: Loaded contents of file with lots of changes.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3080
Summary:
Diff of diffs display changes between new versions of file.
This is bad after rebase because there can be many unrelated changes so it is hard to spot the real change.
This diff unhighlights the lines that were added or removed in rebase.
The changes are still visible (they can be sometimes relevant) but very subtle.
Test Plan:
# Add, change and delete line. Display diff.
# Add and change some lines in parent. Rebase. Display diff. Display diff of diff.
# Change and add some lines. Display diff. Display diff to first diff. Display diff to second diff.
Reviewers: epriestley, jungejason
Reviewed By: jungejason
CC: jungejason, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2761
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:
Some lint errors (e.g. Javelin) don't have a line number.
Put them on the first line.
Putting them above the first line would be even nicer but much more complicated.
Test Plan: Display diff with lint error on line 0 (D2583).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2599
Summary:
The highlighting is distracting according to Nick Shrock and others.
Real designer, Lee Byron, helped me with this.
It also gives us unagressive target for jumping to the source line in future.
Another feature I will probably implement is highlighting also the source of copies/moves.
I will use the right side of the left column for it.
Test Plan:
Hover copied notifier.
Hover coverage notifier.
I've also checked that this doesn't break our super-flaky old/new code JavaScript detector.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, leebyron, schrockn
Differential Revision: https://secure.phabricator.com/D2443
Summary:
The highlighting is distracting according to Nick Shrock and others.
Real designer, Lee Byron, helped me with this.
It also gives us unagressive target for jumping to the source line in future.
Another feature I will probably implement is highlighting also the source of copies/moves.
I will use the right side of the left column for it.
Test Plan:
Hover copied notifier.
Hover coverage notifier.
I've also checked that this doesn't break our super-flaky old/new code JavaScript detector.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin, leebyron, schrockn
Differential Revision: https://secure.phabricator.com/D2403
Summary:
It saves some time on non-highlighting generated and other not interesting code.
The code is quite complex (300 lines methods) so I'm not sure if everything is moved correctly.
P.S. I hope that moved code detector will work...
Test Plan:
Display generated file with all whitespace, verify that it is not highlighted.
Display normal file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1134
Differential Revision: https://secure.phabricator.com/D2358
Summary:
Also reduce the memory usage a little bit (before increasing it again).
I use the same CSS class as for the copied code.
Test Plan: Parsed 100 diffs and checked about 10 of them - looks good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2339
Summary: The color used for this feature is pretty important and I am bad with colors.
Test Plan:
View diff created by D2320 with some copied lines and one line changed:
{F10604, size=full}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2321
Summary: Inspired by D2242.
Test Plan:
Select text in left pane.
Select text in right pane.
Select all.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2249
Summary: NOTE: This is starting to be too hacky.
Test Plan:
View revision with inline diffs, verify that Reply is there.
View standalone - no Reply.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2263
Summary:
If I have Pygments enabled in config but `pygmentize` doesn't work then unhighlighted source is stored to cache.
If I later make `pygmentize` work then the unhighlighted source is still loaded from the cache.
Test Plan:
Break `pygmentize`.
View a diff with JS files.
Fix `pygmenize`.
View the diff again.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, gatos99, Koolvin
Differential Revision: https://secure.phabricator.com/D2227
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
Summary: I'll mark this one up inline since it's all separate bugs.
Test Plan:
- Created a diff with eight changes: (newline absent -> newline present, newline present -> newline absent, newline present -> newline present, newline absent -> newline absent) x (short file with change near end, long file with change near middle).
- Viewed diff in Ignore All, Ignore Most, Ignore Trailing and Show All whitespace modes.
- All 32 results seemed sensible.
- Really wish this stuff was better factored and testable. Need to fix it. :(
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1030
Differential Revision: https://secure.phabricator.com/D1992
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: Works in Safari, Firefox, Chrome.
Test Plan: Copied some text, threw up a little in my mouth.
Reviewers: aran, tuomaspelkonen, tomo, rstout, btrahan
Reviewed By: aran
CC: aran, epriestley, ddfisher
Maniphest Tasks: T145, T995
Differential Revision: https://secure.phabricator.com/D244
Summary: I've done the same stupid copy/paste mistake twice in one day :-(.
Test Plan: Display diff with no newline at end of file in new file.
Reviewers: davidreuss, epriestley
Reviewed By: davidreuss
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1911
Summary: Text "No newline at end of file" was sent to highlighter causing error with most languages.
Test Plan: Display diff containing file with no newline at end of file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1903
Summary:
It looks like there is really this text written e.g. at https://secure.phabricator.com/D1896#0a6a1957
I am not sure that it is the only place which needs to be fixed.
Test Plan: Display diff with no newline at end of file in Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1900
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: 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:
Currently, we mark some lines that otherwise count as "unchanged" to be "changed" when they have internal whitespace changes.
However, we've already excluded them from the intra-line diff algorithm. Unset the flag so they can get intra-line diffed.
Test Plan: Viewed a change with internal whitespace changes, internal whitespace changes were intraline diffed.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T970
Differential Revision: https://secure.phabricator.com/D1871
Summary:
We used to have "ignore all", but it became "ignore most". It does not ignore in-line whitespace changes or whitespace changes in language where whitespace is marked semantic.
Add an explicit "ignore all" option.
Test Plan: Used "ignore all" to view a change with heuristically-detected semantic whitespace, the change was ignored.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T970
Differential Revision: https://secure.phabricator.com/D1865
Summary: We currently parse these as directory changes and discard them. Instead, parse them as a new "SUBMODULE" type of change.
Test Plan:
- Reparsed a commit which changes submodules and verified it parses correctly.
- Reparsed a commit which adds submodules and verified it parses correctly.
Reviewers: btrahan, kdeggelman
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1815
Summary:
This just looks silly:
{F8088, size=full}
It runs in O(N*N) but it's not a big deal because there are usually only few
comments per line.
I didn't implement it for images.
Test Plan:
View revision with compatible inline comments in two diffs.
View revision with different inline comments on same line in two diffs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1570
Summary:
Render coverage information in the right gutter, if available.
We could render some kind of summary report deal too but this seems like a good
start.
Test Plan:
- Looked at diffs with coverage.
- Looked at diffs without coverage.
- Used inline comments, diff-of-diff, "show more", "show entire file", "show
generated file", "undo". Nothing seemed disrupted by the addition of a 5th
column.
Reviewers: btrahan, tuomaspelkonen, jungejason
Reviewed By: btrahan
CC: zeeg, aran, epriestley
Maniphest Tasks: T140
Differential Revision: https://secure.phabricator.com/D1527
Summary:
Not all auto-generated files can include the magical
"generated" annotation for one reason or another, but they may follow
path rules. This patch allows files to be marked as automatically
generated by matching the path with a regular expression.
Test Plan:
Alter 'differential.generated-paths' setting in config.
Create a new diff that affects a file matching one of those regular
expressions. Verify that Differential marks it as automatically
generated and therefore probably not worth reviewing (in the same way as
the magical "generated" annotation.
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1455
Summary: See D1433.
Test Plan: Created a new diff with a line >80chars, observed it wrapping
correctly.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1438
Summary: This is never read anywhere and clearly has no effect.
Test Plan: grep
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D1434
Summary: These blocks do nothing. end() produces a side effect on the internal
array pointer, but the code does not depend on it.
Test Plan: Reasoned about the code? Also viewed some diffs.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D1432
Summary: No callsites anywhere. Unclear what this method is even supposed to do.
Test Plan: grep
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D1435
Summary:
we used to need this function for security purposes, but no longer need
it. remove it so that some call sites can be optimized via smarter data
fetching, and so the whole codebase can have one less thing in it.
Test Plan:
verified the images displayed properly for each of the following
- viewed a diff with added images.
- viewed a user feed
- viewed a user profile
- viewed all image macros
- viewed a paste and clicked through "raw link"
weakness in testing around proxy files and transformed files. not sure what
these are. changes here are very programmatic however.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T672
Differential Revision: https://secure.phabricator.com/D1354
Summary: Clicks all the "Show All" links for you at the touch of a button.
Test Plan:
- Used "reveal entire file" on revealable files.
- Opened on already-visible files, got "entire file shown".
- Used other menu options.
- Used normal "show more" links.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T497
Differential Revision: https://secure.phabricator.com/D1331
Test Plan:
Created a listener that adds some patterns to $matches array, reloaded
Differential, some changesets were not shown as generated.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota
Differential Revision: 1200
Summary: The display of images pairs is not corresponding to the selected two
image diffs. The fix is to use reference to get the phid for each image.
Test Plan: Create a revision with two diffs of images.
Test the display between base and diff1/diff2.
Test the rendering of images between diff1 and diff2.
Test the inline comments also.
Reviewers: epriestley, jungejason
CC:
Differential Revision: 955
Summary: Added line number 1 for each image and added code to display the
comments for each image.
Test Plan: Adding an image in my local directory and create a revision for it.
Click line number 1, and the comment window prompts. Adding and save the
comment. The comment shows in the differential comment list and in the inline
comment. Submit the comment. Create more comments for the image and the
"Previous" and "Next" buttons all work well.
Reviewers: epriestley, jungejason
CC:
Differential Revision: 901
Summary: Delete one line which has no effect.
Test Plan: Open revision page to make sure it still works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 852
Summary:
I think this is the last major step -- use the fields to parse commit messages,
not a hard-coded list of stuff. This adds two primary methods to fields, one to
get all the labels they'll parse (so we can do "CC" and "CCs" and treat them as
the same field) and one to parse the string into a canonical representation
(e.g., lookup reviewers and such).
You'll need to impelement the one block of task-specific stuff I removed in
Facebook's task field:
list($pre_comment) = split(' -- ', $data);
$data = array_filter(preg_split('/[^\d]+/', $pre_comment));
foreach ($data as $k => $v) {
$data[$k] = (int)$v;
}
$data = array_unique($data);
break;
Otherwise I think this is clean.
Test Plan:
- Called the conduit method with various commit messages, parsed fields/errors
seemed correct.
- "arc diff"'d this diff onto localhost, then updated it.
- "arc amend"'d this diff.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 829
Summary: See T368. The current rendering result can cause some confusion for the
first/last chunks, make their behavior more explicit.
Test Plan:
- Clicked various "show more" links on a bunch of top/bottom/middle omitted
context blocks in a variety of diffs.
- Located a @generated shielded file and verified the initial render is
correct when the entire file is default-hidden.
Reviewed By: avitaloliver
Reviewers: avitaloliver, jungejason, tuomaspelkonen, aran
CC: aran, avitaloliver
Differential Revision: 744
Summary:
Python people don't seem to like the 'ignore-all' as default. Provide a way
to configure which file types should not use 'ignore-all'.
Test Plan:
Tested that it worked with bunch of Python of files and non-python
files. Cache was disabled during the test.
Reviewed By: jungejason
Reviewers: epriestley, jungejason
Commenters: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 713
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
Summary:
This thing services every app but it lives inside Differential right now. Pull
it out, and separate the factory interfaces per-application.
This will let us accommodate changes we need to make for Phriction to support
wiki linking.
Test Plan: Tested remarkup in differential, diffusion, maniphest, people,
slowvote.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 646
Summary:
Add the differential parse cache to the GC. This is the largest object in the
system by a wide margin, I think.
This table is potentially gigantic which is why the script truncates it before
doing a schema change.
Test Plan: Ran the GC daemon, it cleaned up some parse caches.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: tuomaspelkonen
CC: aran, jungejason, tuomaspelkonen, epriestley
Differential Revision: 620
Summary: Reduce the amount of code duplication here and allow for an override
configuration on the filename.map stuff.
Test Plan: Checked paste, diffusion and differential syntax highlighting and
everything appeared reasonable.
Reviewed By: codeblock
Reviewers: tuomaspelkonen, codeblock, jungejason, aran
CC: aran, codeblock, epriestley
Differential Revision: 601
Summary: I'll clean some of this stuff up in a followup too, but update the
callers to use the new explicit filename-based API.
Test Plan: Looked at paste, Diffusion and Differential.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, codeblock, jungejason, aran
CC: aran, tuomaspelkonen
Differential Revision: 600
