Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Date: Fri, 7 Aug 2015 11:57:00 +0200
From: Stefan Sperling <stsp@elego.de>
To: Brett Randall <javabrett@gmail.com>
Cc: dev@subversion.apache.org
Subject: Re: [patch][reboot-topic] fix check-mime-type.pl for changes to
 'svnlook proplist' output
Message-ID: <20150807095659.GC4573@fintan.stsp.name>
Mail-Followup-To: Brett Randall <javabrett@gmail.com>,
	dev@subversion.apache.org
References: 
 <CALeEUB56NVCR0nCZ2rYki6oKQNcVPPLvWTeCoYyrMqitsov0ag@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CALeEUB56NVCR0nCZ2rYki6oKQNcVPPLvWTeCoYyrMqitsov0ag@mail.gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)

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