[petsc-dev] Pushing non-working code

[Matthew Knepley]

On Sun, Feb 3, 2013 at 12:02 PM, Jed Brown <jedbrown at mcs.anl.gov> wrote:

>
> 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.
>

If the checkin you originally complained about the build did not fail and a
memory leak did not appear. You
still cannot explain what was wrong there, so you proceed to a different
problem.


> 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
>

Yes, a memory leak did appear here. However, it appeared in a complete
checkin that had a test to go with it,
exactly as you ask. We see that even that is insufficient sometimes to
discover a minor bug like this. So your
second example is really about the shortcomings of the strategy you propose.

   Matt


>
> 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)
>
>


-- 
What most experimenters take for granted before they begin their
experiments is infinitely more interesting than any results to which their
experiments lead.
-- Norbert Wiener
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20130203/cda3e01c/attachment.html>