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 via Apache TVM Discuss <no...@discuss.tvm.ai> on 2022/03/17 19:56:18 UTC

[Apache TVM Discuss] [Development/pre-RFC] Commit Message Guideline

Hi Tristan. Good question ;)

In a sense that issue could be put into the class of the issues I mentioned as "Github-specif issues", because to me ideally such a comments should never exist in the first place. They exist in my understanding primarily because we want to keep the PR conversations (specially regarding the review process) and a git push --force, which is necessary if we amend the initial commits instead of adding a new commit, will be necessary and that will lead to the conversations vanishing sometime after the git push --force (that's because of `git gc`).

OTOH overt time I did realize that is not always the case that contributors void push force and eventually some will do it. BTW, we don't have any rule / guideline to encourage (or dis-encourage it). Let me know if besides keeping the conversations in the PR (which is ultimately a GH limitation I think) there are other advantages on not force pushing in the review process we currently use on GH.

Hence although I do recognize that such additional commits squashed and merged into the final commit tremendously pollute the final commit (how about the CI re-triggers which are so common heh) I don't have any suggestion for now on how to get rid of them. There options like the maintainer working out the final commit before merge to get rid of them (I think Tqchen suggested it once if I'm not mistaken), but that also can add a burden to the review process, so... I don't know. We need to discuss more about it.

Hence, assuming we'll keep the additional commits (commits that go after the initial/main ones), I think that for simple things like a CI retrigger and lint fixes a simple trivial message is ok. I think that enforcing more than that will be tedious for the reviewers and the authors.

However it might be the case that the initial commit message says something like "The buffer size here is set to X because ... " and after one review it becomes clear that it should be larger. In cases like that I think that the additional commit message addressing the comment from the reviewer should mention why X doesn't hold anymore and now Y is being used instead (If git push "allowed" then the initial commit message could be amend in the very first place).

So that's my suggestion: add a guideline saying that non-trivial additional commit messages should try to capture the essence of the additional changes taking into account what/how it affects the initial commits.

What do you think?

---
[Visit Topic](https://discuss.tvm.apache.org/t/commit-message-guideline/12334/6) to respond.

You are receiving this because you enabled mailing list mode.

To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/37aecf13c9acb5905e7ef63f25fd76c053621b38e81da2a7ee4836db5c0a6131).

[Apache TVM Discuss] [Development/pre-RFC] Commit Message Guideline

Posted by "Leandro Nunes (Arm) via Apache TVM Discuss" <no...@discuss.tvm.ai>.

Thanks @gromero for starting this RFC! I think sorting out commit messages will have a very good impact down the line in organising the project as a whole.

Adding automation to check/enforce this is a great and I support it very much, but I think the important thing at project level as a result of this RFC is that during review we can point authors of PRs to those guidelines so that we can politely ask for better commit messages.

[quote="tkonolige, post:10, topic:12334"]
I think some people prefer that new changes to a PR come as new commits so that they can review them on a per-commit basis instead of having to re-review the entire PR.
[/quote]

I think there is a balance here to accommodate "_ways people like to organise their own work_" with the minimum we - as an open source project - need to guarantee some uniformity and quality to the contributions we merge. This is what this RFC is trying to achieve IMHO.

On the topic of "stacking new commits on top of a PR", I feel it is important that such commits are organised in a way that they mean something, so that they can have a commit message that means something more than e.g. "fix lint", "fix test" or "cleanup". These changes should be merged into their relevant commits within a PR.

In terms of how to concretely implement the changes, I think we should make this part of the documentation, near the committer/reviewer guidelines, similar to what other projects e.g. [LLVM](https://llvm.org/docs/DeveloperPolicy.html#commit-messages) do.

[quote="manupa-arm, post:11, topic:12334"]
With that spirit, I would like to suggest something, though I admit I have no experience in using this (hoping someone here might have used this :slight_smile: ) , [Gitlint ](https://jorisroovers.com/gitlint/). I think we can enable this in the CI, fairly easily depending on the collective opinion of the community.
[/quote]

I think this is a great suggestion @manupa-arm, so that we can codify the bare minimum and save a lot of reviewer's time in pointing out empty/not tagged commit messages.

---
[Visit Topic](https://discuss.tvm.apache.org/t/commit-message-guideline/12334/13) to respond.

You are receiving this because you enabled mailing list mode.

To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/eb6e95c9676a520783306efbf4bbeb25d04f87ab6cd912399c92fd1075ea46d5).

[Apache TVM Discuss] [Development/pre-RFC] Commit Message Guideline

Posted by Manupa Karunaratne via Apache TVM Discuss <no...@discuss.tvm.ai>.

Thanks @gromero for taking this initiative.

I would actually push us to take a pragmatic route to enforce these (kind of agreeing @driazati ) given the distributed nature of the TVM/OSS project, failing that we fallback to being at least a "guideline" -- which we dont have at the minute :) .

I've been spending sometime thinking about this how to automate this (like in a way a linter would perform) -- In the past, we have seen majority of review cycles are spent fixing commas, spaces, etc -- which got fixed using a linter in the CI. (For more info : https://discuss.tvm.apache.org/t/ci-lint-enabling-clang-format-based-lint-checks/6170)

With that spirit, I would like to suggest something, though I admit I have no experience in using this (hoping someone here might have used this :) ) , https://jorisroovers.com/gitlint/. I think we can enable this in the CI, fairly easily depending on the collective opinion of the community.

I would like to second this. I personally worked on PRs with varying complexities and it is not practical to assume a PR to *always* contain a single commit. However, one thing we might use to solve this would be use "fixup!" or "squash!" commits that the above mentioned tool (Gitlint) can ignore and could be easily removed from the commit message using --autosquash. One thing I dont know is whether/how we can configure Github to use --autosquash.

Assuming if we can find out a way to use --autosquash with Github, we might be able to enable linter for commit messages, because the above tool can be made to ignore fixup! or squash! commits.

(NOTE : above recommendation of the tool is just based on some short research I did, thus, open for any other alternatives)

---
[Visit Topic](https://discuss.tvm.apache.org/t/commit-message-guideline/12334/11) to respond.

You are receiving this because you enabled mailing list mode.

To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/7de7f6e893443408faf4db2dbe159d16b0fe6dd32f7c8a1738635994ce0f0162).

[Apache TVM Discuss] [Development/pre-RFC] Commit Message Guideline

Posted by Tristan Konolige via Apache TVM Discuss <no...@discuss.tvm.ai>.


> OTOH overt time I did realize that is not always the case that contributors void push force and eventually some will do it. BTW, we don’t have any rule / guideline to encourage (or dis-encourage it). Let me know if besides keeping the conversations in the PR (which is ultimately a GH limitation I think) there are other advantages on not force pushing in the review process we currently use on GH.

I think some people prefer that new changes to a PR come as new commits so that they can review them on a per-commit basis instead of having to re-review the entire PR.

> So that’s my suggestion: add a guideline saying that non-trivial additional commit messages should try to capture the essence of the additional changes taking into account what/how it affects the initial commits.

I think this is a reasonable thing to ask for, but I'm not sure when/how these additional changes should be merged into the final commit message.





---
[Visit Topic](https://discuss.tvm.apache.org/t/commit-message-guideline/12334/10) to respond.

You are receiving this because you enabled mailing list mode.

To unsubscribe from these emails, [click here](https://discuss.tvm.apache.org/email/unsubscribe/6e7649f7312c7eff388673e928902a6aa852b0b1f128187bdd9183fb46c45605).