Thanks Stefan. I've added a trap to deal with an unexpected EOF condition in the svnlook proplist output, as you suggested. Perl would have failed with a "se of uninitialized value $next_line_pval_indented in pattern match", but the new check should make that error more explicit.
Here's an update patch in-line, and I updated https://github.com/javabrett/subversion/tree/check-mime-type-fix in-case. I am stuck with Gmail and have not setup git send-email yet for this list, and Gmail is known for breaking patches with line-wrapping. Let me know if you need a git send-email. diff --git a/contrib/hook-scripts/check-mime-type.pl b/contrib/hook-scripts/check-mime-type.pl index 9ac7e8d..3ded119 100755 --- a/contrib/hook-scripts/check-mime-type.pl +++ b/contrib/hook-scripts/check-mime-type.pl @@ -119,17 +119,48 @@ foreach my $path ( @files_added ) # 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)) + + my @output = &read_from_process($svnlook, 'proplist', $repos, '-t', + $txn, '--verbose', '--', $path); + my $output_line = 0; + + foreach my $prop (@output) { - if ($prop =~ /^\s*svn:mime-type : (\S+)/) + if ($prop =~ /^\s*svn:mime-type( : (\S+))?/) { - $mime_type = $1; + $mime_type = $2; + # 1.7.8 (r1416637) onwards changed the format of svnloop proplist --verbose + # from propname : propvalue format, to values in an indent list on following lines + if (not $mime_type) + { + if ($output_line + 1 >= scalar(@output)) + { + die "$0: Unexpected EOF reading proplist.\n"; + } + my $next_line_pval_indented = $output[$output_line + 1]; + if ($next_line_pval_indented =~ /^\s{4}(.*)/) + { + $mime_type = $1; + } + } } - elsif ($prop =~ /^\s*svn:eol-style : (\S+)/) + elsif ($prop =~ /^\s*svn:eol-style( : (\S+))?/) { - $eol_style = $1; + $eol_style = $2; + if (not $eol_style) + { + if ($output_line + 1 >= scalar(@output)) + { + die "$0: Unexpected EOF reading proplist.\n"; + } + my $next_line_pval_indented = $output[$output_line + 1]; + if ($next_line_pval_indented =~ /^\s{4}(.*)/) + { + $eol_style = $1; + } + } } + $output_line++; } # Detect error conditions and add them to @errors On 7 August 2015 at 19:57, Stefan Sperling <s...@elego.de> wrote: > On Fri, Aug 07, 2015 at 03:25:19PM +1000, Brett Randall wrote: >> On the mailing list back in March 2014, there was a thread[1] which >> correctly observed that since r1416637 (released in 1.7.8), the >> changed output of svnlook proplist from name : value format to a new >> multi-line/multi-value format made the existing check-mime-type.pl >> contrib pre-commit hook script no longer worked correctly, as it could >> no longer parse the multi-line proplist output. >> >> A patch was proposed[2], which took the approach of using svnlook >> proget instead of svnlook proplist. After some feedback, the thread >> went cold, and there's no evidence of a commit or tracking bug. >> >> I took a look at the patch and a similar approach. I found that >> svnlook propget, if it does not find the property present, sets return >> code = 1, which is caught in read_from_process causing the script to >> fail immediately. Perhaps that was how it was intended to work. >> >> The patch also contains a behaviour change, from working only on added >> files in the transaction, to now working with any updated files also. >> >> Having found the contrib script non-functional, I've taken another >> look at this, and pushed a potential fix to my clone on Github[3]. >> This works for me for Subversion/svnlook 1.6.11 (old output) and 1.8.8 >> (new output). I went with a dual-format parse of svnlook proplist >> output, rather than tackling the switch to propget and handling the >> return-code. >> >> So I just wanted to reopen the conversation, to either reopen >> discussion/review of the old patch, or start a review of my new patch. >> >> Thanks >> Brett > > Hi Brett, > > thanks for your patch. I took a quick look at it. > > The handling of the output_line index varliable doesn't seem entirely safe. > If the output is garbled and ends with a property name but no value then > the script might end up using an invalid array index here: > > if (not $mime_type) > { > my $next_line_pval_indented = $output[$output_line + 1]; > > We probably want to add an out-of-bounds check here? Or perhaps perl > will report a useful error message by itself? Not sure. > > BTW, posting patches inline as part of your mail makes review discussion > on this mailing list easier. Because we won't have to copy/paste parts > of your patch from github into email or run git first to get the code > and then copy it. We could just reply in the relevant patch section in > mail directly. So please consider submitting your patches as outlined here: > http://subversion.apache.org/docs/community-guide/general.html#patches > > Thanks, > Stefan
