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 > >>>>>>> > >>>>> > >>>>> > >>>> > >>> > > > >
