[RFC] Add Commit Message Guideline

[gromero]

This RFC proposes adding a Commmit Message Guideline to Apache TVM
documentation to help guide contributors on how to write good commit
messages when submitting code / Pull Requests.

Co-authored-by: Leandro Nunes leandro.nunes@arm.com

[gromero]

@tqchen @jroesch @driazati @areusch @Mousius @manupak @tkonolige @mbaret @leandron @alanmacd

[gromero]

@areusch @leandron Could you please help me on how to trigger a [VOTE] thread on this RFC? Cheers.

[areusch]

@gromero thanks for sending the RFC! let's leave it open for a few days for discussion, then I will create a [VOTE] GH issue.

[tkonolige]

Getting consensus on commit messages is great!

Can you also include guidelines for 1. how the PR author can update their commits if they are not up to this standard (rebase?) 2. How reviewers should handle squashing an merging (should they copy text from the PR description or from a commit) 3. how to handle commits that fix linting or formatting issues in the original PR.

[slyubomirsky]

Committers have the ability to edit others' messages, I believe, so would it be incumbent on the committer who is merging the PR to ensure the topic post is up-to-date?

[slyubomirsky]

I guess it would be easier for the committers to request updates from the author before merging

[mbaret]

I'd say ideally we want it to be the author's responsibility, otherwise this will end up always been left to the committer. That being said, the committer should always seek to fix any issues prior to committing but they should retain the right to inform the author of the rules and encourage them to follow them in future. We probably need more explicit language around the author/committer responsibilities.

[gromero]

I agree with @mbaret I'll improve this part and post a v3.

[gromero]

Addressed here: afcbb98

[leandron]

@mbaret can you have a look and mark it as resolved In case you agree, or propose a suggestion in case this needs further improvement?

[mbaret]

seems good - will leave to @slyubomirsky to mark as resolved though given it's his chain.

[mbaret]

I'd say ideally we want it to be the author's responsibility, otherwise this will end up always been left to the committer. That being said, the committer should always seek to fix any issues prior to committing but they should retain the right to inform the author of the rules and encourage them to follow them in future. We probably need more explicit language around the author/committer responsibilities.

[gromero]

@tkonolige Hi!

Can you also include guidelines for 1. how the PR author can update their commits if they are not up to this standard (rebase?)

Let's say a PR has just been created. The idea is that regarding the commit message (that will compose the final commit message) it doesn't matter the commit(s) message(s) of the commits used to create the PR. It really only matters the PR's title and body. So rather than a rebase + push force it's just a matter of updating the PR's title and body in the GH GUI.

Now, I believe you are thinking of the case when the author's commits are a rather difficult (like, quite badly organized) set of patches and the PR's title and body also don't help to understand the change. Since it's the "initial" commits (the PR has just been created), I think that it would be awesome that the author would reorganize it, splitting the commits in a reasonable way, then rebase and push again it, updating the PR title and body (or even creating a brand new PR) that would ease the review. That's what I tried to capture here in this part: 99c3bc2#diff-759e47e77c87b8e4c14dba43c17bac829cbb502e9369211c1ad751253dae57f5R126 but merely a recommendation, moreover now we have to consider also the comment from Matthew about multiple in a PR:
#88 (comment)

Personally, I don't want people to stuff a bunch of non-related commits into a PR, but, on the other hand I also think it helps me review the changes when the commits are split in reasonable way -- just for a single self-contained feature / change -- even if they will be squashed before merge (which is a pity I might say). But capturing such a nuances in the guideline in very case is hard.

2. How reviewers should handle squashing an merging (should they copy text from the PR description or from a commit)

I tried to address it right in the beginning here:

99c3bc2#diff-759e47e77c87b8e4c14dba43c17bac829cbb502e9369211c1ad751253dae57f5R46-R50

and then in the end here:

99c3bc2#diff-759e47e77c87b8e4c14dba43c17bac829cbb502e9369211c1ad751253dae57f5R138-R139

i.e. they should copy text from the PR description, title and body.

3. how to handle commits that fix linting or formatting issues in the original PR.

Do you mean the additional commits that are pushed to the branch as a consequence of the review process right? The ones we've been calling "additional commit messages"? If so, here is where I tried to address it:

99c3bc2#diff-759e47e77c87b8e4c14dba43c17bac829cbb502e9369211c1ad751253dae57f5R134-R135

