lkml.org 
[lkml]   [2013]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/3] Staging: olpc_dcon: replace some magic numbers
    On Fri, Aug 16, 2013 at 09:40:38AM +0200, Jens Frederich wrote:
    > On Fri, Aug 16, 2013 at 9:13 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
    > > On Thu, Aug 15, 2013 at 09:34:55PM +0200, Jens Frederich wrote:
    > >> The 0x42 initialize squence 0x101 is wrong. According to
    > >> the specification Bit 8 is reserved, thus not in use.
    > >> I removed it.
    > >
    > > Really these code changes should be in a separate patch and labeled
    > > "Don't set reserved bit." instead of hidden away inside a cleanup
    > > patch.
    > >
    >
    > The patch is applied. Still, good to know. It's not so easy to find the
    > right patch granularity as newbie.
    >

    Yeah. Staging is for educating people about kernel process as much
    as it is about writing code.

    The rule here is "Don't mix code changes into a cleanup patches."
    What we want is if you have a bug then you can look through
    `git log --oneline` output and guess which patch introduced the bug.
    This patch is a cleanup patch so it shouldn't introduce any code
    changes or any bugs.

    Meanwhile, if you are making a code change you can make any cleanups
    you need to in order to do the change. Also if there is an existing
    checkpatch warning on any of the lines you touch, then that's ok to
    fix as well. Or if there are tiny related changes than that's fine.

    There are three problems with big patches:
    1) It breaks the --oneline summary to mix two things into one patch.
    2) It makes the patch harder to review. For example, sometimes
    people fix a bug and rename 10 variables as well.
    3) The more lines your patch is, the more chance there is that we
    will reject it based on one of those lines. You don't like
    redoing patches and we don't like making people redo them. So
    small patches are better and put the more controversial ones at
    the end so the first patches can be applied.

    No one totally agrees what "small closely related cleanups" means so
    it's better to be conservative.

    regards,
    dan carpenter



    \
     
     \ /
      Last update: 2013-08-16 11:21    [W:3.109 / U:0.800 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site