[petsc-dev] [petsc-users] MatMPIBAIJSetPreallocationCSR i and j indices

[Barry Smith]

  patch-review doesn't happen. Aside from you, none of the rest of us give a shit about patch review hence we should automatically check for whatever we can to make up for the lack of review.

  Lots of lame excuses why standards shouldn't be enforced automatically. Maybe we should work exclusively inside a IDE that can enforce all kinds of rules without security concerns etc etc or we could do development of each project in its own VM and thus "make normal commands behave differently inside the VM" without security fears. I don't buy these arguments. Our IDE is Emacs+shell+make+python and we can make that IDE help us out, it won't be perfect but if blocks 80% of the goof ups that is a great thing.

  There is also an enormous psychological/sociological advantage to "the development system" enforcing standards: people hate it when after days or weeks of working some wise-ass reviewer says your code doesn't adhere to standards and they will likely push less code in the future but when an automatic test rejects some code and tells the user how to fix it that doesn't make them less likely to push code in the future.  Linus has surely lost many decent developers over the years by being an asshole, we don't need to lose the few we have.  I'm not saying there should be no code review, just that the sooner in the process each problem is found the better for productively and I see no reason why each git repository couldn't have in it somewhere a list of rules for that project that git verifies are being followed as the user commits code, pushes code, edits code etc. Many of these could just be warnings, for example

commit -a directly into maint, next, or master:  "Are you sure you want to do this COMMIT DIRECTLY INTO maint, normally this is not desirable?"
branch off of next:  "Are you sure you want to branch off of NEXT?"
commit a file with // in it.  "Yoo Mark, PETSc requires the silly old fashion /* comments, you cannot commit this file since you used the modern //!"
variables declared in the middle of source code. "Yoo, we use C89 here, make sure all variables are declared at the top of scope!"
commit a file with hundreds of lines of commented out source. "Don't be putting commented out code in the repository!"
etc.

Going back to my maint screw up today. Just one useful warning from git/"our Emacs/shell/make/python IDE" along the way and we would have a nice branch we could continue to improve on, instead we have this ugly illegal commit directly into maint that will be harder to work with. Yes, you can easily say, "that Barry is an idiot who always fucks up procedure" but that doesn't help resolve the problem. One damn warning and it won't have happened, but no, you refuse to consider having any enforcement mechanisms at all and expect all developers to internalize 100% all these rules.

   Barry





On Sep 29, 2013, at 3:01 PM, Jed Brown <jedbrown at mcs.anl.gov> wrote:

> Barry Smith <bsmith at mcs.anl.gov> writes:
> 
>>> On the Reported-by tags, could you do them in a structured way as
>>> 
>>> Reported-by: Matteo Parsani <parsani.matteo at gmail.com>
>>> Reported-by: Lisandro Dalcin <dalcinl at gmail.com>
>> 
>>   This is fine, but there needs to be a mechanism to ensure this,
> 
> We already have whitespace conventions, a convention that there are no
> memory leaks, a convention that type-specialized functions use dynamic
> composition, a convention that we do not create dependency loops, a
> function naming convention, a convention about avoiding globals, a
> branch usage convention, etc.  None of these are enforced and only some
> have automated checking.
> 
>>   imposing requirements on developers that do not have enforcement
>>   mechanisms (besides being yelled at later) don't work. 
> 
> Patch review is more flexible than any automated tool can be.  It is
> only time to write an automated tool when the effort to do so, and the
> attainable accuracy, is a better use of human time than had the same
> time been devoted to patch review.  Note that patch review can catch
> many things, but automated tools can only catch that which has been
> automated.
> 
>>   Note that this is the same kind of situation as me accidentally
>>   editing directly into maint.  I did
>> 
>> git branch barry/fix-matmpibaijsetpreallocationcsr
> 
> This creates a branch without checking it out.  If you want to create
> the branch _and_ check it out, use
> 
> $ git checkout -b barry/fix-matmpibaijsetpreallocationcsr
> 
>> happily edited away and tested
>> git commit -a 
>> git push
>> 
>> Now what should have happened is as soon as I tried to happily edit
>> away, something should come back and said to me: "are you sure you
>> want to edit in maint?" 
> 
> This is policy on the branch rather than the fact that you created a new
> branch prior to starting this work.  (I do that sometimes so that I'll
> be able to easily compare to the old state later.)  We could perhaps
> prevent direct commits on 'maint'.
> 
>> And don't go and mail me some elisp that I can stick in somewhere to
>> warn me about the maint editing business; that would only solve the
>> problem for me, not for the n-1 other people in the world who make the
>> same mistake. Git needs some way of "baking in" a set of standards
>> (optional obviously) that it does its best to enforce and not leaving
>> it up to each user to remember 324 things they need to constantly be
>> checking depending on what project they are working on.
> 
> There is a security problem with making normal commands behave
> differently when run inside some repository that you just cloned.  Any
> such policy would need to be opt-in.