From cc850163f30c4697e925df0d6212469679600a2c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 18 Nov 2019 20:28:58 -0800
Subject: [PATCH] When "arc close-revision --finalize ..." skips closing a
 revision, print a message

Summary: Fixes T13458. Emit an explicit message when "arc close-revision --finalize" bails out because the revision is not "Accepted".

Test Plan: Ran `arc close-revision [--finalize] ...` on various revisions, saw more clear messaging.

Maniphest Tasks: T13458

Differential Revision: https://secure.phabricator.com/D20915
---
 .../ArcanistCloseRevisionWorkflow.php         | 49 ++++++++++++++-----
 1 file changed, 37 insertions(+), 12 deletions(-)

diff --git a/src/workflow/ArcanistCloseRevisionWorkflow.php b/src/workflow/ArcanistCloseRevisionWorkflow.php
index af1e3031..363b5dbe 100644
--- a/src/workflow/ArcanistCloseRevisionWorkflow.php
+++ b/src/workflow/ArcanistCloseRevisionWorkflow.php
@@ -89,11 +89,13 @@ EOTEXT
       ));
     $revision = head($revisions);
 
+    $object_name = "D{$revision_id}";
+
     if (!$revision && !$is_finalize) {
       throw new ArcanistUsageException(
         pht(
           'Revision %s does not exist.',
-          "D{$revision_id}"));
+          $object_name));
     }
 
     $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
@@ -104,15 +106,20 @@ EOTEXT
         pht(
           "Revision %s can not be closed. You can only close ".
           "revisions which have been 'accepted'.",
-          "D{$revision_id}"));
+          $object_name));
     }
 
     if ($revision) {
+      $revision_display = sprintf(
+        '%s %s',
+        $object_name,
+        $revision['title']);
+
       if (!$is_finalize && $revision['authorPHID'] != $this->getUserPHID()) {
         $prompt = pht(
-          'You are not the author of revision %s, '.
+          'You are not the author of revision "%s", '.
           'are you sure you want to close it?',
-          "D{$revision_id}");
+          $object_name);
         if (!phutil_console_confirm($prompt)) {
           throw new ArcanistUserAbortException();
         }
@@ -120,24 +127,42 @@ EOTEXT
 
       $actually_close = true;
       if ($is_finalize) {
-        if ($this->getRepositoryPHID() ||
-            $revision['status'] != $status_accepted) {
+        if ($this->getRepositoryPHID()) {
+          $actually_close = false;
+        } else if ($revision['status'] != $status_accepted) {
+          // See T13458. The server doesn't permit a transition to "Closed"
+          // over the API if the revision is not "Accepted". If we won't be
+          // able to close the revision, skip the attempt and print a
+          // message.
+
+          $this->writeWarn(
+            pht('OPEN REVISION'),
+            pht(
+              'Revision "%s" is not in state "Accepted", so it will '.
+              'be left open.',
+              $object_name));
+
           $actually_close = false;
         }
       }
-      if ($actually_close) {
-        $revision_name = $revision['title'];
 
-        echo pht(
-          "Closing revision %s '%s'...\n",
-          "D{$revision_id}",
-          $revision_name);
+      if ($actually_close) {
+        $this->writeInfo(
+          pht('CLOSE'),
+          pht(
+            'Closing revision "%s"...',
+            $revision_display));
 
         $conduit->callMethodSynchronous(
           'differential.close',
           array(
             'revisionID' => $revision_id,
           ));
+
+        $this->writeOkay(
+          pht('CLOSE'),
+          pht(
+            'Done, closed revision.'));
       }
     }