Code style guidelines

[Keshav Kini]

Hi all,

This is pretty trivial, but hey, sage-devel is a bit slow recently :)

Shouldn't we have mention trailing whitespace somewhere in the developer's guide - namely telling people to omit it? Having such a policy makes for cleaner revision control, as most agree (for example, at some point git by default didn't let you commit changes which include adding trailing whitespace to files). There are way too many patches on trac with chunks that just remove trailing whitespace introduced in other patches, IMHO. If we take some stance on it (much like our official endorsement of PEP 8), this will be reduced. If people agree I'll make a ticket to add a couple lines to the developer's guide.

-Keshav

----
Join us in #sagemath on irc.freenode.net !

[Jeroen Demeyer]

On 2011-10-24 16:54, Keshav Kini wrote:
> If we take some stance on it, this will be reduced.
[citation needed]

It's obvious that it's better not to have trailing whitespace in files.
I'm not against adding a section in the developer's manual for this but
I doubt it will make any difference in practice. If people use editors
which are prone to trailing whitespace, that is not going to change.

[Keshav Kini]

The main point is that it would add legitimacy to people insisting that patches not contain trailing whitespace before giving them positive review on trac :)

[Keshav Kini]

By the way, I think trailing whitespace is often intentional. Some people consider this a good thing:

    Some text$
    $
    Some more text$

Some people also consider this a good thing:

-------------$
    TITLE    $
-------------$

Hell, I used to think these were good things until someone knocked some sense into my head :P And this kind of trailing whitespace (i.e. whitespace for making $ line up with things) accounts for a good portion of the trailing whitespace I see in Sage's codebase. Of 54837 instances of trailing whitespace in files in $SAGE_ROOT/devel/sage/sage , 42576 occurred on otherwise empty lines, which I assume mostly falls under the first example given above.

[Jeroen Demeyer]

On 2011-10-24 17:19, Keshav Kini wrote:
> By the way, I think trailing whitespace is often intentional. Some
> people consider this a good thing:
>
> Some text$
> $
> Some more text$

Personally, I do NOT consider this a problem. I would like to remove
whitespace after non-empty lines, but I don't really care much about
whitespace-only lines.

[Jason Grout]

Here is one of the big situations when I like trailing spaces:

def hello():$
print 'hi'$
$
print 'bye'$

To me, it's annoying to have the cursor jump back to column 0 when
scrolling down through the function. I think that is the only situation
where I get annoyed with my emacs settings of deleting trailing whitespace.

Thanks,

Jason

[John Cremona]

> To me, it's annoying to have the cursor jump back to column 0 when scrolling
> down through the function.  I think that is the only situation where I get
> annoyed with my emacs settings of deleting trailing whitespace.
>
> Thanks,
>
> Jason
>

If you tell me how to make emacs do that, I'll stop being a repeat
offender (just as I no longer -- I think -- submit patches with tabs
in!)

John

[Jason Grout]

Here is what is in my .emacs:

;; trailing whitespace
(setq-default show-trailing-whitespace t)
;(add-hook 'before-save-hook 'delete-trailing-whitespace)

The first line shows trailing whitespace in a big bold red font. The
second automatically deletes it. I commented out the second line because:

1. It was adding too much noise to existing patches (deleting all
trailing whitespace in a file)

2. the annoying issue with an all-whitespace line, pointed out in my
message above.

Thanks,

jason

[William Stein]

Putting the above (emacs code + discussion) in the Developer's Guide
would be very useful.

William

> Thanks,
>
> jason
>
>
> --
> To post to this group, send an email to sage-...@googlegroups.com
> To unsubscribe from this group, send an email to
> sage-devel+...@googlegroups.com
> For more options, visit this group at
> http://groups.google.com/group/sage-devel
> URL: http://www.sagemath.org
>

--
William Stein
Professor of Mathematics
University of Washington
http://wstein.org

[leif]

On 24 Okt., 17:33, Jason Grout <jason-s...@creativetrax.com> wrote:
> Here is one of the big situations when I like trailing spaces:
>
> def hello():$
>      print 'hi'$
>      $
>      print 'bye'$
>
> To me, it's annoying to have the cursor jump back to column 0 when
> scrolling down through the function.  I think that is the only situation
> where I get annoyed with my emacs settings of deleting trailing whitespace.

Well, then make emacs(?) not do that. ;)


-leif

P.S.: It's IMHO a bad idea anyway to make editors
"unintentionally" (or unconditionally) change the formatting of every
file you touch (or edit), at least if they're subject to collaboration
with others. Either you *intentionally* change the whole file
(knowing the consequences), or you should just make changes in those
parts you touch anyway. (Patch bombs and superfluous merge conflicts
FTW!)

[Jason Grout]

On 10/24/11 12:07 PM, leif wrote:
> On 24 Okt., 17:33, Jason Grout<jason-s...@creativetrax.com> wrote:
>> Here is one of the big situations when I like trailing spaces:
>>
>> def hello():$
>> print 'hi'$
>> $
>> print 'bye'$
>>
>> To me, it's annoying to have the cursor jump back to column 0 when
>> scrolling down through the function. I think that is the only situation
>> where I get annoyed with my emacs settings of deleting trailing whitespace.
>
> Well, then make emacs(?) not do that. ;)

Patches to my .emacs are more than welcome! :)

Jason

[Keshav Kini]

Hmm. I have to say I'm surprised that people are advocating for using trailing whitespace at all, as it seemed to me to be quite universally reviled in the programming world. But on some reflection, the only real problem with trailing whitespace - other than that it bloats files and that it, er, grates on certain types of minds :) - is that people remove it from, or, though I imagine this is rare, add it to otherwise untouched lines. I guess if hypothetically we had stuck to a "no trailing whitespace policy" from day 1 of Sage development, such a policy would have minimized diff cruft. But Sage's codebase is already so big that perhaps the best policy is just "don't purposelessly add or remove whitespace to lines that you aren't otherwise editing". As far as that goes, Jason's .emacs snippet seems useful for the developer's guide, indeed.

[Dan Drake]

On Mon, 24 Oct 2011 at 07:54AM -0700, Keshav Kini wrote:
> Shouldn't we have mention trailing whitespace somewhere in the developer's
> guide - namely telling people to omit it? Having such a policy makes for
> cleaner revision control, as most agree (for example, at some point git by
> default didn't let you commit changes which include adding trailing
> whitespace to files). There are way too many patches on trac with chunks
> that just remove trailing whitespace introduced in other patches, IMHO. If
> we take some stance on it (much like our official endorsement of PEP 8),
> this will be reduced. If people agree I'll make a ticket to add a couple
> lines to the developer's guide.

