[Insight-developers] Learning from my mistakes (an adventure in git)

Gaëtan Lehmann gaetan.lehmann at jouy.inra.fr
Mon Aug 2 13:29:06 EDT 2010


Le 2 août 10 à 17:19, Brad King a écrit :

> On 08/02/2010 10:51 AM, Gaëtan Lehmann wrote:
>> Really useful for sure!
>
> Thanks.  We went through several iterations of local hook installation
> enforcement in VTK before arriving at this solution.  The earlier
> attempts generated a CMake-time error so even non-developers could
> not build without installing the hooks.  This is much simpler.

sure, that's better that way

>
>> Would it be possible to fetch the hooks automatically (at least try
>> to, if the network in not available at this time)?
>
> Perhaps, but IMO it is more trouble than it's worth.  The goal is to
> make it hard to commit accidentally without the local hooks installed.
> Any automatic steps to enforce this should have as few failure points
> as possible.
>
> Git intentionally does not allow clones to set up hooks automatically,
> citing security reasons.  Therefore the earliest step we can control
> is when CMake runs.
>
> The "hooks" branch is shared by many Kitware-hosted projects.  To
> avoid duplication we do not want to copy hooks into every project's
> source tree.  Therefore we cannot install all the hooks by copying
> them at CMake configuration time.  We also want to avoid blowing
> away any local modifications the user has made to the local hooks
> so we cannot just overwrite them with new ones every time CMake runs.
>
> We *cannot* run Git during CMake configuration because we might not
> know where to find the Git executable or which one was used if  
> multiple
> are installed (MSysGit and Cygwin git do not interact very well in a
> single work tree).  Instead we write a small .git/hooks/pre-commit
> file as a bootstrapping step.  If the user never does development then
> it has no effect, but when the user tries to commit then it blocks
> the commit and prints the instructions.
>
> The only place we could try to run git to automatically install the
> hooks is in the bootstrapping pre-commit hook.  Inside this script we
> can just run "git" and do not need to find the executable.  However,
> installing the real hooks involves replacing pre-commit while it is
> open (running), which may not work on Windows.  If it fails the hooks
> directory may be left in a confusing state.  It is much simpler to
> give the user one-time cut-and-paste instructions as we do now, and
> it only happens on the first *commit* anyway.

Ok, thanks for the detailed explanation!

>
>> I don't see any style check for now. Is it the expected behavior?
>
> Yes.  We cannot run style checks on the server anymore, at least not
> without adding quite a bit of customized infrastructure code.  We
> cannot enforce style in the local hooks unless we know where to find
> a local installation of KWStyle.  These hooks should be kept as
> simple as possible to ensure they run properly everywhere.  Also,
> I frequently commit work-in-progress changes locally as a backup
> of my work tree.  I would not want this blocked by style checks.
>
> IMO the style checks should be *tests* just like any other.  If the
> style is bad the test fails.  If you push a change that introduces
> a test failure, then shame on you ;)
>

It is not as strong as a commit check, but with the reformating tools  
discussed during the last tcon, fixing the broken style should be  
quite simple, so I guess it can be an acceptable solution.

Gaëtan


-- 
Gaëtan Lehmann
Biologie du Développement et de la Reproduction
INRA de Jouy-en-Josas (France)
tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
http://voxel.jouy.inra.fr  http://www.itk.org
http://www.mandriva.org  http://www.bepo.fr

-------------- next part --------------
A non-text attachment was scrubbed...
Name: PGP.sig
Type: application/pgp-signature
Size: 203 bytes
Desc: Ceci est une signature ?lectronique PGP
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20100802/1a29d757/attachment.pgp>


More information about the Insight-developers mailing list