> > Is it not the case currently? If I am working on a feature modifying class > X and another developer makes some refactoring changes on class X and > pushes it to develop, won't I have to resolve the merge commits anyway.
Yes, you will always have to deal with resolving conflicts with other people's changes. The scenario I was describing was me having to resolve conflicts between my own 2 changes that modify the same files. If I make a refactor commit and a fix commit as two separate PRs that are each based on develop (i.e. they are independent PRs), after I merge the first one to develop the second one will have merge conflicts. The simplest way to avoid this is to put the commits in the same PR and use rebase-and-merge. On Tue, Dec 31, 2019 at 10:46 PM Owen Nichols <onich...@pivotal.io> wrote: > It sounds like there is value in being able to deliver un-squashed PRs, and > we believe the richer commit message history outweighs any potential > inconvenience to bisect operations (as Aaron pointed out, finer-grained > commits should generally be to the benefit of bisect operations). > > We will leave all 3 merge options enabled in GitHub. > > On Tue, Dec 31, 2019 at 7:27 PM Dan Smith <dsm...@pivotal.io> wrote: > > > I also tend to do the same workflow Aaron described - make a refactoring > > change to support a feature followed by the actual change I want to make. > > It makes the history a lot clearer because refactoring tends to touch a > lot > > of files, so it's nice to have those changes clearly marked as a > > refactoring that should not change behavior. > > > > It's possible to do this as to separate PRs, but that drags out the > process > > because you have to get the first in merged before you create the second. > > That encourages bunching changes in a single squashed commit, which makes > > git blame less useful. > > > > > > -Dan > > > > On Tue, Dec 31, 2019, 6:36 PM Nabarun Nag <n...@apache.org> wrote: > > > > > Aaron , > > > > > > Is it not the case currently? If I am working on a feature modifying > > class > > > X and another developer makes some refactoring changes on class X and > > > pushes it to develop, won't I have to resolve the merge commits anyway. > > > > > > > > > Regards > > > Naba > > > > > > > > > On Tue, Dec 31, 2019 at 6:01 PM Aaron Lindsey <aaronlind...@apache.org > > > > > wrote: > > > > > > > Suppose I’m refactoring the same classes I’m touching for the > feature. > > I > > > > do the refactoring on one branch and submit a PR for that. Then I > > > implement > > > > the feature on another branch which is based on develop and does not > > have > > > > the refactoring changes since those are not merged yet. I’ll have to > > > > resolve conflicts on the second PR that I merge. > > > > > > > > Having interdependent PRs where one PR branch is based on another PR > > > > branch is not something I’ve tried. That requires more overhead in > > having > > > > to create more PRs and branches. And if you have N interdependent > PRs, > > > you > > > > have to do N-1 merges each time a PR is merged to develop (to update > > the > > > > other branches). It could be done, but is it worth the hassle? > > > > > > > > One more point about rebase-and-merge is that it seems like this > would > > > > make bisecting a failure easier since there would be smaller commits. > > > > > > > > Aaron > > > > > > > > > On Dec 31, 2019, at 5:41 PM, Owen Nichols <onich...@pivotal.io> > > wrote: > > > > > > > > > > Can you elaborate on why you would have to deal with conflicts if > the > > > > > refactoring work was kept as a separate PR from the fix? > > > > > > > > > > As with many git workflows, there’s an easy way and a hard way to > > > manage > > > > > interdependent PRs (tl;dr only merge, never rebase!). I wonder if > > this > > > > > points out an opportunity for a “tips and tricks” page on the Geode > > > wiki. > > > > > > > > > > On Tue, Dec 31, 2019 at 5:22 PM Aaron Lindsey < > > aaronlind...@apache.org > > > > > > > > > wrote: > > > > > > > > > >> -0.9 > > > > >> > > > > >> I’m not in favor of the revised proposal that disallows > > > > rebase-and-merge. > > > > >> Say I am working on a PR and I have a refactor commit and another > > > commit > > > > >> which implements a new feature. I don’t want those commits to get > > > > squashed > > > > >> because that makes it hard to understand the diff. However, if I > > make > > > > those > > > > >> commits as two separate PRs then I am going to have to deal with > > > > conflicts. > > > > >> > > > > >> I’m not sure when we made it a rule that every commit in develop > had > > > to > > > > >> compile and pass tests. I know we implemented a rule that all PRs > > had > > > to > > > > >> pass certain checks, but I never thought that rule implied all > > commits > > > > had > > > > >> to pass those checks. > > > > >> > > > > >> In general I just don’t see the problem with rebase-and-merge and > > this > > > > >> feels like an unnecessary restriction, but I will go with it if > > that’s > > > > what > > > > >> everyone wants to do. > > > > >> > > > > >> Aaron > > > > >> > > > > >>> On Dec 31, 2019, at 3:09 PM, Owen Nichols <onich...@pivotal.io> > > > wrote: > > > > >>> > > > > >>> To recap, this proposal is now revised to remove 2 of the 3 merge > > > > options > > > > >>> from GitHub, leaving only Squash and Merge. PR #4513 > > > > >>> <https://github.com/apache/geode/pull/4513> serves as an exhibit > > of > > > > >> what is > > > > >>> proposed (it is not to be merged unless discussion leads to > > consensus > > > > >> and a > > > > >>> successful vote). Please use the dev list (not the PR) for all > > > > >> discussion > > > > >>> (and voting, if we get that far). > > > > >>> > > > > >>> Squash and merge is already used almost exclusively by the Geode > > > > >> community, > > > > >>> with any exceptions tending to be accidental more often than > > > > intentional. > > > > >>> However, some have raised the concern that implementing this > > > > restriction > > > > >>> could result in harm or wasted time. Can someone give an example > > of > > > a > > > > >> such > > > > >>> a scenario? > > > > >>> > > > > >>> It seems there is a divide here between junior and senior > community > > > > >>> members. Newer committers appreciate additional guardrails to > > > protect > > > > >> them > > > > >>> from accidentally doing the wrong thing, while those with more > > > > experience > > > > >>> want to be able to work unencumbered by restrictions of any kind. > > > > >>> > > > > >>> Our welcome email to new committers states “We like to work on > > trust > > > > >> rather > > > > >>> than unnecessary constraints...Being a committer enables you to > > more > > > > >> easily > > > > >>> make changes without needing to go through the patch submission > > > > process”. > > > > >>> I can see this as an argument against this proposal (perhaps even > > an > > > > >>> argument against any form of branch protection). > > > > >>> > > > > >>> In the scheme of things, this proposal makes very little > > difference. > > > > >> There > > > > >>> are still other ways to get non-compiling commits onto develop > > (e.g. > > > > >>> waiting a long time between running PR checks and merging to > > > develop). > > > > >>> What’s more important is working well together as a community. > So, > > > > >> perhaps > > > > >>> what’s best for the community is to encourage working on trust > > rather > > > > >> than > > > > >>> unnecessary constraints. > > > > >>> > > > > >>> -Owen > > > > >>> > > > > >>> On Dec 31, 2019, at 10:56 AM, Kirk Lund <kl...@apache.org> > wrote: > > > > >>> > > > > >>> I'm happy to file multiple PRs when I need to merge multiple > > commits > > > to > > > > >>> develop. > > > > >>> > > > > >>> On Mon, Dec 30, 2019 at 5:45 PM Mark Hanson <mhan...@pivotal.io> > > > > wrote: > > > > >>> > > > > >>> This change to disable all but squash-merge would be really easy > to > > > > >>> revert. How about we try it for a while and see? If people decide > > it > > > is > > > > >>> really limiting them, we can change it back. Let’s do it for 1 > > month > > > > and > > > > >>> see how it goes. Does that sound reasonable? > > > > >>> > > > > >>> Thanks, > > > > >>> Mark > > > > >>> > > > > >>> On Dec 30, 2019, at 5:25 PM, Owen Nichols <onich...@pivotal.io> > > > wrote: > > > > >>> > > > > >>> Given that we adopted < > > > > >>> > > > > >>> > > > > >> > > > > > > > > > > https://lists.apache.org/thread.html/c3eb5c028cb3a4d76024f928a7a33c0311228f5dbbcaa86287bf5826@%3Cdev.geode.apache.org%3E > > > > >>>> > > > > >>> and still wish to continue < > > > > >>> > > > > >> > > > > > > > > > > https://lists.apache.org/thread.html/8795697c1ad57068c053b48b4b1845005f3ade0be777e679eafe95db@%3Cdev.geode.apache.org%3E > > > > >>>> > > > > >>> having branch protection rules to ensure every commit landing in > > > > develop > > > > >>> has passed the required PR checks, it seems like that decision > > alone > > > > >>> mandates that we disable all merge mechanisms other than > > > > >> squash-and-merge. > > > > >>> > > > > >>> > > > > >>> Allowing merge commits or rebase merges bypasses branch > protection > > > for > > > > >>> > > > > >>> all commits other than the final one in the merge or rebase set. > > > Given > > > > >>> that we decided < > > > > >>> > > > > >> > > > > > > > > > > https://lists.apache.org/thread.html/1ba19d9aeb206148c922afdd182ba322d6f128bbb83983f2f72a108e@%3Cdev.geode.apache.org%3E > > > > >>>> > > > > >>> that bypassing PR checks should never be allowed, keeping this > > > loophole > > > > >>> open seems untenable. > > > > >>> > > > > >>> > > > > >>> This is not just hypothetical — this loophole is causing real > > > problems. > > > > >>> > > > > >>> We now have commits on develop that don’t compile. For example: > > > > >>> > > > > >>> git checkout 19eee7821322a1301f16bdcd31fd3b8c872a41b6 > > > > >>> ./gradlew devBuild > > > > >>> ...spotlessJava FAILED > > > > >>> We implemented branch protections to make this impossible, right? > > > > >>> > > > > >>> We can very easily close this loophole by disabling all but the > > > > >>> > > > > >>> Squash&Merge button for PRs. This will not make more work for > any > > > > >>> developer. If you want to get multiple commits onto develop, > > simply > > > > >> submit > > > > >>> each as a separate PR — that is the only way to assure that > > required > > > PR > > > > >>> checks have passed for each. > > > > >>> > > > > >>> > > > > >>> On the other hand, if we as a Geode community feel strongly that > > > > >>> > > > > >>> bypassing branch protection via merge commits and rebase commits > is > > > > >>> important to allow, why not also allow arbitrary overrides (or > get > > > rid > > > > of > > > > >>> branch protection entirely)? > > > > >>> > > > > >>> > > > > >>> -Owen > > > > >>> > > > > >>> On Dec 20, 2019, at 12:31 PM, Blake Bender <bben...@pivotal.io> > > > wrote: > > > > >>> > > > > >>> Just FWIW, the situation described of having multiple commits in > a > > > > >>> > > > > >>> single > > > > >>> > > > > >>> PR with separate associated JIRA tickets is still kind of > > > problematic. > > > > >>> > > > > >>> It > > > > >>> > > > > >>> could well be the case that the commits are interdependent, thus > > when > > > > >>> bisecting etc it's still not possible to revert the fix for a > > single > > > > >>> bug/feature/whatever atomically. It's all good, though, I'm > > > satisfied > > > > >>> > > > > >>> no > > > > >>> > > > > >>> one's forcing me to adopt practice I'm opposed to. Apologies for > > > > >>> > > > > >>> getting > > > > >>> > > > > >>> my feathers a little ruffled, or if I ruffled anyone else's in > > > return. > > > > >>> > > > > >>> Thanks, > > > > >>> > > > > >>> Blake > > > > >>> > > > > >>> > > > > >>> On Fri, Dec 20, 2019 at 12:18 PM Nabarun Nag <n...@pivotal.io> > > > wrote: > > > > >>> > > > > >>> Hi Dan, > > > > >>> > > > > >>> When we do squash merge all the commit messages are preserved and > > > also > > > > >>> > > > > >>> the > > > > >>> > > > > >>> co-authored tag works when we do squash merge. > > > > >>> So the authorship and history are preserved. > > > > >>> > > > > >>> In my own personal experience and reverts and pinpointing > > regression > > > > >>> failures are tough when the commits are spread out. Also, reverts > > are > > > > >>> easier when it is just one commit while we are bisecting > failures. > > > > >>> > > > > >>> > > > > >>> Regards > > > > >>> Naba > > > > >>> > > > > >>> On Fri, Dec 20, 2019 at 12:07 PM Dan Smith <dsm...@pivotal.io> > > > wrote: > > > > >>> > > > > >>> I'll change to -0. > > > > >>> > > > > >>> I think merge commits are a nice way to record history in some > > cases, > > > > >>> > > > > >>> and > > > > >>> > > > > >>> can also be a way to avoid messy conflicts that come with rebase. > > > > >>> > > > > >>> Merge > > > > >>> > > > > >>> commits also preserve authorship of commits (compared to > > > > >>> > > > > >>> squash-merge), > > > > >>> > > > > >>> which I think is valuable as an open source community that is > > trying > > > > >>> > > > > >>> to > > > > >>> > > > > >>> keep track of outside contributions. > > > > >>> > > > > >>> That said, if the rest of y'all feel it will help to disable the > > > > >>> > > > > >>> button, > > > > >>> > > > > >>> I > > > > >>> > > > > >>> won't stand in the way. > > > > >>> > > > > >>> -Dan > > > > >>> > > > > >>> On Fri, Dec 20, 2019 at 11:50 AM Anthony Baker < > aba...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> > > > > >>> Whether we are talking about the geode/ repository or the > > > > >>> > > > > >>> geode-native/ > > > > >>> > > > > >>> repository we are all one GEODE community. > > > > >>> > > > > >>> The idea of a native client team may matter in some contexts but > in > > > > >>> > > > > >>> this > > > > >>> > > > > >>> space we are all GEODE contributors. > > > > >>> > > > > >>> Adopting a common approach across our repos will make it easier > for > > > > >>> > > > > >>> new > > > > >>> > > > > >>> contributors to engage, learn our norms, and join our efforts. > > > > >>> > > > > >>> $0.02, > > > > >>> Anthony > > > > >>> > > > > >>> > > > > >>> On Dec 20, 2019, at 11:32 AM, Blake Bender <bben...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> > > > > >>> Is this a policy the native client team must abide by, as well? > It > > > > >>> > > > > >>> varies > > > > >>> > > > > >>> slightly with what we've been doing, and all other things being > > > > >>> > > > > >>> equal I > > > > >>> > > > > >>> see > > > > >>> > > > > >>> no reason for us to change that. If I am to have any measure of > > > > >>> > > > > >>> control > > > > >>> > > > > >>> over the nc repository, I will definitely enforce a 1:1 > > > > >>> > > > > >>> correspondence > > > > >>> > > > > >>> between commits to develop and JIRA tickets. IMO, if your > > > > >>> > > > > >>> refactoring > > > > >>> > > > > >>> in a > > > > >>> > > > > >>> PR is sufficiently large or complex that it's difficult to tease > it > > > > >>> > > > > >>> out > > > > >>> > > > > >>> from the bug you're fixing or feature you're implementing, it > > merits > > > > >>> > > > > >>> its > > > > >>> > > > > >>> own JIRA ticket and a separate PR. If your "actual" fix then > > > > >>> > > > > >>> becomes > > > > >>> > > > > >>> dependent on the refactored code, that's a price I'm willing to > pay > > > > >>> > > > > >>> to > > > > >>> > > > > >>> keep > > > > >>> > > > > >>> history clean. > > > > >>> > > > > >>> On the other hand, I see no real value in squashing to a single > > > > >>> > > > > >>> commit > > > > >>> > > > > >>> prior to submitting a PR, since your view of the changes on > GitHub > > > > >>> > > > > >>> is > > > > >>> > > > > >>> essentially the same either way. We haven't enforced this on the > > nc > > > > >>> > > > > >>> repo, > > > > >>> > > > > >>> and I'd prefer to keep it that way. > > > > >>> > > > > >>> Thanks, > > > > >>> > > > > >>> Blake > > > > >>> > > > > >>> > > > > >>> On Fri, Dec 20, 2019 at 10:29 AM Jinmei Liao <jil...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> > > > > >>> Merge commit is the new contributor's default setting. When we > are > > > > >>> > > > > >>> merging > > > > >>> > > > > >>> new contributor's PR, since we are so used to THINKING > > > > >>> > > > > >>> "squash-and-merge" > > > > >>> > > > > >>> is the default, we forgot to check what the button really says > when > > > > >>> > > > > >>> we > > > > >>> > > > > >>> are > > > > >>> > > > > >>> merging other people's PR. > > > > >>> > > > > >>> On Fri, Dec 20, 2019 at 10:18 AM Ernest Burghardt < > > > > >>> > > > > >>> eburgha...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> I'm a proponent of using squash-and-merge, and once a person has > > > > >>> > > > > >>> chosen > > > > >>> > > > > >>> this option once it comes up by default afterwards... > > > > >>> > > > > >>> Owen, I don't think you have consensus to put forth this PR, > > > > >>> > > > > >>> there > > > > >>> > > > > >>> are > > > > >>> > > > > >>> -1s > > > > >>> > > > > >>> above... (early voting) > > > > >>> > > > > >>> maybe we'll be better off socializing the norm of > squash-and-merge > > > > >>> > > > > >>> and > > > > >>> > > > > >>> gaining a natural consensus that way... > > > > >>> > > > > >>> On Fri, Dec 20, 2019 at 10:07 AM Owen Nichols < > > > > >>> > > > > >>> onich...@pivotal.io > > > > >>> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> > > > > >>> The proposed action manifests as a commit to the Geode git > > > > >>> > > > > >>> repository, > > > > >>> > > > > >>> so > > > > >>> > > > > >>> a PR is an acceptable vehicle for voting in this case. > > > > >>> > > > > >>> On Dec 20, 2019, at 9:38 AM, Bruce Schuchardt < > > > > >>> > > > > >>> bschucha...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> > > > > >>> I see a lot of plus-ones and a "voting deadline" on this DISCUSS > > > > >>> > > > > >>> thread > > > > >>> > > > > >>> and a request to "vote" using a PR. This all seems out of order > > > > >>> > > > > >>> to > > > > >>> > > > > >>> me. > > > > >>> > > > > >>> Our votes are supposed to be on the email list, aren't they? and > > > > >>> > > > > >>> I > > > > >>> > > > > >>> haven't > > > > >>> > > > > >>> seen a VOTE request. > > > > >>> > > > > >>> > > > > >>> On 12/20/19 9:33 AM, Nabarun Nag wrote: > > > > >>> > > > > >>> +1 > > > > >>> > > > > >>> On Fri, Dec 20, 2019 at 9:25 AM Owen Nichols < > > > > >>> > > > > >>> onich...@pivotal.io > > > > >>> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> > > > > >>> Based on the feedback so far, I will amend the proposal to > > > > >>> > > > > >>> drop > > > > >>> > > > > >>> item > > > > >>> > > > > >>> 2). > > > > >>> > > > > >>> Therefore, the current ability to create merge commits using > > > > >>> > > > > >>> command-line > > > > >>> > > > > >>> git will remain available. > > > > >>> > > > > >>> The proposal as amended is now: > > > > >>> > > > > >>> Change GitHub settings to make "Squash and merge" the default > > > > >>> (by removing “Create a merge commit” option). > > > > >>> > > > > >>> Update the PR template to change the text "Is your initial > > > > >>> > > > > >>> contribution > > > > >>> > > > > >>> a single, squashed commit” to “Is your initial contribution > > > > >>> > > > > >>> squashed > > > > >>> > > > > >>> for > > > > >>> > > > > >>> ease of review (e.g. a single commit, or a ‘refactoring’ > > > > >>> > > > > >>> commit > > > > >>> > > > > >>> plus a > > > > >>> > > > > >>> ‘fix’ commit)" > > > > >>> > > > > >>> > > > > >>> As Naba suggested, we can try it, and if we don’t like it, > > > > >>> > > > > >>> it’s > > > > >>> > > > > >>> simple > > > > >>> > > > > >>> to > > > > >>> > > > > >>> revert. > > > > >>> > > > > >>> I’ve create a PR for the proposed change here: > > > > >>> https://github.com/apache/geode/pull/4513 > > > > >>> > > > > >>> Please use the PR to vote for against this proposal. It will > > > > >>> > > > > >>> not > > > > >>> > > > > >>> be > > > > >>> > > > > >>> merged before the VOTING DEADLINE of DEC 31 (if no -1’s at > > > > >>> > > > > >>> that > > > > >>> > > > > >>> time) > > > > >>> > > > > >>> > > > > >>> On Dec 20, 2019, at 8:31 AM, Ju@N <jujora...@gmail.com> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> > > > > >>> +1 > > > > >>> > > > > >>> On Fri 20 Dec 2019 at 16:18, Owen Nichols < > > > > >>> > > > > >>> onich...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> > > > > >>> Hi Bruce, this proposal will not waste a single second of > > > > >>> > > > > >>> your > > > > >>> > > > > >>> time. It > > > > >>> > > > > >>> just prevents people from accidentally pressing a button > > > > >>> > > > > >>> that > > > > >>> > > > > >>> we > > > > >>> > > > > >>> have > > > > >>> > > > > >>> already agreed should never be pressed, but because we never > > > > >>> > > > > >>> configured > > > > >>> > > > > >>> our > > > > >>> > > > > >>> GitHub to match our stated policy, is currently the default. > > > > >>> > > > > >>> However, it will save a lot of time and frustration for > > > > >>> > > > > >>> anyone > > > > >>> > > > > >>> needing > > > > >>> > > > > >>> to > > > > >>> > > > > >>> bisect failures, revert, or cherry-pick changes, which has > > > > >>> > > > > >>> merit > > > > >>> > > > > >>> even if > > > > >>> > > > > >>> you personally never do any of those three things. > > > > >>> > > > > >>> Please start a separate thread if you would like to revisit > > > > >>> > > > > >>> the > > > > >>> > > > > >>> community > > > > >>> > > > > >>> decision to require passing PR checks. > > > > >>> > > > > >>> On Dec 20, 2019, at 7:49 AM, Bruce Schuchardt < > > > > >>> > > > > >>> bschucha...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> I agree with Jake. I would go further by saying that I see > > > > >>> > > > > >>> very > > > > >>> > > > > >>> little > > > > >>> > > > > >>> merit in this proposal. I think we're getting more and more > > > > >>> > > > > >>> bureaucratic > > > > >>> > > > > >>> in our process and that it stifles productivity. I was > > > > >>> > > > > >>> recently > > > > >>> > > > > >>> forced > > > > >>> > > > > >>> to > > > > >>> > > > > >>> spend three days fixing tests in which I had changed an > > > > >>> > > > > >>> import > > > > >>> > > > > >>> statement > > > > >>> > > > > >>> before they would pass stress testing. I'm glad the tests > > > > >>> > > > > >>> now > > > > >>> > > > > >>> pass > > > > >>> > > > > >>> reliably but I was very frustrated by the process. > > > > >>> > > > > >>> On 12/19/19 4:49 PM, Jacob Barrett wrote: > > > > >>> > > > > >>> I’m in agreement with Dan. Changes to the infrastructure > > > > >>> > > > > >>> to > > > > >>> > > > > >>> flat > > > > >>> > > > > >>> out > > > > >>> > > > > >>> prevent things that should be self policing is annoying. > > > > >>> > > > > >>> This > > > > >>> > > > > >>> PR > > > > >>> > > > > >>> review > > > > >>> > > > > >>> lock we have had already cost us valuable time waiting for > > > > >>> > > > > >>> PR > > > > >>> > > > > >>> pipelines > > > > >>> > > > > >>> to > > > > >>> > > > > >>> pass that have no relevance to the commit, like CI work: I’d > > > > >>> > > > > >>> hat > > > > >>> > > > > >>> to > > > > >>> > > > > >>> see > > > > >>> > > > > >>> yet > > > > >>> > > > > >>> another process enforced that Kees us from getting work done > > > > >>> > > > > >>> when > > > > >>> > > > > >>> necessary. > > > > >>> > > > > >>> -Jake > > > > >>> > > > > >>> > > > > >>> On Dec 19, 2019, at 4:43 PM, Dan Smith < > > > > >>> > > > > >>> dsm...@pivotal.io > > > > >>> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> > > > > >>> -1 to (1) and (2). > > > > >>> > > > > >>> I think merge commits are appropriate in certain > > > > >>> > > > > >>> circumstances, > > > > >>> > > > > >>> so I > > > > >>> > > > > >>> don't > > > > >>> > > > > >>> think we should make a blanket restriction. In fact I > > > > >>> > > > > >>> think > > > > >>> > > > > >>> our > > > > >>> > > > > >>> release > > > > >>> > > > > >>> process involves some merges. > > > > >>> > > > > >>> I think setting standards on what is reasonable to be an > > > > >>> > > > > >>> individual > > > > >>> > > > > >>> commit > > > > >>> > > > > >>> will do a lot more to clean up our history than blocking > > > > >>> > > > > >>> merges. > > > > >>> > > > > >>> We'd > > > > >>> > > > > >>> rather not see commits like "Spotless Apply" in the > > > > >>> > > > > >>> history, > > > > >>> > > > > >>> but > > > > >>> > > > > >>> if > > > > >>> > > > > >>> reasonably separate and well written commits come in as > > > > >>> > > > > >>> part > > > > >>> > > > > >>> of > > > > >>> > > > > >>> a > > > > >>> > > > > >>> merge, I > > > > >>> > > > > >>> think that's fine. > > > > >>> > > > > >>> -Dan > > > > >>> > > > > >>> On Thu, Dec 19, 2019 at 4:27 PM Jinmei Liao < > > > > >>> > > > > >>> jil...@pivotal.io > > > > >>> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> +1 > > > > >>> > > > > >>> On Thu, Dec 19, 2019, 4:05 PM Owen Nichols < > > > > >>> > > > > >>> onich...@pivotal.io > > > > >>> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> I’d like to advance this topic from an informal > > > > >>> > > > > >>> request/discussion > > > > >>> > > > > >>> to a > > > > >>> > > > > >>> discussion of a concrete proposal. > > > > >>> > > > > >>> To recap, it sounds like there is general agreement > > > > >>> > > > > >>> that > > > > >>> > > > > >>> commit > > > > >>> > > > > >>> history > > > > >>> > > > > >>> on > > > > >>> > > > > >>> develop should be linear (no merge commits), and that > > > > >>> > > > > >>> the > > > > >>> > > > > >>> biggest > > > > >>> > > > > >>> obstacle > > > > >>> > > > > >>> to this is that the PR merge button in GitHub creates a > > > > >>> > > > > >>> merge > > > > >>> > > > > >>> commit > > > > >>> > > > > >>> by > > > > >>> > > > > >>> default. > > > > >>> > > > > >>> I propose the following changes: > > > > >>> 1) Change GitHub settings to remove the ability to > > > > >>> > > > > >>> create > > > > >>> > > > > >>> a > > > > >>> > > > > >>> merge > > > > >>> > > > > >>> commit. > > > > >>> > > > > >>> This will make Squash & Merge the default. > > > > >>> > > > > >>> 2) Change GitHub settings to require linear history on > > > > >>> > > > > >>> develop. > > > > >>> > > > > >>> This > > > > >>> > > > > >>> prevents merge commits via command-line (not > > > > >>> > > > > >>> recommended, > > > > >>> > > > > >>> but > > > > >>> > > > > >>> wiki > > > > >>> > > > > >>> still > > > > >>> > > > > >>> has instructions for merging PRs this way). > > > > >>> > > > > >>> 3) Update the PR template to change the text "Is your > > > > >>> > > > > >>> initial > > > > >>> > > > > >>> contribution > > > > >>> > > > > >>> a single, squashed commit” to “Is your initial > > > > >>> > > > > >>> contribution > > > > >>> > > > > >>> squashed > > > > >>> > > > > >>> for > > > > >>> > > > > >>> ease of review (e.g. a single commit, or a > > > > >>> > > > > >>> ‘refactoring’ > > > > >>> > > > > >>> commit > > > > >>> > > > > >>> plus > > > > >>> > > > > >>> a > > > > >>> > > > > >>> ‘fix’ commit)" > > > > >>> > > > > >>> For clarity, I am proposing no-change in the following > > > > >>> > > > > >>> areas: > > > > >>> > > > > >>> i) Leave Rebase & Merge as an option for PRs that have > > > > >>> > > > > >>> been > > > > >>> > > > > >>> structured to > > > > >>> > > > > >>> benefit from it (this can make it easier in a bisect to > > > > >>> > > > > >>> see > > > > >>> > > > > >>> whether > > > > >>> > > > > >>> the > > > > >>> > > > > >>> refactoring or the “fix” broke something). > > > > >>> ii) Leave existing wording in the wiki as-is [stating > > > > >>> > > > > >>> that > > > > >>> > > > > >>> squashing > > > > >>> > > > > >>> is > > > > >>> > > > > >>> preferred]. > > > > >>> > > > > >>> > > > > >>> Please comment via this email thread. > > > > >>> -Owen > > > > >>> > > > > >>> > > > > >>> > > > > >>> On Dec 16, 2019, at 10:49 AM, Kirk Lund < > > > > >>> > > > > >>> kl...@apache.org> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> > > > > >>> I think it's already resolved Udo ;) > > > > >>> > > > > >>> Here's the problem, if I fixup a dunit test by > > > > >>> > > > > >>> removing > > > > >>> > > > > >>> all > > > > >>> > > > > >>> uses > > > > >>> > > > > >>> of > > > > >>> > > > > >>> "this." > > > > >>> > > > > >>> and I rename the dunit test, then git doesn't remember > > > > >>> > > > > >>> that > > > > >>> > > > > >>> the > > > > >>> > > > > >>> file > > > > >>> > > > > >>> is a > > > > >>> > > > > >>> rename -- it forever afterwards interprets it as a new > > > > >>> > > > > >>> file > > > > >>> > > > > >>> that I > > > > >>> > > > > >>> created > > > > >>> > > > > >>> if I touch more than 50% of the lines (which "this." > > > > >>> > > > > >>> can > > > > >>> > > > > >>> easily > > > > >>> > > > > >>> do). If > > > > >>> > > > > >>> we > > > > >>> > > > > >>> squash two commits: the rename and the cleanup of that > > > > >>> > > > > >>> dunit > > > > >>> > > > > >>> test > > > > >>> > > > > >>> -- > > > > >>> > > > > >>> then > > > > >>> > > > > >>> we effectively lose the history of that file and it > > > > >>> > > > > >>> shows > > > > >>> > > > > >>> that > > > > >>> > > > > >>> I > > > > >>> > > > > >>> created > > > > >>> > > > > >>> a > > > > >>> > > > > >>> new file. > > > > >>> > > > > >>> Also for the record, I've been working on Geode since > > > > >>> > > > > >>> the > > > > >>> > > > > >>> beginning > > > > >>> > > > > >>> and I > > > > >>> > > > > >>> was never made aware of this change in our process. I > > > > >>> > > > > >>> never > > > > >>> > > > > >>> voted > > > > >>> > > > > >>> on > > > > >>> > > > > >>> it. > > > > >>> > > > > >>> I'm not a big fan of changing various details in our > > > > >>> > > > > >>> process > > > > >>> > > > > >>> every > > > > >>> > > > > >>> single > > > > >>> > > > > >>> week. It's very easy to miss these discussions unless > > > > >>> > > > > >>> someone > > > > >>> > > > > >>> points it > > > > >>> > > > > >>> out > > > > >>> > > > > >>> to me. > > > > >>> > > > > >>> On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer < > > > > >>> > > > > >>> ukohlme...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> I'm not sure what this discussion is about... WE, as > > > > >>> > > > > >>> a > > > > >>> > > > > >>> community, > > > > >>> > > > > >>> have > > > > >>> > > > > >>> agreed in common practices, in two place no less... > > > > >>> > > > > >>> 1) Quoting our PR template > > > > >>> > > > > >>> > > > > >>> For all changes: > > > > >>> > > > > >>> * > > > > >>> > > > > >>> Is there a JIRA ticket associated with this PR? Is it > > > > >>> > > > > >>> referenced > > > > >>> > > > > >>> in > > > > >>> > > > > >>> the commit message? > > > > >>> > > > > >>> * > > > > >>> > > > > >>> Has your PR been rebased against the latest commit > > > > >>> > > > > >>> within > > > > >>> > > > > >>> the > > > > >>> > > > > >>> target > > > > >>> > > > > >>> branch (typically|develop|)? > > > > >>> > > > > >>> * > > > > >>> > > > > >>> ***Is your initial contribution a single, squashed > > > > >>> > > > > >>> commit?* > > > > >>> > > > > >>> > > > > >>> * > > > > >>> > > > > >>> Does|gradlew build|run cleanly? > > > > >>> > > > > >>> * > > > > >>> > > > > >>> Have you written or updated unit tests to verify your > > > > >>> > > > > >>> changes? > > > > >>> > > > > >>> > > > > >>> * > > > > >>> > > > > >>> If adding new dependencies to the code, are these > > > > >>> > > > > >>> dependencies > > > > >>> > > > > >>> licensed in a way that is compatible for inclusion > > > > >>> > > > > >>> underASF > > > > >>> > > > > >>> 2.0 > > > > >>> > > > > >>> < > > > > >>> > > > > >>> http://www.apache.org/legal/resolved.html#category-a > > > > >>> > > > > >>> ? > > > > >>> > > > > >>> > > > > >>> On our PR template we call out that the initial PR > > > > >>> > > > > >>> commit > > > > >>> > > > > >>> should > > > > >>> > > > > >>> be > > > > >>> > > > > >>> squashed. > > > > >>> > > > > >>> 2) > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > https://cwiki.apache.org/confluence/display/GEODE/Code+contributions > > > > >>> > > > > >>> < > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > https://cwiki.apache.org/confluence/display/GEODE/Code+contributions > > > > >>> > > > > >>> -- See "Accepting a PR Using the Command Line" - > > > > >>> > > > > >>> Point > > > > >>> > > > > >>> #3. > > > > >>> > > > > >>> > > > > >>> > > > > >>> @Kirk, if each of your commits "stands alone", I > > > > >>> > > > > >>> commend > > > > >>> > > > > >>> you > > > > >>> > > > > >>> on > > > > >>> > > > > >>> the > > > > >>> > > > > >>> diligence, but in reality, they should either then be > > > > >>> > > > > >>> stand > > > > >>> > > > > >>> alone > > > > >>> > > > > >>> PR's > > > > >>> > > > > >>> or just extra work created for yourself. > > > > >>> > > > > >>> If we want to change the way we have agreed upon we > > > > >>> > > > > >>> submit/commit/merge > > > > >>> > > > > >>> changes back into develop... Then this is another > > > > >>> > > > > >>> discussion > > > > >>> > > > > >>> thread, > > > > >>> > > > > >>> until then, I think we should all remind ourselves on > > > > >>> > > > > >>> our > > > > >>> > > > > >>> agreed > > > > >>> > > > > >>> contributions code of conduct. > > > > >>> > > > > >>> --Udo > > > > >>> > > > > >>> On 12/16/19 9:59 AM, Nabarun Nag wrote: > > > > >>> > > > > >>> Kirk, I believe that creating a Pull Request with > > > > >>> > > > > >>> multiple > > > > >>> > > > > >>> commits is > > > > >>> > > > > >>> ok. > > > > >>> > > > > >>> It's just in the end that when it's being pushed to > > > > >>> > > > > >>> develop > > > > >>> > > > > >>> branch, > > > > >>> > > > > >>> it > > > > >>> > > > > >>> needs to be squash merged. I believe that is what > > > > >>> > > > > >>> you > > > > >>> > > > > >>> have > > > > >>> > > > > >>> mentioned > > > > >>> > > > > >>> in > > > > >>> > > > > >>> the > > > > >>> > > > > >>> first paragraph, and I am more than happy with that. > > > > >>> If you can see in the first screenshot comparison > > > > >>> > > > > >>> that > > > > >>> > > > > >>> I > > > > >>> > > > > >>> had > > > > >>> > > > > >>> attached > > > > >>> > > > > >>> in > > > > >>> > > > > >>> the first email in this thread is what I want to > > > > >>> > > > > >>> avoid. > > > > >>> > > > > >>> > > > > >>> Thank you for your feedback. > > > > >>> > > > > >>> Regards > > > > >>> Naba > > > > >>> > > > > >>> > > > > >>> On Mon, Dec 16, 2019 at 9:47 AM Kirk Lund < > > > > >>> > > > > >>> kl...@apache.org> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> Whenever I submit a PR with multiple commits that I > > > > >>> > > > > >>> intend > > > > >>> > > > > >>> to > > > > >>> > > > > >>> rebase > > > > >>> > > > > >>> to > > > > >>> > > > > >>> develop, I always try to ensure that each commit > > > > >>> > > > > >>> stands > > > > >>> > > > > >>> alone > > > > >>> > > > > >>> as > > > > >>> > > > > >>> is > > > > >>> > > > > >>> (compiles and passes tests). Separating file > > > > >>> > > > > >>> renames > > > > >>> > > > > >>> and > > > > >>> > > > > >>> refactoring > > > > >>> > > > > >>> from > > > > >>> > > > > >>> behavior changes into different commits seems very > > > > >>> > > > > >>> valuable > > > > >>> > > > > >>> to > > > > >>> > > > > >>> me, > > > > >>> > > > > >>> and > > > > >>> > > > > >>> I've > > > > >>> > > > > >>> had trouble getting people to review PRs without > > > > >>> > > > > >>> this > > > > >>> > > > > >>> separation > > > > >>> > > > > >>> (but > > > > >>> > > > > >>> it > > > > >>> > > > > >>> could be squashed as it's merged to develop). > > > > >>> > > > > >>> It sounds to me like the real problem is (a) some > > > > >>> > > > > >>> PRs > > > > >>> > > > > >>> have > > > > >>> > > > > >>> multiple > > > > >>> > > > > >>> commits > > > > >>> > > > > >>> that don't compile or don't pass tests, and (b) > > > > >>> > > > > >>> some > > > > >>> > > > > >>> PRs > > > > >>> > > > > >>> that > > > > >>> > > > > >>> should > > > > >>> > > > > >>> be > > > > >>> > > > > >>> merged with squash are not (by accident most > > > > >>> > > > > >>> likely). > > > > >>> > > > > >>> > > > > >>> I can submit multiple PRs instead of one PR with > > > > >>> > > > > >>> multiple > > > > >>> > > > > >>> commits. > > > > >>> > > > > >>> So > > > > >>> > > > > >>> I'll > > > > >>> > > > > >>> change my response to -0 if that helps prevent > > > > >>> > > > > >>> commits > > > > >>> > > > > >>> to > > > > >>> > > > > >>> develop > > > > >>> > > > > >>> that > > > > >>> > > > > >>> don't compile or pass tests. Without preventing > > > > >>> > > > > >>> rebase > > > > >>> > > > > >>> or > > > > >>> > > > > >>> merge > > > > >>> > > > > >>> commits > > > > >>> > > > > >>> from github, I'm not sure how we can really enforce > > > > >>> > > > > >>> this > > > > >>> > > > > >>> or > > > > >>> > > > > >>> prevent > > > > >>> > > > > >>> (b) > > > > >>> > > > > >>> above. > > > > >>> > > > > >>> On Fri, Dec 13, 2019 at 3:38 PM Alexander Murmann < > > > > >>> > > > > >>> amurm...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> I wonder if Kirk's and Naba's statements are > > > > >>> > > > > >>> necessarily > > > > >>> > > > > >>> at > > > > >>> > > > > >>> odds. > > > > >>> > > > > >>> Make the change easy (warning: this may be hard), > > > > >>> > > > > >>> then > > > > >>> > > > > >>> make > > > > >>> > > > > >>> the > > > > >>> > > > > >>> easy > > > > >>> > > > > >>> change." > > > > >>> > > > > >>> -Kent Beck > > > > >>> > > > > >>> Following Kent Beck's advise might reasonably > > > > >>> > > > > >>> split > > > > >>> > > > > >>> into > > > > >>> > > > > >>> two > > > > >>> > > > > >>> commits. > > > > >>> > > > > >>> One > > > > >>> > > > > >>> refactor commit and a separate commit that > > > > >>> > > > > >>> introduces > > > > >>> > > > > >>> the > > > > >>> > > > > >>> actual > > > > >>> > > > > >>> change. > > > > >>> > > > > >>> They should be individually revertible and might > > > > >>> > > > > >>> be > > > > >>> > > > > >>> easier > > > > >>> > > > > >>> understood > > > > >>> > > > > >>> if > > > > >>> > > > > >>> split out. I vividly remember a change on our code > > > > >>> > > > > >>> base > > > > >>> > > > > >>> where > > > > >>> > > > > >>> someone > > > > >>> > > > > >>> did a > > > > >>> > > > > >>> huge amount of refactoring that resulted than in > > > > >>> > > > > >>> one > > > > >>> > > > > >>> parameter > > > > >>> > > > > >>> changing > > > > >>> > > > > >>> in > > > > >>> > > > > >>> order to get the desired functionality change. If > > > > >>> > > > > >>> that > > > > >>> > > > > >>> was > > > > >>> > > > > >>> in > > > > >>> > > > > >>> one > > > > >>> > > > > >>> commit, > > > > >>> > > > > >>> it would be hard to see the actual change. If > > > > >>> > > > > >>> split > > > > >>> > > > > >>> out, > > > > >>> > > > > >>> it's > > > > >>> > > > > >>> beautiful > > > > >>> > > > > >>> and > > > > >>> > > > > >>> crystal clear. > > > > >>> > > > > >>> I am unsure how that would be reflected in terms > > > > >>> > > > > >>> of > > > > >>> > > > > >>> JIRA > > > > >>> > > > > >>> ticket > > > > >>> > > > > >>> references. > > > > >>> > > > > >>> Usually we assume that if there is a commit with > > > > >>> > > > > >>> the > > > > >>> > > > > >>> ticket > > > > >>> > > > > >>> number, > > > > >>> > > > > >>> the > > > > >>> > > > > >>> issue is resolved. Maybe the key here is to create > > > > >>> > > > > >>> a > > > > >>> > > > > >>> separate > > > > >>> > > > > >>> refactoring > > > > >>> > > > > >>> ticket. > > > > >>> > > > > >>> Would that allow us to have our cake and eat it > > > > >>> > > > > >>> too? > > > > >>> > > > > >>> > > > > >>> On Fri, Dec 13, 2019 at 3:16 PM Nabarun Nag < > > > > >>> > > > > >>> n...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> It is to help with bisect operations when things > > > > >>> > > > > >>> start > > > > >>> > > > > >>> failing > > > > >>> > > > > >>> ... > > > > >>> > > > > >>> helps > > > > >>> > > > > >>> us > > > > >>> > > > > >>> it revert and build faster. > > > > >>> also with cherry picking features / fixes to > > > > >>> > > > > >>> previous > > > > >>> > > > > >>> versions > > > > >>> > > > > >>> . > > > > >>> > > > > >>> And keeping the git history clean with no > > > > >>> > > > > >>> unnecessary > > > > >>> > > > > >>> “merge > > > > >>> > > > > >>> commits”. > > > > >>> > > > > >>> Regards > > > > >>> Naba > > > > >>> > > > > >>> > > > > >>> On Fri, Dec 13, 2019 at 2:25 PM Kirk Lund < > > > > >>> > > > > >>> kl...@apache.org> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> -1 I really like to sometimes have more than 1 > > > > >>> > > > > >>> commit > > > > >>> > > > > >>> in > > > > >>> > > > > >>> a > > > > >>> > > > > >>> PR > > > > >>> > > > > >>> and > > > > >>> > > > > >>> keep > > > > >>> > > > > >>> them > > > > >>> > > > > >>> separate when they merge to develop > > > > >>> > > > > >>> On Tue, Oct 22, 2019 at 5:12 PM Nabarun Nag < > > > > >>> > > > > >>> n...@pivotal.io> > > > > >>> > > > > >>> wrote: > > > > >>> > > > > >>> Hi Geode Committers, > > > > >>> > > > > >>> A kind request for using squash commit instead > > > > >>> > > > > >>> of > > > > >>> > > > > >>> using > > > > >>> > > > > >>> merge. > > > > >>> > > > > >>> This will really help us in our bisect > > > > >>> > > > > >>> operations > > > > >>> > > > > >>> when a > > > > >>> > > > > >>> regression/flakiness in the product is > > > > >>> > > > > >>> introduced. > > > > >>> > > > > >>> We > > > > >>> > > > > >>> can > > > > >>> > > > > >>> automate > > > > >>> > > > > >>> and > > > > >>> > > > > >>> go > > > > >>> > > > > >>> through fewer commits faster, avoiding commits > > > > >>> > > > > >>> like > > > > >>> > > > > >>> "spotless > > > > >>> > > > > >>> fix" > > > > >>> > > > > >>> and > > > > >>> > > > > >>> "re-trigger precheck-in" or other minor commits > > > > >>> > > > > >>> in > > > > >>> > > > > >>> the > > > > >>> > > > > >>> merged > > > > >>> > > > > >>> branch. > > > > >>> > > > > >>> Also, please use the commit format : (helps us > > > > >>> > > > > >>> to > > > > >>> > > > > >>> know > > > > >>> > > > > >>> who > > > > >>> > > > > >>> worked > > > > >>> > > > > >>> on > > > > >>> > > > > >>> it, > > > > >>> > > > > >>> what is the history) > > > > >>> > > > > >>> > > > > >>> > > > > >>> * GEODE-xxxx: <brief intro > > > > > >>> * explanation line 1 > > > > >>> > > > > >>> * > > > > >>> > > > > >>> explanation > > > > >>> > > > > >>> line > > > > >>> > > > > >>> 2* > > > > >>> > > > > >>> This is not a rule or anything, but a request > > > > >>> > > > > >>> to > > > > >>> > > > > >>> help > > > > >>> > > > > >>> out > > > > >>> > > > > >>> your > > > > >>> > > > > >>> fellow > > > > >>> > > > > >>> committers in quickly detecting a problem. > > > > >>> > > > > >>> For inspiration, we can look into Apache Kafka > > > > >>> > > > > >>> / > > > > >>> > > > > >>> Spark > > > > >>> > > > > >>> where > > > > >>> > > > > >>> they > > > > >>> > > > > >>> have > > > > >>> > > > > >>> a > > > > >>> > > > > >>> complete linear graph for their main branch > > > > >>> > > > > >>> HEAD > > > > >>> > > > > >>> [see > > > > >>> > > > > >>> attachment] > > > > >>> > > > > >>> Regards > > > > >>> Naba. > > > > >>> > > > > >>> > > > > >>> > > > > >>> -- > > > > >>> > > > > >>> Ju@N > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> -- > > > > >>> Cheers > > > > >>> > > > > >>> Jinmei > > > > >> > > > > >> > > > > > > > > > > > > > >
