I think in a team with good ownership, enforcing formal reviews slows down a lot. But of course in a larger code base where no single engineer can understand all the effects a change might have, having a bit of knowledge sharing and 4 eyes enforced is often the better approach than yolo-ing it.
Then again I did build an SQL review tool for database access because I felt like "yolo-ing" production access was also not the way it should be. It's an interesting slider between full autonomy and strict review processes where each team needs to find their sweet spot: https://github.com/kviklet/kviklet/
The team was small, around 6 people, and the codebase was maybe medium sized (~500k LOC). There was no formal review process, instead it was up to each team member to ensure the quality of their own and others code. In practice I would read through all commits that came in the previous day while having my morning coffee. If there was some egregious I would talk to whoever made to commit to make discuss if something should change, but this was fairly rare.
Formal PR reviews were only ever really used for new members or for bigger/sketchy changes where someone wanted more eyes on it.
Because I ended up reading most commits, I ended up knowing how pretty much the entire codebase worked. It takes a while for this to develop, but the more you do it the better you get at it, especially in the context of a single codebase.
There's nothing wrong with small, short-lived branches that can be quickly reviewed and merged into main.
That being said, I've been in a small team where the blessed style was to commit directly to main and do reviews "on demand". It quickly gets features deployed in a way that builds a lot of rot and debt into your project that people quickly lose a good understanding of. Then you just keep piling.
There's probably a way to get this done properly with a tiny team of very experienced and well-aligned developers; but that's usually not what you have in an environment that pushes this kind of extreme no-review-interpretation of trunk-based development.
Slow down, do reviews, avoid keeping branches open for more than a day.
I think this is very key, if the development style and the direction of the project is clear, much less review and alignment is necessary.
And also
> avoid keeping branches open for more than a day
Big +1 on that, fast reviews are extremely key. Most teams I have seen often took days or even weeks to merge branches though, often because you end up waiting too long for reviews in the first place. Or because of good old bike-shedding. But also because these code reviews often uncovered uncertainties that needed longer discussions.
However usually code is easy to change, so defaulting to "just merge it" and creating followup tasks is often the cheaper approach than infinite review cycles.
Once the code is merged, chances are it will not get changed Those follow-up tasks will be displaced by more pressing work that will keep piling onto a slightly unstable foundation, increasing the tilt over time.
There is an excluded middle between "no reviews" and "infinite review cycles": proper, timely and efficient reviews. They are worth investing the time to get right. They will start paying dividends months down the line, and boy will they keep paying.
This is not about trying to get things perfect from the get go, but to get them done right while you're there. "We'll fix it later" is not gonna happen, and is much more expensive than it initially seems.
I wish this was the "default" mindset everywhere, especially in those cases where you have that one colleague that loves to nitpick everything and doesn't see an issue with holding up both releases and wasting your time over menial pedantic stuff. It would be so much easier to merge working code and let it work, and keep those TODOs in the backlog (e.g. trash).
In a sane world, code review would be like:
1. Will this work and not break anything? We checked it, it's okay. There are no apparent critical or serious issues here.
2. Here's a list of stuff that you can change if you wish, here's why it might be an improvement.
3. Okay, we have some left-over nice to haves, let's keep track of those for later (or not) and merge.
> There's nothing wrong with small, short-lived branches that can be quickly reviewed and merged into main.
I would have called this "branch based development", personally.
https://www.atlassian.com/git/tutorials/comparing-workflows/...
But yeah, that’s what people generally settled on, naming wise.
I think you're confusing workflows with commit history.
You can work with feature branches all you want, rebase them as you feel like it, and then do squash merges to main.
The likes of GitHub even have a button for this.
Assumption: above mentioned total noobs don't use git rebase -i or equivalent, everyone else does
In 25 years of professional development I’ve never really had a situation where the commits on a branch would have helped me understand what was going on a year ago when the work was done. That includes pretty big bits of project work.
I’d much rather have a trunk with commits at the granularity of features.
What has happened a whole lot though is the exact opposite.
As such, a new codebase is likely to grow by large unwieldy commits and a mature one by targetted small commits.
I've had separate commits come in handy several times when `git blame`ing when working with people who actually described what changes were about in their commits (which, unlike comments, don't go out of date).
It's less about reviewing commits from a year ago, than making change low-risk today. And small commits can easily be rolled back. The bigger the commit, the more likely rollback will be entangled.
It better to have partial features committed and in production and gated behind a feature flag, than risk living in some long-lived branch.
You're not addressing the problem. You're just wishing that the problem wouldn't happen as frequently as it does.
But that's like wishing that race conditions don't happen by making your allocations at a higher frequency.
Also, your comment reads a little bit as a non sequitur.
Coincidentally, every single squash-merge commit advocate I've had the unfortunate debate with was a regular practitioner of public tmp / tmp / try again / linter / tmp / fix / fix / haaaaaands commits.
Note that I'm not against squashing/history rewriting e.g rebase -i and stuff (which I'm a heavy user of so as to present sensible code aggregation reviewable per-commits), only squash-merge.
I chunk my changes into tiny commits ("linter"/"tmp"/"wip"), but then rebase aggressively, turning it into a set of logical changes with well-formed commit messages. git bisect/revert work great with history written in this way even years layer.
But: most of the people I've been interacting with also produce lots of "wip"/"tmp", but then skip the rebase. I can only offer my help with learning git rebase for so long before it starts taking too much time from the actual work. So squash it is: at least it produces coherent history without adding thousands of commits into `--ignore-revs-file`.
> I chunk my changes into tiny commits ("linter"/"tmp"/"wip"), but then rebase aggressively, turning it into a set of logical changes with well-formed commit messages. git bisect/revert work great with history written in this way even years layer.
In a PR based workflow, it has become easier to have the PR be a logical unit than to `rebase -i` all the time on my end.
A ticket should be atomic describing a single change request. PR in this case are the working room. It can be as messy or as clean as you want. But the goal is to produce a patch that introduces one change. Because if you would rebase -i at the end, you would have a single commit too in the PR.
A typical bugfix branch might look like this after rebase -i:
Move property to a more appropriate place
Improve documentation of feature Foo
Fix accidental O(n^2) in feature Bar
Fix interaction of Foo with Bar
TN 43 - Fix mismatched interface between Foo and Bar
We've moved the X property to a more appropriate place and
improved the documentation for Feature Foo. We've also found and fix
an O(n^2) implementation in feature Bar.
The commit message is what's more important. I don't think I've ever needed what is in a merged branch. But I've always wanted the commit at one point to have tests passing and a good description of the patch. And all the talk in the engineering team are always about ticket. It does makes sense to align those.
I strive to do that when making commits for work too, and that helps when going back in history and looking at history to motivate why a change was made.
While working I rebase all the time to move changes into the relevant commit, I don't find that particularly hard or time consuming. Doing this upfront is easy, splitting commits after the fact is not.
I consider this standard practice, at least in the sector I work in (industrial equipment control software, some of which is considered human safety critical).
This constraint is usually enforced by code review.
On Github, the unit of code review is the PR.
Therefore, I prefer squashing.
It is, and that is some bullshit. The only sensible way to work with that is to break up larger features into several PRs - which is often positive anyway, but sometimes it doesn't fit the nature of the change.
But my point is, is that I believe the important thing to preserve in history is whatever your unit of review is. If you could stack PRs and each were subject to the individual review, I would not combine and squash those (just the individual commits within each PR).
Why would you not want to squash merges? It's one of three options offered by GitHub in their PR merge buttons.
They create a linear commit history, which is what you want when you have to audit changes.
> except to clean up messy mini-branches written by total noobs.
Nonsense. You get a messed up commit history as easily as when you create a PR in GitHub, and after team members merge their commits, you click on GitHub's "update branch" button.
You also get a messed up commit history if you merge a PR after someone else merged theirs.
You will always mess up your pristine commit history if you have more than one person posting and merging branches. With one notable exception: squashed commits.
Seriously wish the stacked PR workflow would gain more traction outside of FAANG. Apart from the (somewhat pricey) Graphite offering, there's no standard UI for managing stacked PRs in the wild.
[1]: https://lore.kernel.org/git/CAESOdVAspxUJKGAA58i0tvks4ZOfoGf...
[1]: https://x.com/jaredpalmer/status/2019817235163074881?s=20
Gerrit looks ugly and is not very easy to use at first, but at least it's not slow like GitLab and GitHub.
especially combined with monorepo
amount of time people spend updating dependencies between internal services and libraries in a pursuit of semver for now reason is just absurd
Do you take their tagged release, fix it there, and then send them that branch release with the fix, or do you send them a fix on current main - you know, the main that is now a million releases ahead with multiple breaking changes? And what about all the intermediate release tags? Do you fix each one there too if they have the problem, or do you only update when customers on those releases start having that issue too?
And if you fix to their old tagged release which is incompatible from main, does this mean you have to do this fix twice i.e on their tagged release and also fix it for main? But what if this fix affects other people who are all on different branches too? Now… times this by 20 different customers all running different hardware and different branches with different problems :(
Maybe my comments are off topic, and don’t get me wrong - I prefer “trunk is releasable” motto, but I think maybe as an industry we should all come up with an Acid Test (like the only CSS Acid Tests) so we can through all these branching strategies into the ring
If you build a software that you distribute so people can deploy it themselves (a library, a self-hostable solution, ...), then you most likely semantic versioning. In that case, the best model is to use what semantic release offers.
We just cherry-picked stuff back to release branches, if we needed a fix.
The triple mortal loop, comes that we have two versions of the product. One with the old no responsive frontend and other with a modern responsive frontend. And we need to release and develop the two versions for sometime, before the direction decides to kill the old no responsive version. So we end with 4 branches: release, release_rwd, develop and develop_rwd. If we fix something in release, we need to do a diamond merge : release to release_rwd, release to develop, release_rwd to develop_rwd and develop to develop_rwd