test_source harmful?

[Marius Kruger]

2009/2/27 Robert Collins <robert.collins at canonical.com>:
> On Wed, 2009-02-25 at 11:37 +0200, Marius Kruger wrote:
>> 2009/2/25 Robert Collins <robert.collins at canonical.com>:
>> >> I thought that was the point: to put the pain of fixing whitespace
>> >> mistakes squarely where they belong, at the point of the person
>> >> attempting to introduce them.
>> yes, don't allow new ones to be added.
>
> Or we could just not worry.

then that needs to be the policy: diffs adding/removing trailing in
code you touch is welcome. i.e. reviewer should not even mention
the trailing whitespace since it is a non issue.

>> > I don't think frustrating people was the point; honestly we 'never' (or
>> > extremely rarely) had merge conflicts due to whitespace fixes being
>> > done. Creating pain to solve a non-problem is hard for me to understand.
>> sorry.
>
> Sorry doesn't remove the friction. Removing the test will remove it.

well I thought is might upset people, so I wasn't pushing it too much.
Originally I didn't have an easy way to manage the new trailing
whitespace I add,
other than setting my editor to just remove everything (as it is for
other projects I work on).
This didn't work for the bzr codebase as it turns out it was riddled with it.
I pursued the idea because of:
2006/12/19 Aaron Bentley <aaron.bentley at utoronto.ca>:
> Marius Kruger wrote:
>> thanks, for the vote and the merge.
>> sorry about the whitespace (my editor removes it automatically),
>> I would have reverted that, but I remembered something on the mailinglist
>> about removing all the trailing whitespace, so I removed them.
>> I tried to see if you fixed this, but it looks like you forgot.
>
> Well, I did miss some, but I fixed a bunch of them, like the ones in
> commands.py, where you changed lines from "   " to "".
>
>> 1.) Can we add something to the branch to specify that for certain or
>> all files we should
>>   a.) automatially remove trailing whitespace or
>>   b.) refuse to commit it.
>
> Usually we add test cases for issues like this.  That would suit me fine.
>
>> 2.) Annotate should be able to cope with this. mabye add an option to
>> ignore whitespace changes.
>
> No, not without being orders of magnitude slower.  Annotate relies on
> the fact that knits are a delta-based format whose snapshots are annotated.
>
> For storage purposes, knit deltas must be whitespace-sensitive.

After writing the test stuff, I wrote that plugin to implement 1a,b . The only
missing thing is to be able to configure rules in branches so that we can force
whoever hacks on it to not add new trailing ws. This is not so much
of a priority to me since I now have a way to not worry about this because
I have a tool that does it for me.

to be clear, I actually don't care about trailing white space and I don't
want to waste time caring about if I added any.
But if you accidentally make unrelated/invisible whitespace changes,
this may upset people looking at the diffs.
So I would rather let programs/plugins/editors etc take care of it for me.

>> > I don't think we can expect other projects to use a source code check
>> > like we have done by default. If whitespace conflicts are such an issue
>> > we should make our merge handle whitespace conflicts better - because
>> > thats what our users will need.
>>
>> YES PLEASE!
>> Our policy at work is to not have trailing whitespace because it is
>> too difficult to
>> see what really happend or merge when somebody went crazy with
>> whitespace (lots of accidental changes; most programmers don't check
>> their diffs before committing)
>
> Sound like you agree: we should not have the whitespace check in
> test_source, instead we should deal with any small issues that happen -
> and file bugs to provide feedback on where bzr needs to improve.

actually I think we need both.
But if the test is too painful now, we should remove it until bzr can
deal with this properly.

>> thats why I made the bzr-text-checker plugin to warn you about it at
>> commit time, or prevent you from committing it in the first place.
>> https://launchpad.net/bzr-text-checker
>> Content filtering should be able to remove it automatically for you in
>> the future.
>
> This ignores the issue: Trailing whitespace is not structurally bad; it
> doesn't cause bugs; it might _rarely_ cause merge conflicts. Checking
> that something unimportant is set to a particular value is still
> checking something unimportant.

then that should be the policy.

> I don't want to be trained to jump though an unnecessary hoop: I want to
> remove the hoop.

:) the bzr reviewers already trained me, and its not that bad.
(before I send, I do a quick `bzr cdiff --check-style --old
../bzr.dev` which I aliased)

> The pain isn't helping sort out the problem; its avoiding sorting out
> the problem (which is a merging/tracking issue).

I think the test forces us to deal with it sooner rather than later.


but ok, I get the point. remove it. I would even send the patch in but I
won't have time this weekend (I'm going hiking).
Things to note though:
* maybe you don't want to remove the whole test, just the trailing
whitespace part.
  because we used to check for the existence of tabs,
  and we have always complied to some of the other stuff eg. a newline
at the end of files, and the line-endings.
* update the relevant sections in hacking.

regards
marius