I'm also using Blake's approach when dealing with refactoring + code changes, it's clear and it never caused me any troubles so far. +1 to to require linear commit history on develop. Cheers.
On Thu, Jan 2, 2020 at 6:03 PM Mark Hanson <mhan...@pivotal.io> wrote: > This is the approach I am using as well Blake. > > Thanks, > Mark > > > On Jan 2, 2020, at 7:16 AM, Blake Bender <bben...@pivotal.io> wrote: > > > > That's not how I approach this sort of a change, and to my mind it feels > > sort of like that approach is asking for trouble. When I have a refactor > > plus a code change, I do the refactor on a branch, submit the PR, then > > branch off of *that* branch to do the feature work. This forces an > > ordering of the changes, but alleviates most/all of the conflicts. When > > the refactor PR is accepted, I rebase the feature work off of the new > > develop branch, and submit the second PR, usually without encountering > any > > issues. > > > > > > > > On Wed, Jan 1, 2020 at 9:57 AM Aaron Lindsey <aaronlind...@apache.org> > > wrote: > > > >>> > >>> 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 > >>>>>>>> > >>>>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > -- Ju@N
