On Thu, Dec 16, 2004 at 05:41:32AM -0400, bulia byak wrote:
> crashes now. dir-utils.cpp:30 is dying from a NULL value
> for 'base.'
Latest CVS still craches on bitmap import. Looks like the person who
touched this code recently was pjrm. Peter, could you please look into
this? And please test your changes thoroughly before committing. (Of
course errors happen, but this is a very obvious bug which should have
been caught before committing).
I did indeed test before committing, both by using inkscape and ensuring
that the changed code was executed, and even by creating a new unit test
program (dir-util-test.cpp). To detect this bug, it isn't sufficient to
test the changed code or even to test every caller of the changed code.
I suggest that this bug is not caused by inappropriately little testing
by the developer.
What then can we learn from this bug in terms of how to change our
development practice?
One response would be: "The bug has been found in the normal course of
use, and is believed now fixed. We needn't change our development
practice."
Another response would be to encourage greater peer review. For this
bug, a reviewer of the diff might have noticed the change in behaviour
for base==NULL, and might question whether this change affected any
callers.
The cost would be more time spent reviewing instead of developing, and
more time spent responding to reviewer suggestions. In one project I
know of (developing a compiler, so they have higher quality obligations
than inkscape does), they require all non-trivial diffs to be posted to
a mailing list and reviewed & OK'd by someone before committing. (Some
simple/important changes can be committed before being OK'd, but still
require posting to the mailing list.) If we do go with the `review
more' approach, then I suggest that we start much lower key, merely
encouraging ppl to do `cvs -q diff -dpU5 -r BASE -r HEAD' and read the
diffs.
Incidentally, please compile your changes before committing, and look
for any introduced warnings. E.g. this would have alerted Bulia to a
bug he introduced a few days ago which I've now fixed.
Btw, I won't be around for the next week or two. Visiting my parents in
the country.
pjrm.