I'd argue that the history is almost always "most useful" when one PR == one commit on master. Intermediate commits from a PR may be useful to aid code review, but they're not verified by presubmits and thus aren't necessarily independently revertible, so I see little value in keeping them around on master. In fact if you're breaking up a PR into multiple commits to aid code review, it's worth considering if they could/should be separately reviewed and verified PRs. We could solve the unwanted commit issue if we have a policy to always "Squash and Merge" PRs with rare exceptions.
I agree jira/PR titles could be better, I'm not sure what we can do about it aside from reminding committers of this responsibility. Perhaps the triage process can help catch poorly titled jiras? On Wed, Apr 21, 2021 at 11:38 AM Robert Bradshaw <rober...@google.com> wrote: > +1 to better descriptions for JIRA (and PRs). Thanks for bringing this up. > > For merging unwanted commits, can we automate a simple check (e.g. with > github actions)? > > On Wed, Apr 21, 2021 at 8:00 AM Tomo Suzuki <suzt...@google.com> wrote: > >> BEAM-12173 is on me. I'm sorry about that. Re-reading committer guide >> [1], I see I was not following this >> >> > The reviewer should give the LGTM and then request that the author of >> the pull request rebase, squash, split, etc, the commits, so that the >> history is most useful >> >> >> Thank you for the feedback on this matter! (And I don't think we >> should change the contribution guide) >> >> [1] https://beam.apache.org/contribute/committer-guide/ >> >> On Wed, Apr 21, 2021 at 10:35 AM Ismaël Mejía <ieme...@gmail.com> wrote: >> > >> > Hello, >> > >> > I have noticed an ongoing pattern of carelessness around issues/PR >> titles and >> > descriptions. It is really painful to see more and more examples like: >> > >> > BEAM-12160 Add TODO for fixing the warning >> > BEAM-12165 Fix ParquetIO >> > BEAM-12173 avoid intermediate conversion (PR) and BEAM-12173 use >> > toMinutes (commit) >> > >> > In all these cases with just a bit of detail in the title it would be >> enough to >> > make other contributors or reviewers life easierm as well as to have a >> better >> > project history. What astonishes me apart of the lack of care is that >> some of >> > those are from Beam commmitters. >> > >> > We already have discussed about not paying attention during commit >> merges where >> > some PRs end up merging tons of 'unwanted' fixup commits, and nothing >> has >> > changed so I am wondering if we should maybe just totally remove that >> rule (for >> > commits) and also eventually for titles and descriptions. >> > >> > Ismaël >> > >> > [1] https://beam.apache.org/contribute/ >> >> >> >> -- >> Regards, >> Tomo >> >
