Skip to content

Commit 0dd947c

Browse files
author
epriestley
committed
Move diff extraction from commits to a separate test with a CLI command
Summary: Ref T9319. When we discover a commit, we sometimes update the corresponding revision with a "this is the actual committed change" diff and send out a link to the changes between review and commit. This is currently very difficult to test, because it only happens the first time and you have to either go set up a bunch of objects or add a bunch of special casing to the parser to hit the workflow. I'm making some changes to how it pulls file content. To make those changes easier to test, first start extracting this stuff so the code can be run with `bin/differential extract ...` instead of needing to do a bunch of more complicated setup steps. Test Plan: - Ran `bin/differential extract ...` to extract diffs from commits. - Forced my way through the daemon workflow by faking out a bunch of flags, got a clean extract + attach + update. After this patch, this should rarely be necessary. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9319 Differential Revision: https://secure.phabricator.com/D14967
1 parent 7ba13ed commit 0dd947c

File tree

8 files changed

+195
-60
lines changed

8 files changed

+195
-60
lines changed

‎bin/differential

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
../scripts/setup/manage_differential.php

‎scripts/setup/manage_differential.php

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
#!/usr/bin/env php
2+
<?php
3+
4+
$root = dirname(dirname(dirname(__FILE__)));
5+
require_once $root.'/scripts/__init_script__.php';
6+
7+
$args = new PhutilArgumentParser($argv);
8+
$args->setTagline(pht('manage hunks'));
9+
$args->setSynopsis(<<<EOSYNOPSIS
10+
**differential** __command__ [__options__]
11+
Manage Differential.
12+
13+
EOSYNOPSIS
14+
);
15+
$args->parseStandardArguments();
16+
17+
$workflows = id(new PhutilClassMapQuery())
18+
->setAncestorClass('PhabricatorDifferentialManagementWorkflow')
19+
->execute();
20+
$workflows[] = new PhutilHelpArgumentWorkflow();
21+
$args->parseWorkflows($workflows);

