Aaron , Is it not the case currently? If I am working on a feature modifying class X and another developer makes some refactoring changes on class X and pushes it to develop, won't I have to resolve the merge commits anyway.
Regards Naba On Tue, Dec 31, 2019 at 6:01 PM Aaron Lindsey <aaronlind...@apache.org> wrote: > Suppose I’m refactoring the same classes I’m touching for the feature. I > do the refactoring on one branch and submit a PR for that. Then I implement > the feature on another branch which is based on develop and does not have > the refactoring changes since those are not merged yet. I’ll have to > resolve conflicts on the second PR that I merge. > > Having interdependent PRs where one PR branch is based on another PR > branch is not something I’ve tried. That requires more overhead in having > to create more PRs and branches. And if you have N interdependent PRs, you > have to do N-1 merges each time a PR is merged to develop (to update the > other branches). It could be done, but is it worth the hassle? > > One more point about rebase-and-merge is that it seems like this would > make bisecting a failure easier since there would be smaller commits. > > Aaron > > > On Dec 31, 2019, at 5:41 PM, Owen Nichols <onich...@pivotal.io> wrote: > > > > Can you elaborate on why you would have to deal with conflicts if the > > refactoring work was kept as a separate PR from the fix? > > > > As with many git workflows, there’s an easy way and a hard way to manage > > interdependent PRs (tl;dr only merge, never rebase!). I wonder if this > > points out an opportunity for a “tips and tricks” page on the Geode wiki. > > > > On Tue, Dec 31, 2019 at 5:22 PM Aaron Lindsey <aaronlind...@apache.org> > > wrote: > > > >> -0.9 > >> > >> I’m not in favor of the revised proposal that disallows > rebase-and-merge. > >> Say I am working on a PR and I have a refactor commit and another commit > >> which implements a new feature. I don’t want those commits to get > squashed > >> because that makes it hard to understand the diff. However, if I make > those > >> commits as two separate PRs then I am going to have to deal with > conflicts. > >> > >> I’m not sure when we made it a rule that every commit in develop had to > >> compile and pass tests. I know we implemented a rule that all PRs had to > >> pass certain checks, but I never thought that rule implied all commits > had > >> to pass those checks. > >> > >> In general I just don’t see the problem with rebase-and-merge and this > >> feels like an unnecessary restriction, but I will go with it if that’s > what > >> everyone wants to do. > >> > >> Aaron > >> > >>> On Dec 31, 2019, at 3:09 PM, Owen Nichols <onich...@pivotal.io> wrote: > >>> > >>> To recap, this proposal is now revised to remove 2 of the 3 merge > options > >>> from GitHub, leaving only Squash and Merge. PR #4513 > >>> <https://github.com/apache/geode/pull/4513> serves as an exhibit of > >> what is > >>> proposed (it is not to be merged unless discussion leads to consensus > >> and a > >>> successful vote). Please use the dev list (not the PR) for all > >> discussion > >>> (and voting, if we get that far). > >>> > >>> Squash and merge is already used almost exclusively by the Geode > >> community, > >>> with any exceptions tending to be accidental more often than > intentional. > >>> However, some have raised the concern that implementing this > restriction > >>> could result in harm or wasted time. Can someone give an example of a > >> such > >>> a scenario? > >>> > >>> It seems there is a divide here between junior and senior community > >>> members. Newer committers appreciate additional guardrails to protect > >> them > >>> from accidentally doing the wrong thing, while those with more > experience > >>> want to be able to work unencumbered by restrictions of any kind. > >>> > >>> Our welcome email to new committers states “We like to work on trust > >> rather > >>> than unnecessary constraints...Being a committer enables you to more > >> easily > >>> make changes without needing to go through the patch submission > process”. > >>> I can see this as an argument against this proposal (perhaps even an > >>> argument against any form of branch protection). > >>> > >>> In the scheme of things, this proposal makes very little difference. > >> There > >>> are still other ways to get non-compiling commits onto develop (e.g. > >>> waiting a long time between running PR checks and merging to develop). > >>> What’s more important is working well together as a community. So, > >> perhaps > >>> what’s best for the community is to encourage working on trust rather > >> than > >>> unnecessary constraints. > >>> > >>> -Owen > >>> > >>> On Dec 31, 2019, at 10:56 AM, Kirk Lund <kl...@apache.org> wrote: > >>> > >>> I'm happy to file multiple PRs when I need to merge multiple commits to > >>> develop. > >>> > >>> On Mon, Dec 30, 2019 at 5:45 PM Mark Hanson <mhan...@pivotal.io> > wrote: > >>> > >>> This change to disable all but squash-merge would be really easy to > >>> revert. How about we try it for a while and see? If people decide it is > >>> really limiting them, we can change it back. Let’s do it for 1 month > and > >>> see how it goes. Does that sound reasonable? > >>> > >>> Thanks, > >>> Mark > >>> > >>> On Dec 30, 2019, at 5:25 PM, Owen Nichols <onich...@pivotal.io> wrote: > >>> > >>> Given that we adopted < > >>> > >>> > >> > https://lists.apache.org/thread.html/c3eb5c028cb3a4d76024f928a7a33c0311228f5dbbcaa86287bf5826@%3Cdev.geode.apache.org%3E > >>>> > >>> and still wish to continue < > >>> > >> > https://lists.apache.org/thread.html/8795697c1ad57068c053b48b4b1845005f3ade0be777e679eafe95db@%3Cdev.geode.apache.org%3E > >>>> > >>> having branch protection rules to ensure every commit landing in > develop > >>> has passed the required PR checks, it seems like that decision alone > >>> mandates that we disable all merge mechanisms other than > >> squash-and-merge. > >>> > >>> > >>> Allowing merge commits or rebase merges bypasses branch protection for > >>> > >>> all commits other than the final one in the merge or rebase set. Given > >>> that we decided < > >>> > >> > https://lists.apache.org/thread.html/1ba19d9aeb206148c922afdd182ba322d6f128bbb83983f2f72a108e@%3Cdev.geode.apache.org%3E > >>>> > >>> that bypassing PR checks should never be allowed, keeping this loophole > >>> open seems untenable. > >>> > >>> > >>> This is not just hypothetical — this loophole is causing real problems. > >>> > >>> We now have commits on develop that don’t compile. For example: > >>> > >>> git checkout 19eee7821322a1301f16bdcd31fd3b8c872a41b6 > >>> ./gradlew devBuild > >>> ...spotlessJava FAILED > >>> We implemented branch protections to make this impossible, right? > >>> > >>> We can very easily close this loophole by disabling all but the > >>> > >>> Squash&Merge button for PRs. This will not make more work for any > >>> developer. If you want to get multiple commits onto develop, simply > >> submit > >>> each as a separate PR — that is the only way to assure that required PR > >>> checks have passed for each. > >>> > >>> > >>> On the other hand, if we as a Geode community feel strongly that > >>> > >>> bypassing branch protection via merge commits and rebase commits is > >>> important to allow, why not also allow arbitrary overrides (or get rid > of > >>> branch protection entirely)? > >>> > >>> > >>> -Owen > >>> > >>> On Dec 20, 2019, at 12:31 PM, Blake Bender <bben...@pivotal.io> wrote: > >>> > >>> Just FWIW, the situation described of having multiple commits in a > >>> > >>> single > >>> > >>> PR with separate associated JIRA tickets is still kind of problematic. > >>> > >>> It > >>> > >>> could well be the case that the commits are interdependent, thus when > >>> bisecting etc it's still not possible to revert the fix for a single > >>> bug/feature/whatever atomically. It's all good, though, I'm satisfied > >>> > >>> no > >>> > >>> one's forcing me to adopt practice I'm opposed to. Apologies for > >>> > >>> getting > >>> > >>> my feathers a little ruffled, or if I ruffled anyone else's in return. > >>> > >>> Thanks, > >>> > >>> Blake > >>> > >>> > >>> On Fri, Dec 20, 2019 at 12:18 PM Nabarun Nag <n...@pivotal.io> wrote: > >>> > >>> Hi Dan, > >>> > >>> When we do squash merge all the commit messages are preserved and also > >>> > >>> the > >>> > >>> co-authored tag works when we do squash merge. > >>> So the authorship and history are preserved. > >>> > >>> In my own personal experience and reverts and pinpointing regression > >>> failures are tough when the commits are spread out. Also, reverts are > >>> easier when it is just one commit while we are bisecting failures. > >>> > >>> > >>> Regards > >>> Naba > >>> > >>> On Fri, Dec 20, 2019 at 12:07 PM Dan Smith <dsm...@pivotal.io> wrote: > >>> > >>> I'll change to -0. > >>> > >>> I think merge commits are a nice way to record history in some cases, > >>> > >>> and > >>> > >>> can also be a way to avoid messy conflicts that come with rebase. > >>> > >>> Merge > >>> > >>> commits also preserve authorship of commits (compared to > >>> > >>> squash-merge), > >>> > >>> which I think is valuable as an open source community that is trying > >>> > >>> to > >>> > >>> keep track of outside contributions. > >>> > >>> That said, if the rest of y'all feel it will help to disable the > >>> > >>> button, > >>> > >>> I > >>> > >>> won't stand in the way. > >>> > >>> -Dan > >>> > >>> On Fri, Dec 20, 2019 at 11:50 AM Anthony Baker <aba...@pivotal.io> > >>> > >>> wrote: > >>> > >>> > >>> Whether we are talking about the geode/ repository or the > >>> > >>> geode-native/ > >>> > >>> repository we are all one GEODE community. > >>> > >>> The idea of a native client team may matter in some contexts but in > >>> > >>> this > >>> > >>> space we are all GEODE contributors. > >>> > >>> Adopting a common approach across our repos will make it easier for > >>> > >>> new > >>> > >>> contributors to engage, learn our norms, and join our efforts. > >>> > >>> $0.02, > >>> Anthony > >>> > >>> > >>> On Dec 20, 2019, at 11:32 AM, Blake Bender <bben...@pivotal.io> > >>> > >>> wrote: > >>> > >>> > >>> Is this a policy the native client team must abide by, as well? It > >>> > >>> varies > >>> > >>> slightly with what we've been doing, and all other things being > >>> > >>> equal I > >>> > >>> see > >>> > >>> no reason for us to change that. If I am to have any measure of > >>> > >>> control > >>> > >>> over the nc repository, I will definitely enforce a 1:1 > >>> > >>> correspondence > >>> > >>> between commits to develop and JIRA tickets. IMO, if your > >>> > >>> refactoring > >>> > >>> in a > >>> > >>> PR is sufficiently large or complex that it's difficult to tease it > >>> > >>> out > >>> > >>> from the bug you're fixing or feature you're implementing, it merits > >>> > >>> its > >>> > >>> own JIRA ticket and a separate PR. If your "actual" fix then > >>> > >>> becomes > >>> > >>> dependent on the refactored code, that's a price I'm willing to pay > >>> > >>> to > >>> > >>> keep > >>> > >>> history clean. > >>> > >>> On the other hand, I see no real value in squashing to a single > >>> > >>> commit > >>> > >>> prior to submitting a PR, since your view of the changes on GitHub > >>> > >>> is > >>> > >>> essentially the same either way. We haven't enforced this on the nc > >>> > >>> repo, > >>> > >>> and I'd prefer to keep it that way. > >>> > >>> Thanks, > >>> > >>> Blake > >>> > >>> > >>> On Fri, Dec 20, 2019 at 10:29 AM Jinmei Liao <jil...@pivotal.io> > >>> > >>> wrote: > >>> > >>> > >>> 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 > >> > >> > >
