Stefano Lattarini wrote:
> There are few problems with it though, that I've fixed.
> More details below.
Hi Stefano,
Thanks for the thorough review and for correcting my errors.
> On Friday 09 December 2011, Jim Meyering wrote:
>> Today I noticed that automake-generated Makefile's dist-xz rule
>> used a hard-coded xz -9. That is a problem because on this front, xz
>> differs from gzip and bzip2. While the latter two incur no run-time
>> *decompression* penalty for using a higher compression level, with xz
>> if you specify -9, that imposes a potentially fatal virtual-memory
>> requirement on any client that wants to decompress your tar.xz file.
>> People have complained that a tarball compressed with -9 cannot
>> be uncompressed in a low-memory environment (wrt-based embedded).
>>
>> Hence, instead of defaulting to -9, which is useful only
>> for very large tarballs, it defaults to -e (equivalent to -6e).
>> This limits the default memory requirements imposed on decompressors,
>> yet still gives very good compression ratios.
>>
> Thanks for the detailed explanation. Unfortunately, you have forgotten
> to put it into the ChangeLog entry and git commit message, where IMO it
> belongs; I've taken the liberty of doing so on your behalf.
Habits, habits.
I'm too used to *not* duplicating git log information in a VC'd
ChangeLog file that for the few projects that do require the
duplication I tend to forget that -- esp. when it's late and
I'm out of time.
>> Subject: [PATCH] dist-xz, dist-bzip2: don't hard-code -9: honor envvar
>> settings
>>
>> This is a cherry-pick of commit 6da46f31, with additional changes to
>> reflect that the xz compression level should default to -e, not -9.
>>
>> * lib/am/distdir.am (dist-xz): Do not hard-code xz's -9: that made
>> it impossible to override. Actually don't default to -9, either,
>> since that induced inordinately large virtual memory usage when
>> merely decompressing. Instead, use its XZ_OPT envvar, defaulting
>> to -e if not defined. Suggested by Lasse Collin.
>> (dist-bzip2): Similarly, do not hard-code -9, but do continue to
>> use -9 by default. Honor the BZIP2 envvar.
>> * NEWS (Miscellaneous changes): Mention it.
>> * doc/automake.texi (The Types of Distributions): Describe the
>> newly enabled environment variables.
>> ---
>> ChangeLog | 13 +++++++++++++
>> NEWS | 5 +++++
>> doc/automake.texi | 9 +++++++++
>> lib/am/distdir.am | 4 ++--
>> 4 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/ChangeLog b/ChangeLog
>> index 1aba97a..41fa3bb 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,16 @@
>> +2010-10-05 Jim Meyering <meyer...@redhat.com>
>> +
> Oops, wrong date. Fixed.
Another reason not to VC ChangeLog files.
> Also, I've added myself as secondary author, since I've contributed few
> non-trivial amendings (see below). I hope you're ok with that.
Yes, of course.
>> + dist-xz, dist-bzip2: don't hard-code -9: honor envvar settings
>> + * lib/am/distdir.am (dist-xz): Do not hard-code xz's -9: that
>> + made it impossible to override. Instead, use its XZ_OPT envvar,
>> + defaulting to -9 if not defined. Thus no change in behavior
>> + when XZ_OPT is not set, and now, this rule honors the setting
>> + of that envvar when it is set. Suggested by Lasse Collin.
>> + (dist-bzip2): Likewise for it's corresponding envvar: BZIP2.
>> + * NEWS (Miscellaneous changes): Mention it.
>> + * doc/automake.texi (The Types of Distributions): Describe the
>> + newly enabled environment variables.
>> +
>>
> Oops, you haven't update the ChangeLog entry, and it is not out-of-sync
> with the code changes and the (correct) git commit message. I've fixed
> this.
I have not taken the time to write a commit hook to warn me when
a git log fails to match the corresponding ChangeLog delta.
It doesn't seem worthwhile.
...
> You have forgotten to update the recipes for `dist' and `dist-all':
>
> $ grep -e -9 lib/am/distdir.am
> tardir=$(distdir) && $(am__tar) | BZIP2=$${BZIP2--9} bzip2 -c
> >$(distdir).tar.bz2
> tardir=$(distdir) && $(am__tar) | lzma -9 -c >$(distdir).tar.lzma
> ?BZIP2? tardir=$(distdir) && $(am__tar) | bzip2 -9 -c >$(distdir).tar.bz2
> ?LZMA? tardir=$(distdir) && $(am__tar) | lzma -9 -c >$(distdir).tar.lzma
> ?XZ? tardir=$(distdir) && $(am__tar) | xz -9 -c >$(distdir).tar.xz
Good catch.
> I've fixed this "the dumb way", i.e., by copying & pasting. Extra
> points will go to anyone which comes up with a refactoring patch that
> removes this annoying code duplication.
That looks fine for now.
> And a final nit: you have forgotten to run ./boostrap to regenerate
> Automake's own Makefile.in (sorry, theis is still version-controlled
> for now!). I've fixed that as well.
>
> Attached is what I've pushed to maint (and merged into branch-1.11).
>
> Thanks,
> Stefano
> From c8e01d581a7e7c2445a139def46939a547951746 Mon Sep 17 00:00:00 2001
> Message-Id:
> <c8e01d581a7e7c2445a139def46939a547951746.1323473268.git.stefano.lattar...@gmail.com>
> From: Jim Meyering <meyer...@redhat.com>
> Date: Fri, 9 Dec 2011 23:17:18 +0100
> Subject: [PATCH] dist-xz, dist-bzip2: don't hard-code -9, honor envvar
> settings
>
> Before the present change, automake-generated `dist-xz' rule used
> a hard-coded `xz -9'. That was a problem because on this front,
> xz differs from gzip and bzip2. While the latter two don't incur
> any run-time decompression penalty for using a higher compression
> level, specifying -9 with xz imposes a potentially fatal virtual
> memory requirement on any client that wants to decompress your
> tar.xz file.
> People have complained that a tarball compressed with -9 cannot
> be uncompressed in a low-memory environment (wrt-based embedded).
> Hence, instead of defaulting to -9, which is useful only for very
> large tarballs, it defaults to -e (equivalent to -6e). This
> limits the default memory requirements imposed on decompressors,
> yet still gives very good compression ratios.
>
> * lib/am/distdir.am (dist-xz): Do not hard-code xz's -9: that made
> it impossible to override. Actually don't default to -9, either,
> since that induced inordinately large virtual memory usage when
> merely decompressing. Instead, use its XZ_OPT envvar, defaulting
> to -e if not defined. Suggested by Lasse Collin.
> (dist, dist-all) [?XZ?]: Likewise
> (dist-bzip2): Similarly, do not hard-code -9, but do continue to
> use -9 by default. Honor the BZIP2 envvar.
> (dist, dist-all) [?BZIP2?]: Likewise
> * NEWS: Update.
> * doc/automake.texi (The Types of Distributions): Describe the
> newly enabled environment variables.
>
> This is inspired to commit v1.11-389-g6da46f3, with additional
s/inspired to/inspired by/
And in commit logs I like to use the 8-hex-digit SHA1
representation (perhaps in addition to something like you've
provided) because gitk automatically detects that and renders
it as a clickable link.
I admit that in a non-interactive rendering of the log,
your longer form is much more useful.
> changes to reflect that the xz compression level should default
> to -e, not -9.
