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

Latest force pushes #228

Open
S-S-X opened this issue Oct 24, 2021 · 4 comments
Open

Latest force pushes #228

S-S-X opened this issue Oct 24, 2021 · 4 comments

Comments

@S-S-X
Copy link
Member

S-S-X commented Oct 24, 2021

Lately there's been a lot of force pushing to master branch, this is usually harmful for projects.

As branch protection rules are joke I've just removed those for now, if administrators disagree with rules then I think that's fine but requires some discussion.

What I do disagree with is force pushing over commits made by anyone else without asking first, so to express this I've now rewritten history of my latest commits that were rewritten without first telling about possible problems.

I do also think that protection rules would be good and would improve working and testing, for this we do need some solution everyone can agree with.
If it is to allow rewriting anyone's commits and remove branch protections then fine for me but in that case I just want to hear that everyone else is okay with that.

This issue would actually belong to community meta board but mt-mods does not have such thing yet.

@OgelGames
Copy link
Contributor

OgelGames commented Oct 24, 2021

Yeah, that's my bad... The thing that caused it was the bad (rushed?) commit messages of the recent PRs, thinking of it now I probably should have just asked you to fix them...

Maybe we should enable the PR approval option for the branch protection? I myself always like to have another pair of eyes look at my changes before commiting, so we could enforce that to avoid bad commits.

@S-S-X
Copy link
Member Author

S-S-X commented Nov 4, 2021

PR approval seems to be good, but looking at history I guess it can be just changed any time.

But why merge commits to master are disabled and rebase enabled?
Rebase on master / public branches breaks history of feature branches, in some cases it is usually fine but for large changes it makes it harder especially for outside contributors (see digistuff if you need explanation).

In other words: rebase on master keeps master extremely clean but enforcing linear history makes contributions way more complicated when there's multiple branches and especially if there's conflicts.

@OgelGames
Copy link
Contributor

OgelGames commented Nov 4, 2021

Squashing is the preferred method, I only left rebase enabled for situations like #224, where it was better to merge as multiple commits.

The main goal is to keep the commit history clean and understandable, and avoid extra merge commits.

PR approval seems to be good, but looking at history I guess it can be just changed any time.

Well yes, but we can (and should) still try to stick to it as much as possible.

@S-S-X
Copy link
Member Author

S-S-X commented Nov 4, 2021

Squash is the preferred method, I only left rebase enabled for situations like #224, where it was better to merge as multiple commits, but as that shouldn't happen often, it's probably fine to disable rebase too.

True that squash is fine for most especially small changes, I am however including tests as much as possible and tests should be separated from actual code in almost all cases.
Tests however obviously should still be part of same branch and pull request.

I can still agree using rebase over merges as probably rebase vs merge wont be big thing currently, there's not that much outside contributions currently. It might be enough to look for possible forks and be bit more careful when it seems that someone is working outside of this repo in their own branches. That said it is good to remember that rebasing outside of private branches makes contributions harder and deters outside contributors.

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

No branches or pull requests

2 participants