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