From dev-return-3533-archive-asf-public=cust-asf.ponee.io@mxnet.incubator.apache.org Thu Jul 12 17:25:59 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 7A9FD180676 for ; Thu, 12 Jul 2018 17:25:58 +0200 (CEST) Received: (qmail 65787 invoked by uid 500); 12 Jul 2018 15:24:25 -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 55227 invoked by uid 99); 12 Jul 2018 15:16:43 -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 15:16:43 +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 2FCAB1810DC for ; Thu, 12 Jul 2018 15:16:36 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 2.888 X-Spam-Level: ** X-Spam-Status: No, score=2.888 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_REPLY=1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, 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=gmail.com Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id RHsjIbnMj6So for ; Thu, 12 Jul 2018 15:16:32 +0000 (UTC) Received: from mail-oi0-f44.google.com (mail-oi0-f44.google.com [209.85.218.44]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with ESMTPS id 868155F117 for ; Thu, 12 Jul 2018 15:16:32 +0000 (UTC) Received: by mail-oi0-f44.google.com with SMTP id y207-v6so56451425oie.13 for ; Thu, 12 Jul 2018 08:16:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=t9LSzWvoYwnhnTpYpqLPCLOFUJDP/XtJT208RJhcW7w=; b=Zh3kPRzNA81ouCnqIzfe7du0Kz0W9qGCRxlGfLKwqo3DUPAa8eONehDTGjGI4b22ON G91DVk/39G9a9zJJf/3M4ftCypiwMmxPDev6It3aMVco4BebpjSwYxoR+C8oxBT0vMZ1 whKyFxtXkq5C+usnZzx0XapgWlFWf/Qnco3dXGfYevSyh7TMSKO6uMpp93ZzvznTMvMC wxnXibeXVNmeGi476eoUqylavaWrEs2ci2WDPm/X63fFj43EM0Ew7eLBx+ZntnS9tQbL kHOmMtOiG4221GPpFkxTwUwTT/gHMoWMn6KySGRCxLH3m/iwJxD8gACjAVC6SXszcKf2 CLyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=t9LSzWvoYwnhnTpYpqLPCLOFUJDP/XtJT208RJhcW7w=; b=XOyiX9t+9h2c+5pKrGEkPNij4qj7wvOwSXMiGgxf6wpJL/Xg4XZ+ukDTsnC+tvj9lk ZGbge6qbKEOyLlURmas06fkscyq8svJzKd2ryRJV0NkQNmerG+zwd9l0wtz8NMeN1Upf L3OhRD8bK9PbDkSHlGTPZ8iMEV2t3oihfD9tLvo84gs7lm2zQOEX8g+pRtQ3JnygghX0 k2xJx2v+my+aiXhKxK6CHbS6anEsH4h1lf3OtP0ywKrCxPYQOt9FM/gDhNUAOQpxei48 vxKZ1XZYiJZ/R+yjJZGx3Hf0HTkl8+RK1Wyzn5zTvgih/Wcc7I7j5W+6mlKwKurrCVMc lunA== X-Gm-Message-State: AOUpUlHtMWz02x3TEti9TEEbs9AR6qDu5JKUYIYAdAIayNqacfgLelDl 2fl6MrvlCza12BnAqcezgkvbyroM2WaY4NwpnYEiodKU X-Google-Smtp-Source: AAOMgpdVggVmtpo4Z1dOXLkaN2oO2M/OF6JT93yPyd4RxG4EJot8CLOFms5/LxAqYYrh32GZ5DpZnpZOOywImWypx8o= X-Received: by 2002:aca:d4d7:: with SMTP id l206-v6mr2649144oig.52.1531408585138; Thu, 12 Jul 2018 08:16:25 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a4a:98ca:0:0:0:0:0 with HTTP; Thu, 12 Jul 2018 08:16:24 -0700 (PDT) In-Reply-To: References: From: Aaron Markham Date: Thu, 12 Jul 2018 08:16:24 -0700 Message-ID: Subject: Re: Squash/Merge PRs To: dev@mxnet.incubator.apache.org Content-Type: multipart/alternative; boundary="000000000000b926e70570ced877" --000000000000b926e70570ced877 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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 > > =D1=87=D1=82, 12 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 10:11, Hao Jin <= hjjn.amzn@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 P= Rs > > each submitted by single one of them? > > 2) If a PR is small enough (for example, < 300 lines), why can't a sing= le > > 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 individua= l > > 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 a= nd > > 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.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 bise= ct > > > 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 exampl= e > > and > > > maintain a high quality by keeping our commit messages clean and > > > meaningful. When I receive an email notification that a new commit ha= s > > 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 wi= th > > > 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. 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 sa= ne > > > > 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 multipl= e > > > > contributors or not) to keep a clean commit history by squashing th= e > > > > 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 misunderstoo= d > > how > > > > this works. > > > > > > > > Thanks, Naveen > > > > > > > > > > --000000000000b926e70570ced877--