[petsc-dev] Pushing non-working code

[Jed Brown]

On Sun, Feb 3, 2013 at 12:01 PM, Matthew Knepley <knepley at gmail.com> wrote:

> http://kev.inburke.com/kevin/the-best-ways-to-find-bugs-in-your-code/
>>
>
> Notice here that "Informal code review" is the second worst form of
> checking by percentage of bugs
> found, and thus would appear to be an inefficient way to go about things.
> Formal code review does not
> happen every checkin, but only after a significant amount of work goes
> into the project. He makes a
> big deal about catching bugs early, which is a good point, but not the
> point here.
>

Also note how "informal design review" is significantly higher in the list.
I assume that includes stuff like an incomplete RFC patch.

Mailing list patches in the kernel workflow can be fairly formal. There is
usually discussion and once the reviewer is satisfied, they sign the
commits and it can move on to the next level.

Pull requests are one way to put a (non-email) interface on that process.
There are good continuous integration systems that automatically run the
test suite on each pull request so the reviewer also knows whether
everything passes before reviewing. Doing this before pushing to the main
repository means that the bugs can be fixed before users see it.

If you look back through our commit history, you'll find a large fraction
of retroactive bug fix patches. Every retroactive bug fix meant that
petsc-dev was unstable for some period of time.


>
>
>>
>> http://en.wikipedia.org/wiki/Code_review
>> http://www.codinghorror.com/blog/2006/01/code-reviews-just-do-it.html
>>
>>
>>> Extensibility: Your assertion is that extensibility benefits from code
>>> review. I agree. You are reviewing at the wrong time. Code reviews
>>> should be organized, not carried out after every checkin.
>>>
>>
>> My view is that the history should _be the organization_. If you want to
>> organize it differently, please suggest an alternative system.
>>
>
> Formal code review, which we had to do at Akamai, were organized meetings
> with all the developers, where
> we got a schedule of code to review, for what functionality, and with what
> aim in mind.
>

How was the "schedule of code" organized? How would you suggest that we
keep track of what constitutes a "reviewable feature" and how should we
mark it as ready? How can we keep track of that later, in the process of
understanding the origin of a bug or a design decision?

Using the DVCS to maintain that coherence is an established method that
works well for many projects in varied domains and with varied stability
requirements.


>
>
>>
>>> Ability to recognize distinct features in history: I do not think this
>>> is worth preserving at the cost of a lot more process. This is the linux
>>> model where everything is smoothed out into a series of clean patches. We
>>> have explicitly chosen not to do this. It obscures the actual development
>>> process and I am not convinced it is as useful here as they claim in the
>>> kernel.
>>>
>>
>> The amount of process is proportional to the required stability and
>> number of developers and users. A bug in kernel 'master' is an extremely
>> bad thing. Even when fixed the same day, it can cost many thousands of
>> dollars (it interrupts development for many developers and tends to
>> discourage early adopters). PETSc is not such critical infrastructure, but
>> I think it would be good to offer a more reliable low-latency way to
>> interact with applications, which in turn requires more stability. Also,
>> bugs that make it into a release are much more expensive than bugs fixed
>> early, even without counting the indirect effect of discouraging users.
>>
>> I'm not proposing the full kernel workflow, but having slightly better
>> feature provenance and encouraging up-front review would be good.
>>
>
> I am not proposing anarchy. I think everyone should push code with no
> warnings. We see from the nightly
> tests that it does not happen, but I am sure a timeline will reveal it is
> much better today. I agree that people
> should try to keep features together in coherent checkins. I label all my
> checkins at the top with the components
> that they impact. However, imposing an iron rule seems unwarranted, and I
> think its clear that hard rules and
> lots of process can have bad side effects (witness the reasonable sounding
> rule to MS that all checkins must
> pass the test suite, which resulted in horrible hacking to accomplish
> that).
>

So I'm just asking for better control of feature lineage and for
pushes/merges to be done at semantically meaningful places.

Our quality and process can be better today than in the past, yet still
have lots of room for improvement.


>
>
>>
>>> Warnings: I did respond to that, so its not worth repeating here.
>>>
>>
>> Only that you "try not to".
>>
>
> What else could I possibly promise? Promising infallibility shows a lack
> of understanding of the problem.


Wait until the function is supposed to work before pushing.

That way accidental warnings should actually be harmless and trivial for
anyone to clean up. When you push a non-working checkpoint, we can't even
fix the warnings because it can't be made correct without real development
and we don't want to cause you a merge problem later by just deleting
everything nearby. Then we get your noise in our build output until you get
around to finishing the feature.

It's really not hard, just don't push it until it's ready.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mcs.anl.gov/pipermail/petsc-dev/attachments/20130203/23a78dad/attachment.html>