Hi Jonathan,
On Thu, May 05, 2011 at 07:19:28PM -0500, Jonathan Nieder wrote:
> 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
As I wrote I dropped -i and -I from git-pbuilder with the last upload.
However this broke the 1.0 source format build (since they don't exclude
.git automatically), so I went for this:
http://git.debian.org/?p=users/agx/git-buildpackage.git;a=commit;h=033f359f5c65ae02d81410320c0f2f77ea54ff3b
Any thoughts on this solution?
Cheers,
-- Guido
>
> 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 [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]
