-1 We should stick with always using squash. It maintains PR reference in commit history. It is also common practice.
If you want to included commits from multiple contributors in a single PR, open a branch and make PRs to that branch. Then only use rebase when merging that branch to master. Thanks, Eric On 2018/07/12 23:38:54, Marco de Abreu <marco.g.ab...@googlemail.com.INVALID> wrote: > Excellent :) > > I don't think we need a formal vote for this but rather let this be lazy > consensus if nobody minds. > > Could we maybe revisit this decision in 1 or 2 months and then assess the > state of commit history in all PRs (including squashed ones) and with focus > on rebase merged PRs? > > -Marco > > Naveen Swamy <mnnav...@gmail.com> schrieb am Fr., 13. Juli 2018, 01:33: > > > You are right, its already enabled :) I saw that option greyed out for one > > of the PR(for a different reason) and assumed we need INFRA to enable. > > > > On Thu, Jul 12, 2018 at 4:22 PM, Marco de Abreu < > > marco.g.ab...@googlemail.com.invalid> wrote: > > > > > Coukd you elaborate why we would need a ticket for that? GitHub supports > > it > > > out of the box and it is enabled as far as I can tell. You just have to > > > press the little arrow besides the merge button. > > > > > > -marco > > > > > > Naveen Swamy <mnnav...@gmail.com> schrieb am Fr., 13. Juli 2018, 00:54: > > > > > > > Yes to insist on commit hygiene when rebase-merge. We have to open a > > JIRA > > > > with Apache Infra to enable rebase-merge on the repo. > > > > > > > > On Thu, Jul 12, 2018 at 3:21 PM, Marco de Abreu < > > > > marco.g.ab...@googlemail.com.invalid> wrote: > > > > > > > > > Fully agree, if we can get our commit hygiene up to industry > > standard, > > > I > > > > > don't see any problems in using rebase merge instead. For the short > > > term > > > > I > > > > > agree that it should be fine to rebase merge individual PRs with > > > multiple > > > > > contributors. But in my opinion we should then insist on people > > > amending > > > > > their commit history if we deem it below our standard. A PR should > > not > > > be > > > > > rebase merged if the history is not proper - verifying that is the > > > > > responsibility of the merging committer, but ideally, we'd make > > people > > > > > aware of problems early on. What do you think? > > > > > > > > > > In general, I think we should gradually start taking this into > > account > > > > when > > > > > reviewing with the goal of all PRs having a proper commit history. > > This > > > > > would allow us in the long term to completely move away from > > squashing. > > > > > > > > > > -Marco > > > > > > > > > > Pedro Larroy <pedro.larroy.li...@gmail.com> schrieb am Fr., 13. Juli > > > > 2018, > > > > > 00:07: > > > > > > > > > > > This is a great discussion, thanks for raising the question Naveen. > > > > > > > > > > > > From my experience working in all kinds of software project of > > > varying > > > > > > sizes and flavours: > > > > > > > > > > > > 1. People should be aware of git rebase interactive and have > > more > > > > > > hygiene in their PRs. If a PR is hygienic and is separated in a > > > set > > > > of > > > > > > semantic commits, it's better not squashed as it helps finding > > > bugs > > > > / > > > > > > bisecting. A "dirty" PR with WIP commits, is better squashed, or > > > > > > requested > > > > > > to rebase interactively on CR. > > > > > > 2. Mixing different semantic changes on a single PR is an > > > > > anti-pattern, > > > > > > for example fixing whitespace or reformatting many lines and > > > > changing > > > > > > some > > > > > > code which is hidden in a bunch of trivial changes and could > > > cause a > > > > > > bug or > > > > > > major change of behaviour. > > > > > > 3. Agreed what with others have mentioned about small > > incremental > > > > > steps > > > > > > better than huge PRs. We also have JIRA or issues to relate a > > set > > > of > > > > > PRs > > > > > > together. If not possible, PR with a set of non squashed commits > > > is > > > > > also > > > > > > good. > > > > > > > > > > > > Hope it helps. > > > > > > > > > > > > Pedro. > > > > > > > > > > > > On Thu, Jul 12, 2018 at 11:47 PM Naveen Swamy <mnnav...@gmail.com> > > > > > wrote: > > > > > > > > > > > > > @Aaron, I do not think most contributors(SDE or not) were even > > > aware > > > > > that > > > > > > > their commits are getting squashed into one and thereby others > > > losing > > > > > > > credit on that work. I would assume no bad intentions there. > > > > > > > > > > > > > > @Hao, > > > > > > > Agree to breaking down into small and reasonable sized PRs, but I > > > > think > > > > > > > very small PRs will overwhelm the CI as it stands since it runs > > > > > > > everything(this is a separate topic and needs fixing). > > > > > > > > > > > > > > For cases similar to Aaron's he can raise the PR for the doc > > > > > > > work(regardless of fork or not) and add other contributors as > > > > > co-authors. > > > > > > > > > > > > > > @Marco, co-author might not be suitable always suitable and agree > > > > with > > > > > > Hao > > > > > > > that should be used on exceptions. co-author also makes it hard > > to > > > > see > > > > > > the > > > > > > > contributions individually. > > > > > > > > > > > > > > @Marco, why can we not have Rebase merge enabled on the repo and > > > use > > > > > that > > > > > > > when there are multiple contributors. This discussion is only > > about > > > > Not > > > > > > > Squashing when there are multiple contributors and agree to > > > continue > > > > > the > > > > > > > practice of Squashing in general. > > > > > > > > > > > > > > Until the tooling is fixed, I suggest to use the co-author > > feature > > > > when > > > > > > > collaborating on a fork. > > > > > > > > > > > > > > Also, I just want to reiterate and request contributors to have > > > > > > meaningful > > > > > > > and fewer commits on PRs. > > > > > > > > > > > > > > Thanks, Naveen > > > > > > > > > > > > > > > > > > > > > On Thu, Jul 12, 2018 at 11:40 AM, Marco de Abreu < > > > > > > > marco.g.ab...@googlemail.com.invalid> wrote: > > > > > > > > > > > > > > > As of now it will require the usage of a merge bot as far as I > > > > > > understand > > > > > > > > this issue: https://github.com/python/miss-islington/issues/16 > > . > > > > > > Instead > > > > > > > of > > > > > > > > using the web interface do merge, we'd then trigger the bot to > > do > > > > the > > > > > > > merge > > > > > > > > on our behalf. There are pre-made solutions on the internet we > > > > might > > > > > be > > > > > > > > able to leverage, but it would take some time to get into it > > and > > > > > adapt > > > > > > > our > > > > > > > > process. > > > > > > > > > > > > > > > > Additionally, GitHub has this feature request tracked > > internally. > > > > > Let's > > > > > > > see > > > > > > > > when they get to add it. > > > > > > > > > > > > > > > > -Marco > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Aaron Markham <aaron.s.mark...@gmail.com> schrieb am Do., 12. > > > Juli > > > > > > 2018, > > > > > > > > 21:33: > > > > > > > > > > > > > > > > > My point was about collaboration, or the lack thereof, and > > how > > > > the > > > > > > > > tooling > > > > > > > > > and apparent rewards from contribution statistics can > > reinforce > > > > > lone > > > > > > > > ranger > > > > > > > > > behavior. People can and should be proud of their work, but > > why > > > > > does > > > > > > it > > > > > > > > > have to be alone? Working within the context of a team is > > > > something > > > > > > to > > > > > > > be > > > > > > > > > proud of too, but if your stats and standing are graded by > > how > > > > the > > > > > > > > commits > > > > > > > > > and merges land, and counting lines of code, then incentives > > of > > > > the > > > > > > > > system > > > > > > > > > are skewed. > > > > > > > > > How do we go about implementing the co-author process? It > > > sounds > > > > > like > > > > > > > > > something worth doing! > > > > > > > > > > > > > > > > > > On Thu, Jul 12, 2018 at 11:15 AM, Hao Jin < > > hjjn.a...@gmail.com > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Aaron, > > > > > > > > > > To be fair, this discussion has nothing to do with "pride" > > of > > > > > SDEs, > > > > > > > but > > > > > > > > > > rather a discussion on what is a better software > > engineering > > > > > > practice > > > > > > > > for > > > > > > > > > > the project. Breaking a large project into smaller tasks > > is a > > > > > good > > > > > > > > > software > > > > > > > > > > engineering practice. This article(https://arxiv.org/pdf/ > > > > > > > > 1702.01715.pdf) > > > > > > > > > > on > > > > > > > > > > Google's software engineering practice says: "Engineers are > > > > > > > encouraged > > > > > > > > to > > > > > > > > > > keep each individual change small​, with larger changes > > > > > preferably > > > > > > > > broken > > > > > > > > > > into a series of smaller changes that a reviewer can easily > > > > > review > > > > > > in > > > > > > > > one > > > > > > > > > > go.". If you have concerns or your comments on this > > practice, > > > > we > > > > > > can > > > > > > > > take > > > > > > > > > > the discussion offline. On the other hand, an important > > > spirit > > > > of > > > > > > > > Apache > > > > > > > > > > community is that "one must interact with others, and share > > > > > vision > > > > > > > and > > > > > > > > > > knowledge"(https://community.apache.org/contributors/), if > > > you > > > > > > > > observed > > > > > > > > > > that a majority of the contributors have serious problems > > > with > > > > > > their > > > > > > > > > > writing maybe you can share some tips and hints on how to > > > write > > > > > > > better > > > > > > > > > > documentations, in that way not only a lot of us within the > > > > > > community > > > > > > > > can > > > > > > > > > > have some growth but also you can save some time for > > writing > > > > more > > > > > > > good > > > > > > > > > > documentations and blogposts for MXNet, don't you think so? > > > Or, > > > > > if > > > > > > > you > > > > > > > > > only > > > > > > > > > > have to fix the doc for someone once in a while, I > > definitely > > > > > agree > > > > > > > > that > > > > > > > > > > you should be given the credit for that, and that's where > > the > > > > > > > co-author > > > > > > > > > > field can be useful, which was exactly what I was saying in > > > my > > > > > > > previous > > > > > > > > > > email. Thanks! > > > > > > > > > > Hao > > > > > > > > > > > > > > > > > > > > On Thu, Jul 12, 2018 at 8:16 AM, Aaron Markham < > > > > > > > > > aaron.s.mark...@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > This is a great discussion, and close to my heart, > > > because I > > > > > > want > > > > > > > to > > > > > > > > > > > include more writers and editors in our community, and > > I'm > > > > > > > struggling > > > > > > > > > to > > > > > > > > > > > see how to best manage this. It seems like being the sole > > > > > > > contributor > > > > > > > > > on > > > > > > > > > > a > > > > > > > > > > > feature is like an engineer's Lone Ranger badge of > > pride! I > > > > > feel > > > > > > > like > > > > > > > > > it > > > > > > > > > > > should be a red flag. > > > > > > > > > > > > > > > > > > > > > > I work in collaboration with dozens of engineers to > > improve > > > > > their > > > > > > > > > > > documentation, explain their features, change flows to > > > > improve > > > > > > user > > > > > > > > > > > experience, and sometimes fix bugs and write code. When > > the > > > > > PR's > > > > > > > docs > > > > > > > > > has > > > > > > > > > > > great coverage, is clear, and not loaded with bad grammar > > > and > > > > > > > > spelling > > > > > > > > > > > mistakes, I will put comments in a review. But sometimes > > > > there > > > > > > > needs > > > > > > > > to > > > > > > > > > > be > > > > > > > > > > > a complete rework such that hundreds of review comments > > > isn't > > > > > > > > feasible, > > > > > > > > > > and > > > > > > > > > > > they can't be properly addressed. In a lot of these > > cases, > > > I > > > > > > commit > > > > > > > > my > > > > > > > > > > > changes to someone else's fork. Now I know the people I > > > work > > > > > with > > > > > > > on > > > > > > > > > > their > > > > > > > > > > > PRs appreciate my help, but when all of this work gets > > > > squashed > > > > > > > down > > > > > > > > to > > > > > > > > > > one > > > > > > > > > > > commit, I'm pretty much regularly getting squashed out. > > I'm > > > > not > > > > > > > sure > > > > > > > > > who > > > > > > > > > > > else is in this boat, but does the community recognize > > our > > > > > > > > > contributions > > > > > > > > > > > when this is the general mode of operation? > > > > > > > > > > > > > > > > > > > > > > Regarding co-author - I'm not sure how people would feel > > > > about > > > > > me > > > > > > > > > being a > > > > > > > > > > > co-author on their work. Documentation and clarity in > > your > > > > work > > > > > > > > product > > > > > > > > > > is > > > > > > > > > > > important, but people's views on their personal *code* > > > > > > contribution > > > > > > > > > seems > > > > > > > > > > > to be more important than the overall code & feature > > > quality > > > > > > itself > > > > > > > > > when > > > > > > > > > > > docs are part of the package. I feel that if I do > > follow-on > > > > PRs > > > > > > > > instead > > > > > > > > > > of > > > > > > > > > > > fixing things before they are merged, that we would be > > > > > releasing > > > > > > > > > > incorrect, > > > > > > > > > > > incomplete, and inferior product. But, in absence of a > > > better > > > > > > > > solution, > > > > > > > > > > > maybe that's the pill we have to swallow. > > > > > > > > > > > > > > > > > > > > > > -1 on lots of small PRs (until we expand our range of > > > > > committers > > > > > > > and > > > > > > > > > > reduce > > > > > > > > > > > the latency in reviews and merges). > > > > > > > > > > > +1 on improving the quality of commit messages, so we > > don't > > > > > have > > > > > > to > > > > > > > > > > squash > > > > > > > > > > > & merge all of the time. > > > > > > > > > > > +1 on improving the practice of better commit & merge > > > > > management > > > > > > so > > > > > > > > > that > > > > > > > > > > > the commit history is concise and meaningful. (I'm guilty > > > of > > > > > > this, > > > > > > > > and > > > > > > > > > > > certainly need to improve here.) > > > > > > > > > > > +1 on co-author - assuming that's what most everyone > > thinks > > > > is > > > > > a > > > > > > > good > > > > > > > > > > > solution for now. I'm unclear on how this gets managed > > > > though. > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > Aaron > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jul 12, 2018 at 5:19 AM, Anton Chernov < > > > > > > > mecher...@gmail.com> > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Unfortunately there has been significant push back for > > > > small > > > > > > > > > iterative > > > > > > > > > > > PR's > > > > > > > > > > > > and as a result they have grown substantially involving > > > > > > multiple > > > > > > > > > > > > contributors, multiple commits, sometimes multiple > > > branches > > > > > to > > > > > > be > > > > > > > > > > worked > > > > > > > > > > > > on. > > > > > > > > > > > > > > > > > > > > > > > > I fully agree and support all points that Jin raised: > > > > > > > > > > > > > > > > > > > > > > > > 1) Most contributions should be broken down into > > smallest > > > > > > > possible > > > > > > > > > > PR's. > > > > > > > > > > > > 2) If a PR is small enough a single person can complete > > > it. > > > > > > > > > > > > 3) If a majority of PR is done by someone, and there's > > > some > > > > > > minor > > > > > > > > > issue > > > > > > > > > > > > he/she needs help with it could be done in a follow up > > PR > > > > by > > > > > > > > anybody, > > > > > > > > > > > > including the reviewer. > > > > > > [message truncated...]
