Etienne, thanks for bringing this topic. I think it was already discussed several times before and we have finally came to what we have in the current Committer guide “Granularity of changes" [1].
Personally, I completely agree with these 4 rules presented there. The main concern is that all committers should follow them as well, otherwise we still have sometimes a bunch of small commits with inexpressive messages (I believe they were added during review process and were not squashed before merging). In my opinion, the most important rule is that every commit should be atomic in terms of added/fixed functionality and rolling it back should not break master branch. [1] https://beam.apache.org/contribute/committer-guide/#pull-request-review-objectives > On 22 Mar 2019, at 10:16, Etienne Chauchot <echauc...@apache.org> wrote: > > Hi all, > It has already been discussed partially but I would like that we agree on the > commit granularity that we want in our history. > Some features were squashed to only one commit which seems a bit too granular > to me for a big feature. > On the contrary I see PRs with very small commits such as "apply spotless" or > "fix checkstyle". > > IMHO I think a good commit size is an isolable portion of a feature such as > for ex "implement Read part of Kudu IO" or "reduce concurrency in Test A". > Such a granularity allows to isolate problems easily (git bisect for ex) and > rollback only a part if necessary. > WDYT about: > - squashing non meaningful commits such as "apply review comments" (and > rather state what they do and group them if needed), or "apply spotless" or > "fix checkstyle" > - trying to stick to a commit size as described above > > => and of course update the contribution guide at the end > ? > > Best > Etienne
