PR unexpectedly merged when commits are reordered

[uycire]

I had a stack of 4 PRs, like this:

1. PR A
2. PR B
3. PR C
4. PR D

PR A was taking a while to review, so I rebased my stack to put it at the end:

1. PR B
2. PR C
3. PR D
4. PR A

But for some reason, after I ran ghstack, PR A got merged (!)

At first I thought I misclicked, but this is the second time it's happened to me.

Log

$ git remote get-url origin
$ git fetch origin
$ git merge-base origin/dev HEAD
$ git rev-list --header ^2f730d04bea150a5ac4cdd00a9903b7206d1a523 HEAD
$ git rev-list --header ^2f730d04bea150a5ac4cdd00a9903b7206d1a523^@ 2f730d04bea150a5ac4cdd00a9903b7206d1a523
$ git rev-list --max-count=1 --header origin/gh/uycire/23/orig
Pushing to #15173
$ git rev-parse origin/gh/uycire/23/base
$ git merge-base --is-ancestor origin/gh/uycire/23/base 2f730d04bea150a5ac4cdd00a9903b7206d1a523
$ git commit-tree f3870cc051da430fbb658597890e5f5af73d2aee -p origin/gh/uycire/23/base -p 2f730d04bea150a5ac4cdd00a9903b7206d1a523
$ git rev-parse c1614293dbd668e61ac7bb15bc34ded9187d1f94~^{tree}
$ git commit-tree dbfadf306f49558ed6eb704ec9bdd285a5971d55 -p origin/gh/uycire/23/head -p 58aba48019cbf29bce99550d02324eefe6a6be1d
Restacking commit on 2f730d04bea150a5ac4cdd00a9903b7206d1a523
$ git commit-tree dbfadf306f49558ed6eb704ec9bdd285a5971d55 -p 2f730d04bea150a5ac4cdd00a9903b7206d1a523
$ git rev-list --max-count=1 --header origin/gh/uycire/24/orig
Pushing to #15174
$ git rev-parse origin/gh/uycire/24/base
$ git merge-base --is-ancestor origin/gh/uycire/24/base 1179b46a16042a70e8ebf841278e395acf5ef622
$ git rev-parse b6e9506a067ba95070a4fd373299c959c68cdf14~^{tree}
$ git commit-tree fcd7930234022d4f51d98b5e2ae2df96d9e0786f -p origin/gh/uycire/24/head -p 1179b46a16042a70e8ebf841278e395acf5ef622
Restacking commit on ee28d0e1ab0ae29c708224a48bde2ed40d639684
$ git commit-tree fcd7930234022d4f51d98b5e2ae2df96d9e0786f -p ee28d0e1ab0ae29c708224a48bde2ed40d639684
$ git rev-list --max-count=1 --header origin/gh/uycire/34/orig
Pushing to #15214
$ git rev-parse origin/gh/uycire/34/base
$ git merge-base --is-ancestor origin/gh/uycire/34/base bed81715a6983c25786741de018a111e36a5e901
$ git rev-parse e0858fea3333550cd06a601ce42ace508d25daa2~^{tree}
$ git commit-tree dafe580de6588db31ac4b393531ad5f4a4b6550b -p origin/gh/uycire/34/head -p bed81715a6983c25786741de018a111e36a5e901
Restacking commit on 462d714e2b4e358db54715c1101b9d466ae19f5d
$ git commit-tree dafe580de6588db31ac4b393531ad5f4a4b6550b -p 462d714e2b4e358db54715c1101b9d466ae19f5d
$ git rev-list --max-count=1 --header origin/gh/uycire/22/orig
Pushing to #15167
$ git rev-parse origin/gh/uycire/22/base
$ git merge-base --is-ancestor origin/gh/uycire/22/base 93ea4d8d70e5310ded9411bab98213ed798377a4
$ git rev-parse e0f74da1612150f476eb54d9cc0417ab14dfd421~^{tree}
$ git commit-tree 2bab0f9f6aebf106b6ffa73f847f447e38bb8220 -p origin/gh/uycire/22/head -p 93ea4d8d70e5310ded9411bab98213ed798377a4
Restacking commit on cb0471ac2de373b973f13becd25affcbf0d6eb03
$ git commit-tree 2bab0f9f6aebf106b6ffa73f847f447e38bb8220 -p cb0471ac2de373b973f13becd25affcbf0d6eb03
$ git reset --soft 3ee46301fe403da8c7258003fcd18ae61d8c8155
# Updating https://github.com/benchling/aurelia/pull/15173
# Updating https://github.com/benchling/aurelia/pull/15174
# Updating https://github.com/benchling/aurelia/pull/15214
# Updating https://github.com/benchling/aurelia/pull/15167
$ git push origin 58aba48019cbf29bce99550d02324eefe6a6be1d:refs/heads/gh/uycire/23/base 1179b46a16042a70e8ebf841278e395acf5ef622:refs/heads/gh/uycir
e/24/base bed81715a6983c25786741de018a111e36a5e901:refs/heads/gh/uycire/34/base 93ea4d8d70e5310ded9411bab98213ed798377a4:refs/heads/gh/uycire/22/bas
e
To github.com:benchling/aurelia.git
   6482da57fb..93ea4d8d70  93ea4d8d70e5310ded9411bab98213ed798377a4 -> gh/uycire/22/base
   f2a04320da..58aba48019  58aba48019cbf29bce99550d02324eefe6a6be1d -> gh/uycire/23/base
   616f8d5ef0..1179b46a16  1179b46a16042a70e8ebf841278e395acf5ef622 -> gh/uycire/24/base
   ddcc776f27..bed81715a6  bed81715a6983c25786741de018a111e36a5e901 -> gh/uycire/34/base
