Make some UI things less confusing and reduce the amount of horrific torture

that arc apparently inflicts upon users.

Summary:

Test Plan: .

Reviewers:

CC:

Differential Revision: 223104

src/repository/api/base/ArcanistRepositoryAPI.php

@@ -33,9 +33,13 @@ abstract class ArcanistRepositoryAPI {
   const FLAG_UNCOMMITTED  = 128;
   const FLAG_EXTERNALS    = 256;
 
-  // Occurs in SVN when you replace a file with a directory.
+  // Occurs in SVN when you replace a file with a directory without telling
+  // SVN about it.
   const FLAG_OBSTRUCTED   = 512;
 
+  // Occurs in SVN when an update was interrupted or failed, e.g. you ^C'd it.
+  const FLAG_INCOMPLETE   = 1024;
+
   protected $path;
   protected $diffLinesOfContext = 0x7FFF;
 
@@ -110,6 +114,10 @@ abstract class ArcanistRepositoryAPI {
     return $this->getWorkingCopyFilesWithMask(self::FLAG_CONFLICT);
   }
 
+  public function getIncompleteChanges() {
+    return $this->getWorkingCopyFilesWithMask(self::FLAG_INCOMPLETE);
+  }
+
   private function getWorkingCopyFilesWithMask($mask) {
     $match = array();
     foreach ($this->getWorkingCopyStatus() as $file => $flags) {

src/repository/api/subversion/ArcanistSubversionAPI.php

@@ -125,6 +125,9 @@ class ArcanistSubversionAPI extends ArcanistRepositoryAPI {
           case 'conflicted':
             $mask |= self::FLAG_CONFLICT;
             break;
+          case 'incomplete':
+            $mask |= self::FLAG_INCOMPLETE;
+            break;
           default:
             throw new Exception("Unrecognized item status '{$item}'.");
         }

src/workflow/base/ArcanistBaseWorkflow.php

@@ -319,14 +319,36 @@ class ArcanistBaseWorkflow {
     $untracked = $api->getUntrackedChanges();
     if ($this->shouldRequireCleanUntrackedFiles()) {
       if (!empty($untracked)) {
-        throw new ArcanistUsageException(
+
-          "You have untracked files in this working copy:\n".
+        echo "You have untracked files in this working copy:\n\n".
-          "    ".implode("\n    ", $untracked)."\n\n".
+             "    ".implode("\n    ", $untracked)."\n\n";
-          "Add or delete them before proceeding, or include them in your ".
+
-          "ignore rules. To bypass this check, use --allow-untracked.");
+        if ($api instanceof ArcanistGitAPI) {
+          echo phutil_console_wrap(
+            "Since you don't have .gitignore rules for these files and have ".
+            "not listed them in .git/info/exclude, you may have forgotten ".
+            "to 'git add' them to your commit.");
+        } else if ($api instanceof ArcanistSubversionAPI) {
+          echo phutil_console_wrap(
+            "Since you don't have svn:ignore rules for these files, you may ".
+            "have forgotten to 'svn add' them.");
+        }
+
+        $prompt = "Do you want to continue without adding these files?";
+        if (!phutil_console_confirm($prompt, $default_no = false)) {
+          throw new ArcanistUserAbortException();
+        }
       }
     }
 
+    $incomplete = $api->getIncompleteChanges();
+    if ($incomplete) {
+      throw new ArcanistUsageException(
+        "You have incompletely checked out directories in this working copy. ".
+        "Fix them before proceeding: \n\n".
+        "    ".implode("\n    ", $incomplete)."\n\n".
+        "You can fix these paths by running 'svn update' on them.");
+    }
 
     if ($api->getMergeConflicts()) {
       throw new ArcanistUsageException(

src/workflow/base/__init__.php

@@ -9,6 +9,7 @@
 phutil_require_module('arcanist', 'differential/revision');
 phutil_require_module('arcanist', 'exception');
 phutil_require_module('arcanist', 'exception/usage');
+phutil_require_module('arcanist', 'exception/usage/userabort');
 phutil_require_module('arcanist', 'parser/bundle');
 phutil_require_module('arcanist', 'parser/diff');
 phutil_require_module('arcanist', 'parser/diff/change');

src/workflow/diff/ArcanistDiffWorkflow.php

@@ -203,6 +203,14 @@ EOTEXT
 
     $paths = $this->generateAffectedPaths();
 
+    // Do this before we start linting or running unit tests so we can detect
+    // things like a missing test plan or invalid reviewers immediately.
+    if ($this->shouldOnlyCreateDiff()) {
+      $commit_message = null;
+    } else {
+      $commit_message = $this->getGitCommitMessage();
+    }
+
     $lint_result = $this->runLint($paths);
     $unit_result = $this->runUnit($paths);
 
@@ -322,7 +330,7 @@ EOTEXT
         "        **Diff URI:** __%s__\n\n",
         $diff_info['uri']);
     } else {
-      $message = $this->getGitCommitMessage();
+      $message = $commit_message;
 
       $revision = array(
         'diffid' => $diff_info['diffid'],
@@ -353,6 +361,15 @@ EOTEXT
         }
 
         $should_edit = $this->getArgument('edit');
+
+/*
+
+  TODO: This is a complicated mess. We need to move to storing a checksum
+  of the non-auto-sync fields as they existed at original diff time and using
+  changes from that to detect user edits, not comparison of the client and
+  server values since they diverge without user edits (because of Herald
+  and explicit server-side user changes).
+
         if (!$should_edit) {
           $local_sum = $message->getChecksum();
           $remote_sum = $remote_message->getChecksum();
@@ -366,6 +383,7 @@ EOTEXT
               $default_no = false);
           }
         }
+*/
 
         $revision['fields'] = $remote_message->getFields();