Differential D21760 Diff 51872 src/infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php

Changeset View

Standalone View

src/infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php

Show First 20 Lines • Show All 137 Lines • ▼ Show 20 Lines	foreach ($all as $changeset) {

// 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		// Find the smallest "new" changeset ID. We'll consider everything
// larger than this to be "newer", and everything smaller to be "older".		// larger than this to be "newer", and everything smaller to be "older".

		// (It's possible we have no changesets if the commit is empty.)

		if ($new) {
$first_new_id = min(mpull($new, 'getID'));		$first_new_id = min(mpull($new, 'getID'));
		} else {
		$first_new_id = null;
		}

$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 ($changeset_id >= $first_new_id) {
		cspeckmimUnsubmitted Not Done Inline Actions 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 cspeckmim: Should this be null-checked or would PHP do some sort of coercion? From a quick glance through…
		epriestleyAuthorUnsubmitted Done Inline Actions I 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