-
Notifications
You must be signed in to change notification settings - Fork 79
Add PR number to commit messages during land #314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
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_landfails: Can't find deleted branch, crashesinvalid_resubmitfails: Doesn't detect modified closed PRs, allows invalid resubmitsupdate_after_landfails: Same issue
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gives me the jeebies
ezyang
left a comment
There was a problem hiding this 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).
yeah I actually thought about that just yesterday, no need to wait that long to do that! I'll work on this asap |
This PR modifies
ghstack landto append the PR number to commit messages, matching GitHub's native merge behavior.Changes
land.py:(#<PR_NUMBER>)to the subject linesubmit.py:check_invariantsvalidationExample
Before:
After landing PR #456:
Testing
All 10 existing land tests pass. Tests were updated to expect PR numbers in commit messages using
EXPECTTEST_ACCEPT=1.Closes #313