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 <stsp_at_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
Received on 2015-08-24 02:36:38 CEST