Yes, I got freaked out somehow this morning by thinking I had accidentally clicked the wrong button to include all the individual commits in my PR to the develop branch, (luckily it's just a wrong view). I would think disable that button would be a good idea.
On Thu, Dec 19, 2019 at 5:04 PM Nabarun Nag <n...@pivotal.io> wrote: > Hi all, > > This is not a blanket restriction. > My suggestion, like last time, lets try it out and this time we don't even > need Apache Infra to step in. Feels like it has been requested in multiple > INFRA JIRAs by so many Apache projects to enable only the squash merge > button and disable the others, that this setting has been moved to a simple > entry in .asf.yml file in root git repository > > Again this is a setting only for the develop branch. And can be reversed > with a simple commit. > > I agree too much policing is a bad thing, but I want to look at it as a > safeguard. I have spoken to multiple developers who have mentioned they > clicked the wrong button in a hurry or the page refreshed and the commit > got merged in a wrong way. > > Regards > Naba > > Reference : > > https://cwiki.apache.org/confluence/display/INFRA/.asf.yaml+features+for+git+repositories > Merge buttons > > Projects can enable/disable the "merge PR" button in the GitHub UI and > configure which actions should be allowed by adding the following > configuration (or derivatives thereof): > github: > enabled_merge_buttons: > # enable squash button: > squash: true > # enable merge button: > merge: true > # disable rebase button: > rebase: false > > > On Thu, 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. > > >>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>> > > >>>> > > >>>> > > >>> > > > > > -- Cheers Jinmei
