Trailing Whitespace

Trailing Whitespace

[Ben Longbons]

As I've tried, .

I have my editor set to trim all trailing whitespace, because trailing
whitespace is never desirable.

However, since other people have in times past committed code that
*does* contain trailing whitespace, this creates noise when I'm trying
to create a patch.

Therefore, I suggest that someone create a commit that removes all the
trailing whitespace. The pressing need is to apply this to gdb/ but
there is no reason it couldn't be applied to other parts if need be.

A commit that does nothing besides change trailing whitespace is *not*
harmful to the history, because git-blame and other tools can all
ignore whitespace.

Therefore, the only consideration is whether this will cause problems
for people with changes that are not yet committed in the main repo.
The script at the end of this message demonstrates how to avoid
problems with all 4 of the local branch strategies that I know about.

But first, the script to clean up after it:

#!/bin/bash -eu
# clean.sh
# Make the directory effectively empty
read -p 'Press enter to rm a bunch of directories, ^C to cancel'
rm -rf remote
rm -rf init
rm -rf whitespace
for repo in retv args
do
    rm -rf $repo
    for strategy in merge rebase git_patch dumb_patch
    do
        rm -rf $repo-$strategy
    done
done



#!/bin/bash -eu
# dirty.sh
# Run from an effectively empty directory
! test -f remote
! test -f init
! test -f whitespace
for repo in retv args
do
    ! test -f $repo
    for strategy in merge rebase git_patch dumb_patch
    do
        ! test -f $repo-$strategy
    done
done

# first set up the repo and contents
git init --bare remote
git clone remote init
cd init
echo 'int main() {  ' > file.c
echo '    return 0;' >> file.c
echo '}'             >> file.c
git add file.c
git commit -m 'init'
git push origin master
cd ..

# Two different cases representing people's non-committed changes.
# one is diff on the same line and one is on a nearby line
git clone remote retv
cd retv
sed 's/0/1/' -i file.c
git add -u
git commit -m retv
git branch retv # for backup
cd ..

git clone remote args
cd args
sed 's/()/(int argc, char **argv)/' -i file.c
git commit -a -m args
git branch args # for backup
cd ..

# Then make the whitespace change
git clone remote whitespace
cd whitespace
sed 's/[[:space:]]\+$//' -i file.c
git commit -a -m 'whitespace'
git push
cd ..

strategy_merge() {
    # cannot be used until we ditch changelogs
    # preferred strategy for many projects
    git pull -X ignore-space-at-eol --no-edit
}

strategy_rebase() {
    # best approach given changelogs
    # preferred strategy for many projects
    git pull --rebase -X ignore-space-at-eol
}

strategy_git_patch() {
    # historical approach from when patches had to be mailed around
    # still works fairly well
    git format-patch HEAD^
    git fetch
    git reset --hard origin/master
    git am --ignore-whitespace 0001-$repo.patch
}

strategy_dumb_patch() {
    # not recommended - no advantages, several disadvantages over git_patch
    git show > dumb.diff
    git fetch
    git reset --hard origin/master
    patch --ignore-whitespace -p1 < dumb.diff
    git add -u
    git commit -m $repo
}

for repo in retv args
do
    for strategy in merge rebase git_patch dumb_patch
    do
        echo in $repo, using $strategy
        cp -a $repo $repo-$strategy
        cd $repo-$strategy
        strategy_$strategy
        cd ..
    done
done
echo Done

Re: Trailing Whitespace

[Gary Benson]

Ben Longbons wrote:
> I have my editor set to trim all trailing whitespace, because
> trailing whitespace is never desirable.
> 
> However, since other people have in times past committed code that
> *does* contain trailing whitespace, this creates noise when I'm
> trying to create a patch.
> 
> Therefore, I suggest that someone create a commit that removes all
> the trailing whitespace. The pressing need is to apply this to gdb/
> but there is no reason it couldn't be applied to other parts if need
> be.
> 
> A commit that does nothing besides change trailing whitespace is
> *not* harmful to the history, because git-blame and other tools can
> all ignore whitespace.
> 
> Therefore, the only consideration is whether this will cause
> problems for people with changes that are not yet committed in the
> main repo.  The script at the end of this message demonstrates how
> to avoid problems with all 4 of the local branch strategies that I
> know about.

I see nobody replied to this, but it would be pretty nice to have the
repo cleaned in this way.

Does anybody have any reason not to do this?

Thanks,
Gary

-- 
http://gbenson.net/

Re: Trailing Whitespace

[Phil Muldoon]

On Thu, 09 Jan 2014, Gary Benson wrote:

