1963406 - Automatically reopened revisions are getting reverted & associated with different repo after merge

Status: RESOLVED FIXED

(Conduit :: Phabricator, defect, P2)

Description

[Zeid [:zeid]]

E.g., https://phabricator.services.mozilla.com/D246494, https://phabricator.services.mozilla.com/D246494, https://phabricator.services.mozilla.com/D246242, https://phabricator.services.mozilla.com/D246679.

This may also be related to an issue that moz-phab is encountering while submitting revised patches, which is:

Phabricator Error: array_walk_recursive() expects parameter 1 to be array, null given

Comment 1

[Zeid [:zeid]]

Workaround: manually change the repository field on affected revisions from firefox-autoland to mozilla-central. Or, in the case of beta firefox-beta to mozilla-beta, etc...

Comment 2

[Mathew Hodson]

I don't know if it's related, but something strange seems to have happened to https://phabricator.services.mozilla.com/D246494 on May 26. It's now associated with rFIREFOXBETA firefox-beta when previously it was rMOZILLACENTRAL mozilla-central. If you look in Phabricator, thee only activity is "This revision was automatically updated to reflect the committed changes." There isn't any new commit though so it's not clear what caused that update.

Comment 4

[Mathew Hodson]

The revision contents history is also strange. For example in https://phabricator.services.mozilla.com/D246494

Diff	ID	Base	Description						Created
Diff 10	1026270									Thu, May 1, 4:51 PM
Diff 11	1042956	2118795	rFIREFOXBETA11b70498842daafd5b6f10aa472e6200c26217fc	Mon, Apr 28, 10:17 AM

The latest diff, which seems like the one that updated the repo, actually has an earlier date than the previous diff.

The Base link on that diff goes to a commit on firefox-autoland not firefox-beta.

Comment 5

[Donal Meehan [:dmeehan]]

This bug also has the side effect of creating unnecessary beta-uplift requests
Example https://bugzilla.mozilla.org/show_bug.cgi?id=1844878#a58418056_600971

Comment 6

[Rob Wu [:robwu]]

When I encountered this issue, I thought that it was just changing the repo, and it is easy enough to change it back via the Phabricator UI.
But it is worse! When an attempt to reland with Lando failed, I took a look at the patch and was surprised that the patch was reverted to a previous state:

• Lando: https://lando.moz.tools/D248537/
• Patch that I intended to land: https://phabricator.services.mozilla.com/D248537?id=1040923#toc
  (browser-init.js contains TODO bug 1967937: and gBrowser.adoptTabGroup(tabToAdopt, { tabIndex: 0, selectTab: true });)
• Patch that was added because Nightly went to Beta: https://phabricator.services.mozilla.com/D248537?id=1045183#toc
  (browser-init.js does not contain TODO and has gBrowser.adoptTabGroup(tabToAdopt, 0);)
• Diff: https://phabricator.services.mozilla.com/D248537?vs=1040923&id=1045183#toc

I updated the patch to what I had locally. This bug is however surprising, and can result in people landing something other than what they intended!

Comment 7

[Mathew Hodson]

This issue is similar to the one fixed in bug 1959490, but that bug only prevented closing revisions. They are still getting updated.

I think you want to move the mustCloseRevision logic from DiffusionUpdateObjectAfterCommitWorker.php to closeRevisions in PhabricatorRepositoryCommitPublishWorker.php.

Something like this:

    $revision = id(new DifferentialRevisionQuery())
      ->setViewer($actor)
      ->withIDs(array($revision_id))
      ->executeOne();
    if (!$revision) {
      return;
    }

    $revisionRepositoryCallsign = $revision->getRepository()->getCallsign();
    $commitRepositoryCallsign = $commit->getRepository()->getCallsign();

    $mustCloseRevision = false;
    if ($revisionRepositoryCallsign === $commitRepositoryCallsign) {
      $mustCloseRevision = true;
    } else {
      $config = PhabricatorEnv::getEnvConfig('diffusion.legacy-repos-mapping');
      if (array_key_exists($revisionRepositoryCallsign, $config)) {
        if ($config[$revisionRepositoryCallsign] === $commitRepositoryCallsign) {
          $mustCloseRevision = true;
        }
      }
    }

    if (!$mustCloseRevision) {
      return;
    }

Comment 10

[Zeid [:zeid]]

This bug appears to still affect most revisions that were backed out in autoland, and were merged today into beta. Those revisions had their target repo changed and the actual diff reverted as well. Manual steps are needed to revert the patch back to what is expected, and to fix the repository on the revision itself.

Comment 11

[Zeid [:zeid]]

RE: Comment 7, this approach seems to prevent Phabricator from attaching a "revert" action to the revision when there is a repository mismatch, and instead adds a "mention" in the timeline.

I have not been able to reproduce this issue in a controllable situation unfortunately (i.e., in my local environment) despite using the same steps that trigger this issue in production. I could try implementing this and ensuring it has no undesired side effects, to see if it helps with this bug next time there is a merge.

Comment 12

[Zeid [:zeid]]

Attachment: GitHub Pull Request

Comment 13

[Mathew Hodson]

https://github.com/mozilla-conduit/phabricator/commit/e2b43c6bc619238dce015fd1987ed1ac48d32780