Re: CVS commit: src/usr.bin/gzip

[Valery Ushakov <uwe%stderr.spb.ru@localhost>]

On Mon, Jun 11, 2018 at 21:59:19 -0400, John Hawkinson wrote:

> Valery Ushakov <uwe%stderr.spb.ru@localhost> wrote on Tue, 12 Jun 2018
> at 04:50:36 +0300 in <20180612015036.GE15651%pony.stderr.spb.ru@localhost>:
> 
> > Please, can you keep your commit messages to the point?
> > 
> >   "Fix unportable left shift"
> > 
> > is probably a good enough summary.  You don't have to paste the test
> > suite results and the actual diffs in free form as well.
> 
> What is wrong with a long commit message, as long as it is
> summarized clearly at the top, as was done here? I'd much rather
> have a long explanation than only the summary.

There's a fine line between an explanatory and an overly long commit
message (as Bryan Cantrill put it in one interview, "there's a fine
line between elegant and sleazy").

Also the communication context of a log message is the output of cvs
log.  The commit mail (i.e. the context of source-changes) may be a
factor but only a secondary one.


| Correct Undefined Behavior in gzip(1)

This is being committed to gzip.c.  You will read this commit message
when reading the output of cvs log for gzip.c.  I don't think it's
useful to repeat in the commit message that this commit is to gzip.


| Unportable left shift reported with MKSANITIZER=yes USE_SANITIZER=undefined:
|
| # progress -zf ./games.tgz  tar -xp -C "./" -f -
| /public/src.git/usr.bin/gzip/gzip.c:2126:33: runtime error: left shift of 251 by
|  24 places cannot be represented in type 'int'
| 100% |**************************************************************************
| **************************************| 44500 KiB  119.69 MiB/s    00:00 ETA


An example of incorrect behaviour of a program that is being fixed is
informative.  But the above is like fixing a missing semicolon and
quoting compiler error in the commit message.  E.g.

#   compile  ofwboot/Locore.o
/home/uwe/work/netbsd/build/tools/bin/powerpc--netbsd-gcc -Os -ffreestanding  -msoft-float  -fno-unwind-tables -fno-asynchronous-unwind-tables -Wall -Wmissing-prototypes -Wstrict-prototypes -Wpointer-arith    -std=gnu99   -Werror     -D_STANDALONE -DSUPPORT_DHCP -DSUPPORT_USTARFS -DHAVE_CHANGEDISK_HOOK --sysroot=/home/uwe/work/netbsd/build/distrib/macppc -I. -I/home/uwe/work/netbsd/ro/src/sys/arch/macppc/stand/ofwboot -I/home/uwe/work/netbsd/ro/src/sys/arch/macppc/stand/ofwboot/../../.. -I/home/uwe/work/netbsd/ro/src/sys/arch/macppc/stand/ofwboot/../../../.. -DRELOC=0xE00000 -DRELOC_FLATFILE=0x -I/home/uwe/work/netbsd/ro/src/sys/arch/macppc/stand/ofwboot/../../../../lib/libkern/../../../common/lib/libc/quad -I/home/uwe/work/netbsd/ro/src/sys/arch/macppc/stand/ofwboot/../../../../lib/libkern/../../../common/lib/libc/string -I/home/uwe/work/netbsd/ro/src/sys/arch/macppc/stand/ofwboot/../../../../lib/libkern/../../../common/lib/libc/arch/powerpc/string  -c    /home/uwe/work/netbsd/ro/src/sys/arch/macppc/stand/ofwboot/Locore.c
/home/uwe/work/netbsd/ro/src/sys/arch/macppc/stand/ofwboot/Locore.c: In function 'setup':
/home/uwe/work/netbsd/ro/src/sys/arch/macppc/stand/ofwboot/Locore.c:659:1: error: expected ';' before '}' token
 }
 ^

Do you think that would be a reasonable part of a commit message?

 
| Refactor the following code into something that is more clear
| and fix signed integer shift,

This is the third time the log tells us it's fixing a left shift,
counting the first sentence of the above passage and the output from
the sanitizer.


| [...]  by casting all buf[] elements to
| (unsigned int):
| 
| unsigned char buf[8];
| uint32_t usize;
| [...]
| else {
|     usize = buf[4] | buf[5] << 8 |
|             buf[6] << 16 | buf[7] << 24;
| [...]
| 
| New version:
| 
|     usize = buf[4];
|     usize |= (unsigned int)buf[5] << 8;
|     usize |= (unsigned int)buf[6] << 16;
|     usize |= (unsigned int)buf[7] << 24;

This just repeats the actual diff in free form.


| Only the "<< 24" part needs explicit cast, but for consistency make the
| integer promotion explicit and clear to a code reader.

I'm on the fence for this one.  I'd say it's redundant.  It doesn't
help that the actual change is sloppy as it casts to a different type
than the result - usize is uint32_t, not unsigned int.


| Sponsored by <The NetBSD Foundation>

I'm not sure this is relevant either.


To sum it up, out of 30+ lines of the commit message, the relevant
information is contained only in (part of) one line.

-uwe