Based on the no-tabs-in-Sage-code experience, the only way to have any
kind of actual effect is to add a tool to the merge or doctest scripts
so that new trailing whitespace causes an error.

(Working on Sage has taught me that only automated tools work; asking
people to remember something or behave a certain way always fails. :)

It seems like no one wants to have an epic break-all-patches-in-trac
patch that removes all the trailing whitespace in the Sage library --
and, some people like some trailing whitespace, so perhaps we should
also add just a warning to the sage-merge script (or whatever release
managers use to add patches)?

Dan

--
--- Dan Drake
----- http://mathsci.kaist.ac.kr/~drake
-------

[kcrisman]

> > whitespace to files). There are way too many patches on trac with chunks
> > that just remove trailing whitespace introduced in other patches, IMHO. If
> > we take some stance on it (much like our official endorsement of PEP 8),
> > this will be reduced. If people agree I'll make a ticket to add a couple
> > lines to the developer's guide.
>
> Based on the no-tabs-in-Sage-code experience, the only way to have any
> kind of actual effect is to add a tool to the merge or doctest scripts
> so that new trailing whitespace causes an error.
>
> (Working on Sage has taught me that only automated tools work; asking
> people to remember something or behave a certain way always fails. :)

Yup.

> It seems like no one wants to have an epic break-all-patches-in-trac
> patch that removes all the trailing whitespace in the Sage library --
> and, some people like some trailing whitespace, so perhaps we should
> also add just a warning to the sage-merge script (or whatever release
> managers use to add patches)?
>

I have no idea whether these things are even there in my editor - I
use XCode, pico/nano, and TextEdit. But I'm sure that many first-time
developers would use that at most, if not even more rudimentary tools.

- kcrisman

[Florent Hivert]

+1 to this policy ! WE are trying to do that in the sage-combinat project.

Systematically remove all trailing whitespaces creates a lot of conflicts in
the patch queue so I'm strongly against it.

Cheers,

Florent

[daly]

diff has options to deal with whitespace.
-b --ignore-space-change
-w --ignore-all-space

Tim Daly

[Jeroen Demeyer]

On 2011-10-25 02:33, Dan Drake wrote:
> It seems like no one wants to have an epic break-all-patches-in-trac
> patch that removes all the trailing whitespace in the Sage library --
> and, some people like some trailing whitespace, so perhaps we should
> also add just a warning to the sage-merge script (or whatever release
> managers use to add patches)?

The following would be easy for me to do: remove all *newly added*
trailing whitespace from patches (except on lines consisting only of
spaces). Basically, s/^\(+.*[^ ]\) *$/\1/ in the patches.

[Dan Drake]

On Tue, 25 Oct 2011 at 08:37AM +0200, Jeroen Demeyer wrote:
> The following would be easy for me to do: remove all *newly added*
> trailing whitespace from patches (except on lines consisting only of
> spaces). Basically, s/^\(+.*[^ ]\) *$/\1/ in the patches.

I think that would be a good idea, although if you're just running that
through sed, the exact patches applied would be different from the ones
on the trac server, which would cause some confusion.

[Jeroen Demeyer]

On 2011-10-25 09:51, Dan Drake wrote:
> I think that would be a good idea, although if you're just running that
> through sed, the exact patches applied would be different from the ones
> on the trac server, which would cause some confusion.

Very true, but I think that is only a minor annoyance.

The point is that I don't like warnings. There used to be time when not
having a ticket number in the commit message created a warning in my
merger script. This meant I had to complain on a lot of tickets about
the commit message. Eventually, people were asking, "why doesn't your
script automatically add the ticket number?". Since then, that's how
things are done in the merger script. No warnings, just automatically
(and silently) make the change.

Jeroen.

[Florent Hivert]

On Tue, Oct 25, 2011 at 01:53:31AM -0400, daly wrote:
> diff has options to deal with whitespace.
> -b --ignore-space-change
> -w --ignore-all-space

I'm well aware of that... Mercurial has some extension as well... Or else, I
can rewrite a script to do it... The question is:

- is there a simple and robust way to *transparently* do that for all the 50
contributors of the sage-combinat project, while keeping the compatibility of
the queue with the two/three past versions of sage ???

Each time there has been such a big change within sage, the effect has been
that:
1) the coordinators of the sage-combinat project (ie Nicolas and myself) lost
two days of work, rebasing the queue and ensuring compatibility with two or
three version of sage and solving various contributors problems;
2) each contributors lost upto several hours;
3) but most important to us: as a consequence we lost some contributors.

Nicolas is on vacation now but I'm pretty sure he will agree with me.

Of course any *working* solution is mostly welcome !!!

Cheers,

Florent

[Ivan Andrus]

If you're using Emacs 24 you can install quarter-plane.el through the GNU ELPA. This uses picture mode to automatically insert spaces so that you can move anywhere in the full quarter plane.

I tried it and I don't like it, but YMMV. One problem is that it pollutes the undo space, but I can't believe you can't work around that somehow by fiddling with `buffer-undo-list'.

Looking at quarter-plane mode, it seems like the magic that you want could probably be done by rebinding your movement keys to `picture-move-up' and `picture-move-down' or writing similar functions.

Personally, I detest trailing whitespace—I can't abide hitting C-e and it not going where I expect. It also doesn't bother me to have the cursor jump back to column 0, but it might help that I have the whole line highlighted.

-Ivan

[leif]

On 25 Okt., 12:04, Jeroen Demeyer <jdeme...@cage.ugent.be> wrote:
> On 2011-10-25 09:51, Dan Drake wrote:
> > I think that would be a good idea, although if you're just running that
> > through sed, the exact patches applied would be different from the ones
> > on the trac server, which would cause some confusion.
>
> Very true, but I think that is only a minor annoyance.

I wouldn't say it's minor, since all developers basing their work on
these patches on trac still have the problem until a ticket gets
merged.

So checking for added trailing whitespace in patches uploaded to trac
would IMHO be a task for the patchbot.

> The point is that I don't like warnings.  There used to be time when not
> having a ticket number in the commit message created a warning in my
> merger script.  This meant I had to complain on a lot of tickets about
> the commit message.

