Introduce git diff --check or something similar

[obastemur]

I see the same source file I had cleaned up from white spaces is now with of new whitespaces.

It is really hard to see what was changed with the commit due to white-space noise.

We might introduce git diff --check or something similar to limit whitespace noise.

[Krovatkin]

@obastemur we could consider introducing a --pre-commit git hook which triggers a bash/perl script to clean up extra new lines and whitespaces. Then the new changes could be staged and commited with --no-verify?

[obastemur]

@Krovatkin sounds good. @dilijev @tcare opinions ?

[tcare]

The problem with hooks is that we can't force people to use them. We could add it to the internal scripts to set up the repo but people who have already cloned it will probably not bother. We'd also have to update internal automation to take these into account. It's desired but low pri.

CI checks were meant to be the solution to this. However I don't think most people thought it was important enough to police, apart from the ones that Doug already implemented.

As for the diff, most diff tools should be able to ignore whitespace as an option.

Doug may have other ideas.

[obastemur]

As for the diff, most diff tools should be able to ignore whitespace as an option.

I'm wondering if there is any good / viable reason to allow empty spaces? i.e. if we make a rule and block all the new whitespaces, would we endup limiting something has to be there?

Otherwise, what we have right now is IMHO troubling. We loose more time to understand what was really changed in a particular PR.

[Krovatkin]

@tcare

The problem with hooks is that we can't force people to use them.

I remember I was reading about a workaround for this exact problem. Some1 was suggesting to put all hooks in a hooks folder and then symlink it to .git/hooks

[Krovatkin]

@tcare Referencing the original stackoverflow question

Though, I'm not sure if this is a feasible solution on windows?

[tcare]

You still have to make people do the symlink though :( People that care enough to symlink aren't the ones to be breaking the rules usually.

I am only really for banning things at CI level if they're important enough. Trailing whitespace and tabs are important to me, but they weren't for everyone (read: above me :)) when it was originally put forward.

I'll let Doug reply because he is famous in the office for caring the most about whitespace ;) he definitely investigated some of these.

[Krovatkin]

You still have to make people do the symlink though :(

I might be mistaken here, but I thought git treated symlinks as regular files, so we could just include them as the part of the repo. Frankly, I never tried to share hooks as the part of the repo.

I'll let Doug reply because he is famous in the office for caring the most about whitespace ;) he definitely investigated some of these.

I have a script called doug to remove redundant newlines and trailing whitespaces :-)

[dilijev]

lol I've just started treating trailing whitespace as a quirk of our codebase. When we migrated to the open source repo we ran a script to clean up all the tabs and trailing whitespace and that was the best I could get everyone to agree to. Everyone agrees tabs are bad so I added that CI check when we started seeing tabs show up again (there's really not that many of them yet). Unfortunately, there's a ton of trailing whitespace that has crept in during the past year of being open source. We want to avoid adding a check would force people to clean it up when they really don't want to or when there's an "avoid code churn" reason not to.

I'm all in favor of people using their judgement and cleaning things up as they go but automated cops to nag people about it doesn't seem like the right answer.

For GitHub diff view, try adding ?w=1 to the end of the URL. That will ignore whitespace in the diff. Related, git diff -w and git blame -w ignore changes to whitespace. I use these a lot and it makes my life a lot better :)

[tcare]

I have a script called doug to remove redundant newlines and trailing whitespaces :-)

Ha! Achievement unlocked Doug :)

[obastemur]

For GitHub diff view, try adding ?w=1 to the end of the URL. That will ignore whitespace in the diff. Related, git diff -w and git blame -w ignore changes to whitespace. I use these a lot and it makes my life a lot better :)

If we are into finding solutions to my day to day problems, thanks for the reminders.

• Shall we expect this from anyone looking into our commit history as well ?
• Breaking cursor pos. especially with vim et al. IDE
• Even if we use the ?w=1 and someone introduce a trailing ws into one of our python files, we would have hard times to find that bug? because the whole diff is way too dirty OR we are unable to see that trailing WS with our filters?

So the general opinion is, we are kinda lazy to enable a basic IDE setting that keeps the repo from zillions of white spaces and possible bugs

Cool.

[dilijev]

@obastemur It's somewhat more difficult problem than laziness... Many of us are aware of the issue and would enable (or usually do enable) settings across all of our editors to trim trailing whitespace automatically when saving a file...

Unfortunately, VS does not actually have a setting to just trim trailing whitespace from code files (neither manually nor automatically on save). The format command (Ctrl+K, Ctrl+F) trims trailing whitespace but also does much more, much of which is unwanted when you're just trying to trim whitespace.

