Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add a guide for commit messages #686

Merged
merged 1 commit into from
Jan 17, 2024
Merged

docs: add a guide for commit messages #686

merged 1 commit into from
Jan 17, 2024

Conversation

lsf37
Copy link
Member

@lsf37 lsf37 commented Oct 20, 2023

Add a guide for how to write commit messages and commit message tags to make the messages more consistent and to help new people on the project get started more quickly.

@lsf37 lsf37 added the docs Documentation, READMEs, etc label Oct 20, 2023
@lsf37 lsf37 self-assigned this Oct 20, 2023
@lsf37
Copy link
Member Author

lsf37 commented Oct 20, 2023

The list of currently used tags is generated from the repo and probably still contains some few that are inconsistent or unwanted. Help appreciated in weeding these out.

docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
Copy link
Contributor

@michaelmcinerney michaelmcinerney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small nitpicks. But this looks very good and is very helpful. Especially for newcomers. Thanks Gerwin

docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
@corlewis
Copy link
Member

One thing I'm not sure about for the tags, are we more interested in the intention behind the commits or the specific files changed?
For example, let's say that we're making a change to lib and are then updating the rest of the proofs as required. However, it turns out that the only change needed is in proof/refine/ARM. In this case, which tag would be more useful, proof or arm refine?
I can see arguments for either, the latter is more useful for seeing which part of the repository have or have not changed, but it also implies more relevance than actually exists and could be distracting in the git history.

@lsf37
Copy link
Member Author

lsf37 commented Oct 22, 2023

One thing I'm not sure about for the tags, are we more interested in the intention behind the commits or the specific files changed? For example, let's say that we're making a change to lib and are then updating the rest of the proofs as required. However, it turns out that the only change needed is in proof/refine/ARM. In this case, which tag would be more useful, proof or arm refine? I can see arguments for either, the latter is more useful for seeing which part of the repository have or have not changed, but it also implies more relevance than actually exists and could be distracting in the git history.

Generally we want the intention. If it's cheap it's also good to be specific, though, because like you said it enables one to quickly see what has not changed. So in this specific case, I'd go for arm refine, but I'm open to arguments against.

Copy link
Member

@corlewis corlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up, it should be very helpful in us keeping a useful git history!

docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
- You are trying to explain things to your future self or a future colleague
5-10 years from now. You can assume knowledge of the repository in general,
but you should not assume specific context that is obvious to you right now,
but that will not be known to a different person 5 years from now.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if more emphasis is needed on not being lazy and/or doing header-only commits because they're "obvious". In theory yes, in practice would it really help ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The paragraph is probably long enough. I don't think more emphasis would achieve much.

docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
docs/commit-messages.md Outdated Show resolved Hide resolved
If the change applies to many proofs, for instance large lemma renaming commits,
we use tags such as `proof` and `spec`.

We combine tags with `+` if a change applies to multiple parts, e.g.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's another thing to consider on the line below, when to not combine lib and proof changes. I sometimes want to git show the commit to see what was the relevant library change, and not see every update that could go into a proof: update for <whatever lib changes>.
This is not a clear-cut line, because it means there is a broken commit between the two, but it's very useful and our proofs can't really be bisected anyway due to the horrible runtime. I'm interested in people's thoughts on this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave this outside the commit message guide, because it's more a question of what commits one should make. If the proof upadte is small, I think combining them makes sense, but if the proof update is large I also like the split version better, because it enables you to figure out more easily what content has actually changed in the spec.

docs/commit-messages.md Outdated Show resolved Hide resolved
@Xaphiosis
Copy link
Member

I think this is excellent, especially given the very short production timeframe (and no comma/just avalanche!
Added thoughts for discussion now that I had a sec, and I second Gerwin's opinion about intention and tagging above.

@lsf37
Copy link
Member Author

lsf37 commented Jan 17, 2024

Have addressed the feedback and am planning to merge when the tests are through. Still happy to discuss further changes, the content is not set in stone.

Add a guide for how to write commit messages and commit message tags to
make the messages more consistent and to help new people on the project
get started more quickly.

Signed-off-by: Gerwin Klein <[email protected]>
@lsf37 lsf37 merged commit 8f5e654 into master Jan 17, 2024
9 checks passed
@lsf37 lsf37 deleted the commit-msgs branch January 17, 2024 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation, READMEs, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants