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

Using squash commits in the master #1110

Closed
mathbunnyru opened this issue Jun 14, 2020 · 8 comments
Closed

Using squash commits in the master #1110

mathbunnyru opened this issue Jun 14, 2020 · 8 comments
Labels
type:Maintenance A proposed enhancement to how we maintain this project

Comments

@mathbunnyru
Copy link
Member

I propose to start using squash commits in the master for PRs.
It will make it much easier to read commit history.

I think GitHub handles it quite well, so maybe it's worth giving it a try.

All the details are located here:
https://cloudfour.com/thinks/squashing-your-pull-requests/

I attached tig from the master, which is not easy to read at all.

Screenshot 2020-06-14 at 19 07 29

@romainx
Copy link
Collaborator

romainx commented Aug 16, 2020

Hello, sorry I did not react to that since I have no opinion nor feedback on how it's done on other similar project.
However it's interesting, @parente what is your opinion?

@parente
Copy link
Member

parente commented Nov 29, 2020

I'm OK squashing PRs that have a lot of iteration happening in them (e.g., fix, fix, fix, typo) and as long as the original contributor name is preserved as the author in the git history (i.e., we want to recognize people for their contributions).

@parente parente added the type:Maintenance A proposed enhancement to how we maintain this project label Nov 29, 2020
@mathbunnyru
Copy link
Member Author

mathbunnyru commented Nov 29, 2020

I see, there might be some problems with that.
I need to investigate this question.

isaacs/github#1368

@mathbunnyru
Copy link
Member Author

Sometimes I do squash merging, but I'm actually fine wirth merges, so let's keep the way it is right now.

@mathbunnyru
Copy link
Member Author

I started using squash merges for everything - the history looks much cleaner.

@consideRatio
Copy link
Collaborator

It reads clearer on the high level overview, but when you have a PR that does something somewhat advanced and broken apart in multiple commits, that kind of lower lever overview is lost. I find such lower level overview is relevant in bigger PRs only. Due to that, I think its good to avoid a policy to always do it.

I'm not so happy about a squash also makes the person merging the commit author, and destroys possible git commit's signatures etc.

Overall, I lean towards preferring not squashing. I do rebase my PRs as often I can before they are merged to help keep the commit history cleaner.

@mathbunnyru
Copy link
Member Author

Thank you for your feedback @consideRatio.

I do rebase my PRs as often I can before they are merged to help keep the commit history cleaner.

This doesn't work for me, because it's not easy to review PRs that do lots of rebases and you lose the history of your review and need to watch all the files again.

@mathbunnyru
Copy link
Member Author

I'm not so happy about a squash also makes the person merging the commit author, and destroys possible git commit's signatures etc.

@consideRatio I've just checked this and if you create a squash-merge from the GitHub website (and I always use this feature), then the author is correct.
You can see it here:

Moreover, co-author is added if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Maintenance A proposed enhancement to how we maintain this project
Projects
None yet
Development

No branches or pull requests

4 participants