1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-25 08:12:40 +01:00
Commit graph

41 commits

Author SHA1 Message Date
epriestley
e480233eb8 Remove UTF-8 kludges from Differential
Summary:
This depends on D513 and D514. Those diffs make the display algorithms safe with
respect to mutating utf8, so we no longer need to repair potentially invalid
utf8 sequences with this hack.

Test Plan:
grepped for calls to this method

Reviewers: jungejason, aran, tuomaspelkonen
Commenters: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 515
2011-06-26 11:58:34 -07:00
epriestley
5cfc14cb43 Make Differential linewrap utf-8 aware
Summary:
Differential uses a byte-oriented linewrap algorithm. Instead, use a
character-oriented one which will handle utf-8 properly.

This implies a very slightly performance hit but we only run this code for lines
which need to wrap, and the results get cached. It took about ~2.5ms for the
test file on my machine. I'll keep an eye on it but I think it's currently a
manageable cost.

Test Plan:
Diffed this file: https://secure.phabricator.com/P43
...and got it to render like this:
https://secure.phabricator.com/file/info/PHID-FILE-331ac241bede705b193b/

To do so, I had to disable the un-utf8 block which we can't actually do yet
because of intraline diff, but it shows that once we can get rid of that it
works completely correctly. It will "sort of" work in the meantime (nothing
terrible happens).

Reviewers: jungejason, aran, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 513
2011-06-26 11:58:27 -07:00
epriestley
b664d67a22 Revert "Make Differential linewrap utf-8 aware"
This shouldn't have landed yet.

This reverts commit e5a036e8c9.
2011-06-24 10:48:37 -07:00
epriestley
3fc817d088 Revert "Remove UTF-8 kludges from Differential"
This shouldn't have landed.

This reverts commit fe04d8bf70.
2011-06-24 10:46:30 -07:00
epriestley
fe04d8bf70 Remove UTF-8 kludges from Differential
Summary: This depends on D513 and D514. Those diffs make the display algorithms safe with respect to mutating utf8, so we no longer need to repair potentially invalidate utf8 sequences with this hack.

Test Plan: grepped for calls to this method

Reviewers: jungejason, aran, tuomaspelkonen

CC:

Differential Revision: 515
2011-06-24 10:18:07 -07:00
epriestley
e5a036e8c9 Make Differential linewrap utf-8 aware
Summary: Differential uses a byte-oriented linewrap algorithm. Instead, use a character-oriented one which will handle utf-8 properly.

This implies a very slightly performance hit but we only run this code for lines which need to wrap, and the results get cached. It took about ~2.5ms for the test file on my machine. I'll keep an eye on it but I think it's currently a manageable cost.

Test Plan: Diffed this file: https://secure.phabricator.com/P43
...and got it to render like this: https://secure.phabricator.com/file/info/PHID-FILE-331ac241bede705b193b/

To do so, I had to disable the un-utf8 block which we can't actually do yet because of intraline diff, but it shows that once we can get rid of that it works completely correctly. It will "sort of" work in the meantime (nothing terrible happens).

Reviewers: jungejason, aran, tuomaspelkonen

CC:

Differential Revision: 513
2011-06-24 09:41:45 -07:00
epriestley
9844bbb4f9 Don't use the "ignore all whitespace" algorithm on multi-hunk diffs
Summary:
Diffs with missing context don't render properly in the "ignore all whitespace"
algorith, so don't try to use it. These diffs can occur if someone creates a
diff via the web interface, for example, or if they muck around in their copy of
'arc'.

See D473, T246 (a problem with D473), rPe5bb756b5191720 (revert of D473) and
T231.

Test Plan:
Viewed a diff with missing context from the web interface. Verified normal diffs
still rendered with all whitespace ignored.

Reviewed By: fratrik
Reviewers: jungejason, aran, tuomaspelkonen, fratrik
Commenters: jungejason
CC: aran, epriestley, fratrik, jungejason
Differential Revision: 500
2011-06-23 16:55:04 -07:00
epriestley
d031d3ae32 Slightly improve UTF-8 handling in Differential
Summary:
See comments. I think this will fix the issue, where we end up handling off
garbage to htmlspecialchars() after highlighting a file we've stuck full of \0
bytes.

