[Docs] Add more details to commit message styleguide #1805
Conversation
Signed-off-by: Michał Kowalczyk <mkow@invisiblethingslab.com>
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow)
CONTRIBUTING.rst line 147 at r1 (raw file):
#. Meaningful commit messages (it's much easier to get them right if commits are really atomic). Commit messages should follow `commit message style guidelines <coding-style.html#commit-message-formatting>`__.
https://gramine.readthedocs.io/en/stable/devel/coding-style.html#commit-message-formatting？
Code quote:
coding-style.html#commit-message-formatting
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @kailun-qin and @mkow)
CONTRIBUTING.rst line 147 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
https://gramine.readthedocs.io/en/stable/devel/coding-style.html#commit-message-formatting？
+1, when reading this file outside of ReadTheDocs, it has a broken link: https://github.com/gramineproject/gramine/blob/mkow/commit-style-guidelines/CONTRIBUTING.rst#reviewing-guidelines
(Also, we use full URLs in all other places in this file.)
Documentation/devel/coding-style.rst line 46 at r1 (raw file):
Additionally, commit message one-liner should include which component was changed (PAL-{Linux,SGX} / LibOS / Docs / CI) in the format
Maybe each component with double-backticks, otherwise it will be not-nicely formatted?
Documentation/devel/coding-style.rst line 47 at r1 (raw file):
Additionally, commit message one-liner should include which component was changed (PAL-{Linux,SGX} / LibOS / Docs / CI) in the format "[component] change description". The one-liner should be
change -> Change
Documentation/devel/coding-style.rst line 54 at r1 (raw file):
Thus, please restrain from descriptions like "Fixes #1234" and instead describe the fix in the commit message. If you really need to reference something on our GitHub, then use the full URL instead of #1234, so that it won't break if we
Actually, our current rule of thumb is to use commit titles like:
As was explained in commit "[Docs] Add more details to commit message styleguide",
we don't allow writing commit titles in the past tense.
I don't mind the "full URL" way too, but let's decide on one consistent recommendation.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @kailun-qin and @mkow)
a discussion (no related file):
Do we want to mention git fixup style of follow up commits?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Do we want to mention
git fixupstyle of follow up commits?
+1
Documentation/devel/coding-style.rst line 54 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, our current rule of thumb is to use commit titles like:
As was explained in commit "[Docs] Add more details to commit message styleguide", we don't allow writing commit titles in the past tense.I don't mind the "full URL" way too, but let's decide on one consistent recommendation.
I'm fine w/ "full URL" and "to use commit titles" does not cover the cases where people point to things other than commits (repo, files, other links etc.).
Signed-off-by: Michał Kowalczyk <mkow@invisiblethingslab.com>
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
a discussion (no related file):
Previously, kailun-qin (Kailun Qin) wrote…
+1
We already do, in: https://gramine.readthedocs.io/en/latest/devel/contributing.html#pr-life-cycle (which is probably the right place for it, because it explains how iterating over reviews works).
CONTRIBUTING.rst line 147 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1, when reading this file outside of ReadTheDocs, it has a broken link: https://github.com/gramineproject/gramine/blob/mkow/commit-style-guidelines/CONTRIBUTING.rst#reviewing-guidelines
(Also, we use full URLs in all other places in this file.)
Yeah, I wasn't sure what to do here. The problem is, that if I use full URL then clicking the link in stable docs redirect to latest (or vice versa, depending which URL I hardcode). Currently we sometimes link to stable and sometimes to latest...
Documentation/devel/coding-style.rst line 46 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Maybe each component with double-backticks, otherwise it will be not-nicely formatted?
Done.
Documentation/devel/coding-style.rst line 47 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
change->Change
Good catch, fixed.
Documentation/devel/coding-style.rst line 54 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
I'm fine w/ "full URL" and "to use commit titles" does not cover the cases where people point to things other than commits (repo, files, other links etc.).
I prefer commit titles, because they will survive any repo migration and make our repo self-contained. Of course other links should just be normal links, I only mean referencing stuff from our own project. And for files it should just be project-root-relative paths.
@kailun-qin: Does it sounds good? If so I'll update the description.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
We already do, in: https://gramine.readthedocs.io/en/latest/devel/contributing.html#pr-life-cycle (which is probably the right place for it, because it explains how iterating over reviews works).
Well, yes, ok. No need to multiply entities.
CONTRIBUTING.rst line 147 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Yeah, I wasn't sure what to do here. The problem is, that if I use full URL then clicking the link in
stabledocs redirect tolatest(or vice versa, depending which URL I hardcode). Currently we sometimes link tostableand sometimes tolatest...
Hm, still, I would prefer to be uniform in this file. I also think that most people will read this file from the GitHub repo, not from RTD.
Documentation/devel/coding-style.rst line 46 at r2 (raw file):
Additionally, commit message one-liner should include which component was changed (``PAL-{Linux,SGX}`` / ``LibOS`` / ``Docs`` / ``CI``) in the format
Actually, this PAL-{Linux,SGX} is not how we prepend the commits: https://github.com/gramineproject/gramine/commits/master
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
CONTRIBUTING.rst line 147 at r1 (raw file):
The problem is, that if I use full URL then clicking the link in
stabledocs redirect tolatest(or vice versa, depending which URL I hardcode).
Yes I agree -- but for this case, but for this case maybe we could point to the latest? This is a newly added section (otherwise non-exisiting link), and the vice-versa (link in stable docs redirect to latest) should be fine as we just point to some latest (and potentially more correct) information from an older doc (i.e., we don't really care about the compatibility in this case).
Documentation/devel/coding-style.rst line 54 at r1 (raw file):
I only mean referencing stuff from our own project.
I prefer commit titles, because they will survive any repo migration and make our repo self-contained.
Sure, sounds good.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @mkow)
CONTRIBUTING.rst line 147 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
The problem is, that if I use full URL then clicking the link in
stabledocs redirect tolatest(or vice versa, depending which URL I hardcode).Yes I agree -- but for this case, but for this case maybe we could point to the latest? This is a newly added section (otherwise non-exisiting link), and the vice-versa (link in
stabledocs redirect tolatest) should be fine as we just point to some latest (and potentially more correct) information from an older doc (i.e., we don't really care about the compatibility in this case).
CONTRIBUTING (and README, and all other things in root directory) are mostly for GitHub, not for sphinx, unfortunately. I'd suggest to remove the include from Documentation/devel/contributing.rst and instead wrote something like "please see latest contribution guides at https://github.com/gramineproject/gramine/blob/master/CONTRIBUTING.rst". Then fix this to full URL as @kailun-qin suggested.
That way we also won't have outdated contributing guidelines in old sphinx docs (I mean, docs for older versions on RTD). IIUC contributors are expected to always follow latest guidelines, even if they're reading docs for 1.0.
CONTRIBUTING.rst line 74 at r2 (raw file):
#. Address a single problem. #. Clearly explain the problem and solution in the PR and commit messages, using grammatically correct American English.
Can we please remove the requirement for "American" English yet? We're software project, not linguistic project, and requiring people to write eloquently, in specific dialect of a foreign language, is wasteful. Especially when most frequently judged by non-native speakers (a Polish and a Russian). This requirement is simply a waste of contributor's time, esp. if not already very proficient at American™ English®, as understood by two Europeans.
Documentation/devel/coding-style.rst line 54 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
I only mean referencing stuff from our own project.
I prefer commit titles, because they will survive any repo migration and make our repo self-contained.
Sure, sounds good.
Fixes:, if used, should contain reference to another commit, and that makes it self-contained. If you need to know which PR on Github contains given commit, you go to github's page of the commit (https://github.com/gramineproject/gramine/commit/abcd123/) and it's somewhere near commit title.
When refering to another commit (in Fixes: or not), one should put at least short hash before title, so that if you want to search for it, it's a single git show abcd123 away.
Kernel has nice config available for it (https://kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes):
[core]
abbrev = 12
[pretty]
fixes = Fixes: %h (\"%s\")
$ git log -1 --pretty=fixes 54a4f0239f2e
Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed")
(If not adding fixes, then you just copy-paste whatever it formatted).
Signed-off-by: Michał Kowalczyk <mkow@invisiblethingslab.com>
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @woju)
CONTRIBUTING.rst line 147 at r1 (raw file):
I think I prefer the current way (i.e. CONTRIBUTING.rst rendered in RTD), because it looks and works better there - e.g. links to specific sections doesn't work on GitHub (grep for (see :ref:merging_policy).
Yes I agree -- but for this case, but for this case maybe we could point to the latest?
Yeah, I think it should be that way, done.
CONTRIBUTING.rst line 74 at r2 (raw file):
I'm against removing it, it's here for consistency and it's really that not hard to follow. I don't know how this relates to eloquence, this is mostly about consistent spelling of words and variable names across the whole project (-ise vs -ize, etc). No one is fighting here over American vs British idioms, we don't use them anyway because we want to keep the language simple.
Especially when most frequently judged by non-native speakers (a Polish and a Russian).
How is our nationality an argument here?
Documentation/devel/coding-style.rst line 54 at r1 (raw file):
$ git log -1 --pretty=fixes 54a4f0239f2e
Hmm, my git doesn't recognize this format, is this something very recent? If so, I wouldn't recommend that in the guide, because people will be complaining that it errors out for them.
I'm also not sure whether we should use short hashes anywhere, I don't like them because they are collidable (by a malicious contributor, and we have no chance to notice that). Not super relevant here, but I just don't like the idea of using them anywhere beyond manual command line navigation.
Documentation/devel/coding-style.rst line 46 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, this
PAL-{Linux,SGX}is not how we prepend the commits: https://github.com/gramineproject/gramine/commits/master
Right, I copy-pasted it from CONTRIBUTING and didn't notice that it has a mistake. Fixed.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @woju)
Documentation/devel/coding-style.rst line 54 at r1 (raw file):
Hmm, my
gitdoesn't recognize this format, is this something very recent?
I think it's not recent (as it was in kernel doc quite long ago)? It works fine on my setup (git version 2.34.1).
Well anyway, I don't have strong opinion here.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @woju)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @mkow)
CONTRIBUTING.rst line 147 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I think I prefer the current way (i.e. CONTRIBUTING.rst rendered in RTD), because it looks and works better there - e.g. links to specific sections doesn't work on GitHub (grep for
(see :ref:merging_policy).Yes I agree -- but for this case, but for this case maybe we could point to the latest?
Yeah, I think it should be that way, done.
This also works.
CONTRIBUTING.rst line 74 at r2 (raw file):
it's here for consistency
We should not try to enforce "consistency" in the way people communicate.
How is our nationality an argument here?
Because you're not an authority in the field of English language, at least not to a degree that you're an authority in the field of IT security.
This requirement is problematic, because it is hard to follow. People are already complaining off-line about it being hard to contribute to our repos because of too pedantic review, and this requirement is part of the problem.
Last but not least, this requirement was a pretext for you to block #1670 after we voted on the text by supermajority, contrary to the voting result. You have no right to override the voting result of the whole management committee, and I think you shouldn't have a tool for blocking the voting results.
Documentation/devel/coding-style.rst line 54 at r1 (raw file):
Hmm, my
gitdoesn't recognize this format
It will, after you add the cited config section ([pretty] fixes=)
I'm also not sure whether we should use short hashes anywhere
[core] abbrev=12 is because of the default of 7 hexdigits was dangerous. But this is irrelevant, in this case the hash is used just as an identifier (and is not recalculated e.g. to verify signature or sth). What is relevant is probability of honest, non-malicious collision and kernel decided to use 12 digits because of sheer volume of commits to kernel.
Anyway, the added sentence is OK I think.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @woju)
CONTRIBUTING.rst line 74 at r2 (raw file):
it's here for consistency
We should not try to enforce "consistency" in the way people communicate.
We aren't enforcing it in our discussions, only in committed, official documentation.
How is our nationality an argument here?
Because you're not an authority in the field of English language, at least not to a degree that you're an authority in the field of IT security.
I still don't see why is this a valid argument, especially that you mention the nationality, not my English proficiency.
This requirement is problematic, because it is hard to follow. People are already complaining off-line about it being hard to contribute to our repos because of too pedantic review, and this requirement is part of the problem.
This problem happens almost exclusively in your PRs, and even there it's rare, you just don't want to fix those places after they are pointed out. And yes, our review is very pedantic, but that's really unrelated to Am. Eng. vs Br. Eng.
Last but not least, this requirement was a pretext for you to block #1670 after we voted on the text by supermajority, contrary to the voting result. You have no right to override the voting result of the whole management committee, and I think you shouldn't have a tool for blocking the voting results.
I'm not blocking the resolution (which effects are already in place), just want to fix the typos there. This really shouldn't require re-voting.
Documentation/devel/coding-style.rst line 54 at r1 (raw file):
Hmm, my
gitdoesn't recognize this formatIt will, after you add the cited config section (
[pretty] fixes=)
Ah, it's a custom config, I missed that, sorry.
Description of the changes
As in the title. The text turned out to be a bit clumsy, so if you have a good idea how to write it more nicely and concisely then please suggest it in the review.
This change is