Re: [PATCH] dist-xz: don't hard-code -9: honor setting of XZ_OPT

[Stefano Lattarini]

Hi Jim.

Some more nits again...

On Monday 04 October 2010, Jim Meyering wrote:
> Stefano Lattarini wrote:
> > While I don't feel qualified to discuss the merits of the matter
> > here, I have a couple of nits below...
> 
> Hi Stefano,
> 
> Thanks for the review!
> 
> > On Monday 04 October 2010, Jim Meyering wrote:
> ...
> 
> >>  Makefile.in       |    6 ++++--
> >>  NEWS              |    5 +++++
> >>  aclocal.m4        |    4 ++--
> >>  configure         |   10 +++++-----
> >>  doc/automake.texi |    7 +++++++
> >>  lib/am/distdir.am |    6 ++++--
> >>  6 files changed, 27 insertions(+), 11 deletions(-)
> 
> ...
> 
> >> +    | BZIP2=${BZIP2--9} bzip2 -c >$(distdir).tar.bz2
> > 
> > Typo here: should be "$${BZIP2--9}".
> 
> Well caught.  Fixed.
> I should have tested with dist-bzip2, too.
Maybe we should enhance the testsuite, too.  But that can be easily 
and comfortably done in a follow-up patch, so it's not a show-stopper.
 
> > But then: why not follow consistently the example of the gzip
> > 
> > compression?  As in:
> >   GZIP=$(GZIP_ENV) gzip -c >$(distdir).tar.gz
> > 
> > with GZIP_ENV set to a proper default in the generated makefile,
> > 
> > and overridable through the command line:
> >   $ make dist GZIP_ENV=--fast
> 
> To minimize namespace impact, and to keep the change compact/local.
> With my proposed approach, we do not impinge on
> user name space at all.  With the above, we'd usurp
> BZIP2_ENV and XZ_ENV if you want to be consistent.
> Also, with the _ENV variables, there would have to be
> added definitions.  No fundamental objection, but I do
> prefer the smaller patch.
I have no fundamental objection too, but I prefer consistency ;-)
No big deal, though, as long as the documentation is clear.

> 
> >> diff --git a/aclocal.m4 b/aclocal.m4
> >> index c43a368..4a22182 100644
> >> --- a/aclocal.m4
> >> +++ b/aclocal.m4
> > 
> > Why have you regenerated this file with a unreleased autoconf
> > version?
> 
> I regularly test unreleased versions.  Sorry to have included that.
> I was obviously asleep at the wheel.
> I'm no longer accustomed to submitting patches in projects
> that version-control such generated files.
I made similar errors too many times not to feel sympathy for you :-)

> 
> >> diff --git a/configure
> >> b/configure
> >> index 169d82d..2940276 100755
> >> --- a/configure
> >> +++ b/configure
> > 
> > Ditto.
> 
> Likewise.
> 
> >> diff --git a/doc/automake.texi b/doc/automake.texi
> >> index 22c2f27..0652aa8 100644
> >> --- a/doc/automake.texi
> >> +++ b/doc/automake.texi
> >> @@ -8461,6 +8461,9 @@ The Types of Distributions
> >> 
> >>  @item @code{dist-bzip2}
> >>  Generate a bzip2 tar archive of the distribution.  bzip2
> >>  archives
> >> 
> >> are frequently smaller than gzipped archives.
> >> +By default, this rule makes @samp{bzip2} use a compression
> >> option of @option{-9}. +To make it use a different one, set the
> >> @env{XZ_OPT} environment variable
> > 
> > BZIP2 environment variable
> > 
> >> +For example, @samp{make dist-bzip2 XZ_OPT=-7}.
> > 
> > Ditto.
> 
> Thank you.  Fixed.
Hmm... thinking better, since we are dealing with environment 
variables here, shouldn't the correct idiom be:
  $ BZIP2="-7" make dist-bzip2 
instead?  Or am I missing something?

Regards,
  Stefano