The right fix for this is to make wordwrap and intraline-diff utf8 aware and
throw this whole thing away. I'll work on that but I think this fixes the
immediate issue.

Test Plan:
diffed the file with a UTF-8 quote in it and got a reasonable render in
Differential

Reviewed By: jungejason
Reviewers: jungejason, aran, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 504
2011-06-23 12:28:16 -07:00
epriestley
e5bb756b51 Revert "Fixed 'Create Diff'"
See T246.

This reverts commit 6b7db27524.
2011-06-22 15:49:57 -07:00
epriestley
4a55af7857 Don't repeat the diff property table when clicking "show more"
Summary:
If you "chmod +x" a file and then generate a diff and click "show 20 lines" on
that diff, you get another janky copy of the property table. Render the property
table only for top-level rendering requests.

This started happening after D409, which fixed the far-more-obvious bug of these
things never showing up. We must have changed the logic at some point since this
side effect was surprising to me. :P

Test Plan:
Created a diff with changes and +x, clicked "show 20 lines".

  - Original diff had file property header table thing, showing the +x.
  - New context brought in by "show 20 lines" didn't have it anymore.

Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, jungejason
Differential Revision: 482
2011-06-20 15:54:20 -07:00
tuomaspelkonen
6b7db27524 Fixed 'Create Diff'
Summary:
'Create Diff' with whitespace mode 'ignore-all' is borken, because the
line numbers get mixed up when creating a second diff for the
whitespace changes.

This should be fixed correctly at some point, but currently the
whitespace 'ignore-all' says 'Huge mess' in the comments and I didn't
want to make the mess any bigger.

Test Plan:
Tested that 'Create Diff' showed the diff correctly and a previously
created diff looked correct once the cache was disabled.

Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 473
2011-06-18 15:33:22 -07:00
epriestley
4ec31ef75c Provide basic capabilities to make Differential column width flexible
Summary:
- Make wrap width settable in PHP.
  - Dynamically generate max-width based on configurable maximum width.
  - Constrain non-diff elements to standard width.
  - Provide a configuration setting.

Test Plan:
Set various things to 100 / 120, as far as I could tell everything seemed to
render sensibly? This should have no effect on 80-col changes.

Reviewed By: jdperlow
Reviewers: jdperlow, tuomaspelkonen, jungejason, aran
CC: aran, jdperlow
Differential Revision: 413
2011-06-09 12:01:11 -07:00
epriestley
9284c85876 Display property changes on files
Summary:
Never converted this TODO over from XHP.

NOTE: This looks terrible since the CSS didn't make it over, can one of you grab
the rules for .differential-property-table and .property-table-header? If they
aren't still in trunk, try history for html/intern/css/tools/differential/

Test Plan:
Made a property change, looked at it in Differentila.

Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: codeblock, aran
Differential Revision: 409
2011-06-07 19:58:15 -07:00
epriestley
404c3283cb Don't ignore internal whitespace changes in "Whitespace: Ignore All"
Summary:
When whitespace changes between two non-whitespace characters (e.g., in a
string), always treat it as a change.

Test Plan:
Disabled render cache, made internal and external whitespace changes, rendered a
diff, got internal change always marked and external change marked correctly
depending on mode.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 403
2011-06-07 11:14:16 -07:00
epriestley
729d2f9c93 Remove .sql3 hacks from Differential
Summary:
See T178. D372 is the correct fix for this problem, hardcoding .sql3 is not.

Test Plan:
This code should be unreachable after T178 since these files will always be
marked as binary.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: elgenie, aran, tuomaspelkonen
Differential Revision: 373
2011-05-31 12:14:58 -07:00
jungejason
686ffafa21 Improve logging for syntax highlighting parsing
Summary:
add logging when syntax highlighting parsing throws exception.

Test Plan:
test when exception is thrown with non-php code. I couldn't
create a file to trigger an exception in running pygmentiza, so I
manually threw an exception to test it.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, epriestley
CC: aran, tuomaspelkonen, jungejason
Differential Revision: 350
2011-05-26 17:51:22 -07:00
epriestley
b77e827bec Fix metadata rendering for files moves, etc.
Summary:
Some changeset metadata was not being correctly passed between the top-level
parser and the subparser, so it would be lost or incorrect when rendering
headers like "This file was moved from x to y." or rendering certain content
shields, like "the contents of this file were not modified".

