Page MenuHomePhabricator

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

Authored by epriestley on Mar 6 2019, 6:45 PM.

Details

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".

Diff Detail

Repository
rARC Arcanist
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Mar 6 2019, 6:45 PM
epriestley requested review of this revision.Mar 6 2019, 6:45 PM
amckinley accepted this revision.Mar 7 2019, 8:21 PM
This revision is now accepted and ready to land.Mar 7 2019, 8:21 PM
This revision was automatically updated to reflect the committed changes.