subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brett Randall <javabr...@gmail.com>
Subject Re: [patch][reboot-topic] fix check-mime-type.pl for changes to 'svnlook proplist' output
Date Mon, 24 Aug 2015 00:36:25 GMT
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@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

Mime
View raw message