1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-14 19:02:41 +01:00
Commit graph

21 commits

Author SHA1 Message Date
epriestley
67498f3334 Restore phutil_split_lines() to Differential
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
2012-10-20 07:24:27 -07:00
epriestley
468aeaef0d Revert "Use phutil_split_lines() in Differential"
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
2012-10-20 06:42:08 -07:00
epriestley
f6cb51562e Use phutil_split_lines() in Differential
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
2012-10-20 06:13:24 -07:00
Wez Furlong
1b6eced757 avoid claiming that the source of a file move was unchanged
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
2012-10-09 14:16:21 -07:00
vrana
58206a146f Fix displaying gap context when showing first lines
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
2012-10-04 09:07:47 -07:00
vrana
377a29644f Display shield for generated binary files
Test Plan:
Displayed diff with generated images.

Should hide [[ https://secure.phabricator.com/D3579?vs=on&id=7076&whitespace=show-all#bfb5f5e5 | webroot/rsrc/image/autosprite.png ]] after deploy.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3585
2012-10-01 15:18:43 -07:00
vrana
958e6cd109 Add missing break statement 2012-09-06 15:12:57 -07:00
vrana
0f9f8b5f30 Tidy context displayed in gap
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
2012-08-29 17:25:17 -07:00
vrana
8b715a64b5 Don't waste too much cache by a single changeset
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
2012-08-27 16:51:05 -07:00
Alan Huang
5a0b640ccd Bump cache version
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
2012-08-23 16:16:19 -07:00
vrana
ca1775b468 Display context in changeset gaps
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
2012-08-23 08:57:52 -07:00
vrana
ea5d9d40e1 Delete empty command 2012-08-22 22:54:00 -07:00
Alan Huang
6fc01aa5fd Change background for image views in Differential
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
2012-08-13 17:21:16 -07:00
vrana
251438b2c2 Translate change type header
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
2012-08-06 12:07:26 -07:00
vrana
aefc5654c3 Load contents of shielded files with lots of changes
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
2012-07-26 13:28:10 -07:00
vrana
00160ab2a3 Remove ambiguous test 2012-07-18 18:31:34 -07:00
vrana
273211ae3e Highlight also lines that were added on the other side in diff of diffs 2012-06-29 18:19:01 -07:00
vrana
a2f4d661b9 Fix computing line numbers in diff of diffs
Test Plan: Live in prod.

Auditors: jungejason
2012-06-29 18:03:26 -07:00
vrana
99df72b81c Unhighlight lines modified in rebase
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
2012-06-29 17:21:23 -07:00
vrana
bf10eaf81c Wrap Differential property changes 2012-06-19 14:15:40 -07:00
vrana
6cc196a2e5 Move files in Phabricator one level up
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.

NOTE: `arc diff` timed out so I'm pushing it without review.

Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.

Auditors: epriestley

Maniphest Tasks: T1103
2012-06-01 12:32:44 -07:00
Renamed from src/applications/differential/parser/changeset/DifferentialChangesetParser.php (Browse further)