Improve arc's handling of dirty submodules in Git

Summary:
Fixes T9455. Depends on D14136. When you have a dirty submodule:

  $ nano submodule/file.c # save changes

...we currently ask you to make a commit when you run `arc diff`, which is meaningless and misleading.

Instead, prompt the user separately.

This behavior isn't perfect but I think it's about the best we can do within reason.

Test Plan:
  - Ran `arc diff` in a working copy with uncommitted submodule changes only, got new prompt.
  - Ran `arc diff` in a working copy with submodule base commit changes only, got old (correct) prompt.
  - Ran `arc diff` in a working copy with both, got only old prompt (which is incomplete, but reasonable/meaningful).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9455

Differential Revision: https://secure.phabricator.com/D14137

src/internationalization/ArcanistUSEnglishTranslation.php

@@ -68,6 +68,16 @@ final class ArcanistUSEnglishTranslation extends PhutilTranslation {
         'Ignore this untracked file and continue?',
         'Ignore these untracked files and continue?',
       ),
+
+      '%s submodule(s) have uncommitted or untracked changes:' => array(
+        'A submodule has uncommitted or untracked changes:',
+        'Submodules have uncommitted or untracked changes:',
+      ),
+
+      'Ignore the changes to these %s submodule(s) and continue?' => array(
+        'Ignore the changes to this submodule and continue?',
+        'Ignore the changes to these submodules and continue?',
+      ),
     );
   }
 

src/repository/api/ArcanistGitAPI.php

@@ -672,7 +672,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     $result = new PhutilArrayWithDefaultValue();
 
     list($stdout) = $uncommitted_future->resolvex();
-    $uncommitted_files = $this->parseGitStatus($stdout);
+    $uncommitted_files = $this->parseGitRawDiff($stdout);
     foreach ($uncommitted_files as $path => $mask) {
       $result[$path] |= ($mask | self::FLAG_UNCOMMITTED);
     }
@@ -704,7 +704,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       $this->getDiffBaseOptions(),
       $this->getBaseCommit());
 
-    return $this->parseGitStatus($stdout);
+    return $this->parseGitRawDiff($stdout);
   }
 
   public function getGitConfig($key, $default = null) {
@@ -759,7 +759,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     return $this;
   }
 
