Thanks Valentin. Sounds all good to me. As long as there is the exception you mentioned where a contributor can organize multiple (few) atomic commits in the same PR and merge them as they are.
Cheers Dave On Fri, Aug 28, 2026 at 2:02 AM Valentin Buira via QGIS-Developer < [email protected]> wrote: > Hi devs, > > As the subject of my email suggests, I would like to open up a > discussion on our commit policy. And ultimately, how do we handle the > end of life of a PR. I think there are instances in our workflow where > there is a loss of history and information for git blame and other git > tools, because the commits that matter are buried within less > important commits. > > I think the biggest culprit of noisy history in the git tree is > commits that should exist only in the span of a branch get merged into > master. > For example, imagine you are working on a feature with two commits > initially, and during review you happen to completely change the > implementation, then, after one final review you would have a local > history that looks like this: > > * Merge into master commit > | \ > | * commit D fix typos, grammar, address final review > | | > | * commit C completely reworked the PR pls forget about A and B > | | > | * commit B > | | > | * commit A > | / > | > > On merge, commits A, B, and D get into the tree history and in the > inline git blame, yet they don't matter for eyes outside of the local > branch. > Today, the PR description is the actual authoritative reference, and I > believe this is what should get in the history tree instead of > intermediate work commits. > > * Because of this configuration, two things happen at the same time. > There is a loss of information for the git blame, overridden by the > latest meaningless commit message e.g "apply suggestion from code > review" "grammar". And at the same time, commits that are already > outdated as soon as they leave their working branch get merged into > the main branch anyway. > > * Related to my previous point, git bisect to find regressions is made > more difficult because of noisy commits in-between the real changes. > > * Reverting and cherry-picking a single feature is harder. When > reverting a PR, we have to revert as many commits as there were in the > original PR. The same applies to cherry picking. > > * Vendor lock-in with Github. This one is adjacent to git but also > blends with it. > Currently, my workflow to understand a commit is the following : git > blame > open commit on github > click on the PR associated with the > commit to get the full picture of the changes. > So to effectively understand the history of QGIS we are effectively > dependent on github. > And I think github is becoming a liability. The uptime of github is > noticeably lower than it used to be. We are subject to any policies > they want regardless of how we feel (e.g their pro AI stance). And > more broadly github is based in the USA, which also means a lot of > uncertainty on what the current US administration could do next. > Unfortunately even switching to another git forge would make it > difficult to retrieve the history we have today embedded in github. > > > And now for a little anthology of commits we can find in the git tree: > git log -i -E --grep='fix (\w* )?build' --oneline | wc -l > 1364 occurrences > git log -E --grep='auto sipify' --oneline | wc -l > 763 > git log -i -E --grep="Apply suggestions? from (code review|@)" --oneline | > wc -l > 223 > git log -i -E --grep="add?ress (review )?comments?" --oneline | wc -l > 65 > git log -i -E --grep="^fix typos?" --oneline | wc -l > 387 > git log -i -E --grep="make (\w* )?happy" --oneline | wc -l > 47 // QGIS developers are like that, we like to make people happy > > > So, now that I have presented cases where I think we would benefit > from a more linear history, how do we reduce the noise to signal ratio > in our git history? I think we should tend towards atomic commits. > > What is an atomic commit? Qt's wiki defines an atomic commit as a > "commit that should contain exactly one self-contained change." [0] > A self-contained change is itself not really defined in Qt's wiki, but > to me a self-contained change should provide at least: > * Buildable, and working > * All tests are passing > * Does not mix unrelated changes > The only point at which all these conditions are guaranteed to be met > would be on a squash of an approved and passing ci PR. Now I can also > see value in having multiple commits per PR, for example if in the end > you squashed your PR into two commits, i.e: one for the feature and > one for the tests that would make two atomic commits too. > > In an ideal world developers would write code right on the first try > but in reality it's a much more organic process. So I would suggest > this simple policy: > * By default squash a PR before merging into master > * IF and only if the contributor specifically asks for their branch to > be merged, merge as is > > And abide by a few rules like: > * Always separate refactors from features into two separate PRs. ( Yes > that would probably mean slightly more PRs from core devs, but also > smaller, easier, and faster to review PRs) > * We could introduce a commit message guideline [0] but this is to be > discussed > > I think this way we could achieve a more linear, and more importantly > a more human readable history. > > Any thoughts on this ? It's still early on but depending on the > outcome of the discussion I will create a QEP similar to QEP 314 but > for commit guidelines. > > Cheers, > Valentin > > [0] https://wiki.qt.io/Commit_Policy > [1] https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html > > Finally here are a few references from other open source projects I > used to write this mail: Godot, Qt, and Blender > > https://contributing.godotengine.org/en/latest/pull_requests/pull_request_guidelines.html#contribute-one-change-at-a-time > https://wiki.qt.io/Commit_Policy > https://developer.blender.org/docs/handbook/contributing/review_playbook/ > _______________________________________________ > QGIS-Developer mailing list > [email protected] > List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer > Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer >
_______________________________________________ QGIS-Developer mailing list [email protected] List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
