← Back to team overview

openerp-community team mailing list archive

Re: "Merge pull request" considered harmful

 

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
2) the maintainer does the cleanup himself

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.

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
>

Follow ups

References