From dev-return-3568-archive-asf-public=cust-asf.ponee.io@mxnet.incubator.apache.org Fri Jul 13 00:22:07 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 9E7BA180654 for ; Fri, 13 Jul 2018 00:22:06 +0200 (CEST) Received: (qmail 89036 invoked by uid 500); 12 Jul 2018 22:22:05 -0000 Mailing-List: contact dev-help@mxnet.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@mxnet.incubator.apache.org Delivered-To: mailing list dev@mxnet.incubator.apache.org Received: (qmail 89024 invoked by uid 99); 12 Jul 2018 22:22:05 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 12 Jul 2018 22:22:04 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id 71C7E1805D3 for ; Thu, 12 Jul 2018 22:22:04 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.889 X-Spam-Level: * X-Spam-Status: No, score=1.889 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001, T_DKIMWL_WL_MED=-0.01] autolearn=disabled Authentication-Results: spamd3-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=googlemail.com Received: from mx1-lw-us.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id 0ZO--pgm74CC for ; Thu, 12 Jul 2018 22:21:59 +0000 (UTC) Received: from mail-lj1-f177.google.com (mail-lj1-f177.google.com [209.85.208.177]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with ESMTPS id E0DA45F438 for ; Thu, 12 Jul 2018 22:21:58 +0000 (UTC) Received: by mail-lj1-f177.google.com with SMTP id p10-v6so17717328ljg.2 for ; Thu, 12 Jul 2018 15:21:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=CDxjPRrhGEj8Pwlww8mfSrl0aoeuDSWw6L0qzXAocOg=; b=CahBsPJ4fScG5UBZKH0UYKTY8BDpo6rxQzr355Lgze3izZXoVmjRe2Bd3JCJlvNwMy dng5ZpKU+is0QXynE9mamoziaZK5eKPNim8ZZvdx5rBIbnMF3XWlB5UXWytT532+lioY N+wYR1HDLaE6A2WXMOWop5P/3dvyIvPfRPcRl/ivgD1JHEo+e5NtYcGRwa03NELH/J9P DjY9q7QKxfbyk1NRklnVA05VcK1DsFUpsbEbLc+gfLZ6ePgu/XfMjtti+PuWJT5BAcFD 4QJO5WZWL9/teozRp9Jt8EkEZXc595TZaei8Xy646FQLyv4KNP90cn5IvCHW1tDPlrTS NOpg== X-Gm-Message-State: AOUpUlFpE+BXlKDY2e7TUh5olr0+gprQP/E3F46Q6+/hXwxWpNSn0HKh vdKLFp40udxavdGcf68rAZihOnrUDSA+5d+uWCU= X-Google-Smtp-Source: AAOMgpfOWUkJSOEzVnyttkCVrdrS5Cv7zDy3f5R1yPYD/esXZC4/Zw8py3Z4PhZxIQHCwcbjG+A1qyzPdmmdAYukMS0= X-Received: by 2002:a2e:2e02:: with SMTP id u2-v6mr1736349lju.77.1531434117168; Thu, 12 Jul 2018 15:21:57 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Marco de Abreu Date: Fri, 13 Jul 2018 01:21:44 +0300 Message-ID: Subject: Re: Squash/Merge PRs To: dev@mxnet.incubator.apache.org Content-Type: multipart/alternative; boundary="0000000000008d01f10570d4ca65" --0000000000008d01f10570d4ca65 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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 step= s > better than huge PRs. We also have JIRA or issues to relate a set of P= Rs > together. If not possible, PR with a set of non squashed commits is al= so > good. > > Hope it helps. > > Pedro. > > On Thu, Jul 12, 2018 at 11:47 PM Naveen Swamy wrote: > > > @Aaron, I do not think most contributors(SDE or not) were even aware th= at > > 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-author= s. > > > > @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 th= at > > when there are multiple contributors. This discussion is only about Not > > Squashing when there are multiple contributors and agree to continue th= e > > 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.abreu@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 adap= t > > our > > > process. > > > > > > Additionally, GitHub has this feature request tracked internally. Let= 's > > see > > > when they get to add it. > > > > > > -Marco > > > > > > > > > > > > Aaron Markham 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 lon= e > > > ranger > > > > behavior. People can and should be proud of their work, but why doe= s > 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 li= ke > > > > something worth doing! > > > > > > > > On Thu, Jul 12, 2018 at 11:15 AM, Hao Jin > wrote: > > > > > > > > > Hi Aaron, > > > > > To be fair, this discussion has nothing to do with "pride" of SDE= s, > > but > > > > > rather a discussion on what is a better software engineering > practice > > > for > > > > > the project. Breaking a large project into smaller tasks is a goo= d > > > > 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=E2=80=8B, with larger changes p= referably > > > broken > > > > > into a series of smaller changes that a reviewer can easily revie= w > 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 visio= n > > 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, i= f > > you > > > > only > > > > > have to fix the doc for someone once in a while, I definitely agr= ee > > > 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.markham@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 fee= l > > like > > > > it > > > > > > should be a red flag. > > > > > > > > > > > > I work in collaboration with dozens of engineers to improve the= ir > > > > > > 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 wi= th > > 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 releasin= g > > > > > 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 committer= s > > and > > > > > reduce > > > > > > the latency in reviews and merges). > > > > > > +1 on improving the quality of commit messages, so we don't hav= e > to > > > > > squash > > > > > > & merge all of the time. > > > > > > +1 on improving the practice of better commit & merge managemen= t > 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 < > > mechernov@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 t= o > 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 > > > > > > > > > > > > > > =D1=87=D1=82, 12 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 10:1= 1, Hao Jin : > > > > > > > > > > > > > > > +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 in= to > > > > 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/knowledg= e > 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 i= s > > very > > > > > > > important > > > > > > > > as it could be confusing when reviewing PRs, but that reall= y > > > > depends > > > > > on > > > > > > > > personal preferences (For my case, I usually do "git commit > > > > --amend" > > > > > on > > > > > > > > trivial changes and get a new commit for bigger changes suc= h > > 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 cas= es > > 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.abreu@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 cluttere= d. > > > > > > > > > > > > > > > > > > As alternative, I'd like to suggest the co-author field f= or > > > 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 le= ad > > by > > > > > > example > > > > > > > > and > > > > > > > > > maintain a high quality by keeping our commit messages > clean > > > and > > > > > > > > > meaningful. When I receive an email notification that a n= ew > > > > 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 schrieb am Do., 12. Jul= i > > > 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 ar= e > > > > multiple > > > > > > > > > > contributors involved on the PR. > > > > > > > > > > > > > > > > > > > > Also on the same note, I request contributors(regardles= s > 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. th= is > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --0000000000008d01f10570d4ca65--