Every commit gets 2 diffs

Since the “AutoClose” removal it now seems like a review which is accepted makes 2 diffs in the revision history even though the diffs are identical (and the revision hasn’t been updated as part of the review process)

Using this as an example:

https://secure.phabricator.com/D20536

I believe both diffs are identical it comes form the fact that following the commit the revision is updated with the commitID

image

But in the history we have 2 diffs here? (is that what was intended, the new norm?)

image

I believe it comes from, the new code in updateRevision() that was changed at the end of April

https://secure.phabricator.com/source/phabricator/browse/master/src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php$150

   $type_close = DifferentialRevisionCloseTransaction::TRANSACTIONTYPE;
    $xactions[] = $revision->getApplicationTransactionTemplate()
      ->setTransactionType($type_close)
      ->setNewValue(true)
      ->setMetadataValue('isCommitClose', true)
      ->setMetadataValue('revisionMatchData', $match_data)
      ->setMetadataValue('commitPHID', $commit->getPHID());

    $extraction_engine = id(new DifferentialDiffExtractionEngine())
      ->setViewer($acting_user)
      ->setAuthorPHID($acting_phid);

    $content_source = $this->newContentSource();

    $extraction_engine->updateRevisionWithCommit(
      $revision,
      $commit,
      $xactions,
      $content_source);

Thanks in advance…

This is intended/expected behavior; Differential has worked this way for many years.

“Diff 1” is the diff I submitted with arc diff that was reviewed.
“Diff 2” is the synthetic diff as it actually landed.

There is one known bug described in https://secure.phabricator.com/T13290, but it is primarily related to merge/empty commits.

For example, note that https://secure.phabricator.com/D10620 (an arbitrary old change that landed in 2014) has the same diff behavior.

Perfect thank you…only just noticed it…