I updated the .vimrc I use on my work machine because I was tired of unintentionally introducing huge diffs to files where I made small changes. I think @Cellule has a similar problem with Sublime.

Trailing whitespace is not a problem in Python AFAIK. Only leading whitespace is significant. Banning tabs from the codebase largely solves any problems there.

[Krovatkin]

Unfortunately, VS does not actually have a setting to just trim trailing whitespace from code files (neither manually nor automatically on save). The format command (Ctrl+K, Ctrl+F) trims trailing whitespace but also does much more, much of which is unwanted when you're just trying to trim whitespace.

a combo, "git diff" (to get the line ranges w/ changes) + simple perl/sed script (to remove trailing whitespaces only for those line ranges), triggered by a pre-commit hook would solve this issue no matter which editor one is using 😉

[dilijev]

Good point for solving the problem for a given user, but again, getting everyone to use the pre-commit hook is the problem there.

(And that was basically the scripted approach I took to fixing up the trailing whitespace when we migrated the code to the open source repo.)

[tcare]

simple perl/sed script

Which is then introducing a dependency on Windows.. and how do you make people do that when they don't use the command line for their commits?

CI is the only way to 'enforce' this. But you still have to convince the team that it's worth cleaning up and enforcing.

[Krovatkin]

@tcare

and how do you make people do that when they don't use the command line for their commits?

Lol, this is so true! I just didn't realize that there was this brave new world of GUI tools out there :-)

Which is then introducing a dependency on Windows...

Isn't there an alternative on Windows? Powershell, maybe?

[dilijev]

Basically, there are a lot of problems to solve (and effort to put in) for what many regard as a non-issue.

That said, I'm not necessarily saying we shouldn't fight the good fight.

[tcare]

@Krovatkin

Isn't there an alternative on Windows? Powershell, maybe?

Sure, but you have to have a solution that is cross platform and doesn't introduce independencies unless it's critical. Powershell is ok on Windows but would introduce a dependency on *nix. I would argue whatever is bundled with Git is OK.

[dilijev]

whatever is bundled with Git is OK.

So use a bash script and depend only on whatever git commands and bash utilities are included in the Windows version of Git? Seems reasonable.

[obastemur]

Unfortunately, VS does not actually have a setting to just trim trailing whitespace from code files (neither manually nor automatically on save).

@dilijev This sounds like a good open source hobby project idea! Quickly searched for a plugin doing that and couldn't find something fits for exactly what we are looking for.

It will be fun! Don't know how much free :) time it will take but here is the repository for the project -> https://github.com/obastemur/VSWhiteSpaceFree

[dilijev]

Not sure how productive it is to work on a new VS plug-in. There are already plug-ins that support what we're asking for here -- though usually they package a lot of other features as well (here's one I found: http://www.codemaid.net/). The problem is getting everyone to adopt a single tool when that feature is not provided out of the box with VS.

[kphillisjr]

I believe it might be worth noting that Artistic Style(astyle) has been around for a few years and works on Linux, OSX, and windows. This with a little bit of work can be integrated into both the Visual Studio project files and the linux/osx build script.

[tcare]

We'd like to avoid adding another dependency to the project, since we need to build on stock Visual Studio/msbuild. Internally this is also a problem for us where we cannot even have msbuild plugins.

CI is the only universal gate we can have that would enforce this kind of requirement.

[kphillisjr]

That is understandable, and astyle was the first thing I thought of. I also should mention that clang supports automatic code formatting options (Clang-Format Style Options), and the only issue on this case would be tailoring it into jenkins (CI).

[obastemur]

Clang auto format something I have used and don't advice. Not because it doesn't work well. Because we wouldn't setup a configuration that is exactly matching what we have now. We would have even more issues there.

I wasn't into bringing more dependency though :) If there are enough Whitespace plugins for VS, that is perfect! Actually, for a second I had a dream that my WhiteSpace plugin is becoming the most popular VS plugin... Good old days.

As suggested, what if we put a script on CI that gives an error (similar to tabs) if there is an extra white space with the commit?

[dilijev]

@obastemur We could add that but considering how much trailing whitespace there already is, we need to ensure we only check what was JUST introduced in that pull request, and ignore everything else.

I did something similar with the tab check, since there are few enough tabs in the code, just show the git blame for any tabs in any files which have been changed (that also turns out to be somewhat simpler than checking just the diff over the pull request).

[obastemur]

Thanks for the conversation! Recently we don't have this issue (often) and not much to do though. Closing.