$ git push origin 1179b46a16042a70e8ebf841278e395acf5ef622:refs/heads/gh/uycire/23/head bed81715a6983c25786741de018a111e36a5e901:refs/heads/gh/uycir
e/24/head 93ea4d8d70e5310ded9411bab98213ed798377a4:refs/heads/gh/uycire/34/head eff85617349d7019a1755f2dbcc38ada0a410f72:refs/heads/gh/uycire/22/hea
d
To github.com:benchling/aurelia.git
 ! [remote rejected]       eff85617349d7019a1755f2dbcc38ada0a410f72 -> gh/uycire/22/head (cannot lock ref 'refs/heads/gh/uycire/22/head': unable to
resolve reference 'refs/heads/gh/uycire/22/head')
 ! [remote rejected]       1179b46a16042a70e8ebf841278e395acf5ef622 -> gh/uycire/23/head (failed)
 ! [remote rejected]       bed81715a6983c25786741de018a111e36a5e901 -> gh/uycire/24/head (failed)
 ! [remote rejected]       93ea4d8d70e5310ded9411bab98213ed798377a4 -> gh/uycire/34/head (failed)
error: failed to push some refs to 'git@github.com:benchling/aurelia.git'
ERROR: Fatal exception
Traceback (most recent call last):
  File "/home/eric/.pyenv/versions/3.8.1/lib/python3.8/site-packages/ghstack/logging.py", line 108, in manager
    yield
  File "/home/eric/.pyenv/versions/3.8.1/lib/python3.8/site-packages/ghstack/__main__.py", line 107, in main
    ghstack.submit.main(
  File "/home/eric/.pyenv/versions/3.8.1/lib/python3.8/site-packages/ghstack/submit.py", line 213, in main
    submitter.push_updates()
  File "/home/eric/.pyenv/versions/3.8.1/lib/python3.8/site-packages/ghstack/submit.py", line 985, in push_updates
    self.sh.git("push", self.remote_name, *push_branches)
  File "/home/eric/.pyenv/versions/3.8.1/lib/python3.8/site-packages/ghstack/shell.py", line 272, in git
    return self._maybe_rstrip(self.sh(*(("git",) + args), **kwargs))
  File "/home/eric/.pyenv/versions/3.8.1/lib/python3.8/site-packages/ghstack/shell.py", line 211, in sh
    raise RuntimeError(
RuntimeError: git push origin 1179b46a16042a70e8ebf841278e395acf5ef622:refs/heads/gh/uycire/23/head bed81715a6983c25786741de018a111e36a5e901:refs/h$ads/gh/uycire/24/head 93ea4d8d70e5310ded9411bab98213ed798377a4:refs/heads/gh/uycire/34/head eff85617349d7019a1755f2dbcc38ada0a410f72:refs/heads/gh/$ycire/22/head failed with exit code 1

[ezyang]

Yes, unfortunately, this is a known bug. The way we should fix it is to get ghstack to stop setting up merge parents between the various PRs it creates (so there is no chance of accidentally closing in this way), but I haven't had time to do it.

[uycire]

@ezyang do you have an idea of how involved the fix will be? If it's reasonably simple I'd be happy to take it on!

[ezyang]

It's a little hard to say. Essentially, the problem is that self.base_commit in ghstack/submit.py is being used as a parent -p in the commit-tree command. It might be sufficient to just remove that, but you'll have to check with the tests to see if this actually worked or not.

[ezyang]

This diff might be a start but it needs more careful testing

diff --git a/ghstack/submit.py b/ghstack/submit.py
index 94f67e6..f6264b5 100644
--- a/ghstack/submit.py
+++ b/ghstack/submit.py
@@ -781,38 +781,19 @@ Since we cannot proceed, ghstack will abort now.
             base_args = ()
 
         else:
-            # Second, check if our local base (self.base_commit)
-            # added some new commits, but is still rooted on the
-            # old base.
-            #
-            # If so, all we need to do is include the local base
-            # as a parent when we do the merge.
-            is_ancestor = self.sh.git(
-                "merge-base",
-                "--is-ancestor",
+            # Make a fake commit that
+            # "resets" the tree back to something that makes
+            # sense and merge with that.  This doesn't fix
+            # the fact that we still incorrectly report
+            # the old base as an ancestor of our commit, but
+            # it's better than nothing.
+            new_base = GitCommitHash(self.sh.git(
+                "commit-tree", self.base_tree,
+                "-p",
                 self.remote_name + "/" + branch_base(self.username, ghnum),
-                self.base_commit, exitcode=True)
-
-            if is_ancestor:
-                new_base = self.base_commit
-
-            else:
-                # If we've gotten here, it means that the new
-                # base and the old base are completely
-                # unrelated.  We'll make a fake commit that
-                # "resets" the tree back to something that makes
-                # sense and merge with that.  This doesn't fix
-                # the fact that we still incorrectly report
-                # the old base as an ancestor of our commit, but
-                # it's better than nothing.
-                new_base = GitCommitHash(self.sh.git(
-                    "commit-tree", self.base_tree,
-                    "-p",
-                    self.remote_name + "/" + branch_base(self.username, ghnum),
-                    "-p", self.base_commit,
-                    input='Update base for {} on "{}"\n\n{}\n\n[ghstack-poisoned]'
-                          .format(self.msg, elab_commit.title,
-                                  non_orig_commit_msg)))
+                input='Update base for {} on "{}"\n\n{}\n\n[ghstack-poisoned]'
+                      .format(self.msg, elab_commit.title,
+                              non_orig_commit_msg)))
 
             base_args = ("-p", new_base)