‎src/__phutil_library_map__.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,7 @@
386386
'DifferentialDiffContentRemovedHeraldField' => 'applications/differential/herald/DifferentialDiffContentRemovedHeraldField.php',
387387
'DifferentialDiffCreateController' => 'applications/differential/controller/DifferentialDiffCreateController.php',
388388
'DifferentialDiffEditor' => 'applications/differential/editor/DifferentialDiffEditor.php',
389+
'DifferentialDiffExtractionEngine' => 'applications/differential/engine/DifferentialDiffExtractionEngine.php',
389390
'DifferentialDiffHeraldField' => 'applications/differential/herald/DifferentialDiffHeraldField.php',
390391
'DifferentialDiffHeraldFieldGroup' => 'applications/differential/herald/DifferentialDiffHeraldFieldGroup.php',
391392
'DifferentialDiffInlineCommentQuery' => 'applications/differential/query/DifferentialDiffInlineCommentQuery.php',
@@ -2143,6 +2144,8 @@
21432144
'PhabricatorDifferenceEngine' => 'infrastructure/diff/PhabricatorDifferenceEngine.php',
21442145
'PhabricatorDifferentialApplication' => 'applications/differential/application/PhabricatorDifferentialApplication.php',
21452146
'PhabricatorDifferentialConfigOptions' => 'applications/differential/config/PhabricatorDifferentialConfigOptions.php',
2147+
'PhabricatorDifferentialExtractWorkflow' => 'applications/differential/management/PhabricatorDifferentialExtractWorkflow.php',
2148+
'PhabricatorDifferentialManagementWorkflow' => 'applications/differential/management/PhabricatorDifferentialManagementWorkflow.php',
21462149
'PhabricatorDifferentialRevisionTestDataGenerator' => 'applications/differential/lipsum/PhabricatorDifferentialRevisionTestDataGenerator.php',
21472150
'PhabricatorDiffusionApplication' => 'applications/diffusion/application/PhabricatorDiffusionApplication.php',
21482151
'PhabricatorDiffusionConfigOptions' => 'applications/diffusion/config/PhabricatorDiffusionConfigOptions.php',
@@ -4334,6 +4337,7 @@
43344337
'DifferentialDiffContentRemovedHeraldField' => 'DifferentialDiffHeraldField',
43354338
'DifferentialDiffCreateController' => 'DifferentialController',
43364339
'DifferentialDiffEditor' => 'PhabricatorApplicationTransactionEditor',
4340+
'DifferentialDiffExtractionEngine' => 'Phobject',
43374341
'DifferentialDiffHeraldField' => 'HeraldField',
43384342
'DifferentialDiffHeraldFieldGroup' => 'HeraldFieldGroup',
43394343
'DifferentialDiffInlineCommentQuery' => 'PhabricatorDiffInlineCommentQuery',
@@ -6376,6 +6380,8 @@
63766380
'PhabricatorDifferenceEngine' => 'Phobject',
63776381
'PhabricatorDifferentialApplication' => 'PhabricatorApplication',
63786382
'PhabricatorDifferentialConfigOptions' => 'PhabricatorApplicationConfigOptions',
6383+
'PhabricatorDifferentialExtractWorkflow' => 'PhabricatorDifferentialManagementWorkflow',
6384+
'PhabricatorDifferentialManagementWorkflow' => 'PhabricatorManagementWorkflow',
63796385
'PhabricatorDifferentialRevisionTestDataGenerator' => 'PhabricatorTestDataGenerator',
63806386
'PhabricatorDiffusionApplication' => 'PhabricatorApplication',
63816387
'PhabricatorDiffusionConfigOptions' => 'PhabricatorApplicationConfigOptions',

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

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
<?php
2+
3+
final class DifferentialDiffExtractionEngine extends Phobject {
4+
5+
private $viewer;
6+
private $authorPHID;
7+
8+
public function setViewer(PhabricatorUser $viewer) {
9+
$this->viewer = $viewer;
10+
return $this;
11+
}
12+
13+
public function getViewer() {
14+
return $this->viewer;
15+
}
16+
17+
public function setAuthorPHID($author_phid) {
18+
$this->authorPHID = $author_phid;
19+
return $this;
20+
}
21+
22+
public function getAuthorPHID() {
23+
return $this->authorPHID;
24+
}
25+
26+
public function newDiffFromCommit(PhabricatorRepositoryCommit $commit) {
27+
$viewer = $this->getViewer();
28+
29+
$repository = $commit->getRepository();
30+
$identifier = $commit->getCommitIdentifier();
31+
$monogram = $commit->getMonogram();
32+
33+
$drequest = DiffusionRequest::newFromDictionary(
34+
array(
35+
'user' => $viewer,
36+
'repository' => $repository,
37+
));
38+
39+
$raw_diff = DiffusionQuery::callConduitWithDiffusionRequest(
40+
$viewer,
41+
$drequest,
42+
'diffusion.rawdiffquery',
43+
array(
44+
'commit' => $identifier,
45+
));
46+
47+
// TODO: Support adds, deletes and moves under SVN.
48+
if (strlen($raw_diff)) {
49+
$changes = id(new ArcanistDiffParser())->parseDiff($raw_diff);
50+
} else {
51+
// This is an empty diff, maybe made with `git commit --allow-empty`.
52+
// NOTE: These diffs have the same tree hash as their ancestors, so
53+
// they may attach to revisions in an unexpected way. Just let this
54+
// happen for now, although it might make sense to special case it
55+
// eventually.
56+
$changes = array();
57+
}
58+
59+
$diff = DifferentialDiff::newFromRawChanges($viewer, $changes)
60+
->setRepositoryPHID($repository->getPHID())
61+
->setCreationMethod('commit')
62+
->setSourceControlSystem($repository->getVersionControlSystem())
63+
->setLintStatus(DifferentialLintStatus::LINT_AUTO_SKIP)
64+
->setUnitStatus(DifferentialUnitStatus::UNIT_AUTO_SKIP)
65+
->setDateCreated($commit->getEpoch())
66+
->setDescription($monogram);
67+
68+
$author_phid = $this->getAuthorPHID();
69+
if ($author_phid !== null) {
70+
$diff->setAuthorPHID($author_phid);
71+
}
72+
73+
$parents = DiffusionQuery::callConduitWithDiffusionRequest(
74+
$viewer,
75+
$drequest,
76+
'diffusion.commitparentsquery',
77+
array(
78+
'commit' => $identifier,
79+
));
80+
81+
if ($parents) {
82+
$diff->setSourceControlBaseRevision(head($parents));
83+
}
84+
85+
// TODO: Attach binary files.
86+
87+
return $diff->save();
88+
}
89+
90+
}