Me neither. Of course it would be better if the complaints were
generated automagically...
Reasonable commit messages (and btw. also patch filenames and
attachment comments) are valuable *before* a ticket gets merged as
well; the latter two are more or less irrelevant after a ticket has
been merged, since they don't appear in a release, or as soon as
you've imported a patch. (The filenames of course appear in your
Mercurial patch queues and shell history, so *are* pretty relevant
during development.)

> Eventually, people were asking, "why doesn't your
> script automatically add the ticket number?".  Since then, that's how
> things are done in the merger script.  No warnings, just automatically
> (and silently) make the change.

Well, as far as I know invalid commit messages (or more precisely ones
lacking a ticket number) never created merge conflicts.

Also, if a developer doesn't get warnings (and nobody else complains),
he/she's unlikely to change his/her practice, so they're of
educational value as well. ;-)


-leif

[William Stein]

On Tue, Oct 25, 2011 at 11:06 AM, leif <not.r...@online.de> wrote:
> On 25 Okt., 12:04, Jeroen Demeyer <jdeme...@cage.ugent.be> wrote:
>> On 2011-10-25 09:51, Dan Drake wrote:
>> > I think that would be a good idea, although if you're just running that
>> > through sed, the exact patches applied would be different from the ones
>> > on the trac server, which would cause some confusion.
>>
>> Very true, but I think that is only a minor annoyance.
>
> I wouldn't say it's minor, since all developers basing their work on
> these patches on trac still have the problem until a ticket gets
> merged.
>
> So checking for added trailing whitespace in patches uploaded to trac
> would IMHO be a task for the patchbot.

Even better, it could be done by a trac pluging itself (at the moment
when it matters most), not the patchbot, which may or may not run
later.

>
>
>> The point is that I don't like warnings.  There used to be time when not
>> having a ticket number in the commit message created a warning in my
>> merger script.  This meant I had to complain on a lot of tickets about
>> the commit message.
>
> Me neither.  Of course it would be better if the complaints were
> generated automagically...
> Reasonable commit messages (and btw. also patch filenames and
> attachment comments) are valuable *before* a ticket gets merged as
> well; the latter two are more or less irrelevant after a ticket has
> been merged, since they don't appear in a release, or as soon as
> you've imported a patch. (The filenames of course appear in your
> Mercurial patch queues and shell history, so *are* pretty relevant
> during development.)

For me having ticket numbers (and comments) in a patch are
*incredibly* important *after* a ticket has been merged. I would say
they are way more important after than before merge. Typically,
when I see some code that is suspect in Sage, I use "hg blame" to see
what patch last modified that code, I look at the commit message of
the patch to see the ticket number, then look at the relevant page on
trac.

>> Eventually, people were asking, "why doesn't your
>> script automatically add the ticket number?".  Since then, that's how
>> things are done in the merger script.  No warnings, just automatically
>> (and silently) make the change.
>
> Well, as far as I know invalid commit messages (or more precisely ones
> lacking a ticket number) never created merge conflicts.

But they make future development work on Sage (especially for me) much harder.
Automatically adding them on merge is a great solution.

> Also, if a developer doesn't get warnings (and nobody else complains),
> he/she's unlikely to  change his/her practice, so they're of
> educational value as well. ;-)
>
>
> -leif
>

[leif]

On 25 Okt., 20:44, William Stein <wst...@gmail.com> wrote:

> On Tue, Oct 25, 2011 at 11:06 AM, leif <not.rea...@online.de> wrote:
> > So checking for added trailing whitespace in patches uploaded to trac
> > would IMHO be a task for the patchbot.
>
> Even better, it could be done by a trac pluging itself (at the moment
> when it matters most), not the patchbot, which may or may not run
> later.

Of course. Who implements such?

I can imagine various desirable checks on uploaded files (and
attachment comments), especially patches (where one would probably
also have to differentiate on the type or language of the patched
file)... :P

We haven't even upgraded trac, which was discussed weeks ago (and IIRC
nobody objected).

> > Reasonable commit messages (and btw. also patch filenames and
> > attachment comments) are valuable *before* a ticket gets merged as
> > well; the latter two are more or less irrelevant after a ticket has
> > been merged, since they don't appear in a release, or as soon as
> > you've imported a patch. (The filenames of course appear in your
> > Mercurial patch queues and shell history, so *are* pretty relevant
> > during development.)
>
> For me having ticket numbers (and comments) in a patch are
> *incredibly* important *after* a ticket has been merged.  I would say
> they are way more important after than before merge.       Typically,
> when I see some code that is suspect in Sage, I use "hg blame" to see
> what patch last modified that code, I look at the commit message of
> the patch to see the ticket number, then look at the relevant page on
> trac.

Well, I didn't say they aren't as well *after* a ticket has been
merged. But with our current development process, the "lifetime" of
patches on trac is rather months than days, and of course reviewers
and other people participating in a ticket [only] benefit from
everything that's *already* done *before* a ticket gets merged.

Also, I wanted to point out that potential merge conflicts are
certainly more important than missing ticket numbers in commit
messages of *not-yet-merged* patches. "Cleaning up" patches upon
merging them into a release is IMHO simply too late, which doesn't
mean that (additional) checks at that point are useless; in a perfect
world they would just be redundant (with negligible overhead), so they
can and should be performed regardless.

[A different issue is adding ticket numbers to commit messages that
already contain them.]

> > Well, as far as I know invalid commit messages (or more precisely ones
> > lacking a ticket number) never created merge conflicts.
>
> But they make future development work on Sage (especially for me) much harder.
> Automatically adding them on merge is a great solution.

See above. It would IMHO be better to check that (or add them)
earlier, and not add them unconditionally. Similar applies to spkg
version tags (and uncommitted changes in spkgs on trac).

Automatically adding ticket numbers (and doing other "corrections") in
the merge script (btw. without feedback to the author(s) /
developer(s)) is currently also an excuse to omitting them, so as a
side effect promotes bad practice.


-leif

[john_perry_usm]

On Oct 24, 10:33 am, Jason Grout <jason-s...@creativetrax.com> wrote:
> Here is one of the big situations when I like trailing spaces:
>
> def hello():$
>      print 'hi'$
>      $
>      print 'bye'$

+1

Quite a few text editors do this by default. It's enormously
convenient to the programmer; it's terribly inconvenient to go through
and right-strip these otherwise empty lines.

In addition, I question whether its contribution to "file bloat" is
either as serious as asserted elsewhere, or even all that undesirable.
After all, comments and empty lines contribute to file bloat, but they
also contribute to readability.

Trailing white space on a non-empty line is another story.

regards
john perry

[Keshav Kini]

