@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Begin making change parsers testable
Summary:
Ref T4327. There are a bunch of other probably-related tasks too, some linked there.
We have some rare/unusual bugs in the change parsers, mostly in Subversion, but it's terrifying to touch them because they're complicated and fragile and have no test coverage.
To fix this stuff, I want to make them more testable. In particular, they basically end with this big INSERT right now. Instead, I'm going to make them return objects representing the data to be inserted, then have the common infrastructure do the insert. This gives us two benefits:
- Reduced code duplication on the insert;
- we can stop before the insert and have unit tests examine the objects.
This swaps the Git parser over, but doesn't swap the hg/svn parsers yet. I'll do those separately, the SVN one looks a bit tricky.
Test Plan:
- Used `scripts/repository/reparse.php` to reparse a Git commit, with `--trace`. Verified it looked the same as before and the SQL that was executed seemed reasonable.
- Did the same for `hg` / `svn` commits, to make sure I didn't derp anything. These aren't expected to do anything differently.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7980
+158
-40
+2
src/__phutil_library_map__.php
+2
src/__phutil_library_map__.php
···
1866
1866
'PhabricatorRepositoryPHIDTypeMirror' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypeMirror.php',
1867
1867
'PhabricatorRepositoryPHIDTypePushLog' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypePushLog.php',
1868
1868
'PhabricatorRepositoryPHIDTypeRepository' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypeRepository.php',
1869
+
'PhabricatorRepositoryParsedChange' => 'applications/repository/data/PhabricatorRepositoryParsedChange.php',
1869
1870
'PhabricatorRepositoryPullEngine' => 'applications/repository/engine/PhabricatorRepositoryPullEngine.php',
1870
1871
'PhabricatorRepositoryPullLocalDaemon' => 'applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php',
1871
1872
'PhabricatorRepositoryPushLog' => 'applications/repository/storage/PhabricatorRepositoryPushLog.php',
···
4542
4543
'PhabricatorRepositoryPHIDTypeMirror' => 'PhabricatorPHIDType',
4543
4544
'PhabricatorRepositoryPHIDTypePushLog' => 'PhabricatorPHIDType',
4544
4545
'PhabricatorRepositoryPHIDTypeRepository' => 'PhabricatorPHIDType',
4546
+
'PhabricatorRepositoryParsedChange' => 'Phobject',
4545
4547
'PhabricatorRepositoryPullEngine' => 'PhabricatorRepositoryEngine',
4546
4548
'PhabricatorRepositoryPullLocalDaemon' => 'PhabricatorDaemon',
4547
4549
'PhabricatorRepositoryPushLog' =>
+77
src/applications/repository/data/PhabricatorRepositoryParsedChange.php
+77
src/applications/repository/data/PhabricatorRepositoryParsedChange.php
···
1
+
<?php
2
+
3
+
final class PhabricatorRepositoryParsedChange extends Phobject {
4
+
5
+
private $pathID;
6
+
private $targetPathID;
7
+
private $targetCommitID;
8
+
private $changeType;
9
+
private $fileType;
10
+
private $isDirect;
11
+
private $commitSequence;
12
+
13
+
public function setPathID($path_id) {
14
+
$this->pathID = $path_id;
15
+
return $this;
16
+
}
17
+
18
+
public function getPathID() {
19
+
return $this->pathID;
20
+
}
21
+
22
+
public function setCommitSequence($commit_sequence) {
23
+
$this->commitSequence = $commit_sequence;
24
+
return $this;
25
+
}
26
+
27
+
public function getCommitSequence() {
28
+
return $this->commitSequence;
29
+
}
30
+
31
+
public function setIsDirect($is_direct) {
32
+
$this->isDirect = $is_direct;
33
+
return $this;
34
+
}
35
+
36
+
public function getIsDirect() {
37
+
return $this->isDirect;
38
+
}
39
+
40
+
public function setFileType($file_type) {
41
+
$this->fileType = $file_type;
42
+
return $this;
43
+
}
44
+
45
+
public function getFileType() {
46
+
return $this->fileType;
47
+
}
48
+
49
+
public function setChangeType($change_type) {
50
+
$this->changeType = $change_type;
51
+
return $this;
52
+
}
53
+
54
+
public function getChangeType() {
55
+
return $this->changeType;
56
+
}
57
+
58
+
public function setTargetCommitID($target_commit_id) {
59
+
$this->targetCommitID = $target_commit_id;
60
+
return $this;
61
+
}
62
+
63
+
public function getTargetCommitID() {
64
+
return $this->targetCommitID;
65
+
}
66
+
67
+
68
+
public function setTargetPathID($target_path_id) {
69
+
$this->targetPathID = $target_path_id;
70
+
return $this;
71
+
}
72
+
73
+
public function getTargetPathID() {
74
+
return $this->targetPathID;
75
+
}
76
+
77
+
}
+1
-2
src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php
+1
-2
src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php
+54
-5
src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php
+54
-5
src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php
···
24
24
$this->log("Parsing %s...\n", $full_name);
25
25
if ($this->isBadCommit($full_name)) {
26
26
$this->log("This commit is marked bad!");
27
-
$result = null;
28
-
} else {
29
-
$result = $this->parseCommitChanges($repository, $commit);
27
+
return;
28
+
}
29
+
30
+
$results = $this->parseCommitChanges($repository, $commit);
31
+
if ($results) {
32
+
$this->writeCommitChanges($repository, $commit, $results);
30
33
}
31
34
32
35
$this->finishParse();
33
-
34
-
return $result;
35
36
}
36
37
37
38
public static function lookupOrCreatePaths(array $paths) {
···
96
97
array(
97
98
'commitID' => $commit->getID(),
98
99
));
100
+
}
101
+
}
102
+
103
+
private function writeCommitChanges(
104
+
PhabricatorRepository $repository,
105
+
PhabricatorRepositoryCommit $commit,
106
+
array $changes) {
107
+
108
+
$repository_id = (int)$repository->getID();
109
+
$commit_id = (int)$commit->getID();
110
+
111
+
// NOTE: This SQL is being built manually instead of with qsprintf()
112
+
// because some SVN changes affect an enormous number of paths (millions)
113
+
// and this showed up as significantly slow on a profile at some point.
114
+
115
+
$changes_sql = array();
116
+
foreach ($changes as $change) {
117
+
$values = array(
118
+
$repository_id,
119
+
(int)$change->getPathID(),
120
+
$commit_id,
121
+
nonempty((int)$change->getTargetPathID(), 'null'),
122
+
nonempty((int)$change->getTargetCommitID(), 'null'),
123
+
(int)$change->getChangeType(),
124
+
(int)$change->getFileType(),
125
+
(int)$change->getIsDirect(),
126
+
(int)$change->getCommitSequence(),
127
+
);
128
+
$changes_sql[] = '('.implode(', ', $values).')';
129
+
}
130
+
131
+
$conn_w = $repository->establishConnection('w');
132
+
133
+
queryfx(
134
+
$conn_w,
135
+
'DELETE FROM %T WHERE commitID = %d',
136
+
PhabricatorRepository::TABLE_PATHCHANGE,
137
+
$commit_id);
138
+
139
+
foreach (PhabricatorLiskDAO::chunkSQL($changes_sql) as $chunk) {
140
+
queryfx(
141
+
$conn_w,
142
+
'INSERT INTO %T
143
+
(repositoryID, pathID, commitID, targetPathID, targetCommitID,
144
+
changeType, fileType, isDirect, commitSequence)
145
+
VALUES %Q',
146
+
PhabricatorRepository::TABLE_PATHCHANGE,
147
+
$chunk);
99
148
}
100
149
}
101
150
+20
-33
src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php
+20
-33
src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php
···
228
228
}
229
229
}
230
230
231
-
$conn_w = $repository->establishConnection('w');
232
231
233
-
$changes_sql = array();
232
+
$results = array();
234
233
foreach ($changes as $change) {
235
-
$values = array(
236
-
(int)$change['repositoryID'],
237
-
(int)$change['pathID'],
238
-
(int)$change['commitID'],
239
-
$change['targetPathID']
240
-
? (int)$change['targetPathID']
241
-
: 'null',
242
-
$change['targetCommitID']
243
-
? (int)$change['targetCommitID']
244
-
: 'null',
245
-
(int)$change['changeType'],
246
-
(int)$change['fileType'],
247
-
(int)$change['isDirect'],
248
-
(int)$change['commitSequence'],
249
-
);
250
-
$changes_sql[] = '('.implode(', ', $values).')';
234
+
$target_path_id = $change['targetPathID']
235
+
? (int)$change['targetPathID']
236
+
: null;
237
+
238
+
$target_commit_id = $change['targetCommitID']
239
+
? (int)$change['targetCommitID']
240
+
: null;
241
+
242
+
$result = id(new PhabricatorRepositoryParsedChange())
243
+
->setPathID((int)$change['pathID'])
244
+
->setTargetPathID($target_path_id)
245
+
->setTargetCommitID($target_commit_id)
246
+
->setChangeType((int)$change['changeType'])
247
+
->setFileType($change['fileType'])
248
+
->setIsDirect((int)$change['isDirect'])
249
+
->setCommitSequence((int)$change['commitSequence']);
250
+
251
+
$results[] = $result;
251
252
}
252
253
253
-
queryfx(
254
-
$conn_w,
255
-
'DELETE FROM %T WHERE commitID = %d',
256
-
PhabricatorRepository::TABLE_PATHCHANGE,
257
-
$commit->getID());
258
-
foreach (array_chunk($changes_sql, 256) as $sql_chunk) {
259
-
queryfx(
260
-
$conn_w,
261
-
'INSERT INTO %T
262
-
(repositoryID, pathID, commitID, targetPathID, targetCommitID,
263
-
changeType, fileType, isDirect, commitSequence)
264
-
VALUES %Q',
265
-
PhabricatorRepository::TABLE_PATHCHANGE,
266
-
implode(', ', $sql_chunk));
267
-
}
254
+
return $results;
268
255
}
269
256
270
257
}
+2
src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryMercurialCommitChangeParserWorker.php
+2
src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryMercurialCommitChangeParserWorker.php
+2
src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php
+2
src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php