‎src/applications/differential/management/PhabricatorDifferentialExtractWorkflow.php

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
<?php
2+
3+
final class PhabricatorDifferentialExtractWorkflow
4+
extends PhabricatorDifferentialManagementWorkflow {
5+
6+
protected function didConstruct() {
7+
$this
8+
->setName('extract')
9+
->setExamples('**extract** __commit__')
10+
->setSynopsis(pht('Extract a diff from a commit.'))
11+
->setArguments(
12+
array(
13+
array(
14+
'name' => 'extract',
15+
'wildcard' => true,
16+
'help' => pht('Commit to extract.'),
17+
),
18+
));
19+
}
20+
21+
public function execute(PhutilArgumentParser $args) {
22+
$viewer = $this->getViewer();
23+
24+
$extract = $args->getArg('extract');
25+
26+
if (!$extract) {
27+
throw new PhutilArgumentUsageException(
28+
pht('Specify a commit to extract the diff from.'));
29+
}
30+
31+
if (count($extract) > 1) {
32+
throw new PhutilArgumentUsageException(
33+
pht('Specify exactly one commit to extract.'));
34+
}
35+
36+
$extract = head($extract);
37+
38+
$commit = id(new DiffusionCommitQuery())
39+
->setViewer($viewer)
40+
->withIdentifiers(array($extract))
41+
->executeOne();
42+
43+
if (!$commit) {
44+
throw new PhutilArgumentUsageException(
45+
pht(
46+
'Commit "%s" is not valid.',
47+
$extract));
48+
}
49+
50+
$diff = id(new DifferentialDiffExtractionEngine())
51+
->setViewer($viewer)
52+
->newDiffFromCommit($commit);
53+
54+
$uri = PhabricatorEnv::getProductionURI($diff->getURI());
55+
56+
echo tsprintf(
57+
"%s\n\n %s\n",
58+
pht('Extracted diff from "%s":', $extract),
59+
$uri);
60+
}
61+
62+
63+
}

‎src/applications/differential/management/PhabricatorDifferentialManagementWorkflow.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<?php
2+
3+
abstract class PhabricatorDifferentialManagementWorkflow
4+
extends PhabricatorManagementWorkflow {}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,12 @@ public function loadCoverageMap(PhabricatorUser $viewer) {
385385
return $map;
386386
}
387387

388+
public function getURI() {
389+
$id = $this->getID();
390+
return "/differential/diff/{$id}/";
391+
}
392+
393+
388394
/* -( PhabricatorPolicyInterface )----------------------------------------- */
389395

390396

‎src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php

Lines changed: 4 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,10 @@ final protected function updateCommitData(DiffusionCommitRef $ref) {
216216
$low_level_query->getRevisionMatchData());
217217
}
218218

219-
$diff = $this->generateFinalDiff($revision, $acting_as_phid);
219+
$diff = id(new DifferentialDiffExtractionEngine())
220+
->setViewer($actor)
221+
->setAuthorPHID($acting_as_phid)
222+
->newDiffFromCommit($commit);
220223

221224
$vs_diff = $this->loadChangedByCommit($revision, $diff);
222225
$changed_uri = null;
@@ -277,65 +280,6 @@ final protected function updateCommitData(DiffusionCommitRef $ref) {
277280
PhabricatorRepositoryCommit::IMPORTED_MESSAGE);
278281
}
279282

280-
private function generateFinalDiff(
281-
DifferentialRevision $revision,
282-
$actor_phid) {
283-
284-
$viewer = PhabricatorUser::getOmnipotentUser();
285-
286-
$drequest = DiffusionRequest::newFromDictionary(array(
287-
'user' => $viewer,
288-
'repository' => $this->repository,
289-
));
290-
291-
$raw_diff = DiffusionQuery::callConduitWithDiffusionRequest(
292-
$viewer,
293-
$drequest,
294-
'diffusion.rawdiffquery',
295-
array(
296-
'commit' => $this->commit->getCommitIdentifier(),
297-
));
298-
299-
// TODO: Support adds, deletes and moves under SVN.
300-
if (strlen($raw_diff)) {
301-
$changes = id(new ArcanistDiffParser())->parseDiff($raw_diff);
302-
} else {
303-
// This is an empty diff, maybe made with `git commit --allow-empty`.
304-
// NOTE: These diffs have the same tree hash as their ancestors, so
305-
// they may attach to revisions in an unexpected way. Just let this
306-
// happen for now, although it might make sense to special case it
307-
// eventually.
308-
$changes = array();
309-
}
310-
311-
$diff = DifferentialDiff::newFromRawChanges($viewer, $changes)
312-
->setRepositoryPHID($this->repository->getPHID())
313-
->setAuthorPHID($actor_phid)
314-
->setCreationMethod('commit')
315-
->setSourceControlSystem($this->repository->getVersionControlSystem())
316-
->setLintStatus(DifferentialLintStatus::LINT_AUTO_SKIP)
317-
->setUnitStatus(DifferentialUnitStatus::UNIT_AUTO_SKIP)
318-
->setDateCreated($this->commit->getEpoch())
319-
->setDescription(
320-
pht(
321-
'Commit %s',
322-
$this->commit->getMonogram()));
323-
324-
$parents = DiffusionQuery::callConduitWithDiffusionRequest(
325-
$viewer,
326-
$drequest,
327-
'diffusion.commitparentsquery',
328-
array(
329-
'commit' => $this->commit->getCommitIdentifier(),
330-
));
331-
if ($parents) {
332-
$diff->setSourceControlBaseRevision(head($parents));
333-
}
334-
335-
// TODO: Attach binary files.
336-
337-
return $diff->save();
338-
}
339283

340284
private function loadChangedByCommit(
341285
DifferentialRevision $revision,

0 commit comments

Comments
 (0)