From 78e9cc9c0129053b95b9d0cc405640bb0edaa927 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 8 Jun 2020 06:37:18 -0700
Subject: [PATCH] Add a check for ambiguous merge strategies after the
 "history.immutable" behavioral change

Summary: Ref T13546. When users hit a situation where we would select "squash" but would previously select "merge", prevent them from continuing under ambiguous conditions.

Test Plan: Ran "arc land" in Git with "history.immutable" true, false, and not configured.

Maniphest Tasks: T13546

Differential Revision: https://secure.phabricator.com/D21332
---
 src/land/engine/ArcanistGitLandEngine.php | 48 +++++++++++++++++++++++
 src/land/engine/ArcanistLandEngine.php    | 22 +++++++----
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/src/land/engine/ArcanistGitLandEngine.php b/src/land/engine/ArcanistGitLandEngine.php
index 1e237991..57e8b61f 100644
--- a/src/land/engine/ArcanistGitLandEngine.php
+++ b/src/land/engine/ArcanistGitLandEngine.php
@@ -229,6 +229,8 @@ final class ArcanistGitLandEngine
     $api = $this->getRepositoryAPI();
     $log = $this->getLogEngine();
 
+    $this->confirmLegacyStrategyConfiguration();
+
     $is_empty = ($into_commit === null);
 
     if ($is_empty) {
@@ -1429,4 +1431,50 @@ final class ArcanistGitLandEngine
     return $refspecs;
   }
 
+  private function confirmLegacyStrategyConfiguration() {
+    // TODO: See T13547. Remove this check in the future. This prevents users
+    // from accidentally executing a "squash" workflow under a configuration
+    // which would previously have executed a "merge" workflow.
+
+    // We're fine if we have an explicit "--strategy".
+    if ($this->getStrategyArgument() !== null) {
+      return;
+    }
+
+    // We're fine if we have an explicit "arc.land.strategy".
+    if ($this->getStrategyFromConfiguration() !== null) {
+      return;
+    }
+
+    // We're fine if "history.immutable" is not set to "true".
+    $source_list = $this->getWorkflow()->getConfigurationSourceList();
+    $config_list = $source_list->getStorageValueList('history.immutable');
+    if (!$config_list) {
+      return;
+    }
+
+    $config_value = (bool)last($config_list)->getValue();
+    if (!$config_value) {
+      return;
+    }
+
+    // We're in trouble: we would previously have selected "merge" and will
+    // now select "squash". Make sure the user knows what they're in for.
+
+    echo tsprintf(
+      "\n%!\n%W\n\n",
+      pht('MERGE STRATEGY IS AMBIGUOUS'),
+      pht(
+        'See <%s>. The default merge strategy under Git with '.
+        '"history.immutable" has changed from "merge" to "squash". Your '.
+        'configuration is ambiguous under this behavioral change. '.
+        '(Use "--strategy" or configure "arc.land.strategy" to bypass '.
+        'this check.)',
+        'https://secure.phabricator.com/T13547'));
+
+    throw new PhutilArgumentUsageException(
+      pht(
+        'Desired merge strategy is ambiguous, choose an explicit strategy.'));
+  }
+
 }
diff --git a/src/land/engine/ArcanistLandEngine.php b/src/land/engine/ArcanistLandEngine.php
index bb3d5f95..da354e27 100644
--- a/src/land/engine/ArcanistLandEngine.php
+++ b/src/land/engine/ArcanistLandEngine.php
@@ -269,13 +269,17 @@ abstract class ArcanistLandEngine extends Phobject {
     return $this->localState;
   }
 
+  final protected function getOntoConfigurationKey() {
+    return 'arc.land.onto';
+  }
+
   final protected function getOntoFromConfiguration() {
     $config_key = $this->getOntoConfigurationKey();
     return $this->getWorkflow()->getConfig($config_key);
   }
 
-  final protected function getOntoConfigurationKey() {
-    return 'arc.land.onto';
+  final protected function getOntoRemoteConfigurationKey() {
+    return 'arc.land.onto-remote';
   }
 
   final protected function getOntoRemoteFromConfiguration() {
@@ -283,8 +287,13 @@ abstract class ArcanistLandEngine extends Phobject {
     return $this->getWorkflow()->getConfig($config_key);
   }
 
-  final protected function getOntoRemoteConfigurationKey() {
-    return 'arc.land.onto-remote';
+  final protected function getStrategyConfigurationKey() {
+    return 'arc.land.strategy';
+  }
+
+  final protected function getStrategyFromConfiguration() {
+    $config_key = $this->getStrategyConfigurationKey();
+    return $this->getWorkflow()->getConfig($config_key);
   }
 
   final protected function confirmRevisions(array $sets) {
@@ -1457,8 +1466,7 @@ abstract class ArcanistLandEngine extends Phobject {
       return $strategy;
     }
 
-    $strategy_key = 'arc.land.strategy';
-    $strategy = $this->getWorkflow()->getConfig($strategy_key);
+    $strategy = $this->getStrategyFromConfiguration();
     if ($strategy !== null) {
       if (!isset($supported_strategies[$strategy])) {
         throw new PhutilArgumentUsageException(
@@ -1474,7 +1482,7 @@ abstract class ArcanistLandEngine extends Phobject {
         pht(
           'Merging with "%s" strategy, configured with "%s".',
           $strategy,
-          $strategy_key));
+          $this->getStrategyConfigurationKey()));
 
       return $strategy;
     }