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
