Re: [dev] Subversion precommit hook
Hi all, Jens-Heiner Rechtien wrote: Hi Bjoern, there was no need for crucifying yourself, server side we are python only. Actually we have no perl bindings installed. I think we need to be a bit carefull with pre-commit hooks. Other than post-commit hooks they do influence the user experience of SVN, so they have to be fast. Well, we've got a very fast server so probably this is not really a problem. I somehow don't like tying SCM functionality to commit messages, but that's may be just me. And should we enforce policy (like tabs vs spaces etc) via the SCM tool? In the past we used a cvs wrapper in Hamburg to prevent at least one of the things that Björn's hook tries to address (accidental commits to wrong branches). And we just had a case where exactly this happened. I don't think that fix it later after someone discovered it is the right way to go here. Then why should we test any CWS before we integrate it? We also always wanted to have a way to prevent mixes of tabs and spaces, we even have a requirement for spaces instead of tabs in our coding guidelines. So why not enforce it when there is a simple way to achieve that? Moreover, the hook solution is even superior to the cvs wrapper as Björn's implementation allows for an override mechanism. And additionally it offers a solution for another problem that we already discovered: the unintentional addition of directories, especially top level directories. I think that both make sense, an intentional adding will happen so rarely that it is bearable to require an additional action. Ciao, Mathias -- Mathias Bauer (mba) - Project Lead OpenOffice.org Writer OpenOffice.org Engineering at Sun: http://blogs.sun.com/GullFOSS Please don't reply to [EMAIL PROTECTED]. I use it for the OOo lists and only rarely read other mails sent to it. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
[dev] Subversion precommit hook
Hi list,
since we now have subversion, we might as well use the new features it
provides. I wrote a precommit hook on the weekend that does some
precommit sanity checks:
- It rejects commits changing files in a cws and outside of it (thus
hopefully preventing some accidental commits to a master
- It checks all *.{cxx|c|hxx|h} files for correct indentation (no tab
indentation on added/changed lines)
- Adding a new module has to be properly announced in the commit message
- Adding a new toplevel dir in a module has to be properly announced in
the commit message (this hopefully prevents accidental commit of
output trees)
- Never allows changes/deleting of tagged versions
I crucified myself to write the hook in perl because I thought it to be
the preferred language for releng stuff (I would have preferred python
myself).
All checks can be disabled with adding special strings to the commit
message.
Im looking forward to comments, ideas and additions. Is it a good idea
to use such a precommit hook? Whats relengs position on this?
Have Fun,
Björn
#!/usr/bin/perl
use strict;
use warnings;
use Getopt::Long;
#global constants
my $SVNLOOK_CMD=svnlook;
#global vars for checks
my $log_message = ; # log message of transaction
my @added_files = (); # files added by the transaction
my @deleted_files = (); # files deleted by the transaction
my @updated_files = (); # files updated by the transaction
my @all_files = (); # all files touched by the transaction
my @all_dir = (); # all dirs touched by the transaction
my $error_message = ; # error message of the precommit hook.
# if no check fails, the string is empty
# If an error occurs append the message
my @checks = (); # add checks to this array
sub check_cws_isolation
{
return if($main::log_message =~ /IGNORE CWS ISOLATION/);
my %touched_cws_hash = ();
foreach my $cws (@main::all_dirs)
{
if($cws =~ s/^cws\/([^\/]+)\/.*$/$1/)
{
$touched_cws_hash{$cws} = 1;
}
elsif(!($cws =~ /^cws\/$/))
{
$touched_cws_hash{Non-CWS Location} = 1;
}
}
my @touched_cws = keys(%touched_cws_hash);
if($main::debug)
{
print DEBUG: touched CWS: @touched_cws\n;
}
if(@touched_cws 1)
{
$main::error_message .= cws isolation: Commit touches multiple cws. Do
a seperate commit for each cws.\n;
$main::error_message .= cws isolation: Touched cws are:
@touched_cws.\n;
$main::error_message .= cws isolation: Add 'IGNORE CWS ISOLATION' to
log message to force the commit.\n;
}
}
push(@checks, \check_cws_isolation);
# helper: check if there is a tab-indent in new line
sub check_indentation_from_filediff
{
my @filediff = @_;
return if(@filediff == 0);
my $filename = $filediff[0];
my $nonconforming = ;
$filename =~ s/[^:]+: (.*)$/$1/;
return if(!($filename =~ /^.*\.(cxx|hxx|c|h)$/));
foreach my $line (@filediff)
{
$nonconforming = true if($line =~ /^\+\ *\t/);
}
if($nonconforming)
{
$main::error_message .= indentation: Commit contains new/changed lines
with nonconforming indentation.\n;
$main::error_message .= indentation: Offending file is: $filename.\n;
$main::error_message .= indentation: Add 'NONCONFORMING INDENTATION'
to log message to force the commit.\n;
}
}
sub check_indentation
{
return if($main::log_message =~ /NONCONFORMING INDENTATION/);
my @diff_lines = split(\n, exec_svnlook(diff --no-diff-deleted));
my @one_file_diff = ();
foreach my $line (@diff_lines)
{
if($line =~ /^[A-Za]/)
{
check_indentation_from_filediff(@one_file_diff);
@one_file_diff = ();
}
push(@one_file_diff, $line);
}
check_indentation_from_filediff(@one_file_diff);
}
push(@checks, \check_indentation);
sub check_new_modules_in_cws
{
return if($main::log_message =~ /NEW MODULE/);
foreach my $new_module (@added_files)
{
if($new_module =~ s/^cws\/([^\/]+)\/([^\/]+)\/$/$2 in $3/)
{
$main::error_message .= new module in cws: Commit contains a new
module.\n;
$main::error_message .= new module in cws: new module is:
$new_module.\n;
$main::error_message .= new module in cws: Add 'NEW MODULE' to log
message to force the commit.\n;
}
}
}
push(@checks, \check_new_modules_in_cws);
sub check_new_toplevel_in_module
{
return if($main::log_message =~ /NEW TOPLEVEL DIR/);
foreach my $new_topdir (@added_files)
{
if($new_topdir =~ s/^cws\/([^\/]+)\/([^\/]+)\/([^\/]+)\/$/$3 in
module $2 in cws $1/)
{
$main::error_message .= new toplevel dir in module: Commit
containing a new toplevel dir in a module.\n;
$main::error_message .= new toplevel dir in module: new toplevel
dir is: $new_topdir.\n;
$main::error_message .= new toplevel dir in module: Add 'NEW
Re: [dev] Subversion precommit hook
bjoern michaelsen - Sun Microsystems - Hamburg Germany wrote: Hi list, since we now have subversion, we might as well use the new features it provides. I wrote a precommit hook on the weekend that does some precommit sanity checks: - It rejects commits changing files in a cws and outside of it (thus hopefully preventing some accidental commits to a master How should that be possible when you svn switch a complete tree to a cws (which you should do)? - Adding a new module has to be properly announced in the commit message There's no modules anymore but one big tree. That check imho is moot. - Adding a new toplevel dir in a module has to be properly announced in the commit message (this hopefully prevents accidental commit of output trees) Err? Why should that be disallowed? And how does that help? For adding output trees you'd need a svn add on that anyway, so... If people do that you can't help them anyway... Regards, Rene - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Subversion precommit hook
On 10/20/08 12:56, Frank Schönheit - Sun Microsystems Germany wrote: ... modules ... Not sure this is needed. AFAIK it is (at least in CVS times it was) necessary to do other things for adding a new module (announcement to releng etc.), so just preventing the commit doesn't really solve a problem, IMO. ... toplevel dirs ... Not sure this is worth it. I still think ignore lists are the best way to prevent accidental committing of output trees, and for all other dirs, we should not force us to a special commit message without a need. Subversion does not care about modules, however the build system might. Restricting toplevel dirs are probably an superfluous hassle - it was just something that was easily implemented after having code watching for certain dirs. I added the toplevel dir stuff, because you can still commit an svn:ignored directory. However, Im not vicious about that toplevel stuff. Given that pre- and postcommit hooks are basically working the same, using this precommit hooks as a base to create a postcommit hook should be easy. That hook should automagically svn:ignore output dirs when a new module is created in a cws. I think that would better than a cronjob svn:ignoreing all files as: - output trees are svn:ignored right after the module is created and even in the cws - you only need to manage the platforms in the postcommit hook, no need to track every platform/module combination. - Never allows changes/deleting of tagged versions preventing changes sounds good, preventing deletion of tags - not sure. At least in CVS times, tags became a pain in the neck over time (because there were so many), but this probably doesn't apply to SVN. Its less of an issue with svn and remember this check can be disabled by a special commit message (containing MODIFY TAGS). The svn-client would barf up that info too when rejecting a commit. Have Fun, Björn - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Subversion precommit hook
On 10/20/08 12:51, Rene Engelhard wrote: How should that be possible when you svn switch a complete tree to a cws (which you should do)? There's no need for any checks at all, if everybody does what he should do ;-). There's no modules anymore but one big tree. That check imho is moot. Indeed. However, the build system still sees a difference between a dir and a module. ... toplevel dirs in modules ... Err? Why should that be disallowed? And how does that help? For adding output trees you'd need a svn add on that anyway, so... If people do that you can't help them anyway... Currently we have the output trees svn:ignored. svn add * would still happily add the output trees. I would bet this will bite someone sometime. IIRC someone commited a output tree to the CVS once. However, I dont think it is essential to have such a check in the precommit hook - its merely a proposal. Have Fun, Björn - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Subversion precommit hook
Hi Björn, Given that pre- and postcommit hooks are basically working the same, using this precommit hooks as a base to create a postcommit hook should be easy. See issue 95199 for my currently prepared (and already implemented) solution, though a post-commit hook also sounds interesting. Ciao Frank -- - Frank Schönheit, Software Engineer [EMAIL PROTECTED] - - Sun Microsystems http://www.sun.com/staroffice - - OpenOffice.org Base http://dba.openoffice.org - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Subversion precommit hook
On 10/20/08 15:08, Frank Schönheit - Sun Microsystems Germany wrote: See issue 95199 for my currently prepared (and already implemented) solution, though a post-commit hook also sounds interesting. I just tried to add an svn:ignored dir. That works. If someone does a svn diff in a module, and sees: ? source/somenewfile.cxx ? source/somenewfile.hxx he might be tempted to do a 'svn add *; svn ci -m my message' and goes to grap a coffee. When he returns he has happily commited the output trees. That seems kinda dangerous to me. If we svn:ignore output trees, we should also prevent them from being commited (if we have a list of platforms that are svn:ignored, we could also specifically look for those in the precommit hook). BTW this is just as dangerous when having the output trees in global ignore in the config. Have Fun, Björn - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Subversion precommit hook
Hi Bjoern, I just tried to add an svn:ignored dir. That works. Sure - svn:ignore is just for ignoring the item in status and recursive commits. If someone does a svn diff in a module, and sees: ? source/somenewfile.cxx ? source/somenewfile.hxx he might be tempted to do a 'svn add *; svn ci -m my message' and goes to grap a coffee. When he returns he has happily commited the output trees. That seems kinda dangerous to me. Well, admittedly the intention for my svn:ignore request was never to prevent people from committing them. As I wrote in my original mail, it's about the noise produced by svn status which bothers me. (For instance because I usually check my CWS for uncommitted files before passing it to QA or finally deleting it). If somebody really commits an output tree, we can still a) shot her and b) do an svn delete (which nowadays is much easier than fixing the same problem in CVS would have been.). If we svn:ignore output trees, we should also prevent them from being commited (if we have a list of platforms that are svn:ignored, we could also specifically look for those in the precommit hook). I am undecided here. If you wish - do it. It might be harder to achieve, as it requires people to update the script when new platforms are born, and in opposite to updating svn:ignore, that's nothing an ordinary developer can do. BTW this is just as dangerous when having the output trees in global ignore in the config. I never claimed to invent the idiot-proof solution :) Ciao Frank -- - Frank Schönheit, Software Engineer [EMAIL PROTECTED] - - Sun Microsystems http://www.sun.com/staroffice - - OpenOffice.org Base http://dba.openoffice.org - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Subversion precommit hook
Hi Bjoern,
there was no need for crucifying yourself, server side we are python
only. Actually we have no perl bindings installed.
I think we need to be a bit carefull with pre-commit hooks. Other than
post-commit hooks they do influence the user experience of SVN, so
they have to be fast. Well, we've got a very fast server so probably
this is not really a problem.
I somehow don't like tying SCM functionality to commit messages, but
that's may be just me.
And should we enforce policy (like tabs vs spaces etc) via the SCM tool?
Heiner
bjoern michaelsen - Sun Microsystems - Hamburg Germany wrote:
Hi list,
since we now have subversion, we might as well use the new features it
provides. I wrote a precommit hook on the weekend that does some
precommit sanity checks:
- It rejects commits changing files in a cws and outside of it (thus
hopefully preventing some accidental commits to a master
- It checks all *.{cxx|c|hxx|h} files for correct indentation (no tab
indentation on added/changed lines)
- Adding a new module has to be properly announced in the commit message
- Adding a new toplevel dir in a module has to be properly announced in
the commit message (this hopefully prevents accidental commit of
output trees)
- Never allows changes/deleting of tagged versions
I crucified myself to write the hook in perl because I thought it to be
the preferred language for releng stuff (I would have preferred python
myself).
All checks can be disabled with adding special strings to the commit
message.
Im looking forward to comments, ideas and additions. Is it a good idea
to use such a precommit hook? Whats relengs position on this?
Have Fun,
Björn
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
--
Jens-Heiner Rechtien
[EMAIL PROTECTED]
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Subversion precommit hook
Hi, Jens-Heiner Rechtien wrote: I somehow don't like tying SCM functionality to commit messages, but that's may be just me. And should we enforce policy (like tabs vs spaces etc) via the SCM tool? is there another point where we could actually enforce policy ? Enforce as in preemptive, not cooperating ? It would have the advantage that people cannot simply forget this. OTOH code formatting has the potential to create a holy war about nothing (namely whitespace). Personally I'd prefer this to be not a check, but an automatic on the fly reformat - but I assume that would occasionally break a file, if the input deviates too much (or in an unexpected way) from the expected format. Just my 2 cents, pl -- Someone told me: Smile and be happy, it could be worse And I smiled and was happy and things became worse. -- Author unknown - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Subversion precommit hook
On 10/20/08 17:39, Philipp Lohmann wrote: Hi, Jens-Heiner Rechtien wrote: I somehow don't like tying SCM functionality to commit messages, but that's may be just me. And should we enforce policy (like tabs vs spaces etc) via the SCM tool? is there another point where we could actually enforce policy ? Enforce as in preemptive, not cooperating ? It would have the advantage that people cannot simply forget this. OTOH code formatting has the potential to create a holy war about nothing (namely whitespace). Well, there is a clearcut requirement in the OOo coding style. And whitespace is not nothing, as it is really reducing productivity on merges/rebases. Personally I'd prefer this to be not a check, but an automatic on the fly reformat - but I assume that would occasionally break a file, if the input deviates too much (or in an unexpected way) from the expected format. The only alternative point where this could be enforced is on the master: After all cws are integrated, all lines that were added/updated all reformatted. This would ensure no nonconforming new code will be added, without creating huge diffs that every cws would have to cope with on merge/rebase. Have Fun, Björn - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [dev] Subversion precommit hook
On 10/20/08 17:10, Jens-Heiner Rechtien wrote: there was no need for crucifying yourself, server side we are python only. Actually we have no perl bindings installed. If I only had known that before. I like and know Python a lot better. I think we need to be a bit carefull with pre-commit hooks. Other than post-commit hooks they do influence the user experience of SVN, so they have to be fast. Well, we've got a very fast server so probably this is not really a problem. I would guess the scripts as of now are not really a performance issue - they only work on the diffs of a commit not on whole files/directory trees as the commit itself has to. Also, I would suggest to add features to the hook bit by bit and not all at once. Have Fun, Björn - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
