Merge commit is the new contributor's default setting. When we are merging new contributor's PR, since we are so used to THINKING "squash-and-merge" is the default, we forgot to check what the button really says when we are merging other people's PR.
On Fri, Dec 20, 2019 at 10:18 AM Ernest Burghardt <eburgha...@pivotal.io> wrote: > 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 > > >>> > > > > > -- Cheers Jinmei
