On Tue, 8 Apr 2008, Peter Stuge wrote:
On Mon, Apr 07, 2008 at 03:56:33PM -0600, Marc Jones wrote:
Peter Stuge wrote:
Do you have concerns that the patch is confused by the changes
Yes, I think it is.
or are you just naking on principal?
No, not really.
This is how I reason:
So, yes, you are naking on principal.
It was sort-of a trick question since the principle applies when I think whitespace changes cause confusion.
I NAKed because I thought the whitespace changes were too confusing in this patch, not because of the principle.
On Mon, Apr 07, 2008 at 05:10:34PM -0700, Stefan Reinauer wrote:
While in theory I agree with Peter that separating whitespace is a lot easier to read, I don't believe this is really a practical approach.
Sure it is, just don't change whitespace until the code changes are done.
When working on code, I fix whitespace on the fly, if I do at all.
My point is that it is much better for patch readability and long term repo quality if whitespace is not changed at all while making changes to code.
I never put in a separate round for just fixing whitespace because that's just a bogus way of spending precious time.
Me neither. I don't want to change whitespace at all. Sometimes I will actually play around with comments and whitespace during my thought process but I review and restore forgotten changes before sending a patch.
So is seperating the whitespace fixes from the rest of the code after working on it.
Yes, I completely agree. Improvements done after the fact indeed waste some time but also lead to higher quality source.
Personally I don't care at all about whitespace, but I do care that changes in whitespace is mixed with changes in code.
I am the first to admit that following any coding style will be at least slightly annoying unless it happens to be your own style. :)
If it happens to be viable, yes, please try to make it like that, but NACKing a patch for that reason is unacceptable.
Hm, I should NAK patches that I have reviewed and don't want committed, right?
Therefore I say Acked-by: Stefan Reinauer stepan@coresystems.de for that patch as it is now, and I hope we can keep this low-profile.
Why not add the -b option when using diff? (same as --ignore-space-change ) See man diff for other useful options.
my .02 Russ