From a590db28b254ef14333d5caaafec02e9beaa1ab4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 4 May 2020 15:57:22 -0700
Subject: [PATCH] Fix an issue where non-ID changeset state keys were used as
 changeset IDs

Summary:
Ref T13519. This is a little fuzzy, but I think the workflow here is:

  - View an intradiff, generating an ephemeral comparison changeset with no changeset ID. This produces a state key of "*".
  - Apply "hidden" state changes to the changeset.
  - View some other intradiff and/or diff view.
  - The code attempts to use "*" as a changset ID?

I'm not entirely sure this is accurate; this was observed in production and I couldn't get a clean reproduction case locally.

Optimistically, try making changeset IDs explicit rather than relying on state keys to be "usually changeset-ID-like".

Test Plan: Used "hidden" locally across multiple intradiffs, but I wasn't cleanly able to reproduce the initial issue.

Maniphest Tasks: T13519

Differential Revision: https://secure.phabricator.com/D21223
---
 .../differential/storage/DifferentialViewState.php |  5 +++++
 .../PhabricatorChangesetViewStateEngine.php        | 14 +++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/applications/differential/storage/DifferentialViewState.php b/src/applications/differential/storage/DifferentialViewState.php
index 101061f379..44c1f308d8 100644
--- a/src/applications/differential/storage/DifferentialViewState.php
+++ b/src/applications/differential/storage/DifferentialViewState.php
@@ -49,6 +49,11 @@ final class DifferentialViewState
       $properties['diffID'] = (int)$diff_id;
     }
 
+    $changeset_id = $changeset->getID();
+    if ($changeset_id !== null) {
+      $properties['changesetID'] = (int)$changeset_id;
+    }
+
     $path_hash = $this->getChangesetPathHash($changeset);
     $changeset_phid = $this->getChangesetKey($changeset);
 
diff --git a/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php b/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php
index 2363f7f0ff..5979ca2a7b 100644
--- a/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php
+++ b/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php
@@ -197,12 +197,12 @@ final class PhabricatorChangesetViewStateEngine
     $entries = isort($entries, 'epoch');
 
     if ($entries) {
-      $other_key = last_key($entries);
       $other_spec = last($entries);
 
       $this_version = (int)$changeset->getDiffID();
       $other_version = (int)idx($other_spec, 'diffID');
       $other_value = (bool)idx($other_spec, 'value', false);
+      $other_id = (int)idx($other_spec, 'changesetID');
 
       if ($other_value === false) {
         $is_hidden = false;
@@ -211,10 +211,14 @@ final class PhabricatorChangesetViewStateEngine
       } else {
         $viewer = $this->getViewer();
 
-        $other_changeset = id(new DifferentialChangesetQuery())
-          ->setViewer($viewer)
-          ->withIDs(array($other_key))
-          ->executeOne();
+        if ($other_id) {
+          $other_changeset = id(new DifferentialChangesetQuery())
+            ->setViewer($viewer)
+            ->withIDs(array($other_id))
+            ->executeOne();
+        } else {
+          $other_changeset = null;
+        }
 
         $is_modified = false;
         if ($other_changeset) {