Thanks Robert and Udi for links. Perhaps we should integrate such check if there are no objections from community. I’ll take a look on this more closely.
> On 4 Aug 2020, at 19:31, Udi Meiri <[email protected]> wrote: > > https://github.com/marketplace/actions/gs-commit-message-checker > <https://github.com/marketplace/actions/gs-commit-message-checker> > > On Tue, Aug 4, 2020 at 10:25 AM Robert Bradshaw <[email protected] > <mailto:[email protected]>> wrote: > +1, thanks for the reminder. > > This should be really easy to automate, using > https://developer.github.com/webhooks/event-payloads/#pull_request > <https://developer.github.com/webhooks/event-payloads/#pull_request> to > give a warning when the change history is not sufficiently "clean." > I'm not sure where to host this though (or if it could be integrated > into jenkins--basically I'd just want to run a Python script with the > PR number (or better, just point to the local git repo and have the > master's commit handy) as another precommit). > > On Tue, Aug 4, 2020 at 10:10 AM Rui Wang <[email protected] > <mailto:[email protected]>> wrote: > > > > +1 thanks Alexey. > > > > My apologies that I merged such a case recently (but not intentionally). I > > tried to use the "squash and merge" button with a consolidated commit > > message. After clicking the button, github showed "failed to merge" and > > gave a retry button, and after clicking that retry button, github magically > > switched to "create merge commit" approach thus merged some fixup commits > > to the main branch. > > > > This is a rare case (I only encountered once). But I will pay more > > attention next time. I could ask PR authors to squash their commits before > > merging when it is possible. > > > > > > -Rui > > > > On Tue, Aug 4, 2020 at 9:40 AM Alexey Romanenko <[email protected] > > <mailto:[email protected]>> wrote: > >> > >> Yes, good point, thanks Valentyn. > >> > >> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <[email protected] > >> <mailto:[email protected]>> wrote: > >> > >> +1, thanks, Alexey. > >> > >> Also a reminder from the contributor guide: do not use the default GitHub > >> commit message for merge commits, which looks like: > >> > >> Merge pull request #1234 from some_user/transient_branch_name > >> > >> Instead, add the commit message into the subject line, for example: "Merge > >> pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle". > >> > >> On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko <[email protected] > >> <mailto:[email protected]>> wrote: > >>> > >>> Hi all, > >>> > >>> I’d like to attract your attention regarding our Git commit history and > >>> related issue. A while ago I noticed that it started getting not very > >>> clear and quite verbose comparing to how it was before. We have quite > >>> significant amount of recent commits like “fix”, “address comments”, > >>> “typo”, “spotless”, etc. Most of them also doesn’t contain Jira Tag as a > >>> prefix and actually is just supplementary commits to “main” and initial > >>> commit of PR, added after several PRs review rounds. > >>> > >>> AFAIR, we already had several discussion in the past about this topic and > >>> we agreed that we should avoid such commits in a final merge and have > >>> only one (in most cases) or several (if necessary) logical commits that > >>> should be atomic and properly explain what they do. > >>> > >>> Why these “tiny" commits are bad practice? Just several main reasons: > >>> - They pollute our git repository history and don’t give any additional > >>> and useful further information; > >>> - They are not atomic and we can’t easily revert (rollback) this > >>> supplementary commit since the state of the build before was likely > >>> broken or had incorrect behaviour. So, in this case, the whole set of PRs > >>> commits should be reverted which is not convenient and error-prone. It’s > >>> also expected that all checks were green before merging a PR (take a part > >>> flaky tests). > >>> - They are not informative in terms of commit message. So it makes more > >>> hard to identify Git annotated code and how the lines of code are related > >>> together. > >>> > >>> Following this, I just want to briefly remind our Committers rules > >>> regarding PR merging [1]. > >>> Every commit: > >>> - should do one thing and reflect it in commit message; > >>> - should contain Jira Tag; > >>> - all “fixup” and “address comments” type of commits should be squashed > >>> by author or committer before merging. > >>> > >>> Please, pay attention on what is finally committed and merged into our > >>> repository and it should help to keep our commit history clear, which > >>> will be transferred to saving a time of other developers in the end. > >>> > >>> [1] https://beam.apache.org/contribute/committer-guide/#finishing-touches > >>> <https://beam.apache.org/contribute/committer-guide/#finishing-touches> > >>> > >>> Regards, > >>> Alexey > >> > >>
