Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

DOC: Update policy to prefer squash merging#28821

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
greglucas wants to merge1 commit intomatplotlib:main
base:main
Choose a base branch
Loading
fromgreglucas:doc-squash-merge

Conversation

greglucas
Copy link
Contributor

PR summary

On the weekly call, we discussed our squash merging policy. There was a general consensus to update our documentation to default/prefer squash merging commits. This can be on a case-by-case basis and if the PR author has good clean commit messages and history, we do not need to squash commits. This means that maintainers should clean up commit messages when squashing many commits down for the author to tidy up the message history.

This will help avoid contacting PR authors asking them to squash their commits down, going back and forth, and potentially losing new contributors from this sometimes confusing process. It can also help when accepting the GitHub inline code suggestions which create new commits where a maintainer can now accept suggestions and squash those out of the history.

ping @matplotlib/developers for awareness and more open discussion if anyone has strong preferences for/against this update.

PR checklist

anntzer and oscargus reacted with heart emoji
@greglucasgreglucas added this to thev3.10.0 milestoneSep 14, 2024
@github-actionsgithub-actionsbot added the Documentation: devdocsfiles in doc/devel labelSep 14, 2024
Comment on lines +235 to +237
when going through the history. If the commits are organized and contain
useful messages, the PR author can ask the person merging to not squash
the commits. This is done on a case-by-case basis and at the discretion
Copy link
Member

@rcomerrcomerSep 14, 2024
edited
Loading

Choose a reason for hiding this comment

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

If the commits are organised and contain useful messages, does the author need to actively state they prefer their commits aren't squashed? It seems implied by the fact that they kept their commits in order. I think people who prefer that will mostly be our maintainers - who are a minority of contributors but account for a majority of PRs.

story645 reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

I'm always curious about this argument. Does anyone really care if the commits are "organized"? What do people do with that organization?

Copy link
Member

@timhoffmtimhoffmSep 15, 2024
edited
Loading

Choose a reason for hiding this comment

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

I care when trying to understand why code is written the way it is or since when. Using git blame gives you the commit that a line was changed. It's very helpful then if all related change is in the same commit and all unrelated change is not.

Answering@rcomer's question: Generally, if multiple commits look reasonable (*), we can keep them, no matter if the author stated that explicitly. This may very well happen for experienced non-core devs. They know how to structure compex topics over multiple commits, but may not be aware of our policies.

(*) Reasonable is to a certain extend in the eye of the beholder. One can easily identify the extremes. OTOH, I would not squash commits from other people if they are very clear. I would squash the "small" PRs with many trival commits. If I the PR is too large / touching multiple topics, and I have the feeling it should be split up, I'd ask the author to reorganize - this is likely even necessary for simpler review.

rcomer, story645, oscargus, tacaswell, and scottshambaugh reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

Sure it's good to have reasonable commit histories. I'm just curious why folks would bother being overly meticulous with this. If a change is made in boo.py I'm not too likely to be confused by the blame if a change is also made in doc_boo.rst

But I'm fine if the merger wants to make a judgement call. I guess if in doubt ask. I think 99% of PRs a squash is fine and saves a step as noted in the original justification

Copy link
Member

Choose a reason for hiding this comment

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

Some people just prefer that their own PRs are not squash-merged. I personally do quite like the fact that I can do

git branch --merged upstream/main

and it will tell me which of my branches can be safely deleted. That won't work any more if the branches are squash-merged. That said, this is just a preference, not a hill I'm going to die on.

story645 and tacaswell reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

That won't work any more if the branches are squash-merged.

Eek, that's the only way things get cleaned for me.

I think, like most maintainers, I just do my own rebase squash/plan to get to it after the final round of reviews. Or usually I've done something like tweak css/.toml/conf.py b/c of the PR but unrelated enough that I'll keep it in a separate commit for history/bisecting.

Copy link
Member

Choose a reason for hiding this comment

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

The reason for changing to squash-merge was so that we don't require more inexperienced authors to squash themselves. Wecould have a case-dependent policy: Stick with simple merge, but squash-merge if the PR commit(s) are not well organized. I think one can usually tell the difference (or ask if not). The downside is that we put the burden on reviewers and they have to select the strategy case-by-case - GitHub UI keeps the last used strategy as default and you'd have to go through the dropdown.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

If the commits are organised and contain useful messages, does the author need to actively state they prefer their commits aren't squashed?

I think yes. I don't think it should be on the person merging to consider "oh this person wants X, this person wants Y".

The downside is that we put the burden on reviewers and they have to select the strategy case-by-case

I think that the bottom line of my thinking is that if someone chooses one method or the other, the author should not be upset about it. If someone wanted their commits kept, but they get squashed then 🤷 Make a small PR for each individual change instead with a single commit each which will guarantee those commits get kept separately.

Regarding removing local merged branches:

Rather than checking if a branch wasmerged, I check if a branch exists on my remote. So after someone merges, I click the "delete branch" button, which then deletes it from myorigin. I then have an alias setup locally that I run every so often to prune these branches that don't exist on myorigin.
https://stackoverflow.com/questions/13064613/how-to-prune-local-tracking-branches-that-do-not-exist-on-remote-anymore I think this works as expected with squash merges as well because it is comparing branch names, but even if not, I'm sure there are ways I can adapt my local workflow for this.

scottshambaugh reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

Keeping your local repo clean is an interesting use case of keeping commits the same. However after a merge I typically delete the branch on GitHub and then it's not on my "origin" remote. I treat my local repo as ephemeral.

@timhoffmtimhoffm removed this from thev3.10.0 milestoneOct 17, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@story645story645story645 left review comments

@jklymakjklymakjklymak left review comments

@timhoffmtimhoffmtimhoffm left review comments

@rcomerrcomerrcomer left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@greglucas@story645@jklymak@timhoffm@rcomer

[8]ページ先頭

©2009-2025 Movatter.jp