After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 696471 - Fix trailing whitespace errors
Fix trailing whitespace errors
Status: RESOLVED NOTABUG
Product: gparted
Classification: Other
Component: application
GIT HEAD
Other All
: Normal normal
: ---
Assigned To: gparted maintainers alias
gparted maintainers alias
Depends on:
Blocks:
 
 
Reported: 2013-03-23 22:30 UTC by Sinlu Bes
Modified: 2013-03-27 15:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Result of the cleanup command (303.72 KB, patch)
2013-03-23 22:30 UTC, Sinlu Bes
none Details | Review
remove trailing spaces hook (1.57 KB, application/octet-stream)
2013-03-25 17:05 UTC, Sinlu Bes
  Details

Description Sinlu Bes 2013-03-23 22:30:24 UTC
Created attachment 239650 [details] [review]
Result of the cleanup command

The output of the command:

$ find . -type f \( -name "*.cc" -o -name "*.h" \) -exec bash -c 'grep -e "\s$" "$0" | wc -l | xargs printf ' {} \; -print | sed '/^0\./d'

lists a total number of 90 files affected of trailing (white-)space errors for the source code. Contributors to gparted which copy and then modify such code may run into (white-)space errors they didn't cause.
The command (GNU sed needed)

$ find . -type f \( -name "*.cc" -o -name "*.h" \) -exec sed -i 's/\s\s*$//g' {} \;

removes trailing spaces.
Comment 1 Patrick Verner 2013-03-23 22:46:17 UTC
I obviously need help with this issue. ;) If it's okay with the GParted developers, I'll make a patch that wipes out all the white space errors. I can have this ready tomorrow morning.
Comment 2 Sinlu Bes 2013-03-24 02:12:19 UTC
Hello Patrick,

Thank you for your help. ;)

Sorry for not adding a hint in the description, but I had already attached a patch that adresses this issue. It only needs to be applied and pushed to the repo by the developers.

You could help by reviewing and testing the patch. If you think it doesn't help, or causes issues, you're welcome to post your own :).

Greetings

Sinlu
Comment 3 Patrick Verner 2013-03-24 02:26:29 UTC
I was being sarcastic because I recently submitted a patch filled with white space errors that were my fault. I also didn't see the patch until I opened my big mouth. I see it now. I'll look it over in the morn and see if I have any useful input. I'm sure you have it covered though.
Comment 4 Curtis Gedak 2013-03-24 16:29:20 UTC
Hi Patrick and Sinlu,

As you have discovered, the GParted code base contains many instances of trailing white space.

There are implications to cleaning these up with an all-encompassing patch.  Specifically I think it can make it harder to track problems back to the last time a line of code was changed.  If I a wrong in this regard, then please let me know.  I am thinking of when we need to find regressions in our code using things like git bisect.

If I recall correctly Rogier Goossens had suggested cleaning up all the trailing white space a while back.  At that time we decided to only clean up white space for code that we were actually changing.  This will slowly clean up the trailing white space errors over a long period of time as each piece of code is enhanced.  Our goal with new patches is to never introduce new trailing white space.

If anyone knows more about the potential impact (or not) of removing all the trailing white space, then I am all ears.  :-)

Curtis
Comment 5 Curtis Gedak 2013-03-24 16:39:20 UTC
One side effect of this patch is that it breaks many of the existing patches for GParted that are awaiting review.  This is a short term problem, but is one of the side effects of all encompassing changes like this patch.
Comment 6 Patrick Verner 2013-03-24 18:00:15 UTC
What patches are awaiting review? Is there 1000 or just 10?

If you want Curtis, you could apply Sinlu's patch and I could help re-base the patches in review if needed. It would help me get more familiar with GParted's code at worst.
Comment 7 Curtis Gedak 2013-03-24 18:11:03 UTC
Hi Patrick,

We track the development plans for the next release of GParted in the GParted General Development forum.  See:

General Development
http://gparted-forum.surf4.info/viewforum.php?id=4

The problem with wholesale removing of white space is only temporarily with updating outstanding patches, which we like to keep with the original authors.  I am more concerned about the difficulty such a patch can introduce when trying to search back trying to find a regression in code.

An example that would demonstrate this would be trying to perform a diff compare of two different GParted releases.  If there is a wholesale change to remove the white space patch applied between the two releases, then it will appear as if all the files changed.  This can make it much more difficult to locate problems that originate before the wholesale change was applied to the code.
Comment 8 Mike Fleetwood 2013-03-24 19:38:11 UTC
Just to back up what Curtis has said we prefer not to make white space
only changes, and instead just get white space right when making new
changes.  This seems to be best.  We do this so that we can use git
blame to identify the commit which changed a line of code and can find
the reason for that change.  Otherwise git blame will only report a
white space only commit.  It can be worked around, but we prefer not to.
Comment 9 Curtis Gedak 2013-03-24 20:10:54 UTC
In an effort to help future developers avoid the "whitespace problem" when creating their first patches for GParted, I have added a tip to "Ensure patch applies cleanly" to the GParted git page.

