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