FWIW, I'm trying to write a ticket listener trac plugin right now, for other reasons. I could add some functionality to make it check, or even fix, patch attachments as well. I can't promise it will be done very soon though, since I just started it the other day :) I'm testing it on trac 0.12 locally, so it would be nice if we could upgrade eventually.

-Keshav

----
Join #sagemath on irc.freenode.net !

[leif]

On 26 Okt., 03:17, Keshav Kini <keshav.k...@gmail.com> wrote:
> FWIW, I'm trying to write a ticket listener trac plugin right now, for
> other reasons. I could add some functionality to make it check, or even
> fix, patch attachments as well. I can't promise it will be done very soon
> though, since I just started it the other day :)

Go ahead... :P

Although I think checking attachments *before* they get attached is
quite different. Another, perhaps simpler in the first place approach
would be to mail warnings / complaints to the one who attached a
file... :-)

> I'm testing it on trac
> 0.12 locally, so it would be nice if we could upgrade eventually.

+1 (ceterum censeo)

(I was actually thinking of the same, i.e. "cloning" the Sage trac
installation locally for playing with it. I have no idea yet what
configuration files I'd have to copy though.)


-leif

[Keshav Kini]

> Although I think checking attachments *before* they get attached is
quite different.

No, that's not what I meant. The listener bot would notice that a patch had been posted, fix it, and then overwrite it by posting to the trac ticket like a normal user, posting a comment with a diff on the patch to show what it changed. Trac doesn't save overwritten attachments, but the main purpose of this listener would be to save attachments and comments on disk in a repo per ticket (to provide better revision history of progress on a ticket). Or, it could just post the diff without actually overwriting the patch, and just call it a recommendation.

-Keshav

[leif]

On 25 Okt., 23:49, john_perry_usm <john.pe...@usm.edu> wrote:
> On Oct 24, 10:33 am, Jason Grout <jason-s...@creativetrax.com> wrote:
>
> > Here is one of the big situations when I like trailing spaces:
>
> > def hello():$
> >      print 'hi'$
> >      $
> >      print 'bye'$
>
> +1
>
> Quite a few text editors do this by default. It's enormously
> convenient to the programmer; it's terribly inconvenient to go through
> and right-strip these otherwise empty lines.

Well, this could be done automagically, and your editor would
certainly keep auto-indenting correctly if you happen to edit such a
"stripped" file afterwards.

> After all, comments and empty lines contribute to file bloat, but they
> also contribute to readability.

Trailing blanks in contrast certainly don't, just like spaces on
otherwise empty lines :-) (perhaps unless your editor shows them --
I've configured mine to do so -- and they're consequently used to
indicate that you're still inside the same "scope", e.g. a class
definition).

> Trailing white space on a non-empty line is another story.

I think it would be ok to once remove all trailing whitespace from
*non-empty* lines of all Sage library files (preferably right before a
release is made); "rebasing" patches which due to that do no longer
apply is pretty trivial, i.e., can be done automatically.

Note that doing so (with a simple sed script) could in principle break
some multi-line strings (if the trailing whitespace was relevant), but
I strongly doubt that this is an issue in Sage.


-leif

[leif]

On 26 Okt., 04:30, Keshav Kini <keshav.k...@gmail.com> wrote:
> > Although I think checking attachments *before* they get attached is
> > quite different.
>
> No, that's not what I meant. The listener bot would notice that a patch had
> been posted, fix it, and then overwrite it by posting to the trac ticket
> like a normal user, posting a comment with a diff on the patch to show what
> it changed.

In realtime? That would give a nice opportunity for funny race
conditions... ;-)

I'd really like an unfriendly, annoying "Your attachment has been
rejected - read the Sage Developer's Guide and/or the Trac Guidelines
for possible reasons." XD

> Trac doesn't save overwritten attachments, but the main purpose
> of this listener would be to save attachments and comments on disk in a
> repo per ticket (to provide better revision history of progress on a
> ticket).

Nice, but how much disk space would that eat up?

While you're at it, auto-comments (with some delay, only if nothing
else happens on the ticket) for newly attached files would also be
nice, since otherwise one doesn't get a notification, unless the user
in addition adds a comment or updates the description. Although maybe
that's [meanwhile] configurable. [Still wonder when trac will support
concurrent posting.]

> Or, it could just post the diff without actually overwriting the
> patch, and just call it a recommendation.

I'd prefer an ante-mortem solution, which would also avoid unnecessary
noise on the ticket. (Mailing the uploader would as mentioned be an
alternative, but IMHO less optimal.)


-leif

[Keshav Kini]

Yes, of course there would be some delay. The repo could be disposed of if the ticket was merged into a full release, so disk usage would not build up too much. And keep in mind that if we were doing normal commits and pull-requests etc. instead of developing polished patches on trac and applying them with the merger script, all this would be occurring in the main sage trunk anyway :) Of course, I haven't thought this through very carefully, but let's discuss it on IRC instead as this is getting off topic :)

[Florent Hivert]

> > Trailing white space on a non-empty line is another story.
>
> I think it would be ok to once remove all trailing whitespace from
> *non-empty* lines of all Sage library files (preferably right before a
> release is made); "rebasing" patches which due to that do no longer
> apply is pretty trivial, i.e., can be done automatically.

No ! It is not ! It is a non trivial and painful task for the sage-combinat
community. Please remember that

- we have a queue of approximatively 250 patches !
- we have approximatively 40/50 contributors, not all of them are using the
last version of sage, only a few of them are mercurial advanced users.

So please tell me how trivially solve the following problem:

We want a robust way to *transparently* do remove whitespace in code and
patches for all the 50 contributors of the sage-combinat project, while

keeping the compatibility of the queue with the two/three past versions of
sage ???

As I already said, each time you did such a change:

1) the coordinators of the sage-combinat project (ie Nicolas and myself) lost
two days of work, rebasing the queue and ensuring compatibility with two or
three version of sage and solving various contributors problems;
2) each contributors lost upto several hours;
3) but most important to us: as a consequence we lost some contributors.

Of course any *working* solution is mostly welcome !!!

Cheers,

Florent

[Keshav Kini]

Since we would know where those lines are, we could search for the rest of the line (i.e. the part that's not the trailing spaces) in your patch and replace it with a line missing the whitespace. Voila, it is rebased, and the patch now applies to the new, whitespace-stripped file. If the line that was changed does not occur anywhere in your patch, there's no problem. Rebasing on top of whitespace stripping is much easier because 1) no lines are moved; 2) no lines are deleted; 3) no lines are added; 4) lines do not significantly change (i.e. it is easy to find the before state given the after state or vice versa).

