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