-  private function parseGitStatus($status, $full = false) {
+  private function parseGitRawDiff($status, $full = false) {
     static $flags = array(
       'A' => self::FLAG_ADDED,
       'M' => self::FLAG_MODIFIED,
@@ -777,17 +777,51 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     $files = array();
     foreach ($lines as $line) {
       $mask = 0;
+
+      // "git diff --raw" lines begin with a ":" character.
+      $old_mode = ltrim($line[0], ':');
+      $new_mode = $line[1];
+
+      // The hashes may be padded with "." characters for alignment. Discard
+      // them.
+      $old_hash = rtrim($line[2], '.');
+      $new_hash = rtrim($line[3], '.');
+
       $flag = $line[4];
       $file = $line[5];
-      foreach ($flags as $key => $bits) {
-        if ($flag == $key) {
-          $mask |= $bits;
-        }
+
+      $new_value = intval($new_mode, 8);
+      $is_submodule = (($new_value & 0160000) === 0160000);
+
+      if (($is_submodule) &&
+          ($flag == 'M') &&
+          ($old_hash === $new_hash) &&
+          ($old_mode === $new_mode)) {
+        // See T9455. We see this submodule as "modified", but the old and new
+        // hashes are the same and the old and new modes are the same, so we
+        // don't directly see a modification.
+
+        // We can end up here if we have a submodule which has uncommitted
+        // changes inside of it (for example, the user has added untracked
+        // files or made uncommitted changes to files in the submodule). In
+        // this case, we set a different flag because we can't meaningfully
+        // give users the same prompt.
+
+        // Note that if the submodule has real changes from the parent
+        // perspective (the base commit has changed) and also has uncommitted
+        // changes, we'll only see the real changes and miss the uncommitted
+        // changes. At the time of writing, there is no reasonable porcelain
+        // for finding those changes, and the impact of this error seems small.
+
+        $mask |= self::FLAG_EXTERNALS;
+      } else if (isset($flags[$flag])) {
+        $mask |= $flags[$flag];
       }
+
       if ($full) {
         $files[$file] = array(
           'mask' => $mask,
-          'ref'  => rtrim($line[3], '.'),
+          'ref'  => $new_hash,
         );
       } else {
         $files[$file] = $mask;
@@ -807,7 +841,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     list($stdout) = $this->execxLocal(
       'diff --raw %s',
       $since_commit);
-    return $this->parseGitStatus($stdout);
+    return $this->parseGitRawDiff($stdout);
   }
 
   public function getBlame($path) {

src/repository/api/ArcanistRepositoryAPI.php

@@ -15,6 +15,9 @@ abstract class ArcanistRepositoryAPI extends Phobject {
   const FLAG_MISSING      = 32;
   const FLAG_UNSTAGED     = 64;
   const FLAG_UNCOMMITTED  = 128;
+
+  // Occurs in SVN when you have uncommitted changes to a modified external,
+  // or in Git when you have uncommitted or untracked changes in a submodule.
   const FLAG_EXTERNALS    = 256;
 
   // Occurs in SVN when you replace a file with a directory without telling
@@ -192,6 +195,14 @@ abstract class ArcanistRepositoryAPI extends Phobject {
   }
 
 
+  /**
+   * @task status
+   */
+  final public function getDirtyExternalChanges() {
+    return $this->getUncommittedPathsWithMask(self::FLAG_EXTERNALS);
+  }
+
+
   /**
    * @task status
    */

src/workflow/ArcanistFeatureWorkflow.php

@@ -356,8 +356,8 @@ EOTEXT
       foreach ($out as $line) {
         $table->addRow(array(
           'current' => $line['current'] ? '*' : '',
-          'name'    => phutil_console_format('**%s**', $line['name']),
-          'status'  => phutil_console_format(
+          'name'    => tsprintf('**%s**', $line['name']),
+          'status'  => tsprintf(
             "<fg:{$line['color']}>%s</fg>", $line['status']),
           'descr'   => $line['desc'],
         ));

src/workflow/ArcanistListWorkflow.php

@@ -91,11 +91,11 @@ EOTEXT
       $revision = $revisions[$key];
 
       $table->addRow(array(
-        'exists' => $spec['exists'] ? phutil_console_format('**%s**', '*') : '',
-        'status' => phutil_console_format(
+        'exists' => $spec['exists'] ? tsprintf('**%s**', '*') : '',
+        'status' => tsprintf(
           "<fg:{$spec['color']}>%s</fg>",
           $spec['statusName']),
-        'title'  => phutil_console_format(
+        'title'  => tsprintf(
           '**D%d:** %s',
           $revision['id'],
           $revision['title']),

src/workflow/ArcanistPhrequentWorkflow.php

@@ -56,7 +56,7 @@ abstract class ArcanistPhrequentWorkflow extends ArcanistWorkflow {
 
       $table->addRow(array(
         'type' => '('.$column_type.')',
-        'time' => phutil_format_relative_time($result['time']),
+        'time' => tsprintf($result['time']),
         'name' => $phid_map[$result['phid']],
       ));
 

src/workflow/ArcanistTasksWorkflow.php

@@ -111,7 +111,7 @@ EOTEXT
 
       // Render the "T123" column.
       $task_id = 'T'.$task['id'];
-      $formatted_task_id = phutil_console_format('**%s**', $task_id);
+      $formatted_task_id = tsprintf('**%s**', $task_id);
       $output['id'] = $formatted_task_id;
 
       // Render the "Title" column.
@@ -145,7 +145,7 @@ EOTEXT
       } else {
         $color = 'white';
       }
-      $formatted_priority = phutil_console_format(
+      $formatted_priority = tsprintf(
         "<bg:{$color}> </bg> %s",
         $task['priority']);
       $output['priority'] = $formatted_priority;
@@ -159,7 +159,7 @@ EOTEXT
           $status_text = $task['statusName'];
           $status_color = 'green';
         }
-        $formatted_status = phutil_console_format(
+        $formatted_status = tsprintf(
           "<bg:{$status_color}> </bg> %s",
           $status_text);
         $output['status'] = $formatted_status;

src/workflow/ArcanistWorkflow.php

@@ -893,11 +893,47 @@ abstract class ArcanistWorkflow extends Phobject {
           implode("\n    ", $missing)));
     }
 
+    $externals = $api->getDirtyExternalChanges();
+
+    // TODO: This state can exist in Subversion, but it is currently handled
+    // elsewhere. It should probably be handled here, eventually.
+    if ($api instanceof ArcanistSubversionAPI) {
+      $externals = array();
+    }
+
+    if ($externals) {
+      $message = pht(
+        '%s submodule(s) have uncommitted or untracked changes:',
+        new PhutilNumber(count($externals)));
+
+      $prompt = pht(
+        'Ignore the changes to these %s submodule(s) and continue?',
+        new PhutilNumber(count($externals)));
+
+      $list = id(new PhutilConsoleList())
+        ->setWrap(false)
+        ->addItems($externals);
+
+      id(new PhutilConsoleBlock())
+        ->addParagraph($message)
+        ->addList($list)
+        ->draw();
+
+      $ok = phutil_console_confirm($prompt, $default_no = false);
+      if (!$ok) {
+        throw new ArcanistUserAbortException();
+      }
+    }
+
     $uncommitted = $api->getUncommittedChanges();
     $unstaged = $api->getUnstagedChanges();
 
+    // We already dealt with externals.
+    $unstaged = array_diff($unstaged, $externals);
+
     // We only want files which are purely uncommitted.
     $uncommitted = array_diff($uncommitted, $unstaged);
+    $uncommitted = array_diff($uncommitted, $externals);
 
     $untracked = $api->getUntrackedChanges();
     if (!$this->shouldRequireCleanUntrackedFiles()) {