Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1260254 > unrolled thread

RE: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups for nidstring.c

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups  for nidstring.c "Simmons, James A." <simmonsja@ornl.gov> - 2015-11-02 00:10 +0100
    Re: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups  for nidstring.c Michael Shuey <shuey@purdue.edu> - 2015-11-02 00:30 +0100
    Re: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups  for nidstring.c Dan Carpenter <dan.carpenter@oracle.com> - 2015-11-02 15:50 +0100
      RE: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups  for nidstring.c "Simmons, James A." <simmonsja@ornl.gov> - 2015-11-04 01:00 +0100

#1260254 — RE: [lustre-devel] [PATCH 1/3] staging: lustre: checkpatch cleanups for nidstring.c

>On Thu, Oct 29, 2015 at 07:28:21PM -0400, James Simmons wrote:
>> With nidstring now having the latest fixes we can
>> now clean up all the remaining checkpatch errors
>> for nidstring.c.
>
>Please be specific as to exactly what you changed, and break it up into
>one-patch-per-thing.  And no, "fix all checkpatch errors" is not "one
>thing"

Hmm. This makes me think I might be going about this wrong.  Instead of
doing style changes per file I should be doing one style change per subsystem
instead. Unless you prefer doing these style changes on per file base. Perhaps
for now I should focus on pushing the fixes that have cumulated and once
caught up then finished off the style issues. 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1260259

I suspect you're over-thinking it.  The maintainers appear to be
reacting to the different types of style changes - "checkpatch
cleanups" is an awfully broad commit message.  I'd suggest breaking
this patch (and any others like it) into two pieces; one with
whitespace cleanups, and one with the "== NULL" fixes (and mentioning
both by kind in the commit message, rather than just attributing to
checkpatch).  Then issue a v2 of the series, and see where you land.

Of course, YMMV. :-)

--
Mike Shuey


On Sun, Nov 1, 2015 at 6:07 PM, Simmons, James A. <simmonsja@ornl.gov> wrote:
>>On Thu, Oct 29, 2015 at 07:28:21PM -0400, James Simmons wrote:
>>> With nidstring now having the latest fixes we can
>>> now clean up all the remaining checkpatch errors
>>> for nidstring.c.
>>
>>Please be specific as to exactly what you changed, and break it up into
>>one-patch-per-thing.  And no, "fix all checkpatch errors" is not "one
>>thing"
>
> Hmm. This makes me think I might be going about this wrong.  Instead of
> doing style changes per file I should be doing one style change per subsystem
> instead. Unless you prefer doing these style changes on per file base. Perhaps
> for now I should focus on pushing the fixes that have cumulated and once
> caught up then finished off the style issues.
> _______________________________________________
> lustre-devel mailing list
> lustre-devel@lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1260688

Yeah.  That is often the fastest way to fix all the checkpatch warnings.

Checkpatch warnings are pretty mechanical.  Just send like 100 patches
at a time until everything is fixed.  Don't overthink.  Say your patch
breaks the alignment then you have to fix that, but otherwise only fix
one thing at a time.  Sometimes people will ask you to fix something
else on the same line, but just say "I didn't introduce that, but yes I
am planning to fix that in a later patchset since I am following the
one thing per patch rule."

Don't feel shame about sending many small patches.  We pretty much merge
everything.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261976

>Yeah.  That is often the fastest way to fix all the checkpatch warnings.
>
>Checkpatch warnings are pretty mechanical.  Just send like 100 patches
>at a time until everything is fixed.  Don't overthink.  Say your patch
>breaks the alignment then you have to fix that, but otherwise only fix
>one thing at a time.  Sometimes people will ask you to fix something
>else on the same line, but just say "I didn't introduce that, but yes I
>am planning to fix that in a later patchset since I am following the
>one thing per patch rule."
>
>Don't feel shame about sending many small patches.  We pretty much merge
>everything.

It was the sense of it taking forever with that amount of patches needed with
the one file approach. Looking at the back log of fixes its not as bad as I thought
for libcfs/LNet. Once those fixes are merged the style cleanups can happen
pretty quickly.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web