Hello, I decided to go with the last approach. I've tried it on subversion 1.1.4 and 1.8.5, and the simple testcases seem to indicate it works on both of them.
I've attached the patch (from trunk) and a small collection of testcases in a tarball. [[[ Fix check-mime-type.pl for output changes to 'svnlook proplist' in a maximally backward compatible way. Tested against subversion 1.1.4 and 1.8.5. The output format of 'svnlook proplist' was changed in revision 1416637. See also http://svn.haxx.se/dev/archive-2012-11/0510.shtml * contrib/hook-scripts/check-mime-type.pl: -- Also test files when properties have been modified, not just added, to ensure that properties don't vanish. -- Replace "svnlook proplist --verbose" with a two step process of "svnlook proplist" and "svnlook propget". This was done because "svnlook proplist --verbose" cannot unambiguously be parsed without --xml with multiline properties. -- Remove unused code. ]]] Leo On Monday, March 03, 2014 09:12:32 AM Leo Davis wrote: > Hello, > > Another approach is to dump 'svnlook proplist' altogether and use 'svnlook > propget svn:mime-type' and 'svnlook propget svn:eol-style' instead. That > could be maximally backward compatible without introducing XML. > > Regards, > > Leo > ________________________________________ > From: Ben Reser <b...@reser.org> > Sent: Sunday, March 02, 2014 11:40 PM > To: Leo Davis; Daniel Shahaf > Cc: dev@subversion.apache.org > Subject: Re: [PATCH]: fix check-mime-type.pl for changes to 'svnlook > proplist' output > On 3/2/14, 5:34 PM, Leo Davis wrote: > > As Ben pointed out, the current parser in the script for svnlook <= 1.7.x > > is broken and unfixable for multiline properties. The closest we can get > > to DTRT in this case is to have svnlook output XML. Hopefully no one > > still cares about svnlook <= 1.3 (?) that cannot output XML.> > > On Mar 2, 2014, at 8:11 AM, "Daniel Shahaf" <d...@daniel.shahaf.name> wrote: > >> One more issue: however you change the parser, it will break if > >> svnlook1.7 or older is used. It would be nice to DTRT in that case > >> (either error out or retain the old parser). > > It would be nice to have, but I think the effort to provide it is just too > great unless we go down the XML path. Which is a pretty large change in the > requirements of the script. > > Anyone that wants it can go get a copy of the script off the 1.7.x branch > (assuming we merge the fix to the 1.8.x branch). This is contrib we have no > compatibility guarantees to worry about either. > > Just put a prominent notice at the top of the script saying that it is only > intended for use with 1.8.x or newer servers.
Index: contrib/hook-scripts/check-mime-type.pl
===================================================================
--- contrib/hook-scripts/check-mime-type.pl (revision 1578499)
+++ contrib/hook-scripts/check-mime-type.pl (working copy)
@@ -1,7 +1,7 @@
#!/usr/bin/env perl
# ====================================================================
-# commit-mime-type-check.pl: check that every added file has the
+# commit-mime-type-check.pl: check that every added/modified file has the
# svn:mime-type property set and every added file with a mime-type
# matching text/* also has svn:eol-style set. If any file fails this
# test the user is sent a verbose error message suggesting solutions and
@@ -36,7 +36,6 @@ BEGIN {
use strict;
use Carp;
-
######################################################################
# Configuration section.
@@ -75,74 +74,61 @@ my $svnlook = "/usr/bin/svnlook";
my $repos = shift;
my $txn = shift;
+my $err_msg = "$0: repository directory `$repos' ";
-unless (-e $repos)
- {
- &usage("$0: repository directory `$repos' does not exist.");
- }
-unless (-d $repos)
- {
- &usage("$0: repository directory `$repos' is not a directory.");
- }
+&usage("$err_msg does not exist.") unless (-e $repos);
+&usage("$err_msg is not a directory.") unless (-d $repos);
-# Define two constant subroutines to stand for read-only or read-write
-# access to the repository.
-sub ACCESS_READ_ONLY () { 'read-only' }
-sub ACCESS_READ_WRITE () { 'read-write' }
-
-
######################################################################
# Harvest data using svnlook.
-# Change into /tmp so that svnlook diff can create its .svnlook
-# directory.
-my $tmp_dir = '/tmp';
-chdir($tmp_dir)
- or die "$0: cannot chdir `$tmp_dir': $!\n";
+# Figure out what files have added/modified properties using svnlook.
+my @paths_to_check;
+my $props_changed_re = qr/^(?:A |[U_ ]U) (.*[^\/])$/;
+foreach my $line (&read_from_process($svnlook, 'changed', $repos, '-t', $txn)) {
+ if ($line =~ /$props_changed_re/) {
+ push(@paths_to_check, $1);
+ }
+}
-# Figure out what files have added using svnlook.
-my @files_added;
-foreach my $line (&read_from_process($svnlook, 'changed', $repos, '-t', $txn))
- {
- # Add only files that were added to @files_added
- if ($line =~ /^A. (.*[^\/])$/)
- {
- push(@files_added, $1);
- }
- }
-
my @errors;
-foreach my $path ( @files_added )
- {
- my $mime_type;
- my $eol_style;
- # Parse the complete list of property values of the file $path to extract
- # the mime-type and eol-style
- foreach my $prop (&read_from_process($svnlook, 'proplist', $repos, '-t',
- $txn, '--verbose', '--', $path))
- {
- if ($prop =~ /^\s*svn:mime-type : (\S+)/)
- {
- $mime_type = $1;
- }
- elsif ($prop =~ /^\s*svn:eol-style : (\S+)/)
- {
- $eol_style = $1;
- }
- }
+# We are using 'svnlook propget' here instead of 'svnlook proplist'
+# because the output of 'svnlook proplist' without --xml could be ambiguous
+# with multiline properties.
+my @properties = ('svn:mime-type', 'svn:eol-style');
+my $mime_text_re = qr/^text\//;
+my $proplist_name_re = qr/^ (.*)$/;
+my $properties_pat = '(?:' . join('|', map {quotemeta} @properties) . ')';
+my $grep_re = qr/^$properties_pat$/;
- # Detect error conditions and add them to @errors
- if (not $mime_type)
- {
- push @errors, "$path : svn:mime-type is not set";
- }
- elsif ($mime_type =~ /^text\// and not $eol_style)
- {
- push @errors, "$path : svn:mime-type=$mime_type but svn:eol-style is not set";
- }
- }
+foreach my $path ( @paths_to_check ) {
+ my %prop_map = ();
+
+ # See what properties we do have
+ my @path_props = &read_from_process($svnlook, 'proplist', $repos, '-t', $txn, '--', $path);
+
+ # filter out only the ones we care about
+ my @filtered = grep {/$grep_re/} map { $_ =~ s/$proplist_name_re/$1/; $_; } @path_props;
+
+ # Grab filtered properties
+ foreach my $prop (@filtered) {
+ $prop_map{$prop} = join("\n", &read_from_process($svnlook, 'propget', $repos,
+ $prop, '-t', $txn, '--', $path));
+ }
+
+ # Detect error conditions and add them to @errors
+ if (not $prop_map{$properties[0]})
+ {
+ push @errors, "$path : svn:mime-type is not set";
+ }
+ elsif ($prop_map{$properties[0]} =~ /$mime_text_re/ and not $prop_map{$properties[1]})
+ {
+ push @errors, "$path : svn:mime-type=$prop_map{$properties[0]} but svn:eol-style is not set";
+ }
+}
+
# If there are any errors list the problem files and give information
# on how to avoid the problem. Hopefully people will set up auto-props
# and will not see this verbose message more than once.
@@ -152,8 +138,8 @@ if (@errors)
join("\n", @errors), "\n\n",
<<EOS;
- Every added file must have the svn:mime-type property set. In
- addition text files must have the svn:eol-style property set.
+ Every added/modified file must have the svn:mime-type property set.
+ In addition text files must have the svn:eol-style property set.
For binary files try running
svn propset svn:mime-type application/octet-stream path/of/file
test-cmt.tar.gz
Description: application/compressed-tar
