Skip to content

Commit 38ef910

Browse files
author
epriestley
committedMar 15, 2021
Move "Affected Path" index updates to a separate class
Summary: Ref T13639. Move operations related to updating the "AffectedPath" index to a dedicated class. This change has no functional effect and only moves code. Test Plan: Used an external script to rebuild every revision index; destroyed a revision with `bin/remove destroy`. Maniphest Tasks: T13639 Differential Revision: https://secure.phabricator.com/D21616
1 parent e919b4c commit 38ef910

File tree

4 files changed

+148
-97
lines changed

4 files changed

+148
-97
lines changed
 

‎src/__phutil_library_map__.php

+2
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,7 @@
450450
'DifferentialActionEmailCommand' => 'applications/differential/command/DifferentialActionEmailCommand.php',
451451
'DifferentialAdjustmentMapTestCase' => 'applications/differential/storage/__tests__/DifferentialAdjustmentMapTestCase.php',
452452
'DifferentialAffectedPath' => 'applications/differential/storage/DifferentialAffectedPath.php',
453+
'DifferentialAffectedPathEngine' => 'applications/differential/engine/DifferentialAffectedPathEngine.php',
453454
'DifferentialAsanaRepresentationField' => 'applications/differential/customfield/DifferentialAsanaRepresentationField.php',
454455
'DifferentialAuditorsCommitMessageField' => 'applications/differential/field/DifferentialAuditorsCommitMessageField.php',
455456
'DifferentialAuditorsField' => 'applications/differential/customfield/DifferentialAuditorsField.php',
@@ -6533,6 +6534,7 @@
65336534
'DifferentialActionEmailCommand' => 'MetaMTAEmailTransactionCommand',
65346535
'DifferentialAdjustmentMapTestCase' => 'PhabricatorTestCase',
65356536
'DifferentialAffectedPath' => 'DifferentialDAO',
6537+
'DifferentialAffectedPathEngine' => 'Phobject',
65366538
'DifferentialAsanaRepresentationField' => 'DifferentialCustomField',
65376539
'DifferentialAuditorsCommitMessageField' => 'DifferentialCommitMessageCustomField',
65386540
'DifferentialAuditorsField' => 'DifferentialStoredCustomField',

‎src/applications/differential/editor/DifferentialTransactionEditor.php

+6-87
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,12 @@ protected function applyFinalEffects(
356356
// diff to a revision.
357357

358358
$this->updateRevisionHashTable($object, $diff);
359-
$this->updateAffectedPathTable($object, $diff);
359+
360+
id(new DifferentialAffectedPathEngine())
361+
->setRevision($object)
362+
->setDiff($diff)
363+
->updateAffectedPaths();
364+
360365
break;
361366
}
362367
}
@@ -1258,92 +1263,6 @@ protected function buildHeraldAdapter(
12581263
return $adapter;
12591264
}
12601265

