@naveen - Hanlon's razor applies, so I am not concerned with any individual's action or inaction here. Most people don't even realize its happening (I didn't until recently), and the common practice doesn't manage collaboration well at all. Let's fix that, and turn a less-than-ideal common practice into a best practice.
@eric - We talked about several things in this thread, and your -1 seems to have stymied the dialogue. So I'm going to try to clarify your vote and your proposal to get the discussion going again. Your vote was to never use rebase-merge on pull requests to master, regardless of the number of contributors, even if the commit hygiene is good. Your vote was to maintain the status quo with regard to pull requests. You didn't comment on usage of the co-author field, so please clarify if your vote includes not using this either, or if you agree that the co-author option should be utilized. Your proposal was for contributors to request of a committer, the creation of a branch when there are multiple collaborators on a feature, for the collaborators to work on this branch, then, and only then, committers would use rebase-merge when bringing this branch into master. Contributors who are working alone, or don't mind their contributions squashed should continue to create pull requests to master. I can assume that the collaborators guide would need to be updated and the process for requesting branches be formalized and expeditious. I can also imagine that we might fall into the same discussion about commit hygiene here, and there would need to be formalized expectations in the collaborators guide. Did I get that mostly right? I have more questions about your proposal, but I'd like to hear what other people think first and their responses may provide further clarifications. Cheers, Aaron On Thu, Jul 12, 2018 at 4:46 PM, eric xie <eric.jy....@gmail.com> wrote: > -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...]
