[lkml]   [2009]   [Jul]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [GIT PULL -tip][PATCH 0/9] MTRR fix trivial style patches

    * Jaswinder Singh Rajput <> wrote:

    > > I think even the MTRR code (which is indeed one of the few x86
    > > places still not fully cleaned up) supports my arguments.
    > >
    > > Look at the averages:
    > >
    > > errors lines of code errors/KLOC
    > > arch/x86/kernel/cpu/mtrr/amd.c 2 120 16.6
    > > arch/x86/kernel/cpu/mtrr/centaur.c 8 225 35.5
    > > arch/x86/kernel/cpu/mtrr/cleanup.c 0 1102 0
    > > arch/x86/kernel/cpu/mtrr/cyrix.c 10 275 36.3
    > > arch/x86/kernel/cpu/mtrr/generic.c 16 730 21.9
    > > arch/x86/kernel/cpu/mtrr/if.c 11 428 25.7
    > > arch/x86/kernel/cpu/mtrr/main.c 50 751 66.5
    > > arch/x86/kernel/cpu/mtrr/state.c 0 83 0
    > >
    > By these trivial mtrr cleanup patches:
    > errors
    > arch/x86/kernel/cpu/mtrr/amd.c 0
    > arch/x86/kernel/cpu/mtrr/centaur.c 0
    > arch/x86/kernel/cpu/mtrr/cleanup.c 0
    > arch/x86/kernel/cpu/mtrr/cyrix.c 0
    > arch/x86/kernel/cpu/mtrr/generic.c 0
    > arch/x86/kernel/cpu/mtrr/if.c 0
    > arch/x86/kernel/cpu/mtrr/main.c 0
    > arch/x86/kernel/cpu/mtrr/state.c 0
    > The following changes since commit 2137f10fd78ce8540ec4305f4dcd559039444544:
    > Ingo Molnar (1):
    > Merge branch 'perfcounters/urgent'
    > are available in the git repository at:
    > git:// master
    > Jaswinder Singh Rajput (9):
    > x86: mtrr/amd.c fix trivial style problems
    > x86: mtrr/centaur.c fix style problems
    > x86: mtrr/cleanup.c fix trivial style problems
    > x86: mtrr/cyrix.c fix trivial style problems
    > x86: mtrr/generic.c fix style problems
    > x86: mtrr/if.c fix trivial style problems
    > x86: mtrr/mtrr.h fix trivial style problems
    > x86: mtrr/state.c fix trivial style problems
    > x86: mtrr/main.c fix style problems
    > arch/x86/kernel/cpu/mtrr/amd.c | 7 +-
    > arch/x86/kernel/cpu/mtrr/centaur.c | 127 +++----------------------
    > arch/x86/kernel/cpu/mtrr/cleanup.c | 18 ++--
    > arch/x86/kernel/cpu/mtrr/cyrix.c | 30 +++---
    > arch/x86/kernel/cpu/mtrr/generic.c | 147 +++++++++++++++-------------
    > arch/x86/kernel/cpu/mtrr/if.c | 51 +++++++----
    > arch/x86/kernel/cpu/mtrr/main.c | 185 +++++++++++++++++++-----------------
    > arch/x86/kernel/cpu/mtrr/mtrr.h | 7 +-
    > arch/x86/kernel/cpu/mtrr/state.c | 20 +++--
    > 9 files changed, 263 insertions(+), 329 deletions(-)

    Jaswinder, i'm going to ignore such pull requests from you in the
    future, because your changes are exceedingly painful and you just
    dont seem to learn.

    This is the Nth incident, and there were a countless number of prior
    incidents where i warned you about various classes of avoidable
    problems, and you are not showing signs of significant improvements.

    To get more specific: in this case too it's the same old problem of
    low quality of patches from you again:

    - You managed to put _3_ separate bugs into these 'trivial'
    cleanups. That is not acceptable. If you cannot make trivial
    patches be truly trivial, you should not be doing them really.

    - i had to remove/undo/fix some bogus 'fixes' you did in those
    patches where you just blindly followed checkpatch instead of
    thinking about it for a minute. We dont want checkpatch warriors
    - we want people with taste who use it as a tool to keep their
    new patches tidy, or who use it as a tool to aid cleanups.

    - i had to complete the patches and do all the cleanups you missed
    - sometimes on the next time to a change you did. You apparently
    only checked checkpatch output - you didnt even look at all the
    files for how it all looks like and whether there are other
    things in those files that checkpatch missed. You made the files
    'checkpatch clean' - but you didnt actually look at whether the
    files got cleaned up for good really. You left a half-done
    jumbled mess behind you with files that still had inconsistent
    looking style in them. Check the deltas of the patches i
    committed versus the ones you sent to see the countless cases you
    missed. And you cannot even claim to have done the 'trivial'
    ones safely - because you did it in a buggy way and because the
    final cleanups i did are all trivial too and can be validated.

    - i had to double check each commit on the assembly level to prove
    that the patches are true cleanups. The new commit logs, size
    checks, md5 sums are proof of that due diligence process. You
    obviously didnt do any of that - you'd have noticed the bugs you
    have put in had you done that.

    - i had to edit _all_ your changelogs. Again. You _still_ dont
    think about your changelogs, they still suck 90% of the time.

    All things considered it took me 6 hours to fix up and complete your
    'trivial' patches into which you have put only 3 hours of work:

    - your buggy and incomplete cleanups did:

    9 files changed, 263 insertions(+), 329 deletions(-)

    - the proper, fixed, rounded up, checked, validated and working
    cleanups i committed with 6 extra hours of work:

    9 files changed, 868 insertions(+), 862 deletions(-)

    The MTRR code is a highly critical piece of x86 code and i had to
    check assembly dumps manually and compared them to make sure the
    changes dont introduce subtle regressions.

    The fact is, there is no other way to establish the trust level of
    trivial cleanups but to invest this depth of review and this level
    of testing and checking. I cannot just review until i find the first
    bug or problem and bounce it back to you as a review failure - i
    cannot know whether i can trust your next version of the patch and i
    cannot check the same trivial cleanups again and again as a machine,
    until you manage to submit the correct one.

    So i've tested the trustworthyness of your changes for a final time
    and you failed this test.

    I still committed it all under your name because i kept a fair
    amount of your changes so it's all v2 versions of your original
    patches - and per kernel convention i noted it in the changelog that
    i modified it further (and i didnt want to create 9 more commits,
    especially as some of your patches were broken so not bisection
    safe) - but this was the last time i went through such an effort
    with your patches.

    As i warned you _repeatedly_ in the past that you should insert a
    lot more effort into patches before sending any patches and before
    sending any pull request. Other x86 maintainers warned you about
    that too. You seem to prefer five patches a day (a few of which are
    inevitably broken) instead of one good patch every five days.

    This low level of patch quality just does not scale for maintainers
    of a busy tree which has more than a hundred contributors in every
    cycle. If i cannot trust a contributor i cannot apply such patches

    All in one, you were making the same kinds of mistakes again and
    again, over a many month time-span, and you are causing a
    considerable amount of maintainer overhead that is just not
    sustainable really.

    So this simply does not work in this form. I can work with pretty
    much anyone, but there's a final limit to the energy i can invest
    into this. Please find some other Linux maintainer who can work with
    you and who is willing to apply your patches. If you want to work on
    x86 bits please find an x86 developer or maintainer who is willing
    to apply your patches or who is willing to give you Reviewed-by tags
    before sending them to me.



     \ /
      Last update: 2009-07-05 20:25    [W:0.037 / U:5.060 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site