ghstack: fix "land" rebase

Summary:
First, don't use "--keep" since that results in a duplicate set of commits visible in smartlog. It also makes it hard to map between the rebased commits since it doesn't record mutation markers. Further, note that "--keep" does not output the "nodechanges" data, so we were always assuming the rebase did no work, and would proceed to push the un-rebased commits. This would lead to a "non-fast forward" error whenever the default branch was newer than the stack's base.

Second, use "rebase -r" instead of "rebase -s". "-s" was rebasing the entire stack in the first loop iteration which causes two problems: 1) It rebases too much (i.e. it can rebase later commits in your stack you aren't trying to land right now). This is bad because you could see conflicts in commits you aren't trying to land, which doesn't make sense. And 2) it messes up the loop's mapping of commits since the loop assumes it is rebasing one commit a time. This issue was probably moot due to the "--keep" issue.

I also added some logic to "goto" back to the un-rebased commit after the rebase. In general, landing commits should not change the working copy. We also "hide" the commits created by rebase so the user won't see duplicate commits in their smartlog if there is a failure.

Finally, I tweaked the rebase logic to be more conservative wrt commits being "rebased out". If one of the commits being landed is already on the target branch, we abort and ask the user to rebase manually.

Fixes #333.

Reviewed By: bolinfest

Differential Revision: D42203379

fbshipit-source-id: 39a8dcd4d65e0535fc2379e4ad16fea902bb88d0

eden/scm/ghstack/sapling_land.py

@@ -1,10 +1,11 @@
 import json
 import re
 
+from edenscm import error
+from edenscm.i18n import _
 import ghstack.github
 import ghstack.github_utils
 import ghstack.sapling_shell
-from ghstack.ghs_types import GitCommitHash
 
 
 def main(pull_request: str,
@@ -56,13 +57,20 @@ def main(pull_request: str,
         sh.git("rev-list", "--reverse", "--header", "^" + base, orig_oid),
         github_url=github_url,
     )
+    if not stack:
+        raise error.Abort(_("No stack commits found between base %r and head %r.") % (base, orig_oid))
+
+    starting_parent = sh.run_sapling_command("whereami")
 
     try:
         # Compute the metadata for each commit
         stack_orig_refs = []
         for s in stack:
             pr_resolved = s.pull_request_resolved
             # We got this from GitHub, this better not be corrupted
+            if pr_resolved is None:
+                raise error.Abort(_("Couldn't find PR info in commit header for %r.") % s.oid)
+
             assert pr_resolved is not None
 
             stack_orig_refs.append(ghstack.github_utils.lookup_pr_to_orig_ref(
@@ -71,17 +79,43 @@ def main(pull_request: str,
                 name=pr_resolved.repo,
                 number=pr_resolved.number))
 
-        # Rebase each commit in the stack onto the default branch.
-        rebase_base = default_branch_oid
-        for s in stack:
-            stdout = sh.run_sapling_command("rebase", "--keep", "-s", s.oid, "-d", rebase_base, "-q", "-T", "{nodechanges|json}")
-            mappings = json.loads(stdout) if stdout else None
-            if not mappings:
-                # If there was no stdout, then s.oid was not rebased because
-                # its parent is already the existing `rebase_base`.
-                rebase_base = s.oid
-            else:
-                rebase_base = mappings[s.oid][0]
+        # Rebase each commit we are landing onto the default branch. Be careful
+        # not to rebase further descendants since they may conflict.
+        stdout = sh.run_sapling_command(
+            "rebase",
+            *(x for s in stack for x in ("-r", s.oid)),
+            "-d",
+            default_branch_oid,
+            "-q",
+            "-T",
+            "{nodechanges|json}",
+        )
+
+        mappings = json.loads(stdout) if stdout else None
+        stack_top_oid = stack[-1].oid
+        if mappings:
+            if starting_parent in mappings:
+                # Move back to our starting position in case rebased moved us (i.e.
+                # the user has the stack checked out). The user may have further
+                # descendant commits we didn't rebase, so we don't want to leave
+                # them on a rebased commit. Also, if the "push" fails, we want to
+                # leave them in their starting position.
+                sh.run_sapling_command("goto", "-q", starting_parent)
+
+            # Hide the rebased commits we created. They are internal to "land",
+            # and we don't want them to be visible if the "push" fails.
+            sh.run_sapling_command("hide", "-q", *(x for v in mappings.values() for x in ("-r", v[0])))
+
+            # If only a subset of commits were rebased, that means some commits
+            # were "rebased out". There is a higher likelihood of a semantic
+            # change, so make the user update their stack.
+            if len(mappings) != len(stack):
+                raise error.Abort(_("One or more commits in stack already exists on %s.\nPlease manually rebase and update stack.") % (default_branch))
+
+            push_rev = mappings[stack_top_oid][0]
+        else:
+            # No mappings means the stack is already based on the branch head.
+            push_rev = stack_top_oid
 
         # Advance base to head to "close" the PR for all PRs.
         # This has to happen before the push because the push
@@ -105,7 +139,7 @@ def main(pull_request: str,
             ghstack.github_utils.update_ref(github=github, repo_id=repo_id, ref=base_ref, target_ref=head_ref)
 
         # All good! Push!
-        sh.run_sapling_command("push", "--rev", rebase_base, "--to", default_branch)
+        sh.run_sapling_command("push", "--rev", push_rev, "--to", default_branch)
 
         # Delete the branches
         for orig_ref in stack_orig_refs:

eden/scm/tests/test-ghstack-land.t

@@ -96,22 +96,25 @@ Make a newer commit upstream
   $ git commit -qm z
   $ git checkout -q some-other-branch
 
-Land only the first commit (XXX failing test to be fixed in next diff):
+Land only the first commit
   $ cd $TESTTMP/client
-  $ sl ghstack land https://github.com/facebook/test_github_repo/pull/1 2>&1 | grep rejected
-   ! [rejected]        f4145ca987ad7b3d5c0e733adfebd79602bc423b -> main (non-fast-forward)
-  hint: Updates were rejected because a pushed branch tip is behind its remote
-  $ sl smartlog -T '{node|short} {desc|firstline} {github_pull_request_number}'
-  @  5bdb55e2f7d2 d 2
-  │
-  o  a044d43b2850 c 1
-  │
-  o  4fd2a518c050 z
-  │
-  │ o  012099ee3b71 d 2
-  │ │
-  │ o  f4145ca987ad c 1
-  ├─╯
-  o  6587f91ed4d9 b 1
+  $ sl ghstack land https://github.com/facebook/test_github_repo/pull/1
+  pulling from file:/*/$TESTTMP/upstream (glob)
+  From file:/*/$TESTTMP/upstream (glob)
+     6587f91..4fd2a51  4fd2a518c0503ce36515d98ba908c486679a8854 -> remote/main
+  To file:/*/$TESTTMP/upstream (glob)
+     4fd2a51..a044d43  a044d43b28501875ff456906aee7a53b1e2454f7 -> main
+  To file:/*/$TESTTMP/upstream (glob)
+   - [deleted]         gh/test/0/base
+   - [deleted]         gh/test/0/head
+   - [deleted]         gh/test/0/orig
+  $ sl smartlog -T '{node|short} {bookmarks} {desc|firstline}'
+  o  a044d43b2850  c
+  ╷
+  ╷ @  012099ee3b71  d
+  ╷ │
+  ╷ x  f4145ca987ad  c
+  ╭─╯
+  o  6587f91ed4d9  b
   │
   ~