mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-10 00:42:41 +01:00
Fix an issue where text intradiff bodies may not render
Summary: Ref T13523. In the caching layer, there's a tricky clause about filetypes that skips some body rendering behavior. Provide file type information which at least has a better chance of representing all changes (e.g., an image file may be replaced with a text file, but this can not be represented by a single file type). Formalize "hasSourceTextBody()", to mean the changeset parser should engage the change as source text. Test Plan: Intradiffed text changes, saw the body render properly. Maniphest Tasks: T13523 Differential Revision: https://secure.phabricator.com/D21210
This commit is contained in:
parent
dade977307
commit
17426a60f0
2 changed files with 62 additions and 15 deletions
|
@ -592,6 +592,17 @@ final class DifferentialChangesetParser extends Phobject {
|
|||
}
|
||||
|
||||
private function tryCacheStuff() {
|
||||
$changeset = $this->getChangeset();
|
||||
if (!$changeset->hasSourceTextBody()) {
|
||||
|
||||
// TODO: This isn't really correct (the change is not "generated"), the
|
||||
// intent is just to not render a text body for Subversion directory
|
||||
// changes, etc.
|
||||
$this->markGenerated();
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
$viewstate = $this->getViewState();
|
||||
|
||||
$skip_cache = false;
|
||||
|
@ -610,19 +621,10 @@ final class DifferentialChangesetParser extends Phobject {
|
|||
$skip_cache = true;
|
||||
}
|
||||
|
||||
$changeset = $this->changeset;
|
||||
|
||||
if ($changeset->getFileType() != DifferentialChangeType::FILE_TEXT &&
|
||||
$changeset->getFileType() != DifferentialChangeType::FILE_SYMLINK) {
|
||||
|
||||
$this->markGenerated();
|
||||
|
||||
} else {
|
||||
if ($skip_cache || !$this->loadCache()) {
|
||||
$this->process();
|
||||
if (!$skip_cache) {
|
||||
$this->saveCache();
|
||||
}
|
||||
if ($skip_cache || !$this->loadCache()) {
|
||||
$this->process();
|
||||
if (!$skip_cache) {
|
||||
$this->saveCache();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
@ -32,6 +32,8 @@ final class DifferentialChangeset
|
|||
private $hasNewState;
|
||||
private $oldStateMetadata;
|
||||
private $newStateMetadata;
|
||||
private $oldFileType;
|
||||
private $newFileType;
|
||||
|
||||
const TABLE_CACHE = 'differential_changeset_parse_cache';
|
||||
|
||||
|
@ -550,17 +552,20 @@ final class DifferentialChangeset
|
|||
$left_metadata = $left->getNewStateMetadata();
|
||||
$left_state = $left->hasNewState();
|
||||
$shared_metadata = $left->getMetadata();
|
||||
$left_type = $left->getNewFileType();
|
||||
if ($right) {
|
||||
$right_data = $right->makeNewFile();
|
||||
$right_properties = $right->getNewProperties();
|
||||
$right_metadata = $right->getNewStateMetadata();
|
||||
$right_state = $right->hasNewState();
|
||||
$shared_metadata = $right->getMetadata();
|
||||
$right_type = $right->getNewFileType();
|
||||
} else {
|
||||
$right_data = $left->makeOldFile();
|
||||
$right_properties = $left->getOldProperties();
|
||||
$right_metadata = $left->getOldStateMetadata();
|
||||
$right_state = $left->hasOldState();
|
||||
$right_type = $left->getOldFileType();
|
||||
}
|
||||
|
||||
$engine = new PhabricatorDifferenceEngine();
|
||||
|
@ -576,7 +581,6 @@ final class DifferentialChangeset
|
|||
->setFilename($right->getFilename());
|
||||
|
||||
// TODO: Change type?
|
||||
// TODO: File type?
|
||||
// TODO: Away paths?
|
||||
// TODO: View state key?
|
||||
|
||||
|
@ -589,7 +593,9 @@ final class DifferentialChangeset
|
|||
->setOldStateMetadata($left_metadata)
|
||||
->setNewStateMetadata($right_metadata)
|
||||
->setHasOldState($left_state)
|
||||
->setHasNewState($right_state);
|
||||
->setHasNewState($right_state)
|
||||
->setOldFileType($left_type)
|
||||
->setNewFileType($right_type);
|
||||
|
||||
// NOTE: Some metadata is not stored statefully, like the "generated"
|
||||
// flag. For now, use the rightmost "new state" metadata to fill in these
|
||||
|
@ -602,6 +608,45 @@ final class DifferentialChangeset
|
|||
return $comparison;
|
||||
}
|
||||
|
||||
|
||||
public function setNewFileType($new_file_type) {
|
||||
$this->newFileType = $new_file_type;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function getNewFileType() {
|
||||
if ($this->newFileType !== null) {
|
||||
return $this->newFileType;
|
||||
}
|
||||
|
||||
return $this->getFiletype();
|
||||
}
|
||||
|
||||
public function setOldFileType($old_file_type) {
|
||||
$this->oldFileType = $old_file_type;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function getOldFileType() {
|
||||
if ($this->oldFileType !== null) {
|
||||
return $this->oldFileType;
|
||||
}
|
||||
|
||||
return $this->getFileType();
|
||||
}
|
||||
|
||||
public function hasSourceTextBody() {
|
||||
$type_map = array(
|
||||
DifferentialChangeType::FILE_TEXT => true,
|
||||
DifferentialChangeType::FILE_SYMLINK => true,
|
||||
);
|
||||
|
||||
$old_body = isset($type_map[$this->getOldFileType()]);
|
||||
$new_body = isset($type_map[$this->getNewFileType()]);
|
||||
|
||||
return ($old_body || $new_body);
|
||||
}
|
||||
|
||||
public function getNewStateMetadata() {
|
||||
return $this->getMetadataWithPrefix('new:');
|
||||
}
|
||||
|
|
Loading…
Reference in a new issue