I'm a proponent of using squash-and-merge, and once a person has chosen this option once it comes up by default afterwards...
Owen, I don't think you have consensus to put forth this PR, there are -1s above... (early voting) maybe we'll be better off socializing the norm of squash-and-merge and gaining a natural consensus that way... On Fri, Dec 20, 2019 at 10:07 AM Owen Nichols <onich...@pivotal.io> wrote: > The proposed action manifests as a commit to the Geode git repository, so > a PR is an acceptable vehicle for voting in this case. > > > On Dec 20, 2019, at 9:38 AM, Bruce Schuchardt <bschucha...@pivotal.io> > wrote: > > > > I see a lot of plus-ones and a "voting deadline" on this DISCUSS thread > and a request to "vote" using a PR. This all seems out of order to me. > Our votes are supposed to be on the email list, aren't they? and I haven't > seen a VOTE request. > > > > On 12/20/19 9:33 AM, Nabarun Nag wrote: > >> +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 > >>> > >