Test Plan:
Created a new diff with a file move in it, rendered it, saw "This file was moved
from README to READYOU" correctly.

Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, grglr, aran
CC: aran, epriestley
Differential Revision: 321
2011-05-22 07:23:48 -07:00
epriestley
d202e71ef1 Use parallel syntax highlighting API in differential
Summary:
Use the new API from D322 to highlight text in parallel in Differential.

Test Plan:
Verified that pygemntize calls started within 20ms of one another in DarkConsole
(also: added a feature to let me do this) instead of running serially.

Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 323
2011-05-22 07:21:10 -07:00
epriestley
54154e4f48 Move "Rendering References" to the DifferentialChangesetParser level
Summary:
Separates changeset IDs from rendering. Now each changeset has a "rendering
reference" which is basically a description of what the ajax endpoint should
render. For Differential, it's in the form "id/vs". For Diffusion,
"branch/path;commit".

I believe this fixes pretty much all of the bugs related to "show more" breaking
in various obscure ways, although I never got a great repro for T153.

Test Plan:
Clicked "show more" in diffusion change and commit views and differential diff,
diff-of-diff, standalone-diff, standalone-diff-of-diff views. Verified refs and
'whitespace' were always sent correctly.

Made inline comments on diffs and diffs-of-diffs. Used "Reply".

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 274
2011-05-13 06:10:57 -07:00
tuomaspelkonen
d5dfa5faf5 Never hide file contents if they have comments.
Summary:
Links to comments were not working because file was hidden after it was deleted.

Test Plan:
Tested that comment anchors were working correctly for deleted files.
Tested that generated files were still hidden.

Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, aran, epriestley
Differential Revision: 257
2011-05-09 18:01:22 -07:00
epriestley
3ee7b5380e Revert "Revert "Make DifferentialChangesetParser explicitly map display to storage for comments""
This reverts commit 1c2222f26f.
2011-05-08 21:22:25 -07:00
tuomaspelkonen
1c2222f26f Revert "Make DifferentialChangesetParser explicitly map display to storage for comments"
This reverts commit a0af5b6643.
2011-05-06 18:32:28 -07:00
epriestley
a0af5b6643 Make DifferentialChangesetParser explicitly map display to storage for comments
Summary:
This is a followup to D228. Basically, we use "changeset" (and, implicitly,
changesetID) for way too much stuff now.

One thing it can't possibly capture is the complete, arbitrary mapping between
the left and right sides of the displayed diff and the places we want to store
the left and right side comments. This causes a bunch of bugs; basically adding
inline comments is completely broken in diff-of-diff views prior to this patch.
Make this mapping explicit.

Note that the renderer already passes this mapping to
DifferentialChangesetParser which is why there are no changes outside this file,
I just didn't finish the implementation during the port.

This has the nice side-effect of fixing T132 and several other bugs.

Test Plan:
Made new-file and old-file comments on a normal diff; reloaded page, verified
comments didn't do anything crazy.

Expanded text on a normal diff, made new-file and old-file comments; reloaded
page, verified comments.

Repeated these steps for a previous diff in the same revision; verified
comments.

Loaded diff-of-diffs and verified expected comments appeared. Made new left and
right hand side comments, which almost work, see below.

NOTE: There is still a bug where comments made in the left-display-side of a
diff-of-diffs will incorrectly be written to the right-storage-side of the
right-display-side diff. However, this is an issue with the JS (the PHP is
correct) so I want to pull it out of scope for this patch since I think I need
to fix some other JS stuff too and this improves the overall state of the world
even if not everything is fixed.

Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran, ola
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 237
2011-05-06 15:01:52 -07:00
epriestley
af06bfb1cc Make Changeset ID for render cache explicit
Summary:
DifferentialChangesetParser currently takes the Changeset object to mean a bunch
of different and mutually conflicting things implicitly:

  - Changeset ID is used to access the render cache.
  - Changeset ID is also used to tell the ajax endpoint what to render when
clicking "show more".
  - Changeset object has the actual changes.
  - Changeset ID and "oldChangesetID" are used to choose where to show inline
