Updated land.py to do merging after push was successful (#292)

Author: Chillee

src/ghstack/diff.py

@@ -14,7 +14,7 @@
 
 
 RAW_PULL_REQUEST_RESOLVED = (
-    r"(Pull Request resolved|Pull-Request-resolved): "
+    r"(Pull Request resolved|Pull-Request-resolved|Pull-Request): "
     r"https://{github_url}/(?P<owner>[^/]+)/(?P<repo>[^/]+)/pull/(?P<number>[0-9]+)"
 )
 
@@ -93,7 +93,7 @@ class Diff:
     # be the same.
     source_id: str
 
-    # The contents of 'Pull-Request-resolved'.  This is None for
+    # The contents of 'Pull-Request'.  This is None for
     # diffs that haven't been submitted by ghstack.  For BC reasons,
     # this also accepts gh-metadata.
     pull_request_resolved: Optional[PullRequestResolved]

src/ghstack/land.py

@@ -152,11 +152,18 @@ def main(
                 sh.git("cherry-pick", "--abort")
                 raise
 
+        # All good! Push!
+        maybe_force_arg = []
+        if needs_force:
+            maybe_force_arg = ["--force-with-lease"]
+        sh.git(
+            "push", *maybe_force_arg, remote_name, f"HEAD:refs/heads/{default_branch}"
+        )
+
         # Advance base to head to "close" the PR for all PRs.
-        # This has to happen before the push because the push
-        # will trigger a closure, but we want a *merge*.  This should
-        # happen after the cherry-pick, because the cherry-picks can
-        # fail
+        # This happens after the cherry-pick and push, because the cherry-picks
+        # can fail (merge conflict) and the push can also fail (race condition)
+
         # TODO: It might be helpful to advance orig to reflect the true
         # state of upstream at the time we are doing the land, and then
         # directly *merge* head into base, so that the PR accurately
@@ -173,14 +180,6 @@ def main(
             )
             github.notify_merged(pr_resolved)
 
-        # All good! Push!
-        maybe_force_arg = []
-        if needs_force:
-            maybe_force_arg = ["--force-with-lease"]
-        sh.git(
-            "push", *maybe_force_arg, remote_name, f"HEAD:refs/heads/{default_branch}"
-        )
-
         # Delete the branches
         for orig_ref, _ in stack_orig_refs:
             # TODO: regex here so janky

src/ghstack/submit.py

@@ -810,7 +810,7 @@ def elaborate_diff(
 convert a pre-existing PR in the old style to the new format which
 supports bidirectional syncing.  If you would like to blow away the old
 PR and start anew, edit the Summary in the Phabricator diff to delete
-the line 'Pull-Request-resolved' and then run ghexport again.
+the line 'Pull-Request' and then run ghexport again.
 """
             )
 
@@ -827,7 +827,7 @@ def elaborate_diff(
 to export your diff.
 
 If you think this is in error, edit the Summary in the Phabricator diff
-to delete the line 'Pull-Request-resolved' and then run ghexport again.
+to delete the line 'Pull-Request' and then run ghexport again.
 """
                 )
             else:
@@ -964,9 +964,7 @@ def process_commit(
             if self.direct:
                 trailers_to_add.append(f"ghstack-comment-id: {elab_diff.comment_id}")
 
-            trailers_to_add.append(
-                f"Pull-Request-resolved: {pull_request_resolved.url()}"
-            )
+            trailers_to_add.append(f"Pull-Request: {pull_request_resolved.url()}")
 
             commit_msg = ghstack.trailers.interpret_trailers(
                 strip_mentions(diff.summary.rstrip()), trailers_to_add
@@ -1773,7 +1771,7 @@ def _default_title_and_body(
         commit_body = RE_GHSTACK_SOURCE_ID.sub("", commit_body)
         # Comment ID is not necessary; source of truth is orig commit
         commit_body = RE_GHSTACK_COMMENT_ID.sub("", commit_body)
-        # Don't store Pull request resolved in the PR body; it's
+        # Don't store Pull request in the PR body; it's
         # unnecessary
         commit_body = ghstack.diff.re_pull_request_resolved_w_sp(self.github_url).sub(
             "", commit_body

test/land/default_branch_change.py.test

@@ -48,7 +48,7 @@ assert_expected_inline(
 assert_expected_inline(
     get_upstream_sh().git("log", "--oneline", "main"),
     """\
-19fa5fa Commit A
+8927014 Commit A
 dc8bfe4 Initial commit""",
 )
 
@@ -82,15 +82,15 @@ assert_github_state(
 
             This is commit B
 
-            * f5135dc Initial 2
+            * ab7d5ca Initial 2
 
         Repository state:
 
-            * f5135dc (gh/ezyang/2/head)
+            * ab7d5ca (gh/ezyang/2/head)
             |    Initial 2
-            * 61e5c2a (gh/ezyang/2/base)
+            * 742ae0b (gh/ezyang/2/base)
             |    Initial 2 (base update)
-            * 19fa5fa (main, gh/ezyang/1/orig)
+            * 8927014 (main, gh/ezyang/1/orig)
             |    Commit A
             | * 36fcfdf (gh/ezyang/1/head)
             | |    Initial 1
@@ -107,13 +107,13 @@ gh_land(diff2.pr_url)
 assert_expected_inline(
     get_upstream_sh().git("log", "--oneline", "master"),
     """\
-19e7996 Commit B
-cb4e674 Commit A
+d779d84 Commit B
+542f79d Commit A
 dc8bfe4 Initial commit""",
 )
 assert_expected_inline(
     get_upstream_sh().git("log", "--oneline", "main"),
     """\
-19fa5fa Commit A
+8927014 Commit A
 dc8bfe4 Initial commit""",
 )

test/land/ff.py.test

@@ -11,7 +11,7 @@ gh_land(pr_url)
 assert_expected_inline(
     get_upstream_sh().git("log", "--oneline", "master"),
     """\
-19fa5fa Commit A
+8927014 Commit A
 dc8bfe4 Initial commit""",
 )
 

test/land/ff_stack.py.test

@@ -16,7 +16,7 @@ gh_land(pr_url)
 assert_expected_inline(
     get_upstream_sh().git("log", "--oneline", "master"),
     """\
-3c9c5eb Commit B
-6eb4d4f Commit A
+b6c40ad Commit B
+851cf96 Commit A
 dc8bfe4 Initial commit""",
 )

test/land/ff_stack_two_phase.py.test

@@ -18,7 +18,7 @@ gh_land(pr_url2)
 assert_expected_inline(
     get_upstream_sh().git("log", "--oneline", "master"),
     """\
-3c9c5eb Commit B
-6eb4d4f Commit A
+b6c40ad Commit B
+851cf96 Commit A
 dc8bfe4 Initial commit""",
 )

test/land/invalid_resubmit.py.test

@@ -44,15 +44,15 @@ else:
 
             This is commit A
 
-            * c461f2f New PR
+            * 98fcb03 New PR
 
         Repository state:
 
-            * c461f2f (gh/ezyang/1/head)
+            * 98fcb03 (gh/ezyang/1/head)
             |    New PR
-            * 58e6f57 (gh/ezyang/1/base)
+            * 0d09e7d (gh/ezyang/1/base)
             |    New PR (base update)
-            * 19fa5fa (HEAD -> master)
+            * 8927014 (HEAD -> master)
             |    Commit A
             * dc8bfe4
                  Initial commit

test/land/non_ff.py.test

@@ -17,7 +17,7 @@ gh_land(pr_url)
 assert_expected_inline(
     get_upstream_sh().git("log", "--oneline", "master"),
     """\
-1d53e04 Commit A
+d43d06e Commit A
 38808c0 Commit U
 dc8bfe4 Initial commit""",
 )

test/land/non_ff_stack_two_phase.py.test

@@ -22,8 +22,8 @@ gh_land(pr_url2)
 assert_expected_inline(
     get_upstream_sh().git("log", "--oneline", "master"),
     """\
-613567d Commit B
-4dfc330 Commit A
+ec1d0de Commit B
+d8a6272 Commit A
 a8ca27f Commit C
 dc8bfe4 Initial commit""",
 )