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