You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tvm.apache.org by Gustavo Romero <no...@github.com.INVALID> on 2022/08/15 15:55:36 UTC

[apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

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 &lt;leandro.nunes@arm.com&gt;
You can view, comment on, or merge this pull request online at:

  https://github.com/apache/tvm-rfcs/pull/88

-- Commit Summary --

  * [RFC] Add Commit Message Guideline

-- File Changes --

    A rfcs/0090-commit-message-guideline.md (181)

-- Patch Links --

https://github.com/apache/tvm-rfcs/pull/88.patch
https://github.com/apache/tvm-rfcs/pull/88.diff

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88
You are receiving this because you are subscribed to this thread.

Message ID: &lt;apache/tvm-rfcs/pull/88@github.com&gt;

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Leandro Nunes <no...@github.com.INVALID>.
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.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1222334914
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Leandro Nunes <no...@github.com.INVALID>.
> @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.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1227607913
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Andrew Reusch <no...@github.com.INVALID>.
@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.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1220078234
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Gustavo Romero <no...@github.com.INVALID>.
@areusch @leandron Could you please help me on how to trigger a [VOTE] thread on this RFC? Cheers.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1215261260
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Tristan Konolige <no...@github.com.INVALID>.
@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.


-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1217226784
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Leandro Nunes <no...@github.com.INVALID>.
@areusch can you have a final look and merge if you're happy, before creating the `[VOTE]` thread you mentioned?

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1224165355
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by driazati <no...@github.com.INVALID>.
> @driazati notes that GitHub [just introduced](https://github.blog/changelog/2022-08-23-new-options-for-controlling-the-default-commit-message-when-merging-a-pull-request/) 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 


-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1226673013
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Andrew Reusch <no...@github.com.INVALID>.
I created a [vote thread](https://github.com/apache/tvm/issues/12583) to broadcast this more broadly to the community. Please signal your support there. 

@driazati notes that GitHub [just introduced](https://github.blog/changelog/2022-08-23-new-options-for-controlling-the-default-commit-message-when-merging-a-pull-request/) 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.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1226442023
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Gustavo Romero <no...@github.com.INVALID>.
@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: https://github.com/apache/tvm/issues/12690

Cheers.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1235612896
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Gustavo Romero <no...@github.com.INVALID>.
> @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 keep 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? 


-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1217357505
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by driazati <no...@github.com.INVALID>.
> 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.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1218337517
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Gustavo Romero <no...@github.com.INVALID>.
@tqchen @jroesch @driazati @areusch @Mousius @manupak @tkonolige @mbaret @leandron @alanmacd 

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1215257266
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Andrew Reusch <no...@github.com.INVALID>.
Merged #88 into main.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#event-7300385635
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Manupa Karunaratne <no...@github.com.INVALID>.
Ah ok, just noticing that it was missing in the RFC header

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1227649858
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Leandro Nunes <no...@github.com.INVALID>.
> > 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 same, 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.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1217692311
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Eric Lunderberg <no...@github.com.INVALID>.
[Rendered link](https://github.com/gromero/tvm-rfcs/blob/cmg/rfcs/0088-commit-message-guideline.md)

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1233170752
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Andrew Reusch <no...@github.com.INVALID>.
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).

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1233535253
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Andrew Reusch <no...@github.com.INVALID>.
@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.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1215326331
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Manupa Karunaratne <no...@github.com.INVALID>.
Just a final remark : dont we need a tracking issue to track actual landing of the PR that adds pull_request.rst to docs ?

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1227600044
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Gustavo Romero <no...@github.com.INVALID>.
OK, I agree @leandron @driazati Thanks.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1218386487
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by driazati <no...@github.com.INVALID>.
> i would love for tvm-bot to enforce this

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

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1220859744
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Add Commit Message Guideline (PR #88)

Posted by Gustavo Romero <no...@github.com.INVALID>.
> 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: https://github.com/apache/tvm-rfcs/pull/88/commits/99c3bc2247050eb1d8ef3ed6ab665d455cfd4c1a#diff-759e47e77c87b8e4c14dba43c17bac829cbb502e9369211c1ad751253dae57f5R126 but merely **a recommendation**, moreover now we have to consider also the comment from Matthew about multiple in a PR:
https://github.com/apache/tvm-rfcs/pull/88#discussion_r946033889

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:

https://github.com/apache/tvm-rfcs/commit/99c3bc2247050eb1d8ef3ed6ab665d455cfd4c1a#diff-759e47e77c87b8e4c14dba43c17bac829cbb502e9369211c1ad751253dae57f5R46-R50

and then in the end here:

https://github.com/apache/tvm-rfcs/commit/99c3bc2247050eb1d8ef3ed6ab665d455cfd4c1a#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:

https://github.com/apache/tvm-rfcs/commit/99c3bc2247050eb1d8ef3ed6ab665d455cfd4c1a#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.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/88#issuecomment-1215988221
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>