← Back to team overview

openerp-community team mailing list archive

Re: "Merge pull request" considered harmful

 

On rebase vs pull requests, this is a good read:
http://blogs.atlassian.com/2013/10/git-team-workflows-merge-or-rebase/
Notice the difference between rebase as local cleanup and rebase as team
policy.


2014-07-03 16:13 GMT+02:00 Ovnicraft <ovnicraft@xxxxxxxxx>:

>
>
>
> On Thu, Jul 3, 2014 at 7:36 AM, Raphael Valyi <rvalyi@xxxxxxxxx> wrote:
>
>> Hello,
>>
>> +1
>> for what Leornardo said.
>>
>> There are huge Github projects such as Rails that use these merge buttons
>> just fine and are some of the most successful projects around...
>>
>> We should free maintainers from work, who already give their time for
>> free, even if that means putting more burden of the guys who contribute.
>> In any case Github is lowering the contribution barrier a lot compared to
>> Launchpad.
>>
>> So I would say if a PR is clean, let's jut allow to merge it with that
>> merge button or git merge.
>>
>> Now, if a PR is not clean,
>> then we still have 2 choices to avoid doing ugly merges:
>> 1) ask the contributor to squash or rebase or clean in anyway
>>
>
> Yes please, rebase is the best AFAIK to keep clean PRs, here we are
> working too with edX and that project has a good wiki about rebase
> https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request
>
>
>> 2) the maintainer does the cleanup himself
>>
>
> Would be better if contributor (PR owner) do it.
>
>
>>
>> A concrete example: last time when I submitted a patch to the Gollum
>> project (derivated from github wiki), the maintainer asked me to squash
>> several commits which I did and which was perfectly fine
>> https://github.com/gollum/gollum/pull/787
>> He then probably used that merge button. Everybody fine, no maintenance
>> hassle.
>>
>
> Totally agree here.
>
>>
>>
>
>> In any case we can avoid ugly merges without to have to put all the
>> burden always on the maintainer as Leonardo said.
>>
>>
>> Well, just my opinion.
>>
>>
>> On Thu, Jul 3, 2014 at 9:16 AM, Leonardo Pistone <
>> leonardo.pistone@xxxxxxxxxxxxxx> wrote:
>>
>>> IMHO, what maintainers did on Launchpad was to just to merge, thus
>>> creating one new commit including all the changes in the feature
>>> branch. In addition to that, we could to small modifications ourselves
>>> before pushing to Launchpad.
>>>
>>> On the other hand, git makes it possible to do things that we would
>>> not even dream to do practically with bazaar: an interactive rebase
>>> allows to clean up history doing things like
>>>
>>> - rebasing the history, making it look like history is more linear
>>> than it was at the start
>>> - clean up history by squashing old commits together
>>> - get rid of commits that break the automatic tests, allowing historic
>>> operations like bisect and benchmarks on the history
>>> - change past commit messages to make them better
>>>
>>> and so on.
>>>
>>> Or we can just use the "merge" button on github, that does the same
>>> thing we did manually on Launchpad (clearly without the possibility to
>>> make small changes ourselves).
>>>
>>> So IMO it's good to have some guidelines to contributors (i.e. try to
>>> produce a clean history in their branch) but I would wait a bit before
>>> forcing maintainers to have a complex workflow in all cases.
>>>
>>> In the meantime we can, for example, try to ask kindly to contributors
>>> to improve their feature branches themselves, and use git-rebase-fu
>>> only when necessary.
>>>
>>> On Thu, Jul 3, 2014 at 9:02 AM, Joël Grand-Guillaume
>>> <joel.grandguillaume@xxxxxxxxxxxxxx> wrote:
>>> > I think, after reading carefully, that this is more important than it
>>> seems.
>>> > By increasing the number of project, it's important that such rule are
>>> > given, even if not a constraint for now. But at least we should let
>>> people
>>> > now about.
>>> >
>>> > So I'm in favor of writing this in the docs and howto's. Committers
>>> should
>>> > now that IMO.
>>> >
>>> > Improving the quality is one of our goal, even if not applicable
>>> eveytime by
>>> > lack of time, it should be our goal.
>>> >
>>> >
>>> >
>>> >
>>> > On Thu, Jul 3, 2014 at 12:05 AM, Nhomar Hernández <nhomar@xxxxxxxxx>
>>> wrote:
>>> >>
>>> >> Pedro.
>>> >>
>>> >> It is a "good to have" but I think we can just pull locally test and
>>> then
>>> >> push.
>>> >>
>>> >> If you do this process recommended it ia good too! But IMHO Not the
>>> rule
>>> >> for now.
>>> >>
>>> >> Be careful! The pr button is dangerous without a correct CIS
>>> configured.
>>> >>
>>> >> Written from my android
>>> >>
>>> >> On Jul 2, 2014 3:16 PM, "Pedro Manuel Baeza Romero"
>>> >> <pedro.baeza@xxxxxxxxx> wrote:
>>> >>>
>>> >>> We have to set new contributing HowTos for working with GitHub, and
>>> this
>>> >>> article points out some troubles with Merge PR button, but the way
>>> it says
>>> >>> to merge PR can be very laborious for maintainers. Can we assume
>>> this amount
>>> >>> of extra work? One of the highlights we talk about when the switch
>>> to GitHub
>>> >>> was the Merge PR button.
>>> >>>
>>> >>> Regards.
>>> >>>
>>> >>>
>>> >>> 2014-06-27 14:23 GMT+02:00 Tymoteusz Motylewski
>>> >>> <t.motylewski@xxxxxxxxxxxx>:
>>> >>>>
>>> >>>> +1 for the clean git history. Merge commits are painful.
>>> >>>> With clean commit messages and linear history things like changelog
>>> can
>>> >>>> be easily generated automatically. And by changelog I mean sth
>>> which really
>>> >>>> gives the reader some knowledge.
>>> >>>> See other OSS project TYPO3 Neos for example of such a history
>>> >>>>
>>> >>>>
>>> http://docs.typo3.org/neos/TYPO3NeosDocumentation/Appendixes/ChangeLogs/102.html
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>> 2014-06-27 13:51 GMT+02:00 Lionel Sausin <ls@xxxxxxxxxxxxxxxx>:
>>> >>>>
>>> >>>>> Very interesting reading. I totally support this point of view
>>> even if
>>> >>>>> I'm only a contributor - clean history is hours not lost searching
>>> for
>>> >>>>> stuff. Bisect, annotate, etc all become easier.
>>> >>>>>
>>> >>>>> Le 27/06/2014 12:10, Alexandre Fayolle a écrit :
>>> >>>>>
>>> >>>>>> Hello,
>>> >>>>>>
>>> >>>>>> I found this blog post both interesting and useful :
>>> >>>>>>
>>> >>>>>>
>>> http://blog.spreedly.com/2014/06/24/merge-pull-request-considered-harmful
>>> >>>>>>
>>> >>>>>> Leonardo Pistone had shown me https://github.com/github/hub
>>> before and
>>> >>>>>> it's a nice tool. And since odoo and the community addons are
>>> moving
>>> >>>>>> to
>>> >>>>>> a new platform, I thought I'd share this piece of insight with
>>> all of
>>> >>>>>> you.
>>> >>>>>>
>>> >>>>>
>>> >>>>>
>>> >>>>> _______________________________________________
>>> >>>>> Mailing list: https://launchpad.net/~openerp-community
>>> >>>>> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
>>> >>>>> Unsubscribe : https://launchpad.net/~openerp-community
>>> >>>>> More help   : https://help.launchpad.net/ListHelp
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>> --
>>> >>>>
>>> >>>> Pozdrawiam serdecznie / Best regards
>>> >>>> Tymoteusz Motylewski
>>> >>>> Head of development
>>> >>>> +48 502 472 202
>>> >>>>
>>> >>>> t.motylewski@xxxxxxxxxxxx
>>> >>>>
>>> >>>> http://www.macopedia.pl/
>>> >>>> Ul. Roosevelta 5/8
>>> >>>> 60-829 Poznań
>>> >>>> Tel. +48 61 622 96 95
>>> >>>> Fax +48 61 623 24 48/
>>> >>>> Macopedia Spółka z ograniczoną odpowiedzialnością, z siedzibą ul.
>>> >>>> Roosevelta 5/8; 60-829 Poznań, wpisana do Krajowego Rejestru
>>> Sądowego,
>>> >>>> prowadzonego przez Sąd Rejonowy, VIII Wydział Gospodarczy, KRS
>>> >>>> 0000416744, o opłaconym kapitale zakładowym w wysokości 5.000,00
>>> zł, o
>>> >>>> numerze NIP: 781-187-78-41, numerze Regon 302076699
>>> >>>>
>>> >>>> _______________________________________________
>>> >>>> Mailing list: https://launchpad.net/~openerp-community
>>> >>>> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
>>> >>>> Unsubscribe : https://launchpad.net/~openerp-community
>>> >>>> More help   : https://help.launchpad.net/ListHelp
>>> >>>>
>>> >>>
>>> >>>
>>> >>> _______________________________________________
>>> >>> Mailing list: https://launchpad.net/~openerp-community
>>> >>> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
>>> >>> Unsubscribe : https://launchpad.net/~openerp-community
>>> >>> More help   : https://help.launchpad.net/ListHelp
>>> >>>
>>> >>
>>> >> _______________________________________________
>>> >> Mailing list: https://launchpad.net/~openerp-community
>>> >> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
>>> >> Unsubscribe : https://launchpad.net/~openerp-community
>>> >> More help   : https://help.launchpad.net/ListHelp
>>> >>
>>> >
>>> >
>>> >
>>> > --
>>> >
>>> >
>>> > camptocamp
>>> > INNOVATIVE SOLUTIONS
>>> > BY OPEN SOURCE EXPERTS
>>> >
>>> > Joël Grand-Guillaume
>>> > Division Manager
>>> > Business Solutions
>>> >
>>> > +41 21 619 10 28
>>> > www.camptocamp.com
>>> >
>>> >
>>> >
>>> > _______________________________________________
>>> > Mailing list: https://launchpad.net/~openerp-community
>>> > Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
>>> > Unsubscribe : https://launchpad.net/~openerp-community
>>> > More help   : https://help.launchpad.net/ListHelp
>>> >
>>>
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~openerp-community
>>> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
>>> Unsubscribe : https://launchpad.net/~openerp-community
>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~openerp-community
>> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~openerp-community
>> More help   : https://help.launchpad.net/ListHelp
>>
>>
>
>
> --
>
> [image: Cristian Salamea on about.me]
>
> Cristian Salamea
> about.me/ovnicraft
>     <http://about.me/ovnicraft>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~openerp-community
> Post to     : openerp-community@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~openerp-community
> More help   : https://help.launchpad.net/ListHelp
>
>

References