> Ben Longbons wrote:
> > Therefore, I suggest that someone create a commit that removes all
> > the trailing whitespace. The pressing need is to apply this to gdb/
> > but there is no reason it couldn't be applied to other parts if need
> > be.
 
> I see nobody replied to this, but it would be pretty nice to have the
> repo cleaned in this way.
> 
> Does anybody have any reason not to do this?

Whitespace cleanups inevitably bit-rot unless the maintainer checks
each and every patch for whitespace.  This is pretty easy to do in
emacs, but I am not so sure about other editors.  It's another weight
on the already burdened shoulders of maintainers.

The simple way to make sure that your patch does not have useless
whitespace included is to run:

git diff --check

That will flag all the whitespace.

Also (and I am not sure if git can get around this somehow) whitespace
cleanups tend to obliterate diffs/patches that were written before the
cleanup took place.  This makes merging a massive pain.

So my 2 pence worth is, no, not a good idea ;)

Cheers,

Phil

Re: Trailing Whitespace

[Joel Brobecker]

> Also (and I am not sure if git can get around this somehow) whitespace
> cleanups tend to obliterate diffs/patches that were written before the
> cleanup took place.  This makes merging a massive pain.
> 
> So my 2 pence worth is, no, not a good idea ;)

Strongly seconded. Localized whitespace fixes are OK, because manageable
in the amount of work they generate, but please, not a massive cleanup.
And if we ever do that massive cleanup, I would request that a
pre-requisite is an "update" check on the git server that rejects pushes
of files violating that rule.

On my end of things, I configured my editor to simply highlight
trailing spaces, not strip them.

-- 
Joel

Re: Trailing Whitespace

[Dmitry Samersoff]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

IMHO, better approach is to install on-commit hook blocking commits
with trailing whitespace (and tab characters) and leave old code as is.


On 2014-01-09 19:01, Joel Brobecker wrote:
>> Also (and I am not sure if git can get around this somehow)
>> whitespace cleanups tend to obliterate diffs/patches that were
>> written before the cleanup took place.  This makes merging a
>> massive pain.
>> 
>> So my 2 pence worth is, no, not a good idea ;)
> 
> Strongly seconded. Localized whitespace fixes are OK, because
> manageable in the amount of work they generate, but please, not a
> massive cleanup. And if we ever do that massive cleanup, I would
> request that a pre-requisite is an "update" check on the git server
> that rejects pushes of files violating that rule.
> 
> On my end of things, I configured my editor to simply highlight 
> trailing spaces, not strip them.
> 


- -- 
Dmitry Samersoff
Saint Petersburg, Russia, http://devnull.samersoff.net
* There will come soft rains  ...
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzu57AAoJEHEy08c4gIABafIH/RMOU8+mzZts8yRFUKHKB1D/
89sG/dfLWTShhJoK4omD9h4BzntQgzBviyhEsQuaAgYATg+3Ok503wd+WhEEkVub
IqqXoz4Pu/2GSu2sWzNT9o+sXudUH/gyGsG+/ISVsBr1yF7WMSUMl8TLeOTJMUi6
hY/7lrBfNVFxEnKXS94sdf98dXCz1oGWQXDt9zv26zeayhKjMx41A11IQTZN5Lrd
yMdyd4O4/r+9T6otii1utnDTQwYfRGiOb+UFoE7bD+blEL5pqSDLHsYcXpmnvhXE
sAc3Koa33KimE6krJsFoKSZPDxXbefb/48e959VqF6/1Nh1/qxWmY6lonqHA0Rg=
=2WWt
-----END PGP SIGNATURE-----

Re: Trailing Whitespace

[Samuel Bronson]

Dmitry Samersoff <dms <at> samersoff.net> writes:

> IMHO, better approach is to install on-commit hook blocking commits
> with trailing whitespace (and tab characters) and leave old code as is.

Unfortunately, commit hooks have to be set up in every local copy of the
repository, which I can't see happening.

On the other hand, there *is* a sample "pre-commit" hook to catch trailing
whitespace installed by default in every new git repository, so it would be
as simple as running "mv .git/hooks/pre-commit{.sample,}".

Re: Trailing Whitespace

[Joel Brobecker]

> > IMHO, better approach is to install on-commit hook blocking commits
> > with trailing whitespace (and tab characters) and leave old code as is.
> 
> Unfortunately, commit hooks have to be set up in every local copy of the
> repository, which I can't see happening.

The best way to do this is to do it on the official repository
(on sourceware) - via the "update" hook.

We can also provide client-side hooks for the "commit", but, as you say,
it would rely on everyone of us activating them.

-- 
Joel