1261-
/**
1262-
* Update the table which links Differential revisions to paths they affect,
1263-
* so Diffusion can efficiently find pending revisions for a given file.
1264-
*/
1265-
private function updateAffectedPathTable(
1266-
DifferentialRevision $revision,
1267-
DifferentialDiff $diff) {
1268-
1269-
$repository = $revision->getRepository();
1270-
if (!$repository) {
1271-
// The repository where the code lives is untracked.
1272-
return;
1273-
}
1274-
1275-
$path_prefix = null;
1276-
1277-
$local_root = $diff->getSourceControlPath();
1278-
if ($local_root) {
1279-
// We're in a working copy which supports subdirectory checkouts (e.g.,
1280-
// SVN) so we need to figure out what prefix we should add to each path
1281-
// (e.g., trunk/projects/example/) to get the absolute path from the
1282-
// root of the repository. DVCS systems like Git and Mercurial are not
1283-
// affected.
1284-
1285-
// Normalize both paths and check if the repository root is a prefix of
1286-
// the local root. If so, throw it away. Note that this correctly handles
1287-
// the case where the remote path is "/".
1288-
$local_root = id(new PhutilURI($local_root))->getPath();
1289-
$local_root = rtrim($local_root, '/');
1290-
1291-
$repo_root = id(new PhutilURI($repository->getRemoteURI()))->getPath();
1292-
$repo_root = rtrim($repo_root, '/');
1293-
1294-
if (!strncmp($repo_root, $local_root, strlen($repo_root))) {
1295-
$path_prefix = substr($local_root, strlen($repo_root));
1296-
}
1297-
}
1298-
1299-
$changesets = $diff->getChangesets();
1300-
$paths = array();
1301-
foreach ($changesets as $changeset) {
1302-
$paths[] = $path_prefix.'/'.$changeset->getFilename();
1303-
}
1304-
1305-
// Mark this as also touching all parent paths, so you can see all pending
1306-
// changes to any file within a directory.
1307-
$all_paths = array();
1308-
foreach ($paths as $local) {
1309-
foreach (DiffusionPathIDQuery::expandPathToRoot($local) as $path) {
1310-
$all_paths[$path] = true;
1311-
}
1312-
}
1313-
$all_paths = array_keys($all_paths);
1314-
1315-
$path_ids =
1316-
PhabricatorRepositoryCommitChangeParserWorker::lookupOrCreatePaths(
1317-
$all_paths);
1318-
1319-
$table = new DifferentialAffectedPath();
1320-
$conn_w = $table->establishConnection('w');
1321-
1322-
$sql = array();
1323-
foreach ($path_ids as $path_id) {
1324-
$sql[] = qsprintf(
1325-
$conn_w,
1326-
'(%d, %d, %d, %d)',
1327-
$repository->getID(),
1328-
$path_id,
1329-
time(),
1330-
$revision->getID());
1331-
}
1332-
1333-
queryfx(
1334-
$conn_w,
1335-
'DELETE FROM %T WHERE revisionID = %d',
1336-
$table->getTableName(),
1337-
$revision->getID());
1338-
foreach (array_chunk($sql, 256) as $chunk) {
1339-
queryfx(
1340-
$conn_w,
1341-
'INSERT INTO %T (repositoryID, pathID, epoch, revisionID) VALUES %LQ',
1342-
$table->getTableName(),
1343-
$chunk);
1344-
}
1345-
}
1346-
13471266
/**
13481267
* Update the table connecting revisions to DVCS local hashes, so we can
13491268
* identify revisions by commit/tree hashes.

‎src/applications/differential/engine/DifferentialAffectedPathEngine.php

+137
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
<?php
2+
3+
final class DifferentialAffectedPathEngine
4+
extends Phobject {
5+
6+
private $revision;
7+
private $diff;
8+
9+
public function setRevision(DifferentialRevision $revision) {
10+
$this->revision = $revision;
11+
return $this;
12+
}
13+
14+
public function getRevision() {
15+
return $this->revision;
16+
}
17+
18+
public function setDiff(DifferentialDiff $diff) {
19+
$this->diff = $diff;
20+
return $this;
21+
}
22+
23+
public function getDiff() {
24+
return $this->diff;
25+
}
26+
27+
public function updateAffectedPaths() {
28+
$revision = $this->getRevision();
29+
$diff = $this->getDiff();
30+
$repository = $revision->getRepository();
31+
32+
if ($repository) {
33+
$repository_id = $repository->getID();
34+
} else {
35+
return;
36+
}
37+
38+
$paths = $this->getAffectedPaths();
39+
40+
$path_ids =
41+
PhabricatorRepositoryCommitChangeParserWorker::lookupOrCreatePaths(
42+
$paths);
43+
44+
$table = new DifferentialAffectedPath();
45+
$conn = $table->establishConnection('w');
46+
47+
$sql = array();
48+
foreach ($path_ids as $path_id) {
49+
$sql[] = qsprintf(
50+
$conn,
51+
'(%d, %d, %d, %d)',
52+
$repository_id,
53+
$path_id,
54+
PhabricatorTime::getNow(),
55+
$revision->getID());
56+
}
57+
58+
queryfx(
59+
$conn,
60+
'DELETE FROM %R WHERE revisionID = %d',
61+
$table,
62+
$revision->getID());
63+
if ($sql) {
64+
foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
65+
queryfx(
66+
$conn,
67+
'INSERT INTO %R (repositoryID, pathID, epoch, revisionID) VALUES %LQ',
68+
$table,
69+
$chunk);
70+
}
71+
}
72+
}
73+
74+
public function destroyAffectedPaths() {
75+
$revision = $this->getRevision();
76+
77+
$table = new DifferentialAffectedPath();
78+
$conn = $table->establishConnection('w');
79+
80+
queryfx(
81+
$conn,
82+
'DELETE FROM %R WHERE revisionID = %d',
83+
$table,
84+
$revision->getID());
85+
}
86+
87+
public function getAffectedPaths() {
88+
$revision = $this->getRevision();
89+
$diff = $this->getDiff();
90+
$repository = $revision->getRepository();
91+
92+
$path_prefix = null;
93+
if ($repository) {
94+
$local_root = $diff->getSourceControlPath();
95+
if ($local_root) {
96+
// We're in a working copy which supports subdirectory checkouts (e.g.,
97+
// SVN) so we need to figure out what prefix we should add to each path
98+
// (e.g., trunk/projects/example/) to get the absolute path from the
99+
// root of the repository. DVCS systems like Git and Mercurial are not
100+
// affected.
101+
102+
// Normalize both paths and check if the repository root is a prefix of
103+
// the local root. If so, throw it away. Note that this correctly
104+
// handles the case where the remote path is "/".
105+
$local_root = id(new PhutilURI($local_root))->getPath();
106+
$local_root = rtrim($local_root, '/');
107+
108+
$repo_root = id(new PhutilURI($repository->getRemoteURI()))->getPath();
109+
$repo_root = rtrim($repo_root, '/');
110+
111+
if (!strncmp($repo_root, $local_root, strlen($repo_root))) {
112+
$path_prefix = substr($local_root, strlen($repo_root));
113+
}
114+
}
115+
}
116+
117+
$changesets = $diff->getChangesets();
118+
119+
$paths = array();
120+
foreach ($changesets as $changeset) {
121+
$paths[] = $path_prefix.'/'.$changeset->getFilename();
122+
}
123+
124+
// Mark this as also touching all parent paths, so you can see all pending
125+
// changes to any file within a directory.
126+
$all_paths = array();
127+
foreach ($paths as $local) {
128+
foreach (DiffusionPathIDQuery::expandPathToRoot($local) as $path) {
129+
$all_paths[$path] = true;
130+
}
131+
}
132+
$all_paths = array_keys($all_paths);
133+
134+
return $all_paths;
135+
}
136+
137+
}

‎src/applications/differential/storage/DifferentialRevision.php

+3-10
Original file line numberDiff line numberDiff line change
@@ -1022,16 +1022,9 @@ public function destroyObjectPermanently(
10221022
$engine->destroyObject($diff);
10231023
}
10241024

1025-
$conn_w = $this->establishConnection('w');
1026-
1027-
// we have to do paths a little differently as they do not have
1028-
// an id or phid column for delete() to act on
1029-
$dummy_path = new DifferentialAffectedPath();
1030-
queryfx(
1031-
$conn_w,
1032-
'DELETE FROM %T WHERE revisionID = %d',
1033-
$dummy_path->getTableName(),
1034-
$this->getID());
1025+
id(new DifferentialAffectedPathEngine())
1026+
->setRevision($this)
1027+
->destroyAffectedPaths();
10351028

10361029
$viewstate_query = id(new DifferentialViewStateQuery())
10371030
->setViewer($viewer)

0 commit comments

Comments
 (0)
Failed to load comments.