You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Brett Randall <ja...@gmail.com> on 2015/08/07 07:25:19 UTC
[patch][reboot-topic] fix check-mime-type.pl for changes to 'svnlook
proplist' output
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
[1] http://mail-archives.apache.org/mod_mbox/subversion-dev/201403.mbox/%3C1576503.m6XB7udPXQ@hurry.speechfxinc.com%3E
[2] http://mail-archives.apache.org/mod_mbox/subversion-dev/201403.mbox/raw/%3C1576503.m6XB7udPXQ@hurry.speechfxinc.com%3E/2
[3] https://github.com/javabrett/subversion/commit/be308d1dc17aa601e0b04379c0f530c8475626a7#diff-18a955adda94952d6581a5a72ec1b2bd
Re: [patch][reboot-topic] fix check-mime-type.pl for changes to
'svnlook proplist' output
Posted by Julian Foad <ju...@btopenworld.com>.
On 24 August 2015 Brett Randall wrote:
> 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 [...]
Thanks, Brett. I noticed you've gone to the effort of fixing and
contributing your fixes to this script, and we have been slow to
acknowledge it. So, although I haven't tested this, if it works for
you that's good enough for me, so I have committed it in r1701488.
I have proposed it for backport to 1.7.x (even though this line is no
longer in regular maintenance, so there's a very low probability that
it will be backported there), 1.8.x and 1.9.x, in r1701491 and
r17014913.
- Julian
Re: [patch][reboot-topic] fix check-mime-type.pl for changes to
'svnlook proplist' output
Posted by Brett Randall <ja...@gmail.com>.
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 <st...@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
Re: [patch][reboot-topic] fix check-mime-type.pl for changes to
'svnlook proplist' output
Posted by Stefan Sperling <st...@elego.de>.
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