On 05/24/2013 03:14 PM, Lawrence Mitchell wrote: > On 24/05/2013 13:00, Johan Hake wrote: > ... > >>> On 24 May 2013 13:35, Lawrence Mitchell <[email protected] >>> <mailto:[email protected]>> wrote: >>> > > ... > > >>> 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? > > As Garth says, there are mixed views. One argument in favour of > rerolling over pushing new commits on top is that you would like every > commit that is merged into master to be buildable and passing the test > suite. That way bisection to find regressions can be entirely automated. > > Imagine the following scenario: > > Developer commits: > > #ifdef SOME_FEATURE > do_stuff(); > #else > do_other_stuff() > #endif > > But only ever test-compiled with SOME_FEATURE enabled, and therefore > didn't notice that the do_other_stuff line was missing a semi colon. > > Now, this will be spotted in review and they have two choices. > > 1. Fix up the error in a separate commit > 2. Fix the error and squash it into the original commit (so that it > looks like it never happpened). > > In the former case we might have something like: > > A---B---C---D---E---fixup > ^ ^ > error is here fix is here > > In the latter: > > A'---B'---C'---D'---E' > ^ > no error here > > Now if the first case is merged, an automatic bisection to find a bug > might spuriously stop at A (because the build doesn't work), needing > manual intervention to fix. In the latter, that wouldn't happen.
Thanks for the clarification. I think that is a good practice. We just need to have the same practice for these cases. I will try to stitch this together in a FEniCS version of PETSc's gitworkflow. Just to be clear, if I would have squashed my small fixes in the mentioned issue and then published to the main repo, and you would have followed up by pulling these fixes, bitbucket would got it right? Johan > Cheers, > > Lawrence > _______________________________________________ fenics mailing list [email protected] http://fenicsproject.org/mailman/listinfo/fenics
