Skip to content
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

Squash/merge etc for accepting a pull request #1445

Open
nedbat opened this issue Oct 16, 2024 · 4 comments
Open

Squash/merge etc for accepting a pull request #1445

nedbat opened this issue Oct 16, 2024 · 4 comments
Labels

Comments

@nedbat
Copy link
Member

nedbat commented Oct 16, 2024

Describe the bug

Maybe I missed it somewhere, but I couldn't find a clear description of how to accept a pull request. Squash instead of merge, right? And when squashing, don't leave all the individual commit messages in the message (though, why not?)

@nedbat nedbat added the bug label Oct 16, 2024
@skirpichev
Copy link
Member

Not sure if this is a good idea in general (some core devs seems using it, some apparently - not), but I'm trying to sync in my PR's description with actual content (following review process, etc). In this way, it could serve as a draft for the commit message.

@erlend-aasland
Copy link
Contributor

I think squash is the only option enabled. Regarding commit messages, I was under the impression that we already had some rudimentary instructions on how to word them. If we don't, we definitely should add some!

And when squashing, don't leave all the individual commit messages in the message (though, why not?)

A branch with lots of small commits (often work-in-progress amendments) would pollute the resulting commit message intensely, and end up as an extremely verbose commit message that could consume a screenful or two.

Commit messages should (IMO) be counted as documentation; a succinct description of the intent and effects of the change should be a minimum requirement

@hugovk
Copy link
Member

hugovk commented Oct 16, 2024

Yes, squash merge, it's the only option enabled:

image

I think removing the individual commit messages is a good idea, because usually there's not been much if any editorial care put into them. Partly because we squash anyway, we might have fixup commits, or "Apply changes from review", or other almost throwaway commit titles later in review.

If someone has written a useful description in their commit, we can leave this in place. This is rare though, and I nearly always look up commit -> PR (and sometimes -> issue) when I want more info and context. (And I use the Refined GitHub extension on desktop which auto-clears commit descriptions.)

@nedbat
Copy link
Member Author

nedbat commented Oct 16, 2024

BTW, in this repo (devguide), "Rebase and merge" is enabled along with "Squash and merge."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants