I agree with keeping history clean. Although, Small commits like address PR comments are useful during review process. They allow reviewer to see only new changes, not review whole diff again. Best to squash then before/on merge though.
On Fri, Mar 22, 2019, 07:34 Ismaël Mejía <ieme...@gmail.com> wrote: > > I like the extra delimitation the brackets give, worth the two extra > > characters to me. More importantly, it's nice to have consistency, and > > the only way to be consistent with the past is leave them there. > > My point with the brackets is that we are 'getting close' to 10K issue > so we will then have 3 chars less, probably it does not change much > but still. > > On Fri, Mar 22, 2019 at 3:19 PM Robert Bradshaw <rober...@google.com> > wrote: > > > > On Fri, Mar 22, 2019 at 3:02 PM Ismaël Mejía <ieme...@gmail.com> wrote: > > > > > > It is good to remind committers of their responsability on the > > > 'cleanliness' of the merged code. Github sadly does not have an easy > > > interface to do this and this should be done manually in many cases, > > > sadly I have seen many committers just merging code with multiple > > > 'fixup' style commits by clicking Github's merge button. Maybe it is > > > time to find a way to automatically detect these cases and disallow > > > the merge or maybe we should reconsider the policy altogether if they > > > are people who don't see the value of this. > > > > I agree about keeping our history clean and useful, and think those > > four points summarize things well (but a clarification on fixup > > commits would be good). > > > > +1 to an automated check that there are many extraneous commits. > > Anything the person hitting the merge button would easily see before > > doing the merge. > > > > > I would like to propose a small modification to the commit title style > > > on that guide. We use two brackets to enclose the issue id, but that > > > really does not improve much the readibility and uses 2 extra spaces > > > of the already short space title, what about getting rid of them? > > > > > > Current style: "[BEAM-XXXX] Commit title" > > > Proposed style: "BEAM-XXXX Commit title" > > > > > > Any ideas or opinons pro/con ? > > > > I like the extra delimitation the brackets give, worth the two extra > > characters to me. More importantly, it's nice to have consistency, and > > the only way to be consistent with the past is leave them there. > > > > > On Fri, Mar 22, 2019 at 2:32 PM Etienne Chauchot <echauc...@apache.org> > wrote: > > > > > > > > Thanks Alexey to point this out. I did not know about these 4 points > in the guide. I agree with them also. I would just add "Avoid keeping in > history formatting messages such as checktyle or spotless fixes" > > > > If it is ok, I'll submit a PR to add this point. > > > > Le vendredi 22 mars 2019 à 11:33 +0100, Alexey Romanenko a écrit : > > > > > > > > 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 > > > > > > > > >
