On Mon, Jun 11, 2018 at 21:59:19 -0400, John Hawkinson wrote:
> Valery Ushakov <u...@stderr.spb.ru> wrote on Tue, 12 Jun 2018
> at 04:50:36 +0300 in <20180612015036.ge15...@pony.stderr.spb.ru>:
>
> > 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
