This proposal in no way prevents you from getting work done. Squash is always enabled and always the most acceptable way to bring changes to develop.
Please start a separate thread if you would like to revisit the community decision to require passing PR checks. > On Dec 19, 2019, at 4:49 PM, Jacob Barrett <jbarr...@pivotal.io> 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. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>> >>>> >>>> >>>
