Add PR number to commit messages during land (#314)

Author: vmoens

src/ghstack/land.py

@@ -145,13 +145,41 @@ def main(
             stack_orig_refs.append((ref, pr_resolved))
 
         # OK, actually do the land now
-        for orig_ref, _ in stack_orig_refs:
+        for orig_ref, pr_resolved in stack_orig_refs:
             try:
                 sh.git("cherry-pick", f"{remote_name}/{orig_ref}")
             except BaseException:
                 sh.git("cherry-pick", "--abort")
                 raise
 
+            # Add PR number to commit message like GitHub does
+            commit_msg = sh.git("log", "-1", "--pretty=%B")
+            # Get the original author and committer dates to preserve the commit hash
+            author_date = sh.git("log", "-1", "--pretty=%aD")
+            committer_date = sh.git("log", "-1", "--pretty=%cD")
+            lines = commit_msg.split("\n")
+            if lines:
+                # Add PR number to the subject line (first line)
+                subject = lines[0].rstrip()
+                # Only add if not already present
+                pr_tag = f"(#{pr_resolved.number})"
+                if pr_tag not in subject:
+                    subject = f"{subject} {pr_tag}"
+                lines[0] = subject
+                new_msg = "\n".join(lines)
+                # Preserve dates to keep the commit hash consistent
+                sh.git(
+                    "commit",
+                    "--amend",
+                    "-F",
+                    "-",
+                    input=new_msg,
+                    env={
+                        "GIT_AUTHOR_DATE": author_date,
+                        "GIT_COMMITTER_DATE": committer_date,
+                    },
+                )
+
         # All good! Push!
         maybe_force_arg = []
         if needs_force:

src/ghstack/submit.py

@@ -487,18 +487,20 @@ def run(self) -> List[DiffMeta]:
                 d = ghstack.git.convert_header(h, self.github_url)
                 if d.pull_request_resolved is not None:
                     ed = self.elaborate_diff(d)
-                    pre_branch_state_index[h.commit_id] = PreBranchState(
-                        head_commit_id=GitCommitHash(
-                            self.sh.git(
-                                "rev-parse", f"{self.remote_name}/{ed.head_ref}"
-                            )
-                        ),
-                        base_commit_id=GitCommitHash(
-                            self.sh.git(
-                                "rev-parse", f"{self.remote_name}/{ed.base_ref}"
-                            )
-                        ),
-                    )
+                    # Skip closed PRs (e.g., after landing) where branches have been deleted
+                    if not ed.closed:
+                        pre_branch_state_index[h.commit_id] = PreBranchState(
+                            head_commit_id=GitCommitHash(
+                                self.sh.git(
+                                    "rev-parse", f"{self.remote_name}/{ed.head_ref}"
+                                )
+                            ),
+                            base_commit_id=GitCommitHash(
+                                self.sh.git(
+                                    "rev-parse", f"{self.remote_name}/{ed.base_ref}"
+                                )
+                            ),
+                        )
 
         # NB: deduplicates
         commit_index = {
@@ -870,21 +872,24 @@ def elaborate_diff(
                 "--header",
                 self.remote_name + "/" + branch_orig(username, gh_number),
             )
-        except RuntimeError as e:
+        except RuntimeError:
             if r["closed"]:
-                raise RuntimeError(
-                    f"Cannot ghstack a stack with closed PR #{number} whose branch was deleted.  "
-                    "If you were just trying to update a later PR in the stack, `git rebase` and try again.  "
-                    "Otherwise, you may have been trying to update a PR that was already closed. "
-                    "To disassociate your update from the old PR and open a new PR, "
-                    "run `ghstack unlink`, `git rebase` and then try again."
-                ) from e
-            raise
-        remote_summary = ghstack.git.split_header(rev_list)[0]
-        m_remote_source_id = RE_GHSTACK_SOURCE_ID.search(remote_summary.commit_msg)
-        remote_source_id = m_remote_source_id.group(1) if m_remote_source_id else None
-        m_comment_id = RE_GHSTACK_COMMENT_ID.search(remote_summary.commit_msg)
-        comment_id = int(m_comment_id.group(1)) if m_comment_id else None
+                # If the PR is closed and the branch is deleted (e.g., after landing),
+                # we can't get the remote source ID. Return None for it, which will
+                # signal to process_commit that this commit has been landed and should
+                # be skipped (not updated).
+                remote_source_id = None
+                comment_id = None
+            else:
+                raise
+        else:
+            remote_summary = ghstack.git.split_header(rev_list)[0]
+            m_remote_source_id = RE_GHSTACK_SOURCE_ID.search(remote_summary.commit_msg)
+            remote_source_id = (
+                m_remote_source_id.group(1) if m_remote_source_id else None
+            )
+            m_comment_id = RE_GHSTACK_COMMENT_ID.search(remote_summary.commit_msg)
+            comment_id = int(m_comment_id.group(1)) if m_comment_id else None
 
         return DiffWithGitHubMetadata(
             diff=diff,
@@ -917,6 +922,48 @@ def process_commit(
         if elab_diff is not None and elab_diff.closed:
             if self.direct:
                 self._raise_needs_rebase()
+            # If we're trying to submit a closed commit, check if it has been modified
+            if elab_diff.remote_source_id is None:
+                # The branch was deleted (e.g., after landing). Check if the commit has been
+                # modified by comparing source_ids. If the commit is reachable from master with
+                # the same source_id (tree hash), it means it was landed and we should skip it.
+                # Otherwise, it's been modified and we should raise an error.
+                try:
+                    # Check if there's a commit on master with the same tree (source_id)
+                    master_commits = self.sh.git(
+                        "log",
+                        "--format=%H %T",
+                        f"{self.remote_name}/{self.base}",
+                        "-n",
+                        "100",  # Check last 100 commits
+                    )
+                    for line in master_commits.split("\n"):
+                        if not line.strip():
+                            continue
+                        commit_hash, tree_hash = line.split()
+                        if tree_hash == diff.source_id:
+                            # Found a commit on master with the same tree, so this commit
+                            # was landed (just with a different commit message/hash)
+                            return None
+                except Exception:
+                    pass
+                # Didn't find a matching commit on master, so this is a modified closed commit
+                raise RuntimeError(
+                    f"Cannot ghstack a stack with closed PR #{elab_diff.number} whose branch was deleted.  "
+                    "If you were just trying to update a later PR in the stack, `git rebase` and try again.  "
+                    "Otherwise, you may have been trying to update a PR that was already closed. "
+                    "To disassociate your update from the old PR and open a new PR, "
+                    "run `ghstack unlink`, `git rebase` and then try again."
+                )
+            elif diff.source_id != elab_diff.remote_source_id:
+                # The commit has been modified locally
+                raise RuntimeError(
+                    f"Cannot ghstack a stack with closed PR #{elab_diff.number} whose branch was deleted.  "
+                    "If you were just trying to update a later PR in the stack, `git rebase` and try again.  "
+                    "Otherwise, you may have been trying to update a PR that was already closed. "
+                    "To disassociate your update from the old PR and open a new PR, "
+                    "run `ghstack unlink`, `git rebase` and then try again."
+                )
             return None
 
         # Edge case: check if the commit is empty; if so skip submitting

test/land/default_branch_change.py.test

@@ -107,8 +107,8 @@ gh_land(diff2.pr_url)
 assert_expected_inline(
     get_upstream_sh().git("log", "--oneline", "master"),
     """\
-d779d84 Commit B
-542f79d Commit A
+6b7e56e Commit B (#501)
+1132c50 Commit A (#500)
 dc8bfe4 Initial commit""",
 )
 assert_expected_inline(

test/land/ff.py.test

@@ -11,7 +11,7 @@ gh_land(pr_url)
 assert_expected_inline(
     get_upstream_sh().git("log", "--oneline", "master"),
     """\
-8927014 Commit A
+d518c9f Commit A (#500)
 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"),
     """\
-b6c40ad Commit B
-851cf96 Commit A
+4099517 Commit B (#501)
+c28edd5 Commit A (#500)
 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"),
     """\
-b6c40ad Commit B
-851cf96 Commit A
+4099517 Commit B (#501)
+c28edd5 Commit A (#500)
 dc8bfe4 Initial commit""",
 )

test/land/invalid_resubmit.py.test

@@ -44,16 +44,16 @@ else:
 
             This is commit A
 
-            * 98fcb03 New PR
+            * d1c3c7e New PR
 
         Repository state:
 
-            * 98fcb03 (gh/ezyang/1/head)
+            * d1c3c7e (gh/ezyang/1/head)
             |    New PR
-            * 0d09e7d (gh/ezyang/1/base)
+            * 5f392f5 (gh/ezyang/1/base)
             |    New PR (base update)
-            * 8927014 (HEAD -> master)
-            |    Commit A
+            * d518c9f (HEAD -> master)
+            |    Commit A (#500)
             * 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"),
     """\
-d43d06e Commit A
+8b61aeb Commit A (#500)
 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"),
     """\
-ec1d0de Commit B
-d8a6272 Commit A
+402e96c Commit B (#501)
+e388a10 Commit A (#500)
 a8ca27f Commit C
 dc8bfe4 Initial commit""",
 )

test/land/update_after_land.py.test

@@ -55,17 +55,17 @@ else:
 
             This is commit B
 
-            * c6c8f43 Run 3
+            * 87c9ccd Run 3
             * 16e1e12 Initial 1
 
         Repository state:
 
-            *   c6c8f43 (gh/ezyang/2/head)
+            *   87c9ccd (gh/ezyang/2/head)
             |\\     Run 3
-            | *   36376f9 (gh/ezyang/2/base)
+            | *   a800ca6 (gh/ezyang/2/base)
             | |\\     Run 3 (base update)
-            | | * 9bf93f4 (HEAD -> master)
-            | | |    Commit A
+            | | * 70eb094 (HEAD -> master)
+            | | |    Commit A (#500)
             | | * 7f0288c
             | | |    Commit U
             * | | 16e1e12