+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 >> > >
