re: Whitespace changes in an indentation patch, is it acceptable?
Tom wrote:
Hi all, just curious, in my work on uninstaller, I am writing my patches to where when indentation is changed, due to adding a for loop, it is done in a separate patch file. I was wondering if it is acceptable to make whitespace changes to other parts of the file in that same patch.
In general, mixing large whitespace changes with small functional changes make it hard to code review the functional change. But either way can be fine, it just depends on the situation. Now that you've fixed a couple bugs in your uninstall patch, I think you should post the very simplest form of it possible, without any other change mixed in. In this case, I think that means you should do the whitespace changes in a second patch. - Dan
On 5/31/07, Dan Kegel <dank(a)kegel.com> wrote:
Now that you've fixed a couple bugs in your uninstall patch, I think you should post the very simplest form of it possible, without any other change mixed in. In this case, I think that means you should do the whitespace changes in a second patch. Right, which is the way the patch is already done, I just wanted to see if making changes to the whitespace in other parts of the file (via the 2nd patch, which changes the indentation of my additions) would be considered random whitespace changes, or if it would be helpful, because it reduces filesize..
I recall a few years back (2003?) There was a discussion about removing extra whitespace at the end of lines, and someone came up with a bash/sed script to look thru the entire wine tree, strip trailing whitespace, and then somehow commit it (either with a really large diff, or by it being run on the machine that the upstream tree is on, I cant remember which). Once that was done, the tarballs shrank by at least 1-2 megs in download size, and even more uncompressed. IMHO it may be time to do that again... -- Thanks Tom
On Thu, May 31, 2007 at 02:23:59PM -0500, Tom Spear wrote:
I recall a few years back (2003?) There was a discussion about removing extra whitespace at the end of lines, and someone came up with a bash/sed script to look thru the entire wine tree, strip trailing whitespace, and then somehow commit it (either with a really large diff, or by it being run on the machine that the upstream tree is on, I cant remember which). Once that was done, the tarballs shrank by at least 1-2 megs in download size, and even more uncompressed.
I was curious, and got a really rough estimate of the number of files with trailing whitespace: $ find /var/work/wine/ -name '*.[chly]' | fgrep -v .tab | fgrep -v .yy \ | xargs grep -c '[[:space:]]\+$' | fgrep -v :0 | wc -l reported 1081 files. Checking for trailing whitespace after a backslash didn't turn up anything (which is good). I hardly think getting rid of trailing whitespace will shrink the tarballs by any significant amount, though.
participants (3)
-
Dan Hipschman -
Dan Kegel -
Tom Spear