-Keshav

[Florent Hivert]

On Tue, Oct 25, 2011 at 11:42:05PM -0700, Keshav Kini wrote:
> Since we would know where those lines are, we could search for the rest of
> the line (i.e. the part that's not the trailing spaces) in your patch and
> replace it with a line missing the whitespace. Voila, it is rebased, and
> the patch now applies to the new, whitespace-stripped file.

Please re-read carefully the statement of the problem ! How do you ensure
compatibility with the *former* version of sage (the one where the space in
the source files are still there) ?

Cheers,

Florent

[Keshav Kini]

Ah, I'm sorry, I misunderstood your question. Yes, that is a problem. I see no way around it in general. The only way to mitigate this is to try to avoid changing files that sage-combinat is working on. But I doubt that Sage developers want to commit to such a promise, unless sage-combinat wants to take ownership of parts of Sage... and certainly your objection applies to any broad changes to the Sage codebase. But what leif (I think) and I were trying to say is that at least this change he mentioned would be a lot easier to handle than other large-scale changes (such as, say, python3 migration changes which will eventually happen).

If I understand correctly, sage-combinat is an project which develops extensions of Sage which periodically (when sage-combinat feels a particular ticket is ready for review?) gets included into Sage itself. The standard way to do such a thing with Mercurial is to maintain a separate clone of the Sage repository (or a separate branch within the Sage repository), and merge changes from mainstream Sage into that repo every time Sage releases a new version. This way you don't need to muck about with rebasing multiple patch files etc., and pulling from the sage-combinat repo back into the mainstream Sage repo is relatively painless. In particular, I've never heard of anyone long-term sharing a patch queue repository and using it for development - that sounds really painful to me! (pre-posting edit: People in the IRC channel #mercurial on freenode said that they have heard of this workstyle but find it "quite painful" too...)

However, sadly, Sage does not really properly use Mercurial in a nice way, i.e. nobody touches our repository except the release manager, and there are scripts to handle all of that, which only expect to be merging patches from trac tickets and not doing anything else. So it may be a bit difficult to implement a nicer way for sage-combinat to collaborate with Sage, at least at the moment...

(As you may know, I am a very junior Sage developer so take what I say with a grain of salt, please :) )

[Florent Hivert]

Hi Keshav,

My last answer was a little rude or at least very short. Since Nicolas is on
vacation I'm the only sage-combinat coordinator keeping an eye here this week.
Anyway, we are both currently completely buried under the teaching load (eg:
for me 15 hours new lectures since Monday). So please don't feel offended by
this short answer. I had the impression that you where ready to jump on the
script and that we Nicolas and myself will suffer shortly...

On Wed, Oct 26, 2011 at 02:07:54AM -0700, Keshav Kini wrote:
> Ah, I'm sorry, I misunderstood your question. Yes, that is a problem. I see
> no way around it in general. The only way to mitigate this is to try to
> avoid changing files that sage-combinat is working on.

That's precisely what I was more or less asking... At least, please don't do
it without having a green light from our side.

> But I doubt that Sage developers want to commit to such a promise, unless
> sage-combinat wants to take ownership of parts of Sage... and certainly your
> objection applies to any broad changes to the Sage codebase. But what leif
> (I think) and I were trying to say is that at least this change he mentioned
> would be a lot easier to handle than other large-scale changes (such as,
> say, python3 migration changes which will eventually happen).

If you want some example of what has been painful in the past, here they
are. Note: I don't intend any offense against the responsible of those
changes, I'm just asking for coordination; Trivial change for you are not
trivial at all for us:

- Automatic replacement of
EXAMPLES::$
sage:
by
EXAMPLES::$
$
sage:
- tab replaced by spaces;
- bunch of typos corrections.

> If I understand correctly, sage-combinat is an project which develops
> extensions of Sage which periodically (when sage-combinat feels a
> particular ticket is ready for review?) gets included into Sage itself.

You understand correctly with "periodically" meaning when we feel that things
are stable/tested enough. So this is some kind of coordinated development and
pre-review process. Low level changes (eg: in the category system) are shaken
and tested before inclusion. Being able to do that in a relatively large scale
is quite efficient. Moreover we often need those code for research...

> The standard way to do such a thing with Mercurial is to maintain a separate
> clone of the Sage repository (or a separate branch within the Sage
> repository), and merge changes from mainstream Sage into that repo every
> time Sage releases a new version. This way you don't need to muck about with
> rebasing multiple patch files etc., and pulling from the sage-combinat repo
> back into the mainstream Sage repo is relatively painless. In particular,
> I've never heard of anyone long-term sharing a patch queue repository and
> using it for development - that sounds really painful to me!

This is going definitely off topic...

First of all, I'm not a revision control expert ! Maybe there is a better
solution. We didn't saw it with Nicolas after *quite* some thinking. If a
better working and robust solution come up, we will more than gladly adopt it.
But we won't redo this thinking again, unless we become completely stuck.

We used (with CVS) this kind of workflow when working with MuPAD. I wasn't
satisfactory. The problem is that when you are working in parallel on a stable
and a more experimental modifications of the same files, it is hard to unfold
the change sets to submit it after. More precisely, suppose that on a single
file I'm doing in sequence four modifications 1 2 3 4, I want to submit
modification 1 and 3 to trac while keeping 2 and 4 for a forthcoming
ticket. Of course change 2 and 3 must commute. With mercurial queue, this is
just moving line in the series file. With branches, you will end up with a
sequence of changeset. You need to extract those you want at the end. In my
experience and understanding, you have to follow a very strong discipline to
annotate every changeset committed with the information needed to select it at
the end. With patches, you can do it along the way commuting and folding
patches.

Anyway, the fact is that

- our development workflow more or less work (except during those big
changes);

- actually I feel that it work too well, so that we are abusing it ;-) They
are plenty of patch in the queue which are 1 or 2 hours work away from
being merged into sage and stay here for several months.

As I said, maybe we did it wrong, but it took quite some thinking to setup
properly the workflow and to write the tools (the "sage -combinat" script). I
don't see it changing unless an expert propose us streamlined solution. We
spend already quite a time explaining the workflow to contributors. I don't
think we will do any change to it, unless we are sure that the new workflow is
much easier and robust than the previous one.

