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. > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > Anton > > > > > > > > > > > > > > > > > > > > чт, 12 июл. 2018 г. в 10:11, Hao Jin < > hjjn.a...@gmail.com > > >: > > > > > > > > > > > > > > > > > > > > > +1 for Marco's proposal on using the co-author field. > > IMHO > > > > the > > > > > > > usage > > > > > > > > of > > > > > > > > > > > co-author field should not be that often, consider: > > > > > > > > > > > 1) If a PR is so big that it needs multiple people to > > > > > contribute > > > > > > a > > > > > > > > > > > substantial part of it, why can't that PR be broken > down > > > into > > > > > > > smaller > > > > > > > > > PRs > > > > > > > > > > > each submitted by single one of them? > > > > > > > > > > > 2) If a PR is small enough (for example, < 300 lines), > > why > > > > > can't > > > > > > a > > > > > > > > > single > > > > > > > > > > > person complete it? > > > > > > > > > > > 3) If a majority of PR is done by someone, and there's > > some > > > > > minor > > > > > > > > issue > > > > > > > > > > > he/she needs help with(a small bug, incomplete doc), > why > > > > can't > > > > > > that > > > > > > > > be > > > > > > > > > > done > > > > > > > > > > > through code reviews? > > > > > > > > > > > From the above 3 situations we can see that > > collaborations > > > on > > > > > > > > > individual > > > > > > > > > > > PRs should not be quite often, but I do agree that it > > could > > > > > still > > > > > > > be > > > > > > > > > > > necessary when someone lacks the related > > > expertise/knowledge > > > > on > > > > > > > some > > > > > > > > > > > necessary part of that PR given that the required > > knowledge > > > > > > cannot > > > > > > > be > > > > > > > > > > > picked up in a short period of time. > > > > > > > > > > > > > > > > > > > > > > I do agree that keeping the commit histories of PRs > clean > > > is > > > > > very > > > > > > > > > > important > > > > > > > > > > > as it could be confusing when reviewing PRs, but that > > > really > > > > > > > depends > > > > > > > > on > > > > > > > > > > > personal preferences (For my case, I usually do "git > > commit > > > > > > > --amend" > > > > > > > > on > > > > > > > > > > > trivial changes and get a new commit for bigger changes > > > such > > > > > as a > > > > > > > > > > > checkpoint of my whole PR). With growing the community > > and > > > > > > > attracting > > > > > > > > > > more > > > > > > > > > > > contributors as a high priority for our project, I > would > > > > prefer > > > > > > > that > > > > > > > > we > > > > > > > > > > do > > > > > > > > > > > not put even more burden on the contributors by asking > > them > > > > to > > > > > > > manage > > > > > > > > > and > > > > > > > > > > > squash the commits themselves just for the not-so-often > > > cases > > > > > of > > > > > > > > having > > > > > > > > > > > multiple contributors on a single PR. > > > > > > > > > > > Best regards, > > > > > > > > > > > Hao > > > > > > > > > > > > > > > > > > > > > > On Thu, Jul 12, 2018 at 12:35 AM, Marco de Abreu < > > > > > > > > > > > marco.g.ab...@googlemail.com.invalid> wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi Naveen, > > > > > > > > > > > > > > > > > > > > > > > > I'm in favour of the squashing, considering the > number > > of > > > > > > commits > > > > > > > > in > > > > > > > > > > some > > > > > > > > > > > > PRs and especially because of some people making > commit > > > > > > messages > > > > > > > a > > > > > > > > la > > > > > > > > > > > "fix" > > > > > > > > > > > > "fix" "fix" all the time. Additionally, it gets hard > > (not > > > > > > > > impossible, > > > > > > > > > > > just > > > > > > > > > > > > more inconvenient) to determine the atomic states of > > > > master - > > > > > > > aka, > > > > > > > > > > which > > > > > > > > > > > > commits are separate from each other. You should > > consider > > > > > that > > > > > > > > > > > intermediary > > > > > > > > > > > > commits are unstable (fail CI) and thus it could be > > very > > > > hard > > > > > > to > > > > > > > > > bisect > > > > > > > > > > > > failures in future - and the commit history gets > > > cluttered. > > > > > > > > > > > > > > > > > > > > > > > > As alternative, I'd like to suggest the co-author > field > > > for > > > > > > these > > > > > > > > > > cases. > > > > > > > > > > > > Further documentation is available at > > > > > > > > > > > > > > > > https://blog.github.com/2018-01-29-commit-together-with-co- > > > > > > > > authors/. > > > > > > > > > > > > > > > > > > > > > > > > I definitely agree with the second part. We should > all > > > lead > > > > > by > > > > > > > > > example > > > > > > > > > > > and > > > > > > > > > > > > maintain a high quality by keeping our commit > messages > > > > clean > > > > > > and > > > > > > > > > > > > meaningful. When I receive an email notification > that a > > > new > > > > > > > commit > > > > > > > > > has > > > > > > > > > > > been > > > > > > > > > > > > added and it only contains "fix" as title, it's not > > that > > > > > > helpful > > > > > > > > and > > > > > > > > > > also > > > > > > > > > > > > it's hard to track the development of a PR overtime. > > > E.g., > > > > > why > > > > > > > has > > > > > > > > > > > > something been changed? Was there maybe a bug that we > > > > didn't > > > > > > > cover > > > > > > > > > with > > > > > > > > > > > > tests but the author just hacked something to get it > to > > > > work > > > > > > but > > > > > > > > the > > > > > > > > > > > > problem still lays somewhere? We won't know that way > > and > > > it > > > > > > makes > > > > > > > > it > > > > > > > > > > > harder > > > > > > > > > > > > for us to review. > > > > > > > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > > > > Marco > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Naveen Swamy <mnnav...@gmail.com> schrieb am Do., > 12. > > > Juli > > > > > > 2018, > > > > > > > > > > 10:09: > > > > > > > > > > > > > > > > > > > > > > > > > Hi All, > > > > > > > > > > > > > > > > > > > > > > > > > > I am seeing that maintainers merge PRs into the > repo, > > > > they > > > > > > are > > > > > > > > > > > squashing > > > > > > > > > > > > > the commits in the PR, which I understand and agree > > is > > > to > > > > > > keep > > > > > > > a > > > > > > > > > sane > > > > > > > > > > > > > commit history, however this is causing problem > when > > > > there > > > > > > are > > > > > > > > > > multiple > > > > > > > > > > > > > contributors involved on a PR(by contributing to a > > fork > > > > of > > > > > > the > > > > > > > > > repo) > > > > > > > > > > > this > > > > > > > > > > > > > effectively removes credit for multiple > contributors > > > > > involved > > > > > > > and > > > > > > > > > > shows > > > > > > > > > > > > all > > > > > > > > > > > > > code as authored by the contributor who created the > > PR. > > > > > > > > > > > > > > > > > > > > > > > > > > Can I request maintainers to not squash PRs if > there > > > are > > > > > > > multiple > > > > > > > > > > > > > contributors involved on the PR. > > > > > > > > > > > > > > > > > > > > > > > > > > Also on the same note, I request > > > contributors(regardless > > > > of > > > > > > > > > multiple > > > > > > > > > > > > > contributors or not) to keep a clean commit history > > by > > > > > > > squashing > > > > > > > > > the > > > > > > > > > > > > > commits and not push all your WIP commits to the > PR. > > > this > > > > > > will > > > > > > > > help > > > > > > > > > > us > > > > > > > > > > > > keep > > > > > > > > > > > > > our commit history clean and meaningful. > > > > > > > > > > > > > > > > > > > > > > > > > > Let me know your thoughts/better approach or If I > > have > > > > > > > > > misunderstood > > > > > > > > > > > how > > > > > > > > > > > > > this works. > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, Naveen > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
