mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-02 03:32:42 +01:00
Remove "Large Changes" documentation and make some minor behavioral improvements
Summary: Depends on D19296. Ref T13110. - Remove the "Large Changesets" documentation since we now degrade very large changesets and I don't have any evidence that anyone has ever tried to follow any of the recommendations in this document. - Remove references to it. - When an older revision doesn't have denormalized size information on the Revision object itself, don't render a scale element (instead of rendering a bogus one). - Try to improve terminology consistency around "Large Change" (100-1000 files) vs "Very Large Change" (1000+ files) vs "Enormous Change" (too large to hold in memory). Test Plan: Viewed revisions; grepped for documentation. Maniphest Tasks: T13110 Differential Revision: https://secure.phabricator.com/D19298
This commit is contained in:
parent
1b363a831e
commit
f01c2e3694
7 changed files with 16 additions and 65 deletions
|
@ -56,10 +56,7 @@ final class DifferentialCreateRawDiffConduitAPIMethod
|
||||||
throw new Exception(
|
throw new Exception(
|
||||||
pht(
|
pht(
|
||||||
'The raw diff you have submitted is too large to parse (it affects '.
|
'The raw diff you have submitted is too large to parse (it affects '.
|
||||||
'more than %s paths and hunks). Differential should only be used '.
|
'more than %s paths and hunks).',
|
||||||
'for changes which are small enough to receive detailed human '.
|
|
||||||
'review. See "Differential User Guide: Large Changes" in the '.
|
|
||||||
'documentation for more information.',
|
|
||||||
new PhutilNumber($raw_limit)));
|
new PhutilNumber($raw_limit)));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -158,7 +158,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
|
||||||
if (count($changesets) > $limit && !$large) {
|
if (count($changesets) > $limit && !$large) {
|
||||||
$count = count($changesets);
|
$count = count($changesets);
|
||||||
$warning = new PHUIInfoView();
|
$warning = new PHUIInfoView();
|
||||||
$warning->setTitle(pht('Very Large Diff'));
|
$warning->setTitle(pht('Large Diff'));
|
||||||
$warning->setSeverity(PHUIInfoView::SEVERITY_WARNING);
|
$warning->setSeverity(PHUIInfoView::SEVERITY_WARNING);
|
||||||
$warning->appendChild(hsprintf(
|
$warning->appendChild(hsprintf(
|
||||||
'%s <strong>%s</strong>',
|
'%s <strong>%s</strong>',
|
||||||
|
|
|
@ -742,6 +742,12 @@ final class DifferentialRevision extends DifferentialDAO
|
||||||
return $this->getProperty(self::PROPERTY_LINES_REMOVED);
|
return $this->getProperty(self::PROPERTY_LINES_REMOVED);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function hasLineCounts() {
|
||||||
|
// This data was not populated on older revisions, so it may not be
|
||||||
|
// present on all revisions.
|
||||||
|
return isset($this->properties[self::PROPERTY_LINES_ADDED]);
|
||||||
|
}
|
||||||
|
|
||||||
public function getRevisionScaleGlyphs() {
|
public function getRevisionScaleGlyphs() {
|
||||||
$add = $this->getAddedLineCount();
|
$add = $this->getAddedLineCount();
|
||||||
$rem = $this->getRemovedLineCount();
|
$rem = $this->getRemovedLineCount();
|
||||||
|
|
|
@ -109,7 +109,10 @@ final class DifferentialRevisionListView extends AphrontView {
|
||||||
$item->setHeader($revision->getTitle());
|
$item->setHeader($revision->getTitle());
|
||||||
$item->setHref($revision->getURI());
|
$item->setHref($revision->getURI());
|
||||||
|
|
||||||
$item->addAttribute($this->renderRevisionSize($revision));
|
$size = $this->renderRevisionSize($revision);
|
||||||
|
if ($size !== null) {
|
||||||
|
$item->addAttribute($size);
|
||||||
|
}
|
||||||
|
|
||||||
if ($revision->getHasDraft($viewer)) {
|
if ($revision->getHasDraft($viewer)) {
|
||||||
$draft = id(new PHUIIconView())
|
$draft = id(new PHUIIconView())
|
||||||
|
@ -193,6 +196,10 @@ final class DifferentialRevisionListView extends AphrontView {
|
||||||
}
|
}
|
||||||
|
|
||||||
private function renderRevisionSize(DifferentialRevision $revision) {
|
private function renderRevisionSize(DifferentialRevision $revision) {
|
||||||
|
if (!$revision->hasLineCounts()) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
$size = array();
|
$size = array();
|
||||||
|
|
||||||
$glyphs = $revision->getRevisionScaleGlyphs();
|
$glyphs = $revision->getRevisionScaleGlyphs();
|
||||||
|
|
|
@ -74,9 +74,6 @@ often well-correlated with complexity.
|
||||||
We generally follow these practices in Phabricator. The median change size for
|
We generally follow these practices in Phabricator. The median change size for
|
||||||
Phabricator is 35 lines.
|
Phabricator is 35 lines.
|
||||||
|
|
||||||
See @{article:Differential User Guide: Large Changes} for information about
|
|
||||||
reviewing big checkins.
|
|
||||||
|
|
||||||
= Write Sensible Commit Messages =
|
= Write Sensible Commit Messages =
|
||||||
|
|
||||||
There are lots of resources for this on the internet. All of them say pretty
|
There are lots of resources for this on the internet. All of them say pretty
|
||||||
|
|
|
@ -66,8 +66,6 @@ Continue by:
|
||||||
- diving into the details of inline comments in
|
- diving into the details of inline comments in
|
||||||
@{article:Differential User Guide: Inline Comments}; or
|
@{article:Differential User Guide: Inline Comments}; or
|
||||||
- reading the FAQ at @{article:Differential User Guide: FAQ}; or
|
- reading the FAQ at @{article:Differential User Guide: FAQ}; or
|
||||||
- learning about handling large changesets in
|
|
||||||
@{article:Differential User Guide: Large Changes}; or
|
|
||||||
- learning about test plans in
|
- learning about test plans in
|
||||||
@{article:Differential User Guide: Test Plans}; or
|
@{article:Differential User Guide: Test Plans}; or
|
||||||
- learning more about Herald in @{article:Herald User Guide}.
|
- learning more about Herald in @{article:Herald User Guide}.
|
||||||
|
|
|
@ -1,54 +0,0 @@
|
||||||
@title Differential User Guide: Large Changes
|
|
||||||
@group userguide
|
|
||||||
|
|
||||||
Dealing with huge changesets, and when **not** to use Differential.
|
|
||||||
|
|
||||||
= Overview =
|
|
||||||
|
|
||||||
When you want code review for a given changeset, Differential is not always the
|
|
||||||
right tool to use. The rule of thumb is that you should only send changes to
|
|
||||||
Differential if you expect humans to review the actual differences in the source
|
|
||||||
code from the web interface. This should cover the vast majority of changes but,
|
|
||||||
for example, you usually should //not// submit changes like these through
|
|
||||||
Differential:
|
|
||||||
|
|
||||||
- Committing an entire open source project to a private repo somewhere so
|
|
||||||
you can fork it or link against it.
|
|
||||||
- Committing an enormous text datafile, like a list of every English word or a
|
|
||||||
dump of a database.
|
|
||||||
- Making a trivial (e.g., find/replace or codemod) edit to 10,000 files.
|
|
||||||
|
|
||||||
You can still try submitting these kinds of changes, but you may encounter
|
|
||||||
problems getting them to work (database or connection timeouts, for example).
|
|
||||||
Differential is pretty fast and scalable, but at some point either it or the
|
|
||||||
browser will break down: you simply can't show nine million files on a webpage.
|
|
||||||
|
|
||||||
More importantly, in all these cases, the text of the changes won't be reviewed
|
|
||||||
by a human. The metadata associated with the change is what needs review (e.g.,
|
|
||||||
what are you checking in, where are you putting it, and why? Does the change
|
|
||||||
make sense? In the case of automated transformations, what script did you use?).
|
|
||||||
To get review for these types of changes, one of these strategies will usually
|
|
||||||
work better than trying to get the entire change into Differential:
|
|
||||||
|
|
||||||
- Send an email/AIM/IRC to your reviewer(s) like "Hey, I'm going to check in
|
|
||||||
the source for MySQL 9.3.1 to /full/path/to/whatever. The change is staged
|
|
||||||
in /home/whatever/path/somewhere if you want to take a look. Can I put your
|
|
||||||
name on the review?". This is best for straightforward changes. The reviewer
|
|
||||||
is not going to review MySQL's source itself, instead they are reviewing the
|
|
||||||
change metadata: which version are you checking in, why are you checking it
|
|
||||||
in, and where are you putting it? You won't be able to use "arc commit" or
|
|
||||||
"arc amend" to actually push the change. Just use "svn" or "git" and
|
|
||||||
manually edit the commit message instead. (It is normally sufficient to add
|
|
||||||
a "Reviewed By: <username>" field.)
|
|
||||||
- Create a Differential revision with only the metadata, like the script you
|
|
||||||
used to make automated changes or a text file explaining what you're doing,
|
|
||||||
and maybe a sample of some of the changes if they were automated. Include a
|
|
||||||
link to where the changes are staged so reviewers can look at the actual
|
|
||||||
changeset if they want to. This is best for more complicated changes, since
|
|
||||||
Differential can still be used for discussion and provide a permanent record
|
|
||||||
others can refer to. Once the revision is accepted, amend your local commit
|
|
||||||
(e.g. by `git commit --amend`) with the real change and push as usual.
|
|
||||||
|
|
||||||
These kinds of changes are generally rare and don't have much in common, which
|
|
||||||
is why there's no explicit support for them in Differential. If you frequently
|
|
||||||
run into cases which Differential doesn't handle, let us know what they are.
|
|
Loading…
Reference in a new issue