[petsc-dev] Pushing non-working code

[Jed Brown]

On Sun, Feb 3, 2013 at 9:35 AM, Matthew Knepley <knepley at gmail.com> wrote:

> Again, you misuse words when it is convenient for you. This is dishonest
> argument.
>
> "Stable" for applications means that the behavior of the application will
> not change. This is
> definitely true for your example, so characterization as "unstable" is
> plainly wrong.
>

If a build fails or a memory leak appears, it definitely impacts the
application. In the particular example from yesterday, the build did not
fail, they were "just warnings". But the warnings were from obviously
broken code and cause the application developer to question whether that
code might affect them. This sort of thing erodes trust and willingness to
follow petsc-dev.

Since we make API changes in petsc-dev, people may need to update their
code. For example, I updated parmod last night and had to update a few
things for it to build, but then -malloc_dump told me about a memory leak.
If I was not a PETSc developer, should I have expected it to be due to a
bug in my code or a new bug in PETSc? As it turns out, you introduced the
memory leak as part of a bunch of other stuff

https://bitbucket.org/petsc/petsc-dev/commits/16ecfb2c3de392ee3e460feb23b4342218c52c8a

Even knowing that component of PETSc well, the one-line commit message
"DMPlex: Serial test for two triangles working for cohesive cell
introduction" sure doesn't suggest that something called by
DMPlexConstructGhostCells was refactored. I fixed the bug quickly

https://bitbucket.org/petsc/petsc-dev/commits/2f3734bedc2dade526c09f6b0348515e6a420442

but these things can waste a lot more users' time and turn them off of
following petsc-dev.


When you push incomplete code as a checkpoint, it discourages the rest of
us from looking at it. Pushing it in that form offers no value at all, and
breaks the coherent thread of a feature. It's using a DVCS with an
almost-CVS workflow. When writing code, we often put more work into it
up-front to make it easier to maintain later. We're writing it not just to
execute, but also to be readable. Coherent history helps us all understand
what is going into PETSc. It helps us review and spot bugs before users
find them. More importantly, the review process improves
maintainability/extensibility [1]. When code is checkpointed haphazardly,
the planned design is often not yet coherent enough to even comment on
whether the design is good.

The only professional thing to do is to write code not just with the intent
of being readable once "complete", but specifically to be understandable
_incrementally_. This means breaking patches into pieces that have some
meaning on their own and only merging at semantically meaningful places.

[1] http://lib.tkk.fi/Diss/2009/isbn9789512298570/article5.pdf (and others)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20130203/62cdcbe6/attachment.html>