>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.
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. <[email protected]> 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
> [email protected]
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
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
>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.