Personally, I have pushed directly to “master” many times. The first time was by accident; the rest were intentional. My usual rationale is that the changes are either very minor, or very urgent, or reverting a mistake I made, and furthermore they are unlikely to get reviewed or to benefit from review.
But sometimes I am surprised by the results of my direct pushes.
Also, there are reasons other than code review for creating PRs. PRs provide a mechanism for grouping related commits together. Unlike bare commits, they can be categorized and linked to issues. They show up in reports generated by GitHub. They provide continuous testing to prevent breaking the build. They can be referenced using a 4-digit decimal number that’s easier to type and remember than a hexadecimal commit hash.
To be honest, for the sorts of commits I’m tempted to push directly, creating a PR really isn’t much extra work. So I wouldn’t object to changing current practice.
### Core Contributors
Developers in the Contributors team can push directly to Main instead of submitting pull requests, however for new features it is often a good idea to do a pull request as a means to get a last code review.
If we’re ready to prohibit direct commits to the “master” branch, the first step would be an open discussion (in a new topic) at this Forum.
There is a question worth considering: for example, for a very urgent bug fix, after submitting a PR, if no one reviews and merges it into the core for a long time, then for those who use the master branch for development, the bug cannot be fixed immediately (it must wait until the PR is reviewed and merged). Therefore, unless the reviewers are very active, for some extremely urgent bugs, I personally suggest committing directly to master. For our online games, this is what we do, because by the time the code review process finishes, players may have already uninstalled our game out of frustration. So we usually commit directly to the online branch (release-branch) without review for urgent bugs and very minor issues. (This approach is suitable for urgent bugs and very small functionality problems.)
And then the unreviewed bug fix causes 12 other bugs because no one who knew better actually looked…
For every story like this, there are a dozen where it failed catastrophically.
Think of every rushed game patch for a AAA game that caused more problems than it solved and the above problem is at its root.
This is similar to someone whose car is almost out of gas but they need to go to the store so badly that they don’t have time to go to the gas station… but then apparently have plenty of time to walk to the gas station with a gas can when they don’t make it.
A patch takes as long as it takes. Cutting corners has consequences.
I think the situation here is different. For us, code review is a process that even a one-line rename fix has to go through a complete process (what I mean is that even just renaming bUse->bUseName, initiating a review requires at least a 1 hour full automated testing process). However, some errors are obviously apparent and don’t need this complex review process, because users are very anxious, rules shouldn’t be rigid, there should be flexibility in certain situations. This is like a patient who is about to physically collapse, so there is emergency room that skips the regular queueing process.
Complex = at least one other person looking at it?
We’re talking about not blindly committing to master. We’re not talking about a 12 step complicated review process. A patch can wait an hour or two. We’re not in that much of a hurry unless we already screwed up by not reviewing well enough… and why compound the problem by doing it again?
In general, I agree with you on allowing exceptions but those exceptions should be so exceedingly rare that we don’t even write them into the process. The process says ‘never’ and humans maintain the process and may decide on a very specific occasion to skip some steps and go against the “always used” process.
I basically agree with your point of view, what I mean is: “Do not commit directly to Master/Release 99.9% of time”. We don’t blindly commit directly to “Master/Release”, but allow exceptions (0.1% of time).
One other feature that can be nice is squash merges - then main/master ends up with a single commit from the PR rather than adding a potentially large list of commits to the main branch. Not a huge point but it’s handy for keeping things tidy.
Ofc in best scenario everything should go through Code Review.
If i can tell my opinion, IMO core-devs should have possibility for commiting into Master, but not abusing it.
Because imo its about abusing the power ;p There can be possibility but just for a crisis situations.
Also there might be cases were there would be noone to code review,
example in Minie in situation where Sgold for example will change physics algorithm, i dont think anyone would be capable of verify if its correct anyway.
So in cases like this committing into Master do not seem un-reasonable
If by auto-formatting you mean every commit of Java code to “master” goes through formatting software, then I disagree. There is another answer: we can transition our codebase to compliance with preferred style one step at a time, starting with whitespace, then adding braces, and so on. Or alternatively starting with “com.jme3.math”, then adding other packages. We would configure CheckStyle so that any rule violation causes a build failure. After each step, we would enable the corresponding CheckStyle rule or update the covered portion of the codebase.
I think about this every day. Were I actively working on this, I’d lean heavily on my editor’s auto-formatting functions, of course. But I’d also take responsibility for each edit and especially for the readability of the result. One thing holding me back is the fear that editing thousands of files will create merge conflicts for the open PRs.
I don’t worry about polluting “git blame” output since GitHub’s “blame” function makes it easy to skip past a commit if it’s not the one you’re looking for.