Hmm, this is strange if it was “mergeable” before, never had this. Thanks Rui!
> On 4 Aug 2020, at 19:09, 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] > <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 >