> (pre-posting edit: People in the IRC channel #mercurial on freenode said
> that they have heard of this workstyle but find it "quite painful" too...)
>
> However, sadly, Sage does not really properly use Mercurial in a nice way,
> i.e. nobody touches our repository except the release manager, and there
> are scripts to handle all of that, which only expect to be merging patches
> from trac tickets and not doing anything else. So it may be a bit difficult
> to implement a nicer way for sage-combinat to collaborate with Sage, at
> least at the moment...
>
> (As you may know, I am a very junior Sage developer so take what I say with
> a grain of salt, please :) )

Don't worry ! I'm not a revision control expert either. And thanks for your
suggestions.

Cheers,

Florent

PS: there are plenty of whitespaces at the end of the lines of your message ;-)

[William Stein]

Hi,

[whitespace]

What do other major open source projects (e.g., the Linux kernel) do
regarding whitespace and patches? What are their policies?

William

[Keshav Kini]

Well, the kernel devs try to remove trailing whitespace where it is found (and while in the process of doing something more meaningful) [1] and avoid introducing it in the first place at all [2] but do not create patchbombs by going out of their way to do so. I mean, this just makes sense anyway.

As far as I know, the general attitude among developers is that trailing whitespace is always bad (a Google search for "trailing whitespace" will return rants about it by the dozens as well as plenty of tips on how to get rid of it using various editors, VCS tools, etc.), but removing existing trailing whitespace is not worth creating extraneous chunks in patches/diffs. And I agree with that, just to make it clear to for example Florent above :) As I mentioned in the OP, git used to by default refuse to let you commit changes that introduced new trailing space, but of course didn't complain when you left alone trailing space that was already there. We could probably do a similar thing by hooking into trac and not allowing uploads of patches that introduce trailing whitespace.

But this may not be agreeable to Jason or others who like to put trailing space on otherwise empty lines. Would it be possible to configure emacs to pretend there is space there, or something like that? I imagine it should be easy to add something to ~/.emacs which upon loading a .py or .pyx file searches for blocks of empty lines, checks previous and following lines' indentation, and if it's the same both above and below, apply the same indentation to all the empty lines in between. Then another hook would, upon save-to-disk, just delete all trailing whitespace again. This would transparently make the cursor movement nicer within the open buffer for people who like it that way. Jason, what do you think? (I CC'd Jason).

By the way, there is also a third-party Mercurial extension which bugs you when you try to commit (or qnew/qrefresh, I imagine) trailing whitespace. We could... ship this with the built-in Mercurial, for what it's worth, but I think the above solution (hooking into trac) is better.

Again, sorry for wasting your time with such a trivial matter. I didn't expect the thread to grow so huge! :)

-Keshav

[1] http://www.mail-archive.com/linux-kernel%40vger.kernel.org/msg208985.html
[2] http://www.kernel.org/doc/Documentation/CodingStyle
[3] http://mercurial.selenic.com/wiki/CheckFilesExtension

[Jason Grout]

Interesting solution. But please. The barrier and standards for
contributions are already high enough. Let's not insist on a
contributor using emacs (as much as I personally like it), or
configuring their editor for such an issue. Let's just view not having
spaces on nonempty lines as good style and leave it at that. And let's
not insist that people who spent a lot of their time to program a patch
be rejected, for example if their editor happened to put a space when it
automatically wrapped/filled a documentation line. I think if I was a
new contributor, with limited time, who had just spent a lot of time
learning python, mercurial, queues, trac, docstrings, rest, doctests,
and everything else we demand for a good patch, if I got rejected
because of an extra space at the end of a line, I may just throw my
hands up in the air and give up.

>
> By the way, there is also a third-party Mercurial extension which bugs
> you when you try to commit (or qnew/qrefresh, I imagine) trailing
> whitespace. We could... ship this with the built-in Mercurial, for what
> it's worth, but I think the above solution (hooking into trac) is better.
>
> Again, sorry for wasting your time with such a trivial matter. I didn't
> expect the thread to grow so huge! :)

I agree. As Florent pointed out, the maintenance burden would be huge.
Rejections for such an inconsequential bike-shedding issue that is
just a matter of taste are a huge turn-off for a new developer. The
benefits are practically nil, as I see it. I think the linux policy is
sane---if you happen to be editing a line and remove the trailing
whitespace, good for you. But let's leave it at that.

Sorry, I don't mean to be mean. I just think there are far more
important things that would help Sage, and I don't see any positives to
rejecting patches by people for such an issue.

Thanks,

Jason

[Keshav Kini]

>Let's just view not having
>spaces on nonempty lines as good style and leave it at that.

Perfect! This is exactly what I proposed in the OP, by the way :) Not sure how all these hammers appeared in my hand when the problem is pretty much already nailed down... I guess I indeed caught bikeshedding fever, haha. Sorry about that.

So, I will open a ticket for patching the developer's guide. Thanks for your guidance!

-Keshav

[Keshav Kini]

This is now #11956.

[john_perry_usm]

On Oct 25, 9:43 pm, leif <not.rea...@online.de> wrote:
> > After all, comments and empty lines contribute to file bloat, but they
> > also contribute to readability.
>
> Trailing blanks in contrast certainly don't, just like spaces on
> otherwise empty lines :-) (perhaps unless your editor shows them --
> I've configured mine to do so -- and they're consequently used to
> indicate that you're still inside the same "scope", e.g. a class
> definition).

I understand that; the point was more that trailing white space on
empty lines contributes to programmer convenience, inasmuch as he
doesn't have to think too much about it.

Out of curiosity, *how* does trailing white space usually get added to
non-empty lines? I've noticed it sometimes, and wondered. Do some
programmers just add it habitually?

regards
john perry

[leif]

On 27 Okt., 03:18, Jason Grout <jason-s...@creativetrax.com> wrote:
> As Florent pointed out, the maintenance burden would be huge.

Almost a one-liner for *both* fixing the whole Sage library as well as
all pending patches.

> Rejections for such an inconsequential bike-shedding issue that is
> just a matter of taste are a huge turn-off for a new developer.  The
> benefits are practically nil, as I see it.

It's regularly annoying to edit such lines. I don't care much about
increased file size.

> I think the linux policy is
> sane---if you happen to be editing a line and remove the trailing
> whitespace, good for you.  But let's leave it at that.

But you won't get rid of them if you don't stop people
(re-)introducing them. And it's similarly odd for reviewers to check
and perhaps fix such things again and again on tickets, producing
superfluous noise.

In general, if you want people to follow conventions, you should
remove all the grown counterexamples at some point, since people keep
copy-pasting and of course take present code as an orientation rather
than reading lengthy, perhaps boring guidelines.

