← Back to team overview

openerp-community team mailing list archive

Re: "Merge pull request" considered harmful

 

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>

Follow ups

References