Add a git commit guide #8965
Add a git commit guide #8965
Conversation
wmontwe
force-pushed
the
add-git-commit-guide
branch
from
d539a4b to
e481327
Compare
| - Explain *what* the change does and *why* it was needed. | ||
| - Describe any important details, such as edge cases addressed. | ||
| - Reference relevant issue numbers or discussions. | ||
|
|
There was a problem hiding this comment.
Adding an example for commit message that follows above structure would be helpful.
There was a problem hiding this comment.
Yes, I think having examples would help.
| ### 5. Ensure Commits Are Self-Contained | ||
|
|
||
| Each commit should: | ||
|
|
||
| - Be complete and independently buildable. | ||
| - Not introduce temporary hacks or broken functionality. | ||
| - Ideally, pass tests to maintain project integrity. |
There was a problem hiding this comment.
You mention most of these before, potentially we could consolidate here to not make the guide too long. In section 2 you mention each commit should be functional and not leave it in a broken state. This covers the first item, and potentially the third, and with a light stretch also the second.
There was a problem hiding this comment.
I'll rework the sections.
| ### 7. Use Atomic Commits | ||
|
|
||
| Atomic commits mean that each commit represents a single unit of change that can be reverted independently. This | ||
| practice ensures that if an issue arises, it is easier to pinpoint and undo the specific problematic change. |
There was a problem hiding this comment.
Based on how we've worked before I had the understanding that we're working atomic at the level of a merge, but that individual commits within the merge might not be completely functional.
I think this is worth clarifying. If we really require atomic commits to each individual level, then we might as well do rebases instead of merges because each commit should work as it is and we can just back out the offending commit.
If we're working at the merges level, then while we shouldn't intentionally break between commits, we'd be backing out the merge and bisecting based on merges.
There is also some overlap between (6) and (2) where we're again saying it needs to work inbetween commits.
There was a problem hiding this comment.
This doesn't mean to have something completely functional, but it should compile, tests should succeed and all quality tools issues should be resolved. This is meant to prevent commits like:
- Add funtionality
- Add test for functionality
- Fix broken tests
- Fix linter issues
- Fix code formatting
- ...
These changes should be part of one commit.
I'll reword it.
There was a problem hiding this comment.
So for example in #8987 I should have merged the two commits, because the actions fixes would otherwise cause a broken state?
There was a problem hiding this comment.
This is a special case. If it is detectable using our build, quality checks and tests then yes, otherwise I think it's fine.
Resolves #8964