+1 to automating this! On Wed, Aug 5, 2020 at 8:38 AM Alexey Romanenko <[email protected]> wrote:
> 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 > > On Tue, Aug 4, 2020 at 10:25 AM Robert Bradshaw <[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 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]> 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]> wrote: >> >> >> >> Yes, good point, thanks Valentyn. >> >> >> >> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <[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]> 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 >> >>> >> >>> Regards, >> >>> Alexey >> >> >> >> >> > >
