@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
Separate "shouldPublishRef()" from "isPermanentRef()" and set "IMPORTED_PERMANENT" more narrowly
Summary:
Ref T13591. Currently, the "IMPORTED_PERMANENT" flag (previously "IMPORTED_CLOSEABLE", until D21514) flag is set by using the result of "shouldPublishRef()".
This method returns the wrong value for the flag when there is a repository-level reason not to publish the ref (most commonly, because the repository is currently importing).
Although it's correct that commits should not be published in an importing repository, that's already handled in the "PublishWorker" by testing "shouldPublishCommit()". The "IMPORTED_PERMANENT" flag should only reflect whether a commit is reachable from a permanent ref or not.
- Move the relevant logic to a new method in Publisher.
- Fill "IMPORTED_PERMANENT" narrowly from "isPermanentRef()", rather than broadly from "shouldPublishRef()".
- Deduplicate some logic in "PhabricatorRepositoryRefEngine" which has the same intent as the logic in the Publisher.
Test Plan:
- Ran discovery on a new repository, saw permanent commits marked as permanent from the beginning.
- See later changes in this patch series for additional testing.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21515
+49
-32
+5
-5
src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
+5
-5
src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php
···
189
189
$head_refs = $this->discoverStreamAncestry(
190
190
new PhabricatorGitGraphStream($repository, $commit),
191
191
$commit,
192
-
$publisher->shouldPublishRef($ref));
192
+
$publisher->isPermanentRef($ref));
193
193
194
194
$this->didDiscoverRefs($head_refs);
195
195
···
507
507
}
508
508
509
509
/**
510
-
* Sort branches so we process permanent branches first. This makes the
511
-
* whole import process a little cheaper, since we can publish these commits
512
-
* the first time through rather than catching them in the refs step.
510
+
* Sort refs so we process permanent refs first. This makes the whole import
511
+
* process a little cheaper, since we can publish these commits the first
512
+
* time through rather than catching them in the refs step.
513
513
*
514
514
* @task internal
515
515
*
···
523
523
$head_refs = array();
524
524
$tail_refs = array();
525
525
foreach ($refs as $ref) {
526
-
if ($publisher->shouldPublishRef($ref)) {
526
+
if ($publisher->isPermanentRef($ref)) {
527
527
$head_refs[] = $ref;
528
528
} else {
529
529
$tail_refs[] = $ref;
+11
-15
src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
+11
-15
src/applications/repository/engine/PhabricatorRepositoryRefEngine.php
···
100
100
$this->setPermanentFlagOnCommits($this->permanentCommits);
101
101
}
102
102
103
-
$save_cursors = $this->getCursorsForUpdate($all_cursors);
103
+
$save_cursors = $this->getCursorsForUpdate($repository, $all_cursors);
104
104
105
105
if ($this->newPositions || $this->deadPositions || $save_cursors) {
106
106
$repository->openTransaction();
···
121
121
}
122
122
}
123
123
124
-
private function getCursorsForUpdate(array $cursors) {
124
+
private function getCursorsForUpdate(
125
+
PhabricatorRepository $repository,
126
+
array $cursors) {
125
127
assert_instances_of($cursors, 'PhabricatorRepositoryRefCursor');
128
+
129
+
$publisher = $repository->newPublisher();
126
130
127
131
$results = array();
128
132
129
133
foreach ($cursors as $cursor) {
130
-
$ref_type = $cursor->getRefType();
131
-
$ref_name = $cursor->getRefName();
132
-
133
-
$is_permanent = $this->isPermanentRef($ref_type, $ref_name);
134
+
$diffusion_ref = $cursor->newDiffusionRepositoryRef();
134
135
136
+
$is_permanent = $publisher->isPermanentRef($diffusion_ref);
135
137
if ($is_permanent == $cursor->getIsPermanent()) {
136
138
continue;
137
139
}
···
259
261
$ref_type,
260
262
array $all_closing_heads) {
261
263
$repository = $this->getRepository();
264
+
$publisher = $repository->newPublisher();
262
265
263
266
// NOTE: Mercurial branches may have multiple branch heads; this logic
264
267
// is complex primarily to account for that.
···
341
344
$this->markPositionNew($new_position);
342
345
}
343
346
344
-
if ($this->isPermanentRef($ref_type, $name)) {
347
+
$diffusion_ref = head($refs)->newDiffusionRepositoryRef();
348
+
if ($publisher->isPermanentRef($diffusion_ref)) {
345
349
346
350
// See T13284. If this cursor was already marked as permanent, we
347
351
// only need to publish the newly created ref positions. However, if
···
402
406
$this->markPositionDead($position);
403
407
}
404
408
}
405
-
}
406
-
407
-
private function isPermanentRef($ref_type, $ref_name) {
408
-
if ($ref_type !== PhabricatorRepositoryRefCursor::TYPE_BRANCH) {
409
-
return false;
410
-
}
411
-
412
-
return $this->getRepository()->isBranchPermanentRef($ref_name);
413
409
}
414
410
415
411
/**
+27
-12
src/applications/repository/query/PhabricatorRepositoryPublisher.php
+27
-12
src/applications/repository/query/PhabricatorRepositoryPublisher.php
···
38
38
return !$this->getCommitHoldReasons($commit);
39
39
}
40
40
41
+
public function isPermanentRef(DiffusionRepositoryRef $ref) {
42
+
return !$this->getRefImpermanentReasons($ref);
43
+
}
44
+
41
45
/* -( Hold Reasons )------------------------------------------------------- */
42
46
43
47
public function getRepositoryHoldReasons() {
···
59
63
$repository = $this->getRepository();
60
64
$reasons = $this->getRepositoryHoldReasons();
61
65
62
-
if (!$ref->isBranch()) {
63
-
$reasons[] = self::HOLD_REF_NOT_BRANCH;
64
-
} else {
65
-
$branch_name = $ref->getShortName();
66
-
67
-
if (!$repository->shouldTrackBranch($branch_name)) {
68
-
$reasons[] = self::HOLD_UNTRACKED;
69
-
}
70
-
71
-
if (!$repository->isBranchPermanentRef($branch_name)) {
72
-
$reasons[] = self::HOLD_NOT_PERMANENT_REF;
73
-
}
66
+
foreach ($this->getRefImpermanentReasons($ref) as $reason) {
67
+
$reasons[] = $reason;
74
68
}
75
69
76
70
return $reasons;
···
83
77
if ($repository->isGit()) {
84
78
if (!$commit->isPermanentCommit()) {
85
79
$reasons[] = self::HOLD_NOT_REACHABLE_FROM_PERMANENT_REF;
80
+
}
81
+
}
82
+
83
+
return $reasons;
84
+
}
85
+
86
+
public function getRefImpermanentReasons(DiffusionRepositoryRef $ref) {
87
+
$repository = $this->getRepository();
88
+
$reasons = array();
89
+
90
+
if (!$ref->isBranch()) {
91
+
$reasons[] = self::HOLD_REF_NOT_BRANCH;
92
+
} else {
93
+
$branch_name = $ref->getShortName();
94
+
95
+
if (!$repository->shouldTrackBranch($branch_name)) {
96
+
$reasons[] = self::HOLD_UNTRACKED;
97
+
}
98
+
99
+
if (!$repository->isBranchPermanentRef($branch_name)) {
100
+
$reasons[] = self::HOLD_NOT_PERMANENT_REF;
86
101
}
87
102
}
88
103
+6
src/applications/repository/storage/PhabricatorRepositoryRefCursor.php
+6
src/applications/repository/storage/PhabricatorRepositoryRefCursor.php
···
88
88
return mpull($this->getPositions(), 'getCommitIdentifier');
89
89
}
90
90
91
+
public function newDiffusionRepositoryRef() {
92
+
return id(new DiffusionRepositoryRef())
93
+
->setRefType($this->getRefType())
94
+
->setShortName($this->getRefName());
95
+
}
96
+
91
97
92
98
/* -( PhabricatorPolicyInterface )----------------------------------------- */
93
99