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? > 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 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 5. Pull any new fixes from jrandoms repor 6. Graduate to next 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