> Sorry, I don't mean to be mean.  I just think there are far more
> important things that would help Sage, and I don't see any positives to
> rejecting patches by people for such an issue.

Of course. Although we reject *any* tabs (in Python/Cython files)
regardless of their position.


2ct,

-leif

[Simon King]

Hi Florent,

On 26 Okt., 17:58, Florent Hivert <Florent.Hiv...@lri.fr> wrote:
> If you want some example of what has been painful in the past, here they
> are. Note: I don't intend any offense against the responsible of those
> changes, I'm just asking for coordination; Trivial change for you are not
> trivial at all for us:
>
>  - Automatic replacement of
>      EXAMPLES::$
>         sage:
>    by
>      EXAMPLES::$
>      $
>         sage:

If I am not mistaken, the first version (without an empty or blank
line) results in a misformatted page in the reference manual. So,
those changes are neither trivial and nor just a matter of code style,
but they are bug fixes.

Best regards,
Simon

[kcrisman]

I won't repeat Jason's email above, but +1 to everything he said.

Correct.

[Ivan Andrus]

On Oct 27, 2011, at 2:38 AM, Keshav Kini wrote:
> Would it be possible to configure emacs to pretend there is space there, or something like that? I imagine it should be easy to add something to ~/.emacs which upon loading a .py or .pyx file searches for blocks of empty lines, checks previous and following lines' indentation, and if it's the same both above and below, apply the same indentation to all the empty lines in between. Then another hook would, upon save-to-disk, just delete all trailing whitespace again. This would transparently make the cursor movement nicer within the open buffer for people who like it that way. Jason, what do you think? (I CC'd Jason).

That's kind of a silly question. Of course it's possible, that's why it's emacs. ;-)

I was playing around with something like this yesterday. The following advice for next-line (and the same for previous-line) gets at least part of the way there, but I found I don't really like it personally.

(defadvice next-line (around empty-line-fix
(&optional arg try-vscroll)
activate)
(let ((goal (or goal-column
(if (< (current-column) (current-indentation))
(current-column)
(current-indentation)))))

;; Delete trailing whitespace from the current line before moving
(delete-trailing-whitespace (line-beginning-position)
(line-end-position))

;; Move the lines
ad-do-it

;; Insert spaces as needed
(when (and (> (- goal (current-column)) 0)
(not (string-match "\\S "
(buffer-substring-no-properties
(line-beginning-position)
(line-end-position)))))
(insert-char ?\ (- goal (current-column))))))

-Ivan

[Robert Bradshaw]

On Tue, Oct 25, 2011 at 11:44 AM, William Stein <wst...@gmail.com> wrote:
> On Tue, Oct 25, 2011 at 11:06 AM, leif <not.r...@online.de> wrote:
>> On 25 Okt., 12:04, Jeroen Demeyer <jdeme...@cage.ugent.be> wrote:
>>> On 2011-10-25 09:51, Dan Drake wrote:
>>> > I think that would be a good idea, although if you're just running that
>>> > through sed, the exact patches applied would be different from the ones
>>> > on the trac server, which would cause some confusion.
>>>
>>> Very true, but I think that is only a minor annoyance.
>>
>> I wouldn't say it's minor, since all developers basing their work on
>> these patches on trac still have the problem until a ticket gets
>> merged.
>>
>> So checking for added trailing whitespace in patches uploaded to trac
>> would IMHO be a task for the patchbot.
>
> Even better, it could be done by a trac pluging itself (at the moment
> when it matters most), not the patchbot, which may or may not run
> later.

+1

Ideally, when you upload a patch it would do a bunch of checks and
present to you a "fixed" file with the option of downloading it or
replacing the original file right there.

>>> The point is that I don't like warnings.  There used to be time when not
>>> having a ticket number in the commit message created a warning in my
>>> merger script.  This meant I had to complain on a lot of tickets about
>>> the commit message.
>>
>> Me neither.  Of course it would be better if the complaints were
>> generated automagically...
>> Reasonable commit messages (and btw. also patch filenames and
>> attachment comments) are valuable *before* a ticket gets merged as
>> well; the latter two are more or less irrelevant after a ticket has
>> been merged, since they don't appear in a release, or as soon as
>> you've imported a patch. (The filenames of course appear in your
>> Mercurial patch queues and shell history, so *are* pretty relevant
>> during development.)
>
> For me having ticket numbers (and comments) in a patch are
> *incredibly* important *after* a ticket has been merged.  I would say
> they are way more important after than before merge.       Typically,
> when I see some code that is suspect in Sage, I use "hg blame" to see
> what patch last modified that code, I look at the commit message of
> the patch to see the ticket number, then look at the relevant page on
> trac.

My thoughts exactly.

>>> Eventually, people were asking, "why doesn't your
>>> script automatically add the ticket number?".  Since then, that's how
>>> things are done in the merger script.  No warnings, just automatically
>>> (and silently) make the change.
>>
>> Well, as far as I know invalid commit messages (or more precisely ones
>> lacking a ticket number) never created merge conflicts.
>
> But they make future development work on Sage (especially for me) much harder.
> Automatically adding them on merge is a great solution.

Yep.

As far as whitespace, the difficulty here is merge conflicts with
upstream. Right now the sage-combinat queue is the major player here;
what if we stripped trailing whitespace in the merger script for
everything outside the combinat directory (and also ship the
whitespace-intelligent hg plugin with Sage as a convenience).

If we can't come up with a solution that's low-maintenance on
developers (including the sage-combinat folks), release managers, and
contributors (read, no rejecting patches due to trailing whitespace)
then it's probably better to just leave it as a non-enforced
suggestion.

- Robert

[Robert Bradshaw]

On Thu, Oct 27, 2011 at 3:25 PM, Robert Bradshaw
<robe...@math.washington.edu> wrote:
> On Tue, Oct 25, 2011 at 11:44 AM, William Stein <wst...@gmail.com> wrote:
>> On Tue, Oct 25, 2011 at 11:06 AM, leif <not.r...@online.de> wrote:
>>> On 25 Okt., 12:04, Jeroen Demeyer <jdeme...@cage.ugent.be> wrote:
>>>> On 2011-10-25 09:51, Dan Drake wrote:
>>>> > I think that would be a good idea, although if you're just running that
>>>> > through sed, the exact patches applied would be different from the ones
>>>> > on the trac server, which would cause some confusion.
>>>>
>>>> Very true, but I think that is only a minor annoyance.
>>>
>>> I wouldn't say it's minor, since all developers basing their work on
>>> these patches on trac still have the problem until a ticket gets
>>> merged.
>>>
>>> So checking for added trailing whitespace in patches uploaded to trac
>>> would IMHO be a task for the patchbot.
>>
>> Even better, it could be done by a trac pluging itself (at the moment
>> when it matters most), not the patchbot, which may or may not run
>> later.
>
> +1
>
> Ideally, when you upload a patch it would do a bunch of checks and
> present to you a "fixed" file with the option of downloading it or
> replacing the original file right there.

