Fix a case where "arc patch" could skip submodule changes

Summary:
See PHI1083. Previously, see PHI648 and D19475.

When you apply a submodule patch in Git, it leaves you with a working copy that has the "submodule pointer" dirtied but the actual submodule untouched:

```
$ git status
On branch ...
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   philter

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   philter (new commits)
```

So, if you're applying `D123` and `submodule/` was previously pointed at commit "A" but `D123` updates it to point at commit "B", you get this after `git apply ...`:

  - Git index says "submodule/ = B".
  - On disk, "submodule/ = A".

Now, if you `git add --all` or `git commit --all`, git picks up the "change" on disk as an intended modification of the submodule. This puts the submodule back to "A" and overwrites/undoes the "pointer" update that's trying to make it point to "B".

To avoid this, update submodules after applying the patch.

Also, every time we modify the working copy, just update submodules.

Test Plan:
  - Add a submodule in branch "B1", pointed at commit "A".
  - Branch to create branch "B2". Update the submodule to point at commit "B". Commit this and `arc diff` it.
  - Go back to "B1". Use `arc patch D...` to apply the revision you just created.
  - Before change:
    - "arc patch" applies the submodule change, so "pointer = B", "disk = A".
    - "arc patch" runs "git commit --all", which looks at disk and sets "pointer = A".
    - This isn't a change, so we fail with an empty commit.
  - After change:
    - "arc patch" applies the submodule change, so "pointer = B", "disk = A".
    - "arc patch" updates submodules, so "pointer = B", "disk = B".
    - "arc patch" runs "git commit --all", which now has a change, and commits "submodule = B".

Reviewers: amckinley

Reviewed By: amckinley

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

src/workflow/ArcanistPatchWorkflow.php

@@ -307,6 +307,14 @@ EOTEXT
         $repository_api->execxLocal('checkout -b %s', $branch_name);
       }
 
+      // Synchronize submodule state, since the checkout may have modified
+      // submodule references. See PHI1083.
+
+      // Note that newer versions of "git checkout" include a
+      // "--recurse-submodules" flag which accomplishes this goal a little
+      // more simply. For now, use the more compatible form.
+      $repository_api->execPassthru('submodule update --init --recursive');
+
       echo phutil_console_format(
         "%s\n",
         pht(
@@ -714,6 +722,24 @@ EOTEXT
         throw new ArcanistUsageException(pht('Unable to apply patch!'));
       }
 
+      // See PHI1083 and PHI648. If the patch applied changes to submodules,
+      // it only updates the submodule pointer, not the actual submodule. We're
+      // left with the pointer update staged in the index, and the unmodified
+      // submodule on disk.
+
+      // If we then "git commit --all" or "git add --all", the unmodified
+      // submodule on disk is added to the index as a change, which effectively
+      // undoes the patch we just applied and reverts the submodule back to
+      // the previous state.
+
+      // To avoid this, do a submodule update before we continue.
+
+      // We could also possibly skip the "--all" flag so we don't have to do
+      // this submodule update, but we want to leave the working copy in a
+      // clean state anyway, so we're going to have to do an update at some
+      // point. This usually doesn't cost us anything.
+      $repository_api->execPassthru('submodule update --init --recursive');
+
       if ($this->shouldCommit()) {
         if ($bundle->getFullAuthor()) {
           $author_cmd = csprintf('--author=%s', $bundle->getFullAuthor());
@@ -735,14 +761,23 @@ EOTEXT
       if ($this->canBranch() &&
           !$this->shouldBranch() &&
           $this->shouldCommit() && $has_base_revision) {
+
+        // See PHI1083 and PHI648. Synchronize submodule state after mutating
+        // the working copy.
+
         $repository_api->execxLocal('checkout %s', $original_branch);
+        $repository_api->execPassthru('submodule update --init --recursive');
+
         $ex = null;
         try {
           $repository_api->execxLocal('cherry-pick %s', $new_branch);
+          $repository_api->execPassthru('submodule update --init --recursive');
         } catch (Exception $ex) {
           // do nothing
         }
+
         $repository_api->execxLocal('branch -D %s', $new_branch);
+
         if ($ex) {
           echo phutil_console_format(
             "\n<bg:red>** %s**</bg>\n",
@@ -751,11 +786,6 @@ EOTEXT
         }
       }
 
-      // Synchronize submodule state, since the patch may have made changes
-      // to ".gitmodules". We do this after we finish managing branches so
-      // the behavior is correct under "--nobranch"; see PHI648.
-      $repository_api->execPassthru('submodule update --init --recursive');
-
       echo phutil_console_format(
         "<bg:green>** %s **</bg> %s\n",
         pht('OKAY'),