+1 to (1) and (3) I’m on board with (1). I’m hesitant about agreeing to (2) because it seems harder to “accidentally” do a merge commit via the command line, and I don’t want to add unnecessary restrictions. (3) has needed to be done for some time now, so I’m happy to see a proposal to change that.
In which case would an explicit merge commit be preferred/required to a “rebase and merge”? In my experience working on Geode, I’ve never needed to create an explicit merge commit. I have, however, seen people do it by accident. As a reminder, “rebase and merge" sill allows you to keep all of the individual commits from your PR. - Aaron > 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
