Whitespace patchbombs

[Erik Bray]

How do we feel about large patches full of whitespace cleanup? Lots
of Sage modules have stray whitespace, and my editor usually
automatically removes it when I open files (this is a personal
preference that I have to live with though).

Usually when preparing patches this means manually removing such
distracting whitespace cleanup, though all that means is then removing
it again, and again, and again... (or sometimes I will just leave it
in a commit if it's just one or two lines).

It might be nice to just clean up a whole lot of this at once, but I
think that would require some coordination so as to not create too
many trivial merge conflicts...

Thanks,
E

[Simon King]

Why not doing such cleanup right before releasing a beta version?

[Daniel Krenn]

On 2018-02-20 12:07, Simon King wrote:
> Why not doing such cleanup right before releasing a beta version?

Or even better, right before a release, e.g. doing it for a rc0 or rc1.

[Erik Bray]

That was my thinking as well--I'm sure Volker and I could coordinate
something like that.

However, it could still leave plenty of open tickets with otherwise
good (but as yet unmerged) branches with trivial merge conflicts :/

[Vincent Delecroix]

Indeed, this is the problem. Further patches would not apply for
trivial reason. To my mind the best would be to get rid of trailing
whitespaces once and forever (ie not accepting branches with trailing
whitespaces).

Vincent

[John Cremona]

Would  this help?  https://stackoverflow.com/questions/9776527/merging-without-whitespace-conflicts

In brief:  git merge -Xignore-space-change

There is also a reference there to a commit hook which removes trailing whitespace.

--
You received this message because you are subscribed to the Google Groups "sage-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscribe@googlegroups.com.
To post to this group, send email to sage-...@googlegroups.com.
Visit this group at https://groups.google.com/group/sage-devel.
For more options, visit https://groups.google.com/d/optout.

[Erik Bray]

On Tue, Feb 20, 2018 at 1:44 PM, John Cremona <john.c...@gmail.com> wrote:
> Would this help?
> https://stackoverflow.com/questions/9776527/merging-without-whitespace-conflicts
>
> In brief: git merge -Xignore-space-change
>
> There is also a reference there to a commit hook which removes trailing
> whitespace.

+1 For the release manager who actually does the merges this is
probably useful. Though we would also have to add it to the Sage Trac
plugin and patchbot so that they don't show branches as unmergeable
(and at least for some branches we do want to see whitespace changes,
so I'm not exactly sure how best to manage that)

> On 20 February 2018 at 12:23, Vincent Delecroix <20100.d...@gmail.com>
> wrote:
>>
>> On 20/02/2018 12:45, Erik Bray wrote:
>>>
>>> On Tue, Feb 20, 2018 at 12:22 PM, Daniel Krenn <kr...@aon.at> wrote:
>>>>
>>>> On 2018-02-20 12:07, Simon King wrote:
>>>>>
>>>>> Why not doing such cleanup right before releasing a beta version?
>>>>
>>>>
>>>> Or even better, right before a release, e.g. doing it for a rc0 or rc1.
>>>
>>>
>>> That was my thinking as well--I'm sure Volker and I could coordinate
>>> something like that.
>>>
>>> However, it could still leave plenty of open tickets with otherwise
>>> good (but as yet unmerged) branches with trivial merge conflicts :/
>>
>>
>> Indeed, this is the problem. Further patches would not apply for
>> trivial reason. To my mind the best would be to get rid of trailing
>> whitespaces once and forever (ie not accepting branches with trailing
>> whitespaces).
>>
>> Vincent
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "sage-devel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an

>> email to sage-devel+...@googlegroups.com.

>> To post to this group, send email to sage-...@googlegroups.com.
>> Visit this group at https://groups.google.com/group/sage-devel.
>> For more options, visit https://groups.google.com/d/optout.
>
>

> --
> You received this message because you are subscribed to the Google Groups
> "sage-devel" group.
> To unsubscribe from this group and stop receiving emails from it, send an

> email to sage-devel+...@googlegroups.com.

[Travis Scrimshaw]

What about more of a middle ground where any file you touch and your editor does whitespace cleanup, then you can merge it into that ticket. That way it puts conflict in files that would possibly have conflicts anyways, but it does not make your workflow harder. Although in a broad sense, I do not oppose a big cleanup, but I do slightly worry about it creating too many undue trivial merge conflicts with branches that otherwise would work, pre-(beta-)release or otherwise.

Best,
Travis

 

[Erik Bray]

I'd be completely fine with that so long as no one complains about
otherwise irrelevant whitespace cleanup coming along for the ride in
my tickets. I'll point out also that the cgit diff viewer allows
setting whitespace=ignore when viewing branch diffs.

[Jeroen Demeyer]

First of all, we should ensure that no new trailing whitespace is added.
Otherwise, your effort is futile. The recently added file
src/sage/rings/valuation/valuation_space.py is a bad example.

I recall that we had a discussion about autogenerated patchbombs not so
long ago. I think it's a bad idea because it can potentially generate a
lot of silly merge conflicts. Why not remove it as part of the usual
workflow, for example when refactoring something anyway? I generally
don't mind if a ticket makes a bunch of whitespace changes (as long as
it's not dominating the diff).

By the way, the situation is not too bad: only 19 files have 20 or more
lines with trailing whitespace. One file that stands out is
functions/piecewise_old.py with 244 lines.

[Jeroen Demeyer]

On 2018-02-20 14:16, Erik Bray wrote:
> I'd be completely fine with that so long as no one complains about
> otherwise irrelevant whitespace cleanup coming along for the ride in
> my tickets.

To clarify, I'm fine with this too if it's not too much in one branch
(this is of course very subjective).

[Jeroen Demeyer]

Interesting fact: the number of lines with trailing whitespace is
generally *increasing* with every Sage release. So it seems to me that
the biggest problem (if you find whitespace a problem) is preventing new
whitespace.

[R. Andrew Ohana]

If I recall, when I originally setup the git-trac server, I included a hook that would reject/warn any introduction of new trailing whitespace. I believe this was a compromise made after a long discussion at a Sage days -- there were a lot of concerns at the time about the effort required to rebase mercurial patches with conflicts arising from a whitespace patchbomb. I also believe that we included in the documentation and server side hook a suggestion to use the trailing whitespace stripping commit hook (and there was a sage-dev command that would automatically do that, although sage-dev has now been removed in favor of vanilla git or Volker's git trac).

On Tue, Feb 20, 2018 at 12:47 PM, Jeroen Demeyer <J.De...@ugent.be> wrote:

Interesting fact: the number of lines with trailing whitespace is generally *increasing* with every Sage release. So it seems to me that the biggest problem (if you find whitespace a problem) is preventing new whitespace.

--
You received this message because you are subscribed to the Google Groups "sage-devel" group.

To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscribe@googlegroups.com.

To post to this group, send email to sage-...@googlegroups.com.
Visit this group at https://groups.google.com/group/sage-devel.
For more options, visit https://groups.google.com/d/optout.

--

Andrew

[Volker Braun]

IMHO the evils of trailing whitespace are greatly exagerrated.

The eaisest solution is to just fix your editor to not introduce changes that you did not make yourself.

If you think fighting the windmills of trailing whitespace is a worthwhile use of your time, be my guest. But I want a workflow where I don't have to bother with trailing whitespace, so you better have git hooks to auto-fix everything on commit etc.

[Jori Mäntysalo]

How are merge conflicts handled, and is there any use for priority-flag on
trac? It would make sense that lower priority tickets would be merged
after more important ones.

--
Jori Mäntysalo

[Erik Bray]

On Wed, Feb 21, 2018 at 1:25 AM, Volker Braun <vbrau...@gmail.com> wrote:
> IMHO the evils of trailing whitespace are greatly exagerrated.
>
> The eaisest solution is to just fix your editor to not introduce changes
> that you did not make yourself.
>
> If you think fighting the windmills of trailing whitespace is a worthwhile
> use of your time, be my guest. But I want a workflow where I don't have to
> bother with trailing whitespace, so you better have git hooks to auto-fix
> everything on commit etc.

I feel like I've worked on some projects before that implemented some
pretty strict client-side git commit hooks, where I couldn't even
commit a change unless it passed some linters (it was possible, and
sometimes even desirable, to get around this with `git commit -n` to
disable hooks).

I've never set up something like that before though. I remember
several years ago you could include some pre-commit hooks in your
repository, but the user still had to *manually* enable them which
almost defeats the purpose (because many people won't bother). But I
think I've committed to some projects before where there were
pre-commit hooks that I never did anything to enable...

> On Tuesday, February 20, 2018 at 12:04:26 PM UTC+1, Erik Bray wrote:
>>
>> How do we feel about large patches full of whitespace cleanup? Lots
>> of Sage modules have stray whitespace, and my editor usually
>> automatically removes it when I open files (this is a personal
>> preference that I have to live with though).
>>
>> Usually when preparing patches this means manually removing such
>> distracting whitespace cleanup, though all that means is then removing
>> it again, and again, and again... (or sometimes I will just leave it
>> in a commit if it's just one or two lines).
>>
>> It might be nice to just clean up a whole lot of this at once, but I
>> think that would require some coordination so as to not create too
>> many trivial merge conflicts...
>>
>> Thanks,
>> E
>

> --
> You received this message because you are subscribed to the Google Groups
> "sage-devel" group.
> To unsubscribe from this group and stop receiving emails from it, send an

> email to sage-devel+...@googlegroups.com.

[Erik Bray]

On Wed, Feb 21, 2018 at 12:01 PM, Erik Bray <erik....@gmail.com> wrote:
> On Wed, Feb 21, 2018 at 1:25 AM, Volker Braun <vbrau...@gmail.com> wrote:
>> IMHO the evils of trailing whitespace are greatly exagerrated.
>>
>> The eaisest solution is to just fix your editor to not introduce changes
>> that you did not make yourself.
>>
>> If you think fighting the windmills of trailing whitespace is a worthwhile
>> use of your time, be my guest. But I want a workflow where I don't have to
>> bother with trailing whitespace, so you better have git hooks to auto-fix
>> everything on commit etc.
>
> I feel like I've worked on some projects before that implemented some
> pretty strict client-side git commit hooks, where I couldn't even
> commit a change unless it passed some linters (it was possible, and
> sometimes even desirable, to get around this with `git commit -n` to
> disable hooks).
>
> I've never set up something like that before though. I remember
> several years ago you could include some pre-commit hooks in your
> repository, but the user still had to *manually* enable them which
> almost defeats the purpose (because many people won't bother). But I
> think I've committed to some projects before where there were
> pre-commit hooks that I never did anything to enable...

Oh, I remember what project I saw this in. It was WolfSSL. Turns out
it's nothing fancy though--it just symlinks its pre-commit script to
.git/hooks in the autogen.sh script. So technically you could get
away with making a commit that doesn't get verified, but if you're
doing any real development it stands to reason you would run
autogen.sh first thing. I suppose Sage could do something similar...

[John Cremona]

A simpler, similar example is with the lmfdb (https://github.com/LMFDB/lmfdb) where no pull request is merged unless pyflakes gives no errors or warnings.  I am not suggesting that Sage does the same, but there were rather a lot of bugs discovered when we first started using pyflakes this way!

John

>> email to sage-devel+unsubscribe@googlegroups.com.

>> To post to this group, send email to sage-...@googlegroups.com.
>> Visit this group at https://groups.google.com/group/sage-devel.
>> For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "sage-devel" group.

To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscribe@googlegroups.com.

[Daniel Krenn]

So, there should be a trac plugin which checks if there are whitespace
issues in the changed code lines or in the changed files...

And, as it was mentioned in the other thread, there could be another
trac plugin which runs pyflakes on the changed files.

[Volker Braun]

I'm against a workflow that involves bots for a nothing-burger like trailing whitespace. Either auto-cleanup on commit and have the server check on push that it was cleaned up. Or nothing at all. But waiting a day and then have a bot come back with trailing whitespace nonsense is just an unnecessary drag on development.

[Volker Braun]

+1 for lint; adding a suitable set of linting rules to the testsuite would have a much greater impact on code quality than enforcing a trailing whitespace policy will ever have.