[buildd-tools-devel] [PATCH 2/5] [ChrootInfoSchroot.pm] Support directory value

[Roger Leigh]

On Thu, Sep 17, 2009 at 05:20:59PM +0200, Jan-Marek Glogowski wrote:
> ---
>  lib/Sbuild/ChrootInfoSchroot.pm |   31 ++++++++++++++++++++-----------
>  1 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/Sbuild/ChrootInfoSchroot.pm b/lib/Sbuild/ChrootInfoSchroot.pm
> index 97d839c..4103db4 100644
> --- a/lib/Sbuild/ChrootInfoSchroot.pm
> +++ b/lib/Sbuild/ChrootInfoSchroot.pm
> @@ -59,6 +59,15 @@ sub get_info {
>      $ENV{'LC_ALL'} = 'C';
>      $ENV{'LANGUAGE'} = 'C';
>  
> +    # Path <- Mount Location <- Directory <- Location (deprecated)
> +    my @location_matches = ( 
> +	'^\s*Path:?\s+(.+)$',
> +	'^\s*Mount Location:?\s+(.+)$',
> +	'^\s*Directory:?\s+(.+)$',
> +	'^\s*Location:?\s+(.+)$' 
> +	);
> +    my $location_priority = 1 + scalar @location_matches;
> +
>      open CHROOT_DATA, '-|', $self->get_conf('SCHROOT'), '--info', '--chroot', $chroot
>  	or die 'Can\'t run ' . $self->get_conf('SCHROOT') . ' to get chroot data';
>      while (<CHROOT_DATA>) {
> @@ -66,18 +75,18 @@ sub get_info {
>  	if (/^\s*Type:?\s+(.*)$/) {
>  	    $chroot_type = $1;
>  	}
> -	if (/^\s*Location:?\s+(.*)$/ &&
> -	    $tmp{'Location'} eq "") {
> -	    $tmp{'Location'} = $1;
> -	}
> -	if (/^\s*Mount Location:?\s+(.*)$/ &&
> -	    $tmp{'Location'} eq "") {
> -	    $tmp{'Location'} = $1;
> -	}
> -	# Path takes priority over Location and Mount Location.
> -	if (/^\s*Path:?\s+(.*)$/) {
> -	    $tmp{'Location'} = $1;
> +
> +	# Get the "best" location
> +	my $priority = 0;
> +	foreach my $match (@location_matches) {
> +	    last if( $priority == $location_priority );
> +	    if ($_ =~ /${match}/) {
> +		$tmp{'Location'} = $1;
> +		$location_priority = $priority;
> +	    }
> +	    $priority++;
>  	}
> +
>  	if (/^\s*Priority:?\s+(\d+)$/) {
>  	    $tmp{'Priority'} = $1;
>  	}

Looking at the patch, $priority and $location_priority look useless
due to the fact that the conditional "last if" will always be false.
They are not needed for the foreach loop to work.

I do like the code simplification, but those variables need
removing IMO.  The order of the regexes is fine, you just need to
break out of the loop on a match, and maybe also error out if
no match was found (so add a variable $found and set it on match).

Lastly, I'm not convinced about the match on Directory, and would
like that removed: there's no released version of schroot for which
this behaviour is appropriate.

Could you please check that the current schroot in git (which no
additional changes) works correctly with the current sbuild in
unstable?


Regards,
Roger


-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.