From dfde57ff812b1cd552c0fd2ca519c7e15171b3a4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 22 Oct 2015 19:55:16 +0000
Subject: [PATCH] Improve two error handling behaviors in `arc upgrade`

Summary:
Fixes T9222. Two issues here:

  - First, we currently continue on error. Throw instead. I just swapped us from "phutil_passthru()" to "execx()" since I don't think printing out the "pulling from remote..." status messages is very important, and this makes it easier to raise a useful exception.
  - Second, if you have a dirty working copy we currently may try to do some sort of silly stuff which won't work, like prompt you to amend changes. Instead, do a slightly lower-level check and just bail.

Test Plan:
  - Ran `arc upgrade` with a dirty working copy and got a tailored, useful error.
  - Ran `arc upgrade` with an artificially bad `git pull` command, got a failure with a specific error message.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9222

Differential Revision: https://secure.phabricator.com/D14317
---
 src/workflow/ArcanistUpgradeWorkflow.php | 29 +++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/src/workflow/ArcanistUpgradeWorkflow.php b/src/workflow/ArcanistUpgradeWorkflow.php
index c9e0e858..50449ef7 100644
--- a/src/workflow/ArcanistUpgradeWorkflow.php
+++ b/src/workflow/ArcanistUpgradeWorkflow.php
@@ -53,7 +53,27 @@ EOTEXT
 
       $this->setRepositoryAPI($repository);
 
-      $this->requireCleanWorkingCopy();
+      // NOTE: Don't use requireCleanWorkingCopy() here because it tries to
+      // amend changes and generally move the workflow forward. We just want to
+      // abort if there are local changes and make the user sort things out.
+      $uncommitted = $repository->getUncommittedStatus();
+      if ($uncommitted) {
+        $message = pht(
+          'You have uncommitted changes in the working copy for this '.
+          'library:');
+
+        $list = id(new PhutilConsoleList())
+          ->setWrap(false)
+          ->addItems(array_keys($uncommitted));
+
+        id(new PhutilConsoleBlock())
+          ->addParagraph($message)
+          ->addList($list)
+          ->draw();
+
+        throw new ArcanistUsageException(
+          pht('`arc upgrade` can only upgrade clean working copies.'));
+      }
 
       $branch_name = $repository->getBranchName();
       if ($branch_name != 'master' && $branch_name != 'stable') {
@@ -71,10 +91,13 @@ EOTEXT
       }
 
       chdir($root);
+
       try {
-        phutil_passthru('git pull --rebase');
+        execx('git pull --rebase');
       } catch (Exception $ex) {
-        phutil_passthru('git rebase --abort');
+        // If we failed, try to go back to the old state, then throw the
+        // original exception.
+        exec_manual('git rebase --abort');
         throw $ex;
       }
     }