If the appropriate or necessary process is a merge then this proposal prevents that. I am not interested in any hard restrictions like this. You proposal in response to dan is not palatable by disabling merges via github but allowing it manually.
> On Dec 19, 2019, at 4:54 PM, Owen Nichols <onich...@pivotal.io> wrote: > > 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. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>> >>>>> >>>>> >>>> >
