diff --git a/src/ghstack/land.py b/src/ghstack/land.py index 4a96d60..08adfb6 100644 --- a/src/ghstack/land.py +++ b/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: diff --git a/src/ghstack/submit.py b/src/ghstack/submit.py index b9568b9..dffdf1d 100644 --- a/src/ghstack/submit.py +++ b/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 diff --git a/test/land/default_branch_change.py.test b/test/land/default_branch_change.py.test index 7041dc4..7160e23 100644 --- a/test/land/default_branch_change.py.test +++ b/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( diff --git a/test/land/ff.py.test b/test/land/ff.py.test index c2a1a3d..6e30a9d 100644 --- a/test/land/ff.py.test +++ b/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""", ) diff --git a/test/land/ff_stack.py.test b/test/land/ff_stack.py.test index 55c9ece..5460c9f 100644 --- a/test/land/ff_stack.py.test +++ b/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""", ) diff --git a/test/land/ff_stack_two_phase.py.test b/test/land/ff_stack_two_phase.py.test index 85f6212..d73dff2 100644 --- a/test/land/ff_stack_two_phase.py.test +++ b/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""", ) diff --git a/test/land/invalid_resubmit.py.test b/test/land/invalid_resubmit.py.test index d50f9e8..1ee1657 100644 --- a/test/land/invalid_resubmit.py.test +++ b/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 """ diff --git a/test/land/non_ff.py.test b/test/land/non_ff.py.test index 7ae7ebf..7c9aab9 100644 --- a/test/land/non_ff.py.test +++ b/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""", ) diff --git a/test/land/non_ff_stack_two_phase.py.test b/test/land/non_ff_stack_two_phase.py.test index 751b1fd..ecc45a1 100644 --- a/test/land/non_ff_stack_two_phase.py.test +++ b/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""", ) diff --git a/test/land/update_after_land.py.test b/test/land/update_after_land.py.test index 412304f..88fa5c5 100644 --- a/test/land/update_after_land.py.test +++ b/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