The situation with witespace in our CVS repo is not good, see the numbers - it is only our code, imported libraries are not counted. There are more than 10 000 tbas and 13 000 instances of trailing whitespace.
The plan is to fix them all in HEAD with two big commits tomorrow evening GMT.
I know that it will cause problems for people maintaining their own repos and merging from older branches, but it will help a lot when merging from 1.7 upwards.
This step was already approved by MD. If there are any major objections, please speak now.
skodak
Can we put a filter on incomving CVS commits, so that anyone trying to commit a tab or trailing whitespace gets told to try again?
It is an easy mistake to make without relaising it, and very easy to correct if it is brought to your attention.
It is an easy mistake to make without relaising it, and very easy to correct if it is brought to your attention.
Good idea. I'll add it if someone sends me reliable code to do this!
To be clearer, adding the pre-commit filter to the CVS repository is not hard, but we just need a nice stable well-tested perl or sh script that can check for tabs and trailing whitepace.
This version (stolen from cvs commit messages found in the ffmpeg project) could be a starting point. Our final version has to be really well tested before it goes in obviously, because all commits would break.
#!/bin/sh
FILELIST="`echo $ | sed s/Makefile//`"
for file in $FILELIST; do
if grep -e ' ' -e ' $' $file > /dev/null; then
echo
echo "Tabs or trailing whitespace found in $file."
echo "Commit aborted, fix the issue and try again."
echo
exit 1
fi
if [ `tail -c1 $file | wc -l` = 0 ]; then
echo
echo "$file does not end in a newline."
echo "Commit aborted, fix the issue and try again."
echo
exit 1
fi
done
exit 0
This version (stolen from cvs commit messages found in the ffmpeg project) could be a starting point. Our final version has to be really well tested before it goes in obviously, because all commits would break.
#!/bin/sh
FILELIST="`echo $ | sed s/Makefile//`"
for file in $FILELIST; do
if grep -e ' ' -e ' $' $file > /dev/null; then
echo
echo "Tabs or trailing whitespace found in $file."
echo "Commit aborted, fix the issue and try again."
echo
exit 1
fi
if [ `tail -c1 $file | wc -l` = 0 ]; then
echo
echo "$file does not end in a newline."
echo "Commit aborted, fix the issue and try again."
echo
exit 1
fi
done
exit 0
I think that if you waited until after 1.7 was released, then it would not be too big a risk if you enabled a filter like this one morning, and monitored the situation for a day to see whether it worked.
We've had to cope with worse than one day without CVS access. Just don't experiment when we are trying to get 1.7 done
We've had to cope with worse than one day without CVS access. Just don't experiment when we are trying to get 1.7 done
What I was most worried about was not being able to remove the file afterwards and having to wait for sourceforge to get around to removing it. But don't worry it's not happening that soon!
Don't tell me they make it easy to add filters, but not to remove them!
Well, to change the commitinfo file in CVSROOT you need to do a commit ...
Ah, I see.
This filter assumes everything you commit is a text file. I wonder if this is true for all of the CVS content (I just don't really know).
Saludos. Iñaki.
Saludos. Iñaki.
Hi Petr, could you include the fixing of invalid line endings in this cleanup?
Hi Gustav, I will try to fix the line endings too. Hmm, I knew how to locate them in windows, but will have to find a new way to do it in *nix. If you know any broken file I can start playing with, please tell me
thanks
thanks
pcregrep or just plain perl are good for this. Any case of \r is bad. IIRC, Windows is \n\r and Macs are \r, where Unix is \n.
Having said that, I don't know if I like massive cleanups
It will complicate even merging from MOODLE_16_STABLE to HEAD. (Most cases, fixable with
cvs diff -r MOODLE_16_MERGED -r MOODLE_16_STABLE path/to/file.php | patch -p -l
But it's still a pain. OTOH, I am not opposed to a commit hook script that checks for added invalid whitespace and rejects it
Having said that, I don't know if I like massive cleanups
cvs diff -r MOODLE_16_MERGED -r MOODLE_16_STABLE path/to/file.php | patch -p -l
But it's still a pain. OTOH, I am not opposed to a commit hook script that checks for added invalid whitespace and rejects it
Just a thought:
Would it not suffice to just enable the pre-commit filter and then as files are updated and committed to cvs going forward, the badness will eventually clean itself up, and us CVSers won't have to pull our hair out? We'll be getting the whitespace fix along with some other change. It should be pretty transparent that way.
brian.
Would it not suffice to just enable the pre-commit filter and then as files are updated and committed to cvs going forward, the badness will eventually clean itself up, and us CVSers won't have to pull our hair out? We'll be getting the whitespace fix along with some other change. It should be pretty transparent that way.
brian.
The current plan for tonight in HEAD is to:
The imported 3rd party libraries will not be touched now.
The handling of traling whitespace will be decided later because it would break merging too much for little benefit
- fix all tabs
- fix all incorrect line-endings
- fix trailing whitespace inside files new in 1.7 (those not present in 1.6)
The imported 3rd party libraries will not be touched now.
The handling of traling whitespace will be decided later because it would break merging too much for little benefit