See, http://gparted.org/git.php

You might have to refresh the git page (F5 key) in your browser to see the newly added tip.
Comment 10 Sinlu Bes 2013-03-25 00:02:08 UTC
My idea was, without whitespace errors, one could run the command mentioned above to kill all trailing whitespaces before commiting, without affecting other files not needed for the commit.
There are also editors out there having such features.
But all these methods remove all white spaces, not just the ones caused by the one who invokes them.

Because of the reasons Curtis and Mike mentioned, removing all whitespace errors with one stroke is not a good idea.

There is a stackoverflow thread on how to remove only trailing white spaces of a single commit:
http://stackoverflow.com/questions/591923
Most answers promote to create a git hook, invoked before every commit, that removes the introduced whitespace errors in a commit automatically. Its a simple way to master the problem.
I could expand the hook to create support for tabs after spaces.

I see, git blame breakage is a good point.
There are lines in code that are clearly not containing anything that can influence the program, so they won't cause bugs. Such lines are for example lines only containing space characters or comment lines.
As an example, take the lines immediately after the licence header. A lot of gparted's code files have a space there, as the only character in that line.
These lines are less likely to vanish because of changes to the code, as they are at a position in the code which is less likely to be affected by changes.

I have modified my command to only fix these no-error-causing lines:
find . -type f \( -name "*.cc" -o -name "*.h" \) -exec sed -i 's/^\s\s*$//g' {} \;

To have a prettier diff compare between two gparted versions, i could create a git hook which fixes all faulty blank lines in files changed by a specific commit.

@Curtis
This command can check for white space errors while in active development:
git diff-index --check --cached origin/master --
Perhaps this could help future developers if you put this hint onto the git.php page. ;)

Greetings,
Sinlu
Comment 11 Sinlu Bes 2013-03-25 17:05:20 UTC
Created attachment 239790 [details]
remove trailing spaces hook

OK, I realize now that unlike .gitignore, hooks for git cannot be added into git versioning, so they need to be enabled manually by the developer.
Because of that i post an enhanced space cleaning hook, so that everybody can add it, or use it manually, if wanted.
For adding just rename/move/link the file to .git/hooks/pre-commit.

Of course, one can think of including a special command into the makefile which copies the hook from a versioned directory into .git/hooks.

The hooks from stackoverflow turned out to have lots of flaws. All of them fixed the whole changed file, not just the lines affected.
So they cannot fix the git blame issue.
In addition, they use the working directory version of a file as base for whitespace error fixing.
They should use the staged version instead, and leave the working directory version untouched.
This is important when using the script as a hook. I admit, when using the script manually this could make things more complicated, as the changes are only in the index, and not in the working directory.
The enhanced hook meets these needs, however it doesn't fix spaces followed by tabs, as i have no idea of how to fix them in an automated way.

Greetings,
Sinlu
Comment 12 Curtis Gedak 2013-03-25 19:34:15 UTC
Hi Sinlu,

Thank you for your work on this patch, the git-hook, and for your understanding of our position on not wishing to clean up all of the trailing white space at once.

You make a good point in comment #10:

> There are lines in code that are clearly not containing anything that
> can influence the program, so they won't cause bugs. Such lines are
> for example lines only containing space characters or comment lines.
> As an example, take the lines immediately after the licence header. A
> lot of gparted's code files have a space there, as the only character
> in that line.  These lines are less likely to vanish because of
> changes to the code, as they are at a position in the code which is
> less likely to be affected by changes.

It is indeed unlikely that these lines will need to be changed due to code enhancements.  However, if we clean these up in a single commit, then that will impair our ability to do a side-by-side directory diff compare between two gparted development trees when one includes the commit and the other does not.  

Instead I would suggest that if we are changing code nearby (within a few lines), for example adding a #include to a file, then that might be a good time to clean up this trailing white space for the affected file only.


> This command can check for white space errors while in active
> development:
>   git diff-index --check --cached origin/master --
> Perhaps this could help future developers if you put this hint onto
> the git.php page. ;)

Thanks Sinlu for this tip.  Since this report contains much useful information I have added a link from the GParted git page to this bug report.


Another tip I have is for emacs users.

TO DISPLAY TRAILING WHITESPACE IN EMACS:

Edit the ~/.emacs file to add the following line:
(setq-default show-trailing-whitespace t)


If everything looks okay to you, then I will close this bug report as WONTFIX since it our design decision not to perform a wholesale clean up of whitespace in the existing GPArted code base.
Comment 13 Sinlu Bes 2013-03-26 23:18:35 UTC
Hi Curtis,

thank you for adding a link to this bug to the git page, and thank you for the tip with emacs.

I think you can close the bug now.
Comment 14 Curtis Gedak 2013-03-27 15:08:13 UTC
Based on our discussion of our choice to not to remove all trailing whitespace in a single commit, I think the description of NOTABUG best matches this report.

  NOTABUG
    The problem described is not actually a bug, but a design choice of
    some sort.

As such I am closing this bug report as RESOLVED - NOTABUG.