comments and how to attach new ones.

This indirectly causes a bunch of problems, like T141 and T132. Move toward
making all these separate things explicit. I want to have the changeset object
only mean the actual changes to display.

Test Plan:
Looked at changesets and verified the render cache was accessed correctly (and
not accessed in other cases).

Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 228
2011-05-05 11:12:50 -07:00
epriestley
fc2a2a8d09 Enable Pygments in actual source list renders
Summary:
I only actually enabled it in Remarkup previously.

Test Plan:
Created a python diff, got syntax highlighted.

Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 202
2011-05-02 14:26:24 -07:00
grglr
3f3304fd61 Differential whitespace mode IGNORE ALL now shows correct indentation
Summary:
Fixed buggy and incomplete logic for handling IGNORE ALL mode properly.
A subparser is used to parse the non-ws-ignoring changeset while a
ws-ignoring changeset is handed off to the original parser.  At a later
step, the original parser queries the subparser for its lines of text
(which are formatted properly due to being in non-ws-ignoring mode) and
uses them to replace the text in the ws-ignoring diff.

Task ID: 549940

Test Plan:
-turn off caching temporarily (the cached view is still indented
improperly)
-visit http://phabricator.dev1943.facebook.com/D242591
-note aligned, but not completely highlighted, indentation right above the
comments complaining about
indentation issues

Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: epriestley
CC: aran, epriestley, grglr
Differential Revision: 174
2011-04-28 16:10:59 -07:00
tuomaspelkonen
a6c770217d Fixed whitespace options in Differential.
Summary:
Whitespace options could not be selected and the 'ignore_all' was not working.

Test Plan:
Made sure that whitespace is ignored correctly and selected option is handled
correctly.

Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, jungejason, tuomaspelkonen
Differential Revision: 149
2011-04-19 23:07:44 -07:00
tuomaspelkonen
28fe9f4eca User preferences ported from tools
Summary:

Internal tools, e.g., differential and diffusion  have user defined
preferences for monospaced font and the option for showing either the
name of the tool or the glyph of the tool in the title.

These preferences were ported to phabricator. These preferences can be
modified in /preferences/ and they both affect diffusion and differential
at the moment.

Test Plan:

* Created an empty database
* Loaded /preferences/ and modified the monospaced font and clicked save
* Confirmed that the same page was loaded with the message that preferences
  have been saved and that the example text used the user defined font

* in /preferences/ changed the option to show tool names as plain text and
  clicked save
* Confirmed that the same page was loaded with '[Preferences]' in the title
  instead of a glyph

* These same tests were also executed for differential and diffusion

Reviewers: epriestley
CC: jungejason

Differential Revision: 91
2011-03-31 13:44:20 -07:00
epriestley
60918c9a9e Improved change view support. 2011-03-30 19:22:11 -07:00
epriestley
eccc76dae6 Fix some issues caught by HipHop, and work around some issues
caused by HipHop.
2011-02-26 21:01:42 -08:00
epriestley
905870d793 Various fixes, particularly on the revision update workflow. 2011-02-04 17:53:14 -08:00
epriestley
12df78ed6a Rough cut of diff-of-diffs. 2011-02-03 15:41:58 -08:00
epriestley
5fd28d35d9 Diff-of-diffs radio button logic. 2011-02-03 13:26:52 -08:00
epriestley
c93dd9c090 Flesh out some DarkConsole stuff. 2011-02-02 22:38:42 -08:00
epriestley
c5ce156e71 Edit/Delete for inline comments 2011-02-02 13:51:45 -08:00
epriestley
759eec3a77 Very rough cut of DarkConsole + XHProf 2011-02-02 13:48:52 -08:00
epriestley
246cba2bf0 InlineComments 2011-02-01 21:09:28 -08:00
epriestley
233953bc4a Straighten out the "show more context" stuff. 2011-01-31 20:38:13 -08:00
epriestley
2b7b03ad0c ChangesetParseCache 2011-01-25 16:10:48 -08:00
epriestley
52126c5479 Add proper syntax highlighting with Phutil. 2011-01-24 17:39:14 -08:00
epriestley
a3df19976f DifferentialChangesetView 2011-01-24 17:24:40 -08:00