On Sun, Sep 29, 2013 at 3:07 PM, Barry Smith <[email protected]> wrote:
> > 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?" > If you put this little "script" into .git/hooks/pre-commit, it will do it: > 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 <[email protected]> wrote: > > > Barry Smith <[email protected]> writes: > > > >>> On the Reported-by tags, could you do them in a structured way as > >>> > >>> Reported-by: Matteo Parsani <[email protected]> > >>> Reported-by: Lisandro Dalcin <[email protected]> > >> > >> 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. > >