i.e. it doesn't matter. All that matters for the final commit message that lands into the main tree is to keep the PR's title and body updated and following the Commit Message Guideline.

[tkonolige]

@gromero I think I am getting a little confused at the difference between messages in the commits composing the PR and the final commit in main. To make things clearer, I think it would help to refer to to commit title and commit message as PR title and PR description, respectively. PR title and PR description are the things that the PR author and the reviewers will be discussing (which will become the commit title and message).

To clarify, these guidelines do not apply to the individual commits (on the authors branch) that compose the PR?

i.e. they should copy text from the PR description, title and body.

I think we should explicitly say that whoever is merging the PR needs to copy the description of the PR into the commit message (unless we have changed GitHub to do this automatically?). Having clear instructions (like how to copy formatting) for mergers would be really nice.

[gromero]

@gromero I think I am getting a little confused at the difference between messages in the commits composing the PR and the final commit in main. To make things clearer, I think it would help to refer to to commit title and commit message as PR title and PR description, respectively. PR title and PR description are the things that the PR author and the reviewers will be discussing (which will become the commit title and message).

I tried to be consistent throughout the text the following meanings:

A commit message is comprised of a commit title and body (body is the same term used, for instance, on git-format manual to refer to that part of the commit)

The final commit message is composed of the PR title and body, i.e. the final commit message title = PR tile and final commit message body = PR body. When one creates a PR from, let's say, a single commit, that's the default.

To clarify, these guidelines do not apply to the individual commits (on the authors branch) that compose the PR?

Most importantly these guidelines apply to the PR title and body, since that's what will be used to compose the final commit message that gets committed to the main tree. But that's also applicable to the commits used to initially create the PR, since a good commit messages naturally helps the reviewer. Moreover, because typically (in GH for instance) the PR (title and body) is auto-generated from the initial commit messages, if you apply the guidelines to these commits you kill two birds with one stone: you get a nice PR "for free" and also help the review.

As the review develops (e.g. if changes are requested by the reviewers), all that's necessary is to keep the PR title and body in sync with the code changes.

i.e. they should copy text from the PR description, title and body.

I think we should explicitly say that whoever is merging the PR needs to copy the description of the PR into the commit message (unless we have changed GitHub to do this automatically?). Having clear instructions (like how to copy formatting) for mergers would be really nice.

@leandron @areusch @mbaret @driazati wdyt?

[leandron]

I think we should explicitly say that whoever is merging the PR needs to copy the description of the PR into the commit message (unless we have changed GitHub to do this automatically?). Having clear instructions (like how to copy formatting) for mergers would be really nice.

@leandron @areusch @mbaret @driazati wdyt?

It is a responsibility of the author to keep the GitHub PR title and description consistent with the work done. That's just basic housekeeping. It is a responsibility of the committer, by the time the GitHub PR is ready to be merged, to make sure a message exists and is sane, then copy the PR description and set as contents of the commit message the will be done in the git repository.

This RFC is not describing how people should do their work. It is merely creating a baseline for what is acceptable to maintaining a healthy repository with tracking of commits that contains a description of the changes being made.

Having clear instructions (like how to copy formatting) for mergers would be really nice.

It is out of scope for this specific RFC text to have such information (as in a step-by-step guide here). These guidelines you suggest are implementation details that we can do in many ways, prior to officially communicating committers about this new rule - when it comes to effect. (edit: I agree 100% that such information should be available, I just don't think this RFC text is the right place)

[driazati]

Moreover, because typically (in GH for instance) the PR (title and body) is auto-generated from the initial commit messages, if you apply the guidelines to these commits you kill two birds with one stone: you get a nice PR "for free" and also help the review.

(this isn't really a comment on this RFC since it's more about implementation details) This is a bit of a back and forth with committers / PR authors. As @leandron said it's ultimately the committer's responsibility to ensure that the final commit message (which is presented to them on GitHub when clicking "Squash and Merge") is of high quality. GitHub will auto-fill out that box with the commit messages so an author carefully rebasing their PR commit messages to match their PR title and body can ensure that it is going to be good, but I think that'd be difficult to coordinate across all contributors.

[gromero]

OK, I agree @leandron @driazati Thanks for the comments.

[junrushao]

Thank you @gromero and @leandron for this RFC! It definitely brings quite positive change to our community and codebase management! I went over the guidelines and they all make sense to me, and I just left some clarification questions :-)