An even better option would be a "submit patch to trac" command, which
could take as set of patches from your queue, clean them up,
optionally run a bunch of tests, and upload them (updating the
description to the current set and any dependencies).

- Robert

[leif]

On 28 Okt., 01:57, Robert Bradshaw <rober...@math.washington.edu>
wrote:

> On Thu, Oct 27, 2011 at 3:25 PM, Robert Bradshaw

> > Ideally, when you upload a patch it would do a bunch of checks and
> > present to you a "fixed" file with the option of downloading it or
> > replacing the original file right there.

Obviously. Any idea how hard it is to implement such?

[We should probably also reject binary attachments, and/or could even
*there* provide a way to upload e.g. spkgs to a different place, just
leaving a link to them on the ticket, maybe just like symbolic links
work.]

> An even better option would be a "submit patch to trac" command, which
> could take as set of patches from your queue, clean them up,
> optionally run a bunch of tests, and upload them (updating the
> description to the current set and any dependencies).

Same for this. [Properly updating the patches (in-place) and the
description might be difficult if different people attached [previous]
patches; you do not always have to update all, and not even all
relevant patches of the ticket need to be in the uploader's queue.]

I can imagine shell (or Python) scripts doing at least part of this.
As mentioned, removing added trailing whitespace from patches is a one-
liner. We could also automatically add (to) attachment comments for
which repo a patch is, and on which Sage version it is based (and/or
*per-patch* cross-ticket dependencies). More important than removing
useless whitespace are things the merger script currently checks, such
as "proper Mercurial patches" (without garbage, and including e.g. the
author). Not to mention commit messages only consisting of "[mq]
foo".

I don't know how many people use the hg_*() commands from within Sage,
but at least there one could already do some sanity checks.

Shipping some Mercurial extension(s) in addition wouldn't be bad
either of course.


-leif

[Keshav Kini]

Cleaning up a patch and giving it to the user sounds fine to me. But it's a different matter to create a sage command which automatically doctests and submits patches. That's going too far, IMO. It's nice to allow developers to stay away from the details as much as possible, which lowers the barrier to entry, and certainly an intimate understanding of version control is not really a necessary trait in a competent Sage developer, which I guess justifies stuff like the hg_*() commands.

But surely assuming a developer's ability to type `make ptestlong` on the command line and to upload a file to a website is not a particularly high hurdle? At some point the effort required to learn how to use Sage's easy wrapper around a procedure becomes about as much as the effort required to learn how to do the procedure directly, even though the former only enables you to do Sage development better, whereas the latter often enables you to do general development better. Personally I think the hg_*() commands are pretty close to that line, and automatic doctesting and uploading of patches go over it. But of course I could be wrong.

[Florent Hivert]

Sure ! I wasn't saying that we shouldn't have corrected those. I wasn't saying
either that we shouldn't correct typos. I just said that I remember loosing
quite a few hours ensuring by cutting a big patch boob chunk by chunk that the
combinat queue remain applicable with or without this correction.

Cheers,

Florent

[Jeroen Demeyer]

On 2011-10-25 08:37, Jeroen Demeyer wrote:
> The following would be easy for me to do: remove all *newly added*
> trailing whitespace from patches (except on lines consisting only of
> spaces). Basically, s/^\(+.*[^ ]\) *$/\1/ in the patches.

I just tried to do this for sage-4.8.alpha1 and the problem is reviewer
patches. If a ticket has a "main" patch adding new trailing whitespace
and a "reviewer" ticket changing those lines, then removing new trailing
whitespace from patches creates conflicts.

[Nils Bruin]

On Nov 6, 4:40 am, Jeroen Demeyer <jdeme...@cage.ugent.be> wrote:
> I just tried to do this for sage-4.8.alpha1 and the problem is reviewer
> patches.  If a ticket has a "main" patch adding new trailing whitespace
> and a "reviewer" ticket changing those lines, then removing new trailing
> whitespace from patches creates conflicts.

In that case one solution would be to generate a "whitespace
correction patch". This should be scriptable:
- start from baseline version (*)
- apply all patches of a ticket (**)
- find which lines have changed between (*) and (**)
- generate a patch that fixes whitespace problems on those changed
lines and apply on top.
I'm pretty sure hg can do the heavy lifting for this (i.e., finding
which lines etc.)

Of course the amount of work needed to implement this might not be
justified by the benefits it produces.
It would only touch lines that are changing anyway, so it should not
force rebasing of additional patches.

[leif]

Well, of course you'd have to

- first clean up the whole Sage library once (removing trailing
whitespace),

- then for every patch to the Sage library, remove trailing
whitespace from deleted (-), added (+) as well as context ( ) lines
(the latter perhaps less important, as it will mainly remove fuzz).


-leif

[leif]

On 6 Nov., 19:23, Nils Bruin <nbr...@sfu.ca> wrote:
> In that case one solution would be to generate a "whitespace
> correction patch". This should be scriptable:
>  - start from baseline version (*)
>  - apply all patches of a ticket (**)
>  - find which lines have changed between (*) and (**)
>  - generate a patch that fixes whitespace problems on those changed
> lines and apply on top.
> I'm pretty sure hg can do the heavy lifting for this (i.e., finding
> which lines etc.)

Well, you can easily create a single patch from all patches of a
ticket (to the Sage library), and remove newly introduced whitespace
from that.

But besides that you lose some commit messages (and probably also
authors and reviewers), unless you create some new, cumulative commit
message and meta data from all patches*, this doesn't help with "cross-
ticket reviewer patches".

I.e., you really have to modify previous / deleted (-) and context ( )
lines as well, as mentioned in my previous post.


-leif
____
* One could of course just use this single patch to detect *finally*
newly introduced whitespace, and create a whitespace-fixing patch from
that, to be applied on top of all other, unmodified patches of a
ticket. [But still simply rejecting patches with newly introduced
trailing whitespace is the easiest solution. :-) ]