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).

Reply via email to