Re: [PATCH] Staging: wlags49_h2: fixed 80 line character and white space issue in ap_h2.c Fixed various coding style warnings found by the checkpatch.pl tool

Re: [PATCH] Staging: wlags49_h2: fixed 80 line character and white space issue in ap_h2.c Fixed various coding style warnings found by the checkpatch.pl tool

[Greg KH]

On Wed, Jul 31, 2013 at 01:44:25PM -0700, Evan Ng wrote:
> From: esn89 <evan.sin.ng@gmail.com>

I need a real name here, odds are "esn89" isn't yours :)

Also, look at the subject you created, it's not correct, you need to add
a newline after a short description, followed by the longer description,
for git to pick this up properly.

> Signed-off-by: Evan Ng <evan.sin.ng@gmail.com>
> ---
>  drivers/staging/wlags49_h2/ap_h2.c | 7884 +++++++++++++++++++++---------------
>  1 file changed, 4668 insertions(+), 3216 deletions(-)

That's almos impossible to review, can you do "one thing at a time" so
that we can review it?  Break it up into lots of different pieces and
send a series of patches.

thanks,

greg k-h

Re: [PATCH] Staging: wlags49_h2: fixed 80 line character and white space issue in ap_h2.c Fixed various coding style warnings found by the checkpatch.pl tool

[Greg KH]

On Thu, Aug 01, 2013 at 03:08:21PM -0700, Evan Ng wrote:
> Thanks for the reply, 
> 
> Being this is my first patch that I have submitted, I want to clarify a few
> things:
> 
> My *corrected* subject should read something like this? 
> 
> "Subject: [PATCH] Staging: wlags49_h2: fixed 80 line character and white space
>                issue in ap_h2.c Fixed various coding style warnings found by
> the
>                checkpatch.pl tool
> \n
> LONGER DESCRIPTION GOES HERE"

No.

It should be:
	Subject: [PATCH] Staging: wlags48_h2: ap_h2.c: fixed tab whitespace issue

	This patch fixes the tab vs. spaces white space issues in this
	file.

	Signed-off-by: ....

> As for the doing "one thing at a time" would it be preferred if I fix the
> syntax of one function within a file a time?  Would that mean that for one
> simple .c file I could be making 15 or so commits to it?

How about fixing one specific warning at a time for the whole file?
Like tab issues, and then { issues, and then ' ' issues, and so on.  See
the archives of the mailing list for lots of examples of how to do this
properly.

hope this helps,

greg k-h

Re: [PATCH] Staging: wlags49_h2: fixed 80 line character and white space issue in ap_h2.c Fixed various coding style warnings found by the checkpatch.pl tool

[Joe Perches]

On Fri, 2013-08-02 at 06:28 +0800, Greg KH wrote:
> On Thu, Aug 01, 2013 at 03:08:21PM -0700, Evan Ng wrote:
> > Thanks for the reply, 
> > 
> > Being this is my first patch that I have submitted, I want to clarify a few
> > things:
> > 
> > My *corrected* subject should read something like this? 
> > 
> > "Subject: [PATCH] Staging: wlags49_h2: fixed 80 line character and white space
> >                issue in ap_h2.c Fixed various coding style warnings found by
> > the
> >                checkpatch.pl tool
> > \n
> > LONGER DESCRIPTION GOES HERE"
> 
> No.
> 
> It should be:
> 	Subject: [PATCH] Staging: wlags48_h2: ap_h2.c: fixed tab whitespace issue
> 
> 	This patch fixes the tab vs. spaces white space issues in this
> 	file.
> 
> 	Signed-off-by: ....

> > As for the doing "one thing at a time" would it be preferred if I fix the
> > syntax of one function within a file a time?  Would that mean that for one
> > simple .c file I could be making 15 or so commits to it?
> 
> How about fixing one specific warning at a time for the whole file?
> Like tab issues, and then { issues, and then ' ' issues, and so on.  See
> the archives of the mailing list for lots of examples of how to do this
> properly.

Another possibility is to do all the individual line
whitespace changes in a single patch like trailing spaces,
spaces to tabs, pointer positioning, spaces after
if/while/do/for, etc.

Make sure these changes don't add or delete lines.

Use "git diff -w" to make sure that are no other changes.

Verify that the objects do not change before and after
the change.

That way you don't have to touch the same line multiple
times with all the different types of whitespace changes.

Submit that single patch then do other changes like
80 column wrapping if that sort of change appeals to you.

Re: [PATCH] Staging: wlags49_h2: fixed 80 line character and white space issue in ap_h2.c Fixed various coding style warnings found by the checkpatch.pl tool

[Dan Carpenter]

Hi Evan,

Are you sending html email or something?  Your emails aren't showing
up on the devel@driverdev.osuosl.org list.

regards,
dan carpenter

Re: [PATCH] Staging: wlags49_h2: fixed 80 line character and white space issue in ap_h2.c Fixed various coding style warnings found by the checkpatch.pl tool

[Evan Ng]

Re: [PATCH] Staging: wlags49_h2: fixed 80 line character and white space issue in ap_h2.c Fixed various coding style warnings found by the checkpatch.pl tool

[Henk de Groot]

Hello Evan,

When reformatting the array containing  the firmware data it may be easier 
to use 8 bytes per row. It is currently it is 16 bytes per row 
(overrunning the 80 characters/line) and this just means cutting each row 
exactly in half. In your first proposal it was completely reformatted to 
an odd 11 bytes per row which makes comparison with the original during 
review much harder and it is also easier to make mistakes.

Kind regards,

Henk.