Whitespace cleanups
I'm looking at working on a new area of wine (for me) and the file I'm going to be working with is rife with whitespace issues. Literal tabs are sprinkled around and the 80-col limit has been broken badly and often in easily fixable ways. I'm wondering what the acceptable approach is to these issues. I'd like to fix these issues before I start submitting functional patches. My question is this: Is it acceptable to fix the whole file in one patch (80-cols), or should I just fix the one function I'm planning on touching? Or should I grit my teeth and ignore the style issues entirely? Thanks, ~Nate
Nate Gallaher wrote:
I'm looking at working on a new area of wine (for me) and the file I'm going to be working with is rife with whitespace issues. Literal tabs are sprinkled around and the 80-col limit has been broken badly and often in easily fixable ways. I'm wondering what the acceptable approach is to these issues.
I'd like to fix these issues before I start submitting functional patches. My question is this: Is it acceptable to fix the whole file in one patch (80-cols), No. or should I just fix the one function I'm planning on touching? Or should I grit my teeth and ignore the style issues entirely? Yes, in most cases. If you plan to rework the whole file with functional patches feel free to touch formatting a bit. The common rule is to preserve existing format and remove things like spaces before tabs or trailing spaces (the rest is done while committing automatically).
What file are you talking about?
Nikolay Sivov wrote:
or should I just fix the one function I'm planning on touching? Or should I grit my teeth and ignore the style issues entirely? Yes, in most cases. If you plan to rework the whole file with functional patches feel free to touch formatting a bit. The common rule is to preserve existing format and remove things like spaces before tabs or trailing spaces (the rest is done while committing automatically). I understand the "preserve existing format" rule, particularly in cases of braces, inter-paren spacing, variable names, function names, and alignment, but I don't see the benefit of keeping 80-cols insanity around.
What file are you talking about? dll/ntdll/signal_x86_64.c
I took a first pass at 80-col fixing last night. There were some places where it wouldn't fit without breaking up some parallelism/symmetry/flow, so I just let the violation stand. From what James said, it sounds like if I hold myself to just the functions I touch, I should probably be ok. ~Nate
On Nov 25, 2009, at 9:13 AM, Nate Gallaher wrote:
I understand the "preserve existing format" rule, particularly in cases of braces, inter-paren spacing, variable names, function names, and alignment, but I don't see the benefit of keeping 80-cols insanity around.
One important reason to avoid whitespace-only changes is it makes git-blame essentially useless for finding the real source of functional changes. -Ken
Ken Thomases wrote:
One important reason to avoid whitespace-only changes is it makes git-blame essentially useless for finding the real source of functional changes.
But is that really important? git-bisect would put you on the other side of any whitespace changes, and a prudent bug-hunter should be referring to the commit logs anyway. There should be a path to eventual codebase sanity, no matter how slow. ~Nate
On Nov 25, 2009, at 11:13 AM, Nate Gallaher wrote:
Ken Thomases wrote:
One important reason to avoid whitespace-only changes is it makes git-blame essentially useless for finding the real source of functional changes.
But is that really important? git-bisect would put you on the other side of any whitespace changes, and a prudent bug-hunter should be referring to the commit logs anyway.
Who says git-blame is only useful on occasions when it makes sense to git-bisect? Or that one is hunting bugs. I often use git-blame precisely to figure out which commit log I need to be looking at to understand why the code is the way it is. I see something in the code which is done in a way I don't understand/expect. I git-blame to see what commit introduced the code in that form. I get a commit ID. I look up the commit to see what it contained, what the log message was, what commits were around it, etc. How would git-bisect be helpful in that case?
There should be a path to eventual codebase sanity, no matter how slow.
I think you're going to be disappointed. :-/ -Ken
On Wednesday 25 November 2009 19:02:08 Ken Thomases wrote:
On Nov 25, 2009, at 11:13 AM, Nate Gallaher wrote:
Ken Thomases wrote:
One important reason to avoid whitespace-only changes is it makes git-blame essentially useless for finding the real source of functional changes.
But is that really important? git-bisect would put you on the other side of any whitespace changes, and a prudent bug-hunter should be referring to the commit logs anyway.
Who says git-blame is only useful on occasions when it makes sense to git-bisect? Or that one is hunting bugs.
I often use git-blame precisely to figure out which commit log I need to be looking at to understand why the code is the way it is. I see something in the code which is done in a way I don't understand/expect. I git-blame to see what commit introduced the code in that form. I get a commit ID. I look up the commit to see what it contained, what the log message was, what commits were around it, etc.
There _is_ a -w option to git-blame that ignores whitespace, though. Adding two characters (possibly via an alias) is a small price to pay for non-braindead formatting imho. Cheers, Kai -- Kai Blin WorldForge developer http://www.worldforge.org/ Wine developer http://wiki.winehq.org/KaiBlin Samba team member http://www.samba.org/samba/team/ -- Will code for cotton.
On Wed, 25 Nov 2009, Ken Thomases wrote: [...]
One important reason to avoid whitespace-only changes is it makes git-blame essentially useless for finding the real source of functional changes.
Such changes don't make git-blame useless. They just make it a bit harder to use. Let's say your 'git-blame dlls/mshtml/tests/dom.c' leads you to commit 1e4412d7 which, by looking at it, you decide is a false positive. Then you do: $ git cat-file -p 1e4412d7 tree ea3b2e117b383ade8afc7cc9bf534c48faf891c7 parent 44f520a5b6f2bd6ec6da8793177711e57bda9c72 author Francois Gouget <fgouget(a)free.fr> 1258539107 +0100 committer Alexandre Julliard <julliard(a)winehq.org> 1258553736 +0100 mshtml/tests: Add a trailing '\n' to ok() calls. Then you run git-blame again: git blame 44f520a5 dlls/mshtml/tests/dom.c And continue the analysis until you find the right culprit. -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ I haven't lost my mind, it's backed up on tape around here somewhere...
On Nov 26, 2009, at 10:15 AM, Francois Gouget wrote:
On Wed, 25 Nov 2009, Ken Thomases wrote: [...]
One important reason to avoid whitespace-only changes is it makes git-blame essentially useless for finding the real source of functional changes.
Such changes don't make git-blame useless. They just make it a bit harder to use.
Let's say your 'git-blame dlls/mshtml/tests/dom.c' leads you to commit 1e4412d7 which, by looking at it, you decide is a false positive. Then you do:
$ git cat-file -p 1e4412d7 tree ea3b2e117b383ade8afc7cc9bf534c48faf891c7 parent 44f520a5b6f2bd6ec6da8793177711e57bda9c72 author Francois Gouget <fgouget(a)free.fr> 1258539107 +0100 committer Alexandre Julliard <julliard(a)winehq.org> 1258553736 +0100
mshtml/tests: Add a trailing '\n' to ok() calls.
Then you run git-blame again:
git blame 44f520a5 dlls/mshtml/tests/dom.c
And continue the analysis until you find the right culprit.
Um, it's just easier to add a caret ('^') to the commit ID. git blame 1e4412d7^ dlls/mshtml/tests/dom.c I'm well aware of this technique, but if it doesn't make git-blame useless, it makes it a hell of a lot less useful, especially if whitespace-only changes become non-rare. I wasn't aware of and haven't tried Kai's suggestion of using the -w switch. -Ken
I was just working on shell32/shlview.c: ShellView_OnNotify. There is so much mixing between tabs, spaces, different indent levels, trailing whitespace and what not that the code is properly unreadable. git-blame is one thing, but when it goes as far as almost preventing a developer to make changes... J. Leclanche / Adys On Fri, Nov 27, 2009 at 7:27 AM, Ken Thomases <ken(a)codeweavers.com> wrote:
On Nov 26, 2009, at 10:15 AM, Francois Gouget wrote:
On Wed, 25 Nov 2009, Ken Thomases wrote: [...]
One important reason to avoid whitespace-only changes is it makes git-blame essentially useless for finding the real source of functional changes
Such changes don't make git-blame useless. They just make it a bit harder to use.
Let's say your 'git-blame dlls/mshtml/tests/dom.c' leads you to commit 1e4412d7 which, by looking at it, you decide is a false positive. Then you do:
$ git cat-file -p 1e4412d7 tree ea3b2e117b383ade8afc7cc9bf534c48faf891c7 parent 44f520a5b6f2bd6ec6da8793177711e57bda9c72 author Francois Gouget <fgouget(a)free.fr> 1258539107 +0100 committer Alexandre Julliard <julliard(a)winehq.org> 1258553736 +0100
mshtml/tests: Add a trailing '\n' to ok() calls.
Then you run git-blame again:
git blame 44f520a5 dlls/mshtml/tests/dom.c
And continue the analysis until you find the right culprit.
Um, it's just easier to add a caret ('^') to the commit ID.
git blame 1e4412d7^ dlls/mshtml/tests/dom.c
I'm well aware of this technique, but if it doesn't make git-blame useless, it makes it a hell of a lot less useful, especially if whitespace-only changes become non-rare. I wasn't aware of and haven't tried Kai's suggestion of using the -w switch.
-Ken
participants (6)
-
Francois Gouget -
Jerome Leclanche -
Kai Blin -
Ken Thomases -
Nate Gallaher -
Nikolay Sivov