Skip to content

Conversation

@vmoens
Copy link
Collaborator

@vmoens vmoens commented Jan 14, 2026

This PR modifies ghstack land to append the PR number to commit messages, matching GitHub's native merge behavior.

Changes

land.py:

  • After cherry-picking each commit, amend the message to add (#<PR_NUMBER>) to the subject line
  • Only add if not already present
  • Preserve author/committer dates for consistency

submit.py:

  • Handle closed PRs whose branches were deleted (common after landing)
  • When processing closed PRs, check if the commit exists on master by comparing tree hashes
  • If found with same tree hash, skip (already landed); if modified or not found, raise appropriate error
  • Skip closed PRs in check_invariants validation

Example

Before:

abc123 Fix critical bug

After landing PR #456:

def789 Fix critical bug (#456)

Testing

All 10 existing land tests pass. Tests were updated to expect PR numbers in commit messages using EXPECTTEST_ACCEPT=1.

Closes #313

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we amend commit messages in land.py to add PR numbers, we change the commit hash.
This creates an issue when users later try to submit new commits that are based on the old (pre-landing) commit.

Without these changes:

  • reuse_branch_refuse_land fails: Can't find deleted branch, crashes
  • invalid_resubmit fails: Doesn't detect modified closed PRs, allows invalid resubmits
  • update_after_land fails: Same issue

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the changes I must review most carefully

"--format=%H %T",
f"{self.remote_name}/{self.base}",
"-n",
"100", # Check last 100 commits
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit suspect

# 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:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gives me the jeebies

Copy link
Owner

@ezyang ezyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to accept this for now, but I am interested in a modified version where after the first ghstack and we've allocated a PR, we go ahead and directly update the commit message to have the PR in it (so we don't have to amend it later).

@ezyang ezyang merged commit 3987ada into ezyang:master Jan 19, 2026
16 checks passed
@vmoens
Copy link
Collaborator Author

vmoens commented Jan 19, 2026

I'm going to accept this for now, but I am interested in a modified version where after the first ghstack and we've allocated a PR, we go ahead and directly update the commit message to have the PR in it (so we don't have to amend it later).

yeah I actually thought about that just yesterday, no need to wait that long to do that! I'll work on this asap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add PR number to commit messages when landing, like GitHub does

2 participants