@onichols Correction: The 'git-flow' workflow that Geode follows is to merge a release commit to master and to develop.
On Thu, Dec 19, 2019 at 4:51 PM Owen Nichols <onich...@pivotal.io> wrote: > Dan, to address some of your concerns: > > This proposal is to block merge commit on develop only. The release > process makes a merge commit to master only. > > This proposal explicitly allows reasonably separate and well written > commits in a single PR. For this case simply choose Rebase & Merge, and > all of the commits in the PR will be brought to develop individually (no > merge commit wrapping them). > > If you still feel strongly that merge commits should be allowed on > develop, would you consider approving 1) to prevent accidental GitHub > accidents, but veto only 2) to allow merge commits to be explicitly created > manually when explicitly desired? > > > 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. > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> > >>>>> > >>> > >>> > >> > >
