+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
