Cherry pick test online for direct

ghstack/submit.py

@@ -1,4 +1,4 @@
 #!/usr/bin/env python3
 
 import dataclasses
 import itertools
@@ -1181,7 +1181,98 @@
                 head_args.extend(("-p", new_base))
                 push_branches.base.update(GhCommit(new_base, base.tree))
         else:
+            # So, there is some complication here.  We're computing what base
+            # to use based on the local situation on the user diff stack, but
+            # the remote merge structure may disagree with our local
+            # situation.  For example, suppose I have a commit stack A - B,
+            # and then I insert a new commit A - M - B between them;
+            # previously B would have been based on A, but now it is based
+            # on M.  What should happen here?
+            #
+            # Here are the high level correctness conditions:
+            # - No force pushes (history must be preserved)
+            # - GitHub displays a diff which is equivalent to the original
+            #   user diff
+            #
+            # It turns out the logic here is fine, and the only thing it
+            # chokes on is rebasing back in time on master branch (you can't
+            # go back in time on PR branches, so this is a moot point there.)
+            # The problem is suppose you have:
+            #
+            #   A - B - C
+            #    \   \
+            #     M2  M1  # M1 was cherry-picked onto A, becoming M2
+            #
+            # In branch form, this becomes:
+            #
+            #   A - B - C
+            #    \   \
+            #     \   M1 - M2
+            #      \       /
+            #       \-----/
+            #
+            # However, the merge base for C and M2 will always be computed to
+            # be B, because B is an ancestor of both C and M2, and it always
+            # beets out A (which is an ancestor of B).  This means that you
+            # will diff M2 against B, which will typically result in "remove
+            # changes from B" spuriously showing up on the PR.
+            #
+            # When heads are always monotonically moving forward in time,
+            # there is not any problem with progressively more complicated
+            # merge histories, because we always specify the "correct" base
+            # branch.  For example, consider:
+            #
+            #   A - B
+            #        \
+            #         \- X - Y1
+            #          \
+            #           \- Y2
+            #
+            # Where Y1 is cherry-picked off of X onto B directly.  In branch
+            # form, this becomes:
+            #
+            #   A - B
+            #        \
+            #         \- X - Y1 - Y2
+            #
+            # But we update the base branch to be B, so we correctly diff Y2
+            # against B (where here, the tree for Y2 no longer incorporates
+            # the changes for X).
+            #
+            # What does NOT work in this situation is if you manually (outside
+            # of ghstack) retarget Y2 back at X; we will spuriously report
+            # that the diff X and Y2 removes the changes from X.  If you use
+            # ghstack, however, we will do this:
+            #
+            #   A - B
+            #        \
+            #         \- X - Y1 - Y2 - Y3
+            #
+            # Where here Y3 has restored the changes from X, so the diff from
+            # X to Y3 checks out.
+            #
+            # It turns out there are a subset of manipulations, for which it
+            # is always safe to change the target base commit from GitHub UI
+            # without pushing a new commit.  Intuitively, the idea is that
+            # once you add a commit as a merge base, you can't take it back:
+            # we always consider that branch to have been "merged in".  So
+            # you can effectively only ever insert new commits between
+            # pre-existing commits, but once a commit depends on another
+            # commit, that dependency must always exist.  I'm still
+            # considering whether or not we should force push by default in
+            # this sort of situation.
+            #
+            # By the way, what happens if you reorder commits?  You get this
+            # funny looking graph:
+            #
+            # A - B
+            #      \
+            #       X - Y - Y2
+            #        \        \
+            #         \------- X2
+
+
             # We never have to create a base commit, we read it out from
             # the base
             if base_diff_meta is not None:
                 # The base was submitted the normal way (merge base is either
@@ -1193,6 +1284,8 @@
                     assert base_diff_meta.next == base_diff_meta.head
                 new_base = base_diff_meta.next
             else:
+                # TODO: test that there isn't a more recent ancestor
+                # such that this doesn't actually work
                 new_base = base.commit_id
 
             # Check if the base is already an ancestor, don't need to add it
@@ -1483,8 +1576,10 @@
         (base_commit,) = ghstack.git.split_header(
             self.sh.git("rev-list", "--header", "-1", f"{self.remote_name}/{base_ref}")
         )
-        # TODO: tree equality may not hold for self.direct
-        assert_eq(base_commit.tree, user_parent_commit.tree)
+        # TODO: tree equality may not hold for self.direct, figure out a
+        # related invariant
+        if not self.direct:
+            assert_eq(base_commit.tree, user_parent_commit.tree)
 
         # 6.  Orig commit was correctly pushed
         assert_eq(

test_ghstack.py

@@ -1362,6 +1362,58 @@ def test_cherry_pick(self) -> None:
     * c7e3a0c
          Update base for Initial 2 on "Commit B"
 
+""",
+        )
+
+    @use_direct()
+    def test_direct_cherry_pick(self) -> None:
+        self.sh.git("checkout", "-b", "feature")
+
+        self.commit("A")
+        self.commit("B")
+        A, B = self.gh("Initial 2")
+
+        self.sh.git("checkout", "master")
+        self.commit("M")
+        self.sh.git("push", "origin", "master")
+
+        self.cherry_pick(B)
+        self.gh("Cherry pick")
+
+        self.assertExpectedInline(
+            self.dump_github(),
+            """\
+[O] #500 Commit A (gh/ezyang/1/head -> master)
+
+    Stack:
+    * #501
+    * __->__ #500
+
+
+
+    * 2949b6b (gh/ezyang/1/next, gh/ezyang/1/head)
+    |    Initial 2 on "Commit A"
+    * dc8bfe4
+         Initial commit
+
+[O] #501 Commit B (gh/ezyang/2/head -> gh/ezyang/1/head)
+
+    Stack:
+    * __->__ #501
+
+
+
+    *   fd891f3 (gh/ezyang/2/next, gh/ezyang/2/head)
+    |\\     Cherry pick on "Commit B"
+    | * 686e5ea (HEAD -> master)
+    | |    Commit M
+    * | d8884f2
+    | |    Initial 2 on "Commit B"
+    * | 2949b6b (gh/ezyang/1/next, gh/ezyang/1/head)
+    |/     Initial 2 on "Commit A"
+    * dc8bfe4
+         Initial commit
+
 """,
         )