Differential D21760 Diff 51880 src/infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php
Changeset View
Changeset View
Standalone View
Standalone View
src/infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php
Show First 20 Lines • Show All 135 Lines • ▼ Show 20 Lines | foreach ($all as $changeset) { | ||||
$name_map_old[$filename] = $changeset_id; | $name_map_old[$filename] = $changeset_id; | ||||
} | } | ||||
// We always update the new map (newest changeset overwrites). | // We always update the new map (newest changeset overwrites). | ||||
$name_map_new[$changeset->getFilename()] = $changeset_id; | $name_map_new[$changeset->getFilename()] = $changeset_id; | ||||
} | } | ||||
} | } | ||||
// Find the smallest "new" changeset ID. We'll consider everything | $new_id_map = mpull($new, null, 'getID'); | ||||
// larger than this to be "newer", and everything smaller to be "older". | |||||
$first_new_id = min(mpull($new, 'getID')); | |||||
$results = array(); | $results = array(); | ||||
foreach ($inlines as $inline) { | foreach ($inlines as $inline) { | ||||
$changeset_id = $inline->getChangesetID(); | $changeset_id = $inline->getChangesetID(); | ||||
if (isset($id_map[$changeset_id])) { | if (isset($id_map[$changeset_id])) { | ||||
// This inline is legitimately on one of the current changesets, so | // This inline is legitimately on one of the current changesets, so | ||||
// we can include it in the result set unmodified. | // we can include it in the result set unmodified. | ||||
$results[] = $inline; | $results[] = $inline; | ||||
continue; | continue; | ||||
} | } | ||||
$changeset = idx($changesets, $changeset_id); | $changeset = idx($changesets, $changeset_id); | ||||
if (!$changeset) { | if (!$changeset) { | ||||
// Just discard this inline, as it has bogus data. | // Just discard this inline, as it has bogus data. | ||||
continue; | continue; | ||||
} | } | ||||
$target_id = null; | $target_id = null; | ||||
if ($changeset_id >= $first_new_id) { | if (isset($new_id_map[$changeset_id])) { | ||||
cspeckmim: Should this be null-checked or would PHP do some sort of coercion? From a quick glance through… | |||||
Done Inline ActionsI thought it was unreachable when I was writing it, but I think you're right that it's not unreachable (or, at least, not obviously unreachable). PHP will type coerce, but I'll make this obviously-correct. The >= test is kind of bad anyway since it's depending on data storage behavior, and we can imagine changes where "ids always go up" might not be true, e.g. possibly "revert to an older version of this change" in Differential. epriestley: I thought it was unreachable when I was writing it, but I think you're right that it's not… | |||||
$name_map = $name_map_new; | $name_map = $name_map_new; | ||||
$is_new = true; | $is_new = true; | ||||
} else { | } else { | ||||
$name_map = $name_map_old; | $name_map = $name_map_old; | ||||
$is_new = false; | $is_new = false; | ||||
} | } | ||||
$filename = $changeset->getFilename(); | $filename = $changeset->getFilename(); | ||||
▲ Show 20 Lines • Show All 198 Lines • Show Last 20 Lines |
Should this be null-checked or would PHP do some sort of coercion? From a quick glance through I couldn't be certain that this line would only be hit if $new was valid/present