Hi,

Gilles Filippini wrote:

> The attached patch appears to solve my problem, with
> --extend-diff-ignore specified either from the command line or from a
> debian/source/[local-]options file.
>
> Rationals are:
> * options from the command line should be interpreted first
> * then should come options from debian/source/options
> * and finally options from debian/source/local-options
> * --extend-diff-ignore should actually extend diff-ignore instead of the
> default diff-ignore regex.

So, in an ideal world these would be multiple patches, one fixing each
issue.  No matter; here's some quick reactions nonetheless.  Thanks
very much for making this more concrete.

> --- /usr/bin/dpkg-source      2011-04-16 03:54:49.000000000 +0200
> +++ ./dpkg-source     2011-05-06 01:41:43.000000000 +0200
> @@ -113,7 +113,7 @@
>       "options" => 
> qr/^--(?:format=|unapply-patches$|abort-on-upstream-changes$)/,
>       "local-options" => qr/^--format=/,
>      };
> -    foreach my $filename ("local-options", "options") {
> +    foreach my $filename ("options", "local-options") {
>       my $conf = Dpkg::Conf->new();
>       my $optfile = File::Spec->catfile($dir, "debian", "source", $filename);
>       next unless -f $optfile;
> @@ -122,7 +122,7 @@
>       if (@$conf) {
>           info(_g("using options from %s: %s"), $optfile, join(" ", @$conf))
>               unless $options{'opmode'} eq "--print-format";
> -         unshift @options, @$conf;
> +         push @options, @$conf;
>       }

Could you explain what the net effect of these changes is, preferrably
with an example?

Without reading carefully, I can't see why this wouldn't be a no-op.

> @@ -157,7 +157,11 @@
>      } elsif (m/^-(?:i|-diff-ignore(?:$|=))(.*)$/) {
>          $options{'diff_ignore_regexp'} = $1 ? $1 : 
> $Dpkg::Source::Package::diff_ignore_default_regexp;
>      } elsif (m/^--extend-diff-ignore=(.+)$/) {
> -     $Dpkg::Source::Package::diff_ignore_default_regexp .= "|$1";
> +        if ($options{'diff_ignore_regexp'}) {
> +            $options{'diff_ignore_regexp'} .= "|$1";
> +        } else {
> +            $options{'diff_ignore_regexp'} = 
> $Dpkg::Source::Package::diff_ignore_default_regexp . "|$1";
> +        }
>      } elsif (m/^-(?:I|-tar-ignore=)(.+)$/) {

This means that "dpkg -b <directory> ..."

 --extend-diff-ignore=foo would mean
        --extend-diff-ignore=foo -i

 -ifoo --extend-diff-ignore=bar -i would mean
        -i (i.e., later -i overrides --extend-diff-ignore after earlier -i)

 -ifoo --extend-diff-ignore=bar would mean
        -ifoo|bar

 --extend-diff-ignore=bar -ifoo would mean
        -ifoo (i.e., later -i<regex> overrides --extend-diff-ignore)

So it's a big change in semantics and the result is somewhat
confusing.  Are you sure that's intended?

In general, the "later -i overrides earlier -i" semantics interact
with --extend-diff-ignore in a confusing way already.  So for the
original problem I am not sure what do you.  One way would involve
two fixes:

 1. Teach git-buildpackage to use --extend-diff-ignore instead of
    -i<regex> and use plain -i so packagers' extend-diff-ignore can
    be respected

 2. Make plain -i to toggle an option meaning "use
    diff_ignore_default_regexp", so "-i --extend-diff-ignore=foo" can
    mean the same thing as "--extend-diff-ignore=foo -i".  Introduce a
    new --clear-diff-ignore.  Discourage use of -i<regex> in the hope
    that --clear-diff-ignore, --extend-diff-ignore, and -i will be
    easier to work with.

Thoughts?

Regards,
Jonathan




-- 
To UNSUBSCRIBE, email to debian-dpkg-bugs-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org

Reply via email to