In the meantime, different reviewers, as different human beings, may not be extremely cautious or always have a consistent standard. Therefore, I would love to propose a separate follow-up discussion (CC: @driazati @areusch) if we could somehow let our fellow @tvm-bot handle the enforcement of our standard here.

Let me know what you guys think!

[junrushao]

Just to clarify, we only use uppercase for the first letter and acronyms right? This means:

Fix TVM use of OpenCL library

is better than

Fix TVM Use of OpenCL Library

[leandron]

The example reflects a few variants, do you want us to include the particular one you suggest?

[junrushao]

I don't actually have a strong opinion :-) Let's just clarify it a little bit so that readers could better follow

[gromero]

@junrushao1994 That's right, i.e. form Fix TVM use of OpenCL library is better than form Fix TVM Use of OpenCL Library. But since in the RFC we used exactly the best form as the example, i.e. Fix TVM use of OpenCL library, I'm not sure how we could clarify further ... but wait, hm I think I get it, what can be ambiguous is that when it says "uppercase for the first letter" it can also be interpreted as the first letter of each word is capitalized, i.e. it can be interpreted as title case (https://en.wikipedia.org/wiki/Title_case), right? If so we should clarify saying explicitly it's uppercase for the first letter in the title and that title case must be avoided? Am I following you correctly?

[gromero]

@junrushao friendly ping :)

[areusch]

@junrushao1994 i would love for tvm-bot to enforce this as much as possible. some items are going to be more challenging than others. the thing i think we'd really like is for tvm-bot to handle merging, so that we consistently remember to copy the PR body into the git commit body. we've been debugging some auth problems with that which we're unable to reproduce locally; this has been unfortunately quite tricky to track down due to not known the organization's default GitHub Actions token settings. hopeful we can get a resolution on that and then move towards making "tvm-bot merge" a thing.

[tkonolige]

Thanks @gromero. This is a great improvement!

[driazati]

i would love for tvm-bot to enforce this

We can also enforce whatever rules as part of lint, apache/tvm#12367 has an example implementation for what we could do

[leandron]

I think most comments are addressing implementation details of what it being proposed here. There is not really any opposition to move forward with what is being proposed here.

@areusch you suggested to create a [VOTE] thread with this proposal? I guess we're ready to go?

Also wanted to highlight that despite this looking to be a set of enforced rules, in practice it is just trying to guarantee that we have the bare minimum in terms of tracking the changes in git with a title and textual description of the change being made. Nothing too far from what is enforced in any relevant open-source project.

[leandron]

@areusch can you have a final look and merge if you're happy, before creating the [VOTE] thread you mentioned?

[leandron]

I'm co-author but given I'm happy with the outcome of the discussion so far, therefore marking this approved as I'm not hearing any major disagreement with that is being proposed here.

[Hzfengsy]

A minor nit :)

[areusch]

I created a vote thread to broadcast this more broadly to the community. Please signal your support there.

@driazati notes that GitHub just introduced new options for controlling the default commit messages, so hopefully we can leverage that to implement the merge and complement that with a lint tool.

[driazati]

@driazati notes that GitHub just introduced new options for controlling the default commit messages, so hopefully we can leverage that to implement the merge and complement that with a lint tool.

This setting looks like it includes @ tags from PRs in the default commit message which may make it a non-starter for us since that generates a lot of notifications We can just add a lint rule for this

[manupak]

@leandron @gromero
Just a final remark : dont we need a tracking issue to track actual landing of the PR that adds pull_request.rst to docs ?

[leandron]

@leandron @gromero
Just a final remark : dont we need a tracking issue to track actual landing of the PR that adds pull_request.rst to docs ?

We do, I think we can create that once voting is finished.

[manupak]

Ah ok, just noticing that it was missing in the RFC header

[Lunderberg]

Rendered link

[areusch]

The vote has passed, therefore I'll merge this RFC. @gromero please create a tracking issue (you can create another PR to add it to this doc).

[gromero]

@areusch @driazati @tqchen @tkonolige @manupak @Lunderberg @mbaret @junrushao @Hzfengsy @slyubomirsky @ekalda thanks a lot for the reviews and support of this RFC!

Tracking issue is created at: apache/tvm#12690

Cheers.