On 24 May 2013 13:00, Johan Hake <[email protected]> wrote: > Lawrence! > > Thanks for this write up. We will try publish a version of this > description on fenicsproject.org for best git practice. > > On 05/24/2013 01:47 PM, Martin Sandve Alnæs wrote: >> I think it's best to avoid squashing and other commit id modifying tasks >> after the branch has been made public. That's my understanding of where >> this went wrong, but I must admit I haven't looked in much detail. If no >> commit ids are rewritten, fixes can be commited to both >> of dolfin.git/feature-branch and jrandom-developer.git/feature-__branch >> and merged between them, then merged into next again and finally into >> master, I see no issues then. > > That was my impression too. > >> Martin >> >> >> On 24 May 2013 13:35, Lawrence Mitchell <[email protected] >> <mailto:[email protected]>> wrote: >> >> Dear all, >> >> I recently contributed some code to dolfin which raised some >> questions about how best to carry out pull request/code review and >> subsequent merging. As an outside developer (with no access to the >> main dolfin repository) here is how I envisaged the process working: >> >> 1. Fork dolfin.git >> >> 2. Hack away implementing new feature. > > Should add that this should be done in its own feature branch. > >> 3. When ready submit as pull request. >> >> 4. Respond to review comments by making changes in my branch. >> >> Where appropriate, these would either be as new commits, or >> fixups squashed into existing commits (perhaps fixing a typo). Like >> a good git citizen, I rerolled my patches in response to review >> comments. > > Why is it bad git habit to push small typo commits? Especially when it > potentially will screw the history compared to an already published and > pushed branch on the main dolfin repository? >
People have mixed views on this. I think it's fine to manipulate the history up to the point that the feature branch gets pulled into the main repo. >> 5. Force push latest version out for people to look at. >> >> 6. When everyone is happy, push final version ready for merging. >> >> This seems to conflict slightly with the integration testing that >> maintainers are doing, as a result of which, bitbucket got in a >> terrible tizz (see discussion here >> >> https://bitbucket.org/fenics-__project/dolfin/pull-request/__24/expose-petsc-objects-to-__pydolfin-using/activity >> >> <https://bitbucket.org/fenics-project/dolfin/pull-request/24/expose-petsc-objects-to-pydolfin-using/activity>). >> >> AIUI, the dolfin maintainers do something like the following: > > I change this to: > > 1. Fetch feature branch into local repo > > 2. Test the feature branch locally > And possibly make fixes, improve formatting, etc. > 3. Push to main repo > > 4. If there are issues, either ask jrandom to update his > branch or just fix it in the feature branch > Try to do 4 before 3. > 5. Pull any new fixes from jrandoms repor > > 6. Graduate to next > Try to wait on fixes before going to next. Garth > 7. Do 4-6 until satisfaction. > > 8. Graduate to master > > Johan > >> 2. Merge feature branch to next and test >> >> 3. Fix any minor issues with feature branch >> >> 4. Eventually merge to master >> >> >> This works as long as, before the merge to master, >> dolfin.git/feature-branch and jrandom-developer.git/feature-__branch >> agree on the commit ids. Remember, bitbucket still thinks the merge >> is coming from the latter repository. >> >> So the problem seemed to arise in the interaction between my feature >> branch, and the feature branch pulled in to the main repository. I >> was expecting that it would be reset to whatever I had pushed out in >> response to review, whereas this didn't happen remotely. >> >> Is there any strong feeling and/or guidance on what I should have done? >> >> Lawrence >> >> -- >> The University of Edinburgh is a charitable body, registered in >> Scotland, with registration number SC005336. >> >> _________________________________________________ >> fenics mailing list >> [email protected] <mailto:[email protected]> >> http://fenicsproject.org/__mailman/listinfo/fenics >> <http://fenicsproject.org/mailman/listinfo/fenics> >> >> >> >> >> _______________________________________________ >> fenics mailing list >> [email protected] >> http://fenicsproject.org/mailman/listinfo/fenics >> > > _______________________________________________ > fenics mailing list > [email protected] > http://fenicsproject.org/mailman/listinfo/fenics _______________________________________________ fenics mailing list [email protected] http://fenicsproject.org/